feedback

Author: rjernst

core/src/main/java/org/elasticsearch/cli/Terminal.java

@@ -27,6 +27,7 @@
 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 final void print(Verbosity verbosity, String msg) {
         }
     }
 
+    /**
+     * 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();

core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java

@@ -24,6 +24,7 @@
 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 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
 
         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;
             }

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;
             }

core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java

@@ -37,7 +37,6 @@
 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,22 +58,18 @@ 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;
 
     /** 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<String> 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 @@ static Path keystorePath(Path configDir) {
 
     /** 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 static KeyStoreWrapper loadMetadata(Path configDir) throws IOException {
         }
         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 @@ void save(Path configDir) throws Exception {
             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,15 +188,15 @@ SecureString getStringSetting(String setting) throws GeneralSecurityException {
         }
         // 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;
     }
 
     /** 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);
     }

core/src/main/java/org/elasticsearch/common/settings/ListKeyStoreCommand.java

@@ -54,10 +54,5 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
         for (String entry : sortedEntries) {
             terminal.println(entry);
         }
-
-        // TODO:
-        // 2. delete command
-        // 3. tests for delete
-        // 4. shell script
     }
 }

core/src/main/java/org/elasticsearch/common/settings/SecureString.java

@@ -80,7 +80,7 @@ public SecureString subSequence(int start, int end) {
     }
 
     /**
-     * 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() {

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 static class Builder {
         // we use a sorted map for consistent serialization when using getAsMap()
         private final Map<String, String> map = new TreeMap<>();
 
-        private KeyStoreWrapper keystore;
+        private SetOnce<KeyStoreWrapper> keystore = new SetOnce<>();
 
         private Builder() {
 
@@ -638,9 +639,10 @@ public String get(String key) {
 
         /** 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 Builder normalizePrefix(String prefix) {
          * set on this builder.
          */
         public Settings build() {
-            return new Settings(map, keystore);
+            return new Settings(map, keystore.get());
         }
     }
 

core/src/test/java/org/elasticsearch/cli/TerminalTests.java

@@ -46,6 +46,33 @@ public void testEscaping() throws Exception {
         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();

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 @@ protected Environment createEnv(Terminal terminal, Map<String, String> settings)
     }
 
     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"));
     }