diff --git a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/pom.xml b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/pom.xml index 0d183fd7bb4..03d21fc75f8 100755 --- a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/pom.xml +++ b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/pom.xml @@ -84,6 +84,11 @@ junit-platform-runner test + + ch.qos.logback + logback-classic + test + diff --git a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java index af4bff9d218..89d4a311347 100644 --- a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java +++ b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java @@ -18,6 +18,7 @@ package org.apache.zookeeper.metrics.prometheus; +import static org.apache.zookeeper.common.LogRedactor.redactSensitiveValues; import io.prometheus.client.Collector; import io.prometheus.client.CollectorRegistry; import io.prometheus.client.exporter.MetricsServlet; @@ -113,7 +114,7 @@ public class PrometheusMetricsProvider implements MetricsProvider { @Override public void configure(Properties configuration) throws MetricsProviderLifeCycleException { - LOG.info("Initializing metrics, configuration: {}", configuration); + LOG.info("Initializing metrics, configuration: {}", redactSensitiveValues(configuration)); this.host = configuration.getProperty("httpHost", "0.0.0.0"); this.port = Integer.parseInt(configuration.getProperty("httpPort", "7000")); this.exportJvmInfo = Boolean.parseBoolean(configuration.getProperty("exportJvmInfo", "true")); diff --git a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java index 54c320d69c7..e6446633fc5 100644 --- a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java +++ b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java @@ -18,11 +18,17 @@ package org.apache.zookeeper.metrics.prometheus; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import io.prometheus.client.CollectorRegistry; import java.util.Properties; import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException; import org.junit.Assert; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import org.junit.Test; +import org.slf4j.LoggerFactory; public class PrometheusMetricsProviderConfigTest { @@ -63,4 +69,36 @@ public void testValidConfig() throws MetricsProviderLifeCycleException { provider.start(); } + @Test + public void testLogRedactorRedactsPasswords() throws Exception { + Logger logger = (Logger) LoggerFactory.getLogger(PrometheusMetricsProvider.class); + ListAppender listAppender = new ListAppender<>(); + listAppender.start(); + logger.addAppender(listAppender); + + try { + PrometheusMetricsProvider provider = new PrometheusMetricsProvider(); + Properties configuration = new Properties(); + configuration.setProperty("httpHost", "0.0.0.0"); + configuration.setProperty("httpPort", "65536"); + configuration.setProperty("some.password", "SuperSecret123!"); + configuration.setProperty("some-other.password", "AnotherSecret456!"); + provider.configure(configuration); + + String logOutput = listAppender.list.stream() + .map(ILoggingEvent::getFormattedMessage) + .filter(msg -> msg.contains("configuration")) + .findFirst() + .orElse(""); + + assertFalse(logOutput.contains("SuperSecret123!"), + "Logs should not contain some password"); + assertFalse(logOutput.contains("AnotherSecret456!"), + "Logs should not contain other password"); + assertTrue(logOutput.contains("0.0.0.0"), + "Logs should still contain non-sensitive config like host"); + } finally { + logger.detachAppender(listAppender); + } + } } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/LogRedactor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/LogRedactor.java new file mode 100644 index 00000000000..7a9cf1a85d3 --- /dev/null +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/LogRedactor.java @@ -0,0 +1,62 @@ +/* + * 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.zookeeper.common; + +import java.util.Locale; +import java.util.Map; +import java.util.Properties; + +public class LogRedactor { + + private LogRedactor() {} + + /** + * Redacts all values which ends with "password" from the given properties / map. + * Other values are not changed. + * + * @param properties the properties in which all passwords should be redacted. + * @return new Properties object containing all values, passwords redacted. + */ + public static Properties redactSensitiveValues(Map properties) { + Properties redactedConfig = new Properties(); + properties.forEach((k, v) -> { + if (k != null && v != null) { + redactedConfig.put(k, redactValue((String) k, (String) v)); + } + }); + return redactedConfig; + } + + /** + * Returns redacted value when the key ends with "password". + * Otherwise, just returns the value. + * + * @param key the key to check if it ends with "password". + * @return redacted value when the key ends with "password". Otherwise, the value. + */ + public static String redactValue(String key, String value) { + if (key == null) { + return value; + } + if (key.toLowerCase(Locale.ROOT).endsWith("password")) { + return "***"; + } + return value; + } +} diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java index 4a3805d2602..efc4689a575 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java @@ -18,12 +18,13 @@ package org.apache.zookeeper.common; +import static org.apache.zookeeper.common.LogRedactor.redactSensitiveValues; +import static org.apache.zookeeper.common.LogRedactor.redactValue; import java.io.File; import java.io.FileInputStream; import java.io.IOException; import java.nio.file.Path; import java.util.HashMap; -import java.util.Locale; import java.util.Map; import java.util.Map.Entry; import java.util.Properties; @@ -94,11 +95,7 @@ public ZKConfig(String configPath) throws QuorumPeerConfig.ConfigException { public ZKConfig(File configFile) throws QuorumPeerConfig.ConfigException { this(); addConfiguration(configFile); - Map p = new HashMap<>(); - for (Entry entry : properties.entrySet()) { - p.put(entry.getKey(), logRedactor(entry.getKey(), entry.getValue())); - } - LOG.info("ZK Config {}", p); + LOG.info("ZK Config {}", redactSensitiveValues(properties)); } /** @@ -211,7 +208,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, logRedactor(key, oldValue), logRedactor(key, value)); + LOG.debug("key {}'s value {} is replaced with new value {}", key, redactValue(key, oldValue), redactValue(key, value)); } } @@ -334,14 +331,4 @@ 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; - } } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/common/LogRedactorTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/common/LogRedactorTest.java new file mode 100644 index 00000000000..a15696f0275 --- /dev/null +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/common/LogRedactorTest.java @@ -0,0 +1,100 @@ +/* + * 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.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.assertTrue; +import java.util.HashMap; +import java.util.Map; +import java.util.Properties; +import org.junit.jupiter.api.Test; + +class LogRedactorTest { + + @Test + void testRedactValueWithPasswordKey() { + assertEquals("***", LogRedactor.redactValue("ssl.keyStore.password", "secret")); + assertEquals("***", LogRedactor.redactValue("ssl.trustStore.password", "secret")); + assertEquals("***", LogRedactor.redactValue("somePassword", "secret")); + } + + @Test + void testRedactValueCaseInsensitive() { + assertEquals("***", LogRedactor.redactValue("ssl.keyStore.PASSWORD", "secret")); + assertEquals("***", LogRedactor.redactValue("ssl.keyStore.Password", "secret")); + assertEquals("***", LogRedactor.redactValue("ssl.keyStore.passWORD", "secret")); + } + + @Test + void testRedactValueNonSensitiveKey() { + assertEquals("localhost", LogRedactor.redactValue("server.host", "localhost")); + assertEquals("8080", LogRedactor.redactValue("httpPort", "8080")); + assertEquals("/path/to/keystore", LogRedactor.redactValue("ssl.keyStore.location", "/path/to/keystore")); + } + + @Test + void testRedactValueNullKey() { + assertEquals("someValue", LogRedactor.redactValue(null, "someValue")); + } + + @Test + void testRedactSensitiveValuesWithStringMap() { + Map config = new HashMap<>(); + config.put("ssl.keyStore.location", "/path/to/keystore.jks"); + config.put("ssl.keyStore.password", "SuperSecret"); + config.put("httpPort", "9141"); + + Properties redacted = LogRedactor.redactSensitiveValues(config); + + assertEquals("/path/to/keystore.jks", redacted.get("ssl.keyStore.location")); + assertEquals("***", redacted.get("ssl.keyStore.password")); + assertEquals("9141", redacted.get("httpPort")); + } + + @Test + void testRedactSensitiveValuesWithProperties() { + Properties config = new Properties(); + config.setProperty("ssl.trustStore.password", "TrustSecret"); + config.setProperty("ssl.trustStore.location", "/path/to/truststore.jks"); + + Properties redacted = LogRedactor.redactSensitiveValues(config); + + assertEquals("***", redacted.get("ssl.trustStore.password")); + assertEquals("/path/to/truststore.jks", redacted.get("ssl.trustStore.location")); + } + + @Test + void testRedactSensitiveValuesSkipsNullValues() { + Map config = new HashMap<>(); + config.put("ssl.keyStore.location", null); + config.put("httpPort", "9141"); + + Properties redacted = LogRedactor.redactSensitiveValues(config); + + assertFalse(redacted.containsKey("ssl.keyStore.location")); + assertEquals("9141", redacted.get("httpPort")); + } + + @Test + void testRedactSensitiveValuesEmptyMap() { + Properties redacted = LogRedactor.redactSensitiveValues(new HashMap<>()); + assertTrue(redacted.isEmpty()); + } +}