Add log redactor method when logging ZK config properties

Author: anmolnar

zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java

@@ -25,9 +25,11 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.HashMap;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Properties;
+
 import org.apache.zookeeper.Environment;
 import org.apache.zookeeper.server.util.VerifyingFileFactory;
 import org.slf4j.Logger;
@@ -104,7 +106,11 @@ public ZKConfig(File configFile) throws ConfigException {
     public ZKConfig(Path configPath) throws ConfigException {
         this();
         addConfiguration(configPath);
-        LOG.info("ZK Config {}", this.properties);
+        Map<String, String> p = new HashMap<>();
+        for (Entry<String, String> entry : properties.entrySet()) {
+            p.put(entry.getKey(), logRedactor(entry.getKey(), entry.getValue()));
+        }
+        LOG.info("ZK Config {}", p);
     }
 
     private void init() {
@@ -206,7 +212,7 @@ public void setProperty(String key, String value) {
         }
         String oldValue = properties.put(key, value);
         if (null != oldValue && !oldValue.equals(value)) {
-            LOG.debug("key {}'s value {} is replaced with new value {}", key, oldValue, value);
+            LOG.debug("key {}'s value {} is replaced with new value {}", key, logRedactor(key, oldValue), logRedactor(key, value));
         }
     }
 
@@ -337,4 +343,13 @@ public int getInt(String key, int defaultValue) {
         return defaultValue;
     }
 
+    private String logRedactor(String key, String value) {
+        if (key == null) {
+            return value;
+        }
+        if (key.toLowerCase(Locale.ROOT).endsWith("password")) {
+            return "***";
+        }
+        return value;
+    }
 }

zookeeper-server/src/test/java/org/apache/zookeeper/common/ZKConfigTest.java

@@ -19,13 +19,35 @@
 package org.apache.zookeeper.common;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.zookeeper.test.LoggerTestTool;
