From 24a83f8a44e74144a8f626d872dd99d12383beba Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Fri, 4 May 2018 23:32:05 +1000 Subject: [PATCH 1/4] Fail if reading from closed KeyStoreWrapper In #28255 the implementation of the elasticsearch.keystore was changed to no longer be built on top of a PKCS#12 keystore. A side effect of that change was that calling getString or getFile on a closed KeyStoreWrapper ceased to throw an exception, and would instead return a value consisting of all 0 bytes. This change restores the previous behaviour as closely as possible. It is possible to retrieve the _keys_ from a closed keystore, but any attempt to get or set the entries will throw an IllegalStateException. The only divergence from the previous behaviour is that "isLoaded" will now return false if the keystore is closed, in keeping with the "loaded and retrievable" description in the parent javadoc. --- .../common/settings/KeyStoreWrapper.java | 30 ++++++++++++++----- .../common/settings/KeyStoreWrapperTests.java | 22 ++++++++++---- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 9b994089be0c2..69ff47648a16f 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -157,6 +157,7 @@ private static class Entry { /** The decrypted secret data. See {@link #decrypt(char[])}. */ private final SetOnce> entries = new SetOnce<>(); + private volatile boolean closed; private KeyStoreWrapper(int formatVersion, boolean hasPassword, byte[] dataBytes) { this.formatVersion = formatVersion; @@ -274,7 +275,7 @@ public static void upgrade(KeyStoreWrapper wrapper, Path configDir, char[] passw @Override public boolean isLoaded() { - return entries.get() != null; + return entries.get() != null && closed == false; } /** Return true iff calling {@link #decrypt(char[])} requires a non-empty password. */ @@ -500,16 +501,22 @@ public void save(Path configDir, char[] password) throws Exception { } } + /** + * It is possible to retrieve the setting names even if the keystore is closed. + * This allows {@link SecureSetting} to correctly determine that a entry exists even though it cannot be read. Thus attempting to + * read a secure setting after the keystore is closed will generate a "keystore is closed" exception rather than using the fallback + * setting. + */ @Override public Set getSettingNames() { - assert isLoaded(); + assert entries.get() != null : "Keystore is not loaded"; return entries.get().keySet(); } // TODO: make settings accessible only to code that registered the setting @Override public SecureString getString(String setting) { - assert isLoaded(); + ensureOpen(); Entry entry = entries.get().get(setting); if (entry == null || entry.type != EntryType.STRING) { throw new IllegalArgumentException("Secret setting " + setting + " is not a string"); @@ -521,12 +528,11 @@ public SecureString getString(String setting) { @Override public InputStream getFile(String setting) { - assert isLoaded(); + ensureOpen(); Entry entry = entries.get().get(setting); if (entry == null || entry.type != EntryType.FILE) { throw new IllegalArgumentException("Secret setting " + setting + " is not a file"); } - return new ByteArrayInputStream(entry.bytes); } @@ -544,7 +550,7 @@ public static void validateSettingName(String setting) { /** Set a string setting. */ void setString(String setting, char[] value) { - assert isLoaded(); + ensureOpen(); validateSettingName(setting); ByteBuffer byteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(value)); @@ -557,7 +563,7 @@ void setString(String setting, char[] value) { /** Set a file setting. */ void setFile(String setting, byte[] bytes) { - assert isLoaded(); + ensureOpen(); validateSettingName(setting); Entry oldEntry = entries.get().put(setting, new Entry(EntryType.FILE, Arrays.copyOf(bytes, bytes.length))); @@ -568,15 +574,23 @@ void setFile(String setting, byte[] bytes) { /** Remove the given setting from the keystore. */ void remove(String setting) { - assert isLoaded(); + ensureOpen(); Entry oldEntry = entries.get().remove(setting); if (oldEntry != null) { Arrays.fill(oldEntry.bytes, (byte)0); } } + private void ensureOpen() { + if (closed) { + throw new IllegalStateException("Keystore is closed"); + } + assert isLoaded() : "Keystore is not loaded"; + } + @Override public void close() { + this.closed = true; for (Entry entry : entries.get().values()) { Arrays.fill(entry.bytes, (byte)0); } diff --git a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java index a59cdf13c13ff..42c198b1821e6 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -25,30 +25,27 @@ import java.io.ByteArrayOutputStream; 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.FileSystem; import java.nio.file.Path; import java.security.KeyStore; import java.util.ArrayList; import java.util.Base64; import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; import org.apache.lucene.codecs.CodecUtil; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.SimpleFSDirectory; import org.elasticsearch.core.internal.io.IOUtils; -import org.elasticsearch.bootstrap.BootstrapSettings; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; +import org.hamcrest.Matchers; import org.junit.After; import org.junit.Before; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; public class KeyStoreWrapperTests extends ESTestCase { @@ -92,6 +89,19 @@ public void testCreate() throws Exception { assertTrue(keystore.getSettingNames().contains(KeyStoreWrapper.SEED_SETTING.getKey())); } + public void testCannotReadStringFromClosedKeystore() throws Exception { + KeyStoreWrapper keystore = KeyStoreWrapper.create(); + assertThat(keystore.getSettingNames(), Matchers.hasItem(KeyStoreWrapper.SEED_SETTING.getKey())); + assertThat(keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()), notNullValue()); + + keystore.close(); + + assertThat(keystore.getSettingNames(), Matchers.hasItem(KeyStoreWrapper.SEED_SETTING.getKey())); + final IllegalStateException exception = expectThrows(IllegalStateException.class, + () -> keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey())); + assertThat(exception.getMessage(), containsString("closed")); + } + public void testUpgradeNoop() throws Exception { KeyStoreWrapper keystore = KeyStoreWrapper.create(); SecureString seed = keystore.getString(KeyStoreWrapper.SEED_SETTING.getKey()); From 9fe003f17f0fe140ae8aad39e167f1550a316a55 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 7 May 2018 11:52:44 +1000 Subject: [PATCH 2/4] Fixes for KeyStoreWrapper - synchronize methods - ensureOpen before encrypt or save --- .../common/settings/KeyStoreWrapper.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 69ff47648a16f..901de6cbf70f3 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -355,7 +355,7 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio /** Encrypt the keystore entries and return the encrypted data. */ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSecurityException, IOException { - assert isLoaded(); + ensureOpen(); ByteArrayOutputStream bytes = new ByteArrayOutputStream(); Cipher cipher = createCipher(Cipher.ENCRYPT_MODE, password, salt, iv); @@ -450,7 +450,7 @@ private void decryptLegacyEntries() throws GeneralSecurityException, IOException /** Write the keystore to the given config directory. */ public void save(Path configDir, char[] password) throws Exception { - assert isLoaded(); + ensureOpen(); SimpleFSDirectory directory = new SimpleFSDirectory(configDir); // write to tmp file first, then overwrite @@ -515,7 +515,7 @@ public Set getSettingNames() { // TODO: make settings accessible only to code that registered the setting @Override - public SecureString getString(String setting) { + public synchronized SecureString getString(String setting) { ensureOpen(); Entry entry = entries.get().get(setting); if (entry == null || entry.type != EntryType.STRING) { @@ -527,7 +527,7 @@ public SecureString getString(String setting) { } @Override - public InputStream getFile(String setting) { + public synchronized InputStream getFile(String setting) { ensureOpen(); Entry entry = entries.get().get(setting); if (entry == null || entry.type != EntryType.FILE) { @@ -549,7 +549,7 @@ public static void validateSettingName(String setting) { } /** Set a string setting. */ - void setString(String setting, char[] value) { + synchronized void setString(String setting, char[] value) { ensureOpen(); validateSettingName(setting); @@ -562,7 +562,7 @@ void setString(String setting, char[] value) { } /** Set a file setting. */ - void setFile(String setting, byte[] bytes) { + synchronized void setFile(String setting, byte[] bytes) { ensureOpen(); validateSettingName(setting); @@ -589,7 +589,7 @@ private void ensureOpen() { } @Override - public void close() { + public synchronized void close() { this.closed = true; for (Entry entry : entries.get().values()) { Arrays.fill(entry.bytes, (byte)0); From 3af35a03784cf55d1e97677115d60895a74d29af Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 7 May 2018 11:58:27 +1000 Subject: [PATCH 3/4] Revert behaviour of "isLoaded()" --- .../java/org/elasticsearch/common/settings/KeyStoreWrapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 1752c1e3c4b72..3fbdde8ef2bab 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -276,7 +276,7 @@ public static void upgrade(KeyStoreWrapper wrapper, Path configDir, char[] passw @Override public boolean isLoaded() { - return entries.get() != null && closed == false; + return entries.get() != null; } /** Return true iff calling {@link #decrypt(char[])} requires a non-empty password. */ From 64b57c1a3f4f763c789dd368b84fec08b66fa429 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 7 May 2018 12:18:29 +1000 Subject: [PATCH 4/4] Fix save/encrypt Save needs to be sychronized (so that the data doesn't get zero'd out while being written to disk) Encrypt is only called from save, so it can stay as an assert isLoaded rather than ensureOpen --- .../org/elasticsearch/common/settings/KeyStoreWrapper.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java index 3fbdde8ef2bab..f47760491f8d5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -357,7 +357,7 @@ public void decrypt(char[] password) throws GeneralSecurityException, IOExceptio /** Encrypt the keystore entries and return the encrypted data. */ private byte[] encrypt(char[] password, byte[] salt, byte[] iv) throws GeneralSecurityException, IOException { - ensureOpen(); + assert isLoaded(); ByteArrayOutputStream bytes = new ByteArrayOutputStream(); Cipher cipher = createCipher(Cipher.ENCRYPT_MODE, password, salt, iv); @@ -449,7 +449,7 @@ private void decryptLegacyEntries() throws GeneralSecurityException, IOException } /** Write the keystore to the given config directory. */ - public void save(Path configDir, char[] password) throws Exception { + public synchronized void save(Path configDir, char[] password) throws Exception { ensureOpen(); SimpleFSDirectory directory = new SimpleFSDirectory(configDir);