Fix trappy timeouts in security settings APIs (elastic#109233)

Relates elastic#107984
jimczi · Jun 10, 2024 · 8b759a1 · 8b759a1
1 parent 683245e
commit 8b759a1
Show file tree

Hide file tree

Showing 13 changed files with 169 additions and 44 deletions.
diff --git a/docs/changelog/109233.yaml b/docs/changelog/109233.yaml
@@ -0,0 +1,5 @@
+pr: 109233
+summary: Fix trappy timeouts in security settings APIs
+area: Security
+type: bug
+issues: []
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/security.get_settings.json b/rest-api-spec/src/main/resources/rest-api-spec/api/security.get_settings.json
@@ -18,6 +18,11 @@
         }
       ]
     },
-    "params":{}
+    "params":{
+      "master_timeout":{
+        "type":"time",
+        "description":"Timeout for connection to master"
+      }
+    }
   }
 }
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/api/security.update_settings.json b/rest-api-spec/src/main/resources/rest-api-spec/api/security.update_settings.json
@@ -18,7 +18,16 @@
         }
       ]
     },
-    "params":{},
+    "params":{
+      "master_timeout":{
+        "type":"time",
+        "description":"Timeout for connection to master"
+      },
+      "timeout":{
+        "type":"time",
+        "description":"Timeout for acknowledgements from all nodes"
+      }
+    },
     "body":{
       "description": "An object with the new settings for each index, if any",
       "required": true

diff --git a/server/src/main/java/org/elasticsearch/TransportVersions.java b/server/src/main/java/org/elasticsearch/TransportVersions.java
@@ -186,6 +186,8 @@ static TransportVersion def(int id) {
     public static final TransportVersion ML_CHUNK_INFERENCE_OPTION = def(8_677_00_0);
     public static final TransportVersion RANK_FEATURE_PHASE_ADDED = def(8_678_00_0);
     public static final TransportVersion RANK_DOC_IN_SHARD_FETCH_REQUEST = def(8_679_00_0);
+    public static final TransportVersion SECURITY_SETTINGS_REQUEST_TIMEOUTS = def(8_680_00_0);
+
     /*
      * STOP! READ THIS FIRST! No, really,
      *        ____ _____ ___  ____  _        ____  _____    _    ____    _____ _   _ ___ ____    _____ ___ ____  ____ _____ _

diff --git a/...java/org/elasticsearch/xpack/core/security/action/settings/GetSecuritySettingsAction.java b/...java/org/elasticsearch/xpack/core/security/action/settings/GetSecuritySettingsAction.java
@@ -7,13 +7,16 @@
 
 package org.elasticsearch.xpack.core.security.action.settings;
 
+import org.elasticsearch.TransportVersions;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.action.ActionType;
 import org.elasticsearch.action.support.master.MasterNodeReadRequest;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.xcontent.ToXContentObject;
 import org.elasticsearch.xcontent.XContentBuilder;
 
@@ -23,27 +26,39 @@
 import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.PROFILES_INDEX_NAME;
 import static org.elasticsearch.xpack.core.security.action.settings.UpdateSecuritySettingsAction.TOKENS_INDEX_NAME;
 
-public class GetSecuritySettingsAction extends ActionType<GetSecuritySettingsAction.Response> {
+public class GetSecuritySettingsAction {
 
-    public static final GetSecuritySettingsAction INSTANCE = new GetSecuritySettingsAction();
-    public static final String NAME = "cluster:admin/xpack/security/settings/get";
+    public static final ActionType<GetSecuritySettingsAction.Response> INSTANCE = new ActionType<>(
+        "cluster:admin/xpack/security/settings/get"
+    );
 
-    public GetSecuritySettingsAction() {
-        super(NAME);
-    }
+    private GetSecuritySettingsAction() {/* no instances */}
 
     public static class Request extends MasterNodeReadRequest<GetSecuritySettingsAction.Request> {
 
-        public Request() {
-            super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
+        public Request(TimeValue masterNodeTimeout) {
+            super(masterNodeTimeout);
+        }
+
+        @UpdateForV9 // no need for bwc any more, this can be inlined
+        public static Request readFrom(StreamInput in) throws IOException {
+            if (in.getTransportVersion().onOrAfter(TransportVersions.SECURITY_SETTINGS_REQUEST_TIMEOUTS)) {
+                return new Request(in);
+            } else {
+                return new Request(TimeValue.THIRTY_SECONDS);
+            }
         }
 
-        public Request(StreamInput in) throws IOException {
-            super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT);
+        private Request(StreamInput in) throws IOException {
+            super(in);
         }
 
         @Override
-        public void writeTo(StreamOutput out) throws IOException {}
+        public void writeTo(StreamOutput out) throws IOException {
+            if (out.getTransportVersion().onOrAfter(TransportVersions.SECURITY_SETTINGS_REQUEST_TIMEOUTS)) {
+                super.writeTo(out);
+            }
+        }
 
         @Override
         public ActionRequestValidationException validate() {

diff --git a/...a/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsAction.java b/...a/org/elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsAction.java
@@ -7,6 +7,7 @@
 
 package org.elasticsearch.xpack.core.security.action.settings;
 
+import org.elasticsearch.TransportVersions;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.ActionType;
 import org.elasticsearch.action.ValidateActions;
@@ -16,6 +17,8 @@
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.util.set.Sets;
+import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.xcontent.ConstructingObjectParser;
 import org.elasticsearch.xcontent.ParseField;
 import org.elasticsearch.xcontent.XContentParser;
@@ -28,9 +31,9 @@
 
 import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
-public class UpdateSecuritySettingsAction extends ActionType<AcknowledgedResponse> {
-    public static final UpdateSecuritySettingsAction INSTANCE = new UpdateSecuritySettingsAction();
-    public static final String NAME = "cluster:admin/xpack/security/settings/update";
+public class UpdateSecuritySettingsAction {
+
+    public static final ActionType<AcknowledgedResponse> INSTANCE = new ActionType<>("cluster:admin/xpack/security/settings/update");
 
     // The names here are separate constants for 2 reasons:
     // 1. Keeping the names defined here helps ensure REST compatibility, even if the internal aliases of these indices change,
@@ -44,21 +47,27 @@ public class UpdateSecuritySettingsAction extends ActionType<AcknowledgedRespons
         IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS
     );
 
-    public UpdateSecuritySettingsAction() {
-        super(NAME);
-    }
+    private UpdateSecuritySettingsAction() {/* no instances */}
 
     public static class Request extends AcknowledgedRequest<Request> {
 
         private final Map<String, Object> mainIndexSettings;
         private final Map<String, Object> tokensIndexSettings;
         private final Map<String, Object> profilesIndexSettings;
 
+        public interface Factory {
+            Request create(
+                Map<String, Object> mainIndexSettings,
+                Map<String, Object> tokensIndexSettings,
+                Map<String, Object> profilesIndexSettings
+            );
+        }
+
         @SuppressWarnings("unchecked")
-        private static final ConstructingObjectParser<Request, Void> PARSER = new ConstructingObjectParser<>(
+        private static final ConstructingObjectParser<Request, Factory> PARSER = new ConstructingObjectParser<>(
             "update_security_settings_request",
             false,
-            a -> new Request((Map<String, Object>) a[0], (Map<String, Object>) a[1], (Map<String, Object>) a[2])
+            (a, factory) -> factory.create((Map<String, Object>) a[0], (Map<String, Object>) a[1], (Map<String, Object>) a[2])
         );
 
         static {
@@ -68,32 +77,53 @@ public static class Request extends AcknowledgedRequest<Request> {
         }
 
         public Request(
+            TimeValue masterNodeTimeout,
+            TimeValue ackTimeout,
             Map<String, Object> mainIndexSettings,
             Map<String, Object> tokensIndexSettings,
             Map<String, Object> profilesIndexSettings
         ) {
-            super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
+            super(masterNodeTimeout, ackTimeout);
             this.mainIndexSettings = Objects.requireNonNullElse(mainIndexSettings, Collections.emptyMap());
             this.tokensIndexSettings = Objects.requireNonNullElse(tokensIndexSettings, Collections.emptyMap());
             this.profilesIndexSettings = Objects.requireNonNullElse(profilesIndexSettings, Collections.emptyMap());
         }
 
-        public Request(StreamInput in) throws IOException {
-            super(TRAPPY_IMPLICIT_DEFAULT_MASTER_NODE_TIMEOUT, DEFAULT_ACK_TIMEOUT);
+        @UpdateForV9 // no need for bwc any more, this can be inlined
+        public static Request readFrom(StreamInput in) throws IOException {
+            if (in.getTransportVersion().onOrAfter(TransportVersions.SECURITY_SETTINGS_REQUEST_TIMEOUTS)) {
+                return new Request(in);
+            } else {
+                return new Request(TimeValue.THIRTY_SECONDS, TimeValue.THIRTY_SECONDS, in);
+            }
+        }
+
+        private Request(StreamInput in) throws IOException {
+            super(in);
+            this.mainIndexSettings = in.readGenericMap();
+            this.tokensIndexSettings = in.readGenericMap();
+            this.profilesIndexSettings = in.readGenericMap();
+        }
+
+        private Request(TimeValue masterNodeTimeout, TimeValue ackTimeout, StreamInput in) throws IOException {
+            super(masterNodeTimeout, ackTimeout);
             this.mainIndexSettings = in.readGenericMap();
             this.tokensIndexSettings = in.readGenericMap();
             this.profilesIndexSettings = in.readGenericMap();
         }
 
         @Override
         public void writeTo(StreamOutput out) throws IOException {
+            if (out.getTransportVersion().onOrAfter(TransportVersions.SECURITY_SETTINGS_REQUEST_TIMEOUTS)) {
+                super.writeTo(out);
+            }
             out.writeGenericMap(this.mainIndexSettings);
             out.writeGenericMap(this.tokensIndexSettings);
             out.writeGenericMap(this.profilesIndexSettings);
         }
 
-        public static Request parse(XContentParser parser) {
-            return PARSER.apply(parser, null);
+        public static Request parse(XContentParser parser, Factory factory) {
+            return PARSER.apply(parser, factory);
         }
 
         public Map<String, Object> mainIndexSettings() {

diff --git a/.../java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilegeResolver.java b/.../java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilegeResolver.java
@@ -269,7 +269,7 @@ public class ClusterPrivilegeResolver {
             ActionTypes.QUERY_USER_ACTION.name(),
             GetUserPrivilegesAction.NAME, // normally authorized under the "same-user" authz check, but added here for uniformity
             HasPrivilegesAction.NAME,
-            GetSecuritySettingsAction.NAME
+            GetSecuritySettingsAction.INSTANCE.name()
         )
     );
     public static final NamedClusterPrivilege MANAGE_SAML = new ActionClusterPrivilege("manage_saml", MANAGE_SAML_PATTERN);

diff --git a/.../elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsActionTests.java b/.../elasticsearch/xpack/core/security/action/settings/UpdateSecuritySettingsActionTests.java
@@ -28,7 +28,13 @@
 public class UpdateSecuritySettingsActionTests extends ESTestCase {
 
     public void testValidateSettingsEmpty() {
-        var req = new UpdateSecuritySettingsAction.Request(Collections.emptyMap(), Collections.emptyMap(), Collections.emptyMap());
+        var req = new UpdateSecuritySettingsAction.Request(
+            TEST_REQUEST_TIMEOUT,
+            TEST_REQUEST_TIMEOUT,
+            Collections.emptyMap(),
+            Collections.emptyMap(),
+            Collections.emptyMap()
+        );
         var ex = req.validate();
         assertThat(ex, notNullValue());
         assertThat(ex.getMessage(), containsString("No settings given to update"));
@@ -40,17 +46,41 @@ public void testAllowedSettingsOk() {
         for (String allowedSetting : ALLOWED_SETTING_KEYS) {
             Map<String, Object> allowedSettingMap = Map.of(allowedSetting, randomAlphaOfLength(5));
             allAllowedSettingsMap.put(allowedSetting, randomAlphaOfLength(5));
-            var req = new UpdateSecuritySettingsAction.Request(allowedSettingMap, Collections.emptyMap(), Collections.emptyMap());
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                allowedSettingMap,
+                Collections.emptyMap(),
+                Collections.emptyMap()
+            );
             assertThat(req.validate(), nullValue());
 
-            req = new UpdateSecuritySettingsAction.Request(Collections.emptyMap(), allowedSettingMap, Collections.emptyMap());
+            req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                Collections.emptyMap(),
+                allowedSettingMap,
+                Collections.emptyMap()
+            );
             assertThat(req.validate(), nullValue());
 
-            req = new UpdateSecuritySettingsAction.Request(Collections.emptyMap(), Collections.emptyMap(), allowedSettingMap);
+            req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                Collections.emptyMap(),
+                Collections.emptyMap(),
+                allowedSettingMap
+            );
             assertThat(req.validate(), nullValue());
         }
 
-        var req = new UpdateSecuritySettingsAction.Request(allAllowedSettingsMap, allAllowedSettingsMap, allAllowedSettingsMap);
+        var req = new UpdateSecuritySettingsAction.Request(
+            TEST_REQUEST_TIMEOUT,
+            TEST_REQUEST_TIMEOUT,
+            allAllowedSettingsMap,
+            allAllowedSettingsMap,
+            allAllowedSettingsMap
+        );
         assertThat(req.validate(), nullValue());
     }
 
@@ -63,7 +93,13 @@ public void testDisallowedSettingsFailsValidation() {
             Map.of(randomFrom(ALLOWED_SETTING_KEYS), randomAlphaOfLength(5))
         );
         {
-            var req = new UpdateSecuritySettingsAction.Request(validOrEmptySettingMap, disallowedSettingMap, validOrEmptySettingMap);
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                validOrEmptySettingMap,
+                disallowedSettingMap,
+                validOrEmptySettingMap
+            );
             List<String> errors = req.validate().validationErrors();
             assertThat(errors, hasSize(1));
             for (String errorMsg : errors) {
@@ -81,7 +117,13 @@ public void testDisallowedSettingsFailsValidation() {
         }
 
         {
-            var req = new UpdateSecuritySettingsAction.Request(disallowedSettingMap, validOrEmptySettingMap, disallowedSettingMap);
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                disallowedSettingMap,
+                validOrEmptySettingMap,
+                disallowedSettingMap
+            );
             List<String> errors = req.validate().validationErrors();
             assertThat(errors, hasSize(2));
             for (String errorMsg : errors) {
@@ -101,7 +143,13 @@ public void testDisallowedSettingsFailsValidation() {
         }
 
         {
-            var req = new UpdateSecuritySettingsAction.Request(disallowedSettingMap, disallowedSettingMap, disallowedSettingMap);
+            var req = new UpdateSecuritySettingsAction.Request(
+                TEST_REQUEST_TIMEOUT,
+                TEST_REQUEST_TIMEOUT,
+                disallowedSettingMap,
+                disallowedSettingMap,
+                disallowedSettingMap
+            );
             List<String> errors = req.validate().validationErrors();
             assertThat(errors, hasSize(3));
             for (String errorMsg : errors) {

diff --git a/...e/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java b/...e/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java
@@ -285,7 +285,7 @@ public void testReadSecurityPrivilege() {
             ActionTypes.QUERY_USER_ACTION.name(),
             HasPrivilegesAction.NAME,
             GetUserPrivilegesAction.NAME,
-            GetSecuritySettingsAction.NAME
+            GetSecuritySettingsAction.INSTANCE.name()
         );
         verifyClusterActionAllowed(
             ClusterPrivilegeResolver.READ_SECURITY,
@@ -321,7 +321,7 @@ public void testReadSecurityPrivilege() {
             ActivateProfileAction.NAME,
             SetProfileEnabledAction.NAME,
             UpdateProfileDataAction.NAME,
-            UpdateSecuritySettingsAction.NAME
+            UpdateSecuritySettingsAction.INSTANCE.name()
         );
     }
 

diff --git a/.../org/elasticsearch/xpack/security/action/settings/TransportGetSecuritySettingsAction.java b/.../org/elasticsearch/xpack/security/action/settings/TransportGetSecuritySettingsAction.java
@@ -47,12 +47,12 @@ public TransportGetSecuritySettingsAction(
         IndexNameExpressionResolver indexNameExpressionResolver
     ) {
         super(
-            GetSecuritySettingsAction.NAME,
+            GetSecuritySettingsAction.INSTANCE.name(),
             transportService,
             clusterService,
             threadPool,
             actionFilters,
-            GetSecuritySettingsAction.Request::new,
+            GetSecuritySettingsAction.Request::readFrom,
             indexNameExpressionResolver,
             GetSecuritySettingsAction.Response::new,
             EsExecutors.DIRECT_EXECUTOR_SERVICE

diff --git a/...g/elasticsearch/xpack/security/action/settings/TransportUpdateSecuritySettingsAction.java b/...g/elasticsearch/xpack/security/action/settings/TransportUpdateSecuritySettingsAction.java
@@ -57,12 +57,12 @@ public TransportUpdateSecuritySettingsAction(
         IndexNameExpressionResolver indexNameExpressionResolver
     ) {
         super(
-            UpdateSecuritySettingsAction.NAME,
+            UpdateSecuritySettingsAction.INSTANCE.name(),
             transportService,
             clusterService,
             threadPool,
             actionFilters,
-            UpdateSecuritySettingsAction.Request::new,
+            UpdateSecuritySettingsAction.Request::readFrom,
             indexNameExpressionResolver,
             AcknowledgedResponse::readFrom,
             EsExecutors.DIRECT_EXECUTOR_SERVICE