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 <aled.s...@gmail.com>
Authored: Thu Dec 1 08:50:22 2016 +0000
Committer: Aled Sage <aled.s...@gmail.com>
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();
         }
     }
     

Reply via email to