From 5885f6da3a291ba26679443d01d45793be054594 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 9 Aug 2024 11:07:14 +0200 Subject: [PATCH] Fix remote cluster credential secure settings reload (#111535) Due to the `cluster:admin/xpack/security` action name prefix, the internal action `cluster:admin/xpack/security/remote_cluster_credentials/reload` to reload remote cluster credentials fails for users that have the `manage` cluster privilege. This does not align with our documentation and the overall permission requirements for reloading secure settings. This PR renames the action to match the `manage` privilege. Since this is a local-only action there are no BWC concerns with this rename. Fixes: https://github.com/elastic/elasticsearch/issues/111543 --- docs/changelog/111535.yaml | 5 ++ ...nsportNodesReloadSecureSettingsAction.java | 1 + .../core/security/action/ActionTypes.java | 7 ++- ...AbstractRemoteClusterSecurityTestCase.java | 5 +- ...lusterSecurityReloadCredentialsRestIT.java | 30 ++++++++++++ .../src/javaRestTest/resources/roles.yml | 3 ++ .../xpack/security/operator/Constants.java | 2 +- .../xpack/security/SecurityTests.java | 15 ++++-- ...gsWithPasswordProtectedKeystoreRestIT.java | 46 +++++++++++++++++++ .../src/javaRestTest/resources/roles.yml | 12 +++++ 10 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 docs/changelog/111535.yaml diff --git a/docs/changelog/111535.yaml b/docs/changelog/111535.yaml new file mode 100644 index 0000000000000..4beebbf28d4e1 --- /dev/null +++ b/docs/changelog/111535.yaml @@ -0,0 +1,5 @@ +pr: 111535 +summary: Fix remote cluster credential secure settings reload +area: Authorization +type: bug +issues: [] diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java index f906b7d659b7b..82df12d9cfef7 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java @@ -123,6 +123,7 @@ protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation( final List exceptions = new ArrayList<>(); // broadcast the new settings object (with the open embedded keystore) to all reloadable plugins pluginsService.filterPlugins(ReloadablePlugin.class).forEach(p -> { + logger.debug("Reloading plugin [" + p.getClass().getSimpleName() + "]"); try { p.reload(settingsWithKeystore); } catch (final Exception e) { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ActionTypes.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ActionTypes.java index 52f8c7cf456d9..bf9484c52800a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ActionTypes.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ActionTypes.java @@ -20,8 +20,13 @@ public final class ActionTypes { private ActionTypes() {}; + // Note: this action is *not* prefixed with `cluster:admin/xpack/security` since it would otherwise be excluded from the `manage` + // privilege -- instead it matches its prefix to `TransportNodesReloadSecureSettingsAction` which is the "parent" transport action + // that invokes the overall reload flow. + // This allows us to maintain the invariant that the parent reload secure settings action can be executed with the `manage` privilege + // without trappy system-context switches. public static final ActionType RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION = new ActionType<>( - "cluster:admin/xpack/security/remote_cluster_credentials/reload" + "cluster:admin/nodes/reload_secure_settings/security/remote_cluster_credentials" ); public static final ActionType QUERY_USER_ACTION = new ActionType<>("cluster:admin/xpack/security/user/query"); diff --git a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityTestCase.java b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityTestCase.java index c4a058013caf2..2c61707f57a5a 100644 --- a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityTestCase.java +++ b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityTestCase.java @@ -48,11 +48,12 @@ public abstract class AbstractRemoteClusterSecurityTestCase extends ESRestTestCa protected static final String USER = "test_user"; protected static final SecureString PASS = new SecureString("x-pack-test-password".toCharArray()); protected static final String REMOTE_SEARCH_USER = "remote_search_user"; + protected static final String MANAGE_USER = "manage_user"; protected static final String REMOTE_METRIC_USER = "remote_metric_user"; protected static final String REMOTE_TRANSFORM_USER = "remote_transform_user"; protected static final String REMOTE_SEARCH_ROLE = "remote_search"; protected static final String REMOTE_CLUSTER_ALIAS = "my_remote_cluster"; - private static final String KEYSTORE_PASSWORD = "keystore-password"; + static final String KEYSTORE_PASSWORD = "keystore-password"; protected static LocalClusterConfigProvider commonClusterConfig = cluster -> cluster.module("analysis-common") .keystorePassword(KEYSTORE_PASSWORD) @@ -217,7 +218,7 @@ protected void removeRemoteClusterCredentials(String clusterAlias, MutableSettin } @SuppressWarnings("unchecked") - private void reloadSecureSettings() throws IOException { + protected void reloadSecureSettings() throws IOException { final Request request = new Request("POST", "/_nodes/reload_secure_settings"); request.setJsonEntity("{\"secure_settings_password\":\"" + KEYSTORE_PASSWORD + "\"}"); final Response reloadResponse = adminClient().performRequest(request); diff --git a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityReloadCredentialsRestIT.java b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityReloadCredentialsRestIT.java index a721605d228db..42982e6183613 100644 --- a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityReloadCredentialsRestIT.java +++ b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityReloadCredentialsRestIT.java @@ -34,7 +34,12 @@ import java.util.Map; import java.util.stream.Collectors; +import static org.hamcrest.Matchers.anEmptyMap; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; // account for slow stored secure settings updates (involves removing and re-creating the keystore) @TimeoutSuite(millis = 10 * TimeUnits.MINUTE) @@ -78,6 +83,7 @@ public class RemoteClusterSecurityReloadCredentialsRestIT extends AbstractRemote }) .rolesFile(Resource.fromClasspath("roles.yml")) .user(REMOTE_SEARCH_USER, PASS.toString(), "read_remote_shared_logs", false) + .user(MANAGE_USER, PASS.toString(), "manage_role", false) .build(); } @@ -237,4 +243,28 @@ private Response performRequestWithRemoteSearchUser(final Request request) throw return client().performRequest(request); } + @Override + @SuppressWarnings("unchecked") + protected void reloadSecureSettings() throws IOException { + final Request request = new Request("POST", "/_nodes/reload_secure_settings"); + request.setJsonEntity("{\"secure_settings_password\":\"" + KEYSTORE_PASSWORD + "\"}"); + // execute as user with only `manage` cluster privilege + final Response reloadResponse = performRequestWithManageUser(request); + assertOK(reloadResponse); + final Map map = entityAsMap(reloadResponse); + assertThat(map.get("nodes"), instanceOf(Map.class)); + final Map nodes = (Map) map.get("nodes"); + assertThat(nodes, is(not(anEmptyMap()))); + for (Map.Entry entry : nodes.entrySet()) { + assertThat(entry.getValue(), instanceOf(Map.class)); + final Map node = (Map) entry.getValue(); + assertThat(node.get("reload_exception"), nullValue()); + } + } + + private Response performRequestWithManageUser(final Request request) throws IOException { + request.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", headerFromRandomAuthMethod(MANAGE_USER, PASS))); + return client().performRequest(request); + } + } diff --git a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/resources/roles.yml b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/resources/roles.yml index d0487f378d34e..b61daa068ed1a 100644 --- a/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/resources/roles.yml +++ b/x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/resources/roles.yml @@ -38,3 +38,6 @@ ccr_user_role: - names: [ 'leader-index', 'shared-*', 'metrics-*' ] privileges: [ 'cross_cluster_replication' ] clusters: [ "*" ] + +manage_role: + cluster: [ 'manage' ] diff --git a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java index 04ab277fb09bf..c5304d8313df2 100644 --- a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java +++ b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java @@ -52,6 +52,7 @@ public class Constants { "cluster:admin/migration/get_system_feature", "cluster:admin/migration/post_system_feature", "cluster:admin/nodes/reload_secure_settings", + "cluster:admin/nodes/reload_secure_settings/security/remote_cluster_credentials", "cluster:admin/persistent/completion", "cluster:admin/persistent/remove", "cluster:admin/persistent/start", @@ -276,7 +277,6 @@ public class Constants { "cluster:admin/xpack/security/profile/suggest", "cluster:admin/xpack/security/profile/set_enabled", "cluster:admin/xpack/security/realm/cache/clear", - "cluster:admin/xpack/security/remote_cluster_credentials/reload", "cluster:admin/xpack/security/role/delete", "cluster:admin/xpack/security/role/get", "cluster:admin/xpack/security/role/query", diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index 400bc35b93fd5..a07a7a3a5dd27 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -14,7 +14,6 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionModule; import org.elasticsearch.action.ActionResponse; -import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.client.internal.Client; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; @@ -152,7 +151,6 @@ import static org.mockito.Mockito.when; public class SecurityTests extends ESTestCase { - private Security security = null; private ThreadContext threadContext = null; private SecurityContext securityContext = null; @@ -960,8 +958,11 @@ public List loadExtensions(Class extensionPointType) { public void testReload() throws Exception { final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build(); - final PlainActionFuture value = new PlainActionFuture<>(); + final ThreadPool threadPool = mock(ThreadPool.class); + threadContext = new ThreadContext(Settings.EMPTY); + when(threadPool.getThreadContext()).thenReturn(threadContext); final Client mockedClient = mock(Client.class); + when(mockedClient.threadPool()).thenReturn(threadPool); final JwtRealm mockedJwtRealm = mock(JwtRealm.class); final List reloadableComponents = List.of(mockedJwtRealm); @@ -992,11 +993,17 @@ protected List getReloadableSecurityComponents() { verify(mockedJwtRealm).reload(same(inputSettings)); } - public void testReloadWithFailures() throws Exception { + public void testReloadWithFailures() { final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build(); final boolean failRemoteClusterCredentialsReload = randomBoolean(); + + final ThreadPool threadPool = mock(ThreadPool.class); + threadContext = new ThreadContext(Settings.EMPTY); + when(threadPool.getThreadContext()).thenReturn(threadContext); final Client mockedClient = mock(Client.class); + when(mockedClient.threadPool()).thenReturn(threadPool); + final JwtRealm mockedJwtRealm = mock(JwtRealm.class); final List reloadableComponents = List.of(mockedJwtRealm); if (failRemoteClusterCredentialsReload) { diff --git a/x-pack/qa/password-protected-keystore/src/javaRestTest/java/org/elasticsearch/password_protected_keystore/ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT.java b/x-pack/qa/password-protected-keystore/src/javaRestTest/java/org/elasticsearch/password_protected_keystore/ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT.java index 0625ec166e32c..8e9dca396fb8e 100644 --- a/x-pack/qa/password-protected-keystore/src/javaRestTest/java/org/elasticsearch/password_protected_keystore/ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT.java +++ b/x-pack/qa/password-protected-keystore/src/javaRestTest/java/org/elasticsearch/password_protected_keystore/ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT.java @@ -7,7 +7,9 @@ package org.elasticsearch.password_protected_keystore; import org.elasticsearch.client.Request; +import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; +import org.elasticsearch.client.ResponseException; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; @@ -18,6 +20,7 @@ import org.elasticsearch.xcontent.ObjectPath; import org.junit.ClassRule; +import java.io.IOException; import java.util.Map; import static org.hamcrest.Matchers.anyOf; @@ -39,6 +42,7 @@ public class ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT extends ESR .name("javaRestTest") .keystore(nodeSpec -> Map.of("xpack.security.transport.ssl.secure_key_passphrase", "transport-password")) .setting("xpack.security.enabled", "true") + .setting("xpack.ml.enabled", "false") .setting("xpack.security.authc.anonymous.roles", "anonymous") .setting("xpack.security.transport.ssl.enabled", "true") .setting("xpack.security.transport.ssl.certificate", "transport.crt") @@ -50,6 +54,9 @@ public class ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT extends ESR .configFile("ca.crt", Resource.fromClasspath("ssl/ca.crt")) .user("admin_user", "admin-password") .user("test-user", "test-user-password", "user_role", false) + .user("manage-user", "test-user-password", "manage_role", false) + .user("manage-security-user", "test-user-password", "manage_security_role", false) + .user("monitor-user", "test-user-password", "monitor_role", false) .build(); @Override @@ -74,6 +81,33 @@ public void testReloadSecureSettingsWithCorrectPassword() throws Exception { } } + @SuppressWarnings("unchecked") + public void testReloadSecureSettingsWithDifferentPrivileges() throws Exception { + final Request request = new Request("POST", "/_nodes/reload_secure_settings"); + request.setJsonEntity("{\"secure_settings_password\":\"" + KEYSTORE_PASSWORD + "\"}"); + final Response response = performRequestWithUser("manage-user", request); + final Map map = entityAsMap(response); + assertThat(ObjectPath.eval("cluster_name", map), equalTo("javaRestTest")); + assertThat(map.get("nodes"), instanceOf(Map.class)); + final Map nodes = (Map) map.get("nodes"); + assertThat(nodes.size(), equalTo(NUM_NODES)); + for (Map.Entry entry : nodes.entrySet()) { + assertThat(entry.getValue(), instanceOf(Map.class)); + final Map node = (Map) entry.getValue(); + assertThat(node.get("reload_exception"), nullValue()); + } + expectThrows403(() -> { + final Request innerRequest = new Request("POST", "/_nodes/reload_secure_settings"); + innerRequest.setJsonEntity("{\"secure_settings_password\":\"" + KEYSTORE_PASSWORD + "\"}"); + performRequestWithUser("manage-security-user", innerRequest); + }); + expectThrows403(() -> { + final Request innerRequest = new Request("POST", "/_nodes/reload_secure_settings"); + innerRequest.setJsonEntity("{\"secure_settings_password\":\"" + KEYSTORE_PASSWORD + "\"}"); + performRequestWithUser("monitor-user", request); + }); + } + @SuppressWarnings("unchecked") public void testReloadSecureSettingsWithIncorrectPassword() throws Exception { final Request request = new Request("POST", "_nodes/reload_secure_settings"); @@ -136,4 +170,16 @@ protected Settings restAdminSettings() { String token = basicAuthHeaderValue("admin_user", new SecureString("admin-password".toCharArray())); return Settings.builder().put(ThreadContext.PREFIX + ".Authorization", token).build(); } + + private static void expectThrows403(ThrowingRunnable runnable) { + assertThat(expectThrows(ResponseException.class, runnable).getResponse().getStatusLine().getStatusCode(), equalTo(403)); + } + + private Response performRequestWithUser(final String username, final Request request) throws IOException { + request.setOptions( + RequestOptions.DEFAULT.toBuilder() + .addHeader("Authorization", basicAuthHeaderValue(username, new SecureString("test-user-password"))) + ); + return client().performRequest(request); + } } diff --git a/x-pack/qa/password-protected-keystore/src/javaRestTest/resources/roles.yml b/x-pack/qa/password-protected-keystore/src/javaRestTest/resources/roles.yml index 60dd459d0f824..c551d8200bfe0 100644 --- a/x-pack/qa/password-protected-keystore/src/javaRestTest/resources/roles.yml +++ b/x-pack/qa/password-protected-keystore/src/javaRestTest/resources/roles.yml @@ -2,3 +2,15 @@ user_role: cluster: [ALL] indices: [] + +manage_role: + cluster: [MANAGE] + indices: [] + +manage_security_role: + cluster: [MANAGE_SECURITY] + indices: [] + +monitor_role: + cluster: [MONITOR] + indices: []