Skip to content

Commit 973dc58

Browse files
jasontedorjasontedor
committed
Remove passphrase support from reload settings API (#32889)
We do not support passphrases on the secure settings storage (the keystore). Yet, we added support for this in the API layer. This commit removes this support so that we are not limited in our future options, or have to make a breaking change.
1 parent 1b5faaf commit 973dc58

File tree

5 files changed

+16
-216
lines changed

5 files changed

+16
-216
lines changed

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

Lines changed: 4 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -19,82 +19,22 @@
1919

2020
package org.elasticsearch.action.admin.cluster.node.reload;
2121

22-
23-
import org.elasticsearch.action.ActionRequestValidationException;
2422
import org.elasticsearch.action.support.nodes.BaseNodesRequest;
25-
import org.elasticsearch.common.CharArrays;
26-
import org.elasticsearch.common.io.stream.StreamInput;
27-
import org.elasticsearch.common.io.stream.StreamOutput;
28-
import org.elasticsearch.common.settings.SecureString;
29-
30-
import java.io.IOException;
31-
import java.util.Arrays;
32-
33-
import static org.elasticsearch.action.ValidateActions.addValidationError;
3423

3524
/**
36-
* Request for a reload secure settings action
25+
* Request for a reload secure settings action.
3726
*/
3827
public class NodesReloadSecureSettingsRequest extends BaseNodesRequest<NodesReloadSecureSettingsRequest> {
3928

40-
/**
41-
* The password which is broadcasted to all nodes, but is never stored on
42-
* persistent storage. The password is used to reread and decrypt the contents
43-
* of the node's keystore (backing the implementation of
44-
* {@code SecureSettings}).
45-
*/
46-
private SecureString secureSettingsPassword;
47-
4829
public NodesReloadSecureSettingsRequest() {
4930
}
5031

5132
/**
52-
* Reload secure settings only on certain nodes, based on the nodes ids
53-
* specified. If none are passed, secure settings will be reloaded on all the
54-
* nodes.
33+
* Reload secure settings only on certain nodes, based on the nodes IDs specified. If none are passed, secure settings will be reloaded
34+
* on all the nodes.
5535
*/
56-
public NodesReloadSecureSettingsRequest(String... nodesIds) {
36+
public NodesReloadSecureSettingsRequest(final String... nodesIds) {
5737
super(nodesIds);
5838
}
5939

60-
@Override
61-
public ActionRequestValidationException validate() {
62-
ActionRequestValidationException validationException = null;
63-
if (secureSettingsPassword == null) {
64-
validationException = addValidationError("secure settings password cannot be null (use empty string instead)",
65-
validationException);
66-
}
67-
return validationException;
68-
}
69-
70-
public SecureString secureSettingsPassword() {
71-
return secureSettingsPassword;
72-
}
73-
74-
public NodesReloadSecureSettingsRequest secureStorePassword(SecureString secureStorePassword) {
75-
this.secureSettingsPassword = secureStorePassword;
76-
return this;
77-
}
78-
79-
@Override
80-
public void readFrom(StreamInput in) throws IOException {
81-
super.readFrom(in);
82-
final byte[] passwordBytes = in.readByteArray();
83-
try {
84-
this.secureSettingsPassword = new SecureString(CharArrays.utf8BytesToChars(passwordBytes));
85-
} finally {
86-
Arrays.fill(passwordBytes, (byte) 0);
87-
}
88-
}
89-
90-
@Override
91-
public void writeTo(StreamOutput out) throws IOException {
92-
super.writeTo(out);
93-
final byte[] passwordBytes = CharArrays.toUtf8Bytes(this.secureSettingsPassword.getChars());
94-
try {
95-
out.writeByteArray(passwordBytes);
96-
} finally {
97-
Arrays.fill(passwordBytes, (byte) 0);
98-
}
99-
}
10040
}

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

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -19,66 +19,17 @@
1919

2020
package org.elasticsearch.action.admin.cluster.node.reload;
2121

22-
import org.elasticsearch.ElasticsearchParseException;
2322
import org.elasticsearch.action.support.nodes.NodesOperationRequestBuilder;
2423
import org.elasticsearch.client.ElasticsearchClient;
25-
import org.elasticsearch.common.bytes.BytesReference;
26-
import org.elasticsearch.common.settings.SecureString;
27-
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
28-
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
29-
import org.elasticsearch.common.xcontent.XContentParser;
30-
import org.elasticsearch.common.xcontent.XContentType;
31-
32-
import java.io.IOException;
33-
import java.io.InputStream;
34-
import java.util.Objects;
3524

3625
/**
3726
* Builder for the reload secure settings nodes request
3827
*/
3928
public class NodesReloadSecureSettingsRequestBuilder extends NodesOperationRequestBuilder<NodesReloadSecureSettingsRequest,
4029
NodesReloadSecureSettingsResponse, NodesReloadSecureSettingsRequestBuilder> {
4130

42-
public static final String SECURE_SETTINGS_PASSWORD_FIELD_NAME = "secure_settings_password";
43-
4431
public NodesReloadSecureSettingsRequestBuilder(ElasticsearchClient client, NodesReloadSecureSettingsAction action) {
4532
super(client, action, new NodesReloadSecureSettingsRequest());
4633
}
4734

48-
public NodesReloadSecureSettingsRequestBuilder setSecureStorePassword(SecureString secureStorePassword) {
49-
request.secureStorePassword(secureStorePassword);
50-
return this;
51-
}
52-
53-
public NodesReloadSecureSettingsRequestBuilder source(BytesReference source, XContentType xContentType) throws IOException {
54-
Objects.requireNonNull(xContentType);
55-
// EMPTY is ok here because we never call namedObject
56-
try (InputStream stream = source.streamInput();
57-
XContentParser parser = xContentType.xContent().createParser(NamedXContentRegistry.EMPTY,
58-
LoggingDeprecationHandler.INSTANCE, stream)) {
59-
XContentParser.Token token;
60-
token = parser.nextToken();
61-
if (token != XContentParser.Token.START_OBJECT) {
62-
throw new ElasticsearchParseException("expected an object, but found token [{}]", token);
63-
}
64-
token = parser.nextToken();
65-
if (token != XContentParser.Token.FIELD_NAME || false == SECURE_SETTINGS_PASSWORD_FIELD_NAME.equals(parser.currentName())) {
66-
throw new ElasticsearchParseException("expected a field named [{}], but found [{}]", SECURE_SETTINGS_PASSWORD_FIELD_NAME,
67-
token);
68-
}
69-
token = parser.nextToken();
70-
if (token != XContentParser.Token.VALUE_STRING) {
71-
throw new ElasticsearchParseException("expected field [{}] to be of type string, but found [{}] instead",
72-
SECURE_SETTINGS_PASSWORD_FIELD_NAME, token);
73-
}
74-
final String password = parser.text();
75-
setSecureStorePassword(new SecureString(password.toCharArray()));
76-
token = parser.nextToken();
77-
if (token != XContentParser.Token.END_OBJECT) {
78-
throw new ElasticsearchParseException("expected end of object, but found token [{}]", token);
79-
}
80-
}
81-
return this;
82-
}
83-
8435
}

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import org.elasticsearch.common.io.stream.StreamInput;
3333
import org.elasticsearch.common.io.stream.StreamOutput;
3434
import org.elasticsearch.common.settings.KeyStoreWrapper;
35-
import org.elasticsearch.common.settings.SecureString;
3635
import org.elasticsearch.common.settings.Settings;
3736
import org.elasticsearch.env.Environment;
3837
import org.elasticsearch.plugins.PluginsService;
@@ -83,16 +82,13 @@ protected NodesReloadSecureSettingsResponse.NodeResponse newNodeResponse() {
8382

8483
@Override
8584
protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation(NodeRequest nodeReloadRequest) {
86-
final NodesReloadSecureSettingsRequest request = nodeReloadRequest.request;
87-
final SecureString secureSettingsPassword = request.secureSettingsPassword();
8885
try (KeyStoreWrapper keystore = KeyStoreWrapper.load(environment.configFile())) {
8986
// reread keystore from config file
9087
if (keystore == null) {
9188
return new NodesReloadSecureSettingsResponse.NodeResponse(clusterService.localNode(),
9289
new IllegalStateException("Keystore is missing"));
9390
}
94-
// decrypt the keystore using the password from the request
95-
keystore.decrypt(secureSettingsPassword.getChars());
91+
keystore.decrypt(new char[0]);
9692
// add the keystore to the original node settings object
9793
final Settings settingsWithKeystore = Settings.builder()
9894
.put(environment.settings(), false)

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
5959
.cluster()
6060
.prepareReloadSecureSettings()
6161
.setTimeout(request.param("timeout"))
62-
.source(request.requiredContent(), request.getXContentType())
6362
.setNodesIds(nodesIds);
6463
final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request();
6564
return channel -> nodesRequestBuilder
@@ -68,12 +67,12 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client
6867
public RestResponse buildResponse(NodesReloadSecureSettingsResponse response, XContentBuilder builder)
6968
throws Exception {
7069
builder.startObject();
71-
RestActions.buildNodesHeader(builder, channel.request(), response);
72-
builder.field("cluster_name", response.getClusterName().value());
73-
response.toXContent(builder, channel.request());
70+
{
71+
RestActions.buildNodesHeader(builder, channel.request(), response);
72+
builder.field("cluster_name", response.getClusterName().value());
73+
response.toXContent(builder, channel.request());
74+
}
7475
builder.endObject();
75-
// clear password for the original request
76-
nodesRequest.secureSettingsPassword().close();
7776
return new BytesRestResponse(RestStatus.OK, builder);
7877
}
7978
});

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

Lines changed: 6 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,9 @@
2020
package org.elasticsearch.action.admin;
2121

2222
import org.elasticsearch.action.ActionListener;
23-
import org.elasticsearch.action.ActionRequestValidationException;
2423
import org.elasticsearch.action.admin.cluster.node.reload.NodesReloadSecureSettingsResponse;
2524
import org.elasticsearch.common.settings.KeyStoreWrapper;
2625
import org.elasticsearch.common.settings.SecureSettings;
27-
import org.elasticsearch.common.settings.SecureString;
2826
import org.elasticsearch.common.settings.Settings;
2927
import org.elasticsearch.env.Environment;
3028
import org.elasticsearch.plugins.Plugin;
@@ -44,11 +42,11 @@
4442
import java.util.concurrent.CountDownLatch;
4543
import java.util.concurrent.atomic.AtomicReference;
4644

45+
import static org.hamcrest.Matchers.containsString;
4746
import static org.hamcrest.Matchers.equalTo;
47+
import static org.hamcrest.Matchers.instanceOf;
4848
import static org.hamcrest.Matchers.notNullValue;
4949
import static org.hamcrest.Matchers.nullValue;
50-
import static org.hamcrest.Matchers.instanceOf;
51-
import static org.hamcrest.Matchers.containsString;
5250

5351
public class ReloadSecureSettingsIT extends ESIntegTestCase {
5452

@@ -62,7 +60,7 @@ public void testMissingKeystoreFile() throws Exception {
6260
Files.deleteIfExists(KeyStoreWrapper.keystorePath(environment.configFile()));
6361
final int initialReloadCount = mockReloadablePlugin.getReloadCount();
6462
final CountDownLatch latch = new CountDownLatch(1);
65-
client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
63+
client().admin().cluster().prepareReloadSecureSettings().execute(
6664
new ActionListener<NodesReloadSecureSettingsResponse>() {
6765
@Override
6866
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
@@ -96,44 +94,6 @@ public void onFailure(Exception e) {
9694
assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
9795
}
9896

99-
public void testNullKeystorePassword() throws Exception {
100-
final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
101-
final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class)
102-
.stream().findFirst().get();
103-
final AtomicReference<AssertionError> reloadSettingsError = new AtomicReference<>();
104-
final int initialReloadCount = mockReloadablePlugin.getReloadCount();
105-
final CountDownLatch latch = new CountDownLatch(1);
106-
client().admin().cluster().prepareReloadSecureSettings().execute(
107-
new ActionListener<NodesReloadSecureSettingsResponse>() {
108-
@Override
109-
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
110-
try {
111-
reloadSettingsError.set(new AssertionError("Null keystore password should fail"));
112-
} finally {
113-
latch.countDown();
114-
}
115-
}
116-
117-
@Override
118-
public void onFailure(Exception e) {
119-
try {
120-
assertThat(e, instanceOf(ActionRequestValidationException.class));
121-
assertThat(e.getMessage(), containsString("secure settings password cannot be null"));
122-
} catch (final AssertionError ae) {
123-
reloadSettingsError.set(ae);
124-
} finally {
125-
latch.countDown();
126-
}
127-
}
128-
});
129-
latch.await();
130-
if (reloadSettingsError.get() != null) {
131-
throw reloadSettingsError.get();
132-
}
133-
// in the null password case no reload should be triggered
134-
assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
135-
}
136-
13797
public void testInvalidKeystoreFile() throws Exception {
13898
final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
13999
final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class)
@@ -149,7 +109,7 @@ public void testInvalidKeystoreFile() throws Exception {
149109
Files.copy(keystore, KeyStoreWrapper.keystorePath(environment.configFile()), StandardCopyOption.REPLACE_EXISTING);
150110
}
151111
final CountDownLatch latch = new CountDownLatch(1);
152-
client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
112+
client().admin().cluster().prepareReloadSecureSettings().execute(
153113
new ActionListener<NodesReloadSecureSettingsResponse>() {
154114
@Override
155115
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
@@ -181,52 +141,6 @@ public void onFailure(Exception e) {
181141
assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
182142
}
183143

184-
public void testWrongKeystorePassword() throws Exception {
185-
final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
186-
final MockReloadablePlugin mockReloadablePlugin = pluginsService.filterPlugins(MockReloadablePlugin.class)
187-
.stream().findFirst().get();
188-
final Environment environment = internalCluster().getInstance(Environment.class);
189-
final AtomicReference<AssertionError> reloadSettingsError = new AtomicReference<>();
190-
final int initialReloadCount = mockReloadablePlugin.getReloadCount();
191-
// "some" keystore should be present in this case
192-
writeEmptyKeystore(environment, new char[0]);
193-
final CountDownLatch latch = new CountDownLatch(1);
194-
client().admin()
195-
.cluster()
196-
.prepareReloadSecureSettings()
197-
.setSecureStorePassword(new SecureString(new char[] { 'W', 'r', 'o', 'n', 'g' }))
198-
.execute(new ActionListener<NodesReloadSecureSettingsResponse>() {
199-
@Override
200-
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
201-
try {
202-
assertThat(nodesReloadResponse, notNullValue());
203-
final Map<String, NodesReloadSecureSettingsResponse.NodeResponse> nodesMap = nodesReloadResponse.getNodesMap();
204-
assertThat(nodesMap.size(), equalTo(cluster().size()));
205-
for (final NodesReloadSecureSettingsResponse.NodeResponse nodeResponse : nodesReloadResponse.getNodes()) {
206-
assertThat(nodeResponse.reloadException(), notNullValue());
207-
assertThat(nodeResponse.reloadException(), instanceOf(SecurityException.class));
208-
}
209-
} catch (final AssertionError e) {
210-
reloadSettingsError.set(e);
211-
} finally {
212-
latch.countDown();
213-
}
214-
}
215-
216-
@Override
217-
public void onFailure(Exception e) {
218-
reloadSettingsError.set(new AssertionError("Nodes request failed", e));
219-
latch.countDown();
220-
}
221-
});
222-
latch.await();
223-
if (reloadSettingsError.get() != null) {
224-
throw reloadSettingsError.get();
225-
}
226-
// in the wrong password case no reload should be triggered
227-
assertThat(mockReloadablePlugin.getReloadCount(), equalTo(initialReloadCount));
228-
}
229-
230144
public void testMisbehavingPlugin() throws Exception {
231145
final Environment environment = internalCluster().getInstance(Environment.class);
232146
final PluginsService pluginsService = internalCluster().getInstance(PluginsService.class);
@@ -247,7 +161,7 @@ public void testMisbehavingPlugin() throws Exception {
247161
.get(Settings.builder().put(environment.settings()).setSecureSettings(secureSettings).build())
248162
.toString();
249163
final CountDownLatch latch = new CountDownLatch(1);
250-
client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
164+
client().admin().cluster().prepareReloadSecureSettings().execute(
251165
new ActionListener<NodesReloadSecureSettingsResponse>() {
252166
@Override
253167
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {
@@ -314,7 +228,7 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
314228
private void successfulReloadCall() throws InterruptedException {
315229
final AtomicReference<AssertionError> reloadSettingsError = new AtomicReference<>();
316230
final CountDownLatch latch = new CountDownLatch(1);
317-
client().admin().cluster().prepareReloadSecureSettings().setSecureStorePassword(new SecureString(new char[0])).execute(
231+
client().admin().cluster().prepareReloadSecureSettings().execute(
318232
new ActionListener<NodesReloadSecureSettingsResponse>() {
319233
@Override
320234
public void onResponse(NodesReloadSecureSettingsResponse nodesReloadResponse) {

0 commit comments

Comments
 (0)