From 7ba39c967e617eb9ab89753f14ca8461649afe67 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 6 Dec 2023 16:37:02 -0500 Subject: [PATCH 01/28] Only allow TransportConfigUpdateAction after node has been set as bootstrapped Signed-off-by: Craig Perkins --- .../configupdate/TransportConfigUpdateAction.java | 6 ++++-- .../configuration/ConfigurationRepository.java | 3 +++ .../security/securityconf/DynamicConfigFactory.java | 10 +++++++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java index 64149a7c97..2a20cb7afa 100644 --- a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java +++ b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java @@ -125,8 +125,10 @@ protected ConfigUpdateResponse newResponse( @Override protected ConfigUpdateNodeResponse nodeOperation(final NodeConfigUpdateRequest request) { - configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); - backendRegistry.get().invalidateCache(); + if (dynamicConfigFactory.isBootstrapped()) { + configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); + backendRegistry.get().invalidateCache(); + } return new ConfigUpdateNodeResponse(clusterService.localNode(), request.request.getConfigTypes(), null); } diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 81e9f47370..41c566f52c 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -219,9 +219,11 @@ private ConfigurationRepository( try { LOGGER.debug("Try to load config ..."); reloadConfiguration(Arrays.asList(CType.values())); + dynamicConfigFactory.setBootstrapped(); break; } catch (Exception e) { LOGGER.debug("Unable to load configuration due to {}", String.valueOf(ExceptionUtils.getRootCause(e))); + dynamicConfigFactory.setBootstrapped(); try { Thread.sleep(3000); } catch (InterruptedException e1) { @@ -323,6 +325,7 @@ public void initOnNodeStart() { "Will not attempt to create index {} and default configs if they are absent. Will not perform background initialization", securityIndex ); + dynamicConfigFactory.setBootstrapped(); } } catch (Throwable e2) { LOGGER.error("Error during node initialization: {}", e2, e2); diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java index ed61481885..72735f1e52 100644 --- a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java +++ b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java @@ -125,6 +125,7 @@ public final static SecurityDynamicConfiguration addStatics(SecurityDynamicCo protected final Logger log = LogManager.getLogger(this.getClass()); private final ConfigurationRepository cr; private final AtomicBoolean initialized = new AtomicBoolean(); + private final AtomicBoolean bootstrapped = new AtomicBoolean(); private final EventBus eventBus = EVENT_BUS_BUILDER.logger(new JavaLogger(DynamicConfigFactory.class.getCanonicalName())).build(); private final Settings opensearchSettings; private final Path configPath; @@ -315,7 +316,6 @@ public void onChange(Map> typeToConfig) { } initialized.set(true); - } private static ConfigV6 getConfigV6(SecurityDynamicConfiguration sdc) { @@ -335,6 +335,14 @@ public final boolean isInitialized() { return initialized.get(); } + public final boolean isBootstrapped() { + return bootstrapped.get(); + } + + public final void setBootstrapped() { + bootstrapped.set(true); + } + public void registerDCFListener(Object listener) { eventBus.register(listener); } From d05da1a8abf536ace440011575c53b54d4c034a1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 8 Dec 2023 09:58:36 -0500 Subject: [PATCH 02/28] Set in finally block and change name to bgThreadComplete Signed-off-by: Craig Perkins --- .../configupdate/TransportConfigUpdateAction.java | 2 +- .../configuration/ConfigurationRepository.java | 6 +++--- .../security/securityconf/DynamicConfigFactory.java | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java index 2a20cb7afa..fca3fde5d7 100644 --- a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java +++ b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java @@ -125,7 +125,7 @@ protected ConfigUpdateResponse newResponse( @Override protected ConfigUpdateNodeResponse nodeOperation(final NodeConfigUpdateRequest request) { - if (dynamicConfigFactory.isBootstrapped()) { + if (dynamicConfigFactory.isBackgroundThreadComplete()) { configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); backendRegistry.get().invalidateCache(); } diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 41c566f52c..7762f98db1 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -219,11 +219,9 @@ private ConfigurationRepository( try { LOGGER.debug("Try to load config ..."); reloadConfiguration(Arrays.asList(CType.values())); - dynamicConfigFactory.setBootstrapped(); break; } catch (Exception e) { LOGGER.debug("Unable to load configuration due to {}", String.valueOf(ExceptionUtils.getRootCause(e))); - dynamicConfigFactory.setBootstrapped(); try { Thread.sleep(3000); } catch (InterruptedException e1) { @@ -258,6 +256,8 @@ private ConfigurationRepository( } catch (Exception e) { LOGGER.error("Unexpected exception while initializing node " + e, e); + } finally { + dynamicConfigFactory.setBackgroundThreadComplete(); } }); @@ -325,7 +325,7 @@ public void initOnNodeStart() { "Will not attempt to create index {} and default configs if they are absent. Will not perform background initialization", securityIndex ); - dynamicConfigFactory.setBootstrapped(); + dynamicConfigFactory.setBackgroundThreadComplete(); } } catch (Throwable e2) { LOGGER.error("Error during node initialization: {}", e2, e2); diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java index 72735f1e52..57408197b1 100644 --- a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java +++ b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java @@ -125,7 +125,7 @@ public final static SecurityDynamicConfiguration addStatics(SecurityDynamicCo protected final Logger log = LogManager.getLogger(this.getClass()); private final ConfigurationRepository cr; private final AtomicBoolean initialized = new AtomicBoolean(); - private final AtomicBoolean bootstrapped = new AtomicBoolean(); + private final AtomicBoolean bgThreadComplete = new AtomicBoolean(); private final EventBus eventBus = EVENT_BUS_BUILDER.logger(new JavaLogger(DynamicConfigFactory.class.getCanonicalName())).build(); private final Settings opensearchSettings; private final Path configPath; @@ -335,12 +335,12 @@ public final boolean isInitialized() { return initialized.get(); } - public final boolean isBootstrapped() { - return bootstrapped.get(); + public final boolean isBackgroundThreadComplete() { + return bgThreadComplete.get(); } - public final void setBootstrapped() { - bootstrapped.set(true); + public final void setBackgroundThreadComplete() { + bgThreadComplete.set(true); } public void registerDCFListener(Object listener) { From 58d6f459e4f44f6a156d61c0bd011dd91e1e67b3 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 8 Dec 2023 10:58:48 -0500 Subject: [PATCH 03/28] Add tests that ensure the bg thread completes during bootstrap Signed-off-by: Craig Perkins --- .../SecurityConfigurationBootstrapTests.java | 102 ++++++++++++++++++ .../test/framework/cluster/LocalCluster.java | 10 ++ 2 files changed, 112 insertions(+) create mode 100644 src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java new file mode 100644 index 0000000000..b8aa340e01 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java @@ -0,0 +1,102 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + */ +package org.opensearch.security; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.commons.io.FileUtils; +import org.awaitility.Awaitility; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.client.Client; +import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.support.ConfigConstants; +import org.opensearch.test.framework.TestSecurityConfig.User; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.ContextHeaderDecoratorClient; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class SecurityConfigurationBootstrapTests { + + private final static Path configurationFolder = ConfigurationFiles.createConfigurationDirectory(); + + private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS); + + @ClassRule + public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + .loadConfigurationIntoIndex(false) + .defaultConfigurationInitDirectory(configurationFolder.toString()) + .nodeSettings( + Map.of( + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), + SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, + true + ) + ) + .build(); + + @BeforeClass + public static void initData() { + + Client client = new ContextHeaderDecoratorClient( + cluster.getInternalNodeClient(), + Map.of(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true") + ); + Runnable r = new Runnable() { + public void run() { + long t = System.currentTimeMillis(); + long end = t + 10000; + while (System.currentTimeMillis() < end) { + cluster.triggerConfigurationReloadForSingleCType(client, CType.CONFIG, true); + try { + Thread.sleep(50); + } catch (InterruptedException e) { + break; + } + } + } + }; + + new Thread(r).start(); + } + + @AfterClass + public static void cleanConfigurationDirectory() throws IOException { + FileUtils.deleteDirectory(configurationFolder.toFile()); + } + + @Test + public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + + TestRestClient.HttpResponse response = client.getAuthInfo(); + + response.assertStatusCode(200); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java index 64207ead5b..081ba602c2 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java @@ -297,6 +297,16 @@ private static void triggerConfigurationReload(Client client) { } } + public void triggerConfigurationReloadForSingleCType(Client client, CType cType, boolean ignoreFailures) { + ConfigUpdateResponse configUpdateResponse = client.execute( + ConfigUpdateAction.INSTANCE, + new ConfigUpdateRequest(new String[] { cType.toLCString() }) + ).actionGet(); + if (!ignoreFailures && configUpdateResponse.hasFailures()) { + throw new RuntimeException("ConfigUpdateResponse produced failures: " + configUpdateResponse.failures()); + } + } + public CertificateData getAdminCertificate() { return testCertificates.getAdminCertificateData(); } From c2423b7f9a70cd87953a2acd0f3214a41855b20b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 8 Dec 2023 11:28:14 -0500 Subject: [PATCH 04/28] Add test to provide initialize with securityadmin works Signed-off-by: Craig Perkins --- .../security/ConfigurationFiles.java | 4 +- .../security/SecurityAdminLauncher.java | 18 ++++ ...rationBootstrapWithSecurityAdminTests.java | 83 +++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java diff --git a/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java b/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java index 287bc139b1..dbd3265dfe 100644 --- a/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java +++ b/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java @@ -33,10 +33,12 @@ public static Path createConfigurationDirectory() { "action_groups.yml", "config.yml", "internal_users.yml", + "nodes_dn.yml", "roles.yml", "roles_mapping.yml", "security_tenants.yml", - "tenants.yml" }; + "tenants.yml", + "whitelist.yml" }; for (String fileName : configurationFiles) { Path configFileDestination = tempDirectory.resolve(fileName); copyResourceToFile(fileName, configFileDestination.toFile()); diff --git a/src/integrationTest/java/org/opensearch/security/SecurityAdminLauncher.java b/src/integrationTest/java/org/opensearch/security/SecurityAdminLauncher.java index 164b2cb714..81460d3d91 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityAdminLauncher.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityAdminLauncher.java @@ -10,6 +10,7 @@ package org.opensearch.security; import java.io.File; +import java.nio.file.Path; import org.opensearch.security.tools.SecurityAdmin; import org.opensearch.test.framework.certificate.TestCertificates; @@ -44,4 +45,21 @@ public int updateRoleMappings(File roleMappingsConfigurationFile) throws Excepti return SecurityAdmin.execute(commandLineArguments); } + + public int runSecurityAdmin(Path configurationFolder) throws Exception { + String[] commandLineArguments = { + "-cacert", + certificates.getRootCertificate().getAbsolutePath(), + "-cert", + certificates.getAdminCertificate().getAbsolutePath(), + "-key", + certificates.getAdminKey(null).getAbsolutePath(), + "-nhnv", + "-p", + String.valueOf(port), + "-cd", + configurationFolder.toString() }; + + return SecurityAdmin.execute(commandLineArguments); + } } diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java new file mode 100644 index 0000000000..03c7321a5d --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java @@ -0,0 +1,83 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + */ +package org.opensearch.security; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.commons.io.FileUtils; +import org.awaitility.Awaitility; +import org.junit.AfterClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; + +import org.opensearch.test.framework.TestSecurityConfig.User; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; +import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class SecurityConfigurationBootstrapWithSecurityAdminTests { + + private final static Path configurationFolder = ConfigurationFiles.createConfigurationDirectory(); + + @Rule + public TemporaryFolder configurationDirectory = new TemporaryFolder(); + + private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS); + + @ClassRule + public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + .loadConfigurationIntoIndex(false) + .nodeSettings( + Map.of( + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), + SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, + false, + SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, + false + ) + ) + .build(); + + @AfterClass + public static void cleanConfigurationDirectory() throws IOException { + FileUtils.deleteDirectory(configurationFolder.toFile()); + } + + @Test + public void testInitializeWithSecurityAdminWhenNoBackgroundInitialization() throws Exception { + SecurityAdminLauncher securityAdminLauncher = new SecurityAdminLauncher(cluster.getHttpPort(), cluster.getTestCertificates()); + + int exitCode = securityAdminLauncher.runSecurityAdmin(configurationFolder); + + assertThat(exitCode, equalTo(0)); + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + Awaitility.await() + .alias("Waiting for rolemapping 'readall' availability.") + .until(() -> client.get("_plugins/_security/api/rolesmapping/readall").getStatusCode(), equalTo(200)); + } + } +} From d86feed0b99be6ccc5878eee156d7edf009a5d74 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 8 Dec 2023 12:00:34 -0500 Subject: [PATCH 05/28] Set background init to false to ensure that transport config update actions can pass Signed-off-by: Craig Perkins --- .../opensearch/security/api/CreateResetPasswordTest.java | 2 +- .../security/http/OnBehalfOfJwtAuthenticationTest.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/api/CreateResetPasswordTest.java b/src/integrationTest/java/org/opensearch/security/api/CreateResetPasswordTest.java index 44f8dca20b..8a7795e90f 100644 --- a/src/integrationTest/java/org/opensearch/security/api/CreateResetPasswordTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/CreateResetPasswordTest.java @@ -63,7 +63,7 @@ public class CreateResetPasswordTest { SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, - true, + false, ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_REGEX, CUSTOM_PASSWORD_REGEX, ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE, diff --git a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java index 78af7ffc05..7210c53ad4 100644 --- a/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java +++ b/src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java @@ -48,7 +48,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; -import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; +import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST; import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED; import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; @@ -128,8 +128,8 @@ private static OnBehalfOfConfig defaultOnBehalfOfConfig() { .users(ADMIN_USER, OBO_USER, OBO_USER_NO_PERM, HOST_MAPPING_OBO_USER) .nodeSettings( Map.of( - SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, - true, + SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, + false, SECURITY_RESTAPI_ROLES_ENABLED, ADMIN_USER.getRoleNames(), SECURITY_RESTAPI_ADMIN_ENABLED, From 9606f387882d60e67616dff02618da518121fccc Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 8 Dec 2023 13:49:23 -0500 Subject: [PATCH 06/28] Update SecurityConfigurationTests Signed-off-by: Craig Perkins --- .../org/opensearch/security/SecurityConfigurationTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java index cc95f191f7..e0e216f048 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java @@ -70,7 +70,7 @@ public class SecurityConfigurationTests { SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, - true + false ) ) .build(); From 0b764f1f2130dd4bcd79fa05cf82611c627aff73 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 12:59:48 -0500 Subject: [PATCH 07/28] Use CompletableFuture Signed-off-by: Craig Perkins --- .../configupdate/TransportConfigUpdateAction.java | 2 +- .../configuration/ConfigurationRepository.java | 15 ++++++++++----- .../securityconf/DynamicConfigFactory.java | 9 --------- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java index fca3fde5d7..a50b6bc178 100644 --- a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java +++ b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java @@ -125,7 +125,7 @@ protected ConfigUpdateResponse newResponse( @Override protected ConfigUpdateNodeResponse nodeOperation(final NodeConfigUpdateRequest request) { - if (dynamicConfigFactory.isBackgroundThreadComplete()) { + if (configurationRepository.isBgThreadComplete()) { configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); backendRegistry.get().invalidateCache(); } diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 7762f98db1..8d53c3817a 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -37,6 +37,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.Lock; @@ -91,6 +92,7 @@ public class ConfigurationRepository { private final ThreadPool threadPool; private DynamicConfigFactory dynamicConfigFactory; private static final int DEFAULT_CONFIG_VERSION = 2; + private CompletableFuture bgThreadRunner = new CompletableFuture<>(); private final Thread bgThread; private final AtomicBoolean installDefaultConfig = new AtomicBoolean(); private final boolean acceptInvalid; @@ -256,8 +258,6 @@ private ConfigurationRepository( } catch (Exception e) { LOGGER.error("Unexpected exception while initializing node " + e, e); - } finally { - dynamicConfigFactory.setBackgroundThreadComplete(); } }); @@ -313,23 +313,24 @@ public void initOnNodeStart() { if (settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false)) { LOGGER.info("Will attempt to create index {} and default configs if they are absent", securityIndex); installDefaultConfig.set(true); - bgThread.start(); + bgThreadRunner = CompletableFuture.runAsync(bgThread); } else if (settings.getAsBoolean(ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, true)) { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Use securityadmin to initialize cluster", securityIndex ); - bgThread.start(); + bgThreadRunner = CompletableFuture.runAsync(bgThread); } else { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Will not perform background initialization", securityIndex ); - dynamicConfigFactory.setBackgroundThreadComplete(); + bgThreadRunner.complete(null); } } catch (Throwable e2) { LOGGER.error("Error during node initialization: {}", e2, e2); bgThread.start(); + bgThreadRunner = CompletableFuture.runAsync(bgThread); } } @@ -375,6 +376,10 @@ public SecurityDynamicConfiguration getConfiguration(CType configurationType) private final Lock LOCK = new ReentrantLock(); + public boolean isBgThreadComplete() { + return bgThreadRunner.isDone(); + } + public void reloadConfiguration(Collection configTypes) throws ConfigUpdateAlreadyInProgressException { try { if (LOCK.tryLock(60, TimeUnit.SECONDS)) { diff --git a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java index 57408197b1..f046b4c114 100644 --- a/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java +++ b/src/main/java/org/opensearch/security/securityconf/DynamicConfigFactory.java @@ -125,7 +125,6 @@ public final static SecurityDynamicConfiguration addStatics(SecurityDynamicCo protected final Logger log = LogManager.getLogger(this.getClass()); private final ConfigurationRepository cr; private final AtomicBoolean initialized = new AtomicBoolean(); - private final AtomicBoolean bgThreadComplete = new AtomicBoolean(); private final EventBus eventBus = EVENT_BUS_BUILDER.logger(new JavaLogger(DynamicConfigFactory.class.getCanonicalName())).build(); private final Settings opensearchSettings; private final Path configPath; @@ -335,14 +334,6 @@ public final boolean isInitialized() { return initialized.get(); } - public final boolean isBackgroundThreadComplete() { - return bgThreadComplete.get(); - } - - public final void setBackgroundThreadComplete() { - bgThreadComplete.set(true); - } - public void registerDCFListener(Object listener) { eventBus.register(listener); } From 83961f0ae688fe179329b67ae3a1954bcab31c7f Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 13:29:19 -0500 Subject: [PATCH 08/28] Wrap in doPrivileged Signed-off-by: Craig Perkins --- .../security/configuration/ConfigurationRepository.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 8d53c3817a..362474f3a7 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -28,6 +28,7 @@ import java.io.File; import java.nio.file.Path; +import java.security.AccessController; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Arrays; @@ -132,7 +133,9 @@ private ConfigurationRepository( if (installDefaultConfig.get()) { try { - String lookupDir = System.getProperty("security.default_init.dir"); + String lookupDir = AccessController.doPrivileged( + (java.security.PrivilegedAction) () -> System.getProperty("security.default_init.dir") + ); final String cd = lookupDir != null ? (lookupDir + "/") : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; From b21130250f8eb4bb9b2ee1fd43ee1c7121d03ea8 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 13:44:24 -0500 Subject: [PATCH 09/28] doPrivileged Signed-off-by: Craig Perkins --- .../configuration/ConfigurationRepository.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 362474f3a7..1a7f386e00 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -133,12 +133,12 @@ private ConfigurationRepository( if (installDefaultConfig.get()) { try { - String lookupDir = AccessController.doPrivileged( - (java.security.PrivilegedAction) () -> System.getProperty("security.default_init.dir") - ); - final String cd = lookupDir != null - ? (lookupDir + "/") - : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; + final String cd = AccessController.doPrivileged((java.security.PrivilegedAction) () -> { + String lookupDir = System.getProperty("security.default_init.dir"); + return lookupDir != null + ? (lookupDir + "/") + : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; + }); File confFile = new File(cd + "config.yml"); if (confFile.exists()) { final ThreadContext threadContext = threadPool.getThreadContext(); From 0613161963669ca9de17533dd5e928818ccfdc1b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 13:59:57 -0500 Subject: [PATCH 10/28] Wrap with doPrivileged Signed-off-by: Craig Perkins --- .../configuration/ConfigurationRepository.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 1a7f386e00..2184ec2570 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -29,6 +29,7 @@ import java.io.File; import java.nio.file.Path; import java.security.AccessController; +import java.security.PrivilegedAction; import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Arrays; @@ -133,12 +134,10 @@ private ConfigurationRepository( if (installDefaultConfig.get()) { try { - final String cd = AccessController.doPrivileged((java.security.PrivilegedAction) () -> { - String lookupDir = System.getProperty("security.default_init.dir"); - return lookupDir != null - ? (lookupDir + "/") - : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; - }); + String lookupDir = System.getProperty("security.default_init.dir"); + final String cd = lookupDir != null + ? (lookupDir + "/") + : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; File confFile = new File(cd + "config.yml"); if (confFile.exists()) { final ThreadContext threadContext = threadPool.getThreadContext(); @@ -316,13 +315,13 @@ public void initOnNodeStart() { if (settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false)) { LOGGER.info("Will attempt to create index {} and default configs if they are absent", securityIndex); installDefaultConfig.set(true); - bgThreadRunner = CompletableFuture.runAsync(bgThread); + bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction) () -> bgThread)); } else if (settings.getAsBoolean(ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, true)) { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Use securityadmin to initialize cluster", securityIndex ); - bgThreadRunner = CompletableFuture.runAsync(bgThread); + bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction) () -> bgThread)); } else { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Will not perform background initialization", @@ -332,8 +331,7 @@ public void initOnNodeStart() { } } catch (Throwable e2) { LOGGER.error("Error during node initialization: {}", e2, e2); - bgThread.start(); - bgThreadRunner = CompletableFuture.runAsync(bgThread); + bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction) () -> bgThread)); } } From 258fb28c58506db7b5c4f3ecce03e73ab1a068db Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 14:16:46 -0500 Subject: [PATCH 11/28] Assert not initialized Signed-off-by: Craig Perkins --- ...curityConfigurationBootstrapWithSecurityAdminTests.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java index 03c7321a5d..4122388684 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java @@ -29,7 +29,9 @@ import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; +import static org.apache.http.HttpStatus.SC_SERVICE_UNAVAILABLE; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST; @@ -69,6 +71,11 @@ public static void cleanConfigurationDirectory() throws IOException { @Test public void testInitializeWithSecurityAdminWhenNoBackgroundInitialization() throws Exception { + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + TestRestClient.HttpResponse response = client.getAuthInfo(); + assertThat(response.getStatusCode(), equalTo(SC_SERVICE_UNAVAILABLE)); + assertThat(response.getBody(), containsString("OpenSearch Security not initialized")); + } SecurityAdminLauncher securityAdminLauncher = new SecurityAdminLauncher(cluster.getHttpPort(), cluster.getTestCertificates()); int exitCode = securityAdminLauncher.runSecurityAdmin(configurationFolder); From 91cf4723d3e683e632f9df0c6d563f9d529b584d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 14:23:26 -0500 Subject: [PATCH 12/28] Rename beforeClass method Signed-off-by: Craig Perkins --- .../security/SecurityConfigurationBootstrapTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java index b8aa340e01..8225c686d2 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java @@ -60,7 +60,7 @@ public class SecurityConfigurationBootstrapTests { .build(); @BeforeClass - public static void initData() { + public static void runConfigUpdateRequestsInBgThread() { Client client = new ContextHeaderDecoratorClient( cluster.getInternalNodeClient(), From 06b751304d4ffc2cd7cdb10c20ea71b2101f1761 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 11 Dec 2023 14:43:23 -0500 Subject: [PATCH 13/28] Add AccessControllerWrappedThread Signed-off-by: Craig Perkins --- .../ConfigurationRepository.java | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 2184ec2570..d84227b7fb 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -315,13 +315,13 @@ public void initOnNodeStart() { if (settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false)) { LOGGER.info("Will attempt to create index {} and default configs if they are absent", securityIndex); installDefaultConfig.set(true); - bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction) () -> bgThread)); + bgThreadRunner = CompletableFuture.runAsync(new AccessControllerWrappedThread(bgThread)); } else if (settings.getAsBoolean(ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, true)) { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Use securityadmin to initialize cluster", securityIndex ); - bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction) () -> bgThread)); + bgThreadRunner = CompletableFuture.runAsync(new AccessControllerWrappedThread(bgThread)); } else { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Will not perform background initialization", @@ -331,7 +331,7 @@ public void initOnNodeStart() { } } catch (Throwable e2) { LOGGER.error("Error during node initialization: {}", e2, e2); - bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction) () -> bgThread)); + bgThreadRunner = CompletableFuture.runAsync(new AccessControllerWrappedThread(bgThread)); } } @@ -497,4 +497,24 @@ private static String formatDate(long date) { public static int getDefaultConfigVersion() { return ConfigurationRepository.DEFAULT_CONFIG_VERSION; } + + private class AccessControllerWrappedThread extends Thread { + private final Thread innerThread; + + public AccessControllerWrappedThread(Thread innerThread) { + this.innerThread = innerThread; + } + + @Override + public void run() { + AccessController.doPrivileged(new PrivilegedAction() { + + @Override + public Void run() { + innerThread.run(); + return null; + } + }); + } + } } From a3398e06d78fb136fcaac8e89a725583b64edae3 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 12 Dec 2023 15:27:15 -0500 Subject: [PATCH 14/28] Use threadContext to carry context Signed-off-by: Craig Perkins --- .../TransportConfigUpdateAction.java | 4 ++-- .../configuration/ConfigurationRepository.java | 18 ++++++++++++------ .../security/support/ConfigConstants.java | 1 + 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java index a50b6bc178..6f1f99a434 100644 --- a/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java +++ b/src/main/java/org/opensearch/security/action/configupdate/TransportConfigUpdateAction.java @@ -125,8 +125,8 @@ protected ConfigUpdateResponse newResponse( @Override protected ConfigUpdateNodeResponse nodeOperation(final NodeConfigUpdateRequest request) { - if (configurationRepository.isBgThreadComplete()) { - configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); + boolean didReload = configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes()))); + if (didReload) { backendRegistry.get().invalidateCache(); } return new ConfigUpdateNodeResponse(clusterService.localNode(), request.request.getConfigTypes(), null); diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index d84227b7fb..8d5922e693 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -222,7 +222,11 @@ private ConfigurationRepository( while (!dynamicConfigFactory.isInitialized()) { try { LOGGER.debug("Try to load config ..."); - reloadConfiguration(Arrays.asList(CType.values())); + final ThreadContext threadContext = threadPool.getThreadContext(); + try (StoredContext ctx = threadContext.stashContext()) { + threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_BG_THREAD_HEADER, true); + reloadConfiguration(Arrays.asList(CType.values())); + } break; } catch (Exception e) { LOGGER.debug("Unable to load configuration due to {}", String.valueOf(ExceptionUtils.getRootCause(e))); @@ -377,15 +381,17 @@ public SecurityDynamicConfiguration getConfiguration(CType configurationType) private final Lock LOCK = new ReentrantLock(); - public boolean isBgThreadComplete() { - return bgThreadRunner.isDone(); - } - - public void reloadConfiguration(Collection configTypes) throws ConfigUpdateAlreadyInProgressException { + public boolean reloadConfiguration(Collection configTypes) throws ConfigUpdateAlreadyInProgressException { + final ThreadContext threadContext = threadPool.getThreadContext(); + Boolean isBgThread = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_BG_THREAD_HEADER); + if (!Boolean.TRUE.equals(isBgThread) && !bgThreadRunner.isDone()) { + return false; + } try { if (LOCK.tryLock(60, TimeUnit.SECONDS)) { try { reloadConfiguration0(configTypes, this.acceptInvalid); + return true; } finally { LOCK.unlock(); } diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 1f5728edfb..886947cb79 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -74,6 +74,7 @@ public class ConfigConstants { public static final String OPENDISTRO_SECURITY_MASKED_FIELD_CCS = OPENDISTRO_SECURITY_CONFIG_PREFIX + "masked_fields_ccs"; public static final String OPENDISTRO_SECURITY_CONF_REQUEST_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "conf_request"; + public static final String OPENDISTRO_SECURITY_BG_THREAD_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "bg_thread"; public static final String OPENDISTRO_SECURITY_REMOTE_ADDRESS = OPENDISTRO_SECURITY_CONFIG_PREFIX + "remote_address"; public static final String OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "remote_address_header"; From 7b73715e8e081a768e09b3053a39fbd8b72d47a9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 13:51:38 -0500 Subject: [PATCH 15/28] Small update Signed-off-by: Craig Perkins --- .../security/configuration/ConfigurationRepository.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 0495919697..efb5ac539d 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -503,11 +503,11 @@ private static String formatDate(long date) { public static int getDefaultConfigVersion() { return ConfigurationRepository.DEFAULT_CONFIG_VERSION; } - + public AtomicBoolean getInstallDefaultConfig() { return installDefaultConfig; } - + private class AccessControllerWrappedThread extends Thread { private final Thread innerThread; From 4b99681054626d045f330c46d2bf5ab0cfe367f1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 14:28:23 -0500 Subject: [PATCH 16/28] Use Awaitility Signed-off-by: Craig Perkins --- .../security/InitializationIntegrationTests.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index 79ab0c020b..e6ffc7f333 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -35,6 +35,7 @@ import org.apache.hc.core5.http.HttpVersion; import org.apache.hc.core5.http2.HttpVersionPolicy; import org.apache.http.HttpStatus; +import org.awaitility.Awaitility; import org.junit.Assert; import org.junit.Test; @@ -61,6 +62,8 @@ import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import static org.hamcrest.Matchers.equalTo; + public class InitializationIntegrationTests extends SingleClusterTest { @Test @@ -281,9 +284,10 @@ public void testDefaultConfig() throws Exception { final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build(); setup(Settings.EMPTY, null, settings, false); RestHelper rh = nonSslRestHelper(); - Thread.sleep(10000); - Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode()); + Awaitility.await() + .alias("Load default configuration") + .until(() -> rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode(), equalTo(HttpStatus.SC_OK)); HttpResponse res = rh.executeGetRequest("/_cluster/health", encodeBasicHeader("admin", "admin")); Assert.assertEquals(res.getBody(), HttpStatus.SC_OK, res.getStatusCode()); } From 65875b555a35a69a84ca1a807cf5a222e44fe866 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 14:44:17 -0500 Subject: [PATCH 17/28] Use Awaitility Signed-off-by: Craig Perkins --- .../security/InitializationIntegrationTests.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index e6ffc7f333..e124fa12ff 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -300,18 +300,20 @@ public void testInvalidDefaultConfig() throws Exception { ); final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build(); setup(Settings.EMPTY, null, settings, false); - RestHelper rh = nonSslRestHelper(); Thread.sleep(10000); Assert.assertEquals( HttpStatus.SC_SERVICE_UNAVAILABLE, - rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() + nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() ); ClusterHelper.updateDefaultDirectory(defaultInitDirectory); restart(Settings.EMPTY, null, settings, false); - rh = nonSslRestHelper(); - Thread.sleep(10000); - Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode()); + Awaitility.await() + .alias("Load default configuration") + .until( + () -> nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode(), + equalTo(HttpStatus.SC_OK) + ); } finally { ClusterHelper.resetSystemProperties(); } From 5b4ff8581b1831127091b218adfa119e2382b62c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 17:23:24 -0500 Subject: [PATCH 18/28] Temporarily adding ignore Signed-off-by: Craig Perkins --- .../org/opensearch/security/InitializationIntegrationTests.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index e124fa12ff..60356d1ec4 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -37,6 +37,7 @@ import org.apache.http.HttpStatus; import org.awaitility.Awaitility; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; @@ -293,6 +294,7 @@ public void testDefaultConfig() throws Exception { } @Test + @Ignore public void testInvalidDefaultConfig() throws Exception { try { final String defaultInitDirectory = ClusterHelper.updateDefaultDirectory( From 424a148b0d90972ecc6e4325fe6c239a3e5691bd Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 17:32:40 -0500 Subject: [PATCH 19/28] Use resetSystemProperties Signed-off-by: Craig Perkins --- .../opensearch/security/InitializationIntegrationTests.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index 60356d1ec4..a921d2bf85 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -37,7 +37,6 @@ import org.apache.http.HttpStatus; import org.awaitility.Awaitility; import org.junit.Assert; -import org.junit.Ignore; import org.junit.Test; import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; @@ -294,7 +293,6 @@ public void testDefaultConfig() throws Exception { } @Test - @Ignore public void testInvalidDefaultConfig() throws Exception { try { final String defaultInitDirectory = ClusterHelper.updateDefaultDirectory( @@ -308,7 +306,7 @@ public void testInvalidDefaultConfig() throws Exception { nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() ); - ClusterHelper.updateDefaultDirectory(defaultInitDirectory); + ClusterHelper.resetSystemProperties(); restart(Settings.EMPTY, null, settings, false); Awaitility.await() .alias("Load default configuration") From 83bb6a1791a570d7667eb3270cedd6fe9b50cbd0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 20:10:27 -0500 Subject: [PATCH 20/28] Use stop and start instead of restart Signed-off-by: Craig Perkins --- .../security/InitializationIntegrationTests.java | 7 +++++-- .../opensearch/security/test/SingleClusterTest.java | 13 +++---------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index a921d2bf85..028b7633bf 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -300,14 +300,17 @@ public void testInvalidDefaultConfig() throws Exception { ); final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build(); setup(Settings.EMPTY, null, settings, false); + Thread.sleep(10000); Assert.assertEquals( HttpStatus.SC_SERVICE_UNAVAILABLE, nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() ); - ClusterHelper.resetSystemProperties(); - restart(Settings.EMPTY, null, settings, false); + ClusterHelper.updateDefaultDirectory(defaultInitDirectory); + + stopCluster(); + setup(Settings.EMPTY, null, settings, false); Awaitility.await() .alias("Load default configuration") .until( diff --git a/src/test/java/org/opensearch/security/test/SingleClusterTest.java b/src/test/java/org/opensearch/security/test/SingleClusterTest.java index 2839e1e283..f38cc909ca 100644 --- a/src/test/java/org/opensearch/security/test/SingleClusterTest.java +++ b/src/test/java/org/opensearch/security/test/SingleClusterTest.java @@ -83,16 +83,9 @@ protected void setup( setup(initTransportClientSettings, dynamicSecuritySettings, nodeOverride, initSecurityIndex, ClusterConfiguration.DEFAULT); } - protected void restart( - Settings initTransportClientSettings, - DynamicSecurityConfig dynamicSecuritySettings, - Settings nodeOverride, - boolean initOpendistroSecurityIndex - ) throws Exception { - clusterInfo = clusterHelper.startCluster(minimumSecuritySettings(ccs(nodeOverride)), ClusterConfiguration.DEFAULT); - if (initOpendistroSecurityIndex && dynamicSecuritySettings != null) { - initialize(clusterHelper, clusterInfo, dynamicSecuritySettings); - } + protected void stopCluster() throws Exception { + clusterHelper.stopCluster(); + clusterInfo = null; } private Settings ccs(Settings nodeOverride) throws Exception { From d9912edc30b333c14ab957eeb3c167f183232b50 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 19 Dec 2023 21:49:05 -0500 Subject: [PATCH 21/28] Remove reset of default dir Signed-off-by: Craig Perkins --- .../security/InitializationIntegrationTests.java | 12 +----------- .../opensearch/security/test/SingleClusterTest.java | 5 ----- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index 028b7633bf..44b737d051 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -298,6 +298,7 @@ public void testInvalidDefaultConfig() throws Exception { final String defaultInitDirectory = ClusterHelper.updateDefaultDirectory( new File(TEST_RESOURCE_RELATIVE_PATH + "invalid_config").getAbsolutePath() ); + final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build(); setup(Settings.EMPTY, null, settings, false); @@ -306,17 +307,6 @@ public void testInvalidDefaultConfig() throws Exception { HttpStatus.SC_SERVICE_UNAVAILABLE, nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() ); - - ClusterHelper.updateDefaultDirectory(defaultInitDirectory); - - stopCluster(); - setup(Settings.EMPTY, null, settings, false); - Awaitility.await() - .alias("Load default configuration") - .until( - () -> nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode(), - equalTo(HttpStatus.SC_OK) - ); } finally { ClusterHelper.resetSystemProperties(); } diff --git a/src/test/java/org/opensearch/security/test/SingleClusterTest.java b/src/test/java/org/opensearch/security/test/SingleClusterTest.java index f38cc909ca..cdde57a5c0 100644 --- a/src/test/java/org/opensearch/security/test/SingleClusterTest.java +++ b/src/test/java/org/opensearch/security/test/SingleClusterTest.java @@ -83,11 +83,6 @@ protected void setup( setup(initTransportClientSettings, dynamicSecuritySettings, nodeOverride, initSecurityIndex, ClusterConfiguration.DEFAULT); } - protected void stopCluster() throws Exception { - clusterHelper.stopCluster(); - clusterInfo = null; - } - private Settings ccs(Settings nodeOverride) throws Exception { if (remoteClusterHelper != null) { Assert.assertNull("No remote clusters", remoteClusterInfo); From 55a99ac0e16ff4b51c045a3ea0a42a739cdc4c32 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 20 Dec 2023 09:25:35 -0500 Subject: [PATCH 22/28] Try increasing wait time to 30s Signed-off-by: Craig Perkins --- .../security/InitializationIntegrationTests.java | 11 +++++++++++ .../opensearch/security/test/SingleClusterTest.java | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index 44b737d051..3558d2f292 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -28,6 +28,7 @@ import java.io.File; import java.util.Iterator; +import java.util.concurrent.TimeUnit; import com.fasterxml.jackson.databind.JsonNode; import org.apache.hc.client5.http.classic.methods.HttpGet; @@ -307,6 +308,16 @@ public void testInvalidDefaultConfig() throws Exception { HttpStatus.SC_SERVICE_UNAVAILABLE, nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() ); + + ClusterHelper.updateDefaultDirectory(defaultInitDirectory); + restart(Settings.EMPTY, null, settings, false); + Awaitility.waitAtMost(30, TimeUnit.SECONDS) + .await() + .alias("Load default configuration") + .until( + () -> nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode(), + equalTo(HttpStatus.SC_OK) + ); } finally { ClusterHelper.resetSystemProperties(); } diff --git a/src/test/java/org/opensearch/security/test/SingleClusterTest.java b/src/test/java/org/opensearch/security/test/SingleClusterTest.java index cdde57a5c0..2839e1e283 100644 --- a/src/test/java/org/opensearch/security/test/SingleClusterTest.java +++ b/src/test/java/org/opensearch/security/test/SingleClusterTest.java @@ -83,6 +83,18 @@ protected void setup( setup(initTransportClientSettings, dynamicSecuritySettings, nodeOverride, initSecurityIndex, ClusterConfiguration.DEFAULT); } + protected void restart( + Settings initTransportClientSettings, + DynamicSecurityConfig dynamicSecuritySettings, + Settings nodeOverride, + boolean initOpendistroSecurityIndex + ) throws Exception { + clusterInfo = clusterHelper.startCluster(minimumSecuritySettings(ccs(nodeOverride)), ClusterConfiguration.DEFAULT); + if (initOpendistroSecurityIndex && dynamicSecuritySettings != null) { + initialize(clusterHelper, clusterInfo, dynamicSecuritySettings); + } + } + private Settings ccs(Settings nodeOverride) throws Exception { if (remoteClusterHelper != null) { Assert.assertNull("No remote clusters", remoteClusterInfo); From 4e32d0fd0631fcf8735a3760bbc973092e8d98c0 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 20 Dec 2023 13:53:47 -0500 Subject: [PATCH 23/28] Remote restart Signed-off-by: Craig Perkins --- .../InitializationIntegrationTests.java | 24 ++++--------------- .../security/test/SingleClusterTest.java | 12 ---------- 2 files changed, 4 insertions(+), 32 deletions(-) diff --git a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java index 3558d2f292..7545822620 100644 --- a/src/test/java/org/opensearch/security/InitializationIntegrationTests.java +++ b/src/test/java/org/opensearch/security/InitializationIntegrationTests.java @@ -28,7 +28,6 @@ import java.io.File; import java.util.Iterator; -import java.util.concurrent.TimeUnit; import com.fasterxml.jackson.databind.JsonNode; import org.apache.hc.client5.http.classic.methods.HttpGet; @@ -36,7 +35,6 @@ import org.apache.hc.core5.http.HttpVersion; import org.apache.hc.core5.http2.HttpVersionPolicy; import org.apache.http.HttpStatus; -import org.awaitility.Awaitility; import org.junit.Assert; import org.junit.Test; @@ -63,8 +61,6 @@ import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; -import static org.hamcrest.Matchers.equalTo; - public class InitializationIntegrationTests extends SingleClusterTest { @Test @@ -285,10 +281,9 @@ public void testDefaultConfig() throws Exception { final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build(); setup(Settings.EMPTY, null, settings, false); RestHelper rh = nonSslRestHelper(); + Thread.sleep(10000); - Awaitility.await() - .alias("Load default configuration") - .until(() -> rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode(), equalTo(HttpStatus.SC_OK)); + Assert.assertEquals(HttpStatus.SC_OK, rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode()); HttpResponse res = rh.executeGetRequest("/_cluster/health", encodeBasicHeader("admin", "admin")); Assert.assertEquals(res.getBody(), HttpStatus.SC_OK, res.getStatusCode()); } @@ -299,25 +294,14 @@ public void testInvalidDefaultConfig() throws Exception { final String defaultInitDirectory = ClusterHelper.updateDefaultDirectory( new File(TEST_RESOURCE_RELATIVE_PATH + "invalid_config").getAbsolutePath() ); - final Settings settings = Settings.builder().put(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build(); setup(Settings.EMPTY, null, settings, false); - + RestHelper rh = nonSslRestHelper(); Thread.sleep(10000); Assert.assertEquals( HttpStatus.SC_SERVICE_UNAVAILABLE, - nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() + rh.executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode() ); - - ClusterHelper.updateDefaultDirectory(defaultInitDirectory); - restart(Settings.EMPTY, null, settings, false); - Awaitility.waitAtMost(30, TimeUnit.SECONDS) - .await() - .alias("Load default configuration") - .until( - () -> nonSslRestHelper().executeGetRequest("", encodeBasicHeader("admin", "admin")).getStatusCode(), - equalTo(HttpStatus.SC_OK) - ); } finally { ClusterHelper.resetSystemProperties(); } diff --git a/src/test/java/org/opensearch/security/test/SingleClusterTest.java b/src/test/java/org/opensearch/security/test/SingleClusterTest.java index 2839e1e283..cdde57a5c0 100644 --- a/src/test/java/org/opensearch/security/test/SingleClusterTest.java +++ b/src/test/java/org/opensearch/security/test/SingleClusterTest.java @@ -83,18 +83,6 @@ protected void setup( setup(initTransportClientSettings, dynamicSecuritySettings, nodeOverride, initSecurityIndex, ClusterConfiguration.DEFAULT); } - protected void restart( - Settings initTransportClientSettings, - DynamicSecurityConfig dynamicSecuritySettings, - Settings nodeOverride, - boolean initOpendistroSecurityIndex - ) throws Exception { - clusterInfo = clusterHelper.startCluster(minimumSecuritySettings(ccs(nodeOverride)), ClusterConfiguration.DEFAULT); - if (initOpendistroSecurityIndex && dynamicSecuritySettings != null) { - initialize(clusterHelper, clusterInfo, dynamicSecuritySettings); - } - } - private Settings ccs(Settings nodeOverride) throws Exception { if (remoteClusterHelper != null) { Assert.assertNull("No remote clusters", remoteClusterInfo); From b3b6de74616f5b418bf608ec9c648cc8e95a8b1c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 3 Jan 2024 15:47:37 -0500 Subject: [PATCH 24/28] Add setting to delay initialization in order to write a test with clearer setup and tearDown Signed-off-by: Craig Perkins --- .../security/ConfigurationFiles.java | 1 - .../SecurityConfigurationBootstrapTests.java | 91 +++++++++++++------ .../test/framework/cluster/LocalCluster.java | 4 +- .../security/OpenSearchSecurityPlugin.java | 8 ++ .../ConfigurationRepository.java | 13 ++- .../security/support/ConfigConstants.java | 2 + 6 files changed, 85 insertions(+), 34 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java b/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java index dbd3265dfe..f3b7613aa1 100644 --- a/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java +++ b/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java @@ -31,7 +31,6 @@ public static Path createConfigurationDirectory() { String[] configurationFiles = { "config.yml", "action_groups.yml", - "config.yml", "internal_users.yml", "nodes_dn.yml", "roles.yml", diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java index 8225c686d2..5a569aa03f 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java @@ -18,14 +18,15 @@ import org.apache.commons.io.FileUtils; import org.awaitility.Awaitility; import org.junit.AfterClass; -import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; +import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; import org.opensearch.client.Client; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.support.ConfigHelper; import org.opensearch.test.framework.TestSecurityConfig.User; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.ContextHeaderDecoratorClient; @@ -33,8 +34,11 @@ import org.opensearch.test.framework.cluster.TestRestClient; import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.security.configuration.ConfigurationRepository.DEFAULT_CONFIG_VERSION; +import static org.opensearch.security.support.ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX; import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.security.support.ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @@ -46,7 +50,7 @@ public class SecurityConfigurationBootstrapTests { private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS); @ClassRule - public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) .loadConfigurationIntoIndex(false) .defaultConfigurationInitDirectory(configurationFolder.toString()) .nodeSettings( @@ -54,43 +58,72 @@ public class SecurityConfigurationBootstrapTests { SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, - true + true, + SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, + 5 ) ) .build(); - @BeforeClass - public static void runConfigUpdateRequestsInBgThread() { - - Client client = new ContextHeaderDecoratorClient( - cluster.getInternalNodeClient(), - Map.of(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true") - ); - Runnable r = new Runnable() { - public void run() { - long t = System.currentTimeMillis(); - long end = t + 10000; - while (System.currentTimeMillis() < end) { - cluster.triggerConfigurationReloadForSingleCType(client, CType.CONFIG, true); - try { - Thread.sleep(50); - } catch (InterruptedException e) { - break; - } - } - } - }; - - new Thread(r).start(); - } - @AfterClass public static void cleanConfigurationDirectory() throws IOException { FileUtils.deleteDirectory(configurationFolder.toFile()); } @Test - public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() { + public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() throws Exception { + cluster.getInternalNodeClient() + .admin() + .cluster() + .health(new ClusterHealthRequest(OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX).waitForGreenStatus()) + .actionGet(); + Client internalNodeClient = new ContextHeaderDecoratorClient( + cluster.getInternalNodeClient(), + Map.of(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true") + ); + String cd = System.getProperty("security.default_init.dir") + "/"; + ConfigHelper.uploadFile( + internalNodeClient, + cd + "action_groups.yml", + OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, + CType.ACTIONGROUPS, + DEFAULT_CONFIG_VERSION + ); + ConfigHelper.uploadFile( + internalNodeClient, + cd + "config.yml", + OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, + CType.CONFIG, + DEFAULT_CONFIG_VERSION + ); + ConfigHelper.uploadFile( + internalNodeClient, + cd + "roles.yml", + OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, + CType.ROLES, + DEFAULT_CONFIG_VERSION + ); + ConfigHelper.uploadFile( + internalNodeClient, + cd + "tenants.yml", + OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, + CType.TENANTS, + DEFAULT_CONFIG_VERSION + ); + long t = System.currentTimeMillis(); + long end = t + 10000; + while (System.currentTimeMillis() < end) { + cluster.triggerConfigurationReloadForCTypes( + internalNodeClient, + List.of(CType.ACTIONGROUPS, CType.CONFIG, CType.ROLES, CType.TENANTS), + true + ); + try { + Thread.sleep(100); + } catch (InterruptedException e) { + break; + } + } try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java index 081ba602c2..592c981c57 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java @@ -297,10 +297,10 @@ private static void triggerConfigurationReload(Client client) { } } - public void triggerConfigurationReloadForSingleCType(Client client, CType cType, boolean ignoreFailures) { + public void triggerConfigurationReloadForCTypes(Client client, List cTypes, boolean ignoreFailures) { ConfigUpdateResponse configUpdateResponse = client.execute( ConfigUpdateAction.INSTANCE, - new ConfigUpdateRequest(new String[] { cType.toLCString() }) + new ConfigUpdateRequest(cTypes.stream().map(CType::toLCString).toArray(String[]::new)) ).actionGet(); if (!ignoreFailures && configUpdateResponse.hasFailures()) { throw new RuntimeException("ConfigUpdateResponse produced failures: " + configUpdateResponse.failures()); diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 3c04816c32..0a39e73497 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1784,6 +1784,14 @@ public List> getSettings() { Property.Filtered ) ); + settings.add( + Setting.intSetting( + ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, + 0, + Property.NodeScope, + Property.Filtered + ) + ); // system integration settings.add( diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index efb5ac539d..25b1da8248 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -93,7 +93,7 @@ public class ConfigurationRepository { private final AuditLog auditLog; private final ThreadPool threadPool; private DynamicConfigFactory dynamicConfigFactory; - private static final int DEFAULT_CONFIG_VERSION = 2; + public static final int DEFAULT_CONFIG_VERSION = 2; private CompletableFuture bgThreadRunner = new CompletableFuture<>(); private final Thread bgThread; private final AtomicBoolean installDefaultConfig = new AtomicBoolean(); @@ -147,6 +147,15 @@ private ConfigurationRepository( createSecurityIndexIfAbsent(); waitForSecurityIndexToBeAtLeastYellow(); + int initializationDelay = settings.getAsInt( + ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, + 0 + ); + if (initializationDelay > 0) { + LOGGER.info("Delay initialization for {} seconds", initializationDelay); + TimeUnit.SECONDS.sleep(initializationDelay); + } + ConfigHelper.uploadFile(client, cd + "config.yml", securityIndex, CType.CONFIG, DEFAULT_CONFIG_VERSION); ConfigHelper.uploadFile(client, cd + "roles.yml", securityIndex, CType.ROLES, DEFAULT_CONFIG_VERSION); ConfigHelper.uploadFile( @@ -396,7 +405,7 @@ public boolean reloadConfiguration(Collection configTypes) throws ConfigU LOCK.unlock(); } } else { - throw new ConfigUpdateAlreadyInProgressException("A config update is already imn progress"); + throw new ConfigUpdateAlreadyInProgressException("A config update is already in progress"); } } catch (InterruptedException e) { Thread.currentThread().interrupt(); diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 2ed5e4417f..b6314f3dc1 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -281,6 +281,8 @@ public enum RolesMappingResolution { // Illegal Opcodes from here on public static final String SECURITY_UNSUPPORTED_DISABLE_REST_AUTH_INITIALLY = "plugins.security.unsupported.disable_rest_auth_initially"; + public static final String SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS = + "plugins.security.unsupported.delay_initialization_seconds"; public static final String SECURITY_UNSUPPORTED_DISABLE_INTERTRANSPORT_AUTH_INITIALLY = "plugins.security.unsupported.disable_intertransport_auth_initially"; public static final String SECURITY_UNSUPPORTED_PASSIVE_INTERTRANSPORT_AUTH_INITIALLY = From 9442ad4db11d5a5780cae3e19a6ac145ccdef155 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 3 Jan 2024 18:04:06 -0600 Subject: [PATCH 25/28] Only start initalization the configuration once Signed-off-by: Peter Nied --- .../ConfigurationRepository.java | 307 +++++++++--------- .../security/support/ConfigConstants.java | 1 - .../ConfigurationRepositoryTest.java | 8 +- 3 files changed, 154 insertions(+), 162 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 25b1da8248..a4648c2142 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -41,9 +41,9 @@ import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Supplier; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -89,14 +89,13 @@ public class ConfigurationRepository { private final List configurationChangedListener; private final ConfigurationLoaderSecurity7 cl; private final Settings settings; + private final Path configPath; private final ClusterService clusterService; private final AuditLog auditLog; private final ThreadPool threadPool; private DynamicConfigFactory dynamicConfigFactory; public static final int DEFAULT_CONFIG_VERSION = 2; - private CompletableFuture bgThreadRunner = new CompletableFuture<>(); - private final Thread bgThread; - private final AtomicBoolean installDefaultConfig = new AtomicBoolean(); + private final CompletableFuture initalizeConfigTask = new CompletableFuture<>(); private final boolean acceptInvalid; private ConfigurationRepository( @@ -112,6 +111,7 @@ private ConfigurationRepository( ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX ); this.settings = settings; + this.configPath = configPath; this.client = client; this.threadPool = threadPool; this.clusterService = clusterService; @@ -121,161 +121,150 @@ private ConfigurationRepository( cl = new ConfigurationLoaderSecurity7(client, threadPool, settings, clusterService); configCache = CacheBuilder.newBuilder().build(); + } - bgThread = new Thread(() -> { - try { - LOGGER.info("Background init thread started. Install default config?: " + installDefaultConfig.get()); - // wait for the cluster here until it will finish managed node election - while (clusterService.state().blocks().hasGlobalBlockWithStatus(RestStatus.SERVICE_UNAVAILABLE)) { - LOGGER.info("Wait for cluster to be available ..."); - TimeUnit.SECONDS.sleep(1); - } + private void initalizeClusterConfiguration(final boolean installDefaultConfig) { + try { + LOGGER.info("Background init thread started. Install default config?: " + installDefaultConfig); + // wait for the cluster here until it will finish managed node election + while (clusterService.state().blocks().hasGlobalBlockWithStatus(RestStatus.SERVICE_UNAVAILABLE)) { + LOGGER.info("Wait for cluster to be available ..."); + TimeUnit.SECONDS.sleep(1); + } - if (installDefaultConfig.get()) { + if (installDefaultConfig) { - try { - String lookupDir = System.getProperty("security.default_init.dir"); - final String cd = lookupDir != null - ? (lookupDir + "/") - : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; - File confFile = new File(cd + "config.yml"); - if (confFile.exists()) { - final ThreadContext threadContext = threadPool.getThreadContext(); - try (StoredContext ctx = threadContext.stashContext()) { - threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true"); - - createSecurityIndexIfAbsent(); - waitForSecurityIndexToBeAtLeastYellow(); - - int initializationDelay = settings.getAsInt( - ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, - 0 - ); - if (initializationDelay > 0) { - LOGGER.info("Delay initialization for {} seconds", initializationDelay); - TimeUnit.SECONDS.sleep(initializationDelay); - } - - ConfigHelper.uploadFile(client, cd + "config.yml", securityIndex, CType.CONFIG, DEFAULT_CONFIG_VERSION); - ConfigHelper.uploadFile(client, cd + "roles.yml", securityIndex, CType.ROLES, DEFAULT_CONFIG_VERSION); - ConfigHelper.uploadFile( - client, - cd + "roles_mapping.yml", - securityIndex, - CType.ROLESMAPPING, - DEFAULT_CONFIG_VERSION - ); - ConfigHelper.uploadFile( - client, - cd + "internal_users.yml", - securityIndex, - CType.INTERNALUSERS, - DEFAULT_CONFIG_VERSION - ); - ConfigHelper.uploadFile( - client, - cd + "action_groups.yml", - securityIndex, - CType.ACTIONGROUPS, - DEFAULT_CONFIG_VERSION - ); - if (DEFAULT_CONFIG_VERSION == 2) { - ConfigHelper.uploadFile( - client, - cd + "tenants.yml", - securityIndex, - CType.TENANTS, - DEFAULT_CONFIG_VERSION - ); - } - final boolean populateEmptyIfFileMissing = true; - ConfigHelper.uploadFile( - client, - cd + "nodes_dn.yml", - securityIndex, - CType.NODESDN, - DEFAULT_CONFIG_VERSION, - populateEmptyIfFileMissing - ); - ConfigHelper.uploadFile( - client, - cd + "whitelist.yml", - securityIndex, - CType.WHITELIST, - DEFAULT_CONFIG_VERSION, - populateEmptyIfFileMissing - ); - ConfigHelper.uploadFile( - client, - cd + "allowlist.yml", - securityIndex, - CType.ALLOWLIST, - DEFAULT_CONFIG_VERSION, - populateEmptyIfFileMissing - ); - - // audit.yml is not packaged by default - final String auditConfigPath = cd + "audit.yml"; - if (new File(auditConfigPath).exists()) { - ConfigHelper.uploadFile(client, auditConfigPath, securityIndex, CType.AUDIT, DEFAULT_CONFIG_VERSION); - } + try { + String lookupDir = System.getProperty("security.default_init.dir"); + final String cd = lookupDir != null + ? (lookupDir + "/") + : new Environment(settings, configPath).configDir().toAbsolutePath().toString() + "/opensearch-security/"; + File confFile = new File(cd + "config.yml"); + if (confFile.exists()) { + final ThreadContext threadContext = threadPool.getThreadContext(); + try (StoredContext ctx = threadContext.stashContext()) { + threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true"); + + createSecurityIndexIfAbsent(); + waitForSecurityIndexToBeAtLeastYellow(); + + final int initializationDelaySeconds = settings.getAsInt( + ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, + 0 + ); + if (initializationDelaySeconds > 0) { + LOGGER.error("Test setting loaded to delay initialization for {} seconds", initializationDelaySeconds); + TimeUnit.SECONDS.sleep(initializationDelaySeconds); + } + + ConfigHelper.uploadFile(client, cd + "config.yml", securityIndex, CType.CONFIG, DEFAULT_CONFIG_VERSION); + ConfigHelper.uploadFile(client, cd + "roles.yml", securityIndex, CType.ROLES, DEFAULT_CONFIG_VERSION); + ConfigHelper.uploadFile( + client, + cd + "roles_mapping.yml", + securityIndex, + CType.ROLESMAPPING, + DEFAULT_CONFIG_VERSION + ); + ConfigHelper.uploadFile( + client, + cd + "internal_users.yml", + securityIndex, + CType.INTERNALUSERS, + DEFAULT_CONFIG_VERSION + ); + ConfigHelper.uploadFile( + client, + cd + "action_groups.yml", + securityIndex, + CType.ACTIONGROUPS, + DEFAULT_CONFIG_VERSION + ); + if (DEFAULT_CONFIG_VERSION == 2) { + ConfigHelper.uploadFile(client, cd + "tenants.yml", securityIndex, CType.TENANTS, DEFAULT_CONFIG_VERSION); + } + final boolean populateEmptyIfFileMissing = true; + ConfigHelper.uploadFile( + client, + cd + "nodes_dn.yml", + securityIndex, + CType.NODESDN, + DEFAULT_CONFIG_VERSION, + populateEmptyIfFileMissing + ); + ConfigHelper.uploadFile( + client, + cd + "whitelist.yml", + securityIndex, + CType.WHITELIST, + DEFAULT_CONFIG_VERSION, + populateEmptyIfFileMissing + ); + ConfigHelper.uploadFile( + client, + cd + "allowlist.yml", + securityIndex, + CType.ALLOWLIST, + DEFAULT_CONFIG_VERSION, + populateEmptyIfFileMissing + ); + + // audit.yml is not packaged by default + final String auditConfigPath = cd + "audit.yml"; + if (new File(auditConfigPath).exists()) { + ConfigHelper.uploadFile(client, auditConfigPath, securityIndex, CType.AUDIT, DEFAULT_CONFIG_VERSION); } - } else { - LOGGER.error("{} does not exist", confFile.getAbsolutePath()); } - } catch (Exception e) { - LOGGER.error("Cannot apply default config (this is maybe not an error!)", e); + } else { + LOGGER.error("{} does not exist", confFile.getAbsolutePath()); } + } catch (Exception e) { + LOGGER.error("Cannot apply default config (this is maybe not an error!)", e); } + } - while (!dynamicConfigFactory.isInitialized()) { + while (!dynamicConfigFactory.isInitialized()) { + try { + LOGGER.debug("Try to load config ..."); + reloadConfiguration(Arrays.asList(CType.values()), true); + break; + } catch (Exception e) { + LOGGER.debug("Unable to load configuration due to {}", String.valueOf(ExceptionUtils.getRootCause(e))); try { - LOGGER.debug("Try to load config ..."); - final ThreadContext threadContext = threadPool.getThreadContext(); - try (StoredContext ctx = threadContext.stashContext()) { - threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_BG_THREAD_HEADER, true); - reloadConfiguration(Arrays.asList(CType.values())); - } + Thread.sleep(3000); + } catch (InterruptedException e1) { + Thread.currentThread().interrupt(); + LOGGER.debug("Thread was interrupted so we cancel initialization"); break; - } catch (Exception e) { - LOGGER.debug("Unable to load configuration due to {}", String.valueOf(ExceptionUtils.getRootCause(e))); - try { - Thread.sleep(3000); - } catch (InterruptedException e1) { - Thread.currentThread().interrupt(); - LOGGER.debug("Thread was interrupted so we cancel initialization"); - break; - } } } + } - final Set deprecatedAuditKeysInSettings = AuditConfig.getDeprecatedKeys(settings); + final Set deprecatedAuditKeysInSettings = AuditConfig.getDeprecatedKeys(settings); + if (!deprecatedAuditKeysInSettings.isEmpty()) { + LOGGER.warn( + "Following keys {} are deprecated in opensearch settings. They will be removed in plugin v2.0.0.0", + deprecatedAuditKeysInSettings + ); + } + final boolean isAuditConfigDocPresentInIndex = cl.isAuditConfigDocPresentInIndex(); + if (isAuditConfigDocPresentInIndex) { if (!deprecatedAuditKeysInSettings.isEmpty()) { - LOGGER.warn( - "Following keys {} are deprecated in opensearch settings. They will be removed in plugin v2.0.0.0", - deprecatedAuditKeysInSettings - ); - } - final boolean isAuditConfigDocPresentInIndex = cl.isAuditConfigDocPresentInIndex(); - if (isAuditConfigDocPresentInIndex) { - if (!deprecatedAuditKeysInSettings.isEmpty()) { - LOGGER.warn("Audit configuration settings found in both index and opensearch settings (deprecated)"); - } - LOGGER.info("Hot-reloading of audit configuration is enabled"); - } else { - LOGGER.info( - "Hot-reloading of audit configuration is disabled. Using configuration with defaults from opensearch settings. Populate the configuration in index using audit.yml or securityadmin to enable it." - ); - auditLog.setConfig(AuditConfig.from(settings)); + LOGGER.warn("Audit configuration settings found in both index and opensearch settings (deprecated)"); } - - LOGGER.info("Node '{}' initialized", clusterService.localNode().getName()); - - } catch (Exception e) { - LOGGER.error("Unexpected exception while initializing node " + e, e); + LOGGER.info("Hot-reloading of audit configuration is enabled"); + } else { + LOGGER.info( + "Hot-reloading of audit configuration is disabled. Using configuration with defaults from opensearch settings. Populate the configuration in index using audit.yml or securityadmin to enable it." + ); + auditLog.setConfig(AuditConfig.from(settings)); } - }); + LOGGER.info("Node '{}' initialized", clusterService.localNode().getName()); + + } catch (Exception e) { + LOGGER.error("Unexpected exception while initializing node " + e, e); + } } private boolean createSecurityIndexIfAbsent() { @@ -323,28 +312,32 @@ private void waitForSecurityIndexToBeAtLeastYellow() { } } - public void initOnNodeStart() { + public CompletableFuture initOnNodeStart() { + final boolean installDefaultConfig = settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false); + final Supplier> startInitialization = () -> CompletableFuture.runAsync( + () -> initalizeClusterConfiguration(installDefaultConfig) + ).thenAccept(initalizeConfigTask::complete).thenApply(result -> installDefaultConfig); try { - if (settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false)) { + if (installDefaultConfig) { LOGGER.info("Will attempt to create index {} and default configs if they are absent", securityIndex); - installDefaultConfig.set(true); - bgThreadRunner = CompletableFuture.runAsync(new AccessControllerWrappedThread(bgThread)); + return startInitialization.get(); } else if (settings.getAsBoolean(ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, true)) { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Use securityadmin to initialize cluster", securityIndex ); - bgThreadRunner = CompletableFuture.runAsync(new AccessControllerWrappedThread(bgThread)); + return startInitialization.get(); } else { LOGGER.info( "Will not attempt to create index {} and default configs if they are absent. Will not perform background initialization", securityIndex ); - bgThreadRunner.complete(null); + initalizeConfigTask.complete(null); + return initalizeConfigTask.thenApply(result -> installDefaultConfig); } } catch (Throwable e2) { LOGGER.error("Error during node initialization: {}", e2, e2); - bgThreadRunner = CompletableFuture.runAsync(new AccessControllerWrappedThread(bgThread)); + return startInitialization.get(); } } @@ -390,10 +383,14 @@ public SecurityDynamicConfiguration getConfiguration(CType configurationType) private final Lock LOCK = new ReentrantLock(); - public boolean reloadConfiguration(Collection configTypes) throws ConfigUpdateAlreadyInProgressException { - final ThreadContext threadContext = threadPool.getThreadContext(); - Boolean isBgThread = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_BG_THREAD_HEADER); - if (!Boolean.TRUE.equals(isBgThread) && !bgThreadRunner.isDone()) { + public boolean reloadConfiguration(final Collection configTypes) throws ConfigUpdateAlreadyInProgressException { + return reloadConfiguration(configTypes, false); + } + + private boolean reloadConfiguration(final Collection configTypes, final boolean fromBackgroundThread) + throws ConfigUpdateAlreadyInProgressException { + if (!fromBackgroundThread && !initalizeConfigTask.isDone()) { + LOGGER.warn("Unable to reload configuration, initalization thread has not yet completed."); return false; } try { @@ -513,10 +510,6 @@ public static int getDefaultConfigVersion() { return ConfigurationRepository.DEFAULT_CONFIG_VERSION; } - public AtomicBoolean getInstallDefaultConfig() { - return installDefaultConfig; - } - private class AccessControllerWrappedThread extends Thread { private final Thread innerThread; diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index b6314f3dc1..27dcb9a5d8 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -74,7 +74,6 @@ public class ConfigConstants { public static final String OPENDISTRO_SECURITY_MASKED_FIELD_CCS = OPENDISTRO_SECURITY_CONFIG_PREFIX + "masked_fields_ccs"; public static final String OPENDISTRO_SECURITY_CONF_REQUEST_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "conf_request"; - public static final String OPENDISTRO_SECURITY_BG_THREAD_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "bg_thread"; public static final String OPENDISTRO_SECURITY_REMOTE_ADDRESS = OPENDISTRO_SECURITY_CONFIG_PREFIX + "remote_address"; public static final String OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER = OPENDISTRO_SECURITY_CONFIG_PREFIX + "remote_address_header"; diff --git a/src/test/java/org/opensearch/security/configuration/ConfigurationRepositoryTest.java b/src/test/java/org/opensearch/security/configuration/ConfigurationRepositoryTest.java index c8f41433e0..5ce1873405 100644 --- a/src/test/java/org/opensearch/security/configuration/ConfigurationRepositoryTest.java +++ b/src/test/java/org/opensearch/security/configuration/ConfigurationRepositoryTest.java @@ -81,9 +81,9 @@ public void initOnNodeStart_withSecurityIndexCreationEnabledShouldSetInstallDefa ConfigurationRepository configRepository = createConfigurationRepository(settings); - configRepository.initOnNodeStart(); + final var result = configRepository.initOnNodeStart(); - assertThat(configRepository.getInstallDefaultConfig().get(), is(true)); + assertThat(result.join(), is(true)); } @Test @@ -92,9 +92,9 @@ public void initOnNodeStart_withSecurityIndexNotCreatedShouldNotSetInstallDefaul ConfigurationRepository configRepository = createConfigurationRepository(settings); - configRepository.initOnNodeStart(); + final var result = configRepository.initOnNodeStart(); - assertThat(configRepository.getInstallDefaultConfig().get(), is(false)); + assertThat(result.join(), is(false)); } @Test From 3fe1ddb869d64194edfd121c2b03c317292fb8c4 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 3 Jan 2024 20:11:49 -0600 Subject: [PATCH 26/28] Switch back to thread for execution of the cluster configuration to keep security manager state Signed-off-by: Peter Nied --- .../configuration/ConfigurationRepository.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index a4648c2142..87ad468268 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -314,9 +314,14 @@ private void waitForSecurityIndexToBeAtLeastYellow() { public CompletableFuture initOnNodeStart() { final boolean installDefaultConfig = settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false); - final Supplier> startInitialization = () -> CompletableFuture.runAsync( - () -> initalizeClusterConfiguration(installDefaultConfig) - ).thenAccept(initalizeConfigTask::complete).thenApply(result -> installDefaultConfig); + + final Supplier> startInitialization = () -> { + new Thread(() -> { + initalizeClusterConfiguration(installDefaultConfig); + initalizeConfigTask.complete(null); + }).start(); + return initalizeConfigTask.thenApply(result -> installDefaultConfig); + }; try { if (installDefaultConfig) { LOGGER.info("Will attempt to create index {} and default configs if they are absent", securityIndex); From 66dcfce173e740bca878cd74f410b6ad33df2941 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 3 Jan 2024 20:38:13 -0600 Subject: [PATCH 27/28] Fix spotless spacing Signed-off-by: Peter Nied --- .../security/configuration/ConfigurationRepository.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java index 87ad468268..dfbeb16cb3 100644 --- a/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java +++ b/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java @@ -314,7 +314,7 @@ private void waitForSecurityIndexToBeAtLeastYellow() { public CompletableFuture initOnNodeStart() { final boolean installDefaultConfig = settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false); - + final Supplier> startInitialization = () -> { new Thread(() -> { initalizeClusterConfiguration(installDefaultConfig); From 2f552d3763ed325ade00b263a6a757a3bb6dae56 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 4 Jan 2024 09:54:51 -0600 Subject: [PATCH 28/28] Refactor bootstrap tests to share setup steps and add prevalidation Signed-off-by: Peter Nied --- .../SecurityConfigurationBootstrapTests.java | 168 ++++++++++-------- ...rationBootstrapWithSecurityAdminTests.java | 90 ---------- .../test/framework/cluster/LocalCluster.java | 4 +- 3 files changed, 98 insertions(+), 164 deletions(-) delete mode 100644 src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java index 5a569aa03f..5b83e0d6d0 100644 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java +++ b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java @@ -11,19 +11,19 @@ import java.io.IOException; import java.nio.file.Path; +import java.time.Duration; import java.util.List; import java.util.Map; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import com.google.common.collect.ImmutableMap; import org.apache.commons.io.FileUtils; import org.awaitility.Awaitility; import org.junit.AfterClass; -import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith; import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; -import org.opensearch.client.Client; import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.ConfigHelper; @@ -33,10 +33,14 @@ import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; +import static org.apache.http.HttpStatus.SC_SERVICE_UNAVAILABLE; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.security.configuration.ConfigurationRepository.DEFAULT_CONFIG_VERSION; import static org.opensearch.security.support.ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX; import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; +import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST; import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; import static org.opensearch.security.support.ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS; import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; @@ -46,24 +50,23 @@ public class SecurityConfigurationBootstrapTests { private final static Path configurationFolder = ConfigurationFiles.createConfigurationDirectory(); - private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS); - @ClassRule - public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) - .loadConfigurationIntoIndex(false) - .defaultConfigurationInitDirectory(configurationFolder.toString()) - .nodeSettings( - Map.of( - SECURITY_RESTAPI_ROLES_ENABLED, - List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), - SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, - true, - SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, - 5 + private static LocalCluster createCluster(final Map nodeSettings) { + var cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + .loadConfigurationIntoIndex(false) + .defaultConfigurationInitDirectory(configurationFolder.toString()) + .nodeSettings( + ImmutableMap.builder() + .put(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName())) + .putAll(nodeSettings) + .build() ) - ) - .build(); + .build(); + + cluster.before(); // normally invoked by JUnit rules when run as a class rule - this starts the cluster + return cluster; + } @AfterClass public static void cleanConfigurationDirectory() throws IOException { @@ -71,65 +74,86 @@ public static void cleanConfigurationDirectory() throws IOException { } @Test - public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() throws Exception { - cluster.getInternalNodeClient() - .admin() - .cluster() - .health(new ClusterHealthRequest(OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX).waitForGreenStatus()) - .actionGet(); - Client internalNodeClient = new ContextHeaderDecoratorClient( - cluster.getInternalNodeClient(), - Map.of(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true") - ); - String cd = System.getProperty("security.default_init.dir") + "/"; - ConfigHelper.uploadFile( - internalNodeClient, - cd + "action_groups.yml", - OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, - CType.ACTIONGROUPS, - DEFAULT_CONFIG_VERSION - ); - ConfigHelper.uploadFile( - internalNodeClient, - cd + "config.yml", - OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, - CType.CONFIG, - DEFAULT_CONFIG_VERSION - ); - ConfigHelper.uploadFile( - internalNodeClient, - cd + "roles.yml", - OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, - CType.ROLES, - DEFAULT_CONFIG_VERSION - ); - ConfigHelper.uploadFile( - internalNodeClient, - cd + "tenants.yml", - OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, - CType.TENANTS, - DEFAULT_CONFIG_VERSION - ); - long t = System.currentTimeMillis(); - long end = t + 10000; - while (System.currentTimeMillis() < end) { - cluster.triggerConfigurationReloadForCTypes( - internalNodeClient, - List.of(CType.ACTIONGROUPS, CType.CONFIG, CType.ROLES, CType.TENANTS), - true - ); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - break; + public void testInitializeWithSecurityAdminWhenNoBackgroundInitialization() throws Exception { + final var nodeSettings = ImmutableMap.builder() + .put(SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false) + .put(SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, false) + .build(); + try (final LocalCluster cluster = createCluster(nodeSettings)) { + try (final TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + final var rolesMapsResponse = client.get("_plugins/_security/api/rolesmapping/readall"); + assertThat(rolesMapsResponse.getStatusCode(), equalTo(SC_SERVICE_UNAVAILABLE)); + assertThat(rolesMapsResponse.getBody(), containsString("OpenSearch Security not initialized")); + } + + final var securityAdminLauncher = new SecurityAdminLauncher(cluster.getHttpPort(), cluster.getTestCertificates()); + final int exitCode = securityAdminLauncher.runSecurityAdmin(configurationFolder); + assertThat(exitCode, equalTo(0)); + + try (final TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + Awaitility.await() + .alias("Waiting for rolemapping 'readall' availability.") + .until(() -> client.get("_plugins/_security/api/rolesmapping/readall").getStatusCode(), equalTo(200)); } } - try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { - Awaitility.await().alias("Load default configuration").until(() -> client.getAuthInfo().getStatusCode(), equalTo(200)); + } + + @Test + public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() throws Exception { + final var nodeSettings = ImmutableMap.builder() + .put(SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true) + .put(SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS, 5) + .build(); + try (final LocalCluster cluster = createCluster(nodeSettings)) { + try (final TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + cluster.getInternalNodeClient() + .admin() + .cluster() + .health(new ClusterHealthRequest(OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX).waitForGreenStatus()) + .actionGet(); - TestRestClient.HttpResponse response = client.getAuthInfo(); + // Make sure the cluster is unavalaible to authenticate with the security plugin even though it is green + final var authResponseWhenUnconfigured = client.getAuthInfo(); + authResponseWhenUnconfigured.assertStatusCode(503); - response.assertStatusCode(200); + final var internalNodeClient = new ContextHeaderDecoratorClient( + cluster.getInternalNodeClient(), + Map.of(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true") + ); + final var filesToUpload = ImmutableMap.builder() + .put("action_groups.yml", CType.ACTIONGROUPS) + .put("config.yml", CType.CONFIG) + .put("roles.yml", CType.ROLES) + .put("tenants.yml", CType.TENANTS) + .build(); + + final String defaultInitDirectory = System.getProperty("security.default_init.dir") + "/"; + filesToUpload.forEach((fileName, ctype) -> { + try { + ConfigHelper.uploadFile( + internalNodeClient, + defaultInitDirectory + fileName, + OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX, + ctype, + DEFAULT_CONFIG_VERSION + ); + } catch (final Exception ex) { + throw new RuntimeException(ex); + } + }); + + Awaitility.await().alias("Load default configuration").pollInterval(Duration.ofMillis(100)).until(() -> { + // After the configuration has been loaded, the rest clients should be able to connect successfully + cluster.triggerConfigurationReloadForCTypes( + internalNodeClient, + List.of(CType.ACTIONGROUPS, CType.CONFIG, CType.ROLES, CType.TENANTS), + true + ); + try (final TestRestClient freshClient = cluster.getRestClient(USER_ADMIN)) { + return client.getAuthInfo().getStatusCode(); + } + }, equalTo(200)); + } } } } diff --git a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java deleted file mode 100644 index 4122388684..0000000000 --- a/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapWithSecurityAdminTests.java +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - * - */ -package org.opensearch.security; - -import java.io.IOException; -import java.nio.file.Path; -import java.util.List; -import java.util.Map; - -import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; -import org.apache.commons.io.FileUtils; -import org.awaitility.Awaitility; -import org.junit.AfterClass; -import org.junit.ClassRule; -import org.junit.Rule; -import org.junit.Test; -import org.junit.rules.TemporaryFolder; -import org.junit.runner.RunWith; - -import org.opensearch.test.framework.TestSecurityConfig.User; -import org.opensearch.test.framework.cluster.ClusterManager; -import org.opensearch.test.framework.cluster.LocalCluster; -import org.opensearch.test.framework.cluster.TestRestClient; - -import static org.apache.http.HttpStatus.SC_SERVICE_UNAVAILABLE; -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.equalTo; -import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX; -import static org.opensearch.security.support.ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST; -import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; -import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; - -@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) -@ThreadLeakScope(ThreadLeakScope.Scope.NONE) -public class SecurityConfigurationBootstrapWithSecurityAdminTests { - - private final static Path configurationFolder = ConfigurationFiles.createConfigurationDirectory(); - - @Rule - public TemporaryFolder configurationDirectory = new TemporaryFolder(); - - private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS); - - @ClassRule - public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) - .loadConfigurationIntoIndex(false) - .nodeSettings( - Map.of( - SECURITY_RESTAPI_ROLES_ENABLED, - List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), - SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, - false, - SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, - false - ) - ) - .build(); - - @AfterClass - public static void cleanConfigurationDirectory() throws IOException { - FileUtils.deleteDirectory(configurationFolder.toFile()); - } - - @Test - public void testInitializeWithSecurityAdminWhenNoBackgroundInitialization() throws Exception { - try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { - TestRestClient.HttpResponse response = client.getAuthInfo(); - assertThat(response.getStatusCode(), equalTo(SC_SERVICE_UNAVAILABLE)); - assertThat(response.getBody(), containsString("OpenSearch Security not initialized")); - } - SecurityAdminLauncher securityAdminLauncher = new SecurityAdminLauncher(cluster.getHttpPort(), cluster.getTestCertificates()); - - int exitCode = securityAdminLauncher.runSecurityAdmin(configurationFolder); - - assertThat(exitCode, equalTo(0)); - try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { - Awaitility.await() - .alias("Waiting for rolemapping 'readall' availability.") - .until(() -> client.get("_plugins/_security/api/rolesmapping/readall").getStatusCode(), equalTo(200)); - } - } -} diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java index 592c981c57..217ce99a81 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java @@ -133,7 +133,7 @@ public String getSnapshotDirPath() { } @Override - public void before() throws Throwable { + public void before() { if (localOpenSearchCluster == null) { for (LocalCluster dependency : clusterDependencies) { if (!dependency.isStarted()) { @@ -155,12 +155,12 @@ public void before() throws Throwable { @Override protected void after() { - System.clearProperty(INIT_CONFIGURATION_DIR); close(); } @Override public void close() { + System.clearProperty(INIT_CONFIGURATION_DIR); if (localOpenSearchCluster != null && localOpenSearchCluster.isStarted()) { try { localOpenSearchCluster.destroy();