feat(auth): authorize query-auth reads and carry the grant on the split - #758
plusplusjiajia wants to merge 24 commits into
Conversation
f67b381 to
96e1832
Compare
|
|
||
| /// Whether the server says this table is `query-auth.enabled` right now: the | ||
| /// handle's schema is a snapshot, and a cached `false` would skip the check. | ||
| pub(crate) async fn server_query_auth_enabled(&self) -> Result<bool> { |
There was a problem hiding this comment.
[P1] Apply the live server check to direct search APIs too
This helper closes the stale-handle gap for TableScan, but the direct scored/search entry points still call only CoreOptions::ensure_read_authorized() on the schema cached when the handle was loaded. In particular, BatchVectorSearchBuilder::execute reads the snapshot/index manifest directly, and VectorSearchBuilder::execute_scored, FullTextSearchBuilder::execute_scored, and HybridSearchBuilder::execute_scored reach those direct paths without an authorized TableScan.
Therefore: load a REST table while query auth is false, enable restricted query auth on the server, then reuse the handle for one of these searches. The cached guard passes and row IDs/scores derived from protected data are returned without the auth exchange. Please route every out-of-band search entry through this async server-state check and reject when query auth is enabled (these paths cannot apply masking/filtering), with stale-handle regressions analogous to the new scan test.
There was a problem hiding this comment.
@JingsongLi Good catch — real hole. Fixed and widened past the searches: fourteen sites now ask the server via Table::ensure_read_authorized_live, and delegated searches ask once rather than once per route.
| } | ||
| let canonical = schema_fields | ||
| .iter() | ||
| .any(|f| f.id() == field.id() && f.name() == field.name()); |
There was a problem hiding this comment.
[P1] Validate the full nested field shape, not only the top-level pair
Both this guard and the old-file check above compare only the top-level (id, name). That leaves a concrete disclosure path after nested schema evolution: suppose the authorized current schema contains profile ROW<public>, while a live older file has the same top-level field id/name but profile ROW<public, secret>. A caller can use the public ReadBuilder::with_read_type with that old Row type; this check passes, and data_file_reader::prune_data_type recursively selects the requested old child by id, so profile.secret is decoded even though it is absent from the schema/column set the server authorized. The planning check at lines 87-92 also passes the old file for the same reason.
Please validate canonical fields recursively (including Row children and nested Array/Map/Multiset element types, allowing only explicitly safe evolution), and make the old-file containment check recursive too. An end-to-end test with a dropped nested field and a crafted old read type should be rejected.
There was a problem hiding this comment.
@JingsongLi Good catch. Both checks now compare id, name and full data type; DataType derives structural equality, so nested shapes are covered recursively. End-to-end test as you asked: a Row column read with a read type that keeps the authorized (id, name) but carries an extra nested child — refused.
| core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; | ||
| // The handle's flag is a snapshot; the marker survives a round-trip. | ||
| let required = core_options.query_auth_enabled() | ||
| || data_splits.iter().any(|s| s.query_auth_required()); |
There was a problem hiding this comment.
[P1] Do not accept an unmarked split from a stale REST handle
When a REST handle was loaded while query auth was false, core_options.query_auth_enabled() stays false. If the server later enables restricted query auth, a caller that bypasses TableScan can still pass an old unmarked plan—or a split made through the public DataSplitBuilder—to this synchronous to_arrow boundary. required is false, so the method returns rows without any auth RPC or grant. This contradicts the PR’s stated stale-handle guarantee; the new test covers planning after the toggle, but not this public read path.
Please make REST reads require evidence that the split was planned after a live server-state check even when that check said query auth was disabled (for example, a catalog-session-bound checked-plan capability distinct from query_auth_required), or make materialization able to re-authorize. Unmarked splits should remain acceptable only where no REST authorization boundary exists. Add a regression for: load with false, retain/build an unmarked split, enable a restricted response, then call to_arrow and require refusal.
There was a problem hiding this comment.
@JingsongLi The gap is real, but I could not find a fix worth its cost, so I documented it here and in the PR description.
I implemented your suggestion first. It broke four legitimate flows on ordinary, non-query-auth REST tables — including DataFusion's register_cow_target_table and the documented C paimon_plan_from_split_bytes round trip — because it required enumerating every path that rebuilds a planned split, and I missed several. It also proved nothing: DataSplit is publicly serde-serializable, so {query_auth_checked: true, query_auth_required: false} skips both /auth and grant validation.
And closing it buys little: Table::file_io() and DataSplit::data_file_path() are public, so the same caller can read the bytes without touching TableRead. A real boundary means binding data tokens to an authorization epoch at the data plane — server-side work not available to us. Java stops here too.
So the contract is plan-time: callers must re-plan after an authorization change. A split from a query-auth table still fails closed if it lost its grant.
Do you have a better idea?
f7dfebf to
4477349
Compare
| ) -> Result<()> { | ||
| // A commit validates against the existing snapshot. | ||
| CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; | ||
| self.table.ensure_read_authorized_live("a commit").await?; |
There was a problem hiding this comment.
[P1] Re-authorize before the writer performs lazy reads
This live check runs only when commit starts, but TableWrite has already read table state by then. TableWrite::new still checks only the schema cached on the handle; on the first normal PK write, create_kv_writer calls scan_partition_sequence_numbers, which reads the latest snapshot/manifests, and a dynamic-bucket write additionally runs DynamicBucketAssigner::ensure_index_entries_loaded plus HashIndexFile::read. Thus a handle loaded while query-auth.enabled=false can be reused after the server enables restricted query auth: write_arrow_batch and prepare_commit read protected metadata/index contents before this check eventually rejects the commit. The dynamic path can even emit a replacement hash-index file containing hashes restored from the protected index, so rejecting only at commit does not undo the disclosure. Please put a live authorization check before the first async writer read (or add an auth-aware async initialization) and cover the stale-handle write path.
There was a problem hiding this comment.
@JingsongLi You're right that the commit check comes too late. write_arrow_batch and prepare_commit now ask before the writer's first lazy read, so the snapshot scan and the hash-index load sit behind it.
| if local { | ||
| return Ok(true); | ||
| } | ||
| match rest_env.current_table().await?.schema.as_ref() { |
There was a problem hiding this comment.
[P1] Do not trust a disabled answer from a replacement table
When the cached schema is false, this accepts the current name’s schema without checking that it still has rest_env’s UUID. A stale handle can therefore miss the very transition this helper is meant to catch: load table A with auth disabled, enable restricted query auth for A, then drop/re-create the name as table B with auth disabled. current_table() now reports B’s false, ensure_read_authorized_live succeeds, and direct vector/full-text/index paths read A’s still-reachable files through the stale handle without any auth exchange. The comment says the live answer only strengthens, but a false from a different UUID does not constrain A at all. Please validate the UUID before accepting a live false (schema freshness can remain a separate policy) and add a stale-false/recreated-table regression.
There was a problem hiding this comment.
@JingsongLi Agreed — a bare false proved nothing. It is now trusted only from the uuid this handle was loaded with; a different one errors and asks for a re-load, a missing one reads as true.
| if local { | ||
| return Ok(true); | ||
| } | ||
| match rest_env.current_table().await?.schema.as_ref() { |
There was a problem hiding this comment.
[P1] Check the live branch schema, not the base-table name
For a table obtained through copy_with_branch, self.schema and the managers point at the branch, but rest_env.current_table() still queries the base identifier stored when the original handle was loaded. If the branch handle cached query-auth.enabled=false, the branch is later changed to restricted auth, and the base schema remains false, this method returns false; authorize_read(false) then exits before its branch_reference refusal and the scan reads the branch without an auth exchange. The current branch tests only start with a locally true option, so they do not cover this stale-false case. Please query branch_identifier(self.branch()) for live state (or conservatively refuse REST branch handles) and add a branch-specific toggle regression.
There was a problem hiding this comment.
@JingsongLi This was asking about the wrong table. It now asks about the branch, as db.t$branch_x, built from the base name so a handle already loaded as a branch doesn't double the decoration, with main mapping back.
2cc1bd2 to
3efb6fa
Compare
…leting committed files
c3c3199 to
4b40336
Compare
…leave selector validation to planning
Resolve the table write and commit overlaps while preserving live query authorization checks.
…served columns, not only the schema id
…d read from its splits
…cks beside the new scan validations
|
[P1] Validate the loaded base table UUID before accepting a branch auth-off response (crates/paimon/src/table/rest_env.rs:118). A branch copied from base A retains A's location and snapshot managers. If A enables restricted query auth and the REST name is recreated as B at a new path with branch auth off, this early return trusts B's false without checking the original base UUID. The stale handle can then plan A's old branch files without a grant. A focused mock-server regression changing the base UUID and branch response failed as expected: the branch plan succeeded when it should have refused. The branch endpoint may have a distinct UUID; checking the base identifier against RESTEnv's stored UUID before trusting branch false avoids rejecting legitimate branches. |
…still resolves to the loaded table
| } | ||
|
|
||
| /// The caller already asked the server for this operation. | ||
| pub(crate) fn assume_authorized(mut self) -> Self { |
There was a problem hiding this comment.
@JingsongLi They only saved hybrid search a second get_table. Gone now, together with the live check itself, so these builders are back to main's code.
|
This PR touches too many parts of the code; it adds a lot of public methods rather than simply modifying the implementation, so it's hard for me to judge whether the approach is reasonable. |
…w public method, boxed planning, and partition row counts under query-auth
1a91e6b to
7cc2f10
Compare
…s answer get_table with the schema and uuid, and filter_and_commit asks the server
3bc1839 to
ae995f3
Compare
…, and drop the live query-auth re-check
@JingsongLi Fair. The latest push trims it to 19 files: no new public method (the rest is |
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed head 3c7b1694 for the full REST scan/read path and alternate read entry points. This has end-to-end value: a REST user with an unrestricted query-auth response can now plan and read real rows, while restricted responses fail closed.
[P2] Literal REST table names containing $ become unreadable. RESTCatalog::get_table and load_table now call identifier.reject_decorated() unconditionally (crates/paimon/src/catalog/rest/rest_catalog.rs:277,289). create_table still accepts a literal name such as orders$files (the identifier validator permits $, and the server stores the submitted name). I reproduced the create-then-read path in a temporary REST integration test: creation succeeded, but get_table failed with Unsupported: 'default.orders$files' is a decorated name. This affects ordinary tables with query auth disabled too. Please make create/load semantics consistent and keep the protected-view refusal without breaking previously readable literal names.
The PR description also needs updating before merge. It says query-auth enablement is read live from the server for each entry point and advertises a new public Table::ensure_read_authorized method. The current code uses the option cached on the loaded handle and no longer exposes that method. This matters to the documented behavior of old handles after authorization changes.
Validation: cargo test -p paimon --test rest_catalog_test query_auth (15 passed), cargo test -p paimon --lib query_auth (29 passed), and the REST-server decorated-object test (1 passed). The head CI is green. In cargo test -p paimon-datafusion partition_count, the two matching unit tests passed; a separate read_tables integration test then failed because its default.partitioned_log_table fixture was missing in this local checkout, so that run does not validate the DataFusion fallback end to end. The temporary regression test failed as described above and was removed after verification.
…w is still refused where the grant is decided
@JingsongLi You're right. That rejection only served the live check, which is gone, so |
|
Re-reviewed head Local validation: 15/15 REST query-auth integration tests, 29/29 matching core unit tests, and 14/14 DataFusion partition-count tests passed; formatting and diff checks passed. All 14 CI checks are green. The first local REST run failed only because the sandbox denied the mock server's socket bind; the same suite passed when run with socket permission. Integration follow-up before merge: this head currently conflicts with main in |
|
Merged main; REST auth and DataFusion fallback suites are green on the merged tree. |
…rant and adds main's data-directory validation
2 participants
Purpose
A
query-auth.enabledtable makes the server return a per-user row filter and column masking that the client is expected to apply. This client cannot, so it refuses to read such a table at all — even for a user the server reports as unrestricted. This slice authorizes at scan-plan time and carries the result to the read, so that user can read. A user with rules is refused as before.Brief change log
TableScan::planauthorizes once and stamps every split, as Java wraps each one in aQueryAuthSplit;to_arrowthen requires every split to carry an unrestricted grant issued to this handle. Whether a table is query-auth is the option loaded with the handle, as in Java; a change on the server shows after a re-load. DataFusion'sCOUNT(*)pushdown takes the manifest count's refusal as its cue to fall back to an unpinned scan, which carries the grant.TableScan::planboxes the planning future: engines poll it under deep operator stacks, and that nested fallback ran out of a 2 MB test stack otherwise.Five refusals are deliberate: a restricted grant, at planning, since a plan already carries row counts and bounds; a time-travelled, branch or decorated handle, which reads files the server did not rule on; a read type the current schema does not contain, catching an older nested shape while a projection still reads; old-file statistics for a dropped column; and a reserved system column, which the server's own check would reject.
Known limitation
Authorization is a plan-time decision and a plan is the capability recording it. A split kept from before the option was enabled, or built by hand, is read on the caller's word — as in Java, where
unwrapQueryAuthSplitreturns no result for a plain split. Callers must re-plan after an authorization change. A read-boundary guard could not change this:Table::file_io()andDataSplit::data_file_path()are public.Divergence from Java
Java serializes the auth result with the split; here it is runtime-only, so plan and read must share a process. Java's
QueryAuthSplittrusts whatever it is handed, butto_arrow,Table::newandRESTEnvare public here, so the grant records a session only the catalog mints, and the auth call and the planning that follows are bracketed by a freshness check on the uuid, the schema id and the schema's fields, so a handle carrying other columns under the same id is refused.Tests
A unit test per guard, and mock-server tests for the outcomes: an unrestricted user reads real rows, a restricted one is refused at planning, and a re-created, evolved, assembled or decorated handle is refused.
API and Format
No format change and no new public method.
ReadBuilder::new_readrefuses at construction only a handle no catalog loaded — it can never hold a grant — and otherwise decides atto_arrow, so a loaded handle's empty plan reads as an empty result. An ordinary table costs nothing extra; planning a query-auth table costs four round-trips.