Skip to content
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

Allow keystore add to handle multiple settings #54229

Merged
merged 4 commits into from
Mar 26, 2020
Merged

Allow keystore add to handle multiple settings #54229

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
99c6b22
Allow keystore add to handle multiple settings
jasontedor Mar 25, 2020
e08bca8
Fix checkstyle
jasontedor Mar 25, 2020
9f66508
Merge remote-tracking branch 'elastic/master' into keystore-add-multi…
jasontedor Mar 25, 2020
e822f19
Fix docs
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
88 changes: 51 additions & 37 deletions ...eystore-cli/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -42,13 +46,13 @@ class AddStringKeyStoreCommand extends BaseKeyStoreCommand {
private final OptionSpec<String> 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth updating the equivalent text in RemoveSettingKeyStoreCommand? It still talks about removing "a setting" from the keystore, but it already supports multiple settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will handle this in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #54244.

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
Expand All @@ -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<String> 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<String, char[], IOException> 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 (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());
}

}
36 changes: 30 additions & 6 deletions ...re-cli/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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 {
Expand Down
40 changes: 29 additions & 11 deletions docs/reference/commands/keystore.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ in the {es} keystore.
[source,shell]
--------------------------------------------------
bin/elasticsearch-keystore
([add <setting>] [-f] [--stdin] |
([add <settings>] [-f] [--stdin] |
[add-file <setting> <path>] | [create] [-p] |
[list] | [passwd] | [remove <setting>] | [upgrade])
[-h, --help] ([-s, --silent] | [-v, --verbose])
Expand Down Expand Up @@ -40,12 +40,13 @@ keystore, see the setting reference.
[[elasticsearch-keystore-parameters]]
=== Parameters

`add <setting>`:: 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 <settings>`:: 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 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 <setting> <path>`:: Adds a file to the keystore.

Expand All @@ -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 <setting>`:: Removes a setting from the keystore.
`remove <settings>`:: 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 <<add-string-to-keystore>>.
`--stdin`:: When used with the `add` parameter, you can pass the settings values
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document -x too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I notice we only have -f without --force as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing these. Since these are orthogonal to this change, I will handle them in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #54242.

through standard input (stdin). Separate multiple values with carriage returns
or newlines. See <<add-string-to-keystore>>.

`upgrade`:: Upgrades the internal format of the keystore.

Expand Down Expand Up @@ -143,13 +146,28 @@ 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
Expand Down