From 664354f7518a65026ab156f5d12b10f044f50587 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 24 Jun 2022 12:45:41 +1000 Subject: [PATCH 1/4] Fix unique realm names check to cover default realms All enabled realms must have unique names. This PR tightened the logic for ensuring realm name uniqueness. Previously the unique name check does not cover realms that are automatically enabled. Relates: #69096 --- .../xpack/security/authc/Realms.java | 49 ++++-- .../xpack/security/authc/RealmsTests.java | 153 ++++++++++++++++-- 2 files changed, 182 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java index 153a3502fffcc..c440642517f20 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java @@ -272,18 +272,11 @@ protected List initRealms(List realmConfigs) throws Exceptio checkUniqueOrders(orderToRealmName); Collections.sort(realms); + ensureUniqueExplicitlyConfiguredRealmNames(nameToRealmIdentifier); maybeAddBasicRealms(realms, realmConfigs); // always add built in first! - realms.add(0, reservedRealm); - String duplicateRealms = nameToRealmIdentifier.entrySet() - .stream() - .filter(entry -> entry.getValue().size() > 1) - .map(entry -> entry.getKey() + ": " + entry.getValue()) - .collect(Collectors.joining("; ")); - if (Strings.hasText(duplicateRealms)) { - throw new IllegalArgumentException("Found multiple realms configured with the same name: " + duplicateRealms + ""); - } + addReservedRealm(realms); logDeprecationForReservedPrefixedRealmNames(reservedPrefixedRealmIdentifiers); return Collections.unmodifiableList(realms); } @@ -372,8 +365,9 @@ protected void doClose() throws IOException { private void maybeAddBasicRealms(List realms, List realmConfigs) throws Exception { final Set disabledBasicRealmTypes = findDisabledBasicRealmTypes(realmConfigs); final Set realmTypes = realms.stream().map(Realm::type).collect(Collectors.toUnmodifiableSet()); - // Add native realm first so that file realm will be in the beginning + // Add native realm first so that file realm will be added before it if (false == disabledBasicRealmTypes.contains(NativeRealmSettings.TYPE) && false == realmTypes.contains(NativeRealmSettings.TYPE)) { + ensureRealmNameIsAvailable(realms, NativeRealmSettings.DEFAULT_NAME); var nativeRealmId = new RealmConfig.RealmIdentifier(NativeRealmSettings.TYPE, NativeRealmSettings.DEFAULT_NAME); var realmConfig = new RealmConfig( nativeRealmId, @@ -385,6 +379,7 @@ private void maybeAddBasicRealms(List realms, List realmConf realms.add(0, factories.get(NativeRealmSettings.TYPE).create(realmConfig)); } if (false == disabledBasicRealmTypes.contains(FileRealmSettings.TYPE) && false == realmTypes.contains(FileRealmSettings.TYPE)) { + ensureRealmNameIsAvailable(realms, FileRealmSettings.DEFAULT_NAME); var fileRealmId = new RealmConfig.RealmIdentifier(FileRealmSettings.TYPE, FileRealmSettings.DEFAULT_NAME); var realmConfig = new RealmConfig( fileRealmId, @@ -397,6 +392,29 @@ private void maybeAddBasicRealms(List realms, List realmConf } } + private void addReservedRealm(List realms) { + ensureRealmNameIsAvailable(realms, ReservedRealm.NAME); + realms.add(0, reservedRealm); + } + + /** + * Check the that given realmName is not yet used by the given list of realms. + */ + private void ensureRealmNameIsAvailable(List realms, String realmName) { + assert realms.size() == realms.stream().map(Realm::name).collect(Collectors.toUnmodifiableSet()).size() + : "existing realm names must be unique"; + final Realm misNamedRealm = realms.stream().filter(realm -> realmName.equals(realm.name())).findFirst().orElse(null); + if (misNamedRealm != null) { + throw new IllegalArgumentException( + "Found realm configured with name clashing with the [" + + realmName + + "] realm: [" + + (RealmSettings.realmSettingPrefix(misNamedRealm.type()) + misNamedRealm.name()) + + "]" + ); + } + } + private static Settings ensureOrderSetting(Settings settings, RealmConfig.RealmIdentifier realmIdentifier, int order) { String orderSettingKey = RealmSettings.realmSettingPrefix(realmIdentifier) + "order"; return Settings.builder().put(settings).put(orderSettingKey, order).build(); @@ -413,6 +431,17 @@ private static void checkUniqueOrders(Map> orderToRealmName } } + private void ensureUniqueExplicitlyConfiguredRealmNames(Map> nameToRealmIdentifier) { + String duplicateRealms = nameToRealmIdentifier.entrySet() + .stream() + .filter(entry -> entry.getValue().size() > 1) + .map(entry -> entry.getKey() + ": " + entry.getValue()) + .collect(Collectors.joining("; ")); + if (Strings.hasText(duplicateRealms)) { + throw new IllegalArgumentException("Found multiple realms configured with the same name: " + duplicateRealms + ""); + } + } + private List buildRealmConfigs() { final Map realmsSettings = RealmSettings.getRealmSettings(settings); final Set internalTypes = new HashSet<>(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java index ddb54d8955bdc..c213700c5e328 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.core.Tuple; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.license.License; @@ -125,16 +126,7 @@ public void init() throws Exception { threadContext = new ThreadContext(Settings.EMPTY); - ThreadPool threadPool = mock(ThreadPool.class); - when(threadPool.getThreadContext()).thenReturn(threadContext); - - reservedRealm = new ReservedRealm( - mock(Environment.class), - Settings.EMPTY, - mock(NativeUsersStore.class), - new AnonymousUser(Settings.EMPTY), - threadPool - ); + reservedRealm = buildReservedRealm(); allowAllRealms(); } @@ -1097,6 +1089,147 @@ public void testWarningsForReservedPrefixedRealmNames() throws Exception { ); } + public void testExplicitlyConfiguredRealmNamesMustNotClashWithDefaultRealmNames() throws Exception { + final Tuple defaultRealm = randomFrom( + new Tuple<>("reserved", "reserved"), + new Tuple<>("file", "default_file"), + new Tuple<>("native", "default_native") + ); + + final String configuredRealmType = randomValueOtherThan(defaultRealm.v1(), () -> randomAlphaOfLengthBetween(3, 18)); + factories.put(configuredRealmType, DummyRealm::new); + final String realmSettingKey = "xpack.security.authc.realms." + configuredRealmType + "." + defaultRealm.v2(); + + // The name of an explicitly configured realm must not be either default_native, default_file or "reserved" + final Settings settings1 = Settings.builder() + .put("path.home", createTempDir()) + .put(realmSettingKey + ".order", randomInt()) + .build(); + final IllegalArgumentException e1 = expectThrows( + IllegalArgumentException.class, + () -> new Realms( + settings1, + TestEnvironment.newEnvironment(settings1), + factories, + licenseState, + threadContext, + buildReservedRealm() + ) + ); + assertThat( + e1.getMessage(), + containsString( + "Found realm configured with name clashing with the [" + defaultRealm.v2() + "] realm: [" + realmSettingKey + "]" + ) + ); + + // It is OK if the explicitly configured realm is disabled + final Settings settings2 = Settings.builder() + .put("path.home", createTempDir()) + .put(realmSettingKey + ".order", randomInt()) + .put(realmSettingKey + ".enabled", false) + .build(); + final Realms realms2 = new Realms( + settings2, + TestEnvironment.newEnvironment(settings2), + factories, + licenseState, + threadContext, + buildReservedRealm() + ); + assertThat(realms2.getActiveRealms().stream().map(Realm::type).toList(), containsInAnyOrder("reserved", "file", "native")); + + // This block of tests do not apply to reserved realm since it is not configurable + if ("file".equals(defaultRealm.v1()) || "native".equals(defaultRealm.v1())) { + final String autoEnabledRealmType = "file".equals(defaultRealm.v1()) ? "native" : "file"; + + // It is OK if the default file or native realm is disabled + final Settings settings3 = Settings.builder() + .put("path.home", createTempDir()) + .put(realmSettingKey + ".order", randomInt()) + .put("xpack.security.authc.realms." + defaultRealm.v1() + "." + randomAlphaOfLengthBetween(3, 18) + ".enabled", false) + .build(); + final Realms realms3 = new Realms( + settings3, + TestEnvironment.newEnvironment(settings3), + factories, + licenseState, + threadContext, + buildReservedRealm() + ); + assertThat( + realms3.getActiveRealms().stream().map(Realm::type).toList(), + containsInAnyOrder("reserved", configuredRealmType, autoEnabledRealmType) + ); + + // It is OK if file or native realm is named something else + final Settings settings4 = Settings.builder() + .put("path.home", createTempDir()) + .put(realmSettingKey + ".order", randomInt()) + .put("xpack.security.authc.realms." + defaultRealm.v1() + "." + randomAlphaOfLengthBetween(3, 18) + ".order", randomInt()) + .build(); + final Realms realms4 = new Realms( + settings4, + TestEnvironment.newEnvironment(settings4), + factories, + licenseState, + threadContext, + buildReservedRealm() + ); + assertThat( + realms4.getActiveRealms().stream().map(Realm::type).toList(), + containsInAnyOrder("reserved", configuredRealmType, "file", "native") + ); + + // All explicitly configured realm names still must be unique + final Settings settings5 = Settings.builder() + .put("path.home", createTempDir()) + .put(realmSettingKey + ".order", randomInt()) + .put("xpack.security.authc.realms." + defaultRealm.v1() + "." + defaultRealm.v2() + ".order", randomInt()) + .build(); + final IllegalArgumentException e5 = expectThrows( + IllegalArgumentException.class, + () -> new Realms( + settings5, + TestEnvironment.newEnvironment(settings5), + factories, + licenseState, + threadContext, + buildReservedRealm() + ) + ); + assertThat(e5.getMessage(), containsString("Found multiple realms configured with the same name")); + } + + // It is OK to explicitly configure file to be default_file and native to be default_native + final Settings settings6 = Settings.builder() + .put("path.home", createTempDir()) + .put("xpack.security.authc.realms.file.default_file.order", randomInt()) + .put("xpack.security.authc.realms.native.default_native.order", randomInt()) + .build(); + final Realms realms6 = new Realms( + settings6, + TestEnvironment.newEnvironment(settings6), + factories, + licenseState, + threadContext, + buildReservedRealm() + ); + assertThat(realms6.getActiveRealms().stream().map(Realm::type).toList(), containsInAnyOrder("reserved", "file", "native")); + } + + private ReservedRealm buildReservedRealm() { + final ThreadPool threadPool = mock(ThreadPool.class); + when(threadPool.getThreadContext()).thenReturn(threadContext); + return new ReservedRealm( + mock(Environment.class), + Settings.EMPTY, + mock(NativeUsersStore.class), + new AnonymousUser(Settings.EMPTY), + threadPool + ); + } + private boolean randomDisableRealm(Settings.Builder builder, String type) { final boolean disabled = randomBoolean(); if (disabled) { From ede3bd0a7e1bf03e117c1b4d3e0f3a1c5b1f0e48 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 24 Jun 2022 12:51:53 +1000 Subject: [PATCH 2/4] Update docs/changelog/87999.yaml --- docs/changelog/87999.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/87999.yaml diff --git a/docs/changelog/87999.yaml b/docs/changelog/87999.yaml new file mode 100644 index 0000000000000..9216f0a9b457c --- /dev/null +++ b/docs/changelog/87999.yaml @@ -0,0 +1,5 @@ +pr: 87999 +summary: Fix unique realm name check to cover default realms +area: Authentication +type: bug +issues: [] From 71bbf5aa95526366a00d0f47f5d859dd9080676a Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 24 Jun 2022 14:20:09 +1000 Subject: [PATCH 3/4] remove obsolete test --- .../xpack/security/authc/RealmsTests.java | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java index c213700c5e328..2e0a8f4d56287 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/RealmsTests.java @@ -400,27 +400,6 @@ public void testRealmAssignedToMultipleDomainsNotPermitted() { ); } - public void testReservedRealmIsAlwaysDomainless() throws Exception { - int realmId = randomIntBetween(0, randomRealmTypesCount - 1); - Settings settings = Settings.builder() - .put("xpack.security.authc.realms.type_" + realmId + ".reserved.order", 2) - .put("xpack.security.authc.domains.domain_reserved.realms", "reserved") - .put("xpack.security.authc.realms." + NativeRealmSettings.TYPE + ".disabled_native.enabled", false) - .put("xpack.security.authc.realms." + FileRealmSettings.TYPE + ".disabled_file.enabled", false) - .put("path.home", createTempDir()) - .build(); - Environment env = TestEnvironment.newEnvironment(settings); - Realms realms = new Realms(settings, env, factories, licenseState, threadContext, reservedRealm); - Iterator iterator = realms.iterator(); - assertThat(iterator.hasNext(), is(true)); - Realm realm = iterator.next(); - assertThat(realm, is(reservedRealm)); - realm = iterator.next(); - assertThat(realm.type(), is("type_" + realmId)); - assertThat(realm.name(), is("reserved")); - assertThat(realm.realmRef().getDomain().name(), is("domain_reserved")); - } - public void testDomainWithUndefinedRealms() { Settings settings = Settings.builder() .put("xpack.security.authc.domains.test_domain.realms", "reserved, default_file") From 466583a24d8fb4d48007500a75be79e9286d595d Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 24 Jun 2022 18:06:10 +1000 Subject: [PATCH 4/4] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java Co-authored-by: Nikolaj Volgushev --- .../java/org/elasticsearch/xpack/security/authc/Realms.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java index c440642517f20..f93f4202aa63a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java @@ -398,7 +398,7 @@ private void addReservedRealm(List realms) { } /** - * Check the that given realmName is not yet used by the given list of realms. + * Check that the given realmName is not yet used by the given list of realms. */ private void ensureRealmNameIsAvailable(List realms, String realmName) { assert realms.size() == realms.stream().map(Realm::name).collect(Collectors.toUnmodifiableSet()).size()