[KYUUBI #7623][AUTHZ] Check the effective value of spark.sql.optimizer.excludedRules - #7637
Conversation
…timizer.excludedRules AuthzConfigurationChecker only rejected the SET syntax, so any channel that writes the config without producing a logical plan - spark.conf.set, the Spark Connect Config RPC, a JDBC connection string - could name RuleAuthorization in spark.sql.optimizer.excludedRules and drop Ranger authorization for the rest of the session. Read the value in effect on every plan instead. Check rules are not filtered by excludedRules, which only applies to optimizer batches, so the check itself cannot be removed the same way.
|
The one red check here, It does not look like a master-side break either: the same job passed on #7636 against the same master. This branch is level with master, so there is nothing to rebase onto that would re-trigger the run — could someone re-run that job? |
|
@alexandrefimov Thanks. The failure of |
| final val RESTRICT_LIST_KEY = "spark.kyuubi.conf.restricted.list" | ||
| final val EXCLUDED_RULES_KEY = "spark.sql.optimizer.excludedRules" | ||
|
|
||
| final private val AUTHZ_RANGER_RULE_PACKAGE = "org.apache.kyuubi.plugin.spark.authz.ranger" |
There was a problem hiding this comment.
org.apache.kyuubi.plugin.spark.authz.ranger should be changed to org.apache.kyuubi.plugin.spark.authz. Currently, most rules are located at https://github.com/apache/kyuubi/tree/master/extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/rule cc @cfmcgrady @pan3793
There was a problem hiding this comment.
Happy to do that in a follow-up.
One thing I checked while looking at it: excludedRules only filters optimizer batches, and Spark has no analyzer equivalent, so the rules that apply masking and row filters can't be excluded at all. Of the rules the extension injects, four are reachable this way — RuleAuthorization in …authz.ranger, and RuleEliminateMarker, RuleEliminatePermanentViewMarker, RuleEliminateTypeOf in …authz.rule. Reading those three, excluding one looks like it breaks the query rather than lifting a check: the markers survive into planning and TypeOfPlaceHolder has no eval. So widening the prefix looks like future-proofing to me — new rules land under …authz.rule — rather than closing a live bypass. That's code reading, not a run.
It does flip behaviour for anyone excluding one of those cleanup rules today, so waiting for @cfmcgrady / @pan3793 seems right. Should I open an issue and a PR for it once they weigh in?
There was a problem hiding this comment.
Ran it since, and the reading holds. With the real extension and the test policies, excluding RuleEliminateMarker fails planning for both a masked and a row-filtered query — No plan for RowFilterMarker +- DataMaskingStage0Marker …; RuleEliminatePermanentViewMarker fails the same way (No plan for PermanentViewMarker …), and RuleEliminateTypeOf fails at runtime with UnaryExpressions must override either eval or nullSafeEval. The masking projection is still in the failing plan (md5(cast(cast(value1 as string) as binary))), which is the point: masking and row filtering come from resolution rules that can't be excluded at all, and those three only strip the markers afterwards. A user with no privilege on the table is denied exactly as before with all three excluded.
So nothing is reachable through the narrower prefix today, and the wider one covers rules added under …authz.rule later. Glad to carry these as tests in the follow-up if that's the direction you land on.
wForget
left a comment
wForget
left a comment
There was a problem hiding this comment.
Thanks @alexandrefimov ，LGTM.
I added a comment that may not be relevant to this PR; we can fix it in a new PR.
2 participants
Why are the changes needed?
Closes #7623.
AuthzConfigurationCheckerguards the exclusion by matching aSetCommandin the logical plan (AuthzConfigurationChecker.scala:42-45), so the protection covers theSETsyntax only - which is also how the docs describe it (docs/security/authorization/spark/overview.rst:106, "A set statement with key equal to ..."). Every channel that writes the config without producing a plan keeps working:spark.conf.set, the Spark Connect Config RPC (SparkConnectConfigHandler.handleSetcallsconf.setdirectly), or the key passed in a JDBC connection string. OnceRuleAuthorizationis named there,Optimizer.batchesdrops it - extension rules are not inSparkOptimizer.nonExcludableRules- and the rest of the session runs unauthorized.Reproduced on Spark 4.0.3 with
kyuubi-spark-authzand a Ranger plugin that denies by default:createdenied,SET spark.sql.optimizer.excludedRules=...RuleAuthorizationrejected by the checker, the same key accepted over the Connect Config RPC, the nextcreateallowed.The documented mitigation,
kyuubi.session.conf.restrict.list(docs/security/authorization/spark/overview.rst:85-93), does reject such a JDBC connection - I checked that too - but it is enforced in the server'sSessionManager, so it does not reach a client that talks to the engine directly.This patch reads the value in effect on every plan instead of matching the statement. Check rules are not filtered by
excludedRules, which only applies to optimizer batches, so this check cannot be removed the same way.Two points a reviewer may want to decide differently:
org.apache.kyuubi.plugin.spark.authz.ranger, the prefix the existingSETcase uses. The plugin also injects optimizer rules fromorg.apache.kyuubi.plugin.spark.authz.rule(RuleEliminateMarkerand its neighbours), which neither the old nor the new check covers. I left the prefix as is rather than widen the scope here.SET. That is the intent, but it is a visible behaviour change for a session that set the key before this patch.How was this patch tested?
New test in
AuthzConfigurationCheckerSuite: the config is written throughspark.conf.set- the same write path the Connect Config RPC takes - and the next plan is rejected, while excluding a non-authz rule (ConstantFolding) stays allowed.All three tests in the suite pass; with the new check removed from
apply, exactly the new test fails.The existing test needed one line:
sql("set spark.sql.optimizer.excludedRules=...")applies the value to the shared session before the rule is invoked by hand, so the test now unsets it - otherwise the effective-value check rejects every later plan in that session.Was this patch authored or co-authored using generative AI tooling?
Assisted-by: Claude:claude-opus-5