From 6e406aed2d44663e557c09e4e9d9b32d8226025b Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 4 Jan 2017 12:53:27 -0800 Subject: [PATCH] addressing more PR comments --- .../elasticsearch/bootstrap/Bootstrap.java | 4 +- .../settings/AddStringKeyStoreCommand.java | 11 +- .../common/settings/KeyStoreWrapper.java | 135 +++++++++++++----- .../common/settings/ListKeyStoreCommand.java | 4 +- .../RemoveSettingKeyStoreCommand.java | 4 +- .../common/settings/SecureString.java | 12 +- .../common/settings/Settings.java | 2 +- .../AddStringKeyStoreCommandTests.java | 8 ++ .../settings/CreateKeyStoreCommandTests.java | 7 +- .../settings/KeyStoreCommandTestCase.java | 5 +- 10 files changed, 129 insertions(+), 63 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index b4a95e01cc4..d1d321fcefa 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -226,7 +226,7 @@ final class Bootstrap { 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 @@ final class Bootstrap { } 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); } 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 53571dbada0..e684e925057 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AddStringKeyStoreCommand.java @@ -24,7 +24,6 @@ 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; @@ -57,12 +56,12 @@ class AddStringKeyStoreCommand 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 */); String setting = arguments.value(options); if (keystore.getSettings().contains(setting) && options.has(forceOption) == false) { @@ -80,7 +79,11 @@ class AddStringKeyStoreCommand extends EnvironmentAwareCommand { 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()); } } 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 7c1d3c62585..d209551632d 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/core/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -23,13 +23,17 @@ import javax.crypto.SecretKey; 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,19 +43,32 @@ import java.security.NoSuchAlgorithmException; 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; @@ -59,7 +76,10 @@ public class KeyStoreWrapper implements Closeable { 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,19 +90,19 @@ 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 = new SetOnce<>(); - /** The password for the keystore. See {@link #loadKeystore(char[])}. */ + /** The password for the keystore. See {@link #decrypt(char[])}. */ private final SetOnce keystorePassword = new SetOnce<>(); /** The setting names contained in the loaded keystore. */ private final Set 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 { @@ -90,12 +110,12 @@ public class KeyStoreWrapper implements Closeable { } 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 @@ public class KeyStoreWrapper implements Closeable { /** * 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 class KeyStoreWrapper implements Closeable { /** 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 @@ public class KeyStoreWrapper implements Closeable { 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); 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 3e8f31cf744..b0484fb1526 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/ListKeyStoreCommand.java +++ b/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 sortedEntries = new ArrayList<>(keystore.getSettings()); Collections.sort(sortedEntries); diff --git a/core/src/main/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommand.java b/core/src/main/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommand.java index fb35640b727..e9089b85b56 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommand.java +++ b/core/src/main/java/org/elasticsearch/common/settings/RemoveSettingKeyStoreCommand.java @@ -48,12 +48,12 @@ class RemoveSettingKeyStoreCommand extends EnvironmentAwareCommand { 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) { 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 848393dc7c9..9ce820952e1 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/SecureString.java +++ b/core/src/main/java/org/elasticsearch/common/settings/SecureString.java @@ -40,7 +40,7 @@ public final class SecureString implements CharSequence, AutoCloseable { /** 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 final class SecureString implements CharSequence, AutoCloseable { } @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,7 +83,7 @@ public final class SecureString implements CharSequence, AutoCloseable { * 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); } @@ -91,7 +91,7 @@ public final class SecureString implements CharSequence, AutoCloseable { * Closes the string by clearing the underlying char array. */ @Override - public void close() { + public synchronized void close() { Arrays.fill(chars, '\0'); chars = null; } 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 3543229966d..dbd305bb890 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -642,7 +642,7 @@ public final class Settings implements ToXContent { if (keystore.isLoaded()) { throw new IllegalStateException("The keystore wrapper must already be loaded"); } - this.keystore.set(Objects.requireNonNull(keystore)); + this.keystore.set(keystore); } /** diff --git a/core/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java b/core/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java index 8ac07aedcfc..d4c98953cd5 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/AddStringKeyStoreCommandTests.java @@ -119,6 +119,14 @@ public class AddStringKeyStoreCommandTests extends KeyStoreCommandTestCase { 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)); } diff --git a/core/src/test/java/org/elasticsearch/common/settings/CreateKeyStoreCommandTests.java b/core/src/test/java/org/elasticsearch/common/settings/CreateKeyStoreCommandTests.java index 5722b9e33d4..8584d4d1555 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/CreateKeyStoreCommandTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/CreateKeyStoreCommandTests.java @@ -25,7 +25,6 @@ import java.nio.file.Path; import java.util.Map; import org.elasticsearch.cli.Command; -import org.elasticsearch.cli.MockTerminal; import org.elasticsearch.cli.Terminal; import org.elasticsearch.env.Environment; @@ -44,14 +43,14 @@ public class CreateKeyStoreCommandTests extends KeyStoreCommandTestCase { public void testPosix() throws Exception { execute(); Path configDir = env.configFile(); - assertNotNull(KeyStoreWrapper.loadMetadata(configDir)); + assertNotNull(KeyStoreWrapper.load(configDir)); } public void testNotPosix() throws Exception { setupEnv(false); execute(); Path configDir = env.configFile(); - assertNotNull(KeyStoreWrapper.loadMetadata(configDir)); + assertNotNull(KeyStoreWrapper.load(configDir)); } public void testOverwrite() throws Exception { @@ -69,6 +68,6 @@ public class CreateKeyStoreCommandTests extends KeyStoreCommandTestCase { terminal.addTextInput("y"); execute(); - assertNotNull(KeyStoreWrapper.loadMetadata(env.configFile())); + assertNotNull(KeyStoreWrapper.load(env.configFile())); } } diff --git a/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java b/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java index cb8f84f9d2b..1e02ae6a202 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java +++ b/core/src/test/java/org/elasticsearch/common/settings/KeyStoreCommandTestCase.java @@ -33,7 +33,6 @@ import org.apache.lucene.util.LuceneTestCase; import org.elasticsearch.cli.CommandTestCase; import org.elasticsearch.common.io.PathUtilsForTesting; import org.elasticsearch.env.Environment; -import org.elasticsearch.test.ESTestCase; import org.junit.After; import org.junit.Before; @@ -83,8 +82,8 @@ public abstract class KeyStoreCommandTestCase extends CommandTestCase { } KeyStoreWrapper loadKeystore(String password) throws Exception { - KeyStoreWrapper keystore = KeyStoreWrapper.loadMetadata(env.configFile()); - keystore.loadKeystore(password.toCharArray()); + KeyStoreWrapper keystore = KeyStoreWrapper.load(env.configFile()); + keystore.decrypt(password.toCharArray()); return keystore; }