You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "cccs-cat001 (via GitHub)" <gi...@apache.org> on 2025/12/08 13:51:29 UTC
[PR] Added user token to the PolarisPrincipal [polaris]
cccs-cat001 opened a new pull request, #3236:
URL: https://github.com/apache/polaris/pull/3236
Adding the user token to the PolarisPrincipal/PolarisCredential to allow for more customized authentication and storage integrations.
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "cccs-cat001 (via GitHub)" <gi...@apache.org>.
cccs-cat001 commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2606435340
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -94,7 +95,9 @@ public PolarisPrincipal authenticate(PolarisCredential credentials) {
PrincipalEntity principalEntity = resolvePrincipalEntity(credentials);
Set<String> principalRoles = resolvePrincipalRoles(credentials, principalEntity);
- PolarisPrincipal polarisPrincipal = PolarisPrincipal.of(principalEntity, principalRoles);
+ PolarisPrincipal polarisPrincipal =
+ PolarisPrincipal.of(
+ principalEntity, principalRoles, Optional.ofNullable(credentials.getToken()));
LOGGER.debug("Resolved principal: {}", polarisPrincipal);
Review Comment:
It is a debug log though, if you have that on in prod it's your own fault ;)
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "cccs-cat001 (via GitHub)" <gi...@apache.org>.
cccs-cat001 commented on PR #3236:
URL: https://github.com/apache/polaris/pull/3236#issuecomment-3628221390
> @cccs-cat001 do you mind elaborating how you intend to use the JWT or specific claims?
>
> I was looking into whether there's a nicer way for consumers to get the whole JWT or specific claims.
>
> I wonder whether we could just get away with [CDI injection](https://quarkus.io/guides/security-jwt#using-the-jsonwebtoken-and-claim-injection)?
We're looking for a way to pass the users token along to the STS, like in #3170. Since that was closed due to some concerns over security, I've been given a way to do this in our downstream build. There's another PR open, #3224 which will pass the PolarisPrincipal down to the `PolarisStorageIntegration::getSubscopedCreds`. So mixing these two together would enable our use-case.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #3236:
URL: https://github.com/apache/polaris/pull/3236
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3236:
URL: https://github.com/apache/polaris/pull/3236#issuecomment-3627796817
Note: this change is backward compatible at the java class level, as far as I can tell.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #3236:
URL: https://github.com/apache/polaris/pull/3236#issuecomment-3627966860
> The PR currently suggests Optional<String> getToken(), but this is imho too OAuth2-oriented.
Yea, I was wondering about that as well now, given that re-constructing a JWT requires parsing JSON.
Had the idea of adding a function `<T> Optional<T> as(Class<T> type)` on `PolarisPrincipal`, but that would be a more invasive approach. CDI injection sounds like the better alternative.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2604753504
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -94,7 +95,9 @@ public PolarisPrincipal authenticate(PolarisCredential credentials) {
PrincipalEntity principalEntity = resolvePrincipalEntity(credentials);
Set<String> principalRoles = resolvePrincipalRoles(credentials, principalEntity);
- PolarisPrincipal polarisPrincipal = PolarisPrincipal.of(principalEntity, principalRoles);
+ PolarisPrincipal polarisPrincipal =
+ PolarisPrincipal.of(
+ principalEntity, principalRoles, Optional.ofNullable(credentials.getToken()));
LOGGER.debug("Resolved principal: {}", polarisPrincipal);
Review Comment:
Per my understanding, this would mean that we will write the user's token into the debug logs...
This would be pretty bad. I would consider making the change in PolarisPrincipal to ensure that the token is not serialized when calling `principal.toString()` a must-have requirement to approve this PR.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2598858505
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -18,6 +18,7 @@
*/
package org.apache.polaris.core.auth;
+import com.google.common.base.Optional;
Review Comment:
Should use Java's `Optional`, not the Guava one.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2598955654
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -74,4 +111,7 @@ static PolarisPrincipal of(String name, Map<String, String> properties, Set<Stri
* as permissions, preferences, or other metadata.
*/
Map<String, String> getProperties();
+
+ /** Optionally returns the access token of the current user. */
+ Optional<String> getToken();
Review Comment:
Hm, I wonder where this is populated. I only see the ` of(PrincipalEntity principalEntity, Set<String> roles)` used in production 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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #3236:
URL: https://github.com/apache/polaris/pull/3236#issuecomment-3634900960
@adutra :
> But if the goal is more about exposing ALL the principal credentials through the PolarisPrincipal interface [...]
As far as I understand, the goal of this PR is simply to enable custom code to use the token during the authentication flow against STS.
As far as I understand, `PolarisPrincipal` is involved only because we have to expose the right set of values for the Storage Credentials cache to consider when deciding to reuse previous credentials (because reusing a credential obtained with a different user token would be invalid).
This aspect was [discussed briefly](https://github.com/apache/polaris/pull/3224#discussion_r2594524173) in #3224 (as you probably know) and in #3196 (indirectly).
If `StorageAccessConfig` generation could be achieved with only request-scoped objects, then passing credentials through the call chain would not be necessary... However, this is going to be a rather big refactoring.
From my POV limiting this PR to the unparsed auth token propagation is probably sufficient for now. However, I would not mind using `PolarisCredentials` too, the latter would be a more holistic approach, but again it will require a bigger change.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2604771667
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -94,7 +95,9 @@ public PolarisPrincipal authenticate(PolarisCredential credentials) {
PrincipalEntity principalEntity = resolvePrincipalEntity(credentials);
Set<String> principalRoles = resolvePrincipalRoles(credentials, principalEntity);
- PolarisPrincipal polarisPrincipal = PolarisPrincipal.of(principalEntity, principalRoles);
+ PolarisPrincipal polarisPrincipal =
+ PolarisPrincipal.of(
+ principalEntity, principalRoles, Optional.ofNullable(credentials.getToken()));
LOGGER.debug("Resolved principal: {}", polarisPrincipal);
Review Comment:
Good point! Thanks for your diligence, @adnanhemani !
The token property in `PolarisPrincipal` should probably be annotated with `@Value.Redacted`.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2604771667
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -94,7 +95,9 @@ public PolarisPrincipal authenticate(PolarisCredential credentials) {
PrincipalEntity principalEntity = resolvePrincipalEntity(credentials);
Set<String> principalRoles = resolvePrincipalRoles(credentials, principalEntity);
- PolarisPrincipal polarisPrincipal = PolarisPrincipal.of(principalEntity, principalRoles);
+ PolarisPrincipal polarisPrincipal =
+ PolarisPrincipal.of(
+ principalEntity, principalRoles, Optional.ofNullable(credentials.getToken()));
LOGGER.debug("Resolved principal: {}", polarisPrincipal);
Review Comment:
Good point! Thanks for your diligence, @adnanhemani !
The toke property in `PolarisPrincipal` should probably be annotated with `@Value.Redacted`.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "cccs-cat001 (via GitHub)" <gi...@apache.org>.
cccs-cat001 commented on code in PR #3236:
URL: https://github.com/apache/polaris/pull/3236#discussion_r2598985050
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -74,4 +111,7 @@ static PolarisPrincipal of(String name, Map<String, String> properties, Set<Stri
* as permissions, preferences, or other metadata.
*/
Map<String, String> getProperties();
+
+ /** Optionally returns the access token of the current user. */
+ Optional<String> getToken();
Review Comment:
Oh duh. That doesn't happen magically eh!
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added user token to the PolarisPrincipal [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #3236:
URL: https://github.com/apache/polaris/pull/3236#issuecomment-3627866084
Just re-iterating what I said in #3170:
If the goal is really about retrieving the OAuth2 token that was used to authenticate, I think Quarkus OIDC token propagation might be a better approach. In particular, one can inject `io.quarkus.oidc.client.Tokens` wherever necessary:
https://quarkus.io/guides/security-openid-connect-client-reference#inject-tokens
But if the goal is more about exposing ALL the principal credentials through the `PolarisPrincipal` interface, I think the ideal approach would be to move `PolarisCredential` to polaris-core ; then in `PolarisPrincipal` we would expose instead a method like `Set<PolarisCredential> getCredentials()`. (`PolarisCredential` extends a Quarkus interface though, so this would need some refactoring.)
The PR currently suggests `Optional<String> getToken()`, but this is imho too OAuth2-oriented. My rationale for suggesting something more generic is because Polaris is a relatively open architecture in terms of authentication, and it would be certainly feasible to run Polaris with a completely different authentication mechanism, e.g. Basic auth โ in which case, there is no "token" โ but there is a username which could be useful to expose as a credential as well.
--
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@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org