You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "plusplusjiajia (via GitHub)" <gi...@apache.org> on 2026/08/27 15:51:07 UTC
[PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
plusplusjiajia opened a new pull request, #758:
URL: https://github.com/apache/paimon-rust/pull/758
### Purpose
A `query-auth.enabled` table makes the server return a per-user row filter and column masking that a 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 first 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
`Table::authorize_read` asks the server once per plan, and `TableScan::plan` stamps the result on every split — Java wraps each split in a `QueryAuthSplit` for the same reason. `TableRead::to_arrow` then
decides from the splits: each must carry a grant, matching this table, and unrestricted. Per split, not just the first, since split lists can be concatenated across plans.
`QueryAuthGrant` keeps the response unparsed — parsing belongs with the code that applies it — and the request names every column, ordinary and system alike, since "nothing is restricted" is only sound if the server was asked about everything. A user authorized for a subset is therefore still refused, as today.
Two refusals are deliberate: a restricted grant fails at planning, since a plan carries row counts and min/max that engines answer `COUNT`/`MIN`/`MAX` from without reading a row; and a time-travelled or branch copy is refused, since the server rules on the current schema. The `ReadBuilder::new_read` gate is removed — it runs before any split exists — and the engine-served type check it carried is restored at the plan and read boundaries.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3906430875
##########
crates/paimon/src/table/table_read.rs:
##########
@@ -712,12 +737,68 @@ impl<'a> PaimonTableRead<'a> {
reader.read(splits)
}
+ /// Allowed only if the splits carry a grant saying the server imposed
+ /// nothing. Never fetched here, so a split without one fails closed.
+ fn ensure_authorized_by_splits(
+ &self,
+ core_options: &CoreOptions,
+ data_splits: &[DataSplit],
+ ) -> crate::Result<()> {
+ // Unconditional: unrelated to query-auth.
+ 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());
Review 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?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3930389053
##########
crates/paimon/src/table/table_commit.rs:
##########
@@ -205,7 +205,7 @@ impl TableCommit {
filter_committed: bool,
) -> 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?;
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3893629966
##########
crates/paimon/src/table/table_read.rs:
##########
@@ -712,12 +737,68 @@ impl<'a> PaimonTableRead<'a> {
reader.read(splits)
}
+ /// Allowed only if the splits carry a grant saying the server imposed
+ /// nothing. Never fetched here, so a split without one fails closed.
+ fn ensure_authorized_by_splits(
+ &self,
+ core_options: &CoreOptions,
+ data_splits: &[DataSplit],
+ ) -> crate::Result<()> {
+ // Unconditional: unrelated to query-auth.
+ 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());
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5831788703
Re-reviewed head `62823d6e` after the author's reply. Requirement fit: SUPPORTED. The earlier literal-`$` table-name regression is fixed: `RESTCatalog` no longer rejects the name during load, and the new test covers both a plain literal name and refusal of a protected decorated view. I traced the grant from REST authorization through scan planning, split marking, and read materialization, including the restricted-response, nested-field, stale-handle, and count-pushdown paths. I found no new actionable code finding on this 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 `crates/paimon/src/table/rest_env.rs` and `crates/paimon/tests/mock_server.rs` (`git merge-tree --write-tree main HEAD`). Please rebase and rerun the REST authorization and DataFusion fallback suites on the resolved result. The tests above validate this PR head, not a conflict resolution. The documented plan-time authorization limitation remains part of the proposed contract and should be stated prominently for production callers that retain plans across permission changes.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3906381530
##########
crates/paimon/src/table/mod.rs:
##########
@@ -315,6 +321,97 @@ impl Table {
}
}
+ /// 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> {
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5772131839
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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3930394409
##########
crates/paimon/src/table/mod.rs:
##########
@@ -322,6 +328,108 @@ impl Table {
}
}
+ /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which
+ /// reads the schema this handle was loaded with.
+ pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> {
+ let local = CoreOptions::new(self.schema.options());
+ local.ensure_type_paimon_served(&self.identifier.full_name())?;
+ if self.server_query_auth_enabled().await? {
+ return Err(query_auth::unsupported(&format!(
+ "{path} reads index files directly and cannot apply a row filter or column masking"
+ )));
+ }
+ Ok(())
+ }
+
+ /// 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> {
+ let local = CoreOptions::new(self.schema.options()).query_auth_enabled();
+ let Some(rest_env) = &self.rest_env else {
+ return Ok(local);
+ };
+ // Only ever strengthens: the name can be re-created over this handle's
+ // files, so the answer may be about a different table.
+ if local {
+ return Ok(true);
+ }
+ match rest_env.current_table().await?.schema.as_ref() {
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5779257309
> 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.
@JingsongLi Fair. The latest push trims it to 19 files: no new public method (the rest is `pub(crate)`), the `assume_authorized` flags, the `FileSystemCatalog` and bundled-server changes and the strict auth-response parsing are gone, and so is the live re-check of `query-auth.enabled` your two P1s hardened — the handle now trusts the option it was loaded with, as Java does, so an ordinary table costs nothing. What remains: the grant is fetched at planning and rides on the split.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3995309182
##########
crates/paimon/src/table/mod.rs:
##########
@@ -322,6 +328,108 @@ impl Table {
}
}
+ /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which
+ /// reads the schema this handle was loaded with.
+ pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> {
+ let local = CoreOptions::new(self.schema.options());
+ local.ensure_type_paimon_served(&self.identifier.full_name())?;
+ if self.server_query_auth_enabled().await? {
+ return Err(query_auth::unsupported(&format!(
+ "{path} reads index files directly and cannot apply a row filter or column masking"
+ )));
+ }
+ Ok(())
+ }
+
+ /// 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> {
+ let local = CoreOptions::new(self.schema.options()).query_auth_enabled();
+ let Some(rest_env) = &self.rest_env else {
+ return Ok(local);
+ };
+ // Only ever strengthens: the name can be re-created over this handle's
+ // files, so the answer may be about a different table.
+ if local {
+ return Ok(true);
+ }
+ match rest_env.current_table().await?.schema.as_ref() {
Review 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.
##########
crates/paimon/src/table/mod.rs:
##########
@@ -322,6 +328,108 @@ impl Table {
}
}
+ /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which
+ /// reads the schema this handle was loaded with.
+ pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> {
+ let local = CoreOptions::new(self.schema.options());
+ local.ensure_type_paimon_served(&self.identifier.full_name())?;
+ if self.server_query_auth_enabled().await? {
+ return Err(query_auth::unsupported(&format!(
+ "{path} reads index files directly and cannot apply a row filter or column masking"
+ )));
+ }
+ Ok(())
+ }
+
+ /// 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> {
+ let local = CoreOptions::new(self.schema.options()).query_auth_enabled();
+ let Some(rest_env) = &self.rest_env else {
+ return Ok(local);
+ };
+ // Only ever strengthens: the name can be re-created over this handle's
+ // files, so the answer may be about a different table.
+ if local {
+ return Ok(true);
+ }
+ match rest_env.current_table().await?.schema.as_ref() {
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3893614229
##########
crates/paimon/src/table/mod.rs:
##########
@@ -315,6 +321,97 @@ impl Table {
}
}
+ /// 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> {
Review 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.
##########
crates/paimon/src/table/query_auth.rs:
##########
@@ -0,0 +1,320 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! What the REST server authorized a user to read from one table.
+
+use crate::api::AuthTableQueryResponse;
+
+/// The server's answer for one user on one table, kept unparsed.
+///
+/// `session` pins it to the handle that asked: `to_arrow` is public and the
+/// response names neither table nor principal. Java binds nothing. Routing
+/// options are unbound on purpose — sound only while unrestricted grants
+/// authorize.
+#[derive(Debug, PartialEq)]
+pub(crate) struct QueryAuthGrant {
+ response: AuthTableQueryResponse,
+ session: u64,
+}
+
+impl QueryAuthGrant {
+ pub(crate) fn new(response: AuthTableQueryResponse, session: u64) -> Self {
+ Self { response, session }
+ }
+
+ /// The only case this client can serve.
+ pub(crate) fn is_unrestricted(&self) -> bool {
+ self.response.is_unrestricted()
+ }
+
+ /// Travelled and branch views read a schema the server did not rule on.
+ /// Everything else follows from the session, which only the catalog mints.
+ pub(crate) fn matches_table(&self, table: &super::Table) -> bool {
+ !table.is_time_traveled()
+ && !table.is_branch_reference()
+ && table.query_auth_session() == Some(self.session)
+ }
+}
+
+/// `value_stats` and `write_cols` are public on every split, and an older file
+/// can name a since-dropped column the server never ruled on. Refused rather
+/// than scrubbed: rewriting encoded stats is how bounds get mismatched.
+pub(crate) async fn reject_unauthorized_stats(
+ plan: &super::Plan,
+ current: &crate::spec::TableSchema,
+ schemas: &super::schema_manager::SchemaManager,
+) -> crate::Result<()> {
+ let refuse = |column: &str| {
+ Err(unsupported(&format!(
+ "a data file still carries statistics for '{column}', which the current schema — the \
+ one the server authorized — does not have"
+ )))
+ };
+ let named = |name: &String| current.fields().iter().any(|f| f.name() == name);
+ let mut checked = std::collections::HashSet::new();
+ for split in plan.splits() {
+ for file in split.data_files() {
+ for column in file
+ .value_stats_cols
+ .iter()
+ .chain(file.write_cols.iter())
+ .flatten()
+ {
+ if !named(column) {
+ return refuse(column);
+ }
+ }
+ // The file's own schema is the authority: a name can be dropped and
+ // re-added under a new id, and the lists may be absent entirely.
+ if file.schema_id == current.id() || !checked.insert(file.schema_id) {
+ continue;
+ }
+ let older = schemas.schema(file.schema_id).await?;
+ if let Some(gone) = older.fields().iter().find(|f| {
+ !current
+ .fields()
+ .iter()
+ .any(|c| c.id() == f.id() && c.name() == f.name())
+ }) {
+ return refuse(gone.name());
+ }
+ }
+ }
+ Ok(())
+}
+
+/// A refusal naming the option, so callers never match on prose.
+pub(crate) fn unsupported(reason: &str) -> crate::Error {
+ crate::Error::Unsupported {
+ message: format!(
+ "reading a table with 'query-auth.enabled' = true is not supported: {reason}"
+ ),
+ }
+}
+
+/// Column permissions cover real schema fields, so the server can neither grant
+/// nor refuse `_ROW_ID` and friends.
+pub(crate) fn reject_system_columns<'a>(
+ names: impl IntoIterator<Item = &'a str>,
+) -> crate::Result<()> {
+ for name in names {
+ if crate::spec::is_reserved_system_field_name(name) {
+ return Err(unsupported(&format!(
+ "the system column '{name}' is not one the server can authorize: column \
+ permissions are granted over table columns"
+ )));
+ }
+ }
+ Ok(())
+}
+
+/// The read resolves older files by field id, so a non-canonical `(id, name)`
+/// pair reads as something no grant covered. System fields have no entry.
+pub(crate) fn reject_noncanonical_fields(
+ read_type: &[crate::spec::DataField],
+ schema_fields: &[crate::spec::DataField],
+) -> crate::Result<()> {
+ for field in read_type {
+ if crate::spec::is_reserved_system_field_name(field.name()) {
+ continue;
+ }
+ let canonical = schema_fields
+ .iter()
+ .any(|f| f.id() == field.id() && f.name() == field.name());
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5834792109
Merged main; REST auth and DataFusion fallback suites are green on the merged tree.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5810391218
> 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.
@JingsongLi You're right. That rejection only served the live check, which is gone, so `reject_decorated` is dropped from `RESTCatalog::get_table`/`load_table` and `RESTEnv::build_table` — all three back to main's code, and a literal `orders$files` loads again. A query-auth table under such a name is still refused, but at planning: `authorize_read` treats a `$branch_x`/`$files` handle as reading a schema the server did not rule on. Test covers both: `plain$files` loads, `guarded$files` is refused when planned. Description updated.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3995308366
##########
crates/paimon/src/table/table_commit.rs:
##########
@@ -205,7 +205,7 @@ impl TableCommit {
filter_committed: bool,
) -> 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?;
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3930390747
##########
crates/paimon/src/table/mod.rs:
##########
@@ -322,6 +328,108 @@ impl Table {
}
}
+ /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which
+ /// reads the schema this handle was loaded with.
+ pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> {
+ let local = CoreOptions::new(self.schema.options());
+ local.ensure_type_paimon_served(&self.identifier.full_name())?;
+ if self.server_query_auth_enabled().await? {
+ return Err(query_auth::unsupported(&format!(
+ "{path} reads index files directly and cannot apply a row filter or column masking"
+ )));
+ }
+ Ok(())
+ }
+
+ /// 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> {
+ let local = CoreOptions::new(self.schema.options()).query_auth_enabled();
+ let Some(rest_env) = &self.rest_env else {
+ return Ok(local);
+ };
+ // Only ever strengthens: the name can be re-created over this handle's
+ // files, so the answer may be about a different table.
+ if local {
+ return Ok(true);
+ }
+ match rest_env.current_table().await?.schema.as_ref() {
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #758:
URL: https://github.com/apache/paimon-rust/pull/758
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r3906385535
##########
crates/paimon/src/table/query_auth.rs:
##########
@@ -0,0 +1,320 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! What the REST server authorized a user to read from one table.
+
+use crate::api::AuthTableQueryResponse;
+
+/// The server's answer for one user on one table, kept unparsed.
+///
+/// `session` pins it to the handle that asked: `to_arrow` is public and the
+/// response names neither table nor principal. Java binds nothing. Routing
+/// options are unbound on purpose — sound only while unrestricted grants
+/// authorize.
+#[derive(Debug, PartialEq)]
+pub(crate) struct QueryAuthGrant {
+ response: AuthTableQueryResponse,
+ session: u64,
+}
+
+impl QueryAuthGrant {
+ pub(crate) fn new(response: AuthTableQueryResponse, session: u64) -> Self {
+ Self { response, session }
+ }
+
+ /// The only case this client can serve.
+ pub(crate) fn is_unrestricted(&self) -> bool {
+ self.response.is_unrestricted()
+ }
+
+ /// Travelled and branch views read a schema the server did not rule on.
+ /// Everything else follows from the session, which only the catalog mints.
+ pub(crate) fn matches_table(&self, table: &super::Table) -> bool {
+ !table.is_time_traveled()
+ && !table.is_branch_reference()
+ && table.query_auth_session() == Some(self.session)
+ }
+}
+
+/// `value_stats` and `write_cols` are public on every split, and an older file
+/// can name a since-dropped column the server never ruled on. Refused rather
+/// than scrubbed: rewriting encoded stats is how bounds get mismatched.
+pub(crate) async fn reject_unauthorized_stats(
+ plan: &super::Plan,
+ current: &crate::spec::TableSchema,
+ schemas: &super::schema_manager::SchemaManager,
+) -> crate::Result<()> {
+ let refuse = |column: &str| {
+ Err(unsupported(&format!(
+ "a data file still carries statistics for '{column}', which the current schema — the \
+ one the server authorized — does not have"
+ )))
+ };
+ let named = |name: &String| current.fields().iter().any(|f| f.name() == name);
+ let mut checked = std::collections::HashSet::new();
+ for split in plan.splits() {
+ for file in split.data_files() {
+ for column in file
+ .value_stats_cols
+ .iter()
+ .chain(file.write_cols.iter())
+ .flatten()
+ {
+ if !named(column) {
+ return refuse(column);
+ }
+ }
+ // The file's own schema is the authority: a name can be dropped and
+ // re-added under a new id, and the lists may be absent entirely.
+ if file.schema_id == current.id() || !checked.insert(file.schema_id) {
+ continue;
+ }
+ let older = schemas.schema(file.schema_id).await?;
+ if let Some(gone) = older.fields().iter().find(|f| {
+ !current
+ .fields()
+ .iter()
+ .any(|c| c.id() == f.id() && c.name() == f.name())
+ }) {
+ return refuse(gone.name());
+ }
+ }
+ }
+ Ok(())
+}
+
+/// A refusal naming the option, so callers never match on prose.
+pub(crate) fn unsupported(reason: &str) -> crate::Error {
+ crate::Error::Unsupported {
+ message: format!(
+ "reading a table with 'query-auth.enabled' = true is not supported: {reason}"
+ ),
+ }
+}
+
+/// Column permissions cover real schema fields, so the server can neither grant
+/// nor refuse `_ROW_ID` and friends.
+pub(crate) fn reject_system_columns<'a>(
+ names: impl IntoIterator<Item = &'a str>,
+) -> crate::Result<()> {
+ for name in names {
+ if crate::spec::is_reserved_system_field_name(name) {
+ return Err(unsupported(&format!(
+ "the system column '{name}' is not one the server can authorize: column \
+ permissions are granted over table columns"
+ )));
+ }
+ }
+ Ok(())
+}
+
+/// The read resolves older files by field id, so a non-canonical `(id, name)`
+/// pair reads as something no grant covered. System fields have no entry.
+pub(crate) fn reject_noncanonical_fields(
+ read_type: &[crate::spec::DataField],
+ schema_fields: &[crate::spec::DataField],
+) -> crate::Result<()> {
+ for field in read_type {
+ if crate::spec::is_reserved_system_field_name(field.name()) {
+ continue;
+ }
+ let canonical = schema_fields
+ .iter()
+ .any(|f| f.id() == field.id() && f.name() == field.name());
Review 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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5771953279
[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.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r4068966294
##########
crates/paimon/src/table/vector_search_builder.rs:
##########
@@ -42,9 +44,16 @@ impl<'a> VectorSearchBuilder<'a> {
limit: None,
options: HashMap::new(),
filter: None,
+ authorized: false,
}
}
+ /// The caller already asked the server for this operation.
+ pub(crate) fn assume_authorized(mut self) -> Self {
Review Comment:
Why add these methods?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(auth): authorize query-auth reads and carry the grant on the split [paimon-rust]
Posted by "plusplusjiajia (via GitHub)" <gi...@apache.org>.
plusplusjiajia commented on code in PR #758:
URL: https://github.com/apache/paimon-rust/pull/758#discussion_r4073378204
##########
crates/paimon/src/table/vector_search_builder.rs:
##########
@@ -42,9 +44,16 @@ impl<'a> VectorSearchBuilder<'a> {
limit: None,
options: HashMap::new(),
filter: None,
+ authorized: false,
}
}
+ /// The caller already asked the server for this operation.
+ pub(crate) fn assume_authorized(mut self) -> Self {
Review 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 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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org