From b7fa9086a51ab18763c94186db0b01e681fe0bb9 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 15 Aug 2018 17:27:48 -0400 Subject: [PATCH 1/2] Remove passphrase support from reload settings API 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. --- .../NodesReloadSecureSettingsRequest.java | 128 +----------------- ...desReloadSecureSettingsRequestBuilder.java | 20 --- ...nsportNodesReloadSecureSettingsAction.java | 6 +- .../RestReloadSecureSettingsAction.java | 10 +- .../action/admin/ReloadSecureSettingsIT.java | 98 +------------- 5 files changed, 16 insertions(+), 246 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java index 50df7b1bb26e0..fb3e6ac71adf3 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java @@ -19,142 +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.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.settings.SecureString; - -import java.io.IOException; -import java.nio.ByteBuffer; -import java.nio.CharBuffer; -import java.nio.charset.StandardCharsets; -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 { - /** - * 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(utf8BytesToChars(passwordBytes)); - } finally { - Arrays.fill(passwordBytes, (byte) 0); - } - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - super.writeTo(out); - final byte[] passwordBytes = charsToUtf8Bytes(this.secureSettingsPassword.getChars()); - try { - out.writeByteArray(passwordBytes); - } finally { - Arrays.fill(passwordBytes, (byte) 0); - } - } - - /** - * Encodes the provided char[] to a UTF-8 byte[]. This is done while avoiding - * conversions to String. The provided char[] is not modified by this method, so - * the caller needs to take care of clearing the value if it is sensitive. - */ - private static byte[] charsToUtf8Bytes(char[] chars) { - final CharBuffer charBuffer = CharBuffer.wrap(chars); - final ByteBuffer byteBuffer = StandardCharsets.UTF_8.encode(charBuffer); - final byte[] bytes; - if (byteBuffer.hasArray()) { - // there is no guarantee that the byte buffers backing array is the right size - // so we need to make a copy - bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit()); - Arrays.fill(byteBuffer.array(), (byte) 0); // clear sensitive data - } else { - final int length = byteBuffer.limit() - byteBuffer.position(); - bytes = new byte[length]; - byteBuffer.get(bytes); - // if the buffer is not read only we can reset and fill with 0's - if (byteBuffer.isReadOnly() == false) { - byteBuffer.clear(); // reset - for (int i = 0; i < byteBuffer.limit(); i++) { - byteBuffer.put((byte) 0); - } - } - } - return bytes; - } - - /** - * Decodes the provided byte[] to a UTF-8 char[]. This is done while avoiding - * conversions to String. The provided byte[] is not modified by this method, so - * the caller needs to take care of clearing the value if it is sensitive. - */ - public static char[] utf8BytesToChars(byte[] utf8Bytes) { - final ByteBuffer byteBuffer = ByteBuffer.wrap(utf8Bytes); - final CharBuffer charBuffer = StandardCharsets.UTF_8.decode(byteBuffer); - final char[] chars; - if (charBuffer.hasArray()) { - // there is no guarantee that the char buffers backing array is the right size - // so we need to make a copy - chars = Arrays.copyOfRange(charBuffer.array(), charBuffer.position(), charBuffer.limit()); - Arrays.fill(charBuffer.array(), (char) 0); // clear sensitive data - } else { - final int length = charBuffer.limit() - charBuffer.position(); - chars = new char[length]; - charBuffer.get(chars); - // if the buffer is not read only we can reset and fill with 0's - if (charBuffer.isReadOnly() == false) { - charBuffer.clear(); // reset - for (int i = 0; i < charBuffer.limit(); i++) { - charBuffer.put((char) 0); - } - } - } - return chars; - } } diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java index b5f2f73e56f51..8149e09338704 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java @@ -23,7 +23,6 @@ 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; @@ -39,17 +38,10 @@ public class NodesReloadSecureSettingsRequestBuilder extends NodesOperationRequestBuilder { - 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 @@ -62,18 +54,6 @@ public NodesReloadSecureSettingsRequestBuilder source(BytesReference source, XCo 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); } 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 0f44170fa603b..b8a36bac68d61 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 @@ -31,7 +31,6 @@ 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 @@ protected NodesReloadSecureSettingsResponse.NodeResponse newNodeResponse() { @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) diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java index 0697871ea5d1c..be96db15e2d14 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java @@ -68,12 +68,12 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client 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); } }); diff --git a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java index 7952758240544..3f9e258ffec1c 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java +++ b/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.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 void testMissingKeystoreFile() throws Exception { 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() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @@ -96,44 +94,6 @@ public void onFailure(Exception e) { 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 reloadSettingsError = new AtomicReference<>(); - final int initialReloadCount = mockReloadablePlugin.getReloadCount(); - final CountDownLatch latch = new CountDownLatch(1); - client().admin().cluster().prepareReloadSecureSettings().execute( - new ActionListener() { - @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 void testInvalidKeystoreFile() throws Exception { 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() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @@ -181,52 +141,6 @@ public void onFailure(Exception e) { 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 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() { - @Override - public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { - try { - assertThat(nodesReloadResponse, notNullValue()); - final Map 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 void testMisbehavingPlugin() throws Exception { .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() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @@ -314,7 +228,7 @@ protected Collection> nodePlugins() { private void successfulReloadCall() throws InterruptedException { final AtomicReference 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() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { From 8d9c8bffd15a2393bc9a191f67e1df1ad41cc21c Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 15 Aug 2018 19:53:18 -0400 Subject: [PATCH 2/2] Fix docs --- ...desReloadSecureSettingsRequestBuilder.java | 29 ------------------- .../RestReloadSecureSettingsAction.java | 1 - 2 files changed, 30 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java index 8149e09338704..c8250455e6ba3 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java @@ -19,18 +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.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 @@ -42,23 +32,4 @@ public NodesReloadSecureSettingsRequestBuilder(ElasticsearchClient client, Nodes super(client, action, new NodesReloadSecureSettingsRequest()); } - 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.END_OBJECT) { - throw new ElasticsearchParseException("expected end of object, but found token [{}]", token); - } - } - return this; - } - } diff --git a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java index be96db15e2d14..2251615d678fb 100644 --- a/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java +++ b/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java @@ -59,7 +59,6 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client .cluster() .prepareReloadSecureSettings() .setTimeout(request.param("timeout")) - .source(request.requiredContent(), request.getXContentType()) .setNodesIds(nodesIds); final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request(); return channel -> nodesRequestBuilder