From 99c6b22003653ea63ab7cf9bf8a97629407dbde1 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 25 Mar 2020 16:02:00 -0400 Subject: [PATCH 1/3] Allow keystore add to handle multiple settings Today the keystore add command can only handle adding a single setting/value pair in a single invocation. This incurs the startup costs of the JVM many times, which in some environments can be expensive. This commit teaches the add keystore command to accept adding multiple settings in a single invocation. --- .../settings/AddStringKeyStoreCommand.java | 88 +++++++++++-------- .../AddStringKeyStoreCommandTests.java | 36 ++++++-- docs/reference/commands/keystore.asciidoc | 38 +++++--- 3 files changed, 108 insertions(+), 54 deletions(-) diff --git a/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java b/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java index 421ea6eaee066..67db9f0557b1c 100644 --- a/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java +++ b/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java @@ -19,20 +19,24 @@ package org.elasticsearch.common.settings; -import java.io.BufferedReader; -import java.io.CharArrayWriter; -import java.io.InputStream; -import java.io.InputStreamReader; -import java.nio.charset.StandardCharsets; -import java.util.Arrays; - import joptsimple.OptionSet; import joptsimple.OptionSpec; import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.Terminal; import org.elasticsearch.cli.UserException; +import org.elasticsearch.common.CheckedFunction; import org.elasticsearch.env.Environment; +import java.io.BufferedReader; +import java.io.CharArrayWriter; +import java.io.Closeable; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import java.util.List; + /** * A subcommand for the keystore cli which adds a string setting. */ @@ -42,13 +46,13 @@ class AddStringKeyStoreCommand extends BaseKeyStoreCommand { private final OptionSpec arguments; AddStringKeyStoreCommand() { - super("Add a string setting to the keystore", false); - this.stdinOption = parser.acceptsAll(Arrays.asList("x", "stdin"), "Read setting value from stdin"); + super("Add string settings to the keystore", false); + this.stdinOption = parser.acceptsAll(Arrays.asList("x", "stdin"), "Read setting values from stdin"); this.forceOption = parser.acceptsAll( Arrays.asList("f", "force"), "Overwrite existing setting without prompting, creating keystore if necessary" ); - this.arguments = parser.nonOptions("setting name"); + this.arguments = parser.nonOptions("setting names"); } // pkg private so tests can manipulate @@ -58,43 +62,53 @@ InputStream getStdin() { @Override protected void executeCommand(Terminal terminal, OptionSet options, Environment env) throws Exception { - String setting = arguments.value(options); - if (setting == null) { - throw new UserException(ExitCodes.USAGE, "The setting name can not be null"); + final List settings = arguments.values(options); + if (settings.isEmpty()) { + throw new UserException(ExitCodes.USAGE, "the setting names can not be empty"); } + 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; - } - } - final char[] value; + final Closeable closeable; + final CheckedFunction valueSupplier; if (options.has(stdinOption)) { - try ( - BufferedReader stdinReader = new BufferedReader(new InputStreamReader(getStdin(), StandardCharsets.UTF_8)); - CharArrayWriter writer = new CharArrayWriter() - ) { - int charInt; - while ((charInt = stdinReader.read()) != -1) { - if ((char) charInt == '\r' || (char) charInt == '\n') { - break; + final BufferedReader stdinReader = new BufferedReader(new InputStreamReader(getStdin(), StandardCharsets.UTF_8)); + valueSupplier = s -> { + try (final CharArrayWriter writer = new CharArrayWriter()) { + int c; + while ((c = stdinReader.read()) != -1) { + if ((char) c == '\r' || (char) c == '\n') { + break; + } + writer.write((char) c); } - writer.write((char) charInt); + return writer.toCharArray(); } - value = writer.toCharArray(); - } + }; + closeable = stdinReader; } else { - value = terminal.readSecret("Enter value for " + setting + ": "); + valueSupplier = s -> terminal.readSecret("Enter value for " + s + ": "); + closeable = () -> {}; } - try { - keyStore.setString(setting, value); - } catch (IllegalArgumentException e) { - throw new UserException(ExitCodes.DATA_ERROR, e.getMessage()); + try (closeable) { + for (final String setting : settings) { + 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; + } + } + + try { + keyStore.setString(setting, valueSupplier.apply(setting)); + } catch (final IllegalArgumentException e) { + throw new UserException(ExitCodes.DATA_ERROR, e.getMessage()); + } + } } - keyStore.save(env.configFile(), getKeyStorePassword().getChars()); + keyStore.save(env.configFile(), getKeyStorePassword().getChars()); } + } diff --git a/distribution/tools/keystore-cli/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java b/distribution/tools/keystore-cli/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java index f40d212a9b3d1..15a8aa20b851f 100644 --- a/distribution/tools/keystore-cli/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java +++ b/distribution/tools/keystore-cli/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java @@ -19,17 +19,17 @@ package org.elasticsearch.common.settings; +import org.elasticsearch.cli.Command; +import org.elasticsearch.cli.ExitCodes; +import org.elasticsearch.cli.UserException; +import org.elasticsearch.env.Environment; + import java.io.ByteArrayInputStream; import java.io.CharArrayWriter; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.util.Map; -import org.elasticsearch.cli.Command; -import org.elasticsearch.cli.ExitCodes; -import org.elasticsearch.cli.UserException; -import org.elasticsearch.env.Environment; - import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasToString; @@ -155,6 +155,19 @@ public void testPromptForValue() throws Exception { assertSecureString("foo", "secret value", password); } + public void testPromptForMultipleValues() throws Exception { + final String password = "keystorepassword"; + KeyStoreWrapper.create().save(env.configFile(), password.toCharArray()); + terminal.addSecretInput(password); + terminal.addSecretInput("bar1"); + terminal.addSecretInput("bar2"); + terminal.addSecretInput("bar3"); + execute("foo1", "foo2", "foo3"); + assertSecureString("foo1", "bar1", password); + assertSecureString("foo2", "bar2", password); + assertSecureString("foo3", "bar3", password); + } + public void testStdinShort() throws Exception { String password = "keystorepassword"; KeyStoreWrapper.create().save(env.configFile(), password.toCharArray()); @@ -200,6 +213,17 @@ public void testStdinInputWithCarriageReturn() throws Exception { assertSecureString("foo", "Typedthisandhitenter", password); } + public void testStdinWithMultipleValues() throws Exception { + final String password = "keystorepassword"; + KeyStoreWrapper.create().save(env.configFile(), password.toCharArray()); + terminal.addSecretInput(password); + setInput("bar1\nbar2\nbar3"); + execute(randomFrom("-x", "--stdin"), "foo1", "foo2", "foo3"); + assertSecureString("foo1", "bar1", password); + assertSecureString("foo2", "bar2", password); + assertSecureString("foo3", "bar3", password); + } + public void testAddUtf8String() throws Exception { String password = "keystorepassword"; KeyStoreWrapper.create().save(env.configFile(), password.toCharArray()); @@ -223,7 +247,7 @@ public void testMissingSettingName() throws Exception { terminal.addTextInput(""); UserException e = expectThrows(UserException.class, this::execute); assertEquals(ExitCodes.USAGE, e.exitCode); - assertThat(e.getMessage(), containsString("The setting name can not be null")); + assertThat(e.getMessage(), containsString("the setting names can not be empty")); } public void testSpecialCharacterInName() throws Exception { diff --git a/docs/reference/commands/keystore.asciidoc b/docs/reference/commands/keystore.asciidoc index 085f8acb7adcb..e66b3c42b41f4 100644 --- a/docs/reference/commands/keystore.asciidoc +++ b/docs/reference/commands/keystore.asciidoc @@ -11,7 +11,7 @@ in the {es} keystore. [source,shell] -------------------------------------------------- bin/elasticsearch-keystore -([add ] [-f] [--stdin] | +([add ] [-f] [--stdin] | [add-file ] | [create] [-p] | [list] | [passwd] | [remove ] | [upgrade]) [-h, --help] ([-s, --silent] | [-v, --verbose]) @@ -40,12 +40,13 @@ keystore, see the setting reference. [[elasticsearch-keystore-parameters]] === Parameters -`add `:: Adds settings to the keystore. By default, you are prompted -for the value of the setting. If the keystore is password protected, you are -also prompted to enter the password. If the setting already exists in the -keystore, you 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 `:: Adds settings to the keystore. Multiple setting names can be +specified as arguments to the `add` command. By default, you are prompted for +the values of the settings. If the keystore is password protected, you are also +prompted to enter the password. If the setting already exists in the keystore, +you 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 `:: Adds a file to the keystore. @@ -70,12 +71,14 @@ protected, you are prompted to enter the current password and the new one. You can optionally use an empty string to remove the password. If the keystore is not password protected, you can use this command to set a password. -`remove `:: Removes a setting from the keystore. +`remove `:: Removes settings from the keystore. Multiple setting +names can be specified as arguments to the `add` command. `-s, --silent`:: Shows minimal output. -`--stdin`:: When used with the `add` parameter, you can pass the setting value -through standard input (stdin). See <>. +`--stdin`:: When used with the `add` parameter, you can pass the settings values +through standard input (stdin). Separate multiple values with carriage returns +or newlines. See <>. `upgrade`:: Upgrades the internal format of the keystore. @@ -143,13 +146,26 @@ bin/elasticsearch-keystore add the.setting.name.to.set You are prompted to enter the value of the setting. If the {es} keystore is password protected, you are also prompted to enter the password. -To pass the setting value through standard input (stdin), use the `--stdin` flag: +You can also add multiple settings with the `add` command: + +[source,sh] +---------------------------------------------------------------- +bin/elasticsearch-keystore add the.setting.name.to.set the.other.setting.name.to.set +---------------------------------------------------------------- + +You are prompted to enter the values of the settings. If the {es} keystore is +password protected, you are also prompted to enter the password. + +To pass the settings values through standard input (stdin), use the `--stdin` +flag: [source,sh] ---------------------------------------------------------------- cat /file/containing/setting/value | bin/elasticsearch-keystore add --stdin the.setting.name.to.set ---------------------------------------------------------------- +Values for multiple settings must be separated by carriage returns or newlines. + [discrete] [[add-file-to-keystore]] ==== Add files to the keystore From e08bca8eeeb3998ace9dd4b40eab9a7385b10ba1 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 25 Mar 2020 16:15:31 -0400 Subject: [PATCH 2/3] Fix checkstyle --- .../elasticsearch/common/settings/AddStringKeyStoreCommand.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java b/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java index 67db9f0557b1c..a4c0de633cf1b 100644 --- a/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java +++ b/distribution/tools/keystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java @@ -74,7 +74,7 @@ protected void executeCommand(Terminal terminal, OptionSet options, Environment if (options.has(stdinOption)) { final BufferedReader stdinReader = new BufferedReader(new InputStreamReader(getStdin(), StandardCharsets.UTF_8)); valueSupplier = s -> { - try (final CharArrayWriter writer = new CharArrayWriter()) { + try (CharArrayWriter writer = new CharArrayWriter()) { int c; while ((c = stdinReader.read()) != -1) { if ((char) c == '\r' || (char) c == '\n') { From e822f192379cab2f45b115d6ce91024ac0acf5f3 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 25 Mar 2020 21:10:31 -0400 Subject: [PATCH 3/3] Fix docs --- docs/reference/commands/keystore.asciidoc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/reference/commands/keystore.asciidoc b/docs/reference/commands/keystore.asciidoc index e66b3c42b41f4..fa74b808a9ac9 100644 --- a/docs/reference/commands/keystore.asciidoc +++ b/docs/reference/commands/keystore.asciidoc @@ -43,10 +43,10 @@ keystore, see the setting reference. `add `:: Adds settings to the keystore. Multiple setting names can be specified as arguments to the `add` command. By default, you are prompted for the values of the settings. If the keystore is password protected, you are also -prompted to enter the password. If the setting already exists in the keystore, -you 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. +prompted to enter the password. If a setting already exists in the keystore, you +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 `:: Adds a file to the keystore. @@ -150,7 +150,9 @@ You can also add multiple settings with the `add` command: [source,sh] ---------------------------------------------------------------- -bin/elasticsearch-keystore add the.setting.name.to.set the.other.setting.name.to.set +bin/elasticsearch-keystore add \ + the.setting.name.to.set \ + the.other.setting.name.to.set ---------------------------------------------------------------- You are prompted to enter the values of the settings. If the {es} keystore is