markhoerth opened a new issue, #12569: URL: https://github.com/apache/gravitino/issues/12569
### Version main branch ### Describe what's wrong `GravitinoCatalogManager` is a JVM-wide singleton. Its catalog cache is keyed on catalog name alone and is built with a bare `Caffeine.newBuilder().build()`, so it has no bound and no expiry: ```java // GravitinoCatalogManager private final Cache<String, Catalog> gravitinoCatalogs; ... // Will not evict catalog by default this.gravitinoCatalogs = Caffeine.newBuilder().build(); ... return gravitinoCatalogs.get(name, catalogName -> loadCatalog(catalogName)); ``` `BaseCatalog.initialize` calls `getGravitinoCatalogInfo(name)`, so every Spark session that touches a catalog goes through this cache. There are two problems with it, and they are of different kinds. **1. The entry never expires, so a revoked grant keeps being honoured. This is a live bug on any auth type.** `loadCatalog` is a Gravitino REST call that the server authorizes. Once it has succeeded for a catalog, the result is served for the remaining life of the driver and no further request reaches Gravitino. Revoking a user's access to that catalog in Gravitino therefore does not stop a running Spark application from continuing to use it. On a long-lived driver, a Spark Connect server or a Kyuubi engine, that window is unbounded. **2. The key does not include the caller. This is inert today and becomes a correctness problem the moment per-user credentials exist.** I want to be precise about the exposure here, because it is easy to overstate. On main there is exactly one `GravitinoClient` per driver, and all four auth types (`simple`, `basic`, `oauth2`, `kerberos`) resolve exactly one credential for the whole Spark application. There is never a second identity in play, so there is nothing to confuse and no deployment on 1.3 is affected. This is not a security disclosure. What makes it worth fixing now is that the cached value is not inert metadata, so the defect arms as soon as anything introduces a per-user credential. `GravitinoMetalake.loadCatalog` returns: ```java return DTOConverters.toCatalog(this.name(), resp.getCatalog(), restClient); ``` The `RELATIONAL` branch of `DTOConverters.toCatalog` passes that client to `RelationalCatalog.builder()...withRestClient(client)`, where it is stored as `BaseSchemaCatalog`'s `protected final RESTClient restClient`. `RelationalCatalog.asTableCatalog()` is `return this`, and `listTables` issues its request on that same field. The field is `final` and set at construction, so it is never re-resolved from a calling context. The consequence is that a cached `Catalog` carries the credential of whichever session populated the entry. If a second identity were ever served that entry, its requests would go to Gravitino under the first identity's credential and be authorized as that principal. I confirmed this on the wire rather than by reading, using the `client-java` MockServer harness. Two clients with distinct bearer tokens, a catalog loaded by A, handed to B through a plain map, then `listTables()` called by B: ``` listTables Authorization header = Bearer TOKEN-ALICE control (B via own client) = Bearer TOKEN-BOB ``` The control line matters: the same harness does emit B's token when B goes through its own client, so this is not a rig that only ever prints Alice. The two halves need fixing together. Adding an eviction policy without an identity-aware key still mixes callers once per-user credentials exist. Adding an identity-aware key without eviction still serves revoked grants. ### Error message and/or stacktrace There is none, and that is the substance of the report. Both problems fail by silently succeeding: an authorization check that should have run does not run, nothing is logged, and the query returns results. The closest observable relative is #10978, the inverse case on the Iceberg REST path, where an auth-sensitive cache with the wrong lifetime produced a spurious `ForbiddenException` after an idle period. Same family, opposite symptom. ### How to reproduce Problem 1, on main, with any auth type: 1. Start a long-lived Spark application with the Gravitino Spark connector configured against a Gravitino server. Anything where the driver outlives a single query, for example a Spark Connect server or a Kyuubi engine. 2. Run a query touching a table in catalog `c`, so that `GravitinoCatalogManager` caches `c`. 3. In Gravitino, revoke the user's privileges on catalog `c`. 4. Re-run the same query in the same Spark application. Expected: the query fails, because the user is no longer authorized on `c`. Actual: the query succeeds. The catalog is served from cache and no request reaches Gravitino. Problem 2 cannot be reproduced on main, because nothing there can give two sessions two identities. The MockServer probe quoted above is what demonstrates the underlying mechanism, at the level of which credential goes on the wire. ### Additional context The shape of a fix: - Cache the `GravitinoClient` per identity rather than holding a single one built at driver init, and close a client when it is evicted so it does not leak its HTTP connection pool. - Include that identity in the catalog cache key, and give the catalog cache a TTL. - Derive the identity from the credential itself and from nothing else. Taking it from `UserGroupInformation` or `SparkContext.sparkUser` risks the connector's cache key and the server's authorization decision disagreeing about who is asking, since the server derives its principal from the token. Two limits that a fix of this shape does not remove, worth stating up front: - The catalog list registered with Spark at driver startup is necessarily resolved with the application's identity, so a user may see a catalog name they are not then allowed to open. - This governs metadata resolution only. Executors read data with the credentials the underlying catalog was built with. Related: #11181 and its PR #11182 propose a bearer-token-file auth mode for the Spark connector. That is the kind of change that makes problem 2 live, which is why I would rather see the cache fixed alongside it than after it. I have commented on #11182 proposing that the two efforts merge. This issue is filed separately because the caching defects are independent of how a token is obtained, and problem 1 needs no token work at all to reproduce. I have a branch for both halves and will open a PR against this issue. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
