dongjoon-hyun commented on PR #58594:
URL: https://github.com/apache/spark/pull/58594#issuecomment-5584080339
Thanks for the quick turnaround -- I re-ran my checks against `722f13f` and
all four points are addressed correctly. The escape-aware alternatives do keep
the tail of `password="ab\"cd"` out of the output, the existing token test
still keeps `username` (and thus the token id) readable while masking the
password, and `quoteReplacement` is applied on both branches. CI is green on
the module that actually runs these tests (`token-provider-kafka-0-10/test`
runs inside the `streaming, sql-kafka-0-10, ...` job).
One new issue, and it comes from my own suggestion, so let me be the one to
point it out.
### `spark.redaction.regex` is a replacement, not an addition -- this is now
fail-open
The implementation consults *only* the configured pattern. But
`spark.redaction.regex` is a single regex that overrides the default rather
than extending it, so anyone who sets it to cover their own key and doesn't
think to re-include the defaults silently loses Kafka JAAS password redaction:
```
spark.redaction.regex = (?i)my_custom_key # no 'password'
IN : PlainLoginModule required username="u" password="PLAINTEXT_SECRET";
OUT: PlainLoginModule required username="u" password="PLAINTEXT_SECRET";
```
The old `password=".*"` masked this unconditionally, so this is a
regression, and an easy one to not notice since the default config hides it. I
should have been clearer that I meant the configured pattern as an *extension*
point, not the sole source of truth. Taking the union would fix it -- always
redact the credential options we know about, and additionally honor whatever
the user configured:
```scala
private val alwaysRedactedOptions = "(?i)password|clientSecret".r
// ...
val isSecret = alwaysRedactedOptions.findFirstMatchIn(name).isDefined ||
redactionPattern.findFirstMatchIn(name).isDefined
```
That keeps the guarantee the old code had while still letting
`spark.redaction.regex` widen coverage.
### Nit: `tokenauth=true` is now masked
As expected, `tokenauth` matches on `token`, so the delegation-token entry
now logs `tokenauth="*********(redacted)"`. It's a non-secret boolean flag, so
this only costs a bit of debuggability. Fine to accept, but if you've decided
to, a short comment saying so would save the next reader the trip.
### Nit: the escaped-quote assertion is brittle
`assert(!redacted.contains("cd"))` asserts the absence of a two-character
string, which passes today only because `*********(redacted)` happens not to
contain `cd`. A distinctive tail would make the test say what it means:
```scala
"username=\"u\" password=\"ab\\\"TAIL_SECRET\";"
// assert(!redacted.contains("TAIL_SECRET"))
```
--
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]
