geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From prhomb...@apache.org
Subject [geode] branch develop updated: GEODE-4320: Reconcile disparity between Banner and AbstractConfig
Date Thu, 08 Feb 2018 18:29:17 GMT
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 a39394e  GEODE-4320: Reconcile disparity between Banner and AbstractConfig
a39394e is described below

commit a39394e082029c0c2bfdd24bc77da8d2dc1afdf1
Author: Patrick Rhomberg <prhomberg@pivotal.io>
AuthorDate: Thu Feb 8 10:29:13 2018 -0800

    GEODE-4320: Reconcile disparity between Banner and AbstractConfig
---
 .../cli/commands/GfshStartLocatorLogTest.java      |   3 +-
 .../LogsAreFullyRedactedAcceptanceTest.java        | 126 +++++++
 geode-assembly/src/test/resources/security.json    |  45 +++
 .../internal/locator/LocatorStatusResponse.java    |   3 +-
 .../internal/DistributionConfigImpl.java           |   6 -
 .../distributed/internal/FlowControlParams.java    |   4 -
 .../org/apache/geode/internal/AbstractConfig.java  | 380 +++++++++------------
 .../geode/internal/util/ArgumentRedactor.java      |  27 +-
 .../InternalDistributedSystemJUnitTest.java        |  35 +-
 .../geode/internal/AbstractConfigJUnitTest.java    |  91 -----
 .../internal/util/ArgumentRedactorJUnitTest.java   | 134 +++++---
 .../geode/test/junit/rules/GfshCommandRule.java    |   3 +-
 12 files changed, 481 insertions(+), 376 deletions(-)

diff --git a/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java b/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java
index 9ef8839..dc9fc3b 100644
--- a/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java
+++ b/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/GfshStartLocatorLogTest.java
@@ -24,7 +24,6 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
-import org.apache.geode.internal.AbstractConfig;
 import org.apache.geode.test.junit.categories.AcceptanceTest;
 import org.apache.geode.test.junit.rules.gfsh.GfshExecution;
 import org.apache.geode.test.junit.rules.gfsh.GfshRule;
