Skip to content

Commit

Permalink
Allow TransportConfigUpdateAction when security config initialization…
Browse files Browse the repository at this point in the history
… has completed (#3810)

### Description

Introduces another variable on DynamicConfigFactory called
`bgThreadComplete` that behaves differently than the `initialized`
variable. `bgThreadComplete` is a flag that signals to
TransportConfigUpdateAction that it can start accepting updates.

There are 2 ways the security index can be created from scratch:

1. If `plugins.security.allow_default_init_securityindex` is set to
**true** it will create the security index and load all yaml files
2. If `plugins.security.allow_default_init_securityindex` is set to
**false**, the security index is not created on bootstrap and requires a
user to run securityadmin to initialize security. When securityadmin is
utilized, the cluster does depend on
[TransportConfigUpdateAction](https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/tools/SecurityAdmin.java#L975-L977)
to initialize security so there still needs to be an avenue where this
can update the config before `initialized` is set to **true**

This PR sets `bgThreadComplete` to **false** on node startup and
explicitly sets it to **true** once its ok for
TransportConfigUpdateAction to start accepting transport actions. In
case 2) up above, it can be set to **true** before DynamicConfigFactory
is `initialized` so that it can accept requests from securityadmin.

* Category (Enhancement, New feature, Bug fix, Test fix, Refactoring,
Maintenance, Documentation)

Bug fix

### Issues Resolved

- Resolves #3204

### Check List
- [X] New functionality includes testing
- [ ] New functionality has been documented
- [X] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

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]>
  • Loading branch information
3 people authored Jan 5, 2024
1 parent 03fd79f commit 045d4ef
Show file tree
Hide file tree
Showing 15 changed files with 396 additions and 173 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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();
Expand Down Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Loading

0 comments on commit 045d4ef

Please sign in to comment.