Skip to content

Commit 34a9492

Browse files
PDavidPDavid
authored
ZOOKEEPER-5049: Redact passwords from PrometheusMetricsProvider configuration logging
Reviewers: anmolnar, meszibalu Author: PDavid Closes #2387 from PDavid/ZOOKEEPER-5049-PrometheusMetricsProvider-log-redact
1 parent f94cdd5 commit 34a9492

5 files changed

Lines changed: 209 additions & 18 deletions

File tree

‎zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
package org.apache.zookeeper.metrics.prometheus;
2020

21+
import static org.apache.zookeeper.common.LogRedactor.redactSensitiveValues;
2122
import io.prometheus.metrics.core.metrics.GaugeWithCallback;
2223
import io.prometheus.metrics.exporter.servlet.javax.PrometheusMetricsServlet;
2324
import io.prometheus.metrics.instrumentation.jvm.JvmMetrics;
@@ -136,7 +137,7 @@ protected void doTrace(HttpServletRequest req, HttpServletResponse resp) throws
136137

137138
@Override
138139
public void configure(Properties configuration) throws MetricsProviderLifeCycleException {
139-
LOG.info("Initializing Prometheus metrics with Jetty, configuration: {}", configuration);
140+
LOG.info("Initializing Prometheus metrics with Jetty, configuration: {}", redactSensitiveValues(configuration));
140141

141142
this.host = configuration.getProperty(HTTP_HOST, "0.0.0.0");
142143
this.httpPort = Integer.parseInt(configuration.getProperty(HTTP_PORT, "-1"));

‎zookeeper-metrics-providers/zookeeper-prometheus-metrics/src/test/java/org/apache/zookeeper/metrics/prometheus/PrometheusHttpsMetricsProviderTest.java‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@
2121
import static org.hamcrest.CoreMatchers.containsString;
2222
import static org.hamcrest.MatcherAssert.assertThat;
2323
import static org.junit.jupiter.api.Assertions.assertEquals;
24+
import static org.junit.jupiter.api.Assertions.assertFalse;
25+
import static org.junit.jupiter.api.Assertions.assertTrue;
26+
import ch.qos.logback.classic.Logger;
27+
import ch.qos.logback.classic.spi.ILoggingEvent;
28+
import ch.qos.logback.core.read.ListAppender;
2429
import java.io.BufferedReader;
2530
import java.io.FileInputStream;
2631
import java.io.IOException;
@@ -36,6 +41,7 @@
3641
import org.apache.zookeeper.metrics.Counter;
3742
import org.junit.jupiter.api.AfterEach;
3843
import org.junit.jupiter.api.Test;
44+
import org.slf4j.LoggerFactory;
3945

4046
/**
4147
* 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 {
176182
assertThat(response, containsString("# TYPE cc_total counter"));
177183
assertThat(response, containsString("cc_total 10.0"));
178184
}
185+
186+
@Test
187+
void testLogRedactorRedactsPasswords() throws Exception {
188+
Logger logger = (Logger) LoggerFactory.getLogger(PrometheusMetricsProvider.class);
189+
ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
190+
listAppender.start();
191+
logger.addAppender(listAppender);
192+
193+
try {
194+
provider = new PrometheusMetricsProvider();
195+
Properties configuration = new Properties();
196+
configuration.setProperty("httpPort", String.valueOf(httpPort));
197+
configuration.setProperty("httpsPort", String.valueOf(httpsPort));
198+
configuration.setProperty("ssl.keyStore.location", testDataPath + "/ssl/server_keystore.jks");
199+
configuration.setProperty("ssl.keyStore.password", "SuperSecret123!");
200+
configuration.setProperty("ssl.trustStore.location", testDataPath + "/ssl/server_truststore.jks");
201+
configuration.setProperty("ssl.trustStore.password", "AnotherSecret456!");
202+
provider.configure(configuration);
203+
204+
String logOutput = listAppender.list.stream()
205+
.map(ILoggingEvent::getFormattedMessage)
206+
.filter(msg -> msg.contains("configuration"))
207+
.findFirst()
208+
.orElse("");
209+
210+
assertFalse(logOutput.contains("SuperSecret123!"),
211+
"Logs should not contain keyStore password");
212+
assertFalse(logOutput.contains("AnotherSecret456!"),
213+
"Logs should not contain trustStore password");
214+
assertTrue(logOutput.contains(testDataPath + "/ssl/server_keystore.jks"),
215+
"Logs should still contain non-sensitive config like keyStore location");
216+
} finally {
217+
logger.detachAppender(listAppender);
218+
}
219+
}
179220
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/common/LogRedactor.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
package org.apache.zookeeper.common;
20+
21+
import java.util.Locale;
22+
import java.util.Map;
23+
import java.util.Properties;
24+
25+
public class LogRedactor {
26+
27+
private LogRedactor() {}
28+
29+
/**
30+
* Redacts all values which ends with "password" from the given properties / map.
31+
* Other values are not changed.
32+
*
33+
* @param properties the properties in which all passwords should be redacted.
34+
* @return new Properties object containing all values, passwords redacted.
35+
*/
36+
public static Properties redactSensitiveValues(Map<?, ?> properties) {
37+
Properties redactedConfig = new Properties();
38+
properties.forEach((k, v) -> {
39+
if (k != null && v != null) {
40+
redactedConfig.put(k, redactValue((String) k, (String) v));
41+
}
42+
});
43+
return redactedConfig;
44+
}
45+
46+
/**
47+
* Returns redacted value when the key ends with "password".
48+
* Otherwise, just returns the value.
49+
*
50+
* @param key the key to check if it ends with "password".
51+
* @return redacted value when the key ends with "password". Otherwise, the value.
52+
*/
53+
public static String redactValue(String key, String value) {
54+
if (key == null) {
55+
return value;
56+
}
57+
if (key.toLowerCase(Locale.ROOT).endsWith("password")) {
58+
return "***";
59+
}
60+
return value;
61+
}
62+
}

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

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,15 @@
1818

