Fix remote cluster credential secure settings reload (#111535) (#111738)

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: #111543 Co-authored-by: Elastic Machine <[email protected]>
elastic · Sep 17, 2024 · dc32b36 · dc32b36
1 parent 5d8a04b
commit dc32b36
Show file tree

Hide file tree

Showing 10 changed files with 118 additions and 8 deletions.
diff --git a/docs/changelog/111535.yaml 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/...sticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java b/...sticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java
@@ -123,6 +123,7 @@ protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(
             final List<Exception> 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/...k/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/ActionTypes.java b/...k/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<ActionResponse.Empty> 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<QueryUserResponse> QUERY_USER_ACTION = new ActionType<>("cluster:admin/xpack/security/user/query");

diff --git a/...est/java/org/elasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityTestCase.java b/...est/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/...a/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityReloadCredentialsRestIT.java b/...a/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<String, Object> map = entityAsMap(reloadResponse);
+        assertThat(map.get("nodes"), instanceOf(Map.class));
+        final Map<String, Object> nodes = (Map<String, Object>) map.get("nodes");
+        assertThat(nodes, is(not(anEmptyMap())));
+        for (Map.Entry<String, Object> entry : nodes.entrySet()) {
+            assertThat(entry.getValue(), instanceOf(Map.class));
+            final Map<String, Object> node = (Map<String, Object>) 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
@@ -38,3 +38,6 @@ ccr_user_role:
     - names: [ 'leader-index', 'shared-*', 'metrics-*' ]
       privileges: [ 'cross_cluster_replication' ]
       clusters: [ "*" ]
+
+manage_role:
+  cluster: [ 'manage' ]
diff --git a/...eges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java b/...eges-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",

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
@@ -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 <T> List<T> loadExtensions(Class<T> extensionPointType) {
     public void testReload() throws Exception {
         final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build();
 
-        final PlainActionFuture<ActionResponse.Empty> 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<ReloadableSecurityComponent> reloadableComponents = List.of(mockedJwtRealm);
@@ -992,11 +993,17 @@ protected List<ReloadableSecurityComponent> 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<ReloadableSecurityComponent> reloadableComponents = List.of(mockedJwtRealm);
         if (failRemoteClusterCredentialsReload) {

diff --git a/.../password_protected_keystore/ReloadSecureSettingsWithPasswordProtectedKeystoreRestIT.java b/.../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<String, Object> map = entityAsMap(response);
+        assertThat(ObjectPath.eval("cluster_name", map), equalTo("javaRestTest"));
+        assertThat(map.get("nodes"), instanceOf(Map.class));
+        final Map<String, Object> nodes = (Map<String, Object>) map.get("nodes");
+        assertThat(nodes.size(), equalTo(NUM_NODES));
+        for (Map.Entry<String, Object> entry : nodes.entrySet()) {
+            assertThat(entry.getValue(), instanceOf(Map.class));
+            final Map<String, Object> node = (Map<String, Object>) 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
@@ -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: []