alexandrefimov opened a new pull request, #57892:
URL: https://github.com/apache/spark/pull/57892
### What changes were proposed in this pull request?
The four read paths of the Config RPC — `Get`, `GetWithDefault`, `GetOption`
and `GetAll` — stop returning configuration entries that match
`spark.redaction.regex`. A withheld entry is reported the way that same
operation already reports an unset key: `Get` fails with `SQL_CONF_NOT_FOUND`,
`GetWithDefault` returns the caller's default, `GetOption` returns no value and
`GetAll` omits the entry.
Three details are worth pointing at.
The pattern is read from the `SparkConf`, not from the session config. The
session config is client-writable. `requireNonStaticConf` does guard
`spark.redaction.regex`, but that guard is itself controlled by
`spark.sql.legacy.setCommandRejectsSparkCoreConfs`, an ordinary non-static SQL
conf, so two `Set` calls would otherwise be enough to clear the filter before
reading.
Matching covers the key or the value, following `Utils.redact`. `SetCommand`
already redacts through `SQLConf.redactOptions`, so a key-only match would
leave the Config RPC laxer than `SET` over the same configuration — a password
inside a JDBC URL is the obvious case. Only `spark.redaction.regex` is applied;
`spark.sql.redaction.options.regex` is deliberately left out, since it exists
for SQL command output and its default would withhold every key with `url` in
its name.
`GetAll` filters before the requested prefix is stripped. Filtering
afterwards would return `spark.my.secret.value` as `value` for prefix
`spark.my.secret.`, walking straight past the pattern.
`IsModifiable` is left as it is: whether a key is modifiable is a property
of the name and discloses no value.
### Why are the changes needed?
The Config RPC discloses anything sensitive that reached `SparkConf`,
whatever put it there. `spark.connect.authenticate.token` is the one key Spark
itself puts there, and it looks benign only because it happens to be the
client's own credential.
Apache Kyuubi makes the general shape visible. Kyuubi passes its own
configuration to the Spark engine through `--conf` and prefixes every
non-`spark.` key with `spark.`, so the whole engine configuration ends up in
the driver's `SparkConf`, and `SQLConf.mergeSparkConf` copies all of it into
the session config. Some of those entries are secrets shared across the
deployment rather than owned by the connecting user — a ZooKeeper digest, an
internal pre-shared secret. That is a plain client-to-server deployment with no
proxy anywhere, and a client reads back secrets it never held.
### Does this PR introduce _any_ user-facing change?
Yes. A configuration entry whose key or value matches
`spark.redaction.regex` is no longer returned by the Config RPC, and each read
operation reports it as if it were unset.
Three consequences I would like reviewers to look at explicitly.
A client can no longer read back an entry it set itself in its own session:
after `spark.conf.set("spark.sql.catalog.mycat.password", …)`, reading that key
back now fails. Classic Spark still returns it. The filter could be narrowed to
keys present in `sparkContext.conf`, which would keep session-local entries
readable; I did not do that, because it would let a secret written into the
session config server-side — by a plugin or an extension rather than through
`SparkConf` — stay readable. That trade is a judgement call and I am happy to
switch.
A batched `Get` fails as a whole if any key in the batch is withheld, since
that is how a single unset key in a batch already behaves. With the default
pattern none of the keys the clients batch internally match, but an operator
who widens `spark.redaction.regex` far enough could turn `createDataFrame` into
an error rather than a hidden value.
An empty `spark.redaction.regex` matches every key, so setting it to an
empty string — a plausible attempt to turn redaction off — now withholds the
entire configuration. `Utils.redact` degrades the same way, so I did not
special-case it here, but it is worth a decision.
### How was this patch tested?
New `SparkConnectConfigHandlerSuite` covers the five read operations, the
prefix-stripping order, the value match, and that the pattern is not taken from
the session config. `SparkConnectAuthSuite` gains an end-to-end assertion that
the authentication token is not readable while a non-sensitive server-side
configuration still is.
```
build/sbt "connect/testOnly
org.apache.spark.sql.connect.service.SparkConnectConfigHandlerSuite
org.apache.spark.sql.connect.service.SparkConnectAuthSuite"
```
10 tests, all passing. With the handler reverted to master, 8 of the 10
fail; the two that still pass are the `IsModifiable` case, whose behaviour is
deliberately unchanged, and the pre-existing authentication test.
### Was this patch authored or co-authored using generative AI tooling?
Yes — Claude Code (Opus 5) was used for code reading, drafting and review.
Every line was reviewed by the author, who takes responsibility for the patch.
--
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]
