From 3c1fdc9fc0aa757cdaa48b1217eae18dbc9a1896 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Wed, 3 Oct 2018 09:30:57 -0600 Subject: [PATCH] Security: reduce memory usage of DnRoleMapper (#34250) The `DnRoleMapper` class is used to map distinguished names of groups and users to role names. This mapper builds in an internal map that maps from a `com.unboundid.ldap.sdk.DN` to a `Set`. In cases where a lot of distinct DNs are mapped to roles, this can consume quite a bit of memory. The majority of the memory is consumed by the DN object. For example, a 94 character DN that has 9 relative DNs (RDN) will retain 4KB of memory, whereas the String itself consumes less than 250 bytes. In order to reduce memory usage, we can map from a normalized DN string to a List of roles. The normalized string is actually how the DN class determines equality with another DN and we can drop the overhead of needing to keep all of the other objects in memory. Additionally the use of a List provides memory savings as each HashSet is backed by a HashMap, which consumes a great deal more memory than an appropriately sized ArrayList. The uniqueness we get from a Set is maintained by first building a set when parsing the file and then converting to a list upon completion. Closes #34237 --- .../security/authc/support/DnRoleMapper.java | 25 ++++++++++++------- .../authc/support/DnRoleMapperTests.java | 20 +++++++-------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java index 8a02977d55c..bdabc690f76 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java @@ -8,14 +8,17 @@ package org.elasticsearch.xpack.security.authc.support; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.stream.Collectors; import com.unboundid.ldap.sdk.DN; import com.unboundid.ldap.sdk.LDAPException; @@ -51,7 +54,7 @@ public class DnRoleMapper implements UserRoleMapper { private final Path file; private final boolean useUnmappedGroupsAsRoles; private final CopyOnWriteArrayList listeners = new CopyOnWriteArrayList<>(); - private volatile Map> dnRoles; + private volatile Map> dnRoles; public DnRoleMapper(RealmConfig config, ResourceWatcherService watcherService) { this.config = config; @@ -87,7 +90,7 @@ public class DnRoleMapper implements UserRoleMapper { * logging the error and skipping/removing all mappings. This is aligned with how we handle other auto-loaded files * in security. */ - public static Map> parseFileLenient(Path path, Logger logger, String realmType, String realmName) { + public static Map> parseFileLenient(Path path, Logger logger, String realmType, String realmName) { try { return parseFile(path, logger, realmType, realmName, false); } catch (Exception e) { @@ -98,7 +101,7 @@ public class DnRoleMapper implements UserRoleMapper { } } - public static Map> parseFile(Path path, Logger logger, String realmType, String realmName, boolean strict) { + public static Map> parseFile(Path path, Logger logger, String realmType, String realmName, boolean strict) { logger.trace("reading realm [{}/{}] role mappings file [{}]...", realmType, realmName, path.toAbsolutePath()); @@ -149,7 +152,10 @@ public class DnRoleMapper implements UserRoleMapper { logger.debug("[{}] role mappings found in file [{}] for realm [{}/{}]", dnToRoles.size(), path.toAbsolutePath(), realmType, realmName); - return unmodifiableMap(dnToRoles); + Map> normalizedMap = dnToRoles.entrySet().stream().collect(Collectors.toMap( + entry -> entry.getKey().toNormalizedString(), + entry -> Collections.unmodifiableList(new ArrayList<>(entry.getValue())))); + return unmodifiableMap(normalizedMap); } catch (IOException | SettingsException e) { throw new ElasticsearchException("could not read realm [" + realmType + "/" + realmName + "] role mappings file [" + path.toAbsolutePath() + "]", e); @@ -176,8 +182,9 @@ public class DnRoleMapper implements UserRoleMapper { Set roles = new HashSet<>(); for (String groupDnString : groupDns) { DN groupDn = dn(groupDnString); - if (dnRoles.containsKey(groupDn)) { - roles.addAll(dnRoles.get(groupDn)); + String normalizedGroupDn = groupDn.toNormalizedString(); + if (dnRoles.containsKey(normalizedGroupDn)) { + roles.addAll(dnRoles.get(normalizedGroupDn)); } else if (useUnmappedGroupsAsRoles) { roles.add(relativeName(groupDn)); } @@ -187,14 +194,14 @@ public class DnRoleMapper implements UserRoleMapper { groupDns, file.getFileName(), config.type(), config.name()); } - DN userDn = dn(userDnString); - Set rolesMappedToUserDn = dnRoles.get(userDn); + String normalizedUserDn = dn(userDnString).toNormalizedString(); + List rolesMappedToUserDn = dnRoles.get(normalizedUserDn); if (rolesMappedToUserDn != null) { roles.addAll(rolesMappedToUserDn); } if (logger.isDebugEnabled()) { logger.debug("the roles [{}], are mapped from the user [{}] using file [{}] for realm [{}/{}]", - (rolesMappedToUserDn == null) ? Collections.emptySet() : rolesMappedToUserDn, userDnString, file.getFileName(), + (rolesMappedToUserDn == null) ? Collections.emptySet() : rolesMappedToUserDn, normalizedUserDn, file.getFileName(), config.type(), config.name()); } return roles; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java index 263c5ee4929..83110c7a10a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java @@ -200,27 +200,27 @@ public class DnRoleMapperTests extends ESTestCase { public void testParseFile() throws Exception { Path file = getDataPath("role_mapping.yml"); Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null); - Map> mappings = DnRoleMapper.parseFile(file, logger, "_type", "_name", false); + Map> mappings = DnRoleMapper.parseFile(file, logger, "_type", "_name", false); assertThat(mappings, notNullValue()); assertThat(mappings.size(), is(3)); DN dn = new DN("cn=avengers,ou=marvel,o=superheros"); - assertThat(mappings, hasKey(dn)); - Set roles = mappings.get(dn); + assertThat(mappings, hasKey(dn.toNormalizedString())); + List roles = mappings.get(dn.toNormalizedString()); assertThat(roles, notNullValue()); assertThat(roles, hasSize(2)); assertThat(roles, containsInAnyOrder("security", "avenger")); dn = new DN("cn=shield,ou=marvel,o=superheros"); - assertThat(mappings, hasKey(dn)); - roles = mappings.get(dn); + assertThat(mappings, hasKey(dn.toNormalizedString())); + roles = mappings.get(dn.toNormalizedString()); assertThat(roles, notNullValue()); assertThat(roles, hasSize(1)); assertThat(roles, contains("security")); dn = new DN("cn=Horatio Hornblower,ou=people,o=sevenSeas"); - assertThat(mappings, hasKey(dn)); - roles = mappings.get(dn); + assertThat(mappings, hasKey(dn.toNormalizedString())); + roles = mappings.get(dn.toNormalizedString()); assertThat(roles, notNullValue()); assertThat(roles, hasSize(1)); assertThat(roles, contains("avenger")); @@ -230,7 +230,7 @@ public class DnRoleMapperTests extends ESTestCase { Path file = createTempDir().resolve("foo.yaml"); Files.createFile(file); Logger logger = CapturingLogger.newCapturingLogger(Level.DEBUG, null); - Map> mappings = DnRoleMapper.parseFile(file, logger, "_type", "_name", false); + Map> mappings = DnRoleMapper.parseFile(file, logger, "_type", "_name", false); assertThat(mappings, notNullValue()); assertThat(mappings.isEmpty(), is(true)); List events = CapturingLogger.output(logger.getName(), Level.DEBUG); @@ -242,7 +242,7 @@ public class DnRoleMapperTests extends ESTestCase { public void testParseFile_WhenFileDoesNotExist() throws Exception { Path file = createTempDir().resolve(randomAlphaOfLength(10)); Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null); - Map> mappings = DnRoleMapper.parseFile(file, logger, "_type", "_name", false); + Map> mappings = DnRoleMapper.parseFile(file, logger, "_type", "_name", false); assertThat(mappings, notNullValue()); assertThat(mappings.isEmpty(), is(true)); @@ -272,7 +272,7 @@ public class DnRoleMapperTests extends ESTestCase { // writing in utf_16 should cause a parsing error as we try to read the file in utf_8 Files.write(file, Collections.singletonList("aldlfkjldjdflkjd"), StandardCharsets.UTF_16); Logger logger = CapturingLogger.newCapturingLogger(Level.INFO, null); - Map> mappings = DnRoleMapper.parseFileLenient(file, logger, "_type", "_name"); + Map> mappings = DnRoleMapper.parseFileLenient(file, logger, "_type", "_name"); assertThat(mappings, notNullValue()); assertThat(mappings.isEmpty(), is(true)); List events = CapturingLogger.output(logger.getName(), Level.ERROR);