addressing more PR comments
This commit is contained in:
parent
4e4a40df7a
commit
6e406aed2d
|
@ -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);
|
||||
}
|
||||
|
|
|
@ -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());
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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> 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 {
|
||||
|
@ -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);
|
||||
|
|
|
@ -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);
|
||||
|
|
|
@ -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) {
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
@ -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));
|
||||
}
|
||||
|
|
|
@ -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()));
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in New Issue