addressing more PR comments

Author: rjernst

core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java

@@ -226,7 +226,7 @@ protected void validateNodeBeforeAcceptingRequests(
     private static KeyStoreWrapper loadKeyStore(Environment env0) throws BootstrapException {
         final KeyStoreWrapper keystore;
         try {
-            keystore = KeyStoreWrapper.loadMetadata(env0.configFile());
+            keystore = KeyStoreWrapper.load(env0.configFile());
         } catch (IOException e) {
             throw new BootstrapException(e);
         }
@@ -235,7 +235,7 @@ private static KeyStoreWrapper loadKeyStore(Environment env0) throws BootstrapEx
         }
 
         try {
-            keystore.loadKeystore(new char[0] /* TODO: read password from stdin */);
+            keystore.decrypt(new char[0] /* TODO: read password from stdin */);
         } catch (Exception e) {
             throw new BootstrapException(e);
         }

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

@@ -24,7 +24,6 @@
 import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
-import java.util.Locale;
 
 import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
@@ -57,12 +56,12 @@ InputStream getStdin() {
 
     @Override
     protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
-        KeyStoreWrapper keystore = KeyStoreWrapper.loadMetadata(env.configFile());
+        KeyStoreWrapper keystore = KeyStoreWrapper.load(env.configFile());
         if (keystore == null) {
             throw new UserException(ExitCodes.DATA_ERROR, "Elasticsearch keystore not found. Use 'create' command to create one.");
         }
 
-        keystore.loadKeystore(new char[0] /* TODO: prompt for password when they are supported */);
+        keystore.decrypt(new char[0] /* TODO: prompt for password when they are supported */);
 
         String setting = arguments.value(options);
         if (keystore.getSettings().contains(setting) && options.has(forceOption) == false) {
@@ -80,7 +79,11 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
             value = terminal.readSecret("Enter value for " + setting + ": ");
         }
 
-        keystore.setStringSetting(setting, value);
+        try {
+            keystore.setStringSetting(setting, value);
+        } catch (IllegalArgumentException e) {
+            throw new UserException(ExitCodes.DATA_ERROR, "String value must contain only ASCII");
+        }
         keystore.save(env.configFile());
     }
 }

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

@@ -23,13 +23,17 @@
 import javax.crypto.SecretKeyFactory;
 import javax.crypto.spec.PBEKeySpec;
 import javax.security.auth.DestroyFailedException;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.Closeable;
-import java.io.DataInputStream;
-import java.io.DataOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.CharBuffer;
+import java.nio.charset.CharsetEncoder;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFilePermissions;
 import java.security.GeneralSecurityException;
@@ -39,27 +43,43 @@
 import java.util.Arrays;
 import java.util.Enumeration;
 import java.util.HashSet;
+import java.util.Locale;
 import java.util.Set;
 
