-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix ReloadSecureSettings API to consume password #54771
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.
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
Changes from all commits
4969378
3beb92c
d44163d
92c6cfb
6ad7dfb
ed1cc00
e27bddf
aa5358f
8427317
146eefd
ee31279
78c7b39
5947a16
2b63231
9bb7583
5233d90
cf6315a
f07b504
c4a8496
f740fd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,6 +141,7 @@ public class ElasticsearchNode implements TestClusterConfiguration { | |
| private final Path httpPortsFile; | ||
| private final Path esStdoutFile; | ||
| private final Path esStderrFile; | ||
| private final Path esStdinFile; | ||
| private final Path tmpDir; | ||
|
|
||
| private int currentDistro = 0; | ||
|
|
@@ -152,6 +153,7 @@ public class ElasticsearchNode implements TestClusterConfiguration { | |
| private String httpPort = "0"; | ||
| private String transportPort = "0"; | ||
| private Path confPathData; | ||
| private String keystorePassword = ""; | ||
|
|
||
| ElasticsearchNode(String path, String name, Project project, ReaperService reaper, File workingDirBase) { | ||
| this.path = path; | ||
|
|
@@ -167,6 +169,7 @@ public class ElasticsearchNode implements TestClusterConfiguration { | |
| httpPortsFile = confPathLogs.resolve("http.ports"); | ||
| esStdoutFile = confPathLogs.resolve("es.stdout.log"); | ||
| esStderrFile = confPathLogs.resolve("es.stderr.log"); | ||
| esStdinFile = workingDir.resolve("es.stdin"); | ||
| tmpDir = workingDir.resolve("tmp"); | ||
| waitConditions.put("ports files", this::checkPortsFilesExistWithDelay); | ||
|
|
||
|
|
@@ -302,6 +305,11 @@ public void keystore(String key, FileSupplier valueSupplier) { | |
| keystoreFiles.put(key, valueSupplier); | ||
| } | ||
|
|
||
| @Override | ||
| public void keystorePassword(String password) { | ||
| keystorePassword = password; | ||
| } | ||
|
|
||
| @Override | ||
| public void cliSetup(String binTool, CharSequence... args) { | ||
| cliSetup.add(new CliEntry(binTool, args)); | ||
|
|
@@ -426,21 +434,25 @@ public synchronized void start() { | |
| logToProcessStdout("installed plugins"); | ||
| } | ||
|
|
||
| logToProcessStdout("Creating elasticsearch keystore with password set to [" + keystorePassword + "]"); | ||
| if (keystorePassword.length() > 0) { | ||
| runElasticsearchBinScriptWithInput(keystorePassword + "\n" + keystorePassword, "elasticsearch-keystore", "create", "-p"); | ||
| } else { | ||
| runElasticsearchBinScript("elasticsearch-keystore", "create"); | ||
| } | ||
|
|
||
| if (keystoreSettings.isEmpty() == false || keystoreFiles.isEmpty() == false) { | ||
| logToProcessStdout("Adding " + keystoreSettings.size() + " keystore settings and " + keystoreFiles.size() + " keystore files"); | ||
| runElasticsearchBinScript("elasticsearch-keystore", "create"); | ||
|
|
||
| keystoreSettings.forEach( | ||
| (key, value) -> runElasticsearchBinScriptWithInput(value.toString(), "elasticsearch-keystore", "add", "-x", key) | ||
| ); | ||
| keystoreSettings.forEach((key, value) -> runKeystoreCommandWithPassword(keystorePassword, value.toString(), "add", "-x", key)); | ||
|
|
||
| for (Map.Entry<String, File> entry : keystoreFiles.entrySet()) { | ||
| File file = entry.getValue(); | ||
| requireNonNull(file, "supplied keystoreFile was null when configuring " + this); | ||
| if (file.exists() == false) { | ||
| throw new TestClustersException("supplied keystore file " + file + " does not exist, require for " + this); | ||
| } | ||
| runElasticsearchBinScript("elasticsearch-keystore", "add-file", entry.getKey(), file.getAbsolutePath()); | ||
| runKeystoreCommandWithPassword(keystorePassword, "", "add-file", entry.getKey(), file.getAbsolutePath()); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -644,6 +656,11 @@ private void runElasticsearchBinScriptWithInput(String input, String tool, CharS | |
| } | ||
| } | ||
|
|
||
| private void runKeystoreCommandWithPassword(String keystorePassword, String input, CharSequence... args) { | ||
| final String actualInput = keystorePassword.length() > 0 ? keystorePassword + "\n" + input : input; | ||
| runElasticsearchBinScriptWithInput(actualInput, "elasticsearch-keystore", args); | ||
| } | ||
|
|
||
| private void runElasticsearchBinScript(String tool, CharSequence... args) { | ||
| runElasticsearchBinScriptWithInput("", tool, args); | ||
| } | ||
|
|
@@ -720,6 +737,14 @@ private void startElasticsearchProcess() { | |
| processBuilder.redirectError(ProcessBuilder.Redirect.appendTo(esStderrFile.toFile())); | ||
| processBuilder.redirectOutput(ProcessBuilder.Redirect.appendTo(esStdoutFile.toFile())); | ||
|
|
||
| if (keystorePassword != null && keystorePassword.length() > 0) { | ||
| try { | ||
| Files.write(esStdinFile, (keystorePassword + "\n").getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE); | ||
| processBuilder.redirectInput(esStdinFile.toFile()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nits: the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do we gain by doing so? If
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes no difference in actual execution outcome for this particular scenario. It's a choice of style, keep error handling specific and accurate to what is targeting for. It's akin to formatting and method extraction, but a bit more: if future changes make the extra method calls throw the same exception, it will not be handled automatically and potentially unwanted by the broader-than-necessary try-catch block. It does not apply to this particular case, that's why I called it "nits". But woud anyhow prefer keeping any context as small as possible.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, gotcha |
||
| } catch (IOException e) { | ||
| throw new TestClustersException("Failed to set the keystore password for " + this, e); | ||
| } | ||
| } | ||
| LOGGER.info("Running `{}` in `{}` for {} env: {}", command, workingDir, this, environment); | ||
| try { | ||
| esProcess = processBuilder.start(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,28 @@ | ||
| --- | ||
| "node_reload_secure_settings test": | ||
| "node_reload_secure_settings test wrong password": | ||
|
|
||
| - do: | ||
| nodes.reload_secure_settings: | ||
| node_id: _local | ||
| body: | ||
| secure_settings_password: awrongpasswordhere | ||
| - set: | ||
| nodes._arbitrary_key_: node_id | ||
|
|
||
| - is_true: nodes | ||
| - is_true: cluster_name | ||
| - match: { nodes.$node_id.reload_exception.type: "security_exception" } | ||
| - match: { nodes.$node_id.reload_exception.reason: "Provided keystore password was incorrect" } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a version with no password as well? |
||
| --- | ||
| "node_reload_secure_settings test correct(empty) password": | ||
|
|
||
| - do: | ||
| nodes.reload_secure_settings: {} | ||
|
|
||
| - set: | ||
| nodes._arbitrary_key_: node_id | ||
|
|
||
| - is_true: nodes | ||
| - is_true: cluster_name | ||
| - is_false: nodes.$node_id.reload_exception | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,7 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client | |
| .setNodesIds(nodesIds); | ||
| request.withContentOrSourceParamParserOrNull(parser -> { | ||
| if (parser != null) { | ||
| final NodesReloadSecureSettingsRequest nodesRequest = nodesRequestBuilder.request(); | ||
| final NodesReloadSecureSettingsRequest nodesRequest = PARSER.parse(parser, null); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the result of a master merge gone wrong in the feature branch : 4780880#diff-0187f4b109fbd7256c987cceae7691caL76 This is the actual fix and the only change necessary. If the buildSrc changes need further discussion, we could just merge the fix and the rest of the test changes when ready. |
||
| nodesRequestBuilder.setSecureStorePassword(nodesRequest.getSecureSettingsPassword()); | ||
| } | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| /* | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason this is created in x-pack is that calling the reload settings api with a password require transport SSL to be configured. We could move this to |
||
| * 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. | ||
| */ | ||
|
|
||
| /* | ||
| * Tests that need to run against an Elasticsearch cluster that | ||
| * is using a password protected keystore in its nodes. | ||
| */ | ||
|
|
||
| apply plugin: 'elasticsearch.testclusters' | ||
| apply plugin: 'elasticsearch.standalone-rest-test' | ||
| apply plugin: 'elasticsearch.rest-test' | ||
| dependencies { | ||
| testCompile project(path: xpackModule('core'), configuration: 'default') | ||
| } | ||
|
|
||
| testClusters.integTest { | ||
| testDistribution = 'DEFAULT' | ||
| numberOfNodes = 2 | ||
| keystorePassword 's3cr3t' | ||
|
|
||
| setting 'xpack.security.enabled', 'true' | ||
| setting 'xpack.security.authc.anonymous.roles', 'anonymous' | ||
| setting 'xpack.security.transport.ssl.enabled', 'true' | ||
| setting 'xpack.security.transport.ssl.certificate', 'transport.crt' | ||
| setting 'xpack.security.transport.ssl.key', 'transport.key' | ||
| setting 'xpack.security.transport.ssl.key_passphrase', 'transport-password' | ||
| setting 'xpack.security.transport.ssl.certificate_authorities', 'ca.crt' | ||
|
|
||
| extraConfigFile 'transport.key', file('src/test/resources/ssl/transport.key') | ||
| extraConfigFile 'transport.crt', file('src/test/resources/ssl/transport.crt') | ||
| extraConfigFile 'ca.crt', file('src/test/resources/ssl/ca.crt') | ||
| extraConfigFile 'roles.yml', file('src/test/resources/roles.yml') | ||
|
|
||
| user username: 'admin_user', password: 'admin-password' | ||
| user username:'test-user' ,password: 'test-password', role: 'user_role' | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm not opposed to this, I wonder how useful it is in practice since we have no ability to set values in the keystore via the
runtask?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm not sold on it either. And given that we currently don't have a way to set keystore properties in the
runtask, maybe it's not worth adding ? I don't know , it seemed like a good idea when I added it but I'm not so sure nowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is only relevant when compiled with the
--data-diroption to point at an existing directory which might have a manually setup keystore?