Jackie-Jiang commented on code in PR #19775:
URL: https://github.com/apache/pinot/pull/19775#discussion_r4252758939
##########
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
+ // the value of an otherwise innocuous-looking key such as
"sasl.jaas.config".
+ private static final Pattern EMBEDDED_CREDENTIAL_PATTERN =
+ Pattern.compile("((?:username|password)\\s*=\\s*\")[^\"]*(\")",
Pattern.CASE_INSENSITIVE);
Review Comment:
[P1] Handle all valid JAAS credential forms
Kafka accepts both `password=secret` and `password='secret'`, but this
pattern leaves them unchanged. It also treats an escaped quote as the end of
the value: `password="prefix\"secret-tail"` becomes
`password="*****"secret-tail"`, exposing the suffix. I verified these inputs
with Kafka 3.9.2's actual JAAS parser and the exact regex here.
Because `sasl.jaas.config` does not match the existing Obfuscator key
patterns, these credentials still reach logs through `StreamConfig.toString()`
when decoder initialization fails. Please mask the entire JAAS configuration
value, or handle the complete quoting and escaping rules, and add regression
coverage for these cases.
##########
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);
Review Comment:
Could we centralize this logic in `Obfuscator` and have
`StreamConfig.toString()` delegate to it? Adding `sasl.jaas.config` and its
prefixed forms to the default sensitive-key patterns would mask the entire
value, handle quoting and escaping safely, and protect existing `Obfuscator`
callers too. The redaction cases should be covered in `ObfuscatorTest`, with a
regression test verifying that `StreamConfig` masks both maps without changing
the original configuration values.
Please preserve the custom-pattern constructor's existing contract that
defaults do not apply.
--
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]
