Skip to content

Add support for OAuth 2.0 Client authentication methods #6881

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

Closed
beuvenar opened this issue May 15, 2019 · 38 comments
Closed

Add support for OAuth 2.0 Client authentication methods #6881

beuvenar opened this issue May 15, 2019 · 38 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue

Comments

@beuvenar
Copy link

beuvenar commented May 15, 2019

Currently, Spring Security only supports basic and post authentication methods between client and authorization server. Would it be possible to add other support for other OpenID authentication methods in a future version of Spring Security, in particular client_secret_jwt and private_key_jwt (see https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication)?

Related #8175

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 15, 2019
@rwinch
Copy link
Member

rwinch commented May 17, 2019

Thanks for the report @beuvenar! @jgrandja will take a look at this issue soon

@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 18, 2019
@jgrandja
Copy link
Contributor

jgrandja commented May 18, 2019

@beuvenar We definitely plan on adding support for other client authentication methods at some point and dependent on user demand. I'm not sure it will get into the next release 5.2. Is there a reason you are requesting client_secret_jwt and private_key_jwt? Do you require this? Which provider are you using that supports these authn methods?

@beuvenar
Copy link
Author

@jgrandja We are currently using Srping Security to implement an integration with OIDC provider of EU Login (formerly ECAS), the authentication service of European Commission. They recommend to use client_secret_jwt or private_key_jwt as authentication methods (but they also support basic method so we are not blocked).

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 22, 2019
@jgrandja jgrandja added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels May 31, 2019
@jgrandja jgrandja removed their assignment Jun 4, 2019
@jgrandja jgrandja added this to the 5.3.x milestone Nov 18, 2019
@jgrandja jgrandja changed the title Support for other authentication methods Add support for OAuth 2.0 Client authentication methods Nov 18, 2019
@phughk
Copy link

phughk commented Dec 2, 2019

Hi @jgrandja , is there an ETA on this? Which classes should I be looking at to introduce this?

@erlendfg
Copy link

erlendfg commented Dec 3, 2019

We also need private_key_jwt as a client authentication method in order to implement an integration to the Norwegian ID-porten. We are forced to change client secret often unless we are using private_key_jwt, so I'm looking forward to the 5.3 release.

@jgrandja
Copy link
Contributor

jgrandja commented Dec 3, 2019

@phughk Our goal is to provide client_secret_jwt and private_key_jwt client authentication method support in the 5.3.0 release. We could use some help for sure.

The first place to look at is OAuth2AccessTokenResponseClient, which defines the contract for exchanging an authorization grant for an access token. We have implementations DefaultAuthorizationCodeTokenResponseClient, DefaultClientCredentialsTokenResponseClient, DefaultRefreshTokenTokenResponseClient and DefaultPasswordTokenResponseClient. You will notice that each implementation has a member named requestEntityConverter, which can be customized via setRequestEntityConverter(). The Converter is responsible for providing a RequestEntity. This is where the authentication credentials would be set.

You can also check out the reference doc.

Let me know if you have any other questions.

@matt-williams8
Copy link

matt-williams8 commented Jan 29, 2020

@jgrandja is there any alternative for setRequestEntityConverter in the reactive (ReactiveOAuth2AccessTokenResponseClient) versions of the various OAuth2AccessTokenResponseClients?

Is it instead expected that we add an ExchangeFilterFunction to a WebClient and then provide that WebClient via setWebClient on the chosen ReactiveOAuth2AccessTokenResponseClient?
Even when doing this there does not seem to be a nice way of editing/adding to the body inserter of the original request. I can retrieve the body inserter and key into it, but not retrieve the key first before editing it.

All I want to do is add an extra scope (in addition to the ones already on the OAuth2ClientCredentialsGrantRequest) to the request body and an additional header when making the request to get the token.

@jgrandja
Copy link
Contributor

@matt-williams8

Is it instead expected that we add an ExchangeFilterFunction to a WebClient and then provide that WebClient via setWebClient on the chosen ReactiveOAuth2AccessTokenResponseClient?

Yes, this is the approach for the reactive side. You can either start with ExchangeFilterFunction.ofRequestProcessor() for request pre-processing or ExchangeFilterFunction.ofResponseProcessor() to post-process the response.

