Implement Finer Grained Operations and Privileges For Update Table#2697
Implement Finer Grained Operations and Privileges For Update Table#2697HonahX merged 19 commits intoapache:mainfrom
Conversation
b58ec0e to
96bf4cd
Compare
HonahX
left a comment
There was a problem hiding this comment.
Thanks for working on this! @travis-bowen
I am +1 on the idea of more fine grained commit actions. Left some initial comments. Let me know what you think!
.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
Outdated
Show resolved
Hide resolved
.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisPrivilege.java
Show resolved
Hide resolved
.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
Outdated
Show resolved
Hide resolved
9a9e60e to
b489b73
Compare
655f22a to
2b295d3
Compare
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java
Outdated
Show resolved
Hide resolved
...va/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerFineGrainedAuthzTest.java
Outdated
Show resolved
Hide resolved
|
Overall LGTM! Thanks for adding this and updating! I have some small comments about the spec and test, please let me know WDYT? |
| protected void ensureResolutionManifestForTable(TableIdentifier identifier) { | ||
| if (resolutionManifest == null) { | ||
| resolutionManifest = | ||
| resolutionManifestFactory.createResolutionManifest( | ||
| callContext, securityContext, catalogName); | ||
|
|
||
| // The underlying Catalog is also allowed to fetch "fresh" versions of the target entity. | ||
| resolutionManifest.addPassthroughPath( | ||
| new ResolverPath( | ||
| PolarisCatalogHelpers.tableIdentifierToList(identifier), | ||
| PolarisEntityType.TABLE_LIKE, | ||
| true /* optional */), | ||
| identifier); | ||
| resolutionManifest.resolveAll(); | ||
| } | ||
| } |
There was a problem hiding this comment.
We should verify this with @dennishuo . This is really only so we can control the feature flag at the catalog-level, right? If we control it at a system level, we don't need this? Personally, I think it's fine to control at the realm level and if it means we don't have to muck with the entity resolution for authz, I think that would be better.
There was a problem hiding this comment.
So yes - it is only to be able to control at the catalog level. It was a suggestion from Dmitri here - #2697 (comment)
I think it's okay - if not better performant to do it this way. Before we would have created and resolved the manifest for each operation, which is already required. Now, we just resolve it once here and then get the benefit of being able to have this param be controllable at the catalog level (although it's hard to know why someone would want this controllable at the catalog level).
But happy to review this with @dennishuo as well as I already was going to get him to review.
There was a problem hiding this comment.
How about we just creating another helper in icebergCatalogHandler, like
authorizeUpdateTableOperationOrThrow(
UpdateTableRequest request,
PolarisEntitySubType subType,
TableIdentifier identifier,
) {
// add path, resolve like normal
// get catalog entity and verify the config
getUpdateTableAuthorizableOperations(request);
for (PolarisAuthorizableOperation op : ops) {
authorizer.authorizeOrThrow(
polarisPrincipal,
resolutionManifest.getAllActivatedCatalogRoleAndPrincipalRoles(),
op,
target,
null /* secondary */);
}
}So we do not have to resolve twice. It will indeed create some duplicated code, but that could be solved in future refactoring and is IMHO preferred than resolve path twice (assuming in some case EntityCache is not available)?
There was a problem hiding this comment.
So, went ahead and kept things as they are currently.
There shouldn't be any risk of resolving the path twice because I updated even the CatalogHandler to use ensureResolutionManifestForTable which only resolves if it's null.
Let me know if I might have missed something though.
There was a problem hiding this comment.
Forgot to respond here. Yeah the control flow extracting the ResolutionManifest initialization looks good to me, and I agree it looks like it won't do any extra persistence resolution calls.
I guess conceptually the generation of the ResolutionManifest doesn't need to be strictly inlined in the authorize helper, even though it's intentionally coupled a bit because the intent is that at least the filled-in ResolutionManifest shouldn't "escape" to be used for any real actions before authz has basically "approved" the manifest.
At some point we might want to attach some counters to our ResolutionManifests and/or persistence impls so that in tests we can easily add regression tests to ensure we don't accidentally add extraneous persistence calls.
.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java
Outdated
Show resolved
Hide resolved
...va/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerFineGrainedAuthzTest.java
Outdated
Show resolved
Hide resolved
2b295d3 to
7e6b3e9
Compare
dennishuo
left a comment
There was a problem hiding this comment.
Overall LGTM, just a comment on a test case where the comment didn't seem to match the intent, but I could've been reading it wrong if it's indeed working as intended.
...src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerAuthzTest.java
Show resolved
Hide resolved
3bb0ca2
|
Seems all comments have been addressed and there's also no objection in the dev mailing list. Merging Thanks @dimas-b @collado-mike @dennishuo for reviewing! |
|
Thank you for the change @travis-bowen ! would be really nice to add this in the CHANGELOG.md file too, to help users try this out :) |
|
I was just thinking about this feature today - thank you so much for working on this PR @travis-bowen ! This is such an awesome enhancement to the list of operation types that can be used for authorization decisions on Polaris. |
* Build: remove code to post-process generated Quarkus jars (apache#2667) Before Quarkus 3.28, the Quarkus generated jars used the "current" timestamp for all ZIP entries, which made the jars not-reproducible. Since Quarkus 3.28, the generated jars use a fixed timestamp for all ZIP entries, so the custom code is no longer necessary. This PR depends on Quarkus 3.28. * Update docker.io/jaegertracing/all-in-one Docker tag to v1.74.0 (apache#2751) * Updating metastore documentation with Aurora postgres example (apache#2706) * added Aurora postgres to metastore documentation * Service: Add events for APIs awaiting API changes (apache#2712) * fix(enhancement): add .idea, .vscode, .venv to top level .gitignore (apache#2718) fix(enhancement): add .idea, .vscode, .venv to top level .gitignore * Fix javadocs of `PolarisPrincipal.getPrincipalRoles()` (apache#2752) * fix(enhancement): squash commits (apache#2643) * fix(deps): update dependency io.smallrye.config:smallrye-config-core to v3.14.1 (apache#2755) * Extract interface for RequestIdGenerator (apache#2720) Summary of changes: 1. Extracted an interface from `RequestIdGenerator`. 2. The `generateRequestId` method now returns a `Uni<String>` in case custom implementations need to perform I/O or other blocking calls during request ID generation. 3. Also addressed comments in apache#2602. * JDBC: Handle schema evolution (apache#2714) * Deprecate legacy management endpoints for removal (apache#2749) * Deprecate LegacyManagementEndpoints for removal * Add PolarisResolutionManifestCatalogView.getResolvedCatalogEntity helper (apache#2750) this centralizes some common code and simplifies some test setups * Enforce that S3 credentials are vended when requested (apache#2711) This is a follow-up change to apache#2672 striving to improve user-facing error reporting for S3 storage systems without STS. * Add property to `AccessConfig` to indicate whether the backing storage integration can produce credentials. * Add a check to `IcebergCatalogHandler` (leading to 400) that storage credentials are vended when requested and the backend is capable of vending credentials in principle. * Update `PolarisStorageIntegrationProviderImpl` to indicate that FILE storage does not support credential vending (requesitng redential vending with FILE storage does not produce any credentials and does not flag an error, which matches current Polaris behaviour). * Only those S3 systems where STS is not available (or disabled / not permitted) are affected. * Other storage integrations are not affected by this PR. * [Catalog Federation] Ignore JIT entities when deleting federated catalogs, add integration test for namespace/table-level RBAC (apache#2690) When enabling table/namespace level RBAC in federated catalog, JIT entities will be created during privilege grant. In the short term, we should ignore them when dropping the catalog. In the long term, we will clean-up those entities when deleting the catalog. This will be the first step towards JIT entity clean-up: 1. Ignore JIT entities when dropping federated catalog (orphan entities) 2. Register tasks/in-place cleanup JIT entities during catalog drop 3. Add new functionality to PolarisMetastoreManager to support atomic delete non-used JIT entities during revoke. 4. Global Garbage Collector to clean-up unreachable entities (entities with non-existing catalog path/parent) * SigV4 Auth Support for Catalog Federation - Part 3: Service Identity Info Injection (apache#2523) This PR introduces service identity management for SigV4 Auth Support for Catalog Federation. Unlike user-supplied parameters, the service identity represents the identity of the Polaris service itself and should be managed by Polaris. * Service Identity Injection * Return injected service identity info in response * Use AwsCredentialsProvider to retrieve the credentials * Move some logic to ServiceIdentityConfiguration * Rename ServiceIdentityRegistry to ServiceIdentityProvider * Rename ResolvedServiceIdentity to ServiceIdentityCredential * Simplify the logic and add more tests * Use SecretReference and fix some small issues * Disable Catalog Federation * Update actions/stale digest to 5f858e3 (apache#2758) * Service: RealmContextFilter test refactor (apache#2747) * Update dependency software.amazon.awssdk:bom to v2.35.0 (apache#2760) * Update apache/spark Docker tag to v3.5.7 (apache#2727) * Update eric-maynard Team entry (apache#2763) I'm no longer affiliated with Snowflake, so we should update this page accordingly * Refactor resolutionManifest handling in PolarisAdminService (apache#2748) - remove mutable `resolutionManifest` field in favor of letting the "authorize" methods return their `PolarisResolutionManifest` - replace "find" helpers with "get" helpers that have built-in error handling * Implement Finer Grained Operations and Privileges For Update Table (apache#2697) This implements finer grained operations and privileges for update table in a backwards compatible way as discussed on the mailing list. The idea is that all the existing privileges and operations will work and continue to work even after this change. (i.e. TABLE_WRITE_PROPERTIES will still ensure update table is authorized even after these changes). However, because Polaris will now be able to identify each operation within an UpdateTable request and has a privilege model with inheritance that maps to each operation, users will now have the option of restricting permissions at a finer level if desired. * [Python CLI][CI Failure] Pin pydantic version to < 2.12.0 to fix CI failure (apache#2770) * Delete ServiceSecretReference (apache#2768) * JDBC: Fix Bootstrap with schema options (apache#2762) * Site: Add puppygraph integration (apache#2753) * Update Changelog with finer grained authz (apache#2775) * Add Arguments to Various Event Records (apache#2765) * Update immutables to v2.11.5 (apache#2776) * Client: add support for policy management (apache#2701) Implementation for policy management via Polaris CLI (apache#1867). Here are the subcommands to API mapping: attach - PUT /polaris/v1/{prefix}/namespaces/{namespace}/policies/{policy-name}/mappings create - POST /polaris/v1/{prefix}/namespaces/{namespace}/policies/{policy-name}/mappings delete - DELETE /polaris/v1/{prefix}/namespaces/{namespace}/policies/{policy-name} detach - POST /polaris/v1/{prefix}/namespaces/{namespace}/policies/{policy-name}/mappings get - GET /polaris/v1/{prefix}/namespaces/{namespace}/policies/{policy-name} list - GET /polaris/v1/{prefix}/namespaces/{namespace}/policies - This is default for `list` operation - GET /polaris/v1/{prefix}/applicable-policies - This is when we have `--applicable` option provided update - PUT /polaris/v1/{prefix}/namespaces/{namespace}/policies/{policy-name} * Update dependency com.google.cloud:google-cloud-storage-bom to v2.58.1 (apache#2764) * Update dependency org.jboss.weld:weld-junit5 to v5.0.3.Final (apache#2777) * Update the LICENSE and NOTICE files in the runtime (apache#2779) * SigV4 Auth Support for Catalog Federation - Part 4: Connection Credential Manager (apache#2759) This PR introduces a flexible credential management system for Polaris. Building on Part 3's service identity management, this system combines Polaris service identities with user-provided authentication parameters to generate credentials for remote catalog access. The core of this PR is the new ConnectionCredentialVendor interface, which: Generates connection credentials by combining service identity with user auth parameters Supports different authentication types (AWS SIGV4, AZURE Entra, GCP IAM) through CDI, currently only supports SigV4. Provides on-demand credential generation Enables easy extension for new authentication types In the long term, we should move the storage credential management logic out of PolarisMetastoreManager, PolarisMetastoreManager should only provide persistence interfaces. * Extract IcebergCatalog.getAccessConfig to a separate class AccessConfigProvider (apache#2736) This PR extracts credential vending entrypoint getAccessConfig from IcebergCatalog into a new centralized AccessConfigProvider class, decoupling credential generation from catalog implementations. The old SupportsCredentialVending is removed in this PR upon discussion * Update immutables to v2.11.6 (apache#2780) * Enhance Release docs (apache#2787) * Spark: Remove unnecessary dependency (apache#2789) * Update Pull Request Template (apache#2788) * Freeze 1.2 change log (apache#2783) * [Catalog Federation] Enable Credential Vending for Passthrough Facade Catalog (apache#2784) This PR introduces credential vending support for passthrough-facade catalogs. When creating a passthrough-facade catalog, the configuration currently requires two components: StorageConfig – specifies the storage info for the remote catalog. ConnectionInfo – defines connection parameters for the underlying remote catalog. With this change, the StorageConfig is now also used to vend temporary credentials for user requests. Credential vending honors table-level RBAC policies to determine whether to issue read-only or read-write credentials, ensuring access control consistency with Polaris authorization semantics. A new test case validates the credential vending workflow, verifying both read and write credential vending. Note: the remote catalog referenced by the passthrough-facade does not need to support IRC * Site: Add docs for catalog federation (apache#2761) * Python client: update CHANGELOG.MD for recent changes (apache#2796) * Python client: remove Python 3.9 support (apache#2795) * Update dependency software.amazon.awssdk:bom to v2.35.5 (apache#2799) * FIX REG tests with cloud providers (apache#2793) * [Catalog Federation] Block credential vending for remote tables outside allowed location list (apache#2791) * Correct invalid example in management service OpenAPI spec (apache#2801) The `example` was incorrectly placed as a sibling of `$ref` within a `schema` object in `polaris-management-service.yml`. According to the OpenAPI specification, properties that are siblings of a `$ref` are ignored. This was causing a `NullPointerException` in OpenAPI Generator v7.13.0+ due to a change in how examples are processed. The generator now expects all `examples` to be valid and non-empty, and a misplaced `example` can lead to a null reference when the generator tries to access it (we are not yet using v7.13.0+, thus not a problem at the moment). This commit moves the `example` to be a sibling of the `schema` object, which is the correct placement according to the OpenAPI specification. Reference error when using newer version of openapi-generator-cli: ``` openapi-generator-cli generate -i spec/polaris-catalog-service.yaml -g python -o client/python --additional-properties=packageName=polaris.catalog --additional-properties=apiNameSuffix="" --skip-validate-spec --additional-properties=pythonVersion=3.13 --ignore-file-override /local/client/python/.openapi-generator-ignore ... Exception: Cannot invoke "io.swagger.v3.oas.models.examples.Example.getValue()" because the return value of "java.util.Map.get(Object)" is null at org.openapitools.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:1606) at org.openapitools.codegen.DefaultGenerator.processPaths(DefaultGenerator.java:1474) at org.openapitools.codegen.DefaultGenerator.generateApis(DefaultGenerator.java:663) at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:1296) at org.openapitools.codegen.cmd.Generate.execute(Generate.java:535) at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32) at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66) Caused by: java.lang.NullPointerException: Cannot invoke "io.swagger.v3.oas.models.examples.Example.getValue()" because the return value of "java.util.Map.get(Object)" is null at org.openapitools.codegen.utils.ExamplesUtils.unaliasExamples(ExamplesUtils.java:75) at org.openapitools.codegen.DefaultCodegen.unaliasExamples(DefaultCodegen.java:2343) at org.openapitools.codegen.DefaultCodegen.fromResponse(DefaultCodegen.java:4934) at org.openapitools.codegen.DefaultCodegen.fromOperation(DefaultCodegen.java:4575) at org.openapitools.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:1574) ... 6 more ``` * Update dependency io.opentelemetry:opentelemetry-bom to v1.55.0 (apache#2804) * Update dependency io.micrometer:micrometer-bom to v1.15.5 (apache#2806) * Bump version for python deps (apache#2800) * bump version for python deps * bump version for python deps * bump version for python deps * Update openapi-generatr-cli from 7.11.0.post0 to 7.12.0 * Pin poetry version * Pin poetry version * Update dependency io.projectreactor.netty:reactor-netty-http to v1.2.11 (apache#2809) * [Catalog Federation] Add Connection Credential Vendors for Other Auth Types (apache#2782) Add Connection Credential Vendors for Other Auth Types This change is a prerequisite for enabling connection credential caching. By making PolarisCredentialManager the central entry point for obtaining connection credentials, we can introduce caching cleanly and manage all credential flows in a consistent way. * Last merged commit 6b957ec --------- Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: fabio-rizzo-01 <[email protected]> Co-authored-by: Adnan Hemani <[email protected]> Co-authored-by: Artur Rakhmatulin <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: Prashant Singh <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Honah (Jonas) J. <[email protected]> Co-authored-by: Rulin Xing <[email protected]> Co-authored-by: Eric Maynard <[email protected]> Co-authored-by: Travis Bowen <[email protected]> Co-authored-by: Jaz Ku <[email protected]> Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Yufei Gu <[email protected]>
This implements finer grained operations and privileges for update table in a backwards compatible way as discussed on the mailing list.
(Seems the mailing list split the thread in two but it's here and here)
The design doc is here
The idea is that all the existing privileges and operations will work and continue to work even after this change. (i.e. TABLE_WRITE_PROPERTIES will still ensure update table is authorized even after these changes).
However, because Polaris will now be able to identify each operation within an UpdateTable request and has a privilege model with inheritance that maps to each operation, users will now have the option of restricting permissions at a finer level if desired.