Skip to content

Allow keystore add-file to handle multiple settings #54240

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 4 commits into from
Mar 26, 2020
Merged

Allow keystore add-file to handle multiple settings #54240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d9bf373
Allow keystore add-file to handle multiple settings
jasontedor Mar 26, 2020
57d3a16
Fix long line
jasontedor Mar 26, 2020
e5ef618
Merge branch 'master' into keystore-add-multiple-file-settings
jasontedor Mar 26, 2020
84cf0de
Formatting
jasontedor Mar 26, 2020
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
54 changes: 28 additions & 26 deletions .../keystore-cli/src/main/java/org/elasticsearch/common/settings/AddFileKeyStoreCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,6 @@

package org.elasticsearch.common.settings;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;

import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import org.elasticsearch.cli.ExitCodes;
Expand All @@ -33,6 +28,11 @@
import org.elasticsearch.common.io.PathUtils;
import org.elasticsearch.env.Environment;

import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;

/**
* A subcommand for the keystore cli which adds a file setting.
*/
Expand All @@ -49,43 +49,45 @@ class AddFileKeyStoreCommand extends BaseKeyStoreCommand {
// jopt simple has issue with multiple non options, so we just get one set of them here
// and convert to File when necessary
// see https://github.com/jopt-simple/jopt-simple/issues/103
this.arguments = parser.nonOptions("setting [filepath]");
this.arguments = parser.nonOptions("(setting path)+");
}