@@ -44,7 +43,7 @@ public class GfshStartLocatorLogTest {
 
   @Test
   public void startupConfigsOnlyLogsOnce() throws Exception {
-    final String startupConfigs = AbstractConfig.GEM_FIRE_PROPERTIES_USING_DEFAULT_VALUES;
+    final String startupConfigs = "### GemFire Properties using default values ###";
     String lines = getExecutionLogs();
     assertThat(lines.indexOf(startupConfigs)).isEqualTo(lines.lastIndexOf(startupConfigs));
   }
diff --git a/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LogsAreFullyRedactedAcceptanceTest.java b/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LogsAreFullyRedactedAcceptanceTest.java
new file mode 100644
index 0000000..e9bfc05
--- /dev/null
+++ b/geode-assembly/src/test/java/org/apache/geode/management/internal/cli/commands/LogsAreFullyRedactedAcceptanceTest.java
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
+import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Properties;
+import java.util.Scanner;
+
+import org.apache.commons.io.FileUtils;
+import org.assertj.core.api.SoftAssertions;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import org.apache.geode.examples.security.ExampleSecurityManager;
+import org.apache.geode.management.internal.cli.util.CommandStringBuilder;
+import org.apache.geode.test.junit.categories.AcceptanceTest;
+import org.apache.geode.test.junit.rules.gfsh.GfshRule;
+import org.apache.geode.util.test.TestUtil;
+
+/**
+ * This test class expects a "security.json" file to be visible on the member classpath. The
+ * security.json is consumed by {@link org.apache.geode.examples.security.ExampleSecurityManager}
+ * and should contain each username/password combination present (even though not all will actually
+ * be consumed by the Security Manager).
+ *
+ * Each password shares the string below for easier log scanning.
+ */
+@Category(AcceptanceTest.class)
+public class LogsAreFullyRedactedAcceptanceTest {
+  private static String sharedPasswordString = "abcdefg";
+
+  private File propertyFile;
+  private File securityPropertyFile;
+
+  @Rule
+  public GfshRule gfsh = new GfshRule();
+
+  @Before
+  public void createDirectoriesAndFiles() throws IOException {
+    propertyFile = gfsh.getTemporaryFolder().newFile("geode.properties");
+    securityPropertyFile = gfsh.getTemporaryFolder().newFile("security.properties");
+
+    Properties properties = new Properties();
+    properties.setProperty(LOG_LEVEL, "debug");
+    properties.setProperty("security-username", "propertyFileUser");
+    properties.setProperty("security-password", sharedPasswordString + "-propertyFile");
+    try (FileOutputStream fileOutputStream = new FileOutputStream(propertyFile)) {
+      properties.store(fileOutputStream, null);
+    }
+
+    Properties securityProperties = new Properties();
+    securityProperties.setProperty(SECURITY_MANAGER, ExampleSecurityManager.class.getName());
+    securityProperties.setProperty(ExampleSecurityManager.SECURITY_JSON, "security.json");
+    securityProperties.setProperty("security-file-username", "securityPropertyFileUser");
+    securityProperties.setProperty("security-file-password",
+        sharedPasswordString + "-securityPropertyFile");
+    try (FileOutputStream fileOutputStream = new FileOutputStream(securityPropertyFile)) {
+      securityProperties.store(fileOutputStream, null);
+    }
+  }
+
+  @Test
+  public void logsDoNotContainStringThatShouldBeRedacted() throws FileNotFoundException {
+    // The json is in the root resource directory.
+    String securityJson =
+        TestUtil.getResourcePath(LogsAreFullyRedactedAcceptanceTest.class, "/security.json");
+    // We want to add the folder to the classpath, so we strip off the filename.
+    securityJson = securityJson.substring(0, securityJson.length() - "security.json".length());
+    String startLocatorCmd =
+        new CommandStringBuilder("start locator").addOption("name", "test-locator")
+            .addOption("properties-file", propertyFile.getAbsolutePath())
+            .addOption("security-properties-file", securityPropertyFile.getAbsolutePath())
+            .addOption("J", "-Dsecure-username-jd=user-jd")
+            .addOption("J", "-Dsecure-password-jd=password-jd").addOption("classpath", securityJson)
+            .getCommandString();
+
+    String startServerCmd = new CommandStringBuilder("start server")
+        .addOption("name", "test-server").addOption("user", "viaStartMemberOptions")
+        .addOption("password", sharedPasswordString + "-viaStartMemberOptions")
+        .addOption("properties-file", propertyFile.getAbsolutePath())
+        .addOption("security-properties-file", securityPropertyFile.getAbsolutePath())
+        .addOption("J", "-Dsecure-username-jd=user-jd")
+        .addOption("J", "-Dsecure-password-jd=" + sharedPasswordString + "-password-jd")
+        .addOption("classpath", securityJson).getCommandString();
+
+
+    gfsh.execute(startLocatorCmd, startServerCmd);
+
+    Collection<File> logs =
+        FileUtils.listFiles(gfsh.getTemporaryFolder().getRoot(), new String[] {"log"}, true);
+
+    // Use soft assertions to report all redaction failures, not just the first one.
+    SoftAssertions softly = new SoftAssertions();
+    for (File logFile : logs) {
+      Scanner scanner = new Scanner(logFile);
+      while (scanner.hasNextLine()) {
+        String line = scanner.nextLine();
+        softly.assertThat(line).describedAs("File: %s, Line: %s", logFile.getAbsolutePath(), line)
+            .doesNotContain(sharedPasswordString);
+      }
+    }
+    softly.assertAll();
+  }
+}
diff --git a/geode-assembly/src/test/resources/security.json b/geode-assembly/src/test/resources/security.json
new file mode 100644
index 0000000..7bc9d2e
--- /dev/null
+++ b/geode-assembly/src/test/resources/security.json
@@ -0,0 +1,45 @@
+{
+  "_comments" : ["This file to be consumed by test class LogsAreFullyRedactedAcceptanceTest",
+    "Each user will have the same 'admin' roll / permissions.",
+    "Each username and password will be distinct for identification but contain 'abcdefg',",
+    "making each line immediately identifiable but still easily parsable for an offending log.",
+    "",
+    "Most of these username-password combinations will not actually be consumed, but are listed.",
+    "here for completion.",
+    "Take care to not accidentally use 'password' in any username."
+  ],
+  "roles": [
+    {
+      "name": "superuser",
+      "operationsAllowed": [
+        "CLUSTER:READ", "CLUSTER:MANAGE", "CLUSTER:WRITE",
+        "DATA:READ", "DATA:MANAGE", "DATA:WRITE"
+      ]
+    }
+  ],
+  "users": [
+    {
+      "name": "propertyFileUser",
+      "password": "abcdefg-propertyFile",
+      "roles": ["superuser"]
+    },
+    {
+      "name": "securityPropertyFileUser",
+      "password": "abcdefg-securityPropertyFile",
+      "roles": ["superuser"]
+    },
+    {
+      "name": "viaStartMemberOptions",
+      "password": "abcdefg-viaStartMemberOptions",
+      "roles": ["superuser"]
+    },
+    {
+      "name": "user-jd",
+      "password": "abcdefg-password-jd",
+      "roles": ["superuser"]
+    }
+  ]
+}
+
+
+
diff --git a/geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java b/geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
index ea0afe9..3a1be1a 100644
--- a/geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
+++ b/geode-core/src/main/java/org/apache/geode/cache/client/internal/locator/LocatorStatusResponse.java
@@ -30,6 +30,7 @@ import org.apache.geode.internal.lang.ObjectUtils;
 import org.apache.geode.internal.lang.StringUtils;
 import org.apache.geode.internal.process.PidUnavailableException;
 import org.apache.geode.internal.process.ProcessUtils;
+import org.apache.geode.internal.util.ArgumentRedactor;
 
 /**
  * The LocatorStatusResponse class...
@@ -306,7 +307,7 @@ public class LocatorStatusResponse extends ServerLocationResponse {
     buffer.append("{ pid = ").append(getPid());
     buffer.append(", uptime = ").append(getUptime());
     buffer.append(", workingDirectory = ").append(getWorkingDirectory());
-    buffer.append(", jvmArgs = ").append(getJvmArgs());
+    buffer.append(", jvmArgs = ").append(ArgumentRedactor.redact(getJvmArgs()));
     buffer.append(", classpath = ").append(getClasspath());
     buffer.append(", gemfireVersion = ").append(getGemFireVersion());
     buffer.append(", javaVersion = ").append(getJavaVersion());
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
index 45a8a63..47e0943 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java
@@ -981,12 +981,6 @@ public class DistributionConfigImpl extends AbstractDistributionConfig implement
           securityPeerAuthenticator);
     }
 
-    Iterator iter = security.entrySet().iterator();
-    while (iter.hasNext()) {
-      Map.Entry entry = (Map.Entry) iter.next();
-      System.setProperty(SECURITY_SYSTEM_PREFIX + (String) entry.getKey(),
-          (String) entry.getValue());
-    }
     if (!isConnected) {
       copySSLPropsToServerSSLProps();
       copySSLPropsToJMXSSLProps();
diff --git a/geode-core/src/main/java/org/apache/geode/distributed/internal/FlowControlParams.java b/geode-core/src/main/java/org/apache/geode/distributed/internal/FlowControlParams.java
index e4594eb..20fc244 100644
--- a/geode-core/src/main/java/org/apache/geode/distributed/internal/FlowControlParams.java
+++ b/geode-core/src/main/java/org/apache/geode/distributed/internal/FlowControlParams.java
@@ -51,10 +51,6 @@ public class FlowControlParams implements java.io.Serializable {
   @Override
   public String toString() {
     return ("" + byteAllowance + ", " + rechargeThreshold + ", " + rechargeBlockMs);
-    // return "FlowControlParams(byteAllowance="+byteAllowance
-    // +", rechargeThreshold="+rechargeThreshold
-    // +", rechargeBlockMs="+rechargeBlockMs
-    // +")";
   }
 
   /** returns the byteAllowance setting */
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 4560e49..2e20037 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
@@ -14,7 +14,8 @@
  */
 package org.apache.geode.internal;
 
-import static org.apache.geode.distributed.ConfigurationProperties.*;
+import static org.apache.geode.distributed.ConfigurationProperties.MEMBERSHIP_PORT_RANGE;
+import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_SHIRO_INIT;
 
 import java.io.File;
 import java.io.FileOutputStream;
@@ -37,11 +38,11 @@ import java.util.TreeSet;
 
 import org.apache.geode.InternalGemFireException;
 import org.apache.geode.UnmodifiableException;
-import org.apache.geode.distributed.internal.DistributionConfig;
 import org.apache.geode.distributed.internal.FlowControlParams;
 import org.apache.geode.internal.i18n.LocalizedStrings;
 import org.apache.geode.internal.net.SocketCreator;
 import org.apache.geode.internal.security.SecurableCommunicationChannel;
+import org.apache.geode.internal.util.ArgumentRedactor;
 
 /**
  * Provides an implementation of the {@link Config} interface that implements functionality that all
@@ -49,37 +50,11 @@ import org.apache.geode.internal.security.SecurableCommunicationChannel;
  */
 public abstract class AbstractConfig implements Config {
 
-  public static final String GEM_FIRE_PROPERTIES_USING_DEFAULT_VALUES =
+  private static final String GEM_FIRE_PROPERTIES_USING_DEFAULT_VALUES =
       "### GemFire Properties using default values ###";
-  public static final String GEM_FIRE_PROPERTIES_DEFINED_WITH_PREFIX =
+  private static final String GEM_FIRE_PROPERTIES_DEFINED_WITH_PREFIX =
       "### GemFire Properties defined with ";
-  public static final String GEM_FIRE_PROPERTIES_DEFINED_WITH_SUFFIX = " ###";
-
-  /**
-   * Returns the string to use as the exception message when an attempt is made to set an
-   * unmodifiable attribute.
-   */
-  protected String _getUnmodifiableMsg(String attName) {
-    return LocalizedStrings.AbstractConfig_THE_0_CONFIGURATION_ATTRIBUTE_CAN_NOT_BE_MODIFIED
-        .toLocalizedString(attName);
-  }
-
-  /**
-   * Returns a map that contains attribute descriptions
-   */
-  protected abstract Map getAttDescMap();
-
-  protected abstract Map<String, ConfigSource> getAttSourceMap();
-
-  public static final String sourceHeader = "PropertiesSourceHeader";
-
-  /**
-   * Set to true if most of the attributes can be modified. Set to false if most of the attributes
-   * are read only.
-   */
-  protected boolean _modifiableDefault() {
-    return false;
-  }
+  private static final String GEM_FIRE_PROPERTIES_DEFINED_WITH_SUFFIX = " ###";
 
   /**
    * Use {@link #toLoggerString()} instead. If you need to override this in a subclass, be careful
@@ -88,7 +63,7 @@ public abstract class AbstractConfig implements Config {
    */
   @Override
   public String toString() {
-    return getClass().getName() + "@" + Integer.toHexString(hashCode());
+    return super.toString();
   }
 
   @Override
@@ -111,22 +86,16 @@ public abstract class AbstractConfig implements Config {
   /***
    * Gets the Map of GemFire properties and values from a given ConfigSource
    *
-   * @param source
-   *
    * @return map of GemFire properties and values
    */
   public Map<String, String> getConfigPropsFromSource(ConfigSource source) {
-    Map<String, String> configProps = new HashMap<String, String>();
+    Map<String, String> configProps = new HashMap<>();
     String[] validAttributeNames = getAttributeNames();
     Map<String, ConfigSource> sm = getAttSourceMap();
 
-    for (int i = 0; i < validAttributeNames.length; i++) {
-      String attName = validAttributeNames[i];
-      if (source == null) {
-        if (sm.get(attName) != null) {
-          continue;
-        }
-      } else if (!source.equals(sm.get(attName))) {
+    for (String attName : validAttributeNames) {
+      if ((source == null && sm.get(attName) != null)
+          || (source != null && !source.equals(sm.get(attName)))) {
         continue;
       }
       configProps.put(attName, this.getAttribute(attName));
@@ -140,122 +109,31 @@ public abstract class AbstractConfig implements Config {
    * @return Map of GemFire properties and values set using property files
    */
   public Map<String, String> getConfigPropsDefinedUsingFiles() {
-    Map<String, String> configProps = new HashMap<String, String>();
+    Map<String, String> configProps = new HashMap<>();
     for (ConfigSource fileSource : getFileSources()) {
       configProps.putAll(getConfigPropsFromSource(fileSource));
     }
     return configProps;
   }
 
-  private List<ConfigSource> getFileSources() {
-    ArrayList<ConfigSource> result = new ArrayList<ConfigSource>();
-    for (ConfigSource cs : getAttSourceMap().values()) {
-      if (cs.getType() == ConfigSource.Type.FILE || cs.getType() == ConfigSource.Type.SECURE_FILE) {
-        if (!result.contains(cs)) {
-          result.add(cs);
-        }
-      }
-    }
-    return result;
-  }
-
-  private void printSourceSection(ConfigSource source, PrintWriter pw) {
-    String[] validAttributeNames = getAttributeNames();
-    boolean sourceFound = false;
-    Map<String, ConfigSource> sm = getAttSourceMap();
-    boolean secureSource = false;
-    if (source != null && source.getType() == ConfigSource.Type.SECURE_FILE) {
-      secureSource = true;
-    }
-    for (int i = 0; i < validAttributeNames.length; i++) {
-      String attName = validAttributeNames[i];
-      if (source == null) {
-        if (sm.get(attName) != null) {
-          continue;
-        }
-      } else if (!source.equals(sm.get(attName))) {
-        continue;
-      }
-      if (!sourceFound) {
-        sourceFound = true;
-        if (source == null) {
-          pw.println(GEM_FIRE_PROPERTIES_USING_DEFAULT_VALUES);
-        } else {
-          pw.println(GEM_FIRE_PROPERTIES_DEFINED_WITH_PREFIX + source.getDescription()
-              + GEM_FIRE_PROPERTIES_DEFINED_WITH_SUFFIX);
-        }
-      }
-      // hide the shiro-init configuration for now. Remove after we can allow customer to specify
-      // shiro.ini file
-      if (attName.equals(SECURITY_SHIRO_INIT)) {
-        continue;
-      }
-      pw.print(attName);
-      pw.print('=');
-      if (source == null // always show defaults
-          || (!secureSource // show no values from a secure source
-              && okToDisplayPropertyValue(attName))) {
-        pw.println(getAttribute(attName));
-      } else {
-        pw.println("********");
-      }
-    }
-  }
-
-  private boolean okToDisplayPropertyValue(String attName) {
-    if (attName.startsWith(SECURITY_PREFIX)) {
-      return false;
-    }
-    if (attName.startsWith(DistributionConfig.SSL_SYSTEM_PROPS_NAME)) {
-      return false;
-    }
-    if (attName.startsWith(DistributionConfig.SYS_PROP_NAME)) {
-      return false;
-    }
-    return !attName.toLowerCase().contains("password");
-  }
-
-  /**
-   * This class was added to fix bug 39382. It does this be overriding "keys" which is used by the
-   * store0 implementation of Properties.
-   */
-  protected static class SortedProperties extends Properties {
-
-    private static final long serialVersionUID = 7156507110684631135L;
-
-    @Override
-    public Enumeration keys() {
-      // the TreeSet gets the sorting we desire but is only safe
-      // because the keys in this context are always String which is Comparable
-      return Collections.enumeration(new TreeSet(keySet()));
-    }
-  }
-
-  public boolean isDeprecated(String attName) {
-    return false;
-  }
-
+  @Override
   public Properties toProperties() {
     Properties result = new SortedProperties();
     String[] attNames = getAttributeNames();
-    for (int i = 0; i < attNames.length; i++) {
-      if (isDeprecated(attNames[i])) {
-        continue;
-      }
-      result.setProperty(attNames[i], getAttribute(attNames[i]));
+    for (String attName : attNames) {
+      result.setProperty(attName, getAttribute(attName));
     }
     return result;
   }
 
+  @Override
   public void toFile(File f) throws IOException {
-    FileOutputStream out = new FileOutputStream(f);
-    try {
+    try (FileOutputStream out = new FileOutputStream(f)) {
       toProperties().store(out, null);
-    } finally {
-      out.close();
     }
   }
 
+  @Override
   public boolean sameAs(Config other) {
     if (this == other) {
       return true;
@@ -267,53 +145,39 @@ public abstract class AbstractConfig implements Config {
       return false;
     }
     String[] validAttributeNames = getAttributeNames();
-    for (int i = 0; i < validAttributeNames.length; i++) {
-      String attName = validAttributeNames[i];
-      if (this.isDeprecated(attName)) {
-        // since toProperties skips isDeprecated sameAs
-        // needs to also skip them.
-        // See GEODE-405.
-        continue;
-      }
+    for (String attName : validAttributeNames) {
       Object thisAtt = this.getAttributeObject(attName);
       Object otherAtt = other.getAttributeObject(attName);
-      if (thisAtt == otherAtt) {
-        continue;
-      } else if (thisAtt == null) {
-        return false;
-      } else if (thisAtt.getClass().isArray()) {
-        int thisLength = Array.getLength(thisAtt);
-        int otherLength = Array.getLength(otherAtt);
-        if (thisLength != otherLength) {
+      if (thisAtt != otherAtt) {
+        if (thisAtt == null) {
           return false;
-        }
-        for (int j = 0; j < thisLength; j++) {
-          Object thisArrObj = Array.get(thisAtt, j);
-          Object otherArrObj = Array.get(otherAtt, j);
-          if (thisArrObj == otherArrObj) {
-            continue;
-          } else if (thisArrObj == null) {
-            return false;
-          } else if (!thisArrObj.equals(otherArrObj)) {
+        } else if (thisAtt.getClass().isArray()) {
+          int thisLength = Array.getLength(thisAtt);
+          int otherLength = Array.getLength(otherAtt);
+          if (thisLength != otherLength) {
             return false;
           }
+          for (int j = 0; j < thisLength; j++) {
+            Object thisArrObj = Array.get(thisAtt, j);
+            Object otherArrObj = Array.get(otherAtt, j);
+            if (thisArrObj != otherArrObj) {
+              if (thisArrObj == null) {
+                return false;
+              } else if (!thisArrObj.equals(otherArrObj)) {
+                return false;
+              }
+            }
+          }
+        } else if (!thisAtt.equals(otherAtt)) {
+          return false;
         }
-      } else if (!thisAtt.equals(otherAtt)) {
-        return false;
       }
     }
     return true;
   }
 
-  protected void checkAttributeName(String attName) {
-    String[] validAttNames = getAttributeNames();
-    if (!Arrays.asList(validAttNames).contains(attName.toLowerCase())) {
-      throw new IllegalArgumentException(
-          LocalizedStrings.AbstractConfig_UNKNOWN_CONFIGURATION_ATTRIBUTE_NAME_0_VALID_ATTRIBUTE_NAMES_ARE_1
-              .toLocalizedString(new Object[] {attName, SystemAdmin.join(validAttNames)}));
-    }
-  }
 
+  @Override
   public String getAttribute(String attName) {
     Object result = getAttributeObject(attName);
     if (result instanceof String) {
@@ -335,7 +199,7 @@ public abstract class AbstractConfig implements Config {
 
     if (result instanceof InetAddress) {
       InetAddress addr = (InetAddress) result;
-      String addrName = null;
+      String addrName;
       if (addr.isMulticastAddress() || !SocketCreator.resolve_dns) {
         addrName = addr.getHostAddress(); // on Windows getHostName on mcast addrs takes ~5 seconds
       } else {
@@ -347,10 +211,12 @@ public abstract class AbstractConfig implements Config {
     return result.toString();
   }
 
+  @Override
   public ConfigSource getAttributeSource(String attName) {
     return getAttSourceMap().get(attName);
   }
 
+  @Override
   public void setAttribute(String attName, String attValue, ConfigSource source) {
     Object attObjectValue;
     Class valueType = getAttributeType(attName);
@@ -358,7 +224,7 @@ public abstract class AbstractConfig implements Config {
       if (valueType.equals(String.class)) {
         attObjectValue = attValue;
       } else if (valueType.equals(String[].class)) {
-        attObjectValue = commaDelimitedStringToStringArray(attValue);
+        attObjectValue = attValue.split(",");
       } else if (valueType.equals(Integer.class)) {
         attObjectValue = Integer.valueOf(attValue);
       } else if (valueType.equals(Long.class)) {
@@ -374,8 +240,8 @@ public abstract class AbstractConfig implements Config {
           throw new IllegalArgumentException(
               "expected a setting in the form X-Y but found no dash for attribute " + attName);
         }
-        value[0] = Integer.valueOf(attValue.substring(0, minus)).intValue();
-        value[1] = Integer.valueOf(attValue.substring(minus + 1)).intValue();
+        value[0] = Integer.valueOf(attValue.substring(0, minus));
+        value[1] = Integer.valueOf(attValue.substring(minus + 1));
         attObjectValue = value;
       } else if (valueType.equals(InetAddress.class)) {
         try {
@@ -383,13 +249,13 @@ public abstract class AbstractConfig implements Config {
         } catch (UnknownHostException ex) {
           throw new IllegalArgumentException(
               LocalizedStrings.AbstractConfig_0_VALUE_1_MUST_BE_A_VALID_HOST_NAME_2
-                  .toLocalizedString(new Object[] {attName, attValue, ex.toString()}));
+                  .toLocalizedString(attName, attValue, ex.toString()));
         }
       } else if (valueType.equals(String[].class)) {
         if (attValue == null || attValue.length() == 0) {
           attObjectValue = null;
         } else {
-          String trimAttName = trimAttributeName(attName);
+          String trimAttName = attName.substring(0, attName.length() - 1);
           throw new UnmodifiableException(
               LocalizedStrings.AbstractConfig_THE_0_CONFIGURATION_ATTRIBUTE_CAN_NOT_BE_SET_FROM_THE_COMMAND_LINE_SET_1_FOR_EACH_INDIVIDUAL_PARAMETER_INSTEAD
                   .toLocalizedString(attName, trimAttName));
@@ -399,47 +265,139 @@ public abstract class AbstractConfig implements Config {
         if (values.length != 3) {
           throw new IllegalArgumentException(
               LocalizedStrings.AbstractConfig_0_VALUE_1_MUST_HAVE_THREE_ELEMENTS_SEPARATED_BY_COMMAS
-                  .toLocalizedString(new Object[] {attName, attValue}));
+                  .toLocalizedString(attName, attValue));
         }
-        int credits = 0;
-        float thresh = (float) 0.0;
-        int waittime = 0;
+        int allowance;
+        float threshold;
+        int waitTime;
         try {
-          credits = Integer.parseInt(values[0].trim());
-          thresh = Float.valueOf(values[1].trim()).floatValue();
-          waittime = Integer.parseInt(values[2].trim());
+          allowance = Integer.parseInt(values[0].trim());
+          threshold = Float.valueOf(values[1].trim());
+          waitTime = Integer.parseInt(values[2].trim());
         } catch (NumberFormatException e) {
           throw new IllegalArgumentException(
               LocalizedStrings.AbstractConfig_0_VALUE_1_MUST_BE_COMPOSED_OF_AN_INTEGER_A_FLOAT_AND_AN_INTEGER
-                  .toLocalizedString(new Object[] {attName, attValue}));
+                  .toLocalizedString(attName, attValue));
         }
-        attObjectValue = new FlowControlParams(credits, thresh, waittime);
+        attObjectValue = new FlowControlParams(allowance, threshold, waitTime);
       } else if (valueType.isArray()
           && SecurableCommunicationChannel.class.equals(valueType.getComponentType())) {
         attObjectValue = commaDelimitedStringToSecurableCommunicationChannels(attValue);
       } else {
         throw new InternalGemFireException(
             LocalizedStrings.AbstractConfig_UNHANDLED_ATTRIBUTE_TYPE_0_FOR_1
-                .toLocalizedString(new Object[] {valueType, attName}));
+                .toLocalizedString(valueType, attName));
       }
-    } catch (NumberFormatException ex)
-
-    {
+    } catch (NumberFormatException ex) {
       throw new IllegalArgumentException(LocalizedStrings.AbstractConfig_0_VALUE_1_MUST_BE_A_NUMBER
-          .toLocalizedString(new Object[] {attName, attValue}));
+          .toLocalizedString(attName, attValue));
     }
 
     setAttributeObject(attName, attObjectValue, source);
+  }
 
+  @Override
+  public String getAttributeDescription(String attName) {
+    checkAttributeName(attName);
+    if (!getAttDescMap().containsKey(attName)) {
+      throw new InternalGemFireException(
+          LocalizedStrings.AbstractConfig_UNHANDLED_ATTRIBUTE_NAME_0.toLocalizedString(attName));
+    }
+    return (String) getAttDescMap().get(attName);
   }
 
-  private String[] commaDelimitedStringToStringArray(final String tokenizeString) {
-    StringTokenizer stringTokenizer = new StringTokenizer(tokenizeString, ",");
-    String[] strings = new String[stringTokenizer.countTokens()];
-    for (int i = 0; i < strings.length; i++) {
-      strings[i] = stringTokenizer.nextToken();
+  /**
+   * Returns the string to use as the exception message when an attempt is made to set an
+   * unmodifiable attribute.
+   */
+  protected String _getUnmodifiableMsg(String attName) {
+    return LocalizedStrings.AbstractConfig_THE_0_CONFIGURATION_ATTRIBUTE_CAN_NOT_BE_MODIFIED
+        .toLocalizedString(attName);
+  }
+
+  /**
+   * Returns a map that contains attribute descriptions
+   */
+  protected abstract Map getAttDescMap();
+
+  protected abstract Map<String, ConfigSource> getAttSourceMap();
+
+  /**
+   * Set to true if most of the attributes can be modified. Set to false if most of the attributes
+   * are read only.
+   */
+  protected boolean _modifiableDefault() {
+    return false;
+  }
+
+  protected void checkAttributeName(String attName) {
+    String[] validAttNames = getAttributeNames();
+    if (!Arrays.asList(validAttNames).contains(attName.toLowerCase())) {
+      throw new IllegalArgumentException(
+          LocalizedStrings.AbstractConfig_UNKNOWN_CONFIGURATION_ATTRIBUTE_NAME_0_VALID_ATTRIBUTE_NAMES_ARE_1
+              .toLocalizedString(attName, SystemAdmin.join(validAttNames)));
+    }
+  }
+
+  private List<ConfigSource> getFileSources() {
+    ArrayList<ConfigSource> result = new ArrayList<>();
+    for (ConfigSource cs : getAttSourceMap().values()) {
+      if (cs.getType() == ConfigSource.Type.FILE || cs.getType() == ConfigSource.Type.SECURE_FILE) {
+        if (!result.contains(cs)) {
+          result.add(cs);
+        }
+      }
+    }
+    return result;
+  }
+
+  private void printSourceSection(ConfigSource source, PrintWriter pw) {
+    String[] validAttributeNames = getAttributeNames();
+    boolean sourceFound = false;
+    Map<String, ConfigSource> sourceMap = getAttSourceMap();
+    boolean sourceIsSecured = false;
+    if (source != null && source.getType() == ConfigSource.Type.SECURE_FILE) {
+      sourceIsSecured = true;
+    }
+    for (String attName : validAttributeNames) {
+      if (source == null) {
+        if (sourceMap.get(attName) != null) {
+          continue;
+        }
+      } else if (!source.equals(sourceMap.get(attName))) {
+        continue;
+      }
+      if (!sourceFound) {
+        sourceFound = true;
+        if (source == null) {
+          pw.println(GEM_FIRE_PROPERTIES_USING_DEFAULT_VALUES);
+        } else {
+          pw.println(GEM_FIRE_PROPERTIES_DEFINED_WITH_PREFIX + source.getDescription()
+              + GEM_FIRE_PROPERTIES_DEFINED_WITH_SUFFIX);
+        }
+      }
+      // hide the shiro-init configuration for now. Remove after we can allow customer to specify
+      // shiro.ini file
+      if (attName.equals(SECURITY_SHIRO_INIT)) {
+        continue;
+      }
+
+      String attributeValueToPrint;
+      if (source == null) {
+        // always show defaults values
+        attributeValueToPrint = getAttribute(attName);
+      } else if (sourceIsSecured) {
+        // Never show secure sources
+        attributeValueToPrint = ArgumentRedactor.redacted;
+      } else {
+        // Otherwise, redact based on the key string
+        attributeValueToPrint =
+            ArgumentRedactor.redactValueIfNecessary(attName, getAttribute(attName));
+      }
+      pw.print(attName);
+      pw.print('=');
+      pw.println(attributeValueToPrint);
     }
-    return strings;
   }
 
   private SecurableCommunicationChannel[] commaDelimitedStringToSecurableCommunicationChannels(
@@ -459,18 +417,18 @@ public abstract class AbstractConfig implements Config {
   }
 
   /**
-   * Removes the last character of the input string and returns the trimmed name
+   * This class was added to fix bug 39382. It does this be overriding "keys" which is used by the
+   * store0 implementation of Properties.
    */
-  protected static String trimAttributeName(String attName) {
-    return attName.substring(0, attName.length() - 1);
-  }
+  protected static class SortedProperties extends Properties {
 
-  public String getAttributeDescription(String attName) {
-    checkAttributeName(attName);
-    if (!getAttDescMap().containsKey(attName)) {
-      throw new InternalGemFireException(
-          LocalizedStrings.AbstractConfig_UNHANDLED_ATTRIBUTE_NAME_0.toLocalizedString(attName));
+    private static final long serialVersionUID = 7156507110684631135L;
+
+    @Override
+    public Enumeration keys() {
+      // the TreeSet gets the sorting we desire but is only safe
+      // because the keys in this context are always String which is Comparable
+      return Collections.enumeration(new TreeSet(keySet()));
     }
-    return (String) getAttDescMap().get(attName);
   }
 }
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 f5b0576..a012504 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
@@ -19,15 +19,21 @@ import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import org.apache.geode.distributed.ConfigurationProperties;
+import org.apache.geode.distributed.internal.DistributionConfig;
+
 public class ArgumentRedactor {
-  private static final String redacted = "********";
+  public static final String redacted = "********";
 
   // All taboo words should be entirely lowercase.
-  private static final List<String> tabooWords = ArrayUtils.asList("password");
+  private static final List<String> tabooToContain = ArrayUtils.asList("password");
+  private static final List<String> tabooForKeyToStartWith =
+      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) one or two hyphens
+  // (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:
@@ -37,7 +43,7 @@ public class ArgumentRedactor {
   // 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+))");
+      Pattern.compile("([^ ]--?[^\\s=]+)(?=[ =])( *=? *)(?!-)((?:\"[^\"]*\"|\\S+))");
 
   private ArgumentRedactor() {}
 
@@ -76,7 +82,7 @@ public class ArgumentRedactor {
     Matcher matcher = optionWithValuePattern.matcher(line);
     while (matcher.find()) {
       String option = matcher.group(1);
-      if (!containsTabooWord(option)) {
+      if (!isTaboo(option)) {
         continue;
       }
 
@@ -117,7 +123,7 @@ public class ArgumentRedactor {
    *         unchanged.
    */
   public static String redactValueIfNecessary(String key, String value) {
-    if (containsTabooWord(key)) {
+    if (isTaboo(key)) {
       return redacted;
     }
     return value;
@@ -131,11 +137,16 @@ public class ArgumentRedactor {
    *
    * @return true if the value should be redacted, otherwise false.
    */
-  private static boolean containsTabooWord(String key) {
+  static boolean isTaboo(String key) {
     if (key == null) {
       return false;
     }
-    for (String taboo : tabooWords) {
+    for (String taboo : tabooForKeyToStartWith) {
+      if (key.toLowerCase().startsWith(taboo)) {
+        return true;
+      }
+    }
+    for (String taboo : tabooToContain) {
       if (key.toLowerCase().contains(taboo)) {
         return true;
       }
diff --git a/geode-core/src/test/java/org/apache/geode/distributed/internal/InternalDistributedSystemJUnitTest.java b/geode-core/src/test/java/org/apache/geode/distributed/internal/InternalDistributedSystemJUnitTest.java
index ab74965..6ab7d49 100644
--- a/geode-core/src/test/java/org/apache/geode/distributed/internal/InternalDistributedSystemJUnitTest.java
+++ b/geode-core/src/test/java/org/apache/geode/distributed/internal/InternalDistributedSystemJUnitTest.java
@@ -14,8 +14,32 @@
  */
 package org.apache.geode.distributed.internal;
 
-import static org.apache.geode.distributed.ConfigurationProperties.*;
-import static org.junit.Assert.*;
+import static org.apache.geode.distributed.ConfigurationProperties.ARCHIVE_DISK_SPACE_LIMIT;
+import static org.apache.geode.distributed.ConfigurationProperties.ARCHIVE_FILE_SIZE_LIMIT;
+import static org.apache.geode.distributed.ConfigurationProperties.CACHE_XML_FILE;
+import static org.apache.geode.distributed.ConfigurationProperties.CLUSTER_SSL_ENABLED;
+import static org.apache.geode.distributed.ConfigurationProperties.GATEWAY_SSL_ENABLED;
+import static org.apache.geode.distributed.ConfigurationProperties.GROUPS;
+import static org.apache.geode.distributed.ConfigurationProperties.HTTP_SERVICE_SSL_ENABLED;
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_SSL_ENABLED;
+import static org.apache.geode.distributed.ConfigurationProperties.LOCATORS;
+import static org.apache.geode.distributed.ConfigurationProperties.LOG_DISK_SPACE_LIMIT;
+import static org.apache.geode.distributed.ConfigurationProperties.LOG_FILE_SIZE_LIMIT;
+import static org.apache.geode.distributed.ConfigurationProperties.LOG_LEVEL;
+import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.distributed.ConfigurationProperties.MEMBERSHIP_PORT_RANGE;
+import static org.apache.geode.distributed.ConfigurationProperties.MEMBER_TIMEOUT;
+import static org.apache.geode.distributed.ConfigurationProperties.NAME;
+import static org.apache.geode.distributed.ConfigurationProperties.SERVER_SSL_ENABLED;
+import static org.apache.geode.distributed.ConfigurationProperties.SSL_ENABLED_COMPONENTS;
+import static org.apache.geode.distributed.ConfigurationProperties.START_LOCATOR;
+import static org.apache.geode.distributed.ConfigurationProperties.STATISTIC_ARCHIVE_FILE;
+import static org.apache.geode.distributed.ConfigurationProperties.STATISTIC_SAMPLE_RATE;
+import static org.apache.geode.distributed.ConfigurationProperties.STATISTIC_SAMPLING_ENABLED;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
 
 import java.io.File;
 import java.io.FileWriter;
@@ -640,16 +664,11 @@ public class InternalDistributedSystemJUnitTest {
 
   @Test
   public void testDeprecatedSSLProps() {
+    // ssl-* props are copied to cluster-ssl-*.
     Properties props = getCommonProperties();
     props.setProperty(CLUSTER_SSL_ENABLED, "true");
     Config config1 = new DistributionConfigImpl(props, false);
     Properties props1 = config1.toProperties();
-    // For the deprecated ssl-* properties a decision was made
-    // to not include them in the result of "toProperties".
-    // The cause of this is: org.apache.geode.internal.AbstractConfig.isDeprecated(String)
-    // and its use in toProperties.
-    // The other thing that is done is the ssl-* props are copied to cluster-ssl-*.
-    // The following two assertions demonstrate this.
     assertEquals("true", props1.getProperty(CLUSTER_SSL_ENABLED));
     Config config2 = new DistributionConfigImpl(props1, false);
     assertEquals(true, config1.sameAs(config2));
diff --git a/geode-core/src/test/java/org/apache/geode/internal/AbstractConfigJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/AbstractConfigJUnitTest.java
deleted file mode 100644
index ca20d45..0000000
--- a/geode-core/src/test/java/org/apache/geode/internal/AbstractConfigJUnitTest.java
+++ /dev/null
@@ -1,91 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
- * agreements. See the NOTICE file distributed with this work for additional information regarding
- * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance with the License. You may obtain a
- * copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software distributed under the License
- * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
- * or implied. See the License for the specific language governing permissions and limitations under
- * the License.
- */
-package org.apache.geode.internal;
-
-import static org.apache.geode.distributed.ConfigurationProperties.*;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-import java.lang.reflect.Method;
-import java.util.Map;
-
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-
-import org.apache.geode.test.junit.categories.UnitTest;
-
-@Category(UnitTest.class)
-public class AbstractConfigJUnitTest {
-
-  @Test
-  public void testDisplayPropertyValue() throws Exception {
-    AbstractConfigTestClass actc = new AbstractConfigTestClass();
-    Method method =
-        actc.getClass().getSuperclass().getDeclaredMethod("okToDisplayPropertyValue", String.class);
-    method.setAccessible(true);
-    assertFalse((Boolean) method.invoke(actc, "password"));
-    assertFalse((Boolean) method.invoke(actc, CLUSTER_SSL_TRUSTSTORE_PASSWORD));
-    assertTrue((Boolean) method.invoke(actc, CLUSTER_SSL_ENABLED));
-    assertFalse((Boolean) method.invoke(actc, GATEWAY_SSL_TRUSTSTORE_PASSWORD));
-    assertFalse((Boolean) method.invoke(actc, SERVER_SSL_KEYSTORE_PASSWORD));
-    assertTrue((Boolean) method.invoke(actc, CONSERVE_SOCKETS));
-    assertFalse((Boolean) method.invoke(actc, "javax.net.ssl.keyStorePassword"));
-    assertFalse((Boolean) method.invoke(actc, "javax.net.ssl.keyStoreType"));
-    assertFalse((Boolean) method.invoke(actc, "sysprop-value"));
-  }
-
-  private static class AbstractConfigTestClass extends AbstractConfig {
-
-    @Override
-    protected Map getAttDescMap() {
-      return null;
-    }
-
-    @Override
-    protected Map<String, ConfigSource> getAttSourceMap() {
-      return null;
-    }
-
-    @Override
-    public Object getAttributeObject(String attName) {
-      return null;
-    }
-
-    @Override
-    public void setAttributeObject(String attName, Object attValue, ConfigSource source) {
-
-    }
-
-    @Override
-    public boolean isAttributeModifiable(String attName) {
-      return false;
-    }
-
-    @Override
-    public Class getAttributeType(String attName) {
-      return null;
-    }
-
-    @Override
-    public String[] getAttributeNames() {
-      return new String[0];
-    }
-
-    @Override
-    public String[] getSpecificAttributeNames() {
-      return new String[0];
-    }
-  }
-}
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 9027a6d..3ba8f87 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
@@ -15,20 +15,24 @@
 
 package org.apache.geode.internal.util;
 
+import static org.apache.geode.distributed.ConfigurationProperties.CLUSTER_SSL_ENABLED;
+import static org.apache.geode.distributed.ConfigurationProperties.CLUSTER_SSL_TRUSTSTORE_PASSWORD;
+import static org.apache.geode.distributed.ConfigurationProperties.CONSERVE_SOCKETS;
+import static org.apache.geode.distributed.ConfigurationProperties.GATEWAY_SSL_TRUSTSTORE_PASSWORD;
+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.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
-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;
 
 /**
@@ -42,63 +46,108 @@ public class ArgumentRedactorJUnitTest {
       "redactorTest.aPassword-withCharactersAfterward";
 
   @Test
-  public void testRedactArgList() throws Exception {
+  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();
+    }
+  }
+
+  @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();
+    }
+  }
+
+  @Test
+  public void argListOfPasswordsAllRedact() {
     List<String> argList = new ArrayList<>();
-    argList.add("gemfire.security-password=secret");
-    argList.add("gemfire.security-properties=./security.properties");
-    argList.add("gemfire.sys.security-value=someValue");
-    argList.add("gemfire.use-cluster-configuration=true");
-    argList.add("someotherstringvalue");
-    argList.add("login-password=secret");
-    argList.add("login-name=admin");
-    argList.add("gemfire-password = super-secret");
-    argList.add("geode-password= confidential");
-    argList.add("some-other-password =shhhh");
+    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");
     String redacted = redact(argList);
-    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 =********");
+    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 testRedactArg() throws Exception {
-    String arg = "-Dgemfire.security-password=secret";
-    assertTrue(redact(arg).endsWith("password=********"));
+  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.use-cluster-configuration=true");
+    argList.add("--someotherstringvalue");
+    argList.add("--login-name=admin");
+    String redacted = redact(argList);
+    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-name=admin");
+  }
+
+  @Test
+  public void protectedIndividualOptionsRedact() {
+    String arg;
+
+    arg = "-Dgemfire.security-password=secret";
+    assertThat(redact(arg)).endsWith("password=********");
+
+    arg = "--password=foo";
+    assertThat(redact(arg)).isEqualToIgnoringWhitespace("--password=********");
+
+    arg = "-Dgemfire.security-properties=\"c:\\Program Files (x86)\\My Folder\"";
+    assertThat(redact(arg)).isEqualTo(arg);
+  }
+
+  @Test
+  public void miscIndividualOptionsDoNotRedact() {
+    String arg;
 
     arg = "-Dgemfire.security-properties=./security-properties";
-    assertEquals(arg, (redact(arg)));
+    assertThat(redact(arg)).isEqualTo(arg);
 
     arg = "-J-Dgemfire.sys.security-value=someValue";
-    assertEquals(arg, (redact(arg)));
+    assertThat(redact(arg)).isEqualTo(arg);
 
     arg = "-Dgemfire.sys.value=printable";
-    assertEquals(arg, redact(arg));
+    assertThat(redact(arg)).isEqualTo(arg);
 
     arg = "-Dgemfire.use-cluster-configuration=true";
-    assertEquals(arg, redact(arg));
+    assertThat(redact(arg)).isEqualTo(arg);
 
     arg = "someotherstringvalue";
-    assertEquals(arg, redact(arg));
-
-    arg = "--password=foo";
-    assertEquals("--password=********", redact(arg));
+    assertThat(redact(arg)).isEqualTo(arg);
 
     arg = "--classpath=.";
-    assertEquals(arg, redact(arg));
+    assertThat(redact(arg)).isEqualTo(arg);
+  }
 
+  @Test
+  public void wholeLinesAreProperlyRedacted() {
+    String arg;
     arg = "-DmyArg -Duser-password=foo --classpath=.";
-    assertEquals("-DmyArg -Duser-password=******** --classpath=.", redact(arg));
+    assertThat(redact(arg)).isEqualTo("-DmyArg -Duser-password=******** --classpath=.");
 
     arg = "-DmyArg -Duser-password=foo -DOtherArg -Dsystem-password=bar";
-    assertEquals("-DmyArg -Duser-password=******** -DOtherArg -Dsystem-password=********",
-        redact(arg));
+    assertThat(redact(arg))
+        .isEqualTo("-DmyArg -Duser-password=******** -DOtherArg -Dsystem-password=********");
 
     arg =
         "-Dlogin-password=secret -Dlogin-name=admin -Dgemfire-password = super-secret --geode-password= confidential -J-Dsome-other-password =shhhh";
@@ -108,13 +157,10 @@ public class ArgumentRedactorJUnitTest {
     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)));
   }
 
   @Test
-  public void redactScriptLine() throws Exception {
+  public void redactScriptLine() {
     assertThat(redact("connect --password=test --user=test"))
         .isEqualTo("connect --password=******** --user=test");
 
@@ -123,7 +169,7 @@ public class ArgumentRedactorJUnitTest {
   }
 
   @Test
-  public void systemPropertiesGetRedactedInBanner() throws Exception {
+  public void systemPropertiesGetRedactedInBanner() {
     try {
       System.setProperty(someProperty, "isNotRedacted");
       System.setProperty(somePasswordProperty, "isRedacted");
diff --git a/geode-core/src/test/java/org/apache/geode/test/junit/rules/GfshCommandRule.java b/geode-core/src/test/java/org/apache/geode/test/junit/rules/GfshCommandRule.java
index 6ab7908..bc12c14 100644
--- a/geode-core/src/test/java/org/apache/geode/test/junit/rules/GfshCommandRule.java
+++ b/geode-core/src/test/java/org/apache/geode/test/junit/rules/GfshCommandRule.java
@@ -67,7 +67,8 @@ import org.apache.geode.test.junit.assertions.CommandResultAssert;
  * </pre>
  *
  * When using as a ClassRule, if you call connect in a test, you will need to call disconnect after
- * the test as well. See NetstatDUnitTest for example.
+ * the test as well. See {@link org.apache.geode.management.internal.cli.NetstatDUnitTest} for
+ * example.
  */
 public class GfshCommandRule extends DescribedExternalResource {
 

-- 
To stop receiving notification emails like this one, please contact
prhomberg@apache.org.

Mime
View raw message