+import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.Timeout;
+import ch.qos.logback.classic.Level;
+
+import java.io.File;
+import java.io.IOException;
 
 public class ZKConfigTest {
 
-    X509Util x509Util = new ClientX509Util();
+    private final X509Util x509Util = new ClientX509Util();
+    private static LoggerTestTool loggerTestTool;
+
+    @BeforeAll
+    public static void setUpBeforeClass() {
+        loggerTestTool = new LoggerTestTool(ZKConfig.class, Level.DEBUG);
+    }
+
+    @AfterAll
+    public static void tearDownAfterClass() throws Exception {
+        loggerTestTool.close();
+    }
 
     @AfterEach
     public void tearDown() throws Exception {
@@ -90,4 +112,52 @@ public void testBooleanRetrievalFromPropertyWithWhitespacesAtBeginningAndEnd() {
         boolean result = conf.getBoolean(x509Util.getSslProtocolProperty(), defaultValue);
         assertEquals(value, result);
     }
+
+    @Test
+    public void testLogRedactorFromConfigFile() throws ConfigException, IOException {
+        // Arrange
+        File configFile = new File("./src/test/resources/zookeeper-client.config");
+
+        // Act
+        new ZKConfig(configFile.toPath());
+
+        // Assert
+        String logLine = loggerTestTool.readLogLine("ZK Config");
+        assertNotNull(logLine, "Unable to find ZK Config line in the logs");
+        assertFalse(logLine.contains("FileSecret456!"), "Logs should not contain any secrets");
+        assertFalse(logLine.contains("AnotherFileSecret789!"), "Logs should not contain any secrets");
+        assertTrue(logLine.contains("/home/zookeeper/top_secret.txt"));      // what we shouldn't redact
+    }
+
+    @Test
+    public void testLogRedactorInDebugLogs() throws IOException {
+        // Arrange
+        ZKConfig conf = new ZKConfig();
+
+        // Act
+        conf.setProperty("zookeeper.some.secret.password", "0ldP4ssw0rd");
+        conf.setProperty("zookeeper.some.secret.password", "N3Ws3cr3t");
+
+        // Assert
+        String logLine = loggerTestTool.readLogLine("replaced with new value");
+        assertNotNull(logLine, "Unable to find relevant line in the logs");
+        assertFalse(logLine.contains("0ldP4ssw0rd"), "Logs should not contain any secrets");
+        assertFalse(logLine.contains("N3Ws3cr3t"), "Logs should not contain any secrets");
+    }
+
+    @Test
+    public void testDontRedactorInDebugLogs() throws IOException {
+        // Arrange
+        ZKConfig conf = new ZKConfig();
+
+        // Act
+        conf.setProperty("zookeeper.some.secret.passwordPath", "/home/zookeeper/old_secret.txt");  // what we shouldn't redact
+        conf.setProperty("zookeeper.some.secret.passwordPath", "/home/zookeeper/new_secret.txt");  // what we shouldn't redact
+
+        // Assert
+        String logLine = loggerTestTool.readLogLine("replaced with new value");
+        assertNotNull(logLine, "Unable to find relevant line in the logs");
+        assertTrue(logLine.contains("/home/zookeeper/new_secret.txt"));
+    }
+
 }

zookeeper-server/src/test/java/org/apache/zookeeper/test/LoggerTestTool.java

@@ -27,17 +27,27 @@
 import ch.qos.logback.core.OutputStreamAppender;
 import ch.qos.logback.core.encoder.LayoutWrappingEncoder;
 import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.LineNumberReader;
+import java.io.StringReader;
+
 import org.slf4j.LoggerFactory;
 
 public class LoggerTestTool implements AutoCloseable {
   private final ByteArrayOutputStream os;
   private Appender<ILoggingEvent> appender;
   private Logger qlogger;
+  private Level logLevel = Level.INFO;
 
   public LoggerTestTool(Class<?> cls) {
     os = createLoggingStream(cls);
   }
 
+  public LoggerTestTool(Class<?> cls, Level logLevel) {
+    this.logLevel = logLevel;
+    this.os = createLoggingStream(cls);
+  }
+
   public LoggerTestTool(String cls) {
     os = createLoggingStream(cls);
   }
@@ -51,7 +61,7 @@ private ByteArrayOutputStream createLoggingStream(Class<?> cls) {
     appender = getConsoleAppender(os);
     qlogger = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(cls);
     qlogger.addAppender(appender);
-    qlogger.setLevel(Level.INFO);
+    qlogger.setLevel(logLevel);
     appender.start();
     return os;
   }
@@ -61,14 +71,15 @@ private ByteArrayOutputStream createLoggingStream(String cls) {
     appender = getConsoleAppender(os);
     qlogger = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(cls);
     qlogger.addAppender(appender);
-    qlogger.setLevel(Level.INFO);
+    qlogger.setLevel(logLevel);
     appender.start();
     return os;
   }
 
   private OutputStreamAppender<ILoggingEvent> getConsoleAppender(ByteArrayOutputStream os) {
     Logger rootLogger =
         (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(org.slf4j.Logger.ROOT_LOGGER_NAME);
+    rootLogger.setLevel(logLevel);
     Layout<ILoggingEvent> layout = ((LayoutWrappingEncoder<ILoggingEvent>)
         ((OutputStreamAppender<ILoggingEvent>) rootLogger.getAppender("CONSOLE")).getEncoder()).getLayout();
 
@@ -80,6 +91,21 @@ private OutputStreamAppender<ILoggingEvent> getConsoleAppender(ByteArrayOutputSt
     return appender;
   }
 
+  public String readLogLine(String search) throws IOException {
+    try {
+      LineNumberReader r = new LineNumberReader(new StringReader(os.toString()));
+      String line;
+      while ((line = r.readLine()) != null) {
+        if (line.contains(search)) {
+          return line;
+        }
+      }
+      return null;
+    } finally {
+      os.reset();
+    }
+  }
+
   @Override
   public void close() throws Exception {
     qlogger.detachAppender(appender);

zookeeper-server/src/test/resources/zookeeper-client.config

@@ -0,0 +1,5 @@
+# Sample secrets
+zookeeper.ssl.keyStore.password=FileSecret456!
+zookeeper.ssl.keyStore.passwordPath=/home/zookeeper/top_secret.txt
+ssl.quorum.keyStore.password=AnotherFileSecret789!
+