From 6517ac98eb060baa63fe1e843f8526a117fcdcae Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Tue, 15 May 2018 09:57:34 +1000 Subject: [PATCH] Fail if reading from closed KeyStoreWrapper (#30394) 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. --- .../common/settings/KeyStoreWrapper.java | 42 ++++++++++++------- .../common/settings/KeyStoreWrapperTests.java | 15 +++++++ 2 files changed, 43 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 04bbb9279dab5..f47760491f8d5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java +++ b/server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java @@ -158,6 +158,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; @@ -448,8 +449,8 @@ 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(); + public synchronized void save(Path configDir, char[] password) throws Exception { + ensureOpen(); SimpleFSDirectory directory = new SimpleFSDirectory(configDir); // write to tmp file first, then overwrite @@ -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(); + public synchronized SecureString getString(String setting) { + ensureOpen(); Entry entry = entries.get().get(setting); if (entry == null || entry.type != EntryType.STRING) { throw new IllegalArgumentException("Secret setting " + setting + " is not a string"); @@ -520,13 +527,12 @@ public SecureString getString(String setting) { } @Override - public InputStream getFile(String setting) { - assert isLoaded(); + public synchronized InputStream getFile(String setting) { + 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); } @@ -543,8 +549,8 @@ public static void validateSettingName(String setting) { } /** Set a string setting. */ - void setString(String setting, char[] value) { - assert isLoaded(); + synchronized void setString(String setting, char[] value) { + ensureOpen(); validateSettingName(setting); ByteBuffer byteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(value)); @@ -556,8 +562,8 @@ void setString(String setting, char[] value) { } /** Set a file setting. */ - void setFile(String setting, byte[] bytes) { - assert isLoaded(); + synchronized void setFile(String setting, byte[] bytes) { + 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() { + public synchronized 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 e22836087367c..849841943ecc6 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java @@ -48,11 +48,13 @@ import org.elasticsearch.core.internal.io.IOUtils; 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; import static org.hamcrest.Matchers.instanceOf; public class KeyStoreWrapperTests extends ESTestCase { @@ -97,6 +99,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());