Added user token to the PolarisPrincipal

[cccs-cat001]

Adding the user token to the PolarisPrincipal/PolarisCredential to allow for more customized authentication and storage integrations.

Checklist

• 🛡️ Don't disclose security issues! (contact [email protected])
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 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)

[snazy]

I think this can be useful.

One minor comment on the change.

[dimas-b]

At this stage it probably requires some custom code in order to put the access token to good use, still, as for me, this feature looks pretty useful to have in Polaris.

[dimas-b]

Note: this change is backward compatible at the java class level, as far as I can tell.

[adutra]

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.

[snazy]

@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?

[snazy]

The PR currently suggests Optional 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.

[cccs-cat001]

@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?

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.

[adnanhemani]

Had a chat with @dimas-b offline. Per my understanding, this is very similar to #3170 but stops short of introducing the behavior that is being pushed back against (doing the credential vending itself) on the ML and PR.

As a result, I am okay with the general changes here - given the baseline that we will NOT be re-introducing the changes from #3170 at any point in the future (unless there is consensus to do so on the existing ML thread).

However, there is one callout that is security-critical issue IMO that we must change to let this code be acceptable still. I don't think the change requires us to change the overall approach (I hope...) but needs to be taken care of prior to merging.

[dimas-b]

@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 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.

[adnanhemani]

LGTM as per #3236 (review)

Thanks for this contribution @cccs-cat001!