@Override
protected void executeCommand(Terminal terminal, OptionSet options, Environment env) throws Exception {
List<String> argumentValues = arguments.values(options);
final List<String> argumentValues = arguments.values(options);
if (argumentValues.size() == 0) {
throw new UserException(ExitCodes.USAGE, "Missing setting name");
}
String setting = argumentValues.get(0);
if (argumentValues.size() % 2 != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to also ensure size > 1? Otherwise I think we silently ignore giving the add-file command with no arguments?

Copy link
Member Author

@jasontedor jasontedor Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's handled on the previous if? If size < 1 then it must be == 0 and we bail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if currently on line 58 of this diff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't believe I missed that. :shame:

throw new UserException(ExitCodes.USAGE, "settings and filenames must come in pairs");
}

final KeyStoreWrapper keyStore = getKeyStore();
if (keyStore.getSettingNames().contains(setting) && options.has(forceOption) == false) {
if (terminal.promptYesNo("Setting " + setting + " already exists. Overwrite?", false) == false) {
terminal.println("Exiting without modifying keystore.");
return;

for (int i = 0; i < argumentValues.size(); i += 2) {
final String setting = argumentValues.get(i);

if (keyStore.getSettingNames().contains(setting) && options.has(forceOption) == false) {
if (terminal.promptYesNo("Setting " + setting + " already exists. Overwrite?", false) == false) {
terminal.println("Exiting without modifying keystore.");
return;
}
}
}

if (argumentValues.size() == 1) {
throw new UserException(ExitCodes.USAGE, "Missing file name");
}
Path file = getPath(argumentValues.get(1));
if (Files.exists(file) == false) {
throw new UserException(ExitCodes.IO_ERROR, "File [" + file.toString() + "] does not exist");
}
if (argumentValues.size() > 2) {
throw new UserException(
ExitCodes.USAGE,
"Unrecognized extra arguments [" + String.join(", ", argumentValues.subList(2, argumentValues.size())) + "] after filepath"
);
final Path file = getPath(argumentValues.get(i + 1));
if (Files.exists(file) == false) {
throw new UserException(ExitCodes.IO_ERROR, "File [" + file.toString() + "] does not exist");
}

keyStore.setFile(setting, Files.readAllBytes(file));
}
keyStore.setFile(setting, Files.readAllBytes(file));

keyStore.save(env.configFile(), getKeyStorePassword().getChars());
}

@SuppressForbidden(reason = "file arg for cli")
private Path getPath(String file) {
return PathUtils.get(file);
}

}
36 changes: 28 additions & 8 deletions ...tore-cli/src/test/java/org/elasticsearch/common/settings/AddFileKeyStoreCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,20 @@

package org.elasticsearch.common.settings;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Map;

import org.elasticsearch.cli.Command;
import org.elasticsearch.cli.ExitCodes;
import org.elasticsearch.cli.UserException;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.env.Environment;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.stream.Stream;

import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsString;

Expand All @@ -49,7 +53,7 @@ private Path createRandomFile() throws IOException {
for (int i = 0; i < length; ++i) {
bytes[i] = randomByte();
}
Path file = env.configFile().resolve("randomfile");
Path file = env.configFile().resolve(randomAlphaOfLength(16));
Files.write(file, bytes);
return file;
}
Expand Down Expand Up @@ -164,7 +168,7 @@ public void testMissingFileName() throws Exception {
terminal.addSecretInput(password);
UserException e = expectThrows(UserException.class, () -> execute("foo"));
assertEquals(ExitCodes.USAGE, e.exitCode);
assertThat(e.getMessage(), containsString("Missing file name"));
assertThat(e.getMessage(), containsString("settings and filenames must come in pairs"));
}

public void testFileDNE() throws Exception {
Expand All @@ -183,7 +187,7 @@ public void testExtraArguments() throws Exception {
terminal.addSecretInput(password);
UserException e = expectThrows(UserException.class, () -> execute("foo", file.toString(), "bar"));
assertEquals(e.getMessage(), ExitCodes.USAGE, e.exitCode);
assertThat(e.getMessage(), containsString("Unrecognized extra arguments [bar]"));
assertThat(e.getMessage(), containsString("settings and filenames must come in pairs"));
}

public void testIncorrectPassword() throws Exception {
Expand Down Expand Up @@ -216,4 +220,20 @@ public void testAddToUnprotectedKeystore() throws Exception {
execute("foo", "path/dne");
assertSecureFile("foo", file, password);
}

public void testAddMultipleFiles() throws Exception {
final String password = "keystorepassword";
createKeystore(password);
final int n = randomIntBetween(1, 8);
final List<Tuple<String, Path>> settingFilePairs = new ArrayList<>(n);
for (int i = 0; i < n; i++) {
settingFilePairs.add(Tuple.tuple("foo" + i, createRandomFile()));
}
terminal.addSecretInput(password);
execute(settingFilePairs.stream().flatMap(t -> Stream.of(t.v1(), t.v2().toString())).toArray(String[]::new));
for (int i = 0; i < n; i++) {
assertSecureFile(settingFilePairs.get(i).v1(), settingFilePairs.get(i).v2(), password);
}
}

}
17 changes: 13 additions & 4 deletions docs/reference/commands/keystore.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ in the {es} keystore.
--------------------------------------------------
bin/elasticsearch-keystore
([add <settings>] [-f] [--stdin] |
[add-file <setting> <path>] | [create] [-p] |
[add-file (<setting> <path>)+] | [create] [-p] |
[list] | [passwd] | [remove <setting>] | [upgrade])
[-h, --help] ([-s, --silent] | [-v, --verbose])
--------------------------------------------------
Expand Down Expand Up @@ -48,7 +48,7 @@ must confirm that you want to overwrite the current value. If the keystore does
not exist, you must confirm that you want to create a keystore. To avoid these
two confirmation prompts, use the `-f` parameter.

`add-file <setting> <path>`:: Adds a file to the keystore.
`add-file (<setting> <path>)+`:: Adds files to the keystore.

`create`:: Creates the keystore.

Expand Down Expand Up @@ -173,14 +173,23 @@ Values for multiple settings must be separated by carriage returns or newlines.
==== Add files to the keystore

You can add sensitive files, like authentication key files for Cloud plugins,
using the `add-file` command. Be sure to include your file path as an argument
after the setting name.
using the `add-file` command. Settings and file paths are specified in pairs
consisting of `setting path`.

[source,sh]
----------------------------------------------------------------
bin/elasticsearch-keystore add-file the.setting.name.to.set /path/example-file.json
----------------------------------------------------------------

You can add multiple files with the `add-file` command:

[source,sh]
----------------------------------------------------------------
bin/elasticsearch-keystore add-file \
the.setting.name.to.set /path/example-file.json \
the.other.setting.name.to.set /path/other-example-file.json
----------------------------------------------------------------

If the {es} keystore is password protected, you are prompted to enter the
password.

Expand Down