@matt-williams8
Copy link

@jgrandja I previously tried that approach, but I hit a blocker.

Unlike the request entity Converter you are able to set on non-reactive OAuth2AccessTokenResponseClients, using an ExchangeFilterFunction on a WebClient does not seem to provide access to the current OAuth2AuthorizationCodeGrantRequest containing the ClientRegistration being used.
I need to be able to add to the scopes being sent as part of the OAuth2 client credentials request. In the ExchangeFilterFunction I can get access to the existing BodyInserter (and cast it to a FormInserter...not a big fan of this either as it relies on knowing that that's what the WebClientReactiveClientCredentialsTokenResponseClient is using under the hood) so that I can edit the body, but by not having access to the OAuth2AuthorizationCodeGrantRequest I cannot ensure that the existing scopes to be used would be maintained. As I cannot fetch the existing value of the scopes attribute from the BodyInserter I would only be able to overwrite the value with the additional scope to be added rather than just adding it on the existing value of the scopes attribute.

Apologies if this is polluting this issue, I'm happy to move the conversation elsewhere.

@Budlee
Copy link
Contributor

Budlee commented Jan 30, 2020

Hey,
Started working on the private_jwt authentication method as we have a usecase for this.
Hopping to PR this back in fairly soon

@jgrandja
Copy link
Contributor

jgrandja commented Feb 3, 2020

@matt-williams8

using an ExchangeFilterFunction on a WebClient does not seem to provide access to the current OAuth2AuthorizationCodeGrantRequest containing the ClientRegistration being used

You can provide your own implementation of ReactiveOAuth2AccessTokenResponseClient that sets the OAuth2AuthorizationCodeGrantRequest as a request attribute:

webClient.post().uri(tokenUri).attribute(OAuth2AuthorizationCodeGrantRequest.class.getName(), authorizationGrantRequest)

Now you can lookup OAuth2AuthorizationCodeGrantRequest using ClientRequest from within the ExchangeFilterFunction. Makes sense?

If you have further questions, please log a new ticket so we can continue on there.

@minionOfZuul
Copy link

minionOfZuul commented Mar 26, 2020

@jgrandja is there interest in the idea of adding the OAuth2AuthorizationCodeGrantRequest as a request attribute part of the official API? The way I'm reading it, the non-reactive implementation (request entity converter) gets this kind of functionality out of the box.

@jgrandja
Copy link
Contributor

@zpearce I'm not sure I follow you? Can you please provide more detail. As well, if this is not related to this issue please log a new issue and we can continue the convo there.

@forgo
Copy link

forgo commented Apr 27, 2020

@jgrandja I created the PR. It's definitely a WIP, and I would expect some tests will fail, but I wanted to get what I had out there to start a discussion on if I'm going in the right direction at all.

Much more comments and notes on the PR

@krajcsovszkig-ms
Copy link

Hi @jgrandja ,

I'm taking this over from @Budlee (we work at the same organization). I'm almost done planning out the design for the feature, would you like to review it before I start writing the code? Shall I just put it here, or where can I send it for you to check it out?

Thanks!

@jgrandja
Copy link
Contributor

@krajcsovszkig-ms I haven't heard from @Budlee in a while and @forgo just submitted a PR #8445. Can you take a look at the PR first and maybe provide feedback there?

@krajcsovszkig-ms
Copy link

Huh, not sure how I didn't notice the latest comments.

I'm happy to get involved with the review and testing, and also to help with the implementation if I can.

@jgrandja
Copy link
Contributor

Thanks @krajcsovszkig-ms. It would be great if you can take a look at #8445 to provide feedback and share your design ideas. The more eyes on it the better the design outcome will be.

@krajcsovszkig-ms
Copy link

Hi @jgrandja,

We are having a discussion about the design over on the PR, could you take a look and add your comments?

Thanks!

@jgrandja
Copy link
Contributor

Sure @krajcsovszkig-ms. I will get to it this week for sure.

@krajcsovszkig-ms
Copy link

Great, thanks @jgrandja

@brandon3123
Copy link

Hey there, I also have the requirement for private_key_jwt client authentication method etc.....any idea of the progress of this feature. I'll probably have to implement something custom for my project, but I was just curious.

Thank you

@jgrandja
Copy link
Contributor

@brandon3123 We are currently working on this but it will be tight to get this into 5.4 since it's being released in Sep. If it doesn't get into 5.4, it will get into next release for sure.

@sander-su
Copy link

sander-su commented Aug 11, 2020

Hi, great you are working on this.
We have Azure AD and would also like jwt auth.
See: https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-certificate-credentials
Microsoft needs the x5t header also present in the jwt assertion (which is a thumbprint of the private key).
Is this also in the design? Because it is not in de default spec.

If not could you also include this optionally?
@jgrandja

@jgrandja
Copy link
Contributor

@sander-su Our goal is to implement to spec. With non-standard features, we typically provide hooks to allow for customizations. Please keep track of this issue and feature development to let me know if you require any additional hooks for your requirements with Azure AD.

@sander-su
Copy link

sander-su commented Aug 24, 2020

@jgrandja Thanks for looking into this.
The most generic way to implement this would be having the option to add additional (static) claims to the header of the jwt.
These additional claims could than be specified in the yaml for the client config.

Furthermore, is it in the design to add a JTI to the jwt?
I'd also like the JWT to be as short lived as possible.
So perhaps, 2 profiles, when using JTI's they are generated each time. When not using a JTI the JWT's could be cached for X amount of time.

Btw which issue is best to comment, this one or #6053?

@jgrandja
Copy link
Contributor

jgrandja commented Sep 1, 2020

@sander-su This is the right issue to comment on. When work starts on this feature, we'll consider your feedback.

@jgrandja jgrandja added this to the 5.5.x milestone Sep 25, 2020
@schluemi
Copy link

schluemi commented Mar 1, 2021

@jgrandja Thanks for looking into this, I really appreciate it! I was just wondering if there is any progress on the private_key_jwt authentication method. Many of our customers would like to use the spring security module, however, we as operators of an OP would prefer to enforce the use of keys over passwords.

@jgrandja
Copy link
Contributor

jgrandja commented Mar 1, 2021

@schluemi See comment

@jgrandja jgrandja self-assigned this Mar 24, 2021
@jgrandja
Copy link
Contributor

@beuvenar @phughk @erlendfg @Budlee @forgo @krajcsovszkig-ms @brandon3123 @sander-su @schluemi @mjeffrey @matt-domsch-sp Please see #9520.

It would be greatly appreciated if you can test out the initial implementation with the provider(s) you are using.
Please let me know if the new API provides the flexibility you require for your use case(s).

FYI, I need to get this merged before April 12 for 5.5.0-RC1, so your prompt response would be a huge help.
Thank you.

@krajcsovszkig-ms
Copy link

Thanks @jgrandja for doing this and sorry for not helping more in the end, something more urgent came up and took pretty long to clean up.

I've reviewed your PR and it looks great, I'm pretty sure it should work for us.

I'm unfortunately unable to build spring-security inside our organization to try your PR (I assume it's something with the way our proxies and firewalls are set up). Would it be possible to make a snapshot of it available on maven or some other public repository? We have a process to pull those in but I can't just build it outside and send the jars in I'm afraid.

Thanks!

@jgrandja
Copy link
Contributor

jgrandja commented Apr 7, 2021

@krajcsovszkig-ms

Would it be possible to make a snapshot of it available on maven or some other public repository?

Unfortunately no. It's available on http://repo.spring.io/ until it goes GA and then it will be available in Central.

@jgrandja
Copy link
Contributor

jgrandja commented Apr 8, 2021

Closing as duplicate. See gh-8175 gh-9520

@jgrandja jgrandja closed this as completed Apr 8, 2021
@jgrandja jgrandja removed this from the 5.5.x milestone Apr 8, 2021
@jgrandja jgrandja added status: duplicate A duplicate of another issue and removed type: enhancement A general enhancement labels Apr 8, 2021
@Budlee
Copy link
Contributor

Budlee commented Apr 9, 2021

@jgrandja, I know its a bit late, however, I have gone over the PR and could not see any issues, Looks great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests