Ensure secureString remain open when reloading secure settings (#88922)

The reloading secure settings action sends one node level request with
password (secureString) to each node. These node level requests share
the same secureString instance. This is not a problem when the requests
are sent across the wire because the secureString will end up to be
independent instances after de/serilization. But when the request is
handled by the local node, it skips the de/serialization process. This
means when the secureString gets closed, it is closed for all the node
level requests. If a node level request has not been sent across wire
when the secureString is closed under it, the serialization process will
result into error.

This PR fixes the bug by letting each Node level request creates a clone
of the secureString and have the Nodes level request to track all Node
level requests. All copies of secureString (on the coordinate node) will
be closed at Nodes request level which is safe because it is after
completion of all Node level requests.

Resolves: #88887

docs/changelog/88922.yaml

@@ -0,0 +1,5 @@
+pr: 88922
+summary: Ensure `secureString` remain open when reloading secure settings
+area: Security
+type: bug
+issues: []

server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java

@@ -16,14 +16,18 @@ import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.core.CharArrays;
 import org.elasticsearch.core.Nullable;
+import org.elasticsearch.core.Releasable;
+import org.elasticsearch.transport.TransportRequest;
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * Request for a reload secure settings action
  */
-public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesReloadSecureSettingsRequest> {
+public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesReloadSecureSettingsRequest> implements Releasable {
 
     /**
      * The password is used to re-read and decrypt the contents
@@ -70,12 +74,6 @@ public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesRelo
         this.secureSettingsPassword = secureStorePassword;
     }
 
-    public void closePassword() {
-        if (this.secureSettingsPassword != null) {
-            this.secureSettingsPassword.close();
-        }
-    }
-
     boolean hasPassword() {
         return this.secureSettingsPassword != null && this.secureSettingsPassword.length() > 0;
     }
@@ -94,4 +92,50 @@ public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesRelo
             }
         }
     }
+
+    // This field is intentionally not part of serialization
+    private final Set<NodeRequest> nodeRequests = ConcurrentHashMap.newKeySet();
+
+    NodeRequest newNodeRequest() {
+        final NodesReloadSecureSettingsRequest clone = new NodesReloadSecureSettingsRequest(nodesIds());
+        if (hasPassword()) {
+            clone.setSecureStorePassword(getSecureSettingsPassword().clone());
+        }
+        final NodeRequest nodeRequest = new NodeRequest(clone);
+        nodeRequests.add(nodeRequest);
+        return nodeRequest;
+    }
+
+    @Override
+    public void close() {
+        if (this.secureSettingsPassword != null) {
+            this.secureSettingsPassword.close();
+        }
+        nodeRequests.forEach(NodeRequest::close);
+    }
+
+    public static class NodeRequest extends TransportRequest implements Releasable {
+        NodesReloadSecureSettingsRequest request;
+
+        NodeRequest(StreamInput in) throws IOException {
+            super(in);
+            request = new NodesReloadSecureSettingsRequest(in);
+        }
+
+        NodeRequest(NodesReloadSecureSettingsRequest request) {
+            this.request = request;
+        }
+
+        @Override
+        public void writeTo(StreamOutput out) throws IOException {
+            super.writeTo(out);
+            request.writeTo(out);
+        }
+
+        @Override
+        public void close() {
+            assert request.nodeRequests.isEmpty() : "potential circular reference";
+            request.close();
+        }
+    }
 }

server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

@@ -18,16 +18,13 @@ import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.io.stream.StreamInput;
-import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.settings.KeyStoreWrapper;
-import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.plugins.PluginsService;
 import org.elasticsearch.plugins.ReloadablePlugin;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.threadpool.ThreadPool;
-import org.elasticsearch.transport.TransportRequest;
 import org.elasticsearch.transport.TransportService;
 
 import java.io.IOException;
@@ -37,7 +34,7 @@ import java.util.List;
 public class TransportNodesReloadSecureSettingsAction extends TransportNodesAction<
     NodesReloadSecureSettingsRequest,
     NodesReloadSecureSettingsResponse,
-    TransportNodesReloadSecureSettingsAction.NodeRequest,
+    NodesReloadSecureSettingsRequest.NodeRequest,
     NodesReloadSecureSettingsResponse.NodeResponse> {
 
     private final Environment environment;
@@ -59,7 +56,7 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
             transportService,
             actionFilters,
             NodesReloadSecureSettingsRequest::new,
-            NodeRequest::new,
+            NodesReloadSecureSettingsRequest.NodeRequest::new,
             ThreadPool.Names.GENERIC,
             NodesReloadSecureSettingsResponse.NodeResponse.class
         );
@@ -77,8 +74,8 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
     }
 
     @Override
-    protected NodeRequest newNodeRequest(NodesReloadSecureSettingsRequest request) {
-        return new NodeRequest(request);
+    protected NodesReloadSecureSettingsRequest.NodeRequest newNodeRequest(NodesReloadSecureSettingsRequest request) {
+        return request.newNodeRequest();
     }
 
     @Override
@@ -93,7 +90,7 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
         ActionListener<NodesReloadSecureSettingsResponse> listener
     ) {
         if (request.hasPassword() && isNodeLocal(request) == false && isNodeTransportTLSEnabled() == false) {
-            request.closePassword();
+            request.close();
             listener.onFailure(
                 new ElasticsearchException(
                     "Secure settings cannot be updated cluster wide when TLS for the transport layer"
@@ -101,23 +98,17 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
                 )
             );
         } else {
-            super.doExecute(task, request, ActionListener.wrap(response -> {
-                request.closePassword();
-                listener.onResponse(response);
-            }, e -> {
-                request.closePassword();
-                listener.onFailure(e);
-            }));
+            super.doExecute(task, request, ActionListener.runBefore(listener, request::close));
         }
     }
 
     @Override
-    protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(NodeRequest nodeReloadRequest, Task task) {
+    protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(
+        NodesReloadSecureSettingsRequest.NodeRequest nodeReloadRequest,
+        Task task
+    ) {
         final NodesReloadSecureSettingsRequest request = nodeReloadRequest.request;
         // We default to using an empty string as the keystore password so that we mimic pre 7.3 API behavior
-        final SecureString secureSettingsPassword = request.hasPassword()
-            ? request.getSecureSettingsPassword()
-            : new SecureString(new char[0]);
         try (KeyStoreWrapper keystore = KeyStoreWrapper.load(environment.configFile())) {
             // reread keystore from config file
             if (keystore == null) {
@@ -127,7 +118,7 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
                 );
             }
             // decrypt the keystore using the password from the request
-            keystore.decrypt(secureSettingsPassword.getChars());
+            keystore.decrypt(request.hasPassword() ? request.getSecureSettingsPassword().getChars() : new char[0]);
             // add the keystore to the original node settings object
             final Settings settingsWithKeystore = Settings.builder().put(environment.settings(), false).setSecureSettings(keystore).build();
             final List<Exception> exceptions = new ArrayList<>();
@@ -145,27 +136,7 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
         } catch (final Exception e) {
             return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(), e);
         } finally {
-            secureSettingsPassword.close();
-        }
-    }
-
-    public static class NodeRequest extends TransportRequest {
-
-        NodesReloadSecureSettingsRequest request;
-
-        public NodeRequest(StreamInput in) throws IOException {
-            super(in);
-            request = new NodesReloadSecureSettingsRequest(in);
-        }
-
-        NodeRequest(NodesReloadSecureSettingsRequest request) {
-            this.request = request;
-        }
-
-        @Override
-        public void writeTo(StreamOutput out) throws IOException {
-            super.writeTo(out);
-            request.writeTo(out);
+            request.close();
         }
     }
 

server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java

@@ -78,7 +78,7 @@ public final class RestReloadSecureSettingsAction extends BaseRestHandler implem
                 builder.field("cluster_name", response.getClusterName().value());
                 response.toXContent(builder, channel.request());
                 builder.endObject();
-                nodesRequestBuilder.request().closePassword();
+                nodesRequestBuilder.request().close();
                 return new RestResponse(RestStatus.OK, builder);
             }
         });