1919
package org.apache.zookeeper.common;
2020

21+
import static org.apache.zookeeper.common.LogRedactor.redactSensitiveValues;
22+
import static org.apache.zookeeper.common.LogRedactor.redactValue;
2123
import java.io.File;
2224
import java.io.FileInputStream;
2325
import java.io.IOException;
2426
import java.lang.reflect.Constructor;
2527
import java.nio.file.Path;
2628
import java.nio.file.Paths;
2729
import java.util.HashMap;
28-
import java.util.Locale;
2930
import java.util.Map;
3031
import java.util.Map.Entry;
3132
import java.util.Properties;
@@ -105,11 +106,7 @@ public ZKConfig(File configFile) throws ConfigException {
105106
public ZKConfig(Path configPath) throws ConfigException {
106107
this();
107108
addConfiguration(configPath);
108-
Map<String, String> p = new HashMap<>();
109-
for (Entry<String, String> entry : properties.entrySet()) {
110-
p.put(entry.getKey(), logRedactor(entry.getKey(), entry.getValue()));
111-
}
112-
LOG.info("ZK Config {}", p);
109+
LOG.info("ZK Config {}", redactSensitiveValues(properties));
113110
}
114111

115112
private void init() {
@@ -211,7 +208,7 @@ public void setProperty(String key, String value) {
211208
}
212209
String oldValue = properties.put(key, value);
213210
if (null != oldValue && !oldValue.equals(value)) {
214-
LOG.debug("key {}'s value {} is replaced with new value {}", key, logRedactor(key, oldValue), logRedactor(key, value));
211+
LOG.debug("key {}'s value {} is replaced with new value {}", key, redactValue(key, oldValue), redactValue(key, value));
215212
}
216213
}
217214

@@ -341,14 +338,4 @@ public int getInt(String key, int defaultValue) {
341338
}
342339
return defaultValue;
343340
}
344-
345-
private String logRedactor(String key, String value) {
346-
if (key == null) {
347-
return value;
348-
}
349-
if (key.toLowerCase(Locale.ROOT).endsWith("password")) {
350-
return "***";
351-
}
352-
return value;
353-
}
354341
}

