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

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").