+import org.apache.lucene.codecs.CodecUtil;
+import org.apache.lucene.store.BufferedChecksumIndexInput;
+import org.apache.lucene.store.ChecksumIndexInput;
+import org.apache.lucene.store.IOContext;
+import org.apache.lucene.store.IndexInput;
+import org.apache.lucene.store.IndexOutput;
+import org.apache.lucene.store.NIOFSDirectory;
 import org.apache.lucene.util.SetOnce;
 
 /**
  * A wrapper around a Java KeyStore which provides supplements the keystore with extra metadata.
  *
- * Loading a keystore has 2 phases. First, call {@link #loadMetadata(Path)}. Then call
- * {@link #loadKeystore(char[])} with the keystore password, or an empty char array if
- * {@link #hasPassword()} is {@code false}.
+ * Loading a keystore has 2 phases. First, call {@link #load(Path)}. Then call
+ * {@link #decrypt(char[])} with the keystore password, or an empty char array if
+ * {@link #hasPassword()} is {@code false}.  Loading and decrypting should happen
+ * in a single thread. Once decrypted, keys may be read with the wrapper in
+ * multiple threads.
  */
 public class KeyStoreWrapper implements Closeable {
 
+    /** The name of the keystore file to read and write. */
+    private static final String KEYSTORE_FILENAME = "elasticsearch.keystore";
+
     /** The version of the metadata written before the keystore data. */
     private static final int FORMAT_VERSION = 1;
 
     /** The keystore type for a newly created keystore. */
     private static final String NEW_KEYSTORE_TYPE = "PKCS12";
 
     /** The algorithm used to store password for a newly created keystore. */
-    private static final String NEW_KEYSTORE_SECRET_KEY_ALGO = "PBEWithHmacSHA256AndAES_128";
+    private static final String NEW_KEYSTORE_SECRET_KEY_ALGO = "PBE";//"PBEWithHmacSHA256AndAES_128";
+
+    /** An encoder to check whether string values are ascii. */
+    private static final CharsetEncoder ASCII_ENCODER = StandardCharsets.US_ASCII.newEncoder();
 
     /** True iff the keystore has a password needed to read. */
     private final boolean hasPassword;
@@ -70,32 +90,32 @@ public class KeyStoreWrapper implements Closeable {
     /** 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;
+    /** The raw bytes of the encrypted keystore. */
+    private final byte[] keystoreBytes;
 
-    /** The loaded keystore. See {@link #loadKeystore(char[])}. */
+    /** The loaded keystore. See {@link #decrypt(char[])}. */
     private final SetOnce<KeyStore> keystore = new SetOnce<>();
 
-    /** The password for the keystore. See {@link #loadKeystore(char[])}. */
+    /** The password for the keystore. See {@link #decrypt(char[])}. */
     private final SetOnce<KeyStore.PasswordProtection> keystorePassword = new SetOnce<>();
 
     /** The setting names contained in the loaded keystore. */
     private final Set<String> settingNames = new HashSet<>();
 
-    private KeyStoreWrapper(boolean hasPassword, String type, String secretKeyAlgo, InputStream input) {
+    private KeyStoreWrapper(boolean hasPassword, String type, String secretKeyAlgo, byte[] keystoreBytes) {
         this.hasPassword = hasPassword;
         this.type = type;
         try {
             secretFactory = SecretKeyFactory.getInstance(secretKeyAlgo);
         } catch (NoSuchAlgorithmException e) {
             throw new RuntimeException(e);
         }
-        this.input = input;
+        this.keystoreBytes = keystoreBytes;
     }
 
     /** Returns a path representing the ES keystore in the given config dir. */
     static Path keystorePath(Path configDir) {
-        return configDir.resolve("elasticsearch.keystore");
+        return configDir.resolve(KEYSTORE_FILENAME);
     }
 
     /** Constructs a new keystore with the given password. */
@@ -111,43 +131,61 @@ static KeyStoreWrapper create(char[] password) throws Exception {
     /**
      * Loads information about the Elasticsearch keystore from the provided config directory.
      *
-     * {@link #loadKeystore(char[])} must be called before reading or writing any entries.
+     * {@link #decrypt(char[])} must be called before reading or writing any entries.
      * Returns {@code null} if no keystore exists.
      */
-    public static KeyStoreWrapper loadMetadata(Path configDir) throws IOException {
+    public static KeyStoreWrapper load(Path configDir) throws IOException {
         Path keystoreFile = keystorePath(configDir);
         if (Files.exists(keystoreFile) == false) {
             return null;
         }
-        DataInputStream inputStream = new DataInputStream(Files.newInputStream(keystoreFile));
-        int format = inputStream.readInt();
-        if (format != FORMAT_VERSION) {
-            throw new IllegalStateException("Unknown keystore metadata format [" + format + "]");
+
+        NIOFSDirectory directory = new NIOFSDirectory(configDir);
+        try (IndexInput indexInput = directory.openInput(KEYSTORE_FILENAME, IOContext.READONCE)) {
+            ChecksumIndexInput input = new BufferedChecksumIndexInput(indexInput);
+            CodecUtil.checkHeader(input, KEYSTORE_FILENAME, FORMAT_VERSION, FORMAT_VERSION);
+            byte hasPasswordByte = input.readByte();
+            boolean hasPassword = hasPasswordByte == 1;
+            if (hasPassword == false && hasPasswordByte != 0) {
+                throw new IllegalStateException("hasPassword boolean is corrupt: "
+                    + String.format(Locale.ROOT, "%02x", hasPasswordByte));
+            }
+            String type = input.readString();
+            String secretKeyAlgo = input.readString();
+            byte[] keystoreBytes = new byte[input.readInt()];
+            input.readBytes(keystoreBytes, 0, keystoreBytes.length);
+            CodecUtil.checkFooter(input);
+            return new KeyStoreWrapper(hasPassword, type, secretKeyAlgo, keystoreBytes);
         }
-        boolean hasPassword = inputStream.readBoolean();
-        String type = inputStream.readUTF();
-        String secretKeyAlgo = inputStream.readUTF();
-        return new KeyStoreWrapper(hasPassword, type, secretKeyAlgo, inputStream);
     }
 
-    /** Returns true iff {@link #loadKeystore(char[])} has been called. */
+    /** Returns true iff {@link #decrypt(char[])} has been called. */
     public boolean isLoaded() {
         return keystore.get() != null;
     }
 
-    /** Return true iff calling {@link #loadKeystore(char[])} requires a non-empty password. */
+    /** Return true iff calling {@link #decrypt(char[])} requires a non-empty password. */
     public boolean hasPassword() {
         return hasPassword;
     }
 
-    /** Loads the keystore this metadata wraps. This may only be called once. */
-    public void loadKeystore(char[] password) throws GeneralSecurityException, IOException {
-        this.keystore.set(KeyStore.getInstance(type));
-        try (InputStream in = input) {
+    /**
+     * Decrypts the underlying java keystore.
+     *
+     * This may only be called once. The provided password will be zeroed out.
+     */
+    public void decrypt(char[] password) throws GeneralSecurityException, IOException {
+        if (keystore.get() != null) {
+            throw new IllegalStateException("Keystore has already been decrypted");
+        }
+        keystore.set(KeyStore.getInstance(type));
+        try (InputStream in = new ByteArrayInputStream(keystoreBytes)) {
             keystore.get().load(in, password);
+        } finally {
+            Arrays.fill(keystoreBytes, (byte)0);
         }
 
-        this.keystorePassword.set(new KeyStore.PasswordProtection(password));
+        keystorePassword.set(new KeyStore.PasswordProtection(password));
         Arrays.fill(password, '\0');
 
         // convert keystore aliases enum into a set for easy lookup
@@ -159,15 +197,27 @@ public void loadKeystore(char[] password) throws GeneralSecurityException, IOExc
 
     /** Write the keystore to the given config directory. */
     void save(Path configDir) throws Exception {
-        Path keystoreFile = keystorePath(configDir);
-        try (DataOutputStream outputStream = new DataOutputStream(Files.newOutputStream(keystoreFile))) {
-            outputStream.writeInt(FORMAT_VERSION);
-            char[] password = this.keystorePassword.get().getPassword();
-            outputStream.writeBoolean(password.length != 0);
-            outputStream.writeUTF(type);
-            outputStream.writeUTF(secretFactory.getAlgorithm());
-            keystore.get().store(outputStream, password);
+        char[] password = this.keystorePassword.get().getPassword();
+
+        NIOFSDirectory directory = new NIOFSDirectory(configDir);
+        // write to tmp file first, then overwrite
+        String tmpFile = KEYSTORE_FILENAME + ".tmp";
+        try (IndexOutput output = directory.createOutput(tmpFile, IOContext.DEFAULT)) {
+            CodecUtil.writeHeader(output, KEYSTORE_FILENAME, FORMAT_VERSION);
+            output.writeByte(password.length == 0 ? (byte)0 : (byte)1);
+            output.writeString(type);
+            output.writeString(secretFactory.getAlgorithm());
+
+            ByteArrayOutputStream keystoreBytesStream = new ByteArrayOutputStream();
+            keystore.get().store(keystoreBytesStream, password);
+            byte[] keystoreBytes = keystoreBytesStream.toByteArray();
+            output.writeInt(keystoreBytes.length);
+            output.writeBytes(keystoreBytes, keystoreBytes.length);
+            CodecUtil.writeFooter(output);
         }
+
+        Path keystoreFile = keystorePath(configDir);
+        Files.move(configDir.resolve(tmpFile), keystoreFile, StandardCopyOption.REPLACE_EXISTING);
         PosixFileAttributeView attrs = Files.getFileAttributeView(keystoreFile, PosixFileAttributeView.class);
         if (attrs != null) {
             // don't rely on umask: ensure the keystore has minimal permissions
@@ -194,8 +244,15 @@ SecureString getStringSetting(String setting) throws GeneralSecurityException {
         return value;
     }
 
-    /** Set a string setting. */
+    /**
+     * Set a string setting.
+     *
+     * @throws IllegalArgumentException if the value is not ASCII
+     */
     void setStringSetting(String setting, char[] value) throws GeneralSecurityException {
+        if (ASCII_ENCODER.canEncode(CharBuffer.wrap(value)) == false) {
+            throw new IllegalArgumentException("Value must be ascii");
+        }
         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

@@ -42,12 +42,12 @@ class ListKeyStoreCommand extends EnvironmentAwareCommand {
 
     @Override
     protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception {
-        KeyStoreWrapper keystore = KeyStoreWrapper.loadMetadata(env.configFile());
+        KeyStoreWrapper keystore = KeyStoreWrapper.load(env.configFile());
         if (keystore == null) {
             throw new UserException(ExitCodes.DATA_ERROR, "Elasticsearch keystore not found. Use 'create' command to create one.");
         }
 
-        keystore.loadKeystore(new char[0] /* TODO: prompt for password when they are supported */);
+        keystore.decrypt(new char[0] /* TODO: prompt for password when they are supported */);
 
         List<String> sortedEntries = new ArrayList<>(keystore.getSettings());
         Collections.sort(sortedEntries);

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

@@ -48,12 +48,12 @@ protected void execute(Terminal terminal, OptionSet options, Environment env) th
             throw new UserException(ExitCodes.USAGE, "Must supply at least one setting to remove");
         }
 
-        KeyStoreWrapper keystore = KeyStoreWrapper.loadMetadata(env.configFile());
+        KeyStoreWrapper keystore = KeyStoreWrapper.load(env.configFile());
         if (keystore == null) {
             throw new UserException(ExitCodes.DATA_ERROR, "Elasticsearch keystore not found. Use 'create' command to create one.");
         }
 
-        keystore.loadKeystore(new char[0] /* TODO: prompt for password when they are supported */);
+        keystore.decrypt(new char[0] /* TODO: prompt for password when they are supported */);
 
         for (String setting : arguments.values(options)) {
             if (keystore.getSettings().contains(setting) == false) {

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

@@ -40,7 +40,7 @@ public SecureString(char[] chars) {
 
     /** Constant time equality to avoid potential timing attacks. */
     @Override
-    public boolean equals(Object o) {
+    public synchronized boolean equals(Object o) {
         ensureNotClosed();
         if (this == o) return true;
         if (o == null || o instanceof CharSequence == false) return false;
@@ -58,18 +58,18 @@ public boolean equals(Object o) {
     }
 
     @Override
-    public int hashCode() {
+    public synchronized int hashCode() {
         return Arrays.hashCode(chars);
     }
 
     @Override
-    public int length() {
+    public synchronized int length() {
         ensureNotClosed();
         return chars.length;
     }
 
     @Override
-    public char charAt(int index) {
+    public synchronized char charAt(int index) {
         ensureNotClosed();
         return chars[index];
     }
@@ -83,15 +83,15 @@ public SecureString subSequence(int start, int end) {
      * Convert to a {@link String}. This should only be used with APIs that do not take {@link CharSequence}.
      */
     @Override
-    public String toString() {
+    public synchronized String toString() {
         return new String(chars);
     }
 
     /**
      * Closes the string by clearing the underlying char array.
      */
     @Override
-    public void close() {
+    public synchronized void close() {
         Arrays.fill(chars, '\0');
         chars = null;
     }

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

@@ -642,7 +642,7 @@ public void setKeyStore(KeyStoreWrapper keystore) {
             if (keystore.isLoaded()) {
                 throw new IllegalStateException("The keystore wrapper must already be loaded");
             }
-            this.keystore.set(Objects.requireNonNull(keystore));
+            this.keystore.set(keystore);
         }
 
         /**

core/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java

@@ -119,6 +119,14 @@ public void testStdinLong() throws Exception {
         assertSecureString("foo", "secret value 2");
     }
 
+    public void testNonAsciiValue() throws Exception {
+        KeyStoreWrapper.create(new char[0]).save(env.configFile());
+        terminal.addSecretInput("non-äsčîï");
+        UserException e = expectThrows(UserException.class, () -> execute("foo"));
+        assertEquals(ExitCodes.DATA_ERROR, e.exitCode);
+        assertEquals("String value must contain only ASCII", e.getMessage());
+    }
+
     void setInput(String inputStr) {
         input = new ByteArrayInputStream(inputStr.getBytes(StandardCharsets.UTF_8));
     }