[
https://issues.apache.org/jira/browse/SPARK-58658?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
L. C. Hsieh updated SPARK-58658:
--------------------------------
Description:
The Spark Connect Config RPC hands back any configuration key the session
holds, including keys that Spark treats as sensitive everywhere else. For
example the pre-shared authentication token:
spark.conf.get("spark.connect.authenticate.token") // returns the token
Keys reach the session config because SQLConf.mergeSparkConf copies every
SparkConf entry -- static ones included -- into it, and
SparkConnectConfigHandler's read paths (handleGet, handleGetOption,
handleGetWithDefault, handleGetAll) apply no denylist. RuntimeConfig guards
writes to static configs with requireNonStaticConf, but reads are unguarded. So
any secret that ends up in the driver's SparkConf is readable by a Connect
client, both keyed and through GetAll.
This is broader than the authentication token. Deployments that layer their
own configuration into SparkConf expose their own secrets the same way. Apache
Kyuubi, for instance, passes its engine configuration through --conf and
prefixes non-spark. keys with spark., so deployment-wide secrets -- the
ZooKeeper digest used for discovery-namespace registration, and the pre-shared
secret behind its internal engine tokens -- land in the driver's SparkConf and
are readable through the Config RPC (verified on 4.0.3 and 4.2.0, with the keys
Kyuubi produces and its redaction pattern set).
None of this is a vulnerability in the client-to-server model Spark documents:
a client that can call the Config RPC has already authenticated, and the
authentication token is a credential it had to present to connect. It matters
to deployments that reuse the token, or other secrets, as a value shared
between components while end users authenticate by other means -- a proxy in
front of Spark Connect, or the Kyuubi engine model.
There, any client the deployment admits can read a secret out of the driver and
reach a component directly. Spark does not promise otherwise, so this is
defense in depth rather than a fix for a Spark vulnerability; a server not
disclosing configuration it already considers sensitive is the better default
regardless.
Proposed change: have the Config RPC read paths withhold any key whose name
matches the existing spark.redaction.regex
(default(?i)secret|password|token|access[.]?key), the same pattern Spark
already uses to redact configuration in the UI and logs. A withheld key is
reported the way an unset key is: Get and GetOption return no value,
GetWithDefault returns the caller's default, and GetAll omits it.
spark.connect.authenticate.token matches this pattern, so the token case is
covered as one instance of the general rule. Set/Unset already reject static
configs via requireNonStaticConf, and IsModifiable returns false for one
without disclosing anything.
Note for whoever implements this: handleGetAll strips the requested prefix from
the keys it returns, so the match has to run on the full key before the prefix
is removed.
was:
The Spark Connect Config RPC hands back any configuration key the session
holds, including {{spark.connect.authenticate.token}}:
{code}
spark.conf.get("spark.connect.authenticate.token") // returns the token
{code}
The key reaches the session config because {{SQLConf.mergeSparkConf}} copies
every {{SparkConf}} entry -- static ones included -- into it, and
{{SparkConnectConfigHandler}}'s read paths ({{handleGet}}, {{handleGetOption}},
{{handleGetWithDefault}}, {{handleGetAll}}) apply no denylist.
{{RuntimeConfig}} guards *writes* to static configs with
{{requireNonStaticConf}}, but reads are unguarded.
This is not a vulnerability in the client-to-server deployment Spark itself
documents: the token is a client credential, and a client that can call the
Config RPC has already presented it, so reading it back discloses nothing.
It does matter for deployments that place a proxy in front of Spark Connect and
reuse this config as a secret shared between the proxy and the servers, with
end users authenticating to the proxy by other means. There, any user the proxy
admits can read the token out of a backend and then connect to that backend
directly, bypassing whatever the proxy enforces. That reinterpretation of the
config is the deployment's own, not something Spark promises -- which is why
this is filed as an improvement rather than a security issue -- but a server
not disclosing its own credential is the better default regardless, and it is
cheap.
*Proposed change:* report the key as unset on every read path: {{Get}} and
{{GetOption}} return no value, {{GetWithDefault}} returns the caller's default,
and {{GetAll}} omits it. {{Set}}/{{Unset}} already reject it via
{{requireNonStaticConf}}, and {{IsModifiable}} returns false for a static
config without disclosing anything.
Note for anyone implementing this: {{handleGetAll}} strips the requested prefix
from the keys it returns, so the filter has to run on the full key before the
prefix is removed.
> Do not return redacted configurations from the Config RPC
> ---------------------------------------------------------
>
> Key: SPARK-58658
> URL: https://issues.apache.org/jira/browse/SPARK-58658
> Project: Spark
> Issue Type: Bug
> Components: Connect
> Affects Versions: 4.2.0, 4.1.2, 4.3.0
> Reporter: L. C. Hsieh
> Assignee: L. C. Hsieh
> Priority: Major
> Labels: pull-request-available
>
> The Spark Connect Config RPC hands back any configuration key the session
> holds, including keys that Spark treats as sensitive everywhere else. For
> example the pre-shared authentication token:
> spark.conf.get("spark.connect.authenticate.token") // returns the token
> Keys reach the session config because SQLConf.mergeSparkConf copies every
> SparkConf entry -- static ones included -- into it, and
> SparkConnectConfigHandler's read paths (handleGet, handleGetOption,
> handleGetWithDefault, handleGetAll) apply no denylist. RuntimeConfig guards
> writes to static configs with requireNonStaticConf, but reads are unguarded.
> So any secret that ends up in the driver's SparkConf is readable by a Connect
> client, both keyed and through GetAll.
> This is broader than the authentication token. Deployments that layer their
> own configuration into SparkConf expose their own secrets the same way.
> Apache Kyuubi, for instance, passes its engine configuration through --conf
> and prefixes non-spark. keys with spark., so deployment-wide secrets -- the
> ZooKeeper digest used for discovery-namespace registration, and the
> pre-shared secret behind its internal engine tokens -- land in the driver's
> SparkConf and are readable through the Config RPC (verified on 4.0.3 and
> 4.2.0, with the keys Kyuubi produces and its redaction pattern set).
> None of this is a vulnerability in the client-to-server model Spark
> documents: a client that can call the Config RPC has already authenticated,
> and the authentication token is a credential it had to present to connect. It
> matters to deployments that reuse the token, or other secrets, as a value
> shared between components while end users authenticate by other means -- a
> proxy in front of Spark Connect, or the Kyuubi engine model.
> There, any client the deployment admits can read a secret out of the driver
> and reach a component directly. Spark does not promise otherwise, so this is
> defense in depth rather than a fix for a Spark vulnerability; a server not
> disclosing configuration it already considers sensitive is the better default
> regardless.
> Proposed change: have the Config RPC read paths withhold any key whose name
> matches the existing spark.redaction.regex
> (default(?i)secret|password|token|access[.]?key), the same pattern Spark
> already uses to redact configuration in the UI and logs. A withheld key is
> reported the way an unset key is: Get and GetOption return no value,
> GetWithDefault returns the caller's default, and GetAll omits it.
> spark.connect.authenticate.token matches this pattern, so the token case is
> covered as one instance of the general rule. Set/Unset already reject static
> configs via requireNonStaticConf, and IsModifiable returns false for one
> without disclosing anything.
> Note for whoever implements this: handleGetAll strips the requested prefix
> from the keys it returns, so the match has to run on the full key before the
> prefix is removed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
