From 0ee2851467c7896137f259b9b920882e0038fa21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Thu, 14 May 2026 17:27:31 +0200 Subject: [PATCH 1/3] ZOOKEEPER-5049: Redact passwords from PrometheusMetricsProvider configuration logging Before When PrometheusMetricsProvider was enabled and configured for HTTPS, on startup, PrometheusMetricsProvider logged all it's configs in clear text on INFO level. This included KeyStore and TrustStore passwords. Fix Use the same redaction logic as ZKConfig to avoid logging passwords. --- .../prometheus/PrometheusMetricsProvider.java | 23 +++++++++-- .../PrometheusHttpsMetricsProviderTest.java | 41 +++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) 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 85cacb686f9..bc2ac8f2153 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 @@ -25,6 +25,7 @@ import java.io.IOException; import java.security.GeneralSecurityException; import java.security.KeyStore; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Properties; @@ -136,7 +137,7 @@ protected void doTrace(HttpServletRequest req, HttpServletResponse resp) throws @Override public void configure(Properties configuration) throws MetricsProviderLifeCycleException { - LOG.info("Initializing Prometheus metrics with Jetty, configuration: {}", configuration); + LOG.info("Initializing Prometheus metrics with Jetty, configuration: {}", redactSensitiveValues(configuration)); this.host = configuration.getProperty(HTTP_HOST, "0.0.0.0"); this.httpPort = Integer.parseInt(configuration.getProperty(HTTP_PORT, "-1")); @@ -528,13 +529,13 @@ public SummarySet getSummarySet(String name, DetailLevel detailLevel) { return new PrometheusLabelledSummaryWrapper(prometheusSummary); }); } + } // --- Wrapper classes to adapt Prometheus metrics to ZooKeeper's metric interfaces --- - private static class PrometheusCounterWrapper implements Counter { - private final io.prometheus.metrics.core.metrics.Counter prometheusCounter; + private final io.prometheus.metrics.core.metrics.Counter prometheusCounter; public PrometheusCounterWrapper(io.prometheus.metrics.core.metrics.Counter prometheusCounter) { this.prometheusCounter = prometheusCounter; } @@ -602,4 +603,20 @@ public void add(String key, long value) { this.prometheusSummary.labelValues(key).observe(value); } } + + private static Properties redactSensitiveValues(Properties configuration) { + Properties redactedConfig = new Properties(); + configuration.forEach((k, v) -> redactedConfig.put(k, logRedactor((String) k, (String) v))); + return redactedConfig; + } + + private static 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-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusHttpsMetricsProviderTest.java b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusHttpsMetricsProviderTest.java index f730b4dbe00..53332e24f8d 100644 --- a/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusHttpsMetricsProviderTest.java +++ b/zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusHttpsMetricsProviderTest.java @@ -21,6 +21,11 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.MatcherAssert.assertThat; 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 ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import java.io.BufferedReader; import java.io.FileInputStream; import java.io.IOException; @@ -36,6 +41,7 @@ import org.apache.zookeeper.metrics.Counter; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; /** * Tests about Prometheus Metrics Provider. Please note that we are not testing Prometheus but only our integration. @@ -176,4 +182,39 @@ private void validateMetricResponse(String response) throws IOException { assertThat(response, containsString("# TYPE cc_total counter")); assertThat(response, containsString("cc_total 10.0")); } + + @Test + void testLogRedactorRedactsPasswords() throws Exception { + Logger logger = (Logger) LoggerFactory.getLogger(PrometheusMetricsProvider.class); + ListAppender listAppender = new ListAppender<>(); + listAppender.start(); + logger.addAppender(listAppender); + + try { + provider = new PrometheusMetricsProvider(); + Properties configuration = new Properties(); + configuration.setProperty("httpPort", String.valueOf(httpPort)); + configuration.setProperty("httpsPort", String.valueOf(httpsPort)); + configuration.setProperty("ssl.keyStore.location", testDataPath + "/ssl/server_keystore.jks"); + configuration.setProperty("ssl.keyStore.password", "SuperSecret123!"); + configuration.setProperty("ssl.trustStore.location", testDataPath + "/ssl/server_truststore.jks"); + configuration.setProperty("ssl.trustStore.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 keyStore password"); + assertFalse(logOutput.contains("AnotherSecret456!"), + "Logs should not contain trustStore password"); + assertTrue(logOutput.contains(testDataPath + "/ssl/server_keystore.jks"), + "Logs should still contain non-sensitive config like keyStore location"); + } finally { + logger.detachAppender(listAppender); + } + } } From c3580b8c1ca1bd39cb30f063517ca27318733f6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Mon, 18 May 2026 10:25:58 +0200 Subject: [PATCH 2/3] ZOOKEEPER-5049: Extract log redaction to shared utility from ZKConfig and PrometheusMetricsProvider --- .../prometheus/PrometheusMetricsProvider.java | 18 +--- .../apache/zookeeper/common/LogRedactor.java | 62 +++++++++++ .../org/apache/zookeeper/common/ZKConfig.java | 21 +--- .../zookeeper/common/LogRedactorTest.java | 100 ++++++++++++++++++ 4 files changed, 167 insertions(+), 34 deletions(-) create mode 100644 zookeeper-server/src/main/java/org/apache/zookeeper/common/LogRedactor.java create mode 100644 zookeeper-server/src/test/java/org/apache/zookeeper/common/LogRedactorTest.java 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 bc2ac8f2153..18640344738 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.metrics.core.metrics.GaugeWithCallback; import io.prometheus.metrics.exporter.servlet.javax.PrometheusMetricsServlet; import io.prometheus.metrics.instrumentation.jvm.JvmMetrics; @@ -25,7 +26,6 @@ import java.io.IOException; import java.security.GeneralSecurityException; import java.security.KeyStore; -import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Properties; @@ -603,20 +603,4 @@ public void add(String key, long value) { this.prometheusSummary.labelValues(key).observe(value); } } - - private static Properties redactSensitiveValues(Properties configuration) { - Properties redactedConfig = new Properties(); - configuration.forEach((k, v) -> redactedConfig.put(k, logRedactor((String) k, (String) v))); - return redactedConfig; - } - - private static 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/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 2c9e63199c5..442d8e7221d 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,6 +18,8 @@ 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; @@ -25,7 +27,6 @@ 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; @@ -105,11 +106,7 @@ public ZKConfig(File configFile) throws ConfigException { public ZKConfig(Path configPath) throws ConfigException { this(); addConfiguration(configPath); - 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)); } private void init() { @@ -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)); } } @@ -341,14 +338,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()); + } +} From e748e7871b460a7a490326fb6caa2973ebe3466b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Paksy?= Date: Mon, 18 May 2026 10:57:31 +0200 Subject: [PATCH 3/3] ZOOKEEPER-5049: Fix reformatting of PrometheusCounterWrapper --- .../metrics/prometheus/PrometheusMetricsProvider.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 18640344738..001b7f25dcd 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 @@ -529,13 +529,13 @@ public SummarySet getSummarySet(String name, DetailLevel detailLevel) { return new PrometheusLabelledSummaryWrapper(prometheusSummary); }); } - } // --- Wrapper classes to adapt Prometheus metrics to ZooKeeper's metric interfaces --- - private static class PrometheusCounterWrapper implements Counter { + private static class PrometheusCounterWrapper implements Counter { private final io.prometheus.metrics.core.metrics.Counter prometheusCounter; + public PrometheusCounterWrapper(io.prometheus.metrics.core.metrics.Counter prometheusCounter) { this.prometheusCounter = prometheusCounter; }