spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.48k stars 5.77k forks source link

Add support for OAuth 2.0 Client authentication methods #6881

Closed beuvenar closed 3 years ago

beuvenar commented 5 years ago

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

rwinch commented 5 years ago

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

jgrandja commented 5 years ago

@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 commented 5 years ago

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

phughk commented 4 years ago

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

erlendfg commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@jgrandja

private_key_jwt is the preferred authentication method for web apps integrating with login.gov with OpenID Connect. The only other supported method is PKCE which they suggest for native mobile apps.

Previously, I had managed to get the private_key_jwt flow working in Spring Security (non-reactive) by doing a lot of backflips with a custom OAuth2AuthorizationRequestResolver and custom OAuth2AuthorizationCodeGrantRequestEntityConverter.

It would be nice if I could point Spring to my keystore for signing a JWT, and I would no longer be responsible for creating a JWT by deriving information from ClientRegistrationRepository and programmatically adding URL params to the token request.

If anyone wants to reference what I did in a non-reactive app to accomplish this, here you can find my custom resolver, converter, etc.

Similarly to @matt-williams8, I've been trying to translate this flow into a reactive Spring app, and I'm having a lot of issues and could use some help (or this feature :)).

jgrandja commented 4 years ago

@forgo The most efficient way of getting this feature in is via a PR. We just announced Spring Authorization Server, which will take up quite a bit of resources.

If you or anyone in the community is interested in submitting a PR, I can help guide.

forgo commented 4 years ago

If you or anyone in the community is interested in submitting a PR, I can help guide.

@jgrandja

I've created a branch gh-6881 branch. It's a decent start, but I could use your guidance now.

I signed the "Contributor License Agreement", and I'm following the contributor guidelines, but I have been unable to push my branch. Do I need to do anything else?

jgrandja commented 4 years ago

@forgo Please submit a Draft Pull Request. Also, please review the contributing section, which provides links on how to submit a PR.

forgo commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

krajcsovszkig-ms commented 4 years ago

Great, thanks @jgrandja

brandon3123 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 https://github.com/spring-projects/spring-security/issues/6053?

jgrandja commented 3 years ago

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

schluemi commented 3 years ago

@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 commented 3 years ago

@schluemi See comment

jgrandja commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

Closing as duplicate. See gh-8175 gh-9520

Budlee commented 3 years ago

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