Remove passphrase support from reload settings API (#32889)

We do not support passphrases on the secure settings storage (the
keystore). Yet, we added support for this in the API layer. This commit
removes this support so that we are not limited in our future options,
or have to make a breaking change.

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

@@ -19,82 +19,22 @@
 
 package org.elasticsearch.action.admin.cluster.node.reload;
 
-
-import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.nodes.BaseNodesRequest;
-import org.elasticsearch.common.CharArrays;
-import org.elasticsearch.common.io.stream.StreamInput;
-import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.settings.SecureString;
-
-import java.io.IOException;
-import java.util.Arrays;
-
-import static org.elasticsearch.action.ValidateActions.addValidationError;
 
 /**
- * Request for a reload secure settings action
+ * Request for a reload secure settings action.
  */
 public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesReloadSecureSettingsRequest> {
 
-    /**
-     * The password which is broadcasted to all nodes, but is never stored on
-     * persistent storage. The password is used to reread and decrypt the contents
-     * of the node's keystore (backing the implementation of
-     * {@code SecureSettings}).
-     */
-    private SecureString secureSettingsPassword;
-
     public NodesReloadSecureSettingsRequest() {
     }
 
     /**
-     * Reload secure settings only on certain nodes, based on the nodes ids
-     * specified. If none are passed, secure settings will be reloaded on all the
-     * nodes.
+     * Reload secure settings only on certain nodes, based on the nodes IDs specified. If none are passed, secure settings will be reloaded
+     * on all the nodes.
      */
-    public NodesReloadSecureSettingsRequest(String... nodesIds) {
+    public NodesReloadSecureSettingsRequest(final String... nodesIds) {
         super(nodesIds);
     }
 
-    @Override
-    public ActionRequestValidationException validate() {
-        ActionRequestValidationException validationException = null;
-        if (secureSettingsPassword == null) {
-            validationException = addValidationError("secure settings password cannot be null (use empty string instead)",
-                    validationException);
-        }
-        return validationException;
-    }
-
-    public SecureString secureSettingsPassword() {
-        return secureSettingsPassword;
-    }
-
-    public NodesReloadSecureSettingsRequest secureStorePassword(SecureString secureStorePassword) {
-        this.secureSettingsPassword = secureStorePassword;
-        return this;
-    }
-
-    @Override
-    public void readFrom(StreamInput in) throws IOException {
-        super.readFrom(in);
-        final byte[] passwordBytes = in.readByteArray();
-        try {
-            this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(passwordBytes));
-        } finally {
-            Arrays.fill(passwordBytes, (byte) 0);
-        }
-    }
-
-    @Override
-    public void writeTo(StreamOutput out) throws IOException {
-        super.writeTo(out);
-        final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars());
-        try {
-            out.writeByteArray(passwordBytes);
-        } finally {
-            Arrays.fill(passwordBytes, (byte) 0);
-        }
-    }
 }

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

@@ -19,19 +19,8 @@
 
 package org.elasticsearch.action.admin.cluster.node.reload;
 
-import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.action.support.nodes.NodesOperationRequestBuilder;
 import org.elasticsearch.client.ElasticsearchClient;
