Skip to content

feat(auth): authorize query-auth reads and carry the grant on the split - #758

Open
plusplusjiajia wants to merge 3 commits into
apache:mainfrom
plusplusjiajia:query-auth-carry-grant
Open

feat(auth): authorize query-auth reads and carry the grant on the split#758
plusplusjiajia wants to merge 3 commits into
apache:mainfrom
plusplusjiajia:query-auth-carry-grant

Conversation

@plusplusjiajia

@plusplusjiajia plusplusjiajia commented Aug 27, 2026

Copy link
Copy Markdown
Member

Purpose

A query-auth.enabled table makes the server return a per-user row filter and column masking that the client is expected to apply. This client cannot apply them yet, so it refuses to read such a table at all — even for a user the server reports as unrestricted. This slice fetches the authorization at scan-plan time and carries it to the read, so that user can read. A user with rules gets the same refusal as before.

Brief change log

TableScan::plan authorizes once and stamps the result on every split, as Java wraps each split in a QueryAuthSplit. TableRead::to_arrow then decides from the splits: each must carry a grant, from this handle, and unrestricted.

Whether a table is query-auth comes from the server, not the loaded handle — the option can be set after a load, and a cached false would skip authorization entirely. Fourteen sites ask, including the searches, index builds and rewrites that read files without planning. Sync read boundaries cannot ask, so they read a marker the splits carry; it survives serialization, so a round-tripped split that lost its grant fails closed.

Five refusals are deliberate: a restricted grant, at planning, since a plan carries row counts and min/max that answer COUNT/MIN/MAX without reading a row; a time-travelled, branch or decorated (db.t$branch_x, db.t$files) handle, which reads files the server did not rule on; a field that is not an exact (id, name, type) match, catching an older shape with an extra nested child; a data file whose statistics name a column the current schema lacks, since value_stats is public on every split; and a read naming a reserved system column, which the server's own check would reject.

The request names no columns, which the server expands to the real schema fields — Java sends the read type's names, so a user authorized for only a subset is still refused here. AuthTableQueryResponse rejects unknown fields: an absent one reads as "no rule", so protocol drift would look like an unrestricted grant.

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 unwrapQueryAuthSplit returns 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() and DataSplit::data_file_path() are public, so the same caller can read the files directly without going through TableRead.

@plusplusjiajia
plusplusjiajia force-pushed the query-auth-carry-grant branch 12 times, most recently from f67b381 to 96e1832 Compare August 30, 2026 13:13
@plusplusjiajia
plusplusjiajia marked this pull request as ready for review August 30, 2026 14:20

/// 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> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread crates/paimon/src/table/query_auth.rs Outdated
}
let canonical = schema_fields
.iter()
.any(|f| f.id() == field.id() && f.name() == field.name());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@plusplusjiajia
plusplusjiajia force-pushed the query-auth-carry-grant branch 2 times, most recently from 464a3d4 to f7dfebf Compare September 1, 2026 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants