xiangfu0 commented on code in PR #19775:
URL: https://github.com/apache/pinot/pull/19775#discussion_r4278269467
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -49,6 +52,13 @@ public class StreamConfig {
public static final double CONSUMPTION_RATE_LIMIT_NOT_SPECIFIED = -1;
+ // Matches "username=" or "password=" key-value pairs embedded inside a
single config value, e.g. a Kafka
+ // sasl.jaas.config login-module string: `... required username="foo"
password="bar";`. Obfuscator (used below)
+ // only masks values of fields whose *key* looks sensitive, so it cannot
redact a secret that is embedded inside
Review Comment:
The embedded pattern only matches double-quoted values (`password="..."`).
Unquoted or single-quoted secrets — e.g. a JDBC-style `password=secret` in a
connection string, or `password='p'` — slip through unredacted. JAAS itself
uses double quotes so the primary case is covered, but it may be worth widening
the pattern (optional quote char) so other embedded-secret shapes don't leak.
At minimum a code comment noting the double-quote-only scope would set
expectations.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -443,6 +453,22 @@ public Map<String, String> getStreamConfigsMap() {
return _streamConfigMap;
}
+ /// Returns a copy of the given config map with sensitive values masked,
suitable for logging.
+ ///
+ /// Keys that look sensitive (password, secret, token, apiKey, keytab
suffixes, see [Obfuscator]) have their
+ /// whole value masked. In addition, any `username="..."` / `password="..."`
pair embedded inside a value (as
+ /// found in a Kafka `sasl.jaas.config` login-module string) is redacted,
since such a secret is not the value
+ /// of a sensitive-looking key but is nested inside the value of an
unrelated one.
+ private static Map<String, String> redactSensitiveValues(Map<String, String>
configMap) {
+ JsonNode maskedByKey = Obfuscator.DEFAULT.toJson(configMap);
+ Map<String, String> redacted = new HashMap<>();
Review Comment:
`maskedByKey.get(key).asText()` assumes the node is present and non-null.
For a `Map<String,String>` it always is, so no NPE in practice — but note a
null map value serializes to a JSON null and comes back as the literal string
`"null"` here, which matches the old output so it's harmless. Fine to leave;
flagging so it's a conscious choice.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamConfig.java:
##########
@@ -443,6 +453,22 @@ public Map<String, String> getStreamConfigsMap() {
return _streamConfigMap;
}
+ /// Returns a copy of the given config map with sensitive values masked,
suitable for logging.
+ ///
+ /// Keys that look sensitive (password, secret, token, apiKey, keytab
suffixes, see [Obfuscator]) have their
+ /// whole value masked. In addition, any `username="..."` / `password="..."`
pair embedded inside a value (as
Review Comment:
This redaction logic has no unit test, and a silent regression here leaks
plaintext credentials — exactly what we most want guarded. `StreamConfigTest`
already exists (pinot-core) as a home. Could you add a test that builds a
`StreamConfig` with a `sasl.jaas.config` JAAS string and an
`ssl.keystore.password` key, then asserts `toString()` masks both while leaving
topic/decoder/timeouts readable? The PR's manual-verification checkboxes are
all still unchecked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
