Skip to content

Commit

Permalink
Remove passphrase support from reload settings API (#32889)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jasontedor @jimczi
jasontedor authored and jimczi committed Aug 17, 2018
1 parent 73d8a5d commit f9a6bdf
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 216 deletions.
68 changes: 4 additions & 64 deletions .../org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
49 changes: 0 additions & 49 deletions ...asticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequestBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,66 +19,17 @@

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
*/
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;
}

}
6 changes: 1 addition & 5 deletions ...sticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 5 additions & 6 deletions ...main/java/org/elasticsearch/rest/action/admin/cluster/RestReloadSecureSettingsAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -68,12 +67,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);
}
});
Expand Down
98 changes: 6 additions & 92 deletions server/src/test/java/org/elasticsearch/action/admin/ReloadSecureSettingsIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {

Expand All @@ -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<NodesReloadSecureSettingsResponse>() {
@Override
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
Expand Down Expand Up @@ -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<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)
Expand All @@ -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<NodesReloadSecureSettingsResponse>() {
@Override
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
Expand Down Expand Up @@ -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<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);
Expand All @@ -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<NodesReloadSecureSettingsResponse>() {
@Override
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
Expand Down Expand Up @@ -314,7 +228,7 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
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) {
Expand Down

0 comments on commit f9a6bdf

Please sign in to comment.