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/01/25 23:27:01 UTC
[geode] branch develop updated: GEODE-4309: Refactor
ArgumentRedactor for improved robustness. (#1313)
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 61d1d4d GEODE-4309: Refactor ArgumentRedactor for improved robustness. (#1313)
61d1d4d is described below
commit 61d1d4d116d3cad44be88093bc5a4eaf7d71ec68
Author: Patrick Rhomberg <Pu...@users.noreply.github.com>
AuthorDate: Thu Jan 25 15:26:58 2018 -0800
GEODE-4309: Refactor ArgumentRedactor for improved robustness. (#1313)
---
.../java/org/apache/geode/internal/Banner.java | 140 +++++++++--------
.../apache/geode/internal/net/SocketCreator.java | 6 +-
.../geode/internal/util/ArgumentRedactor.java | 168 ++++++++++-----------
.../geode/management/internal/cli/shell/Gfsh.java | 2 +-
.../internal/util/ArgumentRedactorJUnitTest.java | 83 +++++-----
5 files changed, 205 insertions(+), 194 deletions(-)
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 ea6dace..2e53c4a 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
@@ -20,16 +20,15 @@ import java.lang.management.ManagementFactory;
import java.lang.management.RuntimeMXBean;
import java.util.ArrayList;
import java.util.Collections;
-import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.Properties;
import java.util.StringTokenizer;
import java.util.TreeMap;
import org.apache.geode.SystemFailure;
import org.apache.geode.distributed.internal.DistributionConfig;
-import org.apache.geode.distributed.internal.DistributionConfigImpl;
import org.apache.geode.internal.logging.LogService;
import org.apache.geode.internal.util.ArgumentRedactor;
@@ -57,10 +56,82 @@ public class Banner {
* @param args possibly null list of command line arguments
*/
static void print(PrintWriter out, String args[]) {
+ // Copy the system properties for printing. Some are given explicit lines, and
+ // others are suppressed. Remove these keys, keeping those we want.
Map sp = new TreeMap((Properties) System.getProperties().clone()); // fix for 46822
- int processId = -1;
+ Object userName = sp.get("user.name");
+ Object userDir = sp.get("user.dir");
+ Object userHome = sp.get("user.home");
+ Object javaClassPath = sp.get("java.class.path");
+ Object javaLibraryPath = sp.get("java.library.path");
+ sp.remove("user.name");
+ sp.remove("user.dir");
+ sp.remove("user.home");
+ sp.remove("java.class.path");
+ sp.remove("java.library.path");
+ sp.remove("os.name");
+ sp.remove("os.arch");
+
final String SEPARATOR =
"---------------------------------------------------------------------------";
+ int processId = attemptToReadProcessId();
+ short currentOrdinal = Version.CURRENT_ORDINAL;
+
+ List<String> commandLineArguments = new ArrayList<>();
+ RuntimeMXBean runtimeBean = ManagementFactory.getRuntimeMXBean();
+ if (runtimeBean != null) {
+ commandLineArguments.addAll(runtimeBean.getInputArguments()); // fixes 45353
+ }
+
+ if (args != null && args.length != 0) {
+ Collections.addAll(commandLineArguments, args);
+ }
+
+ // Print it all out.
+ out.println();
+ out.println(SEPARATOR);
+ printASFLicense(out);
+ out.println(SEPARATOR);
+ GemFireVersion.print(out);
+ out.println("Communications version: " + currentOrdinal);
+ out.println("Process ID: " + processId);
+ out.println("User: " + userName);
+ out.println("Current dir: " + userDir);
+ out.println("Home dir: " + userHome);
+
+ if (!commandLineArguments.isEmpty()) {
+ out.println("Command Line Parameters:");
+ for (String arg : commandLineArguments) {
+ out.println(" " + ArgumentRedactor.redact(arg));
+ }
+ }
+
+ out.println("Class Path:");
+ prettyPrintPath((String) javaClassPath, out);
+ out.println("Library Path:");
+ prettyPrintPath((String) javaLibraryPath, out);
+
+ if (Boolean.getBoolean(DistributionConfig.GEMFIRE_PREFIX + "disableSystemPropertyLogging")) {
+ out.println("System property logging disabled.");
+ } else {
+ out.println("System Properties:");
+ for (Object o : sp.entrySet()) {
+ Entry me = (Entry) o;
+ String key = me.getKey().toString();
+ String value = ArgumentRedactor.redactValueIfNecessary(key, String.valueOf(me.getValue()));
+ out.println(" " + key + " = " + value);
+ }
+ out.println("Log4J 2 Configuration:");
+ out.println(" " + LogService.getConfigInformation());
+ }
+ out.println(SEPARATOR);
+ }
+
+ /**
+ * @return The PID of the current process, or -1 if the PID cannot be determined.
+ */
+ private static int attemptToReadProcessId() {
+ int processId = -1;
try {
processId = OSProcess.getId();
} catch (VirtualMachineError err) {
@@ -76,12 +147,10 @@ public class Banner {
// is still usable:
SystemFailure.checkFailure();
}
- out.println();
-
- final String productName = GemFireVersion.getProductName();
-
- out.println(SEPARATOR);
+ return processId;
+ }
+ private static void printASFLicense(PrintWriter out) {
out.println(" ");
out.println(" Licensed to the Apache Software Foundation (ASF) under one or more");
out.println(" contributor license agreements. See the NOTICE file distributed with this");
@@ -99,61 +168,6 @@ public class Banner {
out.println(" License for the specific language governing permissions and limitations");
out.println(" under the License.");
out.println(" ");
-
- out.println(SEPARATOR);
-
- GemFireVersion.print(out);
-
- out.println("Communications version: " + Version.CURRENT_ORDINAL);
-
- out.println("Process ID: " + processId);
- out.println("User: " + sp.get("user.name"));
- sp.remove("user.name");
- sp.remove("os.name");
- sp.remove("os.arch");
- out.println("Current dir: " + sp.get("user.dir"));
- sp.remove("user.dir");
- out.println("Home dir: " + sp.get("user.home"));
- sp.remove("user.home");
- List<String> allArgs = new ArrayList<>();
- {
- RuntimeMXBean runtimeBean = ManagementFactory.getRuntimeMXBean();
- if (runtimeBean != null) {
- allArgs.addAll(runtimeBean.getInputArguments()); // fixes 45353
- }
- }
-
- if (args != null && args.length != 0) {
- Collections.addAll(allArgs, args);
- }
- if (!allArgs.isEmpty()) {
- out.println("Command Line Parameters:");
- for (String arg : allArgs) {
- out.println(" " + ArgumentRedactor.redact(arg));
- }
- }
-
- out.println("Class Path:");
- prettyPrintPath((String) sp.get("java.class.path"), out);
- sp.remove("java.class.path");
- out.println("Library Path:");
- prettyPrintPath((String) sp.get("java.library.path"), out);
- sp.remove("java.library.path");
-
- if (Boolean.getBoolean(DistributionConfig.GEMFIRE_PREFIX + "disableSystemPropertyLogging")) {
- out.println("System property logging disabled.");
- } else {
- out.println("System Properties:");
- Iterator it = sp.entrySet().iterator();
- while (it.hasNext()) {
- Map.Entry me = (Map.Entry) it.next();
- String key = me.getKey().toString();
- out.println(" " + key + " = " + ArgumentRedactor.redact(String.valueOf(me.getValue())));
- }
- out.println("Log4J 2 Configuration:");
- out.println(" " + LogService.getConfigInformation());
- }
- out.println(SEPARATOR);
}
/**
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 be33cae..b049cd1 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
@@ -62,7 +62,6 @@ import javax.net.ssl.KeyManager;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLEngine;
-import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLPeerUnverifiedException;
import javax.net.ssl.SSLServerSocket;
@@ -1086,8 +1085,9 @@ public class SocketCreator {
// add other options here....
for (String key : System.getProperties().stringPropertyNames()) { // fix for 46822
if (key.startsWith("javax.net.ssl")) {
- String redactedString = ArgumentRedactor.redact(key, System.getProperty(key));
- sb.append(" ").append(key).append(" = ").append(redactedString).append("\n");
+ String possiblyRedactedValue =
+ ArgumentRedactor.redactValueIfNecessary(key, System.getProperty(key));
+ sb.append(" ").append(key).append(" = ").append(possiblyRedactedValue).append("\n");
}
}
logger.debug(sb.toString());
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 7a078e9..f5b0576 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
@@ -16,98 +16,93 @@
package org.apache.geode.internal.util;
import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
public class ArgumentRedactor {
+ private static final String redacted = "********";
- private ArgumentRedactor() {}
+ // All taboo words should be entirely lowercase.
+ private static final List<String> tabooWords = ArrayUtils.asList("password");
- public static String redact(final List<String> args) {
- StringBuilder redacted = new StringBuilder();
- for (String arg : args) {
- redacted.append(redact(arg)).append(" ");
- }
- return redacted.toString().trim();
- }
+ // This pattern consists of three capture groups:
+ // The option, consisting of
+ // (a) 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 ArgumentRedactor() {}
/**
- * Accept a map of key/value pairs and produce a printable string, redacting any necessary values.
+ * Parse a string to find key-value pairs and redact the values if necessary.<br>
*
- * @param map A {@link Map} of key/value pairs such as a collection of properties
+ * 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>
+ * <br>
+ * Examples:<br>
+ * "--password=secret"<br>
+ * "--user me --password secret"<br>
+ * "-Dflag -Dkey=value"<br>
+ * "--classpath=."<br>
+ *
+ * @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"
*
- * @return A printable string with redacted fields. E.g., "username=jdoe password=********"
+ * @return A redacted string that has sensitive information obscured.
*/
- public static String redact(final Map<String, String> map) {
- StringBuilder redacted = new StringBuilder();
- for (Entry<String, String> entry : map.entrySet()) {
- redacted.append(entry.getKey());
- redacted.append("=");
- redacted.append(redact(entry));
- redacted.append(" ");
+ public static String redact(String line, boolean permitFirstPairWithoutHyphen) {
+
+ boolean wasPaddedWithHyphen = false;
+ if (!line.trim().startsWith("-") && permitFirstPairWithoutHyphen) {
+ line = "-" + line.trim();
+ wasPaddedWithHyphen = true;
}
- return redacted.toString().trim();
- }
- /**
- * Returns the redacted value of the {@link Entry} if the key indicates redaction is necessary.
- * Otherwise, value is returned, unchanged.
- *
- * @param entry A key/value pair
- *
- * @return The redacted string for value.
- */
- public static String redact(Entry<String, String> entry) {
- return redact(entry.getKey(), entry.getValue());
+ // We capture the key, separator, and values separately, replacing only the value at print.
+ Matcher matcher = optionWithValuePattern.matcher(line);
+ while (matcher.find()) {
+ String option = matcher.group(1);
+ if (!containsTabooWord(option)) {
+ continue;
+ }
+
+ String separator = matcher.group(2);
+ String withRedaction = option + separator + redacted;
+ line = line.replace(matcher.group(), withRedaction);
+ }
+
+ if (wasPaddedWithHyphen) {
+ line = line.substring(1);
+ }
+ return line;
}
/**
- * Parse a string to find key=value pairs and redact the values if necessary. If more than one
- * key=value pair exists in the input, each pair must be preceded by a hyphen '-' to delineate the
- * pairs. <br>
- * Example:<br>
- * Single value: "password=secret" or "--password=secret" Multiple values: "-Dflag -Dkey=value
- * --classpath=."
+ * This alias permits the first key-value pair to be given without a leading hyphen, so that
+ * "password=secret" will be properly redacted.
*
- * @param line The argument input to be parsed
- * @return A redacted string that has sensitive information obscured.
+ * See {@link org.apache.geode.internal.util.ArgumentRedactor#redact(java.lang.String, boolean)}
*/
public static String redact(String line) {
- StringBuilder redacted = new StringBuilder();
- if (line.startsWith("-")) {
- line = " " + line;
- String[] args = line.split(" -");
- StringBuilder param = new StringBuilder();
- for (String arg : args) {
- if (arg.isEmpty()) {
- param.append("-");
- } else {
- String[] pair = arg.split("=", 2);
- param.append(pair[0].trim());
- if (pair.length == 1) {
- redacted.append(param);
- } else {
- redacted.append(param).append("=").append(redact(param.toString(), pair[1].trim()));
- }
- redacted.append(" ");
- }
- param.setLength(0);
- param.append("-");
- }
- } else {
- String[] args = line.split("=", 2);
- if (args.length == 1) {
- redacted.append(line);
- } else {
- redacted.append(args[0].trim()).append("=").append(redact(args[0], args[1]));
- }
- redacted.append(" ");
- }
- return redacted.toString().trim();
+ return redact(line, true);
}
- public static String redactScriptLine(String line) {
- return line.replaceAll("password=[\\S]+", "password=********");
+ public static String redact(final List<String> args) {
+ return redact(String.join(" ", args));
}
/**
@@ -121,33 +116,30 @@ public class ArgumentRedactor {
* @return A redacted string if the key indicates it should be redacted, otherwise the string is
* unchanged.
*/
- public static String redact(String key, String value) {
- if (shouldBeRedacted(key)) {
- return "********";
+ public static String redactValueIfNecessary(String key, String value) {
+ if (containsTabooWord(key)) {
+ return redacted;
}
- return value.trim();
+ return value;
}
+
/**
* Determine whether a key's value should be redacted.
*
- * @param key The key in question.
+ * @param key The option key in question.
*
* @return true if the value should be redacted, otherwise false.
*/
- private static boolean shouldBeRedacted(String key) {
+ private static boolean containsTabooWord(String key) {
if (key == null) {
return false;
}
-
- // Clean off any flags such as -J and -D to get to the actual start of the parameter
- String compareKey = key;
- if (key.startsWith("-J")) {
- compareKey = key.substring(2);
- }
- if (compareKey.startsWith("-D")) {
- compareKey = compareKey.substring(2);
+ for (String taboo : tabooWords) {
+ if (key.toLowerCase().contains(taboo)) {
+ return true;
+ }
}
- return compareKey.toLowerCase().contains("password");
+ return false;
}
}
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
index db85309..7690157 100755
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/Gfsh.java
@@ -584,7 +584,7 @@ public class Gfsh extends JLineShell {
expandedPropCommandsMap.put(withPropsExpanded, line);
}
if (gfshFileLogger.fineEnabled()) {
- gfshFileLogger.fine(logMessage + ArgumentRedactor.redactScriptLine(withPropsExpanded));
+ gfshFileLogger.fine(logMessage + ArgumentRedactor.redact(withPropsExpanded));
}
success = super.executeScriptLine(withPropsExpanded);
} catch (Exception e) {
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 e0d9bc6..9027a6d 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
@@ -21,13 +21,14 @@ import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import java.util.ArrayList;
-import java.util.HashMap;
import java.util.List;
-import java.util.Map;
+import org.apache.logging.log4j.Logger;
import org.junit.Test;
import org.junit.experimental.categories.Category;
+import org.apache.geode.internal.Banner;
+import org.apache.geode.internal.logging.LogService;
import org.apache.geode.test.junit.categories.UnitTest;
/**
@@ -35,6 +36,11 @@ import org.apache.geode.test.junit.categories.UnitTest;
*/
@Category(UnitTest.class)
public class ArgumentRedactorJUnitTest {
+ private static final String someProperty = "redactorTest.someProperty";
+ private static final String somePasswordProperty = "redactorTest.aPassword";
+ private static final String someOtherPasswordProperty =
+ "redactorTest.aPassword-withCharactersAfterward";
+
@Test
public void testRedactArgList() throws Exception {
List<String> argList = new ArrayList<>();
@@ -49,34 +55,16 @@ public class ArgumentRedactorJUnitTest {
argList.add("geode-password= confidential");
argList.add("some-other-password =shhhh");
String redacted = redact(argList);
- assertTrue(redacted.contains("gemfire.security-password=********"));
- assertTrue(redacted.contains("gemfire.security-properties=./security.properties"));
- assertTrue(redacted.contains("gemfire.sys.security-value=someValue"));
- assertTrue(redacted.contains("gemfire.use-cluster-configuration=true"));
- assertTrue(redacted.contains("someotherstringvalue"));
- assertTrue(redacted.contains("login-password=********"));
- assertTrue(redacted.contains("login-name=admin"));
- assertTrue(redacted.contains("gemfire-password=********"));
- assertTrue(redacted.contains("geode-password=********"));
- assertTrue(redacted.contains("some-other-password=********"));
- }
-
- @Test
- public void testRedactMap() throws Exception {
- Map<String, String> argMap = new HashMap<>();
- argMap.put("gemfire.security-password", "secret");
- argMap.put("gemfire.security-properties", "./security.properties");
- argMap.put("gemfire.sys.security-value", "someValue");
- argMap.put("gemfire.use-cluster-configuration", "true");
- argMap.put("login-password", "secret");
- argMap.put("login-name", "admin");
- String redacted = redact(argMap);
- assertTrue(redacted.contains("gemfire.security-password=********"));
- assertTrue(redacted.contains("gemfire.security-properties=./security.properties"));
- assertTrue(redacted.contains("gemfire.sys.security-value=someValue"));
- assertTrue(redacted.contains("gemfire.use-cluster-configuration=true"));
- assertTrue(redacted.contains("login-password=********"));
- assertTrue(redacted.contains("login-name=admin"));
+ assertThat(redacted).contains("gemfire.security-password=********");
+ assertThat(redacted).contains("gemfire.security-properties=./security.properties");
+ assertThat(redacted).contains("gemfire.sys.security-value=someValue");
+ assertThat(redacted).contains("gemfire.use-cluster-configuration=true");
+ assertThat(redacted).contains("someotherstringvalue");
+ assertThat(redacted).contains("login-password=********");
+ assertThat(redacted).contains("login-name=admin");
+ assertThat(redacted).contains("gemfire-password = ********");
+ assertThat(redacted).contains("geode-password= ********");
+ assertThat(redacted).contains("some-other-password =********");
}
@Test
@@ -115,11 +103,11 @@ public class ArgumentRedactorJUnitTest {
arg =
"-Dlogin-password=secret -Dlogin-name=admin -Dgemfire-password = super-secret --geode-password= confidential -J-Dsome-other-password =shhhh";
String redacted = redact(arg);
- assertTrue(redacted.contains("login-password=********"));
- assertTrue(redacted.contains("login-name=admin"));
- assertTrue(redacted.contains("gemfire-password=********"));
- assertTrue(redacted.contains("geode-password=********"));
- assertTrue(redacted.contains("some-other-password=********"));
+ assertThat(redacted).contains("login-password=********");
+ assertThat(redacted).contains("login-name=admin");
+ assertThat(redacted).contains("gemfire-password = ********");
+ assertThat(redacted).contains("geode-password= ********");
+ assertThat(redacted).contains("some-other-password =********");
arg = "-Dgemfire.security-properties=\"c:\\Program Files (x86)\\My Folder\"";
assertEquals(arg, (redact(arg)));
@@ -127,11 +115,28 @@ public class ArgumentRedactorJUnitTest {
@Test
public void redactScriptLine() throws Exception {
- assertThat(ArgumentRedactor.redactScriptLine("connect --password=test --user=test"))
+ assertThat(redact("connect --password=test --user=test"))
.isEqualTo("connect --password=******** --user=test");
- assertThat(
- ArgumentRedactor.redactScriptLine("connect --test-password=test --product-password=test1"))
- .isEqualTo("connect --test-password=******** --product-password=********");
+ assertThat(redact("connect --test-password=test --product-password=test1"))
+ .isEqualTo("connect --test-password=******** --product-password=********");
+ }
+
+ @Test
+ public void systemPropertiesGetRedactedInBanner() throws Exception {
+ try {
+ System.setProperty(someProperty, "isNotRedacted");
+ System.setProperty(somePasswordProperty, "isRedacted");
+ System.setProperty(someOtherPasswordProperty, "isRedacted");
+
+ List<String> args = ArrayUtils.asList("--user=me", "--password=isRedacted",
+ "--another-password-for-some-reason =isRedacted", "--yet-another-password = isRedacted");
+ String banner = Banner.getString(args.toArray(new String[0]));
+ assertThat(banner).doesNotContain("isRedacted");
+ } finally {
+ System.clearProperty(someProperty);
+ System.clearProperty(somePasswordProperty);
+ System.clearProperty(someOtherPasswordProperty);
+ }
}
}
--
To stop receiving notification emails like this one, please contact
prhomberg@apache.org.