BROOKLYN-405: ssh doesn't log password environment variables
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/171843ec Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/171843ec Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/171843ec Branch: refs/heads/master Commit: 171843ecef4b5432c91840de15399bf7aa42fed9 Parents: e5b40cc Author: Aled Sage <[email protected]> Authored: Thu Dec 1 08:50:22 2016 +0000 Committer: Aled Sage <[email protected]> Committed: Thu Dec 1 15:38:53 2016 +0000 ---------------------------------------------------------------------- .../location/ssh/SshMachineLocation.java | 1 - .../system/internal/ExecWithLoggingHelpers.java | 3 +- .../ssh/SshMachineLocationIntegrationTest.java | 7 ++++ .../location/ssh/SshMachineLocationTest.java | 34 ++++++++++++++++++ .../org/apache/brooklyn/test/LogWatcher.java | 36 ++++++++++++++------ 5 files changed, 69 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/171843ec/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java b/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java index 40ce7a6..b611c3c 100644 --- a/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java +++ b/core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java @@ -30,7 +30,6 @@ import java.io.OutputStream; import java.io.PipedInputStream; import java.io.PipedOutputStream; import java.io.Reader; -import java.io.StringReader; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.Socket; http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/171843ec/core/src/main/java/org/apache/brooklyn/util/core/task/system/internal/ExecWithLoggingHelpers.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/system/internal/ExecWithLoggingHelpers.java b/core/src/main/java/org/apache/brooklyn/util/core/task/system/internal/ExecWithLoggingHelpers.java index e96dce9..790dc18 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/system/internal/ExecWithLoggingHelpers.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/system/internal/ExecWithLoggingHelpers.java @@ -27,6 +27,7 @@ import java.util.Map; import org.slf4j.Logger; import org.apache.brooklyn.config.ConfigKey; +import org.apache.brooklyn.core.config.Sanitizer; import org.apache.brooklyn.location.ssh.SshMachineLocation; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.config.ConfigBag; @@ -112,7 +113,7 @@ public abstract class ExecWithLoggingHelpers { String allcmds = (Strings.isBlank(expectedCommandHeaders) ? "" : expectedCommandHeaders + " ; ") + Strings.join(commands, " ; "); commandLogger.debug("{}, initiating "+shortName.toLowerCase()+" on machine {}{}: {}", new Object[] {summaryForLogging, getTargetName(), - env!=null && !env.isEmpty() ? " (env "+env+")": "", allcmds}); + env!=null && !env.isEmpty() ? " (env "+Sanitizer.sanitize(env)+")": "", allcmds}); } if (commands.isEmpty()) { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/171843ec/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationIntegrationTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationIntegrationTest.java b/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationIntegrationTest.java index 1def66f..081d0a8 100644 --- a/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationIntegrationTest.java +++ b/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationIntegrationTest.java @@ -103,6 +103,13 @@ public class SshMachineLocationIntegrationTest extends SshMachineLocationTest { super.testIsSshableWhenTrue(); } + // Overridden just to make it integration (because `newHost()` returns a real ssh'ing host) + @Test(groups="Integration") + @Override + public void testDoesNotLogPasswordsInEnvironmentVariables() { + super.testDoesNotLogPasswordsInEnvironmentVariables(); + } + // Overrides super, because expect real machine details (rather than asserting our stub data) @Test(groups = "Integration") @Override http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/171843ec/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationTest.java b/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationTest.java index c09d790..3b6239c 100644 --- a/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationTest.java +++ b/core/src/test/java/org/apache/brooklyn/location/ssh/SshMachineLocationTest.java @@ -40,6 +40,7 @@ import org.apache.brooklyn.api.location.LocationSpec; import org.apache.brooklyn.api.location.MachineDetails; import org.apache.brooklyn.api.location.MachineLocation; import org.apache.brooklyn.api.location.PortRange; +import org.apache.brooklyn.core.BrooklynLogging; import org.apache.brooklyn.core.effector.EffectorBody; import org.apache.brooklyn.core.effector.EffectorTaskTest; import org.apache.brooklyn.core.effector.Effectors; @@ -53,10 +54,13 @@ import org.apache.brooklyn.core.location.Machines; import org.apache.brooklyn.core.location.PortRanges; import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport; import org.apache.brooklyn.core.test.entity.TestApplication; +import org.apache.brooklyn.test.LogWatcher; +import org.apache.brooklyn.test.LogWatcher.EventPredicates; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.config.ConfigBag; import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool; import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool.CustomResponse; +import org.apache.brooklyn.util.core.internal.ssh.sshj.SshjTool; import org.apache.brooklyn.util.core.task.BasicExecutionContext; import org.apache.brooklyn.util.core.task.BasicExecutionManager; import org.apache.brooklyn.util.guava.Maybe; @@ -70,9 +74,15 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import com.google.common.base.Joiner; +import com.google.common.base.Optional; +import com.google.common.base.Predicate; +import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; + +import ch.qos.logback.classic.spi.ILoggingEvent; /** * Test the {@link SshMachineLocation} implementation of the {@link Location} interface. @@ -273,4 +283,28 @@ public class SshMachineLocationTest extends BrooklynAppUnitTestSupport { assertEquals(host.obtainPort(PortRanges.fromString("8000")), -1); assertEquals(host.obtainPort(PortRanges.fromString("8000+")), 8001); } + + @Test + public void testDoesNotLogPasswordsInEnvironmentVariables() { + List<String> loggerNames = ImmutableList.of( + SshMachineLocation.class.getName(), + BrooklynLogging.SSH_IO, + SshjTool.class.getName()); + ch.qos.logback.classic.Level logLevel = ch.qos.logback.classic.Level.DEBUG; + Predicate<ILoggingEvent> filter = Predicates.or( + EventPredicates.containsMessage("DB_PASSWORD"), + EventPredicates.containsMessage("mypassword")); + LogWatcher watcher = new LogWatcher(loggerNames, logLevel, filter); + + watcher.start(); + try { + host.execCommands("mySummary", ImmutableList.of("true"), ImmutableMap.of("DB_PASSWORD", "mypassword")); + watcher.assertHasEventEventually(); + + Optional<ILoggingEvent> eventWithPasswd = Iterables.tryFind(watcher.getEvents(), EventPredicates.containsMessage("mypassword")); + assertFalse(eventWithPasswd.isPresent(), "event="+eventWithPasswd); + } finally { + watcher.close(); + } + } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/171843ec/test-support/src/main/java/org/apache/brooklyn/test/LogWatcher.java ---------------------------------------------------------------------- diff --git a/test-support/src/main/java/org/apache/brooklyn/test/LogWatcher.java b/test-support/src/main/java/org/apache/brooklyn/test/LogWatcher.java index 6baa852..6495db3 100644 --- a/test-support/src/main/java/org/apache/brooklyn/test/LogWatcher.java +++ b/test-support/src/main/java/org/apache/brooklyn/test/LogWatcher.java @@ -25,6 +25,7 @@ import static org.testng.Assert.assertFalse; import java.io.Closeable; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import org.mockito.Mockito; @@ -37,6 +38,7 @@ import com.google.common.annotations.Beta; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import ch.qos.logback.classic.Level; import ch.qos.logback.classic.spi.ILoggingEvent; @@ -88,13 +90,20 @@ public class LogWatcher implements Closeable { private final List<ILoggingEvent> events = Collections.synchronizedList(Lists.<ILoggingEvent>newLinkedList()); private final AtomicBoolean closed = new AtomicBoolean(); private final ch.qos.logback.classic.Level loggerLevel; - private final ch.qos.logback.classic.Logger watchedLogger; private final Appender<ILoggingEvent> appender; - private volatile Level origLevel; + private final List<ch.qos.logback.classic.Logger> watchedLoggers = Lists.newArrayList(); + private volatile Map<ch.qos.logback.classic.Logger, Level> origLevels = Maps.newLinkedHashMap(); + + public LogWatcher(String loggerName, ch.qos.logback.classic.Level loggerLevel, final Predicate<? super ILoggingEvent> filter) { + this(ImmutableList.of(checkNotNull(loggerName, "loggerName")), loggerLevel, filter); + } @SuppressWarnings("unchecked") - public LogWatcher(String loggerName, ch.qos.logback.classic.Level loggerLevel, final Predicate<? super ILoggingEvent> filter) { - this.watchedLogger = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(checkNotNull(loggerName, "loggerName")); + public LogWatcher(Iterable<String> loggerNames, ch.qos.logback.classic.Level loggerLevel, final Predicate<? super ILoggingEvent> filter) { + for (String loggerName : loggerNames) { + Logger logger = LoggerFactory.getLogger(checkNotNull(loggerName, "loggerName")); + watchedLoggers.add((ch.qos.logback.classic.Logger) logger); + } this.loggerLevel = checkNotNull(loggerLevel, "loggerLevel"); this.appender = Mockito.mock(Appender.class); @@ -115,18 +124,25 @@ public class LogWatcher implements Closeable { public void start() { checkState(!closed.get(), "Cannot start LogWatcher after closed"); - origLevel = watchedLogger.getLevel(); - watchedLogger.setLevel(loggerLevel); - watchedLogger.addAppender(appender); + for (ch.qos.logback.classic.Logger watchedLogger : watchedLoggers) { + origLevels.put(watchedLogger, watchedLogger.getLevel()); + watchedLogger.setLevel(loggerLevel); + watchedLogger.addAppender(appender); + } } @Override public void close() { if (closed.compareAndSet(false, true)) { - if (watchedLogger != null) { - if (origLevel != null) watchedLogger.setLevel(origLevel); - watchedLogger.detachAppender(appender); + if (watchedLoggers != null) { + for (ch.qos.logback.classic.Logger watchedLogger : watchedLoggers) { + Level origLevel = origLevels.get(watchedLogger); + if (origLevel != null) watchedLogger.setLevel(origLevel); + watchedLogger.detachAppender(appender); + } } + watchedLoggers.clear(); + origLevels.clear(); } }
