Add JWT claims as extra credentials

[raj-manvar]

Description

This is for handling #22943 and adds the JWT claims as extraCredentials to the Identity class

Additional context and related issues

Release notes

( ) Release notes are required. Please propose a release note for me.

[cla-bot]

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

[cla-bot]

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

[wendigo]

I don't like the idea of adding claims to the extraCredentials. These can be passed by the user so how do we know that the claims are valid? Instead, I think that we should add claims as first-class citizens to identity class. Wdyt @dain @electrum ?

[electrum]

Can you tell us more about your use case?

[raj-manvar]

Thanks @wendigo, To confirm do you mean adding an Optional<Claims> as a member of the Identity class? If yes, that seems more clean to me as well.

Thanks as well @electrum! While adopting JWT authentication for Trino ( docs ) we found the default sub and aud claims alone aren't sufficient for access control in our case as we also have some custom claims that need to be validated. ( somewhat related to #4244 )

[raj-manvar]

Hi @electrum @wendigo Checking in regarding this. Is adding Optional to Identity class a better approach here ? Also open to other alternatives which will allow plugins to access other JWT claims, I can update the MR accordingly. Thanks!

[wendigo]

@raj-manvar we need to understand the use-case first ("why" is more important than "how")

[hashhar]

Also see #15669 to learn of issues with using claims from JWTs received during authentication.

[raj-manvar]

Thanks @wendigo! yes I can add more context here. We use the standard SPIFFE/SPIRE for generation of the ID tokens to be auth with Trino. While spire has great support for authentication, it's not sufficient for authorization.
i.e. given a spiffeId passes as sub claim, how do we determine if it has access to a particular Catalog, Database, Table? In order to bridge the gap between authentication and authorization, we are using OpenLdap groups to manage authorization and map spiffeIDs to openldap groups which determine authorization of different datasets
It's best if the ID token itself are source of truth for this mapping between SpiffeID and OpenLdap groups instead of Trino making external calls to get the mapping. So we plan to add the relevant OpenLdap UID associated with SpiffeId as another claim for the ID tokens.
From the readings I have done on my side since spire doesn't support authorization, we need some solutions like this for authorization of data via Trino. Are there any other standard solutions that can help with Trino being able to authorize data access ? Do you think this usecase is sufficient for adding some similar feature set to opensrouce Trino or there are better ways to do this ?

Thanks @hashhar The issue you linked seems for the oauth https://trino.io/docs/current/security/oauth2.html, the current issue is more around JWT auth https://trino.io/docs/current/security/jwt.html if I understand correctly

[raj-manvar]

Hi @wendigo checking in if this usecase is generic enough to add a solution to opensource Trino or if you feel this should be done later / some other way.

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[raj-manvar]

Please let me know if more info or updates required for the MR! 🙏

[raj-manvar]

Hello all, following up again. Any feedback will be useful here. even if this can't be added to Trino now or in future, knowing that would be useful to evaluate if we should start exploring other ways for access control for our usecase :)

[ebyhr]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[hashhar]

@raj-manvar we were discussing this offline and came to the conclusion that we need additional fields in the Identity to carry such non-secret but trusted information.

extra-credentials is not the right carrier for this since it's considered secret and can also be overridden by clients which can lead to issues if the authorization system does not verify the information passed through there.

Another reason to add the information to Identity is that Identity is what is used as the basis for all access control checks in the SystemAccessControl interface.

We'll get back to you once we have more clarity on how the new Identity object should look like.

[raj-manvar]

Thanks @hashhar for the update! Makes complete sense :)
I'll try to update the MR to the updated fields of Identity class once we have more clarity. 🙇‍♂️

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[raj-manvar]

Checking in for any updates. Removed the stale label

[hashhar]

I'm copying some offline discussion here so that we can continue discussing this with @electrum / @dain.

Praveen Krishna
Oct 3rd at 15:22
I think it should be something similar to a group provider kind of a lighter version of it - I guess there was also a similar discussion around mapping custom LDAP attributes as a part of group - I think we need a different abstraction or object to represent those information

Dain Sundstrom
Oct 3rd at 20:17
I mentioned a few times we should have an extra carrier for non-secure data in the identity.

Praveen Krishna
Oct 3rd at 20:19
But which module should populate it out ? Authenticator or GroupProvider or both ?

Dain Sundstrom
Oct 3rd at 21:30
It would be the authenticator. Backing up. I’d like to have an extra bag of data the authenticator can set into the identity that isn’s consider secure (think Github CI variables vs secrets). We would attach the data to events and show it in the UI because they are not secret. The big use cases are OICD (JWT) and LDAP, where authentication can have additional data. For example providers can attache the name of the region the user logged into. Then this information can be used to make security decisions like which groups they belong to. Also I’d like to add SQL functions to extract these values so they can be used in row filter expressions, which is why I think we need another bag in identity. The query optimizer will inline the value so it can’t reference secrets.

David Phillips
Oct 3rd at 21:32
This is kind of the opposite of extra credentials: they’re not secret but they are trusted.

Dain Sundstrom
Oct 4th at 02:58
The real challenge here is the design.
@david
any suggestion for the field name currently we have getExtraCredentials as a Map<String, String> . Also is a string/string map good enough?

[github-actions]

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

[github-actions]

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.