HIVE-29817: enforce read/list authorization at the Iceberg REST choke point - #6702
Open
henrib wants to merge 20 commits into
Open
HIVE-29817: enforce read/list authorization at the Iceberg REST choke point#6702henrib wants to merge 20 commits into
henrib wants to merge 20 commits into
Conversation
…e DB to ensure no stale table object is returned;
- improved check; - quiesce console logs due to internal throws in servlet;
… HMSCachingCatalog; - added l1 cache (default 3s / 32 entries) to reduce the latency for repeated access to the same table; - fix license header and addressed review comments;
…cache performance metrics; - remove end point to access cache performance metrics; - enhanced tests to check L1 cache; - simplified MetadataLocator exception handling;
The HMSCachingCatalog serves tables and access decisions out of an in-JVM
cache to avoid HMS round-trips. Caching the catalog this way silently
bypassed Ranger: once a Table object lived in the Caffeine cache, every
subsequent loadTable/dropTable/rename served it without re-consulting the
authorizer, so a user could read or mutate a table they were never granted.
Caching must never widen access. This change makes every table, view and
namespace operation go through an explicit per-request authorization check,
and caches the *decision* (not just the table) so enforcement stays cheap.
Authorization
- New HMSPrivilegeHelper interface: resolves an AccessLevel
(NONE / READ_ONLY / READ_WRITE) for a (db, table, user) or (db, user)
triple, with isAvailable() to report whether an authorizer is wired.
- New RangerPrivilegeHelper implementation calls the Hive authorizer's
showPrivileges API directly (no Thrift hop) and maps Ranger's Hive
access-type names onto AccessLevel:
* read (shared): SELECT, READ
* table/view write: UPDATE, WRITE, ALL (DML / data-plane)
* namespace write: CREATE, ALTER, DROP, ALL (DDL)
ALTER and DROP are DDL and are authorized at the namespace level, not
per-table. Ranger qualifiers (e.g. "SELECT(ACCESS_CONDITIONAL)") are
stripped before matching.
- Fail-closed by default: when no authorizer is configured the helper
returns NONE, so access is denied rather than open. Initialization
failures likewise degrade to NONE. Only when authorization is explicitly
disabled does the helper grant READ_WRITE.
- HMSCachingCatalog enforces READ_ONLY for load/list and READ_WRITE for
drop/rename/register/build on both tables and views, resolving the caller
from UserGroupInformation.getCurrentUser().
Decision caching and invalidation
- Access levels are held in a dedicated Caffeine cache keyed by
TableIdentifier, expiring on the same TTL as the table cache. Namespace
decisions use a synthetic TableIdentifier(namespace, "*") key that cannot
collide with a real table.
- Authorization entries are invalidated together with the object they guard:
table-level on invalidateTable, namespace-level on dropNamespace.
Catalog hardening
- HMSCachingCatalog is now final; its cache callbacks and logger are private.
It is instantiated only by HMSCatalogFactory.
- tableExists uses MetadataLocator (a null location means no table),
avoiding a full load.
Tests
- TestHMSCachingCatalogAuthz drives a StubPrivilegeHelper to assert the
access matrix (grant/deny per level), that decisions are cached, and that
cache invalidation re-checks authorization.
- Surefire runs with reuseForks=false in this module to isolate JVM-static
state (the metastore PMF and Iceberg's CachedClientPool) across classes.
- Fail-closed: don't override the privilege helper's NONE with READ_WRITE when !isAvailable(). - Authorize metadata tables (db.tbl.snapshots) against their base table (db.tbl), not a same-named decoy. - loadTable throws NoSuchTableException on a dropped table instead of serving the stale cached instance. - MetadataLocator.getLocation returns null (not throws) for a missing db/catalog, so null uniformly means not-found. - Guard L1 recency-guard writes so they no-op when L1 is disabled (empty map no longer throws). - Log JMX registration failure at error, not warn. - Fix class javadoc to the real MBean ObjectName and note catalog.name() == CATALOG_DEFAULT. - Tests: fail-closed denial, metadata-table authz, L1 disabled, dropped-table reload.
Roll back the AccessLevel-based authorization recently added to HMSCachingCatalog: remove the authz fields, methods, and per-operation guards, delete HMSPrivilegeHelper and RangerPrivilegeHelper, and drop the 3-arg constructor. L1/L2 caching, the JMX MBean, and dropped-table -> NoSuchTableException are unchanged. Per-operation authorization belongs in IcebergAuthorizer, which already does it right for stage-create; extending it to the other operations is deferred to a follow-up PR. Until then, writes and cache-miss reads are authorized by HMS and stage-create by IcebergAuthorizer; only cache-hit reads are unchecked at the catalog level, which the follow-up closes. Tests: replaced TestHMSCachingCatalogAuthz with TestHMSCachingCatalogCache (pure-cache cases, 2-arg constructor).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enforces read/list authorization for Iceberg REST catalog operations at the REST “choke point” (via HMSCatalogAdapter + IcebergAuthorizer) so cache hits and list operations don’t bypass authorization, and it adds/updates supporting tests.
Changes:
- Add explicit
QUERYauthorization forloadTable/loadViewand result-filtering forlistTables/listViews/listNamespacesin the REST adapter layer. - Introduce/extend caching infrastructure and observability (JMX MXBean), plus a Thrift-based metadata-location lookup helper.
- Add new cache-behavior/JMX integration tests and adjust Surefire for per-class JVM isolation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestIcebergAuthorizer.java | Adds unit tests for new read authorization and list filtering logic. |
| standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCachingCatalogStats.java | Adds JMX/integration tests validating cache counters and reset behavior. |
| standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/TestHMSCachingCatalogCache.java | Adds integration tests for cache invalidation/eviction and L1-disable behavior. |
| standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/extension/MockHiveAuthorizer.java | Updates mock authorizer to reflect list filtering semantics by user. |
| standalone-metastore/metastore-rest-catalog/src/test/java/org/apache/iceberg/rest/BaseRESTCatalogTests.java | Updates permission tests to expect filtered list results for denied users. |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/IcebergAuthorizer.java | Adds authorizeLoad* and list filtering helpers; refactors privilege-check plumbing. |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogServlet.java | Treats handled REST client errors differently in logging (debug vs error). |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogFactory.java | License header formatting update. |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCatalogAdapter.java | Enforces authorization on loads and filters list responses; minor refactors. |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalogMXBean.java | Adds MXBean interface for cache stats + reset operation. |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/rest/HMSCachingCatalog.java | Implements two-level caching + stats/JMX registration + invalidation behaviors. |
| standalone-metastore/metastore-rest-catalog/src/main/java/org/apache/iceberg/hive/MetadataLocator.java | Adds efficient metadata-location lookup for staleness detection. |
| standalone-metastore/metastore-rest-catalog/pom.xml | Configures Surefire to not reuse forks to avoid cross-class static state issues. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
henrib
force-pushed
the
HIVE-29817
branch
2 times, most recently
from
August 16, 2026 13:30
cf3757e to
ab33f44
Compare
Make the L1 recency guard access-ordered (LRU) so re-confirming a hot table moves it to the tail and the eldest evicted is the least-recently-used entry, not the least-recently-inserted one. Fix the MetadataLocator.getLocation javadoc, which claimed it returns null for non-metadata tables when it also serves base-table identifiers.
…aLocator Narrows the metadata-location lookup validation to only reject an Iceberg view loaded as a table (throwing NoSuchTableException), matching loadTable semantics, instead of rejecting any non-Iceberg-table object.
LongAdder scales better than AtomicLong under concurrent increments on the cache callback path. The debug log now reads the running total via sum(), guarded by isDebugEnabled() so the write path stays contention-free.
…ingCatalog Extract HMSCatalogFactory.createHiveCatalog so tests build catalogs through the production path, and have the server extension expose newServerCatalog / newCachingCatalog keyed off the metastore's real Thrift URI. Drop the static cacheRef SoftReference, getLatestCache, and the HIVE_IN_TEST hook from HMSCachingCatalog. Rework the caching cache/stats tests to drive their own catalog instance and assert counters via getters and JMX.
… point Authorize loadTable/loadView (QUERY) in HMSCatalogAdapter so cache-served reads authorize identically to HMS-served ones, and result-filter listTables/listViews/listNamespaces via filterListCmdObjects so users see only what they may read. Writes and stage-create authz unchanged.
…header HIVE-29755 restandardized the ASF header; bring the remaining metastore-rest-catalog files in line so checkstyle's header check passes.
…horizer Move the loadTable read check out of the adapter (where it double-authorized cold loads) into HMSCachingCatalog, enforced only on cache hits; misses reload through HMS and are authorized there. Drop the redundant loadView adapter check for the same reason (views are never cached). Memoize the per-thread authorizer toolkit so cache-hit checks avoid a full HiveConf clone and reflective lookups per call, extracted into newRequestAuthorizer, while refreshing identity per call for pooled threads.
Contributor
Author
|
Update: moved the |
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Enforces per-operation authorization for Iceberg REST catalog read/list operations, closing gaps left after HIVE-29035 reduced
HMSCachingCatalogto a pure cache:loadTable— theQUERYread check now lives in the caching catalog itself (HMSCachingCatalog), not the adapter. It runs only on a cache hit (L1 or L2), which never reaches HMS and would otherwise bypassReadTableEvent. A cache miss reloads through HMS and is authorized there, so a cold load is authorized exactly once. This makes the cache authz-aware rather than double-authorizing every load at the adapter.loadView— no adapter check: views are never cached, soloadViewalways reaches HMS, whose pre-event listener authorizes the read.listTables/listViews/listNamespaces— result filtering viaHiveAuthorizer.filterListCmdObjects(mirroringSHOW TABLES/SHOW DATABASES): a user sees only entries they may read; a fully-denied user gets an empty list rather than an error. HMS fires no pre-event authorization forget_tables/get_databases, so this closes a standing gap independent of caching.Because read authorization now runs on every cache hit, the per-thread
HiveAuthorizerbuilding blocks (clonedHiveConf, reflective factory/authenticator lookups) are memoized in aThreadLocal; only the identity-sensitivesetConf/createHiveAuthorizersteps run per call, so pooled Jetty threads bind the current request's identity without re-cloning the conf each time.Writes (create/drop/rename/register) remain authorized by HMS's
HiveMetaStoreAuthorizerpre-event listener, and stage-create authorization is unchanged.Why are the changes needed?
After HIVE-29035, cache hits and list operations were not authorized. This makes read/list authorization explicit and uniform, independent of whether an operation happens to reach HMS.
Does this PR introduce any user-facing change?
No behavioral change for authorized users. Unauthorized reads now consistently return
403 Forbidden, and list results are filtered to the caller's visible subset.How was this patch tested?
TestIcebergAuthorizer(unit, incl. filter cases),TestHMSCachingCatalogCache(cache-hit authz enforcement),TestRESTCatalogSimpleAuth, andTestRESTViewCatalogSimpleAuth(end-to-end permission enforcement through the REST path) all pass.