xiangfu0 commented on code in PR #19884:
URL: https://github.com/apache/pinot/pull/19884#discussion_r4288268117
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Obfuscator.java:
##########
@@ -58,16 +58,25 @@
public final class Obfuscator {
private static final String DEFAULT_MASKED_VALUE = "*****";
+ /// Suffix-anchored, so that a key is masked for what it ends in and a
structural name such as
+ /// `pinot.broker.auth.provider.class` stays readable. Beyond the generic
credential words, this covers
+ /// credentials whose key does not end in one: the AWS `accessKey` /
`accessKeyId` pair (as in
+ /// `input.fs.prop.accessKey` and `fs.s3a.access.key`), Kafka's
`sasl.jaas.config`, Confluent's
+ /// `basic.auth.user.info` (a literal `user:password`), and GCP
service-account key material.
private static final List<Pattern> DEFAULT_PATTERNS =
Stream.of("(?i).*secret$", "(?i).*secret[\\s_-]*key$",
"(?i).*api[\\s_-]*key$", "(?i).*password$",
- "(?i).*keytab$", "(?i).*token$")
+ "(?i).*passwd$", "(?i).*passphrase$", "(?i).*keytab$",
"(?i).*token$", "(?i).*credentials?$",
+ "(?i).*access[\\s._-]*key([\\s._-]*id)?$",
"(?i).*private[\\s._-]*key([\\s._-]*id)?$",
Review Comment:
**MAJOR** — You added dotted support for `access.key` here, so
`fs.s3a.access.key` is now masked. But its counterpart `fs.s3a.secret.key` is
still only matched by the pre-existing `(?i).*secret[\s_-]*key$` on line 67,
whose separator class lacks `.`, so the dotted secret key is **not** masked and
leaks. Since the secret key is the more sensitive half of the S3A pair,
consider adding `.` to that class (or an `(?i).*secret[\s._-]*key$` pattern) so
both are covered symmetrically.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Obfuscator.java:
##########
@@ -58,16 +58,25 @@
public final class Obfuscator {
private static final String DEFAULT_MASKED_VALUE = "*****";
+ /// Suffix-anchored, so that a key is masked for what it ends in and a
structural name such as
+ /// `pinot.broker.auth.provider.class` stays readable. Beyond the generic
credential words, this covers
+ /// credentials whose key does not end in one: the AWS `accessKey` /
`accessKeyId` pair (as in
+ /// `input.fs.prop.accessKey` and `fs.s3a.access.key`), Kafka's
`sasl.jaas.config`, Confluent's
+ /// `basic.auth.user.info` (a literal `user:password`), and GCP
service-account key material.
private static final List<Pattern> DEFAULT_PATTERNS =
Stream.of("(?i).*secret$", "(?i).*secret[\\s_-]*key$",
"(?i).*api[\\s_-]*key$", "(?i).*password$",
- "(?i).*keytab$", "(?i).*token$")
+ "(?i).*passwd$", "(?i).*passphrase$", "(?i).*keytab$",
"(?i).*token$", "(?i).*credentials?$",
+ "(?i).*access[\\s._-]*key([\\s._-]*id)?$",
"(?i).*private[\\s._-]*key([\\s._-]*id)?$",
+ "(?i).*jaas[\\s._-]*config$", "(?i).*user[\\s._-]*info$",
"(?i).*json[\\s._-]*key$",
+ "(?i).*key[\\s._-]*file$",
"(?i).*key[\\s._-]*content([\\s._-]*base64)?$")
Review Comment:
**MINOR** — `key[\s._-]*file$` also matches non-secret path keys like
`sslKeyFile`, `publicKeyFile`, and `clientKeyFile`. The masked values there are
filesystem paths rather than secrets, so masking them only costs a bit of
debuggability. Fine to keep if intentional; just flagging the slight
over-breadth.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Obfuscator.java:
##########
@@ -58,16 +58,25 @@
public final class Obfuscator {
private static final String DEFAULT_MASKED_VALUE = "*****";
+ /// Suffix-anchored, so that a key is masked for what it ends in and a
structural name such as
+ /// `pinot.broker.auth.provider.class` stays readable. Beyond the generic
credential words, this covers
+ /// credentials whose key does not end in one: the AWS `accessKey` /
`accessKeyId` pair (as in
+ /// `input.fs.prop.accessKey` and `fs.s3a.access.key`), Kafka's
`sasl.jaas.config`, Confluent's
+ /// `basic.auth.user.info` (a literal `user:password`), and GCP
service-account key material.
private static final List<Pattern> DEFAULT_PATTERNS =
Stream.of("(?i).*secret$", "(?i).*secret[\\s_-]*key$",
"(?i).*api[\\s_-]*key$", "(?i).*password$",
- "(?i).*keytab$", "(?i).*token$")
+ "(?i).*passwd$", "(?i).*passphrase$", "(?i).*keytab$",
"(?i).*token$", "(?i).*credentials?$",
+ "(?i).*access[\\s._-]*key([\\s._-]*id)?$",
"(?i).*private[\\s._-]*key([\\s._-]*id)?$",
Review Comment:
**MAJOR** — You added dotted support for `access.key` here, so
`fs.s3a.access.key` is now masked. But its counterpart `fs.s3a.secret.key` is
still only matched by the pre-existing `(?i).*secret[\s_-]*key$` on line 67,
whose separator class lacks `.`, so the dotted secret key is **not** masked and
leaks. Since the secret key is the more sensitive half of the S3A pair,
consider adding `.` to that class (or an `(?i).*secret[\s._-]*key$` pattern) so
both are covered symmetrically.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/Obfuscator.java:
##########
@@ -58,16 +58,25 @@
public final class Obfuscator {
private static final String DEFAULT_MASKED_VALUE = "*****";
+ /// Suffix-anchored, so that a key is masked for what it ends in and a
structural name such as
+ /// `pinot.broker.auth.provider.class` stays readable. Beyond the generic
credential words, this covers
+ /// credentials whose key does not end in one: the AWS `accessKey` /
`accessKeyId` pair (as in
+ /// `input.fs.prop.accessKey` and `fs.s3a.access.key`), Kafka's
`sasl.jaas.config`, Confluent's
+ /// `basic.auth.user.info` (a literal `user:password`), and GCP
service-account key material.
private static final List<Pattern> DEFAULT_PATTERNS =
Stream.of("(?i).*secret$", "(?i).*secret[\\s_-]*key$",
"(?i).*api[\\s_-]*key$", "(?i).*password$",
- "(?i).*keytab$", "(?i).*token$")
+ "(?i).*passwd$", "(?i).*passphrase$", "(?i).*keytab$",
"(?i).*token$", "(?i).*credentials?$",
+ "(?i).*access[\\s._-]*key([\\s._-]*id)?$",
"(?i).*private[\\s._-]*key([\\s._-]*id)?$",
+ "(?i).*jaas[\\s._-]*config$", "(?i).*user[\\s._-]*info$",
"(?i).*json[\\s._-]*key$",
+ "(?i).*key[\\s._-]*file$",
"(?i).*key[\\s._-]*content([\\s._-]*base64)?$")
Review Comment:
**MINOR** — `key[\s._-]*file$` also matches non-secret path keys like
`sslKeyFile`, `publicKeyFile`, and `clientKeyFile`. The masked values there are
filesystem paths rather than secrets, so masking them only costs a bit of
debuggability. Fine to keep if intentional; just flagging the slight
over-breadth.
--
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]
