acruise opened a new issue, #7593: URL: https://github.com/apache/kyuubi/issues/7593
### Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) ### Search before asking - [x] I have searched in the [issues](https://github.com/apache/kyuubi/issues?q=is%3Aissue) and found no similar issues. ### Describe the feature Add an opt-in, _runtime_ enforcement mode to the Spark authz plugin that makes unrecognized plan nodes fail closed (i.e. throw an authz exception) instead of falling through the match and silently contributing no access request. Plus, at _build time_, catch classification drift by checking that every plan node class found in the classpath is one of: * Explicitly allow-listed as "known harmless" * Durably asserted by FQCN to be checked properly, in each Spark version -- in text files, not easily-ignored tests * Not a `Command` or `LeafNode` Any newly introduced plan node type would need to be allow-listed, or manually asserted as safe (hopefully with careful PR review!) ### Runtime config: `spark.kyuubi.authz.unclassifiedNode.behavior` = `allow` | `warn` | `deny` (default: warn) * `allow`: today's behavior; violations counted and visible at DEBUG. * `warn`: log once per (class name, violation kind) per JVM and count occurrences. The accretion mode for building an allowlist from real workloads. * `deny`: throw `AccessControlException` naming the unclassified class and the config key. The enforced invariant is deliberately narrower than "flag every fallthrough", which would be unusably noisy since intermediate operators legitimately recurse: Every `Command`, every `LeafNode`, every node that can execute or mutate state outside the checked path (e.g. Spark 4's `ExecutableDuringAnalysis`), and every node whose class name has a spec that dispatch did not consult — including in subtrees pruned by constant-projection elimination — must be either matched by a spec or present on an explicit allowlist. Ordinary non-leaf query operators recurse freely. ### Motivation In the authz plugin, recognition of plan nodes is the first and mandatory step to forming a security boundary, and non-recognition (e.g. a `match` that falls through) fails open. Privilege building walks the Catalyst plan and matches nodes against the JSON spec files; any node that falls through is implicitly treated as not-authz-relevant and contributes no access request. Spark's plan space is open, meaning that new commands appear in every minor release, and third-party catalogs inject their own nodes at user runtime, so the unrecognized set grows silently. A green test suite is unfortunately insufficient to bound this risk: every test was written for a node type someone had already classified, so the tests certify that the previously known sample still authorizes correctly, but can't verify nodes that nobody has written a test for yet. Unfortunately, this is not hypothetical. Running the existing suites in deny mode against master immediately surfaced two real fail-opens, both fixed and regression-tested in the patch: 1. Iceberg metadata tables (`SELECT * FROM t.snapshots`) were never authorized: the 4-part `name()` threw a `MatchError` that fail-open swallowed. 2. Iceberg `MERGE INTO`: the rewrite embeds an already-planned `DataSourceV2ScanRelation` for its read of the target table, which `buildQuery` skipped entirely. The sharpest structural case is `CALL` on Spark 4. On Spark 3.x with Iceberg it resolves to `o.a.s.sql.catalyst.plans.logical.Call`, an Iceberg-injected `Command`, so dispatch reaches its spec. Spark 4 ships its own class under the identical fully qualified name with a different hierarchy — a `UnaryNode` implementing `ExecutableDuringAnalysis`, not a `Command`. Dispatch never reached the spec lookup and the procedure executed during analysis, before any optimizer-phase check could run. The spec entry still existed and still named the right class; it was simply unreachable. Neither a class-name check nor the presence of a spec entry detects this. When the build-time enumeration first ran, it counted 136 unclassified authz-relevant plan classes on Spark 3.5 and 195 on Spark 4.1 — the population fail-open had been hiding. The gap between "policies are enforced on every operation the plugin recognizes" and "policies are enforced on every operation" is something very few users can audit from the outside. ### Describe the solution Four fail-open layers; the fourth was found during implementation. 1. Unknown commands: `buildCommand`'s fallthrough returned `QUERY` with zero privilege objects, so `RuleAuthorization` never called `verify`. 2. Unknown leaf relations: an unmatched leaf (or a known scan node not resolved) fell into the generic recursive arm and contributed nothing. 3. Extractor drift on known commands: extraction was wrapped in `catch { case e: Exception => LOG.debug(...); Nil }`, so a spec'd command failed open when its extractors broke against a new Spark version. 4. Constant-projection pruning: `buildQuery` skips the subtree of a constant Project, but the subtree still executes (`SELECT 'x' FROM t`). Supporting pieces: an allowlist with a required `reason` per entry (`Command`s take a second colocated review); per-command extraction-failure semantics, so single descriptor failures stay expected variance rather than drift; dispatch hardening so a spec'd class routes to buildCommand even when it isn't a `Command` on this version; `verifiedSparkVersions` as an explicit enumeration of exact major.minor pairs, never a range — gating for allowlist entries, advisory for specs; and build-time coverage checks that enumerate every concrete `LogicalPlan` descendant into exactly one of four buckets, failing the build on any class new to the diff. These do not replace the runtime mode, for two independent reasons: build-time enumeration cannot see third-party catalog plugins loaded only in the user's environment, and the set of dangerous node shapes is open, so the only sound default is "unrecognized ⇒ deny". ### Additional context Implemented and green in deny mode on all four profiles (3.5, 4.0, 4.1, 4.2); backlogs 135/182/195/208. Design doc ships in the patch. Known limitations stated up front: * analysis-time execution is unreachable at runtime (`CALL` on Spark 4 is an acknowledged gap, not a fix) * partial extractor drift isn't reported at runtime * row-filtering and data-masking traversals aren't covered and want the same treatment as a follow-up * a startup Spark-major assertion is worth considering separately. Three questions for the community: 1. is `warn` the right default or should the first release ship `allow`?; 2. should the allowlist ship in-repo or be user-supplied? 3. should the per-profile backlog files be committed or generated on demand? ### Are you willing to submit PR? - [x] Yes. I would be willing to submit a PR with guidance from the Kyuubi community to improve. - [ ] No. I cannot submit a PR at this time. -- 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]