‎zookeeper-server/src/test/java/org/apache/zookeeper/common/LogRedactorTest.java‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
package org.apache.zookeeper.common;
20+
21+
import static org.junit.jupiter.api.Assertions.assertEquals;
22+
import static org.junit.jupiter.api.Assertions.assertFalse;
23+
import static org.junit.jupiter.api.Assertions.assertTrue;
24+
import java.util.HashMap;
25+
import java.util.Map;
26+
import java.util.Properties;
27+
import org.junit.jupiter.api.Test;
28+
29+
class LogRedactorTest {
30+
31+
@Test
32+
void testRedactValueWithPasswordKey() {
33+
assertEquals("***", LogRedactor.redactValue("ssl.keyStore.password", "secret"));
34+
assertEquals("***", LogRedactor.redactValue("ssl.trustStore.password", "secret"));
35+
assertEquals("***", LogRedactor.redactValue("somePassword", "secret"));
36+
}
37+
38+
@Test
39+
void testRedactValueCaseInsensitive() {
40+
assertEquals("***", LogRedactor.redactValue("ssl.keyStore.PASSWORD", "secret"));
41+
assertEquals("***", LogRedactor.redactValue("ssl.keyStore.Password", "secret"));
42+
assertEquals("***", LogRedactor.redactValue("ssl.keyStore.passWORD", "secret"));
43+
}
44+
45+
@Test
46+
void testRedactValueNonSensitiveKey() {
47+
assertEquals("localhost", LogRedactor.redactValue("server.host", "localhost"));
48+
assertEquals("8080", LogRedactor.redactValue("httpPort", "8080"));
49+
assertEquals("/path/to/keystore", LogRedactor.redactValue("ssl.keyStore.location", "/path/to/keystore"));
50+
}
51+
52+
@Test
53+
void testRedactValueNullKey() {
54+
assertEquals("someValue", LogRedactor.redactValue(null, "someValue"));
55+
}
56+
57+
@Test
58+
void testRedactSensitiveValuesWithStringMap() {
59+
Map<String, String> config = new HashMap<>();
60+
config.put("ssl.keyStore.location", "/path/to/keystore.jks");
61+
config.put("ssl.keyStore.password", "SuperSecret");
62+
config.put("httpPort", "9141");
63+
64+
Properties redacted = LogRedactor.redactSensitiveValues(config);
65+
66+
assertEquals("/path/to/keystore.jks", redacted.get("ssl.keyStore.location"));
67+
assertEquals("***", redacted.get("ssl.keyStore.password"));
68+
assertEquals("9141", redacted.get("httpPort"));
69+
}
70+
71+
@Test
72+
void testRedactSensitiveValuesWithProperties() {
73+
Properties config = new Properties();
74+
config.setProperty("ssl.trustStore.password", "TrustSecret");
75+
config.setProperty("ssl.trustStore.location", "/path/to/truststore.jks");
76+
77+
Properties redacted = LogRedactor.redactSensitiveValues(config);
78+
79+
assertEquals("***", redacted.get("ssl.trustStore.password"));
80+
assertEquals("/path/to/truststore.jks", redacted.get("ssl.trustStore.location"));
81+
}
82+
83+
@Test
84+
void testRedactSensitiveValuesSkipsNullValues() {
85+
Map<String, String> config = new HashMap<>();
86+
config.put("ssl.keyStore.location", null);
87+
config.put("httpPort", "9141");
88+
89+
Properties redacted = LogRedactor.redactSensitiveValues(config);
90+
91+
assertFalse(redacted.containsKey("ssl.keyStore.location"));
92+
assertEquals("9141", redacted.get("httpPort"));
93+
}
94+
95+
@Test
96+
void testRedactSensitiveValuesEmptyMap() {
97+
Properties redacted = LogRedactor.redactSensitiveValues(new HashMap<>());
98+
assertTrue(redacted.isEmpty());
99+
}
100+
}

0 commit comments

Comments
 (0)