Fail if reading from closed KeyStoreWrapper

In elastic#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.

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;
@@ -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<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();
+        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);
         }

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());