You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by pr...@apache.org on 2018/03/08 23:33:03 UTC
[geode] branch develop updated: GEODE-4792: Expand ArgumentRedactor
regular expression logic for better self-documentation.
This is an automated email from the ASF dual-hosted git repository.
prhomberg pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new 457f1c5 GEODE-4792: Expand ArgumentRedactor regular expression logic for better self-documentation.
457f1c5 is described below
commit 457f1c54f84d64378302a209c91242aca0bec570
Author: Patrick Rhomberg <pr...@pivotal.io>
AuthorDate: Thu Mar 8 15:32:59 2018 -0800
GEODE-4792: Expand ArgumentRedactor regular expression logic for better self-documentation.
---
.../org/apache/geode/internal/AbstractConfig.java | 2 +-
.../java/org/apache/geode/internal/Banner.java | 3 +-
.../apache/geode/internal/net/SocketCreator.java | 2 +-
.../geode/internal/util/ArgumentRedactor.java | 156 ++++++++++++++-------
.../internal/util/ArgumentRedactorJUnitTest.java | 82 ++++++++---
5 files changed, 168 insertions(+), 77 deletions(-)
diff --git a/geode-core/src/main/java/org/apache/geode/internal/AbstractConfig.java b/geode-core/src/main/java/org/apache/geode/internal/AbstractConfig.java
index 2e20037..7fd094f 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/AbstractConfig.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/AbstractConfig.java
@@ -392,7 +392,7 @@ public abstract class AbstractConfig implements Config {
} else {
// Otherwise, redact based on the key string
attributeValueToPrint =
- ArgumentRedactor.redactValueIfNecessary(attName, getAttribute(attName));
+ ArgumentRedactor.redactArgumentIfNecessary(attName, getAttribute(attName));
}
pw.print(attName);
pw.print('=');
diff --git a/geode-core/src/main/java/org/apache/geode/internal/Banner.java b/geode-core/src/main/java/org/apache/geode/internal/Banner.java
index 360c057..c80f435 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/Banner.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/Banner.java
@@ -119,7 +119,8 @@ public class Banner {
for (Object o : sp.entrySet()) {
Entry me = (Entry) o;
String key = me.getKey().toString();
- String value = ArgumentRedactor.redactValueIfNecessary(key, String.valueOf(me.getValue()));
+ String value =
+ ArgumentRedactor.redactArgumentIfNecessary(key, String.valueOf(me.getValue()));
out.println(" " + key + " = " + value);
}
out.println("Log4J 2 Configuration:");
diff --git a/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java b/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
index b049cd1..9478953 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/net/SocketCreator.java
@@ -1086,7 +1086,7 @@ public class SocketCreator {
for (String key : System.getProperties().stringPropertyNames()) { // fix for 46822
if (key.startsWith("javax.net.ssl")) {
String possiblyRedactedValue =
- ArgumentRedactor.redactValueIfNecessary(key, System.getProperty(key));
+ ArgumentRedactor.redactArgumentIfNecessary(key, System.getProperty(key));
sb.append(" ").append(key).append(" = ").append(possiblyRedactedValue).append("\n");
}
}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java b/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
index db67f5f..fa63de2 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/util/ArgumentRedactor.java
@@ -28,43 +28,98 @@ public class ArgumentRedactor {
// All taboo words should be entirely lowercase.
private static final List<String> tabooToContain = ArrayUtils.asList("password");
- private static final List<String> tabooForKeyToStartWith =
+ private static final List<String> tabooForOptionToStartWith =
ArrayUtils.asList(DistributionConfig.SYS_PROP_NAME, DistributionConfig.SSL_SYSTEM_PROPS_NAME,
ConfigurationProperties.SECURITY_PREFIX);
- // This pattern consists of three capture groups:
- // The option, consisting of
- // (a) A leading space or starting string boundary, followed by one or two hyphens
- // (b) one or more non-whitespace, non-"=" characters, matching greedily
- // The option-value separator, consisting of: any amount of whitespace surrounding at most 1 "="
- // The value, consisting of:
- // (a) If not wrapped in quotes, all non-whitespace characters, matching greedily.
- // (b) If wrapped in quotes, any non-quote character, matching greedily, until the closing quote.
- // -- -- This will therefore break on, e.g., --opt="escaped \" quote" and only redact "escaped."
- // Positive lookahead between groups 1 and 2 to require space or "=", while * and ? match empty.
- // Negative lookahead between groups 2 and 3 to avoid "--boolFlag --newOption" matching as a pair.
- private static final Pattern optionWithValuePattern =
- Pattern.compile("([^ ]--?[^\\s=]+)(?=[ =])( *=? *)(?!-)((?:\"[^\"]*\"|\\S+))");
+ private static final Pattern optionWithArgumentPattern = getOptionWithArgumentPattern();
+
+
+ /**
+ * This method returns the {@link java.util.regex.Pattern} given below, used to capture
+ * command-line options that accept an argument. For clarity, the regex is given here without
+ * the escape characters required by Java's string handling.
+ * <p>
+ *
+ * {@code ((?:^| )(?:--J=)?--?)([^\s=]+)(?=[ =])( *[ =] *)(?! *-)((?:"[^"]*"|\S+))}
+ *
+ * <p>
+ * This pattern consists of one captured boundary,
+ * three additional capture groups, and two look-ahead boundaries.
+ *
+ * <p>
+ * The four capture groups are:
+ * <ul>
+ * <li>[1] The beginning boundary, including at most one leading space,
+ * possibly including "--J=", and including the option's leading "-" or "--"</li>
+ * <li>[2] The option, which cannot include spaces</li>
+ * <li>[3] The option / argument separator, consisting of at least one character
+ * made of spaces and/or at most one "="</li>
+ * <li>[4] The argument, which terminates at the next space unless it is encapsulated by
+ * quotation-marks, in which case it terminates at the next quotation mark.</li>
+ * </ul>
+ *
+ * Look-ahead groups avoid falsely identifying two flag options (e.g. `{@code --help --all}`) from
+ * interpreting the second flag as the argument to the first option
+ * (here, misinterpreting as `{@code --help="--all"}`).
+ * <p>
+ *
+ * Note that at time of writing, the argument (capture group 4) is not consumed by this class's
+ * logic, but its capture has proven repeatedly useful during iteration and testing.
+ */
+ private static Pattern getOptionWithArgumentPattern() {
+ String capture_beginningBoundary;
+ {
+ String spaceOrBeginningAnchor = "(?:^| )";
+ String maybeLeadingWithDashDashJEquals = "(?:--J=)?";
+ String oneOrTwoDashes = "--?";
+ capture_beginningBoundary =
+ "(" + spaceOrBeginningAnchor + maybeLeadingWithDashDashJEquals + oneOrTwoDashes + ")";
+ }
+
+ String capture_optionNameHasNoSpaces = "([^\\s=]+)";
+
+ String boundary_lookAheadForSpaceOrEquals = "(?=[ =])";
+
+ String capture_optionArgumentSeparator = "( *[ =] *)";
+
+ String boundary_negativeLookAheadToPreventNextOptionAsThisArgument = "(?! *-)";
+
+ String capture_Argument;
+ {
+ String argumentCanBeAnythingBetweenQuotes = "\"[^\"]*\"";
+ String argumentCanHaveNoSpacesWithoutQuotes = "\\S+";
+ String argumentCanBeEitherOfTheAbove = "(?:" + argumentCanBeAnythingBetweenQuotes + "|"
+ + argumentCanHaveNoSpacesWithoutQuotes + ")";
+ capture_Argument = "(" + argumentCanBeEitherOfTheAbove + ")";
+ }
+
+ String fullPattern = capture_beginningBoundary + capture_optionNameHasNoSpaces
+ + boundary_lookAheadForSpaceOrEquals + capture_optionArgumentSeparator
+ + boundary_negativeLookAheadToPreventNextOptionAsThisArgument + capture_Argument;
+ return Pattern.compile(fullPattern);
+ }
private ArgumentRedactor() {}
/**
- * Parse a string to find key-value pairs and redact the values if necessary.<br>
+ * Parse a string to find option/argument pairs and redact the arguments if necessary.<br>
*
* The following format is expected:<br>
- * - Each key-value pair should be separated by spaces.<br>
- * - The key of each key-value pair must be preceded by a hyphen '-'.<br>
- * - Values may or may not be wrapped in quotation marks.<br>
- * - If a value is wrapped in quotation marks, the actual value should not contain any quotation
- * mark.<br>
- * - Keys and values may be separated by an equals sign '=' or any number of spaces.<br>
+ * - Each option/argument pair should be separated by spaces.<br>
+ * - The option of each pair must be preceded by at least one hyphen '-'.<br>
+ * - Arguments may or may not be wrapped in quotation marks.<br>
+ * - Options and arguments may be separated by an equals sign '=' or any number of spaces.<br>
* <br>
* Examples:<br>
* "--password=secret"<br>
* "--user me --password secret"<br>
- * "-Dflag -Dkey=value"<br>
+ * "-Dflag -Dopt=arg"<br>
* "--classpath=."<br>
*
+ * See {@link #getOptionWithArgumentPattern()} for more information on
+ * the regular expression used.
+ *
* @param line The argument input to be parsed
* @param permitFirstPairWithoutHyphen When true, prepends the line with a "-", which is later
* removed. This allows the use on, e.g., "password=secret" rather than "--password=secret"
@@ -79,16 +134,16 @@ public class ArgumentRedactor {
wasPaddedWithHyphen = true;
}
- // We capture the key, separator, and values separately, replacing only the value at print.
- Matcher matcher = optionWithValuePattern.matcher(line);
+ Matcher matcher = optionWithArgumentPattern.matcher(line);
while (matcher.find()) {
- String option = matcher.group(1);
+ String option = matcher.group(2);
if (!isTaboo(option)) {
continue;
}
- String separator = matcher.group(2);
- String withRedaction = option + separator + redacted;
+ String leadingBoundary = matcher.group(1);
+ String separator = matcher.group(3);
+ String withRedaction = leadingBoundary + option + separator + redacted;
line = line.replace(matcher.group(), withRedaction);
}
@@ -99,10 +154,8 @@ public class ArgumentRedactor {
}
/**
- * This alias permits the first key-value pair to be given without a leading hyphen, so that
- * "password=secret" will be properly redacted.
- *
- * See {@link org.apache.geode.internal.util.ArgumentRedactor#redact(java.lang.String, boolean)}
+ * Alias for {@code redact(line, true)}. See
+ * {@link org.apache.geode.internal.util.ArgumentRedactor#redact(java.lang.String, boolean)}
*/
public static String redact(String line) {
return redact(line, true);
@@ -113,49 +166,50 @@ public class ArgumentRedactor {
}
/**
- * Return a redacted value if the key indicates redaction is necessary. Otherwise, return the
- * value unchanged.
+ * Return the redaction string if the provided option's argument should be redacted.
+ * Otherwise, return the provided argument unchanged.
*
- * @param key A string such as a system property, jvm parameter or similar in a key=value
- * situation.
- * @param value A string that is the value assigned to the key.
+ * @param option A string such as a system property, jvm parameter or command-line option.
+ * @param argument A string that is the argument assigned to the option.
*
- * @return A redacted string if the key indicates it should be redacted, otherwise the string is
- * unchanged.
+ * @return A redacted string if the option indicates it should be redacted, otherwise the
+ * provided argument.
*/
- public static String redactValueIfNecessary(String key, String value) {
- if (isTaboo(key)) {
+ public static String redactArgumentIfNecessary(String option, String argument) {
+ if (isTaboo(option)) {
return redacted;
}
- return value;
+ return argument;
}
-
/**
- * Determine whether a key's value should be redacted.
+ * Determine whether a option's argument should be redacted.
*
- * @param key The option key in question.
+ * @param option The option option in question.
*
* @return true if the value should be redacted, otherwise false.
*/
- static boolean isTaboo(String key) {
- if (key == null) {
+ static boolean isTaboo(String option) {
+ if (option == null) {
return false;
}
- for (String taboo : tabooForKeyToStartWith) {
- if (key.toLowerCase().startsWith(taboo)) {
+ for (String taboo : tabooForOptionToStartWith) {
+ // If a parameter is passed with -Dsecurity-option=argument, the option option is
+ // "Dsecurity-option".
+ // With respect to taboo words, also check for the addition of the extra D
+ if (option.toLowerCase().startsWith(taboo) || option.toLowerCase().startsWith("d" + taboo)) {
return true;
}
}
for (String taboo : tabooToContain) {
- if (key.toLowerCase().contains(taboo)) {
+ if (option.toLowerCase().contains(taboo)) {
return true;
}
}
return false;
}
- public static List<String> redactEachInList(List<String> inputArguments) {
- return inputArguments.stream().map(ArgumentRedactor::redact).collect(Collectors.toList());
+ public static List<String> redactEachInList(List<String> argList) {
+ return argList.stream().map(ArgumentRedactor::redact).collect(Collectors.toList());
}
}
diff --git a/geode-core/src/test/java/org/apache/geode/internal/util/ArgumentRedactorJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/util/ArgumentRedactorJUnitTest.java
index 3ba8f87..3da76a7 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/util/ArgumentRedactorJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/util/ArgumentRedactorJUnitTest.java
@@ -22,8 +22,8 @@ import static org.apache.geode.distributed.ConfigurationProperties.GATEWAY_SSL_T
import static org.apache.geode.distributed.ConfigurationProperties.SERVER_SSL_KEYSTORE_PASSWORD;
import static org.apache.geode.internal.util.ArgumentRedactor.isTaboo;
import static org.apache.geode.internal.util.ArgumentRedactor.redact;
+import static org.apache.geode.internal.util.ArgumentRedactor.redactEachInList;
import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.assertEquals;
import java.util.ArrayList;
import java.util.Arrays;
@@ -35,9 +35,6 @@ import org.junit.experimental.categories.Category;
import org.apache.geode.internal.Banner;
import org.apache.geode.test.junit.categories.UnitTest;
-/**
- * ArgumentRedactor Tester.
- */
@Category(UnitTest.class)
public class ArgumentRedactorJUnitTest {
private static final String someProperty = "redactorTest.someProperty";
@@ -46,24 +43,38 @@ public class ArgumentRedactorJUnitTest {
"redactorTest.aPassword-withCharactersAfterward";
@Test
+ public void theseLinesShouldRedact() {
+ String argumentThatShouldBeRedacted = "__this_should_be_redacted__";
+ List<String> someTabooOptions =
+ Arrays.asList("-Dgemfire.password=" + argumentThatShouldBeRedacted,
+ "--password=" + argumentThatShouldBeRedacted,
+ "--J=-Dgemfire.some.very.qualified.item.password=" + argumentThatShouldBeRedacted,
+ "--J=-Dsysprop-secret.information=" + argumentThatShouldBeRedacted);
+
+ List<String> fullyRedacted = redactEachInList(someTabooOptions);
+ assertThat(fullyRedacted).doesNotContainAnyElementsOf(someTabooOptions);
+ }
+
+ @Test
public void redactorWillIdentifySampleTabooProperties() {
- List<String> shouldBeRedacted = Arrays.asList("password", "security-username",
- "security-manager", CLUSTER_SSL_TRUSTSTORE_PASSWORD, GATEWAY_SSL_TRUSTSTORE_PASSWORD,
- SERVER_SSL_KEYSTORE_PASSWORD, "javax.net.ssl.keyStorePassword",
- "javax.net.ssl.keyStoreType", "sysprop-value");
- for (String key : shouldBeRedacted) {
- assertThat(isTaboo(key)).describedAs("This key should be identified as taboo: " + key)
- .isTrue();
+ List<String> shouldBeRedacted = Arrays.asList("gemfire.security-password", "password",
+ "other-password-option", CLUSTER_SSL_TRUSTSTORE_PASSWORD, GATEWAY_SSL_TRUSTSTORE_PASSWORD,
+ SERVER_SSL_KEYSTORE_PASSWORD, "security-username", "security-manager",
+ "security-important-property", "javax.net.ssl.keyStorePassword",
+ "javax.net.ssl.some.security.item", "javax.net.ssl.keyStoreType", "sysprop-secret-prop");
+ for (String option : shouldBeRedacted) {
+ assertThat(isTaboo(option))
+ .describedAs("This option should be identified as taboo: " + option).isTrue();
}
}
@Test
public void redactorWillAllowSampleMiscProperties() {
- List<String> shouldNotBeRedacted =
- Arrays.asList("gemfire.security-manager", CLUSTER_SSL_ENABLED, CONSERVE_SOCKETS);
- for (String key : shouldNotBeRedacted) {
- assertThat(isTaboo(key)).describedAs("This key should not be identified as taboo: " + key)
- .isFalse();
+ List<String> shouldNotBeRedacted = Arrays.asList("gemfire.security-manager",
+ CLUSTER_SSL_ENABLED, CONSERVE_SOCKETS, "username", "just-an-option");
+ for (String option : shouldNotBeRedacted) {
+ assertThat(isTaboo(option))
+ .describedAs("This option should not be identified as taboo: " + option).isFalse();
}
}
@@ -85,21 +96,43 @@ public class ArgumentRedactorJUnitTest {
assertThat(redacted).contains("--justapassword =********");
}
+ @Test
+ public void argListOfPasswordsAllRedactViaRedactEachInList() {
+ List<String> argList = new ArrayList<>();
+ argList.add("--gemfire.security-password=secret");
+ argList.add("--login-password=secret");
+ argList.add("--gemfire-password = super-secret");
+ argList.add("--geode-password= confidential");
+ argList.add("--some-other-password =shhhh");
+ argList.add("--justapassword =failed");
+ List<String> redacted = redactEachInList(argList);
+ assertThat(redacted).contains("--gemfire.security-password=********");
+ assertThat(redacted).contains("--login-password=********");
+ assertThat(redacted).contains("--gemfire-password = ********");
+ assertThat(redacted).contains("--geode-password= ********");
+ assertThat(redacted).contains("--some-other-password =********");
+ assertThat(redacted).contains("--justapassword =********");
+ }
+
@Test
public void argListOfMiscOptionsDoNotRedact() {
List<String> argList = new ArrayList<>();
argList.add("--gemfire.security-properties=./security.properties");
- argList.add("--gemfire.sys.security-value=someValue");
+ argList.add("--gemfire.sys.security-option=someArg");
argList.add("--gemfire.use-cluster-configuration=true");
- argList.add("--someotherstringvalue");
+ argList.add("--someotherstringoption");
argList.add("--login-name=admin");
+ argList.add("--myArg --myArg2 --myArg3=-arg4");
+ argList.add("--myArg --myArg2 --myArg3=\"-arg4\"");
String redacted = redact(argList);
assertThat(redacted).contains("--gemfire.security-properties=./security.properties");
- assertThat(redacted).contains("--gemfire.sys.security-value=someValue");
+ assertThat(redacted).contains("--gemfire.sys.security-option=someArg");
assertThat(redacted).contains("--gemfire.use-cluster-configuration=true");
- assertThat(redacted).contains("--someotherstringvalue");
+ assertThat(redacted).contains("--someotherstringoption");
assertThat(redacted).contains("--login-name=admin");
+ assertThat(redacted).contains("--myArg --myArg2 --myArg3=-arg4");
+ assertThat(redacted).contains("--myArg --myArg2 --myArg3=\"-arg4\"");
}
@Test
@@ -109,6 +142,9 @@ public class ArgumentRedactorJUnitTest {
arg = "-Dgemfire.security-password=secret";
assertThat(redact(arg)).endsWith("password=********");
+ arg = "--J=-Dsome.highly.qualified.password=secret";
+ assertThat(redact(arg)).endsWith("password=********");
+
arg = "--password=foo";
assertThat(redact(arg)).isEqualToIgnoringWhitespace("--password=********");
@@ -123,16 +159,16 @@ public class ArgumentRedactorJUnitTest {
arg = "-Dgemfire.security-properties=./security-properties";
assertThat(redact(arg)).isEqualTo(arg);
- arg = "-J-Dgemfire.sys.security-value=someValue";
+ arg = "-J-Dgemfire.sys.security-option=someArg";
assertThat(redact(arg)).isEqualTo(arg);
- arg = "-Dgemfire.sys.value=printable";
+ arg = "-Dgemfire.sys.option=printable";
assertThat(redact(arg)).isEqualTo(arg);
arg = "-Dgemfire.use-cluster-configuration=true";
assertThat(redact(arg)).isEqualTo(arg);
- arg = "someotherstringvalue";
+ arg = "someotherstringoption";
assertThat(redact(arg)).isEqualTo(arg);
arg = "--classpath=.";
--
To stop receiving notification emails like this one, please contact
prhomberg@apache.org.