From 4e4a40df7afe99eb0d6b7026037263ccce15d48b Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Tue, 3 Jan 2017 15:42:38 -0800 Subject: [PATCH] feedback --- .../java/org/elasticsearch/cli/Terminal.java | 21 ++++++++++++ .../settings/AddStringKeyStoreCommand.java | 4 +-- .../settings/CreateKeyStoreCommand.java | 3 +- .../common/settings/KeyStoreWrapper.java | 32 ++++++++++--------- .../common/settings/ListKeyStoreCommand.java | 5 --- .../common/settings/SecureString.java | 2 +- .../common/settings/Settings.java | 12 ++++--- .../org/elasticsearch/cli/TerminalTests.java | 27 ++++++++++++++++ .../RemoveSettingKeyStoreCommandTests.java | 5 ++- 9 files changed, 80 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/cli/Terminal.java b/core/src/main/java/org/elasticsearch/cli/Terminal.java index 58eb5012d07..cd7fc76e681 100644 --- a/core/src/main/java/org/elasticsearch/cli/Terminal.java +++ b/core/src/main/java/org/elasticsearch/cli/Terminal.java @@ -27,6 +27,7 @@ import java.io.IOException; import java.io.InputStreamReader; import java.io.PrintWriter; import java.nio.charset.Charset; +import java.util.Locale; /** * A Terminal wraps access to reading input and writing output for a cli. @@ -92,6 +93,26 @@ public abstract class Terminal { } } + /** + * Prompt for a yes or no answer from the user. This method will loop until 'y' or 'n' + * (or the default empty value) is entered. + */ + public final boolean promptYesNo(String prompt, boolean defaultYes) { + String answerPrompt = defaultYes ? " [Y/n]" : " [y/N]"; + while (true) { + String answer = readText(prompt + answerPrompt).toLowerCase(Locale.ROOT); + if (answer.isEmpty()) { + return defaultYes; + } + boolean answerYes = answer.equals("y"); + if (answerYes == false && answer.equals("n") == false) { + println("Did not understand answer '" + answer + "'"); + continue; + } + return answerYes; + } + } + private static class ConsoleTerminal extends Terminal { private static final Console CONSOLE = System.console(); diff --git a/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java b/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java index 289c02a1781..53571dbada0 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java @@ -24,6 +24,7 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.Locale; import joptsimple.OptionSet; import joptsimple.OptionSpec; @@ -65,8 +66,7 @@ class AddStringKeyStoreCommand extends EnvironmentAwareCommand { String setting = arguments.value(options); if (keystore.getSettings().contains(setting) && options.has(forceOption) == false) { - String answer = terminal.readText("Setting " + setting + " already exists. Overwrite? [y/N]"); - if (answer.equals("y") == false) { + if (terminal.promptYesNo("Setting " + setting + " already exists. Overwrite?", false) == false) { terminal.println("Exiting without modifying keystore."); return; } diff --git a/core/src/main/java/org/elasticsearch/common/settings/CreateKeyStoreCommand.java b/core/src/main/java/org/elasticsearch/common/settings/CreateKeyStoreCommand.java index 7c2f8aec19a..08860cb5ea9 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/CreateKeyStoreCommand.java +++ b/core/src/main/java/org/elasticsearch/common/settings/CreateKeyStoreCommand.java @@ -40,8 +40,7 @@ class CreateKeyStoreCommand extends EnvironmentAwareCommand { protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception { Path keystoreFile = KeyStoreWrapper.keystorePath(env.configFile()); if (Files.exists(keystoreFile)) { - String answer = terminal.readText("An elasticsearch keystore already exists. Overwrite? [y/N] "); - if (answer.equals("y") == false) { + if (terminal.promptYesNo("An elasticsearch keystore already exists. Overwrite?", false) == false) { terminal.println("Exiting without creating keystore."); return; } diff --git a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index d1a605fff92..7c1d3c62585 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -37,7 +37,6 @@ import java.security.KeyStore; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; import java.util.Arrays; -import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; import java.util.Set; @@ -59,15 +58,8 @@ public class KeyStoreWrapper implements Closeable { /** The keystore type for a newly created keystore. */ private static final String NEW_KEYSTORE_TYPE = "PKCS12"; - /** A factory necessary for constructing instances of secrets in a {@link KeyStore}. */ - private static final SecretKeyFactory passwordFactory; - static { - try { - passwordFactory = SecretKeyFactory.getInstance("PBE"); - } catch (NoSuchAlgorithmException e) { - throw new RuntimeException(e); - } - } + /** The algorithm used to store password for a newly created keystore. */ + private static final String NEW_KEYSTORE_SECRET_KEY_ALGO = "PBEWithHmacSHA256AndAES_128"; /** True iff the keystore has a password needed to read. */ private final boolean hasPassword; @@ -75,6 +67,9 @@ public class KeyStoreWrapper implements Closeable { /** The type of the keystore, as passed to {@link java.security.KeyStore#getInstance(String)} */ private final String type; + /** A factory necessary for constructing instances of secrets in a {@link KeyStore}. */ + private final SecretKeyFactory secretFactory; + /** A stream of the actual keystore data. */ private final InputStream input; @@ -87,9 +82,14 @@ public class KeyStoreWrapper implements Closeable { /** The setting names contained in the loaded keystore. */ private final Set settingNames = new HashSet<>(); - private KeyStoreWrapper(boolean hasPassword, String type, InputStream input) { + private KeyStoreWrapper(boolean hasPassword, String type, String secretKeyAlgo, InputStream input) { this.hasPassword = hasPassword; this.type = type; + try { + secretFactory = SecretKeyFactory.getInstance(secretKeyAlgo); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } this.input = input; } @@ -100,7 +100,7 @@ public class KeyStoreWrapper implements Closeable { /** Constructs a new keystore with the given password. */ static KeyStoreWrapper create(char[] password) throws Exception { - KeyStoreWrapper wrapper = new KeyStoreWrapper(password.length != 0, NEW_KEYSTORE_TYPE, null); + KeyStoreWrapper wrapper = new KeyStoreWrapper(password.length != 0, NEW_KEYSTORE_TYPE, NEW_KEYSTORE_SECRET_KEY_ALGO, null); KeyStore keyStore = KeyStore.getInstance(NEW_KEYSTORE_TYPE); keyStore.load(null, null); wrapper.keystore.set(keyStore); @@ -126,7 +126,8 @@ public class KeyStoreWrapper implements Closeable { } boolean hasPassword = inputStream.readBoolean(); String type = inputStream.readUTF(); - return new KeyStoreWrapper(hasPassword, type, inputStream); + String secretKeyAlgo = inputStream.readUTF(); + return new KeyStoreWrapper(hasPassword, type, secretKeyAlgo, inputStream); } /** Returns true iff {@link #loadKeystore(char[])} has been called. */ @@ -164,6 +165,7 @@ public class KeyStoreWrapper implements Closeable { char[] password = this.keystorePassword.get().getPassword(); outputStream.writeBoolean(password.length != 0); outputStream.writeUTF(type); + outputStream.writeUTF(secretFactory.getAlgorithm()); keystore.get().store(outputStream, password); } PosixFileAttributeView attrs = Files.getFileAttributeView(keystoreFile, PosixFileAttributeView.class); @@ -186,7 +188,7 @@ public class KeyStoreWrapper implements Closeable { } // TODO: only allow getting a setting once? KeyStore.SecretKeyEntry secretKeyEntry = (KeyStore.SecretKeyEntry)entry; - PBEKeySpec keySpec = (PBEKeySpec)passwordFactory.getKeySpec(secretKeyEntry.getSecretKey(), PBEKeySpec.class); + PBEKeySpec keySpec = (PBEKeySpec) secretFactory.getKeySpec(secretKeyEntry.getSecretKey(), PBEKeySpec.class); SecureString value = new SecureString(keySpec.getPassword()); keySpec.clearPassword(); return value; @@ -194,7 +196,7 @@ public class KeyStoreWrapper implements Closeable { /** Set a string setting. */ void setStringSetting(String setting, char[] value) throws GeneralSecurityException { - SecretKey secretKey = passwordFactory.generateSecret(new PBEKeySpec(value)); + SecretKey secretKey = secretFactory.generateSecret(new PBEKeySpec(value)); keystore.get().setEntry(setting, new KeyStore.SecretKeyEntry(secretKey), keystorePassword.get()); settingNames.add(setting); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/ListKeyStoreCommand.java b/core/src/main/java/org/elasticsearch/common/settings/ListKeyStoreCommand.java index 07bf4263d19..3e8f31cf744 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/ListKeyStoreCommand.java +++ b/core/src/main/java/org/elasticsearch/common/settings/ListKeyStoreCommand.java @@ -54,10 +54,5 @@ class ListKeyStoreCommand extends EnvironmentAwareCommand { for (String entry : sortedEntries) { terminal.println(entry); } - - // TODO: - // 2. delete command - // 3. tests for delete - // 4. shell script } } diff --git a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java index 7d209fc6171..848393dc7c9 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java @@ -80,7 +80,7 @@ public final class SecureString implements CharSequence, AutoCloseable { } /** - * Convert to a {@link String}. This only be used with APIs that do not take {@link CharSequence}. + * Convert to a {@link String}. This should only be used with APIs that do not take {@link CharSequence}. */ @Override public String toString() { diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java index 2bf93fb8e0e..3543229966d 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.settings; +import org.apache.lucene.util.SetOnce; import org.elasticsearch.Version; import org.elasticsearch.common.Booleans; import org.elasticsearch.common.Strings; @@ -612,7 +613,7 @@ public final class Settings implements ToXContent { // we use a sorted map for consistent serialization when using getAsMap() private final Map map = new TreeMap<>(); - private KeyStoreWrapper keystore; + private SetOnce keystore = new SetOnce<>(); private Builder() { @@ -638,9 +639,10 @@ public final class Settings implements ToXContent { /** Sets the secret store for these settings. */ public void setKeyStore(KeyStoreWrapper keystore) { - assert this.keystore == null; // only set once! - assert keystore.isLoaded(); - this.keystore = Objects.requireNonNull(keystore); + if (keystore.isLoaded()) { + throw new IllegalStateException("The keystore wrapper must already be loaded"); + } + this.keystore.set(Objects.requireNonNull(keystore)); } /** @@ -1049,7 +1051,7 @@ public final class Settings implements ToXContent { * set on this builder. */ public Settings build() { - return new Settings(map, keystore); + return new Settings(map, keystore.get()); } } diff --git a/core/src/test/java/org/elasticsearch/cli/TerminalTests.java b/core/src/test/java/org/elasticsearch/cli/TerminalTests.java index 6673bdbc858..795780b4890 100644 --- a/core/src/test/java/org/elasticsearch/cli/TerminalTests.java +++ b/core/src/test/java/org/elasticsearch/cli/TerminalTests.java @@ -46,6 +46,33 @@ public class TerminalTests extends ESTestCase { assertPrinted(terminal, Terminal.Verbosity.NORMAL, "This message contains percent like %20n"); } + public void testPromptYesNoDefault() throws Exception { + MockTerminal terminal = new MockTerminal(); + terminal.addTextInput(""); + assertTrue(terminal.promptYesNo("Answer?", true)); + terminal.addTextInput(""); + assertFalse(terminal.promptYesNo("Answer?", false)); + } + + public void testPromptYesNoReprompt() throws Exception { + MockTerminal terminal = new MockTerminal(); + terminal.addTextInput("blah"); + terminal.addTextInput("y"); + assertTrue(terminal.promptYesNo("Answer? [Y/n]\nDid not understand answer 'blah'\nAnswer? [Y/n]", true)); + } + + public void testPromptYesNoCase() throws Exception { + MockTerminal terminal = new MockTerminal(); + terminal.addTextInput("Y"); + assertTrue(terminal.promptYesNo("Answer?", false)); + terminal.addTextInput("y"); + assertTrue(terminal.promptYesNo("Answer?", false)); + terminal.addTextInput("N"); + assertFalse(terminal.promptYesNo("Answer?", true)); + terminal.addTextInput("n"); + assertFalse(terminal.promptYesNo("Answer?", true)); + } + private void assertPrinted(MockTerminal logTerminal, Terminal.Verbosity verbosity, String text) throws Exception { logTerminal.println(verbosity, text); String output = logTerminal.getOutput(); diff --git a/core/src/test/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommandTests.java b/core/src/test/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommandTests.java index e83ed873116..e1d526c88be 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommandTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommandTests.java @@ -19,6 +19,9 @@ package org.elasticsearch.common.settings; +import javax.crypto.SecretKeyFactory; +import java.security.Provider; +import java.security.Security; import java.util.Map; import java.util.Set; @@ -43,7 +46,7 @@ public class RemoveSettingKeyStoreCommandTests extends KeyStoreCommandTestCase { } public void testMissing() throws Exception { - UserException e = expectThrows(UserException.class, this::execute); + UserException e = expectThrows(UserException.class, () -> execute("foo")); assertEquals(ExitCodes.DATA_ERROR, e.exitCode); assertThat(e.getMessage(), containsString("keystore not found")); }