-
Notifications
You must be signed in to change notification settings - Fork 24.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
* Improve performance for role mapping with DNs (#92074) DNs in a role mapping are parsed each time a matching is performed against a user group. If there are large number of role mappings and large number of user groups, this process can be quite slow since DN parsing is non-trivial. This PR improves the performance by introducing a cache for DN parsing so that an unique DN from all role mappings is only parsed once per user (regardless of how many groups the user has). The cache stores the normalized string format of the DN instead of the DN object itself since DN is rather memory expensive. The cache's lifecycle is tied to the UserData model which goes out of scope once role mapping process is completed. Hence it is short-lived and does not need to externally managed. Thanks to @tvernum for the original idea. Co-authored-by: Tim Vernum <tim.vernum@elastic.co> * fix backport omission Co-authored-by: Tim Vernum <tim.vernum@elastic.co>
- Loading branch information
Showing
4 changed files
with
281 additions
and
34 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 92074 | ||
| summary: Improve performance for role mapping with DNs | ||
| area: Authentication | ||
| type: bug | ||
| issues: [] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
177 changes: 177 additions & 0 deletions
177
...org/elasticsearch/xpack/core/security/authc/support/DistinguishedNameNormalizerTests.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.core.security.authc.support; | ||
|
|
||
| import org.elasticsearch.test.ESTestCase; | ||
| import org.elasticsearch.xpack.core.security.authc.RealmConfig; | ||
| import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.ExpressionModel; | ||
| import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.FieldExpression.FieldValue; | ||
| import org.junit.Before; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.function.Predicate; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.mockito.ArgumentMatchers.anyString; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.never; | ||
| import static org.mockito.Mockito.spy; | ||
| import static org.mockito.Mockito.times; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| public class DistinguishedNameNormalizerTests extends ESTestCase { | ||
|
|
||
| private UserRoleMapper.DistinguishedNameNormalizer dnNormalizer; | ||
|
|
||
| @Before | ||
| public void init() { | ||
| dnNormalizer = getDnNormalizer(); | ||
| } | ||
|
|
||
| public void testDnNormalizingIsCached() { | ||
| // Parse same DN multiple times, only 1st time DN parsing is performed, 2nd time reads from the cache | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String dn = randomDn(); | ||
| parseDnMultipleTimes(dn); | ||
| verify(dnNormalizer, times(1)).doNormalize(dn); | ||
|
|
||
| // The cache is keyed by the literal string form. | ||
| // Therefore if the literal string changes, it needs to be parsed again even though it is still the same DN | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String mutatedDn = mutateDn(dn); | ||
| parseDnMultipleTimes(mutatedDn); | ||
| verify(dnNormalizer, times(1)).doNormalize(mutatedDn); | ||
|
|
||
| // Invalid DNs should also be cached | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String invalidDn = randomFrom( | ||
| "", | ||
| randomAlphaOfLengthBetween(1, 8), | ||
| randomAlphaOfLengthBetween(1, 8) + "*", | ||
| randomAlphaOfLengthBetween(1, 8) + "=", | ||
| "=" + randomAlphaOfLengthBetween(1, 8) | ||
| ); | ||
| parseDnMultipleTimes(invalidDn); | ||
| verify(dnNormalizer, times(1)).doNormalize(invalidDn); | ||
| } | ||
|
|
||
| public void testDnNormalizingIsCachedForDnPredicate() { | ||
| final String dn = randomDn(); | ||
| final Predicate<FieldValue> predicate = new UserRoleMapper.DistinguishedNamePredicate(dn, dnNormalizer); | ||
| verify(dnNormalizer, times(1)).doNormalize(dn); | ||
|
|
||
| // Same DN, it's cached | ||
| runPredicateMultipleTimes(predicate, dn); | ||
| verify(dnNormalizer, times(1)).doNormalize(dn); | ||
|
|
||
| // Predicate short-circuits for case differences | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String casedDn = randomFrom(dn.toLowerCase(Locale.ENGLISH), dn.toUpperCase(Locale.ENGLISH)); | ||
| runPredicateMultipleTimes(predicate, casedDn); | ||
| verify(dnNormalizer, never()).doNormalize(anyString()); | ||
|
|
||
| // Literal string form changes, it will be parsed again | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String mutatedDn = randomFrom(dn.replace(" ", ""), dn.replace(",", " ,")); | ||
| runPredicateMultipleTimes(predicate, mutatedDn); | ||
| verify(dnNormalizer, times(1)).doNormalize(mutatedDn); | ||
|
|
||
| // Subtree DN is also cached | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String subtreeDn = "*," + randomDn(); | ||
| runPredicateMultipleTimes(predicate, subtreeDn); | ||
| verify(dnNormalizer, times(1)).doNormalize(subtreeDn.substring(2)); | ||
|
|
||
| // Subtree DN is also keyed by the literal form, so they are space sensitive | ||
| Mockito.clearInvocations(dnNormalizer); | ||
| final String mutatedSubtreeDn = "*, " + subtreeDn.substring(2); | ||
| runPredicateMultipleTimes(predicate, mutatedSubtreeDn); | ||
| verify(dnNormalizer, times(1)).doNormalize(mutatedSubtreeDn.substring(2)); | ||
| } | ||
|
|
||
| public void testUserDataUsesCachedDnNormalizer() { | ||
| final String userDn = "uid=foo," + randomDn(); | ||
| final List<String> groups = IntStream.range(0, randomIntBetween(50, 100)) | ||
| .mapToObj(i -> "gid=g" + i + "," + randomDn()) | ||
| .distinct() | ||
| .collect(Collectors.toList()); | ||
| final RealmConfig realmConfig = mock(RealmConfig.class); | ||
| when(realmConfig.name()).thenReturn(randomAlphaOfLengthBetween(3, 8)); | ||
| final UserRoleMapper.UserData userData = new UserRoleMapper.UserData( | ||
| randomAlphaOfLengthBetween(5, 8), | ||
| userDn, | ||
| groups, | ||
| Collections.emptyMap(), | ||
| realmConfig | ||
| ); | ||
| UserRoleMapper.UserData spyUserdata = spy(userData); | ||
| final UserRoleMapper.DistinguishedNameNormalizer spyDnNormalizer = spy(userData.getDnNormalizer()); | ||
| when(spyUserdata.getDnNormalizer()).thenReturn(spyDnNormalizer); | ||
|
|
||
| final ExpressionModel expressionModel = spyUserdata.asModel(); | ||
|
|
||
| // All DNs to be tested should only be parsed once no matter how many groups the userData may have | ||
| Mockito.clearInvocations(spyDnNormalizer); | ||
| final List<String> dnList = randomList(100, 200, DistinguishedNameNormalizerTests::randomDn).stream() | ||
| .distinct() | ||
| .collect(Collectors.toList()); | ||
| final List<FieldValue> fieldValues = dnList.stream() | ||
| .map(dn -> randomBoolean() ? new FieldValue(dn) : new FieldValue("*," + dn)) | ||
| .collect(Collectors.toList()); | ||
| expressionModel.test("groups", fieldValues); | ||
| // Also does not matter how many times the model is tested | ||
| expressionModel.test("groups", randomNonEmptySubsetOf(fieldValues)); | ||
|
|
||
| final ArgumentCaptor<String> argumentCaptor = ArgumentCaptor.forClass(String.class); | ||
| verify(spyDnNormalizer, times(dnList.size())).doNormalize(argumentCaptor.capture()); | ||
| assertThat(argumentCaptor.getAllValues(), equalTo(dnList)); | ||
| } | ||
|
|
||
| private void parseDnMultipleTimes(String dn) { | ||
| IntStream.range(0, randomIntBetween(3, 5)).forEach(i -> dnNormalizer.normalize(dn)); | ||
| } | ||
|
|
||
| private void runPredicateMultipleTimes(Predicate<FieldValue> predicate, Object value) { | ||
| IntStream.range(0, randomIntBetween(3, 5)).forEach(i -> predicate.test(new FieldValue(value))); | ||
| } | ||
|
|
||
| private UserRoleMapper.DistinguishedNameNormalizer getDnNormalizer() { | ||
| return spy(new UserRoleMapper.DistinguishedNameNormalizer()); | ||
| } | ||
|
|
||
| private static String randomDn() { | ||
| return "CN=" | ||
| + randomAlphaOfLengthBetween(3, 12) | ||
| + ",OU=" | ||
| + randomAlphaOfLength(4) | ||
| + ", O=" | ||
| + randomAlphaOfLengthBetween(2, 6) | ||
| + ",dc=" | ||
| + randomAlphaOfLength(3); | ||
| } | ||
|
|
||
| private static String mutateDn(String dn) { | ||
| switch (randomIntBetween(1, 4)) { | ||
| case 1: | ||
| return dn.toLowerCase(Locale.ENGLISH); | ||
| case 2: | ||
| return dn.toUpperCase(Locale.ENGLISH); | ||
| case 3: | ||
| return dn.replace(" ", ""); | ||
| default: | ||
| return dn.replace(",", " ,"); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.