From 564f63caca0d497b7dccafa5d8840d9ce727d600 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 10:18:42 +0200 Subject: [PATCH 01/12] Fix reload secure settings privileges --- ...AbstractRemoteClusterSecurityTestCase.java | 9 ++++++++- ...lusterSecurityReloadCredentialsRestIT.java | 1 + .../src/javaRestTest/resources/roles.yml | 3 +++ .../xpack/security/Security.java | 19 ++++++++++++------- 4 files changed, 24 insertions(+), 8 deletions(-) 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..2eab8a45905f9 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 @@ -10,6 +10,7 @@ import org.apache.http.HttpHost; import org.apache.http.client.methods.HttpPost; import org.elasticsearch.client.Request; +import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; import org.elasticsearch.client.RestClient; import org.elasticsearch.client.RestClientBuilder; @@ -48,6 +49,7 @@ 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"; @@ -220,7 +222,7 @@ protected void removeRemoteClusterCredentials(String clusterAlias, MutableSettin private 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); + final Response reloadResponse = performRequestWithManageUser(request); assertOK(reloadResponse); final Map map = entityAsMap(reloadResponse); assertThat(map.get("nodes"), instanceOf(Map.class)); @@ -233,6 +235,11 @@ private void reloadSecureSettings() throws IOException { } } + private Response performRequestWithManageUser(final Request request) throws IOException { + request.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", headerFromRandomAuthMethod(MANAGE_USER, PASS))); + return client().performRequest(request); + } + protected void putRemoteClusterSettings( String clusterAlias, ElasticsearchCluster targetFulfillingCluster, 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..a2f9c733ee051 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 @@ -78,6 +78,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(); } 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/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 11c688e9ee5eb..0d5b13b93fc9d 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -2202,13 +2202,18 @@ private void reloadRemoteClusterCredentials(Settings settingsWithKeystore) { return; } - final PlainActionFuture future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC); - getClient().execute( - ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION, - new TransportReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore), - future - ); - future.actionGet(); + // Run this action in system context -- it was authorized upstream and should not be tied to end-user permissions + final ThreadContext ctx = threadContext.get(); + try (ThreadContext.StoredContext ignore = ctx.stashContext()) { + ctx.markAsSystemContext(); + final PlainActionFuture future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC); + getClient().execute( + ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION, + new TransportReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore), + future + ); + future.actionGet(); + } } public Map getAuthContextForSlowLog() { From 85fccbe4c2ff8b49b75289160a121204660b77fa Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 10:20:07 +0200 Subject: [PATCH 02/12] Update docs/changelog/111535.yaml --- docs/changelog/111535.yaml | 5 +++++ 1 file changed, 5 insertions(+) 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..f764bfe745a5f --- /dev/null +++ b/docs/changelog/111535.yaml @@ -0,0 +1,5 @@ +pr: 111535 +summary: Fix reload secure settings privileges +area: Authorization +type: bug +issues: [] From b89c5076f131d38383c88eb8d97929bf05eba6c6 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 11:34:50 +0200 Subject: [PATCH 03/12] Log and tests --- .../TransportNodesReloadSecureSettingsAction.java | 1 + .../org/elasticsearch/xpack/security/Security.java | 3 ++- .../xpack/security/SecurityTests.java | 14 +++++++++++--- 3 files changed, 14 insertions(+), 4 deletions(-) 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/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 0d5b13b93fc9d..d43eddd9fda60 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -2203,7 +2203,8 @@ private void reloadRemoteClusterCredentials(Settings settingsWithKeystore) { } // Run this action in system context -- it was authorized upstream and should not be tied to end-user permissions - final ThreadContext ctx = threadContext.get(); + final ThreadContext ctx = getClient().threadPool().getThreadContext(); + assert ctx != null : "Thread context must be set for reload call"; try (ThreadContext.StoredContext ignore = ctx.stashContext()) { ctx.markAsSystemContext(); final PlainActionFuture future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC); 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..02b9ffb0acea0 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; @@ -960,8 +959,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 +994,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) { From 89381ddf88d228d9b9bf45a00e7895ebc1ec7a13 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 12:54:57 +0200 Subject: [PATCH 04/12] Fix tests --- ...AbstractRemoteClusterSecurityTestCase.java | 12 ++------ ...lusterSecurityReloadCredentialsRestIT.java | 29 +++++++++++++++++++ 2 files changed, 32 insertions(+), 9 deletions(-) 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 2eab8a45905f9..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 @@ -10,7 +10,6 @@ import org.apache.http.HttpHost; import org.apache.http.client.methods.HttpPost; import org.elasticsearch.client.Request; -import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; import org.elasticsearch.client.RestClient; import org.elasticsearch.client.RestClientBuilder; @@ -54,7 +53,7 @@ public abstract class AbstractRemoteClusterSecurityTestCase extends ESRestTestCa 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) @@ -219,10 +218,10 @@ 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 = performRequestWithManageUser(request); + final Response reloadResponse = adminClient().performRequest(request); assertOK(reloadResponse); final Map map = entityAsMap(reloadResponse); assertThat(map.get("nodes"), instanceOf(Map.class)); @@ -235,11 +234,6 @@ private void reloadSecureSettings() throws IOException { } } - private Response performRequestWithManageUser(final Request request) throws IOException { - request.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", headerFromRandomAuthMethod(MANAGE_USER, PASS))); - return client().performRequest(request); - } - protected void putRemoteClusterSettings( String clusterAlias, ElasticsearchCluster targetFulfillingCluster, 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 a2f9c733ee051..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) @@ -238,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); + } + } From 1dec1876f03cb9ea7eb778ac3b2caffde2cbb95e Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 12:57:24 +0200 Subject: [PATCH 05/12] Changelog --- docs/changelog/111535.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog/111535.yaml b/docs/changelog/111535.yaml index f764bfe745a5f..255815d3833b6 100644 --- a/docs/changelog/111535.yaml +++ b/docs/changelog/111535.yaml @@ -1,5 +1,5 @@ pr: 111535 -summary: Fix reload secure settings privileges +summary: Fix remote cluster credential secure settings reload area: Authorization type: bug -issues: [] +issues: ["https://github.com/elastic/elasticsearch/issues/111543"] From f6d905a1217b4bc7866e4a5cdf682baaba485ceb Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 13:03:47 +0200 Subject: [PATCH 06/12] Fix changelog --- docs/changelog/111535.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/111535.yaml b/docs/changelog/111535.yaml index 255815d3833b6..a47c7472f760c 100644 --- a/docs/changelog/111535.yaml +++ b/docs/changelog/111535.yaml @@ -2,4 +2,4 @@ pr: 111535 summary: Fix remote cluster credential secure settings reload area: Authorization type: bug -issues: ["https://github.com/elastic/elasticsearch/issues/111543"] +issues: [111543] From b7d560f4092afc9e70c65602e765c97724d2259d Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 13:49:41 +0200 Subject: [PATCH 07/12] REST its --- ...gsWithPasswordProtectedKeystoreRestIT.java | 38 +++++++++++++++++++ .../src/javaRestTest/resources/roles.yml | 12 ++++++ 2 files changed, 50 insertions(+) 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..73c4a9406dbe7 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,25 @@ 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(() -> performRequestWithUser("manage-security-user", request)); + expectThrows403(() -> performRequestWithUser("monitor-user", request)); + } + @SuppressWarnings("unchecked") public void testReloadSecureSettingsWithIncorrectPassword() throws Exception { final Request request = new Request("POST", "_nodes/reload_secure_settings"); @@ -136,4 +162,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: [] From 3211c9aa13fc5981acc577a041bc5e4f3c192344 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 13:51:31 +0200 Subject: [PATCH 08/12] New req instances --- ...eSettingsWithPasswordProtectedKeystoreRestIT.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 73c4a9406dbe7..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 @@ -96,8 +96,16 @@ public void testReloadSecureSettingsWithDifferentPrivileges() throws Exception { final Map node = (Map) entry.getValue(); assertThat(node.get("reload_exception"), nullValue()); } - expectThrows403(() -> performRequestWithUser("manage-security-user", request)); - expectThrows403(() -> performRequestWithUser("monitor-user", request)); + 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") From cbde0d4d91f7a4b63aa6c3d6fe1ec80a03f134ef Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Fri, 2 Aug 2024 15:02:49 +0200 Subject: [PATCH 09/12] Update docs/changelog/111535.yaml --- docs/changelog/111535.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/111535.yaml b/docs/changelog/111535.yaml index a47c7472f760c..4beebbf28d4e1 100644 --- a/docs/changelog/111535.yaml +++ b/docs/changelog/111535.yaml @@ -2,4 +2,4 @@ pr: 111535 summary: Fix remote cluster credential secure settings reload area: Authorization type: bug -issues: [111543] +issues: [] From 52639f3111180bc51ab130718ebf78a02def0e85 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Mon, 5 Aug 2024 19:09:33 +0200 Subject: [PATCH 10/12] Rename action instead --- .../core/security/action/ActionTypes.java | 7 ++++++- .../xpack/security/Security.java | 20 +++++++------------ .../xpack/security/SecurityTests.java | 1 - 3 files changed, 13 insertions(+), 15 deletions(-) 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/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index d43eddd9fda60..11c688e9ee5eb 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -2202,19 +2202,13 @@ private void reloadRemoteClusterCredentials(Settings settingsWithKeystore) { return; } - // Run this action in system context -- it was authorized upstream and should not be tied to end-user permissions - final ThreadContext ctx = getClient().threadPool().getThreadContext(); - assert ctx != null : "Thread context must be set for reload call"; - try (ThreadContext.StoredContext ignore = ctx.stashContext()) { - ctx.markAsSystemContext(); - final PlainActionFuture future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC); - getClient().execute( - ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION, - new TransportReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore), - future - ); - future.actionGet(); - } + final PlainActionFuture future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC); + getClient().execute( + ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION, + new TransportReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore), + future + ); + future.actionGet(); } public Map getAuthContextForSlowLog() { 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 02b9ffb0acea0..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 @@ -151,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; From a2bb171c3747a730565342ba815c453a6ddd3738 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Mon, 5 Aug 2024 20:55:29 +0200 Subject: [PATCH 11/12] Operator reg --- .../xpack/core/security/authz/privilege/SystemPrivilege.java | 4 +--- .../org/elasticsearch/xpack/security/operator/Constants.java | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java index 013d7cc21a54a..2616b63df7c01 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java @@ -12,7 +12,6 @@ import org.elasticsearch.index.seqno.RetentionLeaseSyncAction; import org.elasticsearch.persistent.CompletionPersistentTaskAction; import org.elasticsearch.transport.TransportActionProxy; -import org.elasticsearch.xpack.core.security.action.ActionTypes; import org.elasticsearch.xpack.core.security.support.StringMatcher; import java.util.Collections; @@ -44,8 +43,7 @@ public final class SystemPrivilege extends Privilege { "indices:data/read/*", // needed for SystemIndexMigrator "indices:admin/refresh", // needed for SystemIndexMigrator "indices:admin/aliases", // needed for SystemIndexMigrator - TransportSearchShardsAction.TYPE.name(), // added so this API can be called with the system user by other APIs - ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION.name() // needed for Security plugin reload of remote cluster credentials + TransportSearchShardsAction.TYPE.name() // added so this API can be called with the system user by other APIs ); private static final Predicate PREDICATE = (action) -> { 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 9eee5b0bd7a6f..15eda5dabc726 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 @@ -54,6 +54,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", @@ -278,7 +279,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", From b9669f162f5789eaf788af88c0611f6fffe18ac9 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Mon, 5 Aug 2024 21:52:43 +0200 Subject: [PATCH 12/12] Undo --- .../xpack/core/security/authz/privilege/SystemPrivilege.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java index 2616b63df7c01..013d7cc21a54a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/SystemPrivilege.java @@ -12,6 +12,7 @@ import org.elasticsearch.index.seqno.RetentionLeaseSyncAction; import org.elasticsearch.persistent.CompletionPersistentTaskAction; import org.elasticsearch.transport.TransportActionProxy; +import org.elasticsearch.xpack.core.security.action.ActionTypes; import org.elasticsearch.xpack.core.security.support.StringMatcher; import java.util.Collections; @@ -43,7 +44,8 @@ public final class SystemPrivilege extends Privilege { "indices:data/read/*", // needed for SystemIndexMigrator "indices:admin/refresh", // needed for SystemIndexMigrator "indices:admin/aliases", // needed for SystemIndexMigrator - TransportSearchShardsAction.TYPE.name() // added so this API can be called with the system user by other APIs + TransportSearchShardsAction.TYPE.name(), // added so this API can be called with the system user by other APIs + ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION.name() // needed for Security plugin reload of remote cluster credentials ); private static final Predicate PREDICATE = (action) -> {