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.

Author: tvernum

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<Map<String, Entry>> 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<String> 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);
         }

server/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java

@@ -45,10 +45,14 @@
 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;
+import static org.hamcrest.Matchers.instanceOf;
 
 public class KeyStoreWrapperTests extends ESTestCase {
 
@@ -92,6 +96,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());