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

Allow TransportConfigUpdateAction when security config initialization has completed #3810

Merged
merged 33 commits into from
Jan 5, 2024
Merged
Show file tree
Hide file tree
Changes from 10 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 @@ -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",
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
"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,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)
cwperks marked this conversation as resolved.
Show resolved Hide resolved
public class SecurityConfigurationBootstrapTests {
peternied marked this conversation as resolved.
Show resolved Hide resolved

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();
peternied marked this conversation as resolved.
Show resolved Hide resolved
}

@AfterClass
public static void cleanConfigurationDirectory() throws IOException {
FileUtils.deleteDirectory(configurationFolder.toFile());
}

@Test
public void shouldStillLoadSecurityConfigDuringBootstrapAndActiveConfigUpdateRequests() {
peternied marked this conversation as resolved.
Show resolved Hide resolved
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);
}
}
}
Original file line number Diff line number Diff line change
@@ -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));
}
cwperks marked this conversation as resolved.
Show resolved Hide resolved
}
}
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 @@ -297,6 +297,16 @@ private static void triggerConfigurationReload(Client client) {
}
}

public void triggerConfigurationReloadForSingleCType(Client client, CType cType, boolean ignoreFailures) {
peternied marked this conversation as resolved.
Show resolved Hide resolved
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();
}
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();
if (configurationRepository.isBgThreadComplete()) {
configurationRepository.reloadConfiguration(CType.fromStringValues((request.request.getConfigTypes())));
peternied marked this conversation as resolved.
Show resolved Hide resolved
backendRegistry.get().invalidateCache();
}
return new ConfigUpdateNodeResponse(clusterService.localNode(), request.request.getConfigTypes(), null);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@

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;
Expand All @@ -37,6 +39,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;
Expand Down Expand Up @@ -91,6 +94,7 @@ public class ConfigurationRepository {
private final ThreadPool threadPool;
private DynamicConfigFactory dynamicConfigFactory;
private static final int DEFAULT_CONFIG_VERSION = 2;
private CompletableFuture<Void> bgThreadRunner = new CompletableFuture<>();
private final Thread bgThread;
private final AtomicBoolean installDefaultConfig = new AtomicBoolean();
private final boolean acceptInvalid;
Expand Down Expand Up @@ -311,22 +315,23 @@ 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(AccessController.doPrivileged((PrivilegedAction<Thread>) () -> 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(AccessController.doPrivileged((PrivilegedAction<Thread>) () -> bgThread));
} 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);
}
} catch (Throwable e2) {
LOGGER.error("Error during node initialization: {}", e2, e2);
bgThread.start();
bgThreadRunner = CompletableFuture.runAsync(AccessController.doPrivileged((PrivilegedAction<Thread>) () -> bgThread));
}
}

Expand Down Expand Up @@ -372,6 +377,10 @@ public SecurityDynamicConfiguration<?> getConfiguration(CType configurationType)

private final Lock LOCK = new ReentrantLock();

public boolean isBgThreadComplete() {
return bgThreadRunner.isDone();
}

public void reloadConfiguration(Collection<CType> configTypes) throws ConfigUpdateAlreadyInProgressException {
try {
if (LOCK.tryLock(60, TimeUnit.SECONDS)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,6 @@ public void onChange(Map<CType, SecurityDynamicConfiguration<?>> typeToConfig) {
}

initialized.set(true);

}

private static ConfigV6 getConfigV6(SecurityDynamicConfiguration<?> sdc) {
Expand Down
Loading