Skip to content

Commit 6e0c2dc

Browse files
mergify[bot]mashhurs
mergify[bot]
and
mashhurs
authored
[8.18] (backport #17351) Improve the key validation in secret identifier. (#17586)
* 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]>
1 parent d8726a6 commit 6e0c2dc

File tree

5 files changed

+101
-56
lines changed

5 files changed

+101
-56
lines changed

‎docs/static/keystore.asciidoc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ bin/logstash-keystore add ES_USER ES_PWD
159159

160160
When prompted, enter a value for each key.
161161

162-
NOTE: Key values are limited to ASCII characters. It includes digits, letters, and a few special symbols.
162+
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.
163163

164164
[discrete]
165165
[[list-settings]]

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,14 @@
3535
* */
3636
public class ConfigVariableExpander implements AutoCloseable {
3737

38-
private static String SUBSTITUTION_PLACEHOLDER_REGEX = "\\$\\{(?<name>[a-zA-Z_.][a-zA-Z0-9_.]*)(:(?<default>[^}]*))?}";
39-
40-
private Pattern substitutionPattern = Pattern.compile(SUBSTITUTION_PLACEHOLDER_REGEX);
41-
private SecretStore secretStore;
42-
private EnvironmentVariableProvider envVarProvider;
38+
public static final Pattern KEY_PATTERN = Pattern.compile("[a-zA-Z_.][a-zA-Z0-9_.]*");
39+
public static final String KEY_PATTERN_DESCRIPTION = "Key names are limited to ASCII letters (`a`-`z`, `A`-`Z`), numbers (`0`-`9`), " +
40+
"underscores (`_`), and dots (`.`); they must be at least one character long and cannot begin with a number";
41+
private static final String SUBSTITUTION_PLACEHOLDER_REGEX = "\\$\\{(?<name>" + KEY_PATTERN + ")(:(?<default>[^}]*))?}";
42+
43+
private static final Pattern substitutionPattern = Pattern.compile(SUBSTITUTION_PLACEHOLDER_REGEX);
44+
private final SecretStore secretStore;
45+
private final EnvironmentVariableProvider envVarProvider;
4346

4447
/**
4548
* Creates a ConfigVariableExpander that doesn't lookup any secreted placeholder.

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

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@
2020

2121
package org.logstash.secret;
2222

23+
import org.apache.logging.log4j.Logger;
24+
import org.apache.logging.log4j.LogManager;
25+
import org.apache.logging.log4j.util.Strings;
26+
import org.logstash.plugins.ConfigVariableExpander;
27+
2328
import java.util.Locale;
2429
import java.util.regex.Pattern;
2530

@@ -34,12 +39,14 @@ public class SecretIdentifier {
3439
private final static Pattern urnPattern = Pattern.compile("urn:logstash:secret:"+ VERSION + ":.*$");
3540
private final String key;
3641

42+
private static final Logger logger = LogManager.getLogger(SecretIdentifier.class);
43+
3744
/**
3845
* Constructor
3946
*
4047
* @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}
4148
*/
42-
public SecretIdentifier(String key) {
49+
public SecretIdentifier(final String key) {
4350
this.key = validateWithTransform(key, "key");
4451
}
4552

@@ -49,28 +56,14 @@ public SecretIdentifier(String key) {
4956
* @param urn The {@link String} formatted identifier obtained originally from {@link SecretIdentifier#toExternalForm()}
5057
* @return The {@link SecretIdentifier} object used to identify secrets, null if not valid external form.
5158
*/
52-
public static SecretIdentifier fromExternalForm(String urn) {
59+
public static SecretIdentifier fromExternalForm(final String urn) {
5360
if (urn == null || !urnPattern.matcher(urn).matches()) {
5461
throw new IllegalArgumentException("Invalid external form " + urn);
5562
}
5663
String[] parts = colonPattern.split(urn, 5);
5764
return new SecretIdentifier(validateWithTransform(parts[4], "key"));
5865
}
5966

60-
/**
61-
* Minor validation and downcases the parts
62-
*
63-
* @param part The part of the URN to validate
64-
* @param partName The name of the part used for logging.
65-
* @return The validated and transformed part.
66-
*/
67-
private static String validateWithTransform(String part, String partName) {
68-
if (part == null || part.isEmpty()) {
69-
throw new IllegalArgumentException(String.format("%s may not be null or empty", partName));
70-
}
71-
return part.toLowerCase(Locale.US);
72-
}
73-
7467
@Override
7568
public boolean equals(Object o) {
7669
if (this == o) return true;
@@ -118,4 +111,22 @@ public String toExternalForm() {
118111
public String toString() {
119112
return toExternalForm();
120113
}
114+
115+
/**
116+
* Minor validation and downcases the parts
117+
*
118+
* @param key The key, a part of the URN to validate.
119+
* @param partName The name of the part used for logging.
120+
* @return The validated and transformed part.
121+
*/
122+
private static String validateWithTransform(final String key, final String partName) {
123+
if (key == null || key.isEmpty() || Strings.isBlank(key)) {
124+
throw new IllegalArgumentException(String.format("%s may not be null or empty", partName));
125+
}
126+
127+
if (!ConfigVariableExpander.KEY_PATTERN.matcher(key).matches()) {
128+
logger.warn(String.format("Invalid secret key name `%s` provided. %s", key, ConfigVariableExpander.KEY_PATTERN_DESCRIPTION));
129+
}
130+
return key.toLowerCase(Locale.US);
131+
}
121132
}

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

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,23 @@
2020

2121
package org.logstash.secret.cli;
2222

23+
import org.logstash.plugins.ConfigVariableExpander;
2324
import org.logstash.secret.SecretIdentifier;
24-
import org.logstash.secret.store.*;
25+
import org.logstash.secret.store.SecretStore;
26+
import org.logstash.secret.store.SecretStoreFactory;
27+
import org.logstash.secret.store.SecretStoreUtil;
28+
import org.logstash.secret.store.SecureConfig;
2529

2630
import java.nio.CharBuffer;
2731
import java.nio.charset.CharsetEncoder;
2832
import java.nio.charset.StandardCharsets;
29-
import java.util.*;
33+
import java.util.Arrays;
34+
import java.util.Collection;
35+
import java.util.Collections;
36+
import java.util.EnumSet;
37+
import java.util.List;
38+
import java.util.Optional;
39+
import java.util.Set;
3040
import java.util.stream.Collectors;
3141

3242
import static org.logstash.secret.store.SecretStoreFactory.LOGSTASH_MARKER;
@@ -178,7 +188,7 @@ Set<CommandOptions> getValidOptions() {
178188
}
179189
}
180190

181-
public SecretStoreCli(Terminal terminal){
191+
public SecretStoreCli(Terminal terminal) {
182192
this(terminal, SecretStoreFactory.fromEnvironment());
183193
}
184194

@@ -189,9 +199,10 @@ public SecretStoreCli(Terminal terminal){
189199

190200
/**
191201
* Entry point to issue a command line command.
202+
*
192203
* @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.
193-
* @param config The configuration needed to work a secret store. May be null for help.
194-
* @param allArguments This can be either identifiers for a secret, or a sub command like --help. May be null.
204+
* @param config The configuration needed to work a secret store. May be null for help.
205+
* @param allArguments This can be either identifiers for a secret, or a sub command like --help. May be null.
195206
*/
196207
public void command(String primaryCommand, SecureConfig config, String... allArguments) {
197208
terminal.writeLine("");
@@ -212,7 +223,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
212223
final CommandLine commandLine = commandParseResult.get();
213224
switch (commandLine.getCommand()) {
214225
case CREATE: {
215-
if (commandLine.hasOption(CommandOptions.HELP)){
226+
if (commandLine.hasOption(CommandOptions.HELP)) {
216227
terminal.writeLine("Creates a new keystore. For example: 'bin/logstash-keystore create'");
217228
return;
218229
}
@@ -227,7 +238,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
227238
break;
228239
}
229240
case LIST: {
230-
if (commandLine.hasOption(CommandOptions.HELP)){
241+
if (commandLine.hasOption(CommandOptions.HELP)) {
231242
terminal.writeLine("List all secret identifiers from the keystore. For example: " +
232243
"`bin/logstash-keystore list`. Note - only the identifiers will be listed, not the secrets.");
233244
return;
@@ -239,7 +250,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
239250
break;
240251
}
241252
case ADD: {
242-
if (commandLine.hasOption(CommandOptions.HELP)){
253+
if (commandLine.hasOption(CommandOptions.HELP)) {
243254
terminal.writeLine("Add secrets to the keystore. For example: " +
244255
"`bin/logstash-keystore add my-secret`, at the prompt enter your secret. You will use the identifier ${my-secret} in your Logstash configuration.");
245256
return;
@@ -251,6 +262,9 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
251262
if (secretStoreFactory.exists(config.clone())) {
252263
final SecretStore secretStore = secretStoreFactory.load(config);
253264
for (String argument : commandLine.getArguments()) {
265+
if (!ConfigVariableExpander.KEY_PATTERN.matcher(argument).matches()) {
266+
throw new IllegalArgumentException(String.format("Invalid secret key name `%s` provided. %s", argument, ConfigVariableExpander.KEY_PATTERN_DESCRIPTION));
267+
}
254268
final SecretIdentifier id = new SecretIdentifier(argument);
255269
final byte[] existingValue = secretStore.retrieveSecret(id);
256270
if (existingValue != null) {
@@ -263,7 +277,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
263277

264278
final String enterValueMessage = String.format("Enter value for %s: ", argument);
265279
char[] secret = null;
266-
while(secret == null) {
280+
while (secret == null) {
267281
terminal.write(enterValueMessage);
268282
final char[] readSecret = terminal.readSecret();
269283

@@ -288,7 +302,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
288302
break;
289303
}
290304
case REMOVE: {
291-
if (commandLine.hasOption(CommandOptions.HELP)){
305+
if (commandLine.hasOption(CommandOptions.HELP)) {
292306
terminal.writeLine("Remove secrets from the keystore. For example: " +
293307
"`bin/logstash-keystore remove my-secret`");
294308
return;
@@ -314,7 +328,7 @@ public void command(String primaryCommand, SecureConfig config, String... allArg
314328
}
315329
}
316330

317-
private void printHelp(){
331+
private void printHelp() {
318332
terminal.writeLine("Usage:");
319333
terminal.writeLine("--------");
320334
terminal.writeLine("bin/logstash-keystore [option] command [argument]");

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

Lines changed: 40 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
import java.util.Map;
3636
import java.util.Optional;
3737
import java.util.Queue;
38-
import java.util.UUID;
38+
import java.util.Random;
3939

4040
import static junit.framework.TestCase.assertTrue;
4141
import static org.assertj.core.api.Assertions.assertThat;
@@ -147,8 +147,8 @@ public void testAddEmptyValue() {
147147
terminal.in.add(""); // sets the empty value
148148
terminal.in.add("value");
149149

150-
String id = UUID.randomUUID().toString();
151-
cli.command("add", newStoreConfig.clone(), id);
150+
final String keyName = getRandomKeyName();
151+
cli.command("add", newStoreConfig.clone(), keyName);
152152
assertThat(terminal.out).containsIgnoringCase("ERROR: Value cannot be empty");
153153
}
154154

@@ -159,7 +159,7 @@ public void testAddNonAsciiValue() {
159159
terminal.in.add("€€€€€"); // sets non-ascii value value
160160
terminal.in.add("value");
161161

162-
String id = UUID.randomUUID().toString();
162+
final String id = getRandomKeyName();
163163
cli.command("add", newStoreConfig.clone(), id);
164164
assertThat(terminal.out).containsIgnoringCase("ERROR: Value must contain only ASCII characters");
165165
}
@@ -168,8 +168,8 @@ public void testAddNonAsciiValue() {
168168
public void testAdd() {
169169
createKeyStore();
170170

171-
terminal.in.add(UUID.randomUUID().toString()); // sets the value
172-
String id = UUID.randomUUID().toString();
171+
terminal.in.add(getRandomKeyName()); // sets the value
172+
final String id = getRandomKeyName();
173173
cli.command("add", newStoreConfig.clone(), id);
174174
terminal.reset();
175175

@@ -197,11 +197,11 @@ public void testAddWithNoIdentifiers() {
197197
public void testAddMultipleKeys() {
198198
createKeyStore();
199199

200-
terminal.in.add(UUID.randomUUID().toString());
201-
terminal.in.add(UUID.randomUUID().toString());
200+
terminal.in.add(getRandomKeyName());
201+
terminal.in.add(getRandomKeyName());
202202

203-
final String keyOne = UUID.randomUUID().toString();
204-
final String keyTwo = UUID.randomUUID().toString();
203+
final String keyOne = getRandomKeyName();
204+
final String keyTwo = getRandomKeyName();
205205
cli.command("add", newStoreConfig.clone(), keyOne, keyTwo);
206206
terminal.reset();
207207

@@ -211,18 +211,18 @@ public void testAddMultipleKeys() {
211211

212212
@Test
213213
public void testAddWithoutCreatedKeystore() {
214-
cli.command("add", newStoreConfig.clone(), UUID.randomUUID().toString());
214+
cli.command("add", newStoreConfig.clone(), getRandomKeyName());
215215
assertThat(terminal.out).containsIgnoringCase("ERROR: Logstash keystore not found. Use 'create' command to create one.");
216216
}
217217

218218
@Test
219219
public void testAddWithStdinOption() {
220220
createKeyStore();
221221

222-
terminal.in.add(UUID.randomUUID().toString()); // sets the value
223-
terminal.in.add(UUID.randomUUID().toString()); // sets the value
222+
terminal.in.add(getRandomKeyName()); // sets the value
223+
terminal.in.add(getRandomKeyName()); // sets the value
224224

225-
String id = UUID.randomUUID().toString();
225+
final String id = getRandomKeyName();
226226
cli.command("add", newStoreConfig.clone(), id, SecretStoreCli.CommandOptions.STDIN.getOption());
227227
terminal.reset();
228228

@@ -235,8 +235,8 @@ public void testAddWithStdinOption() {
235235
public void testRemove() {
236236
createKeyStore();
237237

238-
terminal.in.add(UUID.randomUUID().toString()); // sets the value
239-
String id = UUID.randomUUID().toString();
238+
terminal.in.add(getRandomKeyName()); // sets the value
239+
final String id = getRandomKeyName();
240240
cli.command("add", newStoreConfig.clone(), id);
241241
System.out.println(terminal.out);
242242
terminal.reset();
@@ -256,11 +256,11 @@ public void testRemove() {
256256
public void testRemoveMultipleKeys() {
257257
createKeyStore();
258258

259-
terminal.in.add(UUID.randomUUID().toString());
260-
terminal.in.add(UUID.randomUUID().toString());
259+
terminal.in.add(getRandomKeyName());
260+
terminal.in.add(getRandomKeyName());
261261

262-
final String keyOne = UUID.randomUUID().toString();
263-
final String keyTwo = UUID.randomUUID().toString();
262+
final String keyOne = getRandomKeyName();
263+
final String keyTwo = getRandomKeyName();
264264

265265
cli.command("add", newStoreConfig.clone(), keyOne, keyTwo);
266266
terminal.reset();
@@ -281,8 +281,8 @@ public void testRemoveMultipleKeys() {
281281
public void testRemoveMissing() {
282282
createKeyStore();
283283

284-
terminal.in.add(UUID.randomUUID().toString()); // sets the value
285-
String id = UUID.randomUUID().toString();
284+
terminal.in.add(getRandomKeyName()); // sets the value
285+
final String id = getRandomKeyName();
286286
cli.command("add", newStoreConfig.clone(), id);
287287
System.out.println(terminal.out);
288288
terminal.reset();
@@ -318,7 +318,7 @@ public void testCommandWithUnrecognizedOption() {
318318
terminal.in.add("foo");
319319

320320
final String invalidOption = "--invalid-option";
321-
cli.command("add", newStoreConfig.clone(), UUID.randomUUID().toString(), invalidOption);
321+
cli.command("add", newStoreConfig.clone(), getRandomKeyName(), invalidOption);
322322
assertThat(terminal.out).contains(String.format("Unrecognized option '%s' for command 'add'", invalidOption));
323323

324324
terminal.reset();
@@ -387,6 +387,23 @@ private void assertNotListed(String... expected) {
387387
assertTrue(Arrays.stream(expected).noneMatch(terminal.out::contains));
388388
}
389389

390+
private final static String KEY_FIRST_CHAR_CANDIDATES = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_.";
391+
private final static String KEY_REST_CHARS = KEY_FIRST_CHAR_CANDIDATES.concat("01234567890");
392+
private String getRandomKeyName() {
393+
final Random random = new Random();
394+
final StringBuilder sb = new StringBuilder();
395+
396+
// add a random legal first-character
397+
sb.append(KEY_FIRST_CHAR_CANDIDATES.charAt(random.nextInt(KEY_FIRST_CHAR_CANDIDATES.length())));
398+
399+
// add between 0 and 40 random legal rest-characters
400+
final int more = 40;
401+
random.ints(more, 0, KEY_REST_CHARS.length())
402+
.mapToObj(KEY_REST_CHARS::charAt)
403+
.forEach(sb::append);
404+
return sb.toString().toLowerCase();
405+
}
406+
390407
private void assertPrimaryHelped() {
391408
assertThat(terminal.out).
392409
containsIgnoringCase("Commands").

0 commit comments

Comments
 (0)