feat(auth): authorize query-auth reads and carry the grant on the split - #758
feat(auth): authorize query-auth reads and carry the grant on the split#758plusplusjiajia wants to merge 3 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?
464a3d4 to
f7dfebf
Compare
f7dfebf to
4477349
Compare
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
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, so the same caller can read the files directly without going throughTableRead.