Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only allow TransportConfigUpdateAction after node has been set as bootstrapped #13

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7ba39c9
Only allow TransportConfigUpdateAction after node has been set as boo…
cwperks Dec 6, 2023
d05da1a
Set in finally block and change name to bgThreadComplete
cwperks Dec 8, 2023
58d6f45
Add tests that ensure the bg thread completes during bootstrap
cwperks Dec 8, 2023
c2423b7
Add test to provide initialize with securityadmin works
cwperks Dec 8, 2023
d86feed
Set background init to false to ensure that transport config update a…
cwperks Dec 8, 2023
9606f38
Update SecurityConfigurationTests
cwperks Dec 8, 2023
0b764f1
Use CompletableFuture
cwperks Dec 11, 2023
83961f0
Wrap in doPrivileged
cwperks Dec 11, 2023
b211302
doPrivileged
cwperks Dec 11, 2023
0613161
Wrap with doPrivileged
cwperks Dec 11, 2023
258fb28
Assert not initialized
cwperks Dec 11, 2023
91cf472
Rename beforeClass method
cwperks Dec 11, 2023
06b7513
Add AccessControllerWrappedThread
cwperks Dec 11, 2023
a3398e0
Use threadContext to carry context
cwperks Dec 12, 2023
07902e2
Merge branch 'main' into initialization-fix
cwperks Dec 14, 2023
d80e98d
Merge branch 'main' into initialization-fix
cwperks Dec 19, 2023
67272e0
Merge branch 'initialization-fix' of https://github.com/cwperks/secur…
cwperks Dec 19, 2023
7b73715
Small update
cwperks Dec 19, 2023
4b99681
Use Awaitility
cwperks Dec 19, 2023
65875b5
Use Awaitility
cwperks Dec 19, 2023
5b4ff85
Temporarily adding ignore
cwperks Dec 19, 2023
424a148
Use resetSystemProperties
cwperks Dec 19, 2023
83bb6a1
Use stop and start instead of restart
cwperks Dec 20, 2023
d9912ed
Remove reset of default dir
cwperks Dec 20, 2023
55a99ac
Try increasing wait time to 30s
cwperks Dec 20, 2023
4e32d0f
Remote restart
cwperks Dec 20, 2023
b74d7f4
Merge branch 'main' into initialization-fix
cwperks Dec 22, 2023
b3b6de7
Add setting to delay initialization in order to write a test with cle…
cwperks Jan 3, 2024
9442ad4
Only start initalization the configuration once
peternied Jan 4, 2024
3fe1ddb
Switch back to thread for execution of the cluster configuration to k…
peternied Jan 4, 2024
66dcfce
Fix spotless spacing
peternied Jan 4, 2024
2f552d3
Refactor bootstrap tests to share setup steps and add prevalidation
peternied Jan 4, 2024
4d53edc
Merge remote-tracking branches 'peternied/initialization-fix' and 'pe…
peternied Jan 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -1784,6 +1784,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
Loading