From 8956cfb902e79c1bd8dd20c79cb52eb362162459 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 13 Jun 2019 07:57:58 +0300 Subject: [PATCH 01/11] Allow password in reload secure settings API If a password is not set, we assume an empty string to be compatible with previous behavior. Only allow the reload to be broadcast to other nodes if TLS is enabled for the transport layer. --- .../NodesReloadSecureSettingsRequest.java | 77 +++++++- ...desReloadSecureSettingsRequestBuilder.java | 49 +++++ ...nsportNodesReloadSecureSettingsAction.java | 33 +++- .../RestReloadSecureSettingsAction.java | 11 +- .../action/admin/ReloadSecureSettingsIT.java | 187 +++++++++++++++--- 5 files changed, 321 insertions(+), 36 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 fb3e6ac71adf3..5262a90340aae 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 @@ -20,21 +20,90 @@ package org.elasticsearch.action.admin.cluster.node.reload; import org.elasticsearch.action.support.nodes.BaseNodesRequest; +import org.elasticsearch.common.CharArrays; +import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.bytes.BytesReference; +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; /** - * Request for a reload secure settings action. + * Request for a reload secure settings action */ public class NodesReloadSecureSettingsRequest extends BaseNodesRequest { + /** + * The password is used to re-read and decrypt the contents + * of the node's keystore (backing the implementation of + * {@code SecureSettings}). + */ + @Nullable + 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(final String... nodesIds) { + public NodesReloadSecureSettingsRequest(String... nodesIds) { super(nodesIds); } + @Nullable + public SecureString getSecureSettingsPassword() { + return secureSettingsPassword; + } + + public NodesReloadSecureSettingsRequest setSecureStorePassword(SecureString secureStorePassword) { + this.secureSettingsPassword = secureStorePassword; + return this; + } + + public void closePassword() { + if (this.secureSettingsPassword != null) { + this.secureSettingsPassword.close(); + } + } + + public boolean hasPassword() { + return this.secureSettingsPassword != null && this.secureSettingsPassword.length() > 0; + } + + @Override + public void readFrom(StreamInput in) throws IOException { + super.readFrom(in); + final BytesReference bytesRef = in.readOptionalBytesReference(); + if (bytesRef != null) { + byte[] bytes = BytesReference.toBytes(bytesRef); + try { + this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(bytes)); + } finally { + Arrays.fill(bytes, (byte) 0); + } + } else { + this.secureSettingsPassword = null; + } + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + super.writeTo(out); + if (this.secureSettingsPassword == null) { + out.writeOptionalBytesReference(null); + } else { + final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars()); + try { + out.writeOptionalBytesReference(new BytesArray(passwordBytes)); + } finally { + Arrays.fill(passwordBytes, (byte) 0); + } + } + } } 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 c8250455e6ba3..1a2830bbc3433 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,8 +19,19 @@ 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 @@ -28,8 +39,46 @@ 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.setSecureStorePassword(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); + } + + 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; + } + } 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 f2fef743a0d37..bac35142e477d 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 @@ -22,19 +22,23 @@ import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; import org.elasticsearch.ExceptionsHelper; +import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.FailedNodeException; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.nodes.BaseNodeRequest; import org.elasticsearch.action.support.nodes.TransportNodesAction; +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.TransportService; @@ -77,15 +81,30 @@ protected NodesReloadSecureSettingsResponse.NodeResponse newNodeResponse() { return new NodesReloadSecureSettingsResponse.NodeResponse(); } + @Override + protected void doExecute(Task task, NodesReloadSecureSettingsRequest request, ActionListener listener) { + if (request.hasPassword() && isNodeLocal(request) == false && isNodeTransportTLSEnabled() == false) { + listener.onFailure(new IllegalStateException("Secure settings cannot be updated cluster wide when TLS for the transport layer" + + " is not enabled. Enable TLS or use the API with a `_local` filter on each node.")); + } else { + super.doExecute(task, request, listener); + } + } + @Override protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(NodeRequest nodeReloadRequest) { + 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) { return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(), new IllegalStateException("Keystore is missing")); } - keystore.decrypt(new char[0]); + // decrypt the keystore using the password from the request + keystore.decrypt(secureSettingsPassword.getChars()); // add the keystore to the original node settings object final Settings settingsWithKeystore = Settings.builder() .put(environment.settings(), false) @@ -134,4 +153,16 @@ public void writeTo(StreamOutput out) throws IOException { request.writeTo(out); } } + + /** + * Returns true if the node is configured for TLS on the transport layer + */ + private boolean isNodeTransportTLSEnabled() { + return this.environment.settings().getAsBoolean("xpack.security.transport.ssl.enabled", false); + } + + private boolean isNodeLocal(NodesReloadSecureSettingsRequest request) { + final DiscoveryNode[] nodes = request.concreteNodes(); + return nodes.length == 1 && nodes[0].getId().equals(clusterService.localNode().getId()); + } } 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 2251615d678fb..f9b7f7abf366a 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,6 +59,7 @@ 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 @@ -67,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.closePassword(); 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 3f9e258ffec1c..1732bd60ad247 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java @@ -21,8 +21,10 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsResponse; +import org.elasticsearch.common.Strings; 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; @@ -42,11 +44,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 { @@ -60,7 +62,9 @@ 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().execute( + final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; + client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) + .setNodesIds(Strings.EMPTY_ARRAY).execute( new ActionListener() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @@ -97,7 +101,7 @@ public void onFailure(Exception e) { public void testInvalidKeystoreFile() throws Exception { final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class); final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class) - .stream().findFirst().get(); + .stream().findFirst().get(); final Environment environment = internalCluster().getInstance(Environment.class); final AtomicReference reloadSettingsError = new AtomicReference<>(); final int initialReloadCount = mockReloadablePlugin.getReloadCount(); @@ -109,35 +113,162 @@ 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().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()); - } - } catch (final AssertionError e) { - reloadSettingsError.set(e); - } finally { - latch.countDown(); + final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; + client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) + .setNodesIds(Strings.EMPTY_ARRAY).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()); } + } 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 invalid keystore format case no reload should be triggered + assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount)); + } - @Override - public void onFailure(Exception e) { - reloadSettingsError.set(new AssertionError("Nodes request failed", e)); + public void testReloadAllNodesWithPasswordWithoutTLSFails() 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(); + final char[] password = randomAlphaOfLength(12).toCharArray(); + writeEmptyKeystore(environment, password); + final CountDownLatch latch = new CountDownLatch(1); + client().admin() + .cluster() + .prepareReloadSecureSettings() + // No filter should try to hit all nodes + .setNodesIds(Strings.EMPTY_ARRAY) + .setSecureStorePassword(new SecureString(password)) + .execute(new ActionListener() { + @Override + public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { + reloadSettingsError.set(new AssertionError("Nodes request succeeded when it should have failed", null)); + latch.countDown(); + } + + @Override + public void onFailure(Exception e) { + assertThat(e, instanceOf(IllegalStateException.class)); + assertThat(e.getMessage(), containsString("Secure settings cannot be updated cluster wide when TLS is not enabled")); + latch.countDown(); + } + }); + latch.await(); + if (reloadSettingsError.get() != null) { + throw reloadSettingsError.get(); + } + //no reload should be triggered + assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount)); + } + + public void testReloadLocalNodeWithPasswordWithoutTLSSucceeds() throws Exception { + final Environment environment = internalCluster().getInstance(Environment.class); + final AtomicReference reloadSettingsError = new AtomicReference<>(); + final char[] password = randomAlphaOfLength(12).toCharArray(); + writeEmptyKeystore(environment, password); + final CountDownLatch latch = new CountDownLatch(1); + client().admin() + .cluster() + .prepareReloadSecureSettings() + .setNodesIds("_local") + .setSecureStorePassword(new SecureString(password)) + .execute(new ActionListener() { + @Override + public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { + try { + assertThat(nodesReloadResponse, notNullValue()); + final Map nodesMap = nodesReloadResponse.getNodesMap(); + assertThat(nodesMap.size(), equalTo(1)); + assertThat(nodesReloadResponse.getNodes().size(), equalTo(1)); + final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse = nodesReloadResponse.getNodes().get(0); + assertThat(nodeResponse.reloadException(), nullValue()); + } 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 invalid keystore format case no reload should be triggered + } + + 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() + .setNodesIds("_local") + .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(1)); + 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)); } @@ -161,7 +292,9 @@ 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().execute( + final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; + client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) + .setNodesIds(Strings.EMPTY_ARRAY).execute( new ActionListener() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @@ -228,7 +361,9 @@ protected Collection> nodePlugins() { private void successfulReloadCall() throws InterruptedException { final AtomicReference reloadSettingsError = new AtomicReference<>(); final CountDownLatch latch = new CountDownLatch(1); - client().admin().cluster().prepareReloadSecureSettings().execute( + final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; + client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) + .setNodesIds(Strings.EMPTY_ARRAY).execute( new ActionListener() { @Override public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { From 76a88de209d5049fb9464319a0e86e91ee07208b Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 13 Jun 2019 14:29:07 +0300 Subject: [PATCH 02/11] Expose TLS configuration in Transport and TransportService --- ...nsportNodesReloadSecureSettingsAction.java | 6 +- .../elasticsearch/transport/Transport.java | 4 + .../transport/TransportService.java | 4 + .../action/admin/ReloadSecureSettingsIT.java | 148 +++++++++--------- .../netty4/SecurityNetty4Transport.java | 5 + .../transport/nio/SecurityNioTransport.java | 5 + 6 files changed, 98 insertions(+), 74 deletions(-) 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 bac35142e477d..d4cd977ea133f 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 @@ -158,10 +158,14 @@ public void writeTo(StreamOutput out) throws IOException { * Returns true if the node is configured for TLS on the transport layer */ private boolean isNodeTransportTLSEnabled() { - return this.environment.settings().getAsBoolean("xpack.security.transport.ssl.enabled", false); + return transportService.isTransportSecure(); } private boolean isNodeLocal(NodesReloadSecureSettingsRequest request) { + if (null == request.concreteNodes()) { + resolveRequest(request, clusterService.state()); + assert request.concreteNodes() != null; + } final DiscoveryNode[] nodes = request.concreteNodes(); return nodes.length == 1 && nodes[0].getId().equals(clusterService.localNode().getId()); } diff --git a/server/src/main/java/org/elasticsearch/transport/Transport.java b/server/src/main/java/org/elasticsearch/transport/Transport.java index 0b79b6aecf093..0590b679eaba5 100644 --- a/server/src/main/java/org/elasticsearch/transport/Transport.java +++ b/server/src/main/java/org/elasticsearch/transport/Transport.java @@ -54,6 +54,10 @@ public interface Transport extends LifecycleComponent { void setMessageListener(TransportMessageListener listener); + default boolean isSecure() { + return false; + } + /** * The address the transport is bound on. */ diff --git a/server/src/main/java/org/elasticsearch/transport/TransportService.java b/server/src/main/java/org/elasticsearch/transport/TransportService.java index dca7f52e60474..824845718686d 100644 --- a/server/src/main/java/org/elasticsearch/transport/TransportService.java +++ b/server/src/main/java/org/elasticsearch/transport/TransportService.java @@ -300,6 +300,10 @@ public TransportStats stats() { return transport.getStats(); } + public boolean isTransportSecure() { + return transport.isSecure(); + } + public BoundTransportAddress boundAddress() { return transport.boundAddress(); } 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 1732bd60ad247..5451a7805a43b 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java @@ -50,12 +50,13 @@ import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.containsString; +@ESIntegTestCase.ClusterScope(minNumDataNodes = 2) public class ReloadSecureSettingsIT extends ESIntegTestCase { public void testMissingKeystoreFile() throws Exception { final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class); final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class) - .stream().findFirst().get(); + .stream().findFirst().get(); final Environment environment = internalCluster().getInstance(Environment.class); final AtomicReference reloadSettingsError = new AtomicReference<>(); // keystore file should be missing for this test case @@ -65,31 +66,31 @@ public void testMissingKeystoreFile() throws Exception { final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) .setNodesIds(Strings.EMPTY_ARRAY).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(IllegalStateException.class)); - assertThat(nodeResponse.reloadException().getMessage(), containsString("Keystore is missing")); - } - } catch (final AssertionError e) { - reloadSettingsError.set(e); - } finally { - latch.countDown(); + 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(IllegalStateException.class)); + assertThat(nodeResponse.reloadException().getMessage(), containsString("Keystore is missing")); } - } - - @Override - public void onFailure(Exception e) { - reloadSettingsError.set(new AssertionError("Nodes request failed", e)); + } 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(); @@ -173,7 +174,8 @@ public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @Override public void onFailure(Exception e) { assertThat(e, instanceOf(IllegalStateException.class)); - assertThat(e.getMessage(), containsString("Secure settings cannot be updated cluster wide when TLS is not enabled")); + assertThat(e.getMessage(), + containsString("Secure settings cannot be updated cluster wide when TLS for the transport layer is not enabled")); latch.countDown(); } }); @@ -276,12 +278,12 @@ public void testMisbehavingPlugin() throws Exception { final Environment environment = internalCluster().getInstance(Environment.class); final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class); final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class) - .stream().findFirst().get(); + .stream().findFirst().get(); // make plugins throw on reload for (final String nodeName : internalCluster().getNodeNames()) { internalCluster().getInstance(PluginsService.class, nodeName) - .filterPlugins(MisbehavingReloadablePlugin.class) - .stream().findFirst().get().setShouldThrow(true); + .filterPlugins(MisbehavingReloadablePlugin.class) + .stream().findFirst().get().setShouldThrow(true); } final AtomicReference reloadSettingsError = new AtomicReference<>(); final int initialReloadCount = mockReloadablePlugin.getReloadCount(); @@ -289,36 +291,36 @@ public void testMisbehavingPlugin() throws Exception { final SecureSettings secureSettings = writeEmptyKeystore(environment, new char[0]); // read seed setting value from the test case (not from the node) final String seedValue = KeyStoreWrapper.SEED_SETTING - .get(Settings.builder().put(environment.settings()).setSecureSettings(secureSettings).build()) - .toString(); + .get(Settings.builder().put(environment.settings()).setSecureSettings(secureSettings).build()) + .toString(); final CountDownLatch latch = new CountDownLatch(1); final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) .setNodesIds(Strings.EMPTY_ARRAY).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().getMessage(), containsString("If shouldThrow I throw")); - } - } catch (final AssertionError e) { - reloadSettingsError.set(e); - } finally { - latch.countDown(); + 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().getMessage(), containsString("If shouldThrow I throw")); } - } - - @Override - public void onFailure(Exception e) { - reloadSettingsError.set(new AssertionError("Nodes request failed", e)); + } 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(); @@ -333,7 +335,7 @@ public void onFailure(Exception e) { public void testReloadWhileKeystoreChanged() throws Exception { final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class); final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class) - .stream().findFirst().get(); + .stream().findFirst().get(); final Environment environment = internalCluster().getInstance(Environment.class); final int initialReloadCount = mockReloadablePlugin.getReloadCount(); for (int i = 0; i < randomIntBetween(4, 8); i++) { @@ -341,8 +343,8 @@ public void testReloadWhileKeystoreChanged() throws Exception { final SecureSettings secureSettings = writeEmptyKeystore(environment, new char[0]); // read seed setting value from the test case (not from the node) final String seedValue = KeyStoreWrapper.SEED_SETTING - .get(Settings.builder().put(environment.settings()).setSecureSettings(secureSettings).build()) - .toString(); + .get(Settings.builder().put(environment.settings()).setSecureSettings(secureSettings).build()) + .toString(); // reload call successfulReloadCall(); assertThat(mockReloadablePlugin.getSeedValue(), equalTo(seedValue)); @@ -364,29 +366,29 @@ private void successfulReloadCall() throws InterruptedException { final SecureString emptyPassword = randomBoolean() ? new SecureString(new char[0]) : null; client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(emptyPassword) .setNodesIds(Strings.EMPTY_ARRAY).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(), nullValue()); - } - } catch (final AssertionError e) { - reloadSettingsError.set(e); - } finally { - latch.countDown(); + 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(), nullValue()); } - } - - @Override - public void onFailure(Exception e) { - reloadSettingsError.set(new AssertionError("Nodes request failed", e)); + } 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(); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/transport/netty4/SecurityNetty4Transport.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/transport/netty4/SecurityNetty4Transport.java index 6e2b9c1a7efdd..624b90125b0db 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/transport/netty4/SecurityNetty4Transport.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/transport/netty4/SecurityNetty4Transport.java @@ -131,6 +131,11 @@ protected ServerChannelInitializer getSslChannelInitializer(final String name, f return new SslChannelInitializer(name, sslConfiguration); } + @Override + public boolean isSecure() { + return this.sslEnabled; + } + private class SecurityClientChannelInitializer extends ClientChannelInitializer { private final boolean hostnameVerificationEnabled; diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SecurityNioTransport.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SecurityNioTransport.java index c2722cb3a0fb6..cfb6e0934c423 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SecurityNioTransport.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SecurityNioTransport.java @@ -128,6 +128,11 @@ protected Function clientChannelFactoryFunctio }; } + @Override + public boolean isSecure() { + return this.sslEnabled; + } + private class SecurityTcpChannelFactory extends TcpChannelFactory { private final String profileName; From dabb53bcaefb1473dc01ae74f9537ec58a83fa9b Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 13 Jun 2019 15:20:56 +0300 Subject: [PATCH 03/11] checkstyle --- .../reload/TransportNodesReloadSecureSettingsAction.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 d4cd977ea133f..faffaa79b0d31 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 @@ -82,9 +82,11 @@ protected NodesReloadSecureSettingsResponse.NodeResponse newNodeResponse() { } @Override - protected void doExecute(Task task, NodesReloadSecureSettingsRequest request, ActionListener listener) { + protected void doExecute(Task task, NodesReloadSecureSettingsRequest request, + ActionListener listener) { if (request.hasPassword() && isNodeLocal(request) == false && isNodeTransportTLSEnabled() == false) { - listener.onFailure(new IllegalStateException("Secure settings cannot be updated cluster wide when TLS for the transport layer" + + listener.onFailure( + new IllegalStateException("Secure settings cannot be updated cluster wide when TLS for the transport layer" + " is not enabled. Enable TLS or use the API with a `_local` filter on each node.")); } else { super.doExecute(task, request, listener); From c647b24ea00e48e4eecb9703fa47589436cde4f3 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 13 Jun 2019 17:52:33 +0300 Subject: [PATCH 04/11] Body is not required --- .../action/admin/cluster/RestReloadSecureSettingsAction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f9b7f7abf366a..c075802273b33 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,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client .cluster() .prepareReloadSecureSettings() .setTimeout(request.param("timeout")) - .source(request.requiredContent(), request.getXContentType()) + .source(request.content(), request.getXContentType()) .setNodesIds(nodesIds); final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request(); return channel -> nodesRequestBuilder From 9dc75a48e28f5678cc7bc80c328250d50d898e24 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Fri, 14 Jun 2019 08:15:24 +0300 Subject: [PATCH 05/11] Content is OPTIONAL --- .../cluster/RestReloadSecureSettingsAction.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 c075802273b33..62eb9b9dfe9d6 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 @@ -56,11 +56,13 @@ public String getName() { public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { final String[] nodesIds = Strings.splitStringByCommaToArray(request.param("nodeId")); final NodesReloadSecureSettingsRequestBuilder nodesRequestBuilder = client.admin() - .cluster() - .prepareReloadSecureSettings() - .setTimeout(request.param("timeout")) - .source(request.content(), request.getXContentType()) - .setNodesIds(nodesIds); + .cluster() + .prepareReloadSecureSettings() + .setTimeout(request.param("timeout")) + .setNodesIds(nodesIds); + if (request.hasContent()) { + nodesRequestBuilder.source(request.content(), request.getXContentType()); + } final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request(); return channel -> nodesRequestBuilder .execute(new RestBuilderListener(channel) { From a515988459219623ebb055bfc05cd874dcfaf7a3 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 26 Jun 2019 11:39:01 +0300 Subject: [PATCH 06/11] Address review feedback --- .../NodesReloadSecureSettingsRequest.java | 41 +++++++++++-------- ...nsportNodesReloadSecureSettingsAction.java | 14 ++++++- 2 files changed, 35 insertions(+), 20 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 5262a90340aae..fe22e2a0de374 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,6 +19,7 @@ package org.elasticsearch.action.admin.cluster.node.reload; +import org.elasticsearch.Version; import org.elasticsearch.action.support.nodes.BaseNodesRequest; import org.elasticsearch.common.CharArrays; import org.elasticsearch.common.Nullable; @@ -72,37 +73,41 @@ public void closePassword() { } } - public boolean hasPassword() { + boolean hasPassword() { return this.secureSettingsPassword != null && this.secureSettingsPassword.length() > 0; } @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); - final BytesReference bytesRef = in.readOptionalBytesReference(); - if (bytesRef != null) { - byte[] bytes = BytesReference.toBytes(bytesRef); - try { - this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(bytes)); - } finally { - Arrays.fill(bytes, (byte) 0); + if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + final BytesReference bytesRef = in.readOptionalBytesReference(); + if (bytesRef != null) { + byte[] bytes = BytesReference.toBytes(bytesRef); + try { + this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(bytes)); + } finally { + Arrays.fill(bytes, (byte) 0); + } + } else { + this.secureSettingsPassword = null; } - } else { - this.secureSettingsPassword = null; } } @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - if (this.secureSettingsPassword == null) { - out.writeOptionalBytesReference(null); - } else { - final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars()); - try { - out.writeOptionalBytesReference(new BytesArray(passwordBytes)); - } finally { - Arrays.fill(passwordBytes, (byte) 0); + if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + if (this.secureSettingsPassword == null) { + out.writeOptionalBytesReference(null); + } else { + final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars()); + try { + out.writeOptionalBytesReference(new BytesArray(passwordBytes)); + } finally { + Arrays.fill(passwordBytes, (byte) 0); + } } } } 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 faffaa79b0d31..b1a853e1832a3 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 @@ -21,6 +21,7 @@ import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.FailedNodeException; @@ -85,11 +86,18 @@ protected NodesReloadSecureSettingsResponse.NodeResponse newNodeResponse() { protected void doExecute(Task task, NodesReloadSecureSettingsRequest request, ActionListener listener) { if (request.hasPassword() && isNodeLocal(request) == false && isNodeTransportTLSEnabled() == false) { + request.closePassword(); listener.onFailure( - new IllegalStateException("Secure settings cannot be updated cluster wide when TLS for the transport layer" + + new ElasticsearchException("Secure settings cannot be updated cluster wide when TLS for the transport layer" + " is not enabled. Enable TLS or use the API with a `_local` filter on each node.")); } else { - super.doExecute(task, request, listener); + super.doExecute(task, request, ActionListener.wrap(response -> { + request.closePassword(); + listener.onResponse(response); + }, e -> { + request.closePassword(); + listener.onFailure(e); + })); } } @@ -127,6 +135,8 @@ protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(NodeReque return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(), null); } catch (final Exception e) { return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(), e); + } finally { + secureSettingsPassword.close(); } } From 2760e5979c2448b728363b071030d27d4e076bde Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 26 Jun 2019 14:48:37 +0300 Subject: [PATCH 07/11] fix test --- .../org/elasticsearch/action/admin/ReloadSecureSettingsIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 5451a7805a43b..fbd3fe0432e6c 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java @@ -19,6 +19,7 @@ package org.elasticsearch.action.admin; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsResponse; import org.elasticsearch.common.Strings; @@ -173,7 +174,7 @@ public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) { @Override public void onFailure(Exception e) { - assertThat(e, instanceOf(IllegalStateException.class)); + assertThat(e, instanceOf(ElasticsearchException.class)); assertThat(e.getMessage(), containsString("Secure settings cannot be updated cluster wide when TLS for the transport layer is not enabled")); latch.countDown(); From 33cedf020c110ada2614a471fff5697769dd3985 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 10 Jul 2019 17:46:38 +0300 Subject: [PATCH 08/11] address feedback --- .../NodesReloadSecureSettingsRequest.java | 3 +- ...desReloadSecureSettingsRequestBuilder.java | 43 --------------- .../RestReloadSecureSettingsAction.java | 38 ++++++++----- .../RestReloadSecureSettingsActionTests.java | 53 +++++++++++++++++++ 4 files changed, 79 insertions(+), 58 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsActionTests.java 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 fe22e2a0de374..827429cfbb25c 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 @@ -62,9 +62,8 @@ public SecureString getSecureSettingsPassword() { return secureSettingsPassword; } - public NodesReloadSecureSettingsRequest setSecureStorePassword(SecureString secureStorePassword) { + public void setSecureStorePassword(SecureString secureStorePassword) { this.secureSettingsPassword = secureStorePassword; - return this; } public void closePassword() { 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 1a2830bbc3433..c3c0401efdf17 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,19 +19,9 @@ 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,8 +29,6 @@ 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()); } @@ -50,35 +38,4 @@ public NodesReloadSecureSettingsRequestBuilder setSecureStorePassword(SecureStri 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); - } - - 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; - } - } 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 62eb9b9dfe9d6..24b71b534389b 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 @@ -23,9 +23,13 @@ import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsRequestBuilder; import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsResponse; import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestController; @@ -40,6 +44,12 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; public final class RestReloadSecureSettingsAction extends BaseRestHandler { + static final ObjectParser PARSER = new ObjectParser<>("reload_secure_settings", NodesReloadSecureSettingsRequest::new); + + static { + PARSER.declareString((request, value) -> request.setSecureStorePassword(new SecureString(value.toCharArray())), + new ParseField("secure_settings_password")); + } public RestReloadSecureSettingsAction(Settings settings, RestController controller) { super(settings); @@ -54,31 +64,33 @@ public String getName() { @Override public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + final String[] nodesIds = Strings.splitStringByCommaToArray(request.param("nodeId")); final NodesReloadSecureSettingsRequestBuilder nodesRequestBuilder = client.admin() .cluster() .prepareReloadSecureSettings() .setTimeout(request.param("timeout")) .setNodesIds(nodesIds); - if (request.hasContent()) { - nodesRequestBuilder.source(request.content(), request.getXContentType()); - } - final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request(); + return channel -> nodesRequestBuilder .execute(new RestBuilderListener(channel) { @Override 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()); - builder.endObject(); - // clear password for the original request - nodesRequest.closePassword(); - return new BytesRestResponse(RestStatus.OK, builder); + throws Exception { + try (XContentParser parser = request.contentParser()) { + final NodesReloadSecureSettingsRequest nodesRequest = PARSER.parse(parser, null); + builder.startObject(); + 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.closePassword(); + return new BytesRestResponse(RestStatus.OK, builder); + } } }); + } @Override diff --git a/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsActionTests.java b/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsActionTests.java new file mode 100644 index 0000000000000..7dfd294e8ae34 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsActionTests.java @@ -0,0 +1,53 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.rest.action.admin.cluster; + +import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsRequest; +import org.elasticsearch.common.xcontent.DeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.nullValue; + +public class RestReloadSecureSettingsActionTests extends ESTestCase { + + public void testParserWithPassword() throws Exception { + final String request = "{" + + "\"secure_settings_password\": \"secure_settings_password_string\"" + + "}"; + try (XContentParser parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, request)) { + NodesReloadSecureSettingsRequest reloadSecureSettingsRequest = RestReloadSecureSettingsAction.PARSER.parse(parser, null); + assertEquals("secure_settings_password_string", reloadSecureSettingsRequest.getSecureSettingsPassword().toString()); + } + } + + public void testParserWithoutPassword() throws Exception { + final String request = "{" + + "}"; + try (XContentParser parser = XContentType.JSON.xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, request)) { + NodesReloadSecureSettingsRequest reloadSecureSettingsRequest = RestReloadSecureSettingsAction.PARSER.parse(parser, null); + assertThat(reloadSecureSettingsRequest.getSecureSettingsPassword(), nullValue()); + } + } +} From fccb76f41a377b2c44dc38812daa679eb61ac0ea Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 10 Jul 2019 18:17:44 +0300 Subject: [PATCH 09/11] checkstyle --- .../action/admin/cluster/RestReloadSecureSettingsAction.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 24b71b534389b..0849f14f97f73 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 @@ -44,7 +44,8 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; public final class RestReloadSecureSettingsAction extends BaseRestHandler { - static final ObjectParser PARSER = new ObjectParser<>("reload_secure_settings", NodesReloadSecureSettingsRequest::new); + static final ObjectParser PARSER = + new ObjectParser<>("reload_secure_settings", NodesReloadSecureSettingsRequest::new); static { PARSER.declareString((request, value) -> request.setSecureStorePassword(new SecureString(value.toCharArray())), From 01c4787ede3cdec9f99093a0be3d66357b827ba3 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 10 Jul 2019 20:11:20 +0300 Subject: [PATCH 10/11] parser might be null when the request body is empty --- .../RestReloadSecureSettingsAction.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) 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 0849f14f97f73..aadf6e18fae26 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 @@ -29,7 +29,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestController; @@ -44,6 +43,7 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; public final class RestReloadSecureSettingsAction extends BaseRestHandler { + static final ObjectParser PARSER = new ObjectParser<>("reload_secure_settings", NodesReloadSecureSettingsRequest::new); @@ -65,33 +65,33 @@ public String getName() { @Override public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - final String[] nodesIds = Strings.splitStringByCommaToArray(request.param("nodeId")); final NodesReloadSecureSettingsRequestBuilder nodesRequestBuilder = client.admin() .cluster() .prepareReloadSecureSettings() .setTimeout(request.param("timeout")) .setNodesIds(nodesIds); + request.withContentOrSourceParamParserOrNull(parser -> { + if (parser != null) { + final NodesReloadSecureSettingsRequest nodesRequest = PARSER.parse(parser, null); + nodesRequestBuilder.setSecureStorePassword(nodesRequest.getSecureSettingsPassword()); + } + }); return channel -> nodesRequestBuilder .execute(new RestBuilderListener(channel) { @Override public RestResponse buildResponse(NodesReloadSecureSettingsResponse response, XContentBuilder builder) throws Exception { - try (XContentParser parser = request.contentParser()) { - final NodesReloadSecureSettingsRequest nodesRequest = PARSER.parse(parser, null); - builder.startObject(); - 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.closePassword(); - return new BytesRestResponse(RestStatus.OK, builder); - } + builder.startObject(); + RestActions.buildNodesHeader(builder, channel.request(), response); + builder.field("cluster_name", response.getClusterName().value()); + response.toXContent(builder, channel.request()); + builder.endObject(); + nodesRequestBuilder.request().closePassword(); + return new BytesRestResponse(RestStatus.OK, builder); } }); - } @Override From 7fa964829930c10f635d6256422c8c6dc4bf3364 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 11 Jul 2019 10:18:25 +0300 Subject: [PATCH 11/11] use proper way of backporting --- build.gradle | 4 ++-- .../cluster/node/reload/NodesReloadSecureSettingsRequest.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/build.gradle b/build.gradle index 5c1fe80668283..c0fa7953097da 100644 --- a/build.gradle +++ b/build.gradle @@ -160,8 +160,8 @@ task verifyVersions { * after the backport of the backcompat code is complete. */ -boolean bwc_tests_enabled = true -final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */ +boolean bwc_tests_enabled = false +final String bwc_tests_disabled_issue = "https://github.com/elastic/elasticsearch/pull/43197" if (bwc_tests_enabled == false) { if (bwc_tests_disabled_issue.isEmpty()) { throw new GradleException("bwc_tests_disabled_issue must be set when bwc_tests_enabled == false") 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 827429cfbb25c..c22424ad4227f 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 @@ -79,7 +79,7 @@ boolean hasPassword() { @Override public void readFrom(StreamInput in) throws IOException { super.readFrom(in); - if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + if (in.getVersion().onOrAfter(Version.V_7_4_0)) { final BytesReference bytesRef = in.readOptionalBytesReference(); if (bytesRef != null) { byte[] bytes = BytesReference.toBytes(bytesRef); @@ -97,7 +97,7 @@ public void readFrom(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { super.writeTo(out); - if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + if (out.getVersion().onOrAfter(Version.V_7_4_0)) { if (this.secureSettingsPassword == null) { out.writeOptionalBytesReference(null); } else {