[SPARK-57480][SQL] Stash allowSettingSystemProperties on a static field to prevent set:hiveconf bypass#56528
Open
yadavay-amzn wants to merge 1 commit into
Conversation
dongjoon-hyun
previously requested changes
Jun 15, 2026
Member
There was a problem hiding this comment.
Thank you, @yadavay-amzn . Just one tip, you should not ping someone in the PR description because it will become a commit log. Please revise the PR description.
| } | ||
|
|
||
| // SPARK-57480: resolved at server init time by SparkSQLSessionManager so the per-session | ||
| // JDBC `set:` overlay cannot mutate it through `set:hiveconf:`. See setAllowSettingSystemProperties. |
| } | ||
| } | ||
|
|
||
| // SPARK-57480: resolved at server init time by SparkSQLSessionManager so the per-session |
Member
There was a problem hiding this comment.
Please remove this, SPARK-57480: .
| * Configure whether `set:system:*` is permitted. Intended to be called once at server init | ||
| * from SparkSQLSessionManager. Reading and writing this value is intentionally not routed | ||
| * through any per-session HiveConf, since that would let a low-privilege client mutate it | ||
| * via `set:hiveconf:` from the same JDBC overlay this gate is meant to guard (SPARK-57480). |
Member
There was a problem hiding this comment.
Let's remove (SPARK-57480).
| if (ss.getConf().getBoolean( | ||
| "spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties", false)) { | ||
| // The toggle is read from a static field set once at server init time, NOT from the | ||
| // per-session HiveConf, to prevent bypass via `set:hiveconf:` (SPARK-57480). |
Member
There was a problem hiding this comment.
Ditto. Remove (SPARK-57480)
| // which runs during session open before the SparkSession is attached, can read it. | ||
| hiveConf.setBoolean( | ||
| StaticSQLConf.LEGACY_HIVE_THRIFT_SERVER_ALLOW_SETTING_SYSTEM_PROPERTIES.key, | ||
| // SPARK-57480: stash the toggle on a static field instead of writing it into HiveConf. |
Member
There was a problem hiding this comment.
ditto. Remove SPARK-57480: .
| val flagKey = StaticSQLConf.LEGACY_HIVE_THRIFT_SERVER_ALLOW_SETTING_SYSTEM_PROPERTIES.key | ||
| try { | ||
| withSystemPropSession(allowSettingSystemProperties = false) { | ||
| // Before SPARK-57480, the system: gate read its toggle from the per-session HiveConf, |
Member
There was a problem hiding this comment.
Let's remove Before SPARK-57480, because this is meaningless because we add a test coverage to prevent a regression and the test prefix is supposed to mention that as you did already in line 111. This is a simple duplication without giving any new information.
…ld to prevent set:hiveconf bypass
### What changes were proposed in this pull request?
This PR moves the `spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties` toggle
out of per-session `HiveConf` and onto a static field on `HiveSessionImpl`, populated once
at server init by `SparkSQLSessionManager`.
### Why are the changes needed?
SPARK-57441 added a default-deny gate on `set:system:*` in `HiveSessionImpl.setVariable`,
but read its toggle from `ss.getConf().getBoolean(...)` -- the per-session `HiveConf`. The
same `setVariable` method writes to that same `HiveConf` from its `HIVECONF_PREFIX` branch
via `setConf` -> `verifyAndSet`, with no validation that rejects `spark.*` keys (Hive's
`getConfVars` returns null for non-Hive keys, the `key.startsWith("hive.")` fallback does
not fire, and `verifyAndSet` has no default `restrictList` entry for this key).
A JDBC URL of the form
```
jdbc:hive2://host:port/db?set:hiveconf:spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties=true;set:system:foo=bar
```
flips the flag in the per-session `HiveConf` via the first directive, then the second
directive's gate read sees `true` and proceeds with `System.setProperty`, bypassing the
default-deny. See SPARK-57480 for the repro and impact analysis.
The fix detaches the gate read from per-session state. The static field is set once at
`SparkSQLSessionManager.init` from the server-wide `StaticSQLConf`, and is no longer
reachable from any per-session `set:` overlay. The `set:hiveconf:<flag-key>=true` write
still succeeds in writing into the per-session `HiveConf` (preserved for backward
compatibility), but it is now a no-op for the gate.
### Does this PR introduce _any_ user-facing change?
No behavior change for users. The default-deny posture from SPARK-57441 is preserved, and
the legacy escape-hatch (`spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties=true`
in the server's static SQLConf) still works.
### How was this patch tested?
Added `HiveSessionImplSuite` regression test that exercises the exact bypass:
1. Open a session with the legacy flag explicitly disabled.
2. Call `setVariable("hiveconf:<flag-key>", "true")` -- this is what `configureSession` does
for `set:hiveconf:` in a JDBC URL.
3. Call `setVariable("system:<prop>", "<val>")`.
4. Assert that step 3 returns `1` (rejected) and `System.getProperty` is null.
I verified the regression test fails when the production fix is reverted (gate read goes
back to `ss.getConf().getBoolean(...)`): test reports
`0 did not equal 1 system:* should still be rejected, got rc=0`.
All four `HiveSessionImplSuite` tests pass with the fix in place.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.7
yadavay-amzn
force-pushed
the
SPARK-57480-thriftserver-system-prop-bypass
branch
from
944468e to
704b6e3
Compare
Contributor
Author
|
Thanks for the review @dongjoon-hyun. Addressed all comments in 704b6e3:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR moves the
spark.sql.legacy.hive.thriftServer.allowSettingSystemPropertiestoggle out of per-sessionHiveConfand onto a static field onHiveSessionImpl, populated once at server init bySparkSQLSessionManager.Why are the changes needed?
SPARK-57441 added a default-deny gate on
set:system:*inHiveSessionImpl.setVariable, but read its toggle fromss.getConf().getBoolean(...)-- the per-sessionHiveConf. The samesetVariablemethod writes to that sameHiveConffrom itsHIVECONF_PREFIXbranch viasetConf->verifyAndSet, with no validation that rejectsspark.*keys (Hive'sgetConfVarsreturns null for non-Hive keys, thekey.startsWith("hive.")fallback does not fire, andverifyAndSethas no defaultrestrictListentry for this key).A JDBC URL of the form
flips the flag in the per-session
HiveConfvia the first directive, then the second directive's gate read seestrueand proceeds withSystem.setProperty, bypassing the default-deny. See SPARK-57480 for the repro and impact analysis.The fix detaches the gate read from per-session state. The static field is set once at
SparkSQLSessionManager.initfrom the server-wideStaticSQLConf, and is no longer reachable from any per-sessionset:overlay. Theset:hiveconf:write to the flag key still succeeds in writing into the per-sessionHiveConf(preserved for backward compatibility), but it is now a no-op for the gate.Does this PR introduce any user-facing change?
No behavior change for users. The default-deny posture from SPARK-57441 is preserved, and the legacy escape-hatch (
spark.sql.legacy.hive.thriftServer.allowSettingSystemProperties=truein the server's static SQLConf) still works.How was this patch tested?
Added
HiveSessionImplSuiteregression test that exercises the exact bypass:setVariable("hiveconf:" + flagKey, "true")-- this is whatconfigureSessiondoes forset:hiveconf:in a JDBC URL.setVariable("system:" + propKey, propValue).1(rejected) andSystem.getPropertyis null.I verified the regression test fails when the production fix is reverted (gate read goes back to
ss.getConf().getBoolean(...)): test reports0 did not equal 1 system:* should still be rejected, got rc=0.All four
HiveSessionImplSuitetests pass with the fix in place.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.7