Skip to content

Remove passphrase support from reload settings API #32889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 16, 2018
Merged

Remove passphrase support from reload settings API #32889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7fa908
Remove passphrase support from reload settings API
jasontedor Aug 15, 2018
eaa0675
Merge branch 'master' into reload-secure-settings-passphrase
jasontedor Aug 15, 2018
8d9c8bf
Fix docs
jasontedor Aug 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 4 additions & 64 deletions .../org/elasticsearch/action/admin/cluster/node/reload/NodesReloadSecureSettingsRequest.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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