[8.19] (backport #17351) Improve the key validation in secret identifier. (#17587)

* Improve the key validation in secret identifier. (#17351)

* Improve the key validation in secret identifier.

* Restrict adding invalid (from ConfigVariableExpander point of view) key names to the keystore through keystore CLI. Keystore now warns on invalid keys if they were already added.

* Key pattern is moved to a central config expander class with its description.

Co-authored-by: Rye Biesemeyer <[email protected]>
(cherry picked from commit d24675a)

# Conflicts:
#	docs/reference/keystore.md

* Adjust docs against 8.x

---------

Co-authored-by: Mashhur <[email protected]>
Co-authored-by: Mashhur <[email protected]>

Author: mergify[bot]

docs/static/keystore.asciidoc

@@ -159,7 +159,7 @@ bin/logstash-keystore add ES_USER ES_PWD
 
 When prompted, enter a value for each key.
 
-NOTE: Key values are limited to ASCII characters. It includes digits, letters, and a few special symbols.
+NOTE: Key values are limited to ASCII letters (`a`-`z`, `A`-`Z`), numbers (`0`-`9`), underscores (`_`), and dots (`.`); they must be at least one character long and cannot begin with a number.
 
 [discrete]
 [[list-settings]]

logstash-core/src/main/java/org/logstash/plugins/ConfigVariableExpander.java

@@ -35,11 +35,14 @@
  * */
 public class ConfigVariableExpander implements AutoCloseable {
 
-    private static String SUBSTITUTION_PLACEHOLDER_REGEX = "\\$\\{(?<name>[a-zA-Z_.][a-zA-Z0-9_.]*)(:(?<default>[^}]*))?}";
-
-    private Pattern substitutionPattern = Pattern.compile(SUBSTITUTION_PLACEHOLDER_REGEX);
-    private SecretStore secretStore;
-    private EnvironmentVariableProvider envVarProvider;
+    public static final Pattern KEY_PATTERN = Pattern.compile("[a-zA-Z_.][a-zA-Z0-9_.]*");
+    public static final String KEY_PATTERN_DESCRIPTION = "Key names are limited to ASCII letters (`a`-`z`, `A`-`Z`), numbers (`0`-`9`), " +
+            "underscores (`_`), and dots (`.`); they must be at least one character long and cannot begin with a number";
+    private static final String SUBSTITUTION_PLACEHOLDER_REGEX = "\\$\\{(?<name>" + KEY_PATTERN + ")(:(?<default>[^}]*))?}";
+
+    private static final Pattern substitutionPattern = Pattern.compile(SUBSTITUTION_PLACEHOLDER_REGEX);
+    private final SecretStore secretStore;
+    private final EnvironmentVariableProvider envVarProvider;
 
     /**
      * Creates a ConfigVariableExpander that doesn't lookup any secreted placeholder.

logstash-core/src/main/java/org/logstash/secret/SecretIdentifier.java

@@ -20,6 +20,11 @@
 
 package org.logstash.secret;
 
+import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.util.Strings;
+import org.logstash.plugins.ConfigVariableExpander;
+
 import java.util.Locale;
 import java.util.regex.Pattern;
 
@@ -34,12 +39,14 @@ public class SecretIdentifier {
     private final static Pattern urnPattern = Pattern.compile("urn:logstash:secret:"+ VERSION + ":.*$");
     private final String key;
 
+    private static final Logger logger = LogManager.getLogger(SecretIdentifier.class);
+
     /**
      * Constructor
      *
      * @param key The unique part of the identifier. This is the key to reference the secret, and the key itself should not be sensitive. For example: {@code db.pass}
      */
-    public SecretIdentifier(String key) {
+    public SecretIdentifier(final String key) {
         this.key = validateWithTransform(key, "key");
     }
 
@@ -49,28 +56,14 @@ public SecretIdentifier(String key) {
      * @param urn The {@link String} formatted identifier obtained originally from {@link SecretIdentifier#toExternalForm()}
      * @return The {@link SecretIdentifier} object used to identify secrets, null if not valid external form.
      */
-    public static SecretIdentifier fromExternalForm(String urn) {
+    public static SecretIdentifier fromExternalForm(final String urn) {
         if (urn == null || !urnPattern.matcher(urn).matches()) {
             throw new IllegalArgumentException("Invalid external form " + urn);
         }
         String[] parts = colonPattern.split(urn, 5);
         return new SecretIdentifier(validateWithTransform(parts[4], "key"));
     }
 
-    /**
-     * Minor validation and downcases the parts
-     *
-     * @param part     The part of the URN to validate
-     * @param partName The name of the part used for logging.
-     * @return The validated and transformed part.
-     */
-    private static String validateWithTransform(String part, String partName) {
-        if (part == null || part.isEmpty()) {
-            throw new IllegalArgumentException(String.format("%s may not be null or empty", partName));
-        }
-        return part.toLowerCase(Locale.US);
-    }
-
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
@@ -118,4 +111,22 @@ public String toExternalForm() {
     public String toString() {
         return toExternalForm();
     }
+
+    /**
+     * Minor validation and downcases the parts
+     *
+     * @param key      The key, a part of the URN to validate.
+     * @param partName The name of the part used for logging.
+     * @return The validated and transformed part.
+     */
+    private static String validateWithTransform(final String key, final String partName) {
+        if (key == null || key.isEmpty() || Strings.isBlank(key)) {
+            throw new IllegalArgumentException(String.format("%s may not be null or empty", partName));
+        }
+
+        if (!ConfigVariableExpander.KEY_PATTERN.matcher(key).matches()) {
+            logger.warn(String.format("Invalid secret key name `%s` provided. %s", key, ConfigVariableExpander.KEY_PATTERN_DESCRIPTION));
+        }
+        return key.toLowerCase(Locale.US);
+    }
 }

logstash-core/src/main/java/org/logstash/secret/cli/SecretStoreCli.java

@@ -20,13 +20,23 @@
 
 package org.logstash.secret.cli;
 
+import org.logstash.plugins.ConfigVariableExpander;
 import org.logstash.secret.SecretIdentifier;
-import org.logstash.secret.store.*;
+import org.logstash.secret.store.SecretStore;
+import org.logstash.secret.store.SecretStoreFactory;
+import org.logstash.secret.store.SecretStoreUtil;
+import org.logstash.secret.store.SecureConfig;
 
 import java.nio.CharBuffer;
 import java.nio.charset.CharsetEncoder;
 import java.nio.charset.StandardCharsets;
-import java.util.*;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 import static org.logstash.secret.store.SecretStoreFactory.LOGSTASH_MARKER;
@@ -178,7 +188,7 @@ Set<CommandOptions> getValidOptions() {
         }
     }
 
-    public SecretStoreCli(Terminal terminal){
+    public SecretStoreCli(Terminal terminal) {
         this(terminal, SecretStoreFactory.fromEnvironment());
     }
 
@@ -189,9 +199,10 @@ public SecretStoreCli(Terminal terminal){
 
     /**
      * Entry point to issue a command line command.
+     *
      * @param primaryCommand The string representation of a {@link SecretStoreCli.Command}, if the String does not map to a {@link SecretStoreCli.Command}, then it will show the help menu.
-     * @param config The configuration needed to work a secret store. May be null for help.
-     * @param allArguments This can be either identifiers for a secret, or a sub command like --help. May be null.
+     * @param config         The configuration needed to work a secret store. May be null for help.
+     * @param allArguments   This can be either identifiers for a secret, or a sub command like --help. May be null.
      */
     public void command(String primaryCommand, SecureConfig config, String... allArguments) {
         terminal.writeLine("");
@@ -212,7 +223,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
         final CommandLine commandLine = commandParseResult.get();
         switch (commandLine.getCommand()) {
             case CREATE: {
-                if (commandLine.hasOption(CommandOptions.HELP)){
+                if (commandLine.hasOption(CommandOptions.HELP)) {
                     terminal.writeLine("Creates a new keystore. For example: 'bin/logstash-keystore create'");
                     return;
                 }
@@ -227,7 +238,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
                 break;
             }
             case LIST: {
-                if (commandLine.hasOption(CommandOptions.HELP)){
+                if (commandLine.hasOption(CommandOptions.HELP)) {
                     terminal.writeLine("List all secret identifiers from the keystore. For example: " +
                             "`bin/logstash-keystore list`. Note - only the identifiers will be listed, not the secrets.");
                     return;
@@ -239,7 +250,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
                 break;
             }
             case ADD: {
-                if (commandLine.hasOption(CommandOptions.HELP)){
+                if (commandLine.hasOption(CommandOptions.HELP)) {
                     terminal.writeLine("Add secrets to the keystore. For example: " +
                             "`bin/logstash-keystore add my-secret`, at the prompt enter your secret. You will use the identifier ${my-secret} in your Logstash configuration.");
                     return;
@@ -251,6 +262,9 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
                 if (secretStoreFactory.exists(config.clone())) {
                     final SecretStore secretStore = secretStoreFactory.load(config);
                     for (String argument : commandLine.getArguments()) {
+                        if (!ConfigVariableExpander.KEY_PATTERN.matcher(argument).matches()) {
+                            throw new IllegalArgumentException(String.format("Invalid secret key name `%s` provided. %s", argument, ConfigVariableExpander.KEY_PATTERN_DESCRIPTION));
+                        }
                         final SecretIdentifier id = new SecretIdentifier(argument);
                         final byte[] existingValue = secretStore.retrieveSecret(id);
                         if (existingValue != null) {
@@ -263,7 +277,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
 
                         final String enterValueMessage = String.format("Enter value for %s: ", argument);
                         char[] secret = null;
-                        while(secret == null) {
+                        while (secret == null) {
                             terminal.write(enterValueMessage);
                             final char[] readSecret = terminal.readSecret();
 
@@ -288,7 +302,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
                 break;
             }
             case REMOVE: {
-                if (commandLine.hasOption(CommandOptions.HELP)){
+                if (commandLine.hasOption(CommandOptions.HELP)) {
                     terminal.writeLine("Remove secrets from the keystore. For example: " +
                             "`bin/logstash-keystore remove my-secret`");
                     return;
@@ -314,7 +328,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
         }
     }
 
-    private void printHelp(){
+    private void printHelp() {
         terminal.writeLine("Usage:");
         terminal.writeLine("--------");
         terminal.writeLine("bin/logstash-keystore [option] command [argument]");

logstash-core/src/test/java/org/logstash/secret/cli/SecretStoreCliTest.java

@@ -35,7 +35,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Queue;
-import java.util.UUID;
+import java.util.Random;
 
 import static junit.framework.TestCase.assertTrue;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -147,8 +147,8 @@ public void testAddEmptyValue() {
         terminal.in.add(""); // sets the empty value
         terminal.in.add("value");
 
-        String id = UUID.randomUUID().toString();
-        cli.command("add", newStoreConfig.clone(), id);
+        final String keyName = getRandomKeyName();
+        cli.command("add", newStoreConfig.clone(), keyName);
         assertThat(terminal.out).containsIgnoringCase("ERROR: Value cannot be empty");
     }
 
@@ -159,7 +159,7 @@ public void testAddNonAsciiValue() {
         terminal.in.add("€€€€€"); // sets non-ascii value value
         terminal.in.add("value");
 
-        String id = UUID.randomUUID().toString();
+        final String id = getRandomKeyName();
         cli.command("add", newStoreConfig.clone(), id);
         assertThat(terminal.out).containsIgnoringCase("ERROR: Value must contain only ASCII characters");
     }
@@ -168,8 +168,8 @@ public void testAddNonAsciiValue() {
     public void testAdd() {
         createKeyStore();
 
-        terminal.in.add(UUID.randomUUID().toString()); // sets the value
-        String id = UUID.randomUUID().toString();
+        terminal.in.add(getRandomKeyName()); // sets the value
+        final String id = getRandomKeyName();
         cli.command("add", newStoreConfig.clone(), id);
         terminal.reset();
 
@@ -197,11 +197,11 @@ public void testAddWithNoIdentifiers() {
     public void testAddMultipleKeys() {
         createKeyStore();
 
-        terminal.in.add(UUID.randomUUID().toString());
-        terminal.in.add(UUID.randomUUID().toString());
+        terminal.in.add(getRandomKeyName());
+        terminal.in.add(getRandomKeyName());
 
-        final String keyOne = UUID.randomUUID().toString();
-        final String keyTwo = UUID.randomUUID().toString();
+        final String keyOne = getRandomKeyName();
+        final String keyTwo = getRandomKeyName();
         cli.command("add", newStoreConfig.clone(), keyOne, keyTwo);
         terminal.reset();
 
@@ -211,18 +211,18 @@ public void testAddMultipleKeys() {
 
     @Test
     public void testAddWithoutCreatedKeystore() {
-        cli.command("add", newStoreConfig.clone(), UUID.randomUUID().toString());
+        cli.command("add", newStoreConfig.clone(), getRandomKeyName());
         assertThat(terminal.out).containsIgnoringCase("ERROR: Logstash keystore not found. Use 'create' command to create one.");
     }
 
     @Test
     public void testAddWithStdinOption() {
         createKeyStore();
 
-        terminal.in.add(UUID.randomUUID().toString()); // sets the value
-        terminal.in.add(UUID.randomUUID().toString()); // sets the value
+        terminal.in.add(getRandomKeyName()); // sets the value
+        terminal.in.add(getRandomKeyName()); // sets the value
 
-        String id = UUID.randomUUID().toString();
+        final String id = getRandomKeyName();
         cli.command("add", newStoreConfig.clone(), id, SecretStoreCli.CommandOptions.STDIN.getOption());
         terminal.reset();
 
@@ -235,8 +235,8 @@ public void testAddWithStdinOption() {
     public void testRemove() {
         createKeyStore();
 
-        terminal.in.add(UUID.randomUUID().toString()); // sets the value
-        String id = UUID.randomUUID().toString();
+        terminal.in.add(getRandomKeyName()); // sets the value
+        final String id = getRandomKeyName();
         cli.command("add", newStoreConfig.clone(), id);
         System.out.println(terminal.out);
         terminal.reset();
@@ -256,11 +256,11 @@ public void testRemove() {
     public void testRemoveMultipleKeys() {
         createKeyStore();
 
-        terminal.in.add(UUID.randomUUID().toString());
-        terminal.in.add(UUID.randomUUID().toString());
+        terminal.in.add(getRandomKeyName());
+        terminal.in.add(getRandomKeyName());
 
-        final String keyOne = UUID.randomUUID().toString();
-        final String keyTwo = UUID.randomUUID().toString();
+        final String keyOne = getRandomKeyName();
+        final String keyTwo = getRandomKeyName();
 
         cli.command("add", newStoreConfig.clone(), keyOne, keyTwo);
         terminal.reset();
@@ -281,8 +281,8 @@ public void testRemoveMultipleKeys() {
     public void testRemoveMissing() {
         createKeyStore();
 
-        terminal.in.add(UUID.randomUUID().toString()); // sets the value
-        String id = UUID.randomUUID().toString();
+        terminal.in.add(getRandomKeyName()); // sets the value
+        final String id = getRandomKeyName();
         cli.command("add", newStoreConfig.clone(), id);
         System.out.println(terminal.out);
         terminal.reset();
@@ -318,7 +318,7 @@ public void testCommandWithUnrecognizedOption() {
         terminal.in.add("foo");
 
         final String invalidOption = "--invalid-option";
-        cli.command("add", newStoreConfig.clone(), UUID.randomUUID().toString(), invalidOption);
+        cli.command("add", newStoreConfig.clone(), getRandomKeyName(), invalidOption);
         assertThat(terminal.out).contains(String.format("Unrecognized option '%s' for command 'add'", invalidOption));
 
         terminal.reset();
@@ -387,6 +387,23 @@ private void assertNotListed(String... expected) {
         assertTrue(Arrays.stream(expected).noneMatch(terminal.out::contains));
     }
 
+    private final static String KEY_FIRST_CHAR_CANDIDATES = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_.";
+    private final static String KEY_REST_CHARS = KEY_FIRST_CHAR_CANDIDATES.concat("01234567890");
+    private String getRandomKeyName() {
+        final Random random = new Random();
+        final StringBuilder sb = new StringBuilder();
+
+        // add a random legal first-character
+        sb.append(KEY_FIRST_CHAR_CANDIDATES.charAt(random.nextInt(KEY_FIRST_CHAR_CANDIDATES.length())));
+
+        // add between 0 and 40 random legal rest-characters
+        final int more = 40;
+        random.ints(more, 0, KEY_REST_CHARS.length())
+                .mapToObj(KEY_REST_CHARS::charAt)
+                .forEach(sb::append);
+        return sb.toString().toLowerCase();
+    }
+
     private void assertPrimaryHelped() {
         assertThat(terminal.out).
                 containsIgnoringCase("Commands").