From commits-return-25587-archive-asf-public=cust-asf.ponee.io@geode.apache.org Thu Feb 8 19:29:26 2018 Return-Path: X-Original-To: archive-asf-public@eu.ponee.io Delivered-To: archive-asf-public@eu.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by mx-eu-01.ponee.io (Postfix) with ESMTP id C125918064F for ; Thu, 8 Feb 2018 19:29:26 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id B1061160C4A; Thu, 8 Feb 2018 18:29:26 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id D1A18160C34 for ; Thu, 8 Feb 2018 19:29:24 +0100 (CET) Received: (qmail 10406 invoked by uid 500); 8 Feb 2018 18:29:19 -0000 Mailing-List: contact commits-help@geode.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@geode.apache.org Delivered-To: mailing list commits@geode.apache.org Received: (qmail 10396 invoked by uid 99); 8 Feb 2018 18:29:19 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 08 Feb 2018 18:29:18 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id BE5B0823E3; Thu, 8 Feb 2018 18:29:17 +0000 (UTC) Date: Thu, 08 Feb 2018 18:29:17 +0000 To: "commits@geode.apache.org" Subject: [geode] branch develop updated: GEODE-4320: Reconcile disparity between Banner and AbstractConfig MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <151811455729.30787.7233366052175313515@gitbox.apache.org> From: prhomberg@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: geode X-Git-Refname: refs/heads/develop X-Git-Reftype: branch X-Git-Oldrev: ff215d4c8cb55f071f70553f4410df446aaec6d8 X-Git-Newrev: a39394e082029c0c2bfdd24bc77da8d2dc1afdf1 X-Git-Rev: a39394e082029c0c2bfdd24bc77da8d2dc1afdf1 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated 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 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 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 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 getConfigPropsFromSource(ConfigSource source) { - Map configProps = new HashMap(); + Map configProps = new HashMap<>(); String[] validAttributeNames = getAttributeNames(); Map 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 getConfigPropsDefinedUsingFiles() { - Map configProps = new HashMap(); + Map configProps = new HashMap<>(); for (ConfigSource fileSource : getFileSources()) { configProps.putAll(getConfigPropsFromSource(fileSource)); } return configProps; } - private List getFileSources() { - ArrayList 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 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 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 getFileSources() { + ArrayList 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 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 tabooWords = ArrayUtils.asList("password"); + private static final List tabooToContain = ArrayUtils.asList("password"); + private static final List 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 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 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 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 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 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; * * * 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.