acruise opened a new pull request, #7594:
URL: https://github.com/apache/kyuubi/pull/7594
### Why are the changes needed?
Privilege building walks the logical plan and classifies each node: it has a
command spec, a
scan spec, or it is a pass-through. A node matching none of these is
currently silent: the
walk continues and the statement executes with whatever privileges the rest
of the plan
happened to require. Nothing logs at a level an operator would see, so an
unauthorized plan
shape is indistinguishable from an authorized one.
That silence is load-bearing in two ways. A plan node can change shape under
the same fully
qualified name between Spark releases (`CALL` changed supertype between
Spark 3 and 4 and
stopped matching its dispatch arm), and a third-party catalog can introduce
nodes the spec
set has never seen. In both cases authorization stops happening rather than
fails.
Running the existing suites with unclassified nodes set to `deny` surfaced
two live
fail-opens on master. Both are fixed here with regression tests:
1. **Iceberg metadata tables were never authorized.** `SELECT * FROM
db.t.snapshots` (and
`.history`, `.manifests`, ...) reports a four-part `name()`.
`StringTableExtractor`
matches only one-, two- and three-part names, so a four-part name throws
`MatchError`;
`CommandSpec` catches `case e: Exception`, logs at `debug`, and returns
`None`. The table
therefore carries no access request and the read is not authorized at
all. Iceberg
metadata tables expose table history, manifest listings and file paths.
`TableTableExtractor` now reflectively unwraps `BaseMetadataTable` to its
base table, so
the read is authorized as a read of the table it exposes.
2. **Iceberg `MERGE INTO` skipped its read of the target.** The rewrite
embeds an
already-planned `DataSourceV2ScanRelation` for that read, which the plan
builder passed
over silently. It now has its own scan spec.
`TableTableExtractor` is byte-identical in `v1.10.3`, `v1.11.1` and current
`master`, so both
gaps are present in released versions, not just on master.
The main change adds `spark.kyuubi.authz.unclassifiedNode.behavior` (`allow`
| `warn` |
`deny`, default `warn`) so an operator can choose to fail closed on an
unclassified node,
plus the build-time accounting that makes the classification set auditable
rather than
assumed: every concrete `LogicalPlan` descendant on the classpath is placed
in exactly one of
four buckets — spec'd, allowlisted, pass-through, or a per-Spark-minor
backlog file that is
checked in and reviewed at PR time.
Two design points worth reviewer attention:
- Allowlist entries require a written `reason`. An entry grants silence, so
it should record a
reviewed decision rather than a reflexive silencing.
- Every entry names the exact Spark `major.minor` versions its review
covers, as an explicit
enumeration rather than a range. Ranges invite boundary misreadings ("less
than 4.0,
exclusive" read as inclusive); an enumeration has no boundary to misread
and makes a new
Spark minor unverified by default, which is exactly when re-review is due.
Allowlist entries
*gate* on this — unverified means inert, so the node counts as
unclassified and fails closed.
Command and scan specs are advisory, because a spec imposes checks and
staying active on an
unaudited version is the safe direction.
Specs added before the Spark 4 port take their versions from a frozen ledger
(`spec_verified_spark_versions.txt`) that records an inherited `3.3/3.4/3.5`
baseline rather
than a per-minor audit; the ledger header says so plainly and is closed, so
a new spec must
declare its own versions rather than inherit that baseline.
`docs/paranoid-mode.md` carries the full design, including the known
limitation that
`RuleAuthorization` is an optimizer rule and so cannot stop an
`ExecutableDuringAnalysis` node
(Spark 4 `CALL`) that has already run during analysis — which is what the
build-time check
exists to cover.
Default behaviour is unchanged: `warn` logs and proceeds.
### How was this patch tested?
`ParanoidModeSuite` (15 tests) covers the policy: the `warn` default,
case-insensitive
parsing, an invalid value rejected loudly, and each behaviour end to end —
`deny` failing
closed on an unclassified leaf relation and on an unclassified command,
`warn` passing but
counting every occurrence, `allow` passing silently. It also covers the
shapes that made this
worth doing: an unclassified node cannot hide under a constant projection,
scan spec
extraction failures are surfaced rather than swallowed, allowlisted nodes
still pass under
`deny`, and ordinary multi-operator queries recurse freely. Allowlist
hygiene is tested too —
entries require a reason and explicitly enumerated versions rather than
ranges, and an entry
applies only to the Spark minors it was reviewed against.
`ClassificationCoverageSuite` (5 tests) does the build-time accounting:
every plan node class
on the classpath is accounted for, every command spec entry is routable to
`buildCommand`,
allowlisted classes are not Commands in disguise, allowlist entries are
shapes the classifier
would actually flag (so an entry for a pass-through shape is rejected as
dead weight that
reads as coverage), and no class has both a command spec and an allowlist
entry.
For the two fail-opens:
- The Iceberg metadata table gap has a dedicated regression test, `selecting
an Iceberg
metadata table requires select on the base table`, asserting both
directions — denied for an
unprivileged user, allowed for one with select on the base table. It fails
without the
`TableTableExtractor` change.
- The `MERGE INTO` gap has no new test of its own; it is covered by the
existing
`[KYUUBI #3515] MERGE INTO` test now running under `deny`. Without the new
`DataSourceV2ScanRelation` scan spec the embedded relation is
unclassified, so that test
fails. This holds on the 3.5, 4.0 and 4.1 profiles; Iceberg is
tag-excluded on 4.2.
The ledger's two guards — a spec that neither declares
`verifiedSparkVersions` nor appears in
the ledger, and a ledger entry whose spec no longer exists — are assertions
inside the existing
`check spec json files` generator test rather than standalone tests. Both
fail generation.
Existing tests: all suites now run with sessions in `deny` mode
(`SparkSessionProvider`), so
any unclassified node reached by an existing test fails that test rather
than passing
silently. That is the main verification vehicle for the classification set,
and it is how both
fail-opens above were found.
```bash
# full module, default profile (Spark 3.5)
build/mvn test -pl extensions/spark/kyuubi-spark-authz -am
# the new suites alone
build/mvn test -pl extensions/spark/kyuubi-spark-authz -am -Dtest=none \
-DwildcardSuites=org.apache.kyuubi.plugin.spark.authz.ParanoidModeSuite,org.apache.kyuubi.plugin.spark.authz.ClassificationCoverageSuite
# other Spark profiles
build/mvn test -pl extensions/spark/kyuubi-spark-authz -am -Pspark-4.0
-Pscala-2.13
build/mvn test -pl extensions/spark/kyuubi-spark-authz -am -Pspark-4.1
-Pscala-2.13
build/mvn test -pl extensions/spark/kyuubi-spark-authz -am -Pspark-4.2
-Pscala-2.13
# regenerate the spec JSON and backlog files after changing a spec
dev/gen/gen_ranger_spec_json.sh
```
Results: the default profile passes on this branch (697 tests). The one
failure in that run is
unrelated to this change and reproduces on `master` — Hudi's embedded
timeline server timing
out to a LAN address in
`DeleteHoodieTableCommand/UpdateHoodieTableCommand/MergeIntoHoodieTableCommand`.
The 4.0, 4.1
and 4.2 profiles were green in `deny` mode before this branch was rebased
onto current master
and are being re-run against it; I will confirm here before asking for a
merge.
### Was this patch authored or co-authored using generative AI tooling?
Assisted-by: Claude:claude-opus-5
Used for code review, rebasing onto master, and drafting; every line
reviewed by me.
--
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]
