dongjoon-hyun commented on PR #58594:
URL: https://github.com/apache/spark/pull/58594#issuecomment-5581048522
Thanks for tightening this up. The direction looks right to me, and I
verified two of the premises rather than assuming them:
- `clientSecret` is indeed the JAAS option name Kafka's
`OAuthBearerLoginCallbackHandler` reads, so covering it is justified.
- Single quotes really are parseable here. Kafka's `JaasConfig` builds a
`StreamTokenizer` and never calls `ordinaryChar('\'')`, so `'` stays a quote
char from the default syntax table. I ran the tokenizer with Kafka's exact
setup to confirm.
That said, I have two correctness concerns and a test concern.
### 1. Escaped double quotes leak the tail of the credential (regression vs.
the old pattern)
`"[^"]*"` treats `\"` as the end of the value, but `\"` is a valid way to
embed a quote. Same tokenizer run confirms it:
```
password="ab\"cd" -> parsed value: ab"cd
```
So with this patch:
```
IN : PlainLoginModule required password="ab\"cd" username="u";
OLD: PlainLoginModule required password="*********(redacted)";
NEW: PlainLoginModule required password="*********(redacted)"cd"
username="u";
```
The trailing `cd` of the password survives into the log. The old greedy
pattern happened to cover this. Making the quoted alternatives escape-aware
would fix it:
```
"(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*'|[^\s;]+
```
### 2. The change also *narrows* redaction, which the PR description doesn't
mention
The description says "No behavior change other than that more JAAS
credential fields are masked", but the greedy-to-precise switch cuts the other
way too. `password=".*"` swallowed everything from the first `password="` to
the last `"` in the string, so every field that followed was incidentally
masked. Now only the two hard-coded key names are:
```
IN : PlainLoginModule required username="admin" password="admin-secret" \
user_admin="admin-secret" user_alice="alice-secret";
OLD: ... username="admin" password="*********(redacted)";
NEW: ... password="*********(redacted)" user_admin="admin-secret"
user_alice="alice-secret";
```
`user_*` is broker-side syntax so it's unlikely on the Spark client path --
I'd call this lower-confidence than #1. But the structural point stands:
`redactJaasParam` runs over whatever `sasl.jaas.config` the user supplies, and
a two-name allow-list is fail-open, whereas a redaction helper should fail safe.
One option worth considering instead of hard-coding key names: split on
`key=value` and redact any value whose *key* matches Spark's existing
`SECRET_REDACTION_PATTERN` (`(?i)secret|password|token|access[.]key`). That
covers `password` and `clientSecret` for free, needs no maintenance when a new
credential option appears, and is consistent with `redactParams` a few lines
above, which already reads that config. The tradeoff is that `tokenauth=true`
would get masked too, which is noise -- your call whether that's acceptable.
### 3. The new test would pass against a whole-string redaction
It only asserts `!redacted.contains(secret)` and
`contains(REDACTION_REPLACEMENT_TEXT)`, so an implementation that blanks the
entire string would satisfy it. The reason `redactJaasParam` exists rather than
blanket-redacting `sasl.jaas.config` is to keep the debuggable context, and
that isn't covered. The existing test right above does assert this
(`assert(redactedJaasParams.contains(tokenId1))`). Could you add assertions
that `username="u"` and `clientId="id"` survive? The escaped-quote case from #1
would be worth a case here as well.
### Nit: `replaceAllIn(target, f)` does not quote the replacement
The function form calls `Matcher.appendReplacement` directly, so `$` and `\`
in the returned string are interpreted. It's safe today since
`REDACTION_REPLACEMENT_TEXT` is `*********(redacted)` and group(1) is just the
key name, but that constant lives over in `Utils`. Wrapping with
`Regex.quoteReplacement` would make it robust to a change there.
### Nit: `Generated-by`
`Generated-by: Isaac` isn't a model name -- could you use the actual model
that was used?
--
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]
