Drop distinction in entries for keystore (#41701)

Today we allow adding entries from a file or from a string, yet we
internally maintain this distinction such that if you try to add a value
from a file for a setting that expects a string or add a value from a
string for a setting that expects a file, you will have a bad time. This
causes a pain for operators such that for each setting they need to know
this difference. Yet, we do not need to maintain this distinction
internally as they are bytes after all. This commit removes that
distinction and includes logic to upgrade legacy keystores.

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java

@@ -66,6 +66,7 @@ public class ForbiddenPatternsTask extends DefaultTask {
         .exclude("**/*.zip")
         .exclude("**/*.jks")
         .exclude("**/*.crt")
+        .exclude("**/*.keystore")
         .exclude("**/*.png");
 
     /*

distribution/tools/keystore-cli/src/test/java/org/elasticsearch/common/settings/KeyStoreWrapperTests.java

@@ -19,27 +19,6 @@
 
 package org.elasticsearch.common.settings;
 
-import javax.crypto.Cipher;
-import javax.crypto.CipherOutputStream;
-import javax.crypto.SecretKey;
-import javax.crypto.SecretKeyFactory;
-import javax.crypto.spec.GCMParameterSpec;
-import javax.crypto.spec.PBEKeySpec;
-import javax.crypto.spec.SecretKeySpec;
-import java.io.ByteArrayOutputStream;
-import java.io.DataOutputStream;
-import java.io.EOFException;
-import java.io.IOException;
-import java.io.InputStream;
-import java.nio.charset.StandardCharsets;
-import java.nio.file.FileSystem;
-import java.nio.file.Path;
-import java.security.KeyStore;
-import java.security.SecureRandom;
-import java.util.ArrayList;
-import java.util.Base64;
-import java.util.List;
-
 import org.apache.lucene.codecs.CodecUtil;
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.IndexOutput;
@@ -52,10 +31,36 @@ import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Before;
 
+import javax.crypto.Cipher;
+import javax.crypto.CipherOutputStream;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
+import java.io.EOFException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.FileSystem;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+import java.util.Set;
+
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.notNullValue;
 
 public class KeyStoreWrapperTests extends ESTestCase {
 
@@ -386,4 +391,56 @@ public class KeyStoreWrapperTests extends ESTestCase {
             assertEquals(-1, fileInput.read());
         }
     }
+
+    public void testStringAndFileDistinction() throws Exception {
+        final KeyStoreWrapper wrapper = KeyStoreWrapper.create();
+        wrapper.setString("string_setting", "string_value".toCharArray());
+        final Path temp = createTempDir();
+        Files.writeString(temp.resolve("file_setting"), "file_value", StandardCharsets.UTF_8);
+        wrapper.setFile("file_setting", Files.readAllBytes(temp.resolve("file_setting")));
+        wrapper.save(env.configFile(), new char[0]);
+        wrapper.close();
+
+        final KeyStoreWrapper afterSave = KeyStoreWrapper.load(env.configFile());
+        assertNotNull(afterSave);
+        afterSave.decrypt(new char[0]);
+        assertThat(afterSave.getSettingNames(), equalTo(Set.of("keystore.seed", "string_setting", "file_setting")));
+        assertThat(afterSave.getString("string_setting"), equalTo("string_value"));
+        assertThat(toByteArray(afterSave.getFile("string_setting")), equalTo("string_value".getBytes(StandardCharsets.UTF_8)));
+        assertThat(afterSave.getString("file_setting"), equalTo("file_value"));
+        assertThat(toByteArray(afterSave.getFile("file_setting")), equalTo("file_value".getBytes(StandardCharsets.UTF_8)));
+    }
+
+    public void testLegacyV3() throws GeneralSecurityException, IOException {
+        final Path configDir = createTempDir();
+        final Path keystore = configDir.resolve("elasticsearch.keystore");
+        try (InputStream is = KeyStoreWrapperTests.class.getResourceAsStream("/format-v3-elasticsearch.keystore");
+             OutputStream os = Files.newOutputStream(keystore)) {
+            final byte[] buffer = new byte[4096];
+            int readBytes;
+            while ((readBytes = is.read(buffer)) > 0) {
+                os.write(buffer, 0, readBytes);
+            }
+        }
+        final KeyStoreWrapper wrapper = KeyStoreWrapper.load(configDir);
+        assertNotNull(wrapper);
+        wrapper.decrypt(new char[0]);
+        assertThat(wrapper.getFormatVersion(), equalTo(3));
+        assertThat(wrapper.getSettingNames(), equalTo(Set.of("keystore.seed", "string_setting", "file_setting")));
+        assertThat(wrapper.getString("string_setting"), equalTo("string_value"));
+        assertThat(toByteArray(wrapper.getFile("string_setting")), equalTo("string_value".getBytes(StandardCharsets.UTF_8)));
+        assertThat(wrapper.getString("file_setting"), equalTo("file_value"));
+        assertThat(toByteArray(wrapper.getFile("file_setting")), equalTo("file_value".getBytes(StandardCharsets.UTF_8)));
+    }
+
+    private byte[] toByteArray(final InputStream is) throws IOException {
+        final ByteArrayOutputStream os = new ByteArrayOutputStream();
+        final byte[] buffer = new byte[1024];
+        int readBytes;
+        while ((readBytes = is.read(buffer)) > 0) {
+            os.write(buffer, 0, readBytes);
+        }
+        return os.toByteArray();
+    }
+
 }

server/src/main/java/org/elasticsearch/common/settings/KeyStoreWrapper.java

@@ -19,6 +19,18 @@
 
 package org.elasticsearch.common.settings;
 
+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.SimpleFSDirectory;
+import org.apache.lucene.util.SetOnce;
+import org.elasticsearch.cli.ExitCodes;
+import org.elasticsearch.cli.UserException;
+import org.elasticsearch.common.Randomness;
+
 import javax.crypto.Cipher;
 import javax.crypto.CipherInputStream;
 import javax.crypto.CipherOutputStream;
@@ -27,6 +39,7 @@ import javax.crypto.SecretKeyFactory;
 import javax.crypto.spec.GCMParameterSpec;
 import javax.crypto.spec.PBEKeySpec;
 import javax.crypto.spec.SecretKeySpec;
+
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.DataInputStream;
@@ -56,18 +69,6 @@ import java.util.Map;
 import java.util.Set;
 import java.util.regex.Pattern;
 
-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.SimpleFSDirectory;
-import org.apache.lucene.util.SetOnce;
-import org.elasticsearch.cli.ExitCodes;
-import org.elasticsearch.cli.UserException;
-import org.elasticsearch.common.Randomness;
-
 /**
  * A disk based container for sensitive settings in Elasticsearch.
  *
@@ -84,17 +85,6 @@ public class KeyStoreWrapper implements SecureSettings {
         FILE
     }
 
-    /** An entry in the keystore. The bytes are opaque and interpreted based on the entry type. */
-    private static class Entry {
-        final EntryType type;
-        final byte[] bytes;
-
-        Entry(EntryType type, byte[] bytes) {
-            this.type = type;
-            this.bytes = bytes;
-        }
-    }
-
     /**
      * A regex for the valid characters that a setting name in the keystore may use.
      */
@@ -110,7 +100,7 @@ public class KeyStoreWrapper implements SecureSettings {
     private static final String KEYSTORE_FILENAME = "elasticsearch.keystore";
 
     /** The version of the metadata written before the keystore data. */
-    private static final int FORMAT_VERSION = 3;
+    private static final int FORMAT_VERSION = 4;
 
     /** The oldest metadata format version that can be read. */
     private static final int MIN_FORMAT_VERSION = 1;
@@ -146,6 +136,7 @@ public class KeyStoreWrapper implements SecureSettings {
     // 1: initial version, ES 5.3
     // 2: file setting, ES 5.4
     // 3: FIPS compliant algos, ES 6.3
+    // 4: remove distinction between string/files, ES 6.8/7.1
 
     /** The metadata format version used to read the current keystore wrapper. */
     private final int formatVersion;
@@ -157,7 +148,7 @@ public class KeyStoreWrapper implements SecureSettings {
     private final byte[] dataBytes;
 
     /** The decrypted secret data. See {@link #decrypt(char[])}. */
-    private final SetOnce<Map<String, Entry>> entries = new SetOnce<>();
+    private final SetOnce<Map<String, byte[]>> entries = new SetOnce<>();
     private volatile boolean closed;
 
     private KeyStoreWrapper(int formatVersion, boolean hasPassword, byte[] dataBytes) {
@@ -273,11 +264,13 @@ public class KeyStoreWrapper implements SecureSettings {
 
     /** Upgrades the format of the keystore, if necessary. */
     public static void upgrade(KeyStoreWrapper wrapper, Path configDir, char[] password) throws Exception {
-        // ensure keystore.seed exists
-        if (wrapper.getSettingNames().contains(SEED_SETTING.getKey())) {
+        if (wrapper.getFormatVersion() == FORMAT_VERSION && wrapper.getSettingNames().contains(SEED_SETTING.getKey())) {
             return;
         }
-        addBootstrapSeed(wrapper);
+        // add keystore.seed if necessary
+        if (wrapper.getSettingNames().contains(SEED_SETTING.getKey()) == false) {
+            addBootstrapSeed(wrapper);
+        }
         wrapper.save(configDir, password);
     }
 
@@ -350,11 +343,14 @@ public class KeyStoreWrapper implements SecureSettings {
             int numEntries = input.readInt();
             while (numEntries-- > 0) {
                 String setting = input.readUTF();
-                EntryType entryType = EntryType.valueOf(input.readUTF());
+                if (formatVersion == 3) {
+                    // legacy, the keystore format would previously store the entry type
+                    input.readUTF();
+                }
                 int entrySize = input.readInt();
                 byte[] entryBytes = new byte[entrySize];
                 input.readFully(entryBytes);
-                entries.get().put(setting, new Entry(entryType, entryBytes));
+                entries.get().put(setting, entryBytes);
             }
             if (input.read() != -1) {
                 throw new SecurityException("Keystore has been corrupted or tampered with");
@@ -373,12 +369,11 @@ public class KeyStoreWrapper implements SecureSettings {
         try (CipherOutputStream cipherStream = new CipherOutputStream(bytes, cipher);
              DataOutputStream output = new DataOutputStream(cipherStream)) {
             output.writeInt(entries.get().size());
-            for (Map.Entry<String, Entry> mapEntry : entries.get().entrySet()) {
+            for (Map.Entry<String, byte[]> mapEntry : entries.get().entrySet()) {
                 output.writeUTF(mapEntry.getKey());
-                Entry entry = mapEntry.getValue();
-                output.writeUTF(entry.type.name());
-                output.writeInt(entry.bytes.length);
-                output.write(entry.bytes);
+                byte[] entry = mapEntry.getValue();
+                output.writeInt(entry.length);
+                output.write(entry);
             }
         }
         return bytes.toByteArray();
@@ -453,7 +448,7 @@ public class KeyStoreWrapper implements SecureSettings {
             }
             Arrays.fill(chars, '\0');
 
-            entries.get().put(setting, new Entry(settingType, bytes));
+            entries.get().put(setting, bytes);
         }
     }
 
@@ -526,11 +521,8 @@ public class KeyStoreWrapper implements SecureSettings {
     @Override
     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");
-        }
-        ByteBuffer byteBuffer = ByteBuffer.wrap(entry.bytes);
+        byte[] entry = entries.get().get(setting);
+        ByteBuffer byteBuffer = ByteBuffer.wrap(entry);
         CharBuffer charBuffer = StandardCharsets.UTF_8.decode(byteBuffer);
         return new SecureString(Arrays.copyOfRange(charBuffer.array(), charBuffer.position(), charBuffer.limit()));
     }
@@ -538,11 +530,8 @@ public class KeyStoreWrapper implements SecureSettings {
     @Override
     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);
+        byte[] entry = entries.get().get(setting);
+        return new ByteArrayInputStream(entry);
     }
 
     /**
@@ -564,9 +553,9 @@ public class KeyStoreWrapper implements SecureSettings {
 
         ByteBuffer byteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(value));
         byte[] bytes = Arrays.copyOfRange(byteBuffer.array(), byteBuffer.position(), byteBuffer.limit());
-        Entry oldEntry = entries.get().put(setting, new Entry(EntryType.STRING, bytes));
+        byte[] oldEntry = entries.get().put(setting, bytes);
         if (oldEntry != null) {
-            Arrays.fill(oldEntry.bytes, (byte)0);
+            Arrays.fill(oldEntry, (byte)0);
         }
     }
 
@@ -575,18 +564,18 @@ public class KeyStoreWrapper implements SecureSettings {
         ensureOpen();
         validateSettingName(setting);
 
-        Entry oldEntry = entries.get().put(setting, new Entry(EntryType.FILE, Arrays.copyOf(bytes, bytes.length)));
+        byte[] oldEntry = entries.get().put(setting, Arrays.copyOf(bytes, bytes.length));
         if (oldEntry != null) {
-            Arrays.fill(oldEntry.bytes, (byte)0);
+            Arrays.fill(oldEntry, (byte)0);
         }
     }
 
     /** Remove the given setting from the keystore. */
     void remove(String setting) {
         ensureOpen();
-        Entry oldEntry = entries.get().remove(setting);
+        byte[] oldEntry = entries.get().remove(setting);
         if (oldEntry != null) {
-            Arrays.fill(oldEntry.bytes, (byte)0);
+            Arrays.fill(oldEntry, (byte)0);
         }
     }
 
@@ -601,8 +590,8 @@ public class KeyStoreWrapper implements SecureSettings {
     public synchronized void close() {
         this.closed = true;
         if (null != entries.get() && entries.get().isEmpty() == false) {
-            for (Entry entry : entries.get().values()) {
-                Arrays.fill(entry.bytes, (byte) 0);
+            for (byte[] entry : entries.get().values()) {
+                Arrays.fill(entry, (byte) 0);
             }
         }
     }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPS140SecureSettingsBootstrapCheck.java

@@ -36,7 +36,7 @@ public class FIPS140SecureSettingsBootstrapCheck implements BootstrapCheck {
         if (fipsModeEnabled) {
             try (KeyStoreWrapper secureSettings = KeyStoreWrapper.load(environment.configFile())) {
                 if (secureSettings != null && secureSettings.getFormatVersion() < 3) {
-                    return BootstrapCheckResult.failure("Secure settings store is not of the latest version. Please use " +
+                    return BootstrapCheckResult.failure("Secure settings store is not of the appropriate version. Please use " +
                         "bin/elasticsearch-keystore create to generate a new secure settings store and migrate the secure settings there.");
                 }
             } catch (IOException e) {