[SPARK-59306][SS] Redact all JAAS credential styles in Kafka parameter logging

[HyukjinKwon]

What changes were proposed in this pull request?

KafkaRedactionUtil.redactJaasParam redacts credential values in a sasl.jaas.config string
before it is logged (at DEBUG). The previous regex password="..." only masked a double-quoted
value of an option named password, so a password value in single-quoted or unquoted form, and
the OAUTHBEARER clientSecret value, were not masked.

This parses each key=value option in the JAAS entry (escape-aware for quoted values, so an
escaped quote does not cut the value short) and redacts the value of the credential options
password and clientSecret (matched case-insensitively against the full option name), in any
quoting style. Non-secret options (the login-module class, control flag, username,
serviceName, clientId, ...) are left intact so the entry stays readable in logs.

Why are the changes needed?

So Kafka JAAS credentials are not exposed in DEBUG logs regardless of how they are quoted, and so
the OAUTHBEARER clientSecret is masked as well as password.

Does this PR introduce any user-facing change?

No functional change. This only affects DEBUG log output: password values in single-quoted and
unquoted forms (in addition to the double-quoted form already masked) and clientSecret values are
now shown as <option>="[redacted]". Nothing used to connect is affected.

How was this patch tested?

KafkaRedactionUtilSuite: password (double-quoted, single-quoted, unquoted) and clientSecret
values are redacted while non-secret context (username, clientId) is preserved, and an escaped
quote inside a credential value does not leak the tail.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Opus 4.8

[dongjoon-hyun]

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?

[dongjoon-hyun]

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:

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:

"username=\"u\" password=\"ab\\\"TAIL_SECRET\";"
// assert(!redacted.contains("TAIL_SECRET"))

[dongjoon-hyun]

LGTM. All three points from my last round are addressed, and I verified the result rather than taking it on faith:

• The union (alwaysRedactedOptions || redactionPattern) closes the fail-open hole -- a narrowed spark.redaction.regex can no longer disable credential masking, and the new test pins that down. (?i) does carry across the | in "(?i)password|clientSecret", so CLIENTSECRET matches too.
• The tokenauth tradeoff is now documented at the call site, which is what I was after.
• TAIL_SECRET makes the escaped-quote test assert what it means.

Test isolation on the two new setSparkEnv cases is fine -- afterEach restores the saved SparkEnv.

CI backs this up: every test job passed, including the one that runs token-provider-kafka-0-10/test.

One trivial thing left, which is the sole reason Linters, licenses, and dependencies is red:

[error] .../KafkaRedactionUtilSuite.scala:163: File line length exceeds 100 characters

Suggestion inline. Approving -- please merge once the linter is green.

[HyukjinKwon]

Merge Summary:

• merged into master 2d7b7af
• merged into branch-4.x 4a8573a
• merged into branch-4.3 5b0d8f9

Posted by merge_spark_pr.py