-import org.elasticsearch.common.bytes.BytesReference;
-import org.elasticsearch.common.settings.SecureString;
-import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
-import org.elasticsearch.common.xcontent.NamedXContentRegistry;
-import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.common.xcontent.XContentType;
-
-import java.io.IOException;
-import java.io.InputStream;
-import java.util.Objects;
 
 /**
  * Builder for the reload secure settings nodes request
@@ -39,46 +28,8 @@ import java.util.Objects;
 public class NodesReloadSecureSettingsRequestBuilder extends NodesOperationRequestBuilder<NodesReloadSecureSettingsRequest,
         NodesReloadSecureSettingsResponse, NodesReloadSecureSettingsRequestBuilder> {
 
-    public static final String SECURE_SETTINGS_PASSWORD_FIELD_NAME = "secure_settings_password";
-
     public NodesReloadSecureSettingsRequestBuilder(ElasticsearchClient client, NodesReloadSecureSettingsAction action) {
         super(client, action, new NodesReloadSecureSettingsRequest());
     }
 
-    public NodesReloadSecureSettingsRequestBuilder setSecureStorePassword(SecureString secureStorePassword) {
-        request.secureStorePassword(secureStorePassword);
-        return this;
-    }
-
-    public NodesReloadSecureSettingsRequestBuilder source(BytesReference source, XContentType xContentType) throws IOException {
-        Objects.requireNonNull(xContentType);
-        // EMPTY is ok here because we never call namedObject
-        try (InputStream stream = source.streamInput();
-                XContentParser parser = xContentType.xContent().createParser(NamedXContentRegistry.EMPTY,
-                        LoggingDeprecationHandler.INSTANCE, stream)) {
-            XContentParser.Token token;
-            token = parser.nextToken();
-            if (token != XContentParser.Token.START_OBJECT) {
-                throw new ElasticsearchParseException("expected an object, but found token [{}]", token);
-            }
-            token = parser.nextToken();
-            if (token != XContentParser.Token.FIELD_NAME || false == SECURE_SETTINGS_PASSWORD_FIELD_NAME.equals(parser.currentName())) {
-                throw new ElasticsearchParseException("expected a field named [{}], but found [{}]", SECURE_SETTINGS_PASSWORD_FIELD_NAME,
-                        token);
-            }
-            token = parser.nextToken();
-            if (token != XContentParser.Token.VALUE_STRING) {
-                throw new ElasticsearchParseException("expected field [{}] to be of type string, but found [{}] instead",
-                        SECURE_SETTINGS_PASSWORD_FIELD_NAME, token);
-            }
-            final String password = parser.text();
-            setSecureStorePassword(new SecureString(password.toCharArray()));
-            token = parser.nextToken();
-            if (token != XContentParser.Token.END_OBJECT) {
-                throw new ElasticsearchParseException("expected end of object, but found token [{}]", token);
-            }
-        }
-        return this;
-    }
-
 }

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

@@ -31,7 +31,6 @@ 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;
@@ -82,16 +81,13 @@ public class TransportNodesReloadSecureSettingsAction extends TransportNodesActi
 
     @Override
     protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(NodeRequest nodeReloadRequest) {
-        final NodesReloadSecureSettingsRequest request = nodeReloadRequest.request;
-        final SecureString secureSettingsPassword = request.secureSettingsPassword();
         try (KeyStoreWrapper keystore = KeyStoreWrapper.load(environment.configFile())) {
             // reread keystore from config file
             if (keystore == null) {
                 return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(),
                         new IllegalStateException("Keystore is missing"));
             }
-            // decrypt the keystore using the password from the request
-            keystore.decrypt(secureSettingsPassword.getChars());
+            keystore.decrypt(new char[0]);
             // add the keystore to the original node settings object
             final Settings settingsWithKeystore = Settings.builder()
                     .put(environment.settings(), false)

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

@@ -59,7 +59,6 @@ public final class RestReloadSecureSettingsAction extends BaseRestHandler {
                 .cluster()
                 .prepareReloadSecureSettings()
                 .setTimeout(request.param("timeout"))
-                .source(request.requiredContent(), request.getXContentType())
                 .setNodesIds(nodesIds);
         final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request();
         return channel -> nodesRequestBuilder
@@ -68,12 +67,12 @@ public final class RestReloadSecureSettingsAction extends BaseRestHandler {
                     public RestResponse buildResponse(NodesReloadSecureSettingsResponse response, XContentBuilder builder)
                             throws Exception {
                         builder.startObject();
-                        RestActions.buildNodesHeader(builder, channel.request(), response);
-                        builder.field("cluster_name", response.getClusterName().value());
-                        response.toXContent(builder, channel.request());
+                        {
+                            RestActions.buildNodesHeader(builder, channel.request(), response);
+                            builder.field("cluster_name", response.getClusterName().value());
+                            response.toXContent(builder, channel.request());
+                        }
                         builder.endObject();
-                        // clear password for the original request
-                        nodesRequest.secureSettingsPassword().close();
                         return new BytesRestResponse(RestStatus.OK, builder);
                     }
                 });

server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java

@@ -20,11 +20,9 @@
 package org.elasticsearch.action.admin;
 
 import org.elasticsearch.action.ActionListener;
-import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsResponse;
 import org.elasticsearch.common.settings.KeyStoreWrapper;
 import org.elasticsearch.common.settings.SecureSettings;
-import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.plugins.Plugin;
@@ -44,11 +42,11 @@ import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
-import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.containsString;
 
 public class ReloadSecureSettingsIT extends ESIntegTestCase {
 
@@ -62,7 +60,7 @@ public class ReloadSecureSettingsIT extends ESIntegTestCase {
         Files.deleteIfExists(KeyStoreWrapper.keystorePath(environment.configFile()));
         final int initialReloadCount = mockReloadablePlugin.getReloadCount();
         final CountDownLatch latch = new CountDownLatch(1);
-        client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
+        client().admin().cluster().prepareReloadSecureSettings().execute(
                 new ActionListener<NodesReloadSecureSettingsResponse>() {
                     @Override
                     public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
@@ -96,44 +94,6 @@ public class ReloadSecureSettingsIT extends ESIntegTestCase {
         assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
     }
 
-    public void testNullKeystorePassword() throws Exception {
-        final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
-        final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class)
-                .stream().findFirst().get();
-        final AtomicReference<AssertionError> reloadSettingsError = new AtomicReference<>();
-        final int initialReloadCount = mockReloadablePlugin.getReloadCount();
-        final CountDownLatch latch = new CountDownLatch(1);
-        client().admin().cluster().prepareReloadSecureSettings().execute(
-                new ActionListener<NodesReloadSecureSettingsResponse>() {
-                    @Override
-                    public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
-                        try {
-                            reloadSettingsError.set(new AssertionError("Null keystore password should fail"));
-                        } finally {
-                            latch.countDown();
-                        }
-                    }
-
-                    @Override
-                    public void onFailure(Exception e) {
-                        try {
-                            assertThat(e, instanceOf(ActionRequestValidationException.class));
-                            assertThat(e.getMessage(), containsString("secure settings password cannot be null"));
-                        } catch (final AssertionError ae) {
-                            reloadSettingsError.set(ae);
-                        } finally {
-                            latch.countDown();
-                        }
-                    }
-                });
-        latch.await();
-        if (reloadSettingsError.get() != null) {
-            throw reloadSettingsError.get();
-        }
-        // in the null password case no reload should be triggered
-        assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
-    }
-
     public void testInvalidKeystoreFile() throws Exception {
         final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
         final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class)
@@ -149,7 +109,7 @@ public class ReloadSecureSettingsIT extends ESIntegTestCase {
             Files.copy(keystore, KeyStoreWrapper.keystorePath(environment.configFile()), StandardCopyOption.REPLACE_EXISTING);
         }
         final CountDownLatch latch = new CountDownLatch(1);
-        client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
+        client().admin().cluster().prepareReloadSecureSettings().execute(
                 new ActionListener<NodesReloadSecureSettingsResponse>() {
                     @Override
                     public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
@@ -181,52 +141,6 @@ public class ReloadSecureSettingsIT extends ESIntegTestCase {
         assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
     }
 
-    public void testWrongKeystorePassword() throws Exception {
-        final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
-        final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class)
-                .stream().findFirst().get();
-        final Environment environment = internalCluster().getInstance(Environment.class);
-        final AtomicReference<AssertionError> reloadSettingsError = new AtomicReference<>();
-        final int initialReloadCount = mockReloadablePlugin.getReloadCount();
-        // "some" keystore should be present in this case
-        writeEmptyKeystore(environment, new char[0]);
-        final CountDownLatch latch = new CountDownLatch(1);
-        client().admin()
-                .cluster()
-                .prepareReloadSecureSettings()
-                .setSecureStorePassword(new SecureString(new char[] { 'W', 'r', 'o', 'n', 'g' }))
-                .execute(new ActionListener<NodesReloadSecureSettingsResponse>() {
-                    @Override
-                    public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
-                        try {
-                            assertThat(nodesReloadResponse, notNullValue());
-                            final Map<String, NodesReloadSecureSettingsResponse.NodeResponse> nodesMap = nodesReloadResponse.getNodesMap();
-                            assertThat(nodesMap.size(), equalTo(cluster().size()));
-                            for (final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse : nodesReloadResponse.getNodes()) {
-                                assertThat(nodeResponse.reloadException(), notNullValue());
-                                    assertThat(nodeResponse.reloadException(), instanceOf(SecurityException.class));
-                            }
-                        } catch (final AssertionError e) {
-                            reloadSettingsError.set(e);
-                        } finally {
-                            latch.countDown();
-                        }
-                    }
-
-                    @Override
-                    public void onFailure(Exception e) {
-                        reloadSettingsError.set(new AssertionError("Nodes request failed", e));
-                        latch.countDown();
-                    }
-                });
-        latch.await();
-        if (reloadSettingsError.get() != null) {
-            throw reloadSettingsError.get();
-        }
-        // in the wrong password case no reload should be triggered
-        assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
-    }
-
     public void testMisbehavingPlugin() throws Exception {
         final Environment environment = internalCluster().getInstance(Environment.class);
         final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
@@ -247,7 +161,7 @@ public class ReloadSecureSettingsIT extends ESIntegTestCase {
                 .get(Settings.builder().put(environment.settings()).setSecureSettings(secureSettings).build())
                 .toString();
         final CountDownLatch latch = new CountDownLatch(1);
-        client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
+        client().admin().cluster().prepareReloadSecureSettings().execute(
                 new ActionListener<NodesReloadSecureSettingsResponse>() {
                     @Override
                     public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
@@ -314,7 +228,7 @@ public class ReloadSecureSettingsIT extends ESIntegTestCase {
     private void successfulReloadCall() throws InterruptedException {
         final AtomicReference<AssertionError> reloadSettingsError = new AtomicReference<>();
         final CountDownLatch latch = new CountDownLatch(1);
-        client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
+        client().admin().cluster().prepareReloadSecureSettings().execute(
                 new ActionListener<NodesReloadSecureSettingsResponse>() {
                     @Override
                     public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {