[Backport 2.x] Allow TransportConfigUpdateAction when security config…

… initialization has completed (opensearch-project#3810) Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> (cherry picked from commit 045d4ef)
peternied · Jan 5, 2024 · 2d5a169 · 2d5a169
1 parent 13a5641
commit 2d5a169
Show file tree

Hide file tree

Showing 15 changed files with 511 additions and 167 deletions.
diff --git a/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java b/src/integrationTest/java/org/opensearch/security/ConfigurationFiles.java
@@ -31,12 +31,13 @@ public static Path createConfigurationDirectory() {
             String[] configurationFiles = {
                 "config.yml",
                 "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
@@ -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/SecurityConfigurationBootstrapTests.java b/src/integrationTest/java/org/opensearch/security/SecurityConfigurationBootstrapTests.java
@@ -0,0 +1,159 @@
+/*
+ * 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.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.Test;
+import org.junit.runner.RunWith;
+
+import org.opensearch.action.admin.cluster.health.ClusterHealthRequest;
+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;
+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;
+
+@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);
+
+    private static LocalCluster createCluster(final Map<String, Object> nodeSettings) {
+        var cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
+            .loadConfigurationIntoIndex(false)
+            .defaultConfigurationInitDirectory(configurationFolder.toString())
+            .nodeSettings(
+                ImmutableMap.<String, Object>builder()
+                    .put(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()))
+                    .putAll(nodeSettings)
+                    .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 {
+        FileUtils.deleteDirectory(configurationFolder.toFile());
+    }
+
+    @Test
+    public void testInitializeWithSecurityAdminWhenNoBackgroundInitialization() throws Exception {
+        final var nodeSettings = ImmutableMap.<String, Object>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));
+            }
+        }
+    }
+
+    @Test
+    public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() throws Exception {
+        final var nodeSettings = ImmutableMap.<String, Object>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();
+
+                // 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);
+
+                final var internalNodeClient = new ContextHeaderDecoratorClient(
+                    cluster.getInternalNodeClient(),
+                    Map.of(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true")
+                );
+                final var filesToUpload = ImmutableMap.<String, CType>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/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();

diff --git 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
@@ -46,7 +46,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;
@@ -126,8 +126,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,

diff --git 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();
@@ -297,6 +297,16 @@ private static void triggerConfigurationReload(Client client) {
         }
     }
 
+    public void triggerConfigurationReloadForCTypes(Client client, List<CType> cTypes, boolean ignoreFailures) {
+        ConfigUpdateResponse configUpdateResponse = client.execute(
+            ConfigUpdateAction.INSTANCE,
+            new ConfigUpdateRequest(cTypes.stream().map(CType::toLCString).toArray(String[]::new))
+        ).actionGet();
+        if (!ignoreFailures && configUpdateResponse.hasFailures()) {
+            throw new RuntimeException("ConfigUpdateResponse produced failures: " + configUpdateResponse.failures());
+        }
+    }
+
     public CertificateData getAdminCertificate() {
         return testCertificates.getAdminCertificateData();
     }

diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -1791,6 +1791,14 @@ public List<Setting<?>> 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/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();
+        boolean didReload = configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes())));
+        if (didReload) {
+            backendRegistry.get().invalidateCache();
+        }
         return new ConfigUpdateNodeResponse(clusterService.localNode(), request.request.getConfigTypes(), null);
     }