-
Notifications
You must be signed in to change notification settings - Fork 331
SigV4 Auth Support for Catalog Federation - Part 3: Service Identity Info Injection #2523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SigV4 Auth Support for Catalog Federation - Part 3: Service Identity Info Injection #2523
Conversation
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for keeping improving service identity handling in Polaris, @XJDKC !
The approach LGTM in general, but I believe the CDI-related code can be simplified :)
Also, I'm not sure I understand how AWS credentials are going to be used in practice... so those comments are mostly for the sake of clarification.
.../src/main/java/org/apache/polaris/core/identity/registry/ServiceIdentityRegistryFactory.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/org/apache/polaris/service/identity/RealmServiceIdentityConfiguration.java
Show resolved
Hide resolved
...ce/src/main/java/org/apache/polaris/service/identity/AwsIamServiceIdentityConfiguration.java
Show resolved
Hide resolved
...e/src/main/java/org/apache/polaris/core/identity/resolved/ResolvedAwsIamServiceIdentity.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/polaris/core/identity/resolved/ResolvedAwsIamServiceIdentity.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/polaris/service/identity/ServiceIdentityRegistryConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XJDKC Thanks for the great work! Just for my understanding, the Part 1: transformation is no longer needed?
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java
Show resolved
Hide resolved
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @XJDKC !
It is still not very clear to me how identity secrets are plugged into the runtime components that interact with remote systems... so some of my comments may look odd because of this 😅 but I hope I'm not completely off the point :)
...e/src/main/java/org/apache/polaris/core/identity/resolved/ResolvedAwsIamServiceIdentity.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/polaris/service/identity/registry/DefaultServiceIdentityRegistry.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/polaris/service/identity/registry/DefaultServiceIdentityRegistry.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/polaris/service/identity/registry/DefaultServiceIdentityRegistry.java
Outdated
Show resolved
Hide resolved
cf9a113 to
e386298
Compare
...is-core/src/main/java/org/apache/polaris/core/identity/registry/ServiceIdentityRegistry.java
Outdated
Show resolved
Hide resolved
020fc1b to
fe883ce
Compare
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall 👍 Thanks, @XJDKC !
Call paths to ServiceIdentityProvider now distinguish user-facing (API) data from backend secrets / credentials.
I made some minor comments, but I do not think they are blockers. I hope you could address them (if you agree) while we're waiting for another review from @dennishuo .
Please consider all my previous comment threads "resolved" (but you may want to keep them open for context to other reviews).
...c/main/java/org/apache/polaris/service/identity/provider/DefaultServiceIdentityProvider.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/connection/ConnectionConfigInfoDpo.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java
Show resolved
Hide resolved
...c/main/java/org/apache/polaris/service/identity/provider/DefaultServiceIdentityProvider.java
Show resolved
Hide resolved
...ore/src/main/java/org/apache/polaris/core/identity/credential/ServiceIdentityCredential.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/identity/dpo/ServiceIdentityInfoDpo.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java
Outdated
Show resolved
Hide resolved
dimas-b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Looking forward to review / approval from @dennishuo before merging.
Thanks @dimas-b for reviewing and approving the PR, the feedback is very valuable, now the PR is in a good state. |
HonahX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
dennishuo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Many thanks @dimas-b @dennishuo @HonahX for taking the time to review the PR! I will open a PR for part 4 soon! |
Milestones
This is Part 3 of the [Splitting] Initial SigV4 Auth Support for Catalog Federation. Upcoming parts will build on this system:
SigV4 Auth Support for Catalog Federation - Part 1: Entity Transformation System #1899Introduction
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.
We introduce a new
ServiceIdentityProviderinterface responsible for three core operations:1. Allocating a service identity to a catalog entity
2. Retrieving identity information for API responses
getCatalog, Polaris usesgetServiceIdentityInfo()to return identity metadata (e.g., AWS IAM ARN) without exposing credentials3. Retrieving credentials for authentication
getServiceIdentityCredential()to obtain the full credential with AWS access keysThis PR also introduces a
DefaultServiceIdentityProvider, which supports static configuration via Quarkus application properties. While it does not support dynamic runtime resolution (all identities must be specified before server startup), it provides:In the future, vendors may implement custom providers that integrate with their internal infrastructure for on-demand identity management or dynamic credential rotation.
Design Overview
Each
ConnectionConfigInfoDpo(used for remote catalog federation) now contains aServiceIdentityInfoDpo, which holds aSecretReferencethat serves as a unique identifier for the service identity instance. This design allows:SigV4AuthenticationParametersDpo(supplied by the user)Key Components
ServiceIdentityProvider: The central provider interface with three methods:allocateServiceIdentity(ConnectionConfigInfo)- Assigns a service identity reference to a catalog during creation based on the authentication typegetServiceIdentityInfo(ServiceIdentityInfoDpo)- Returns identity metadata (e.g., IAM ARN) without credentials for API responsesgetServiceIdentityCredential(ServiceIdentityInfoDpo)- Returns the full credential with secrets, resolved lazily only when authentication is neededDefaultServiceIdentityProvider: The default implementation that:polaris.service-identity.*properties at startupgetServiceIdentityCredential()is calledServiceIdentityCredential: Base class representing a credential with:SecretReferenceserving as a unique identifier for lookupsConfiguration
Configuration examples are provided in the JavaDoc of
ServiceIdentityConfigurationfor developers. Since this feature is experimental and part of a multi-part series, configuration examples are not included inapplication.propertiesto avoid confusion for end-users.Key Implementation Details
Lazy Credential Resolution:
getServiceIdentityInfo) never creates credential providersgetServiceIdentityCredential) are only resolved when actually needed for authenticationReference-Based Lookups:
SecretReferenceserves as a unique identifier for each service identity instanceFlowchart