Consider enabling PKCE for confidential clients

[jgrandja]

The goal of PKCE is to provide an added level of security for OAuth 2.0 public clients (utilizing the Authorization Code Grant) from an authorization code interception attack.

However, based on OAuth 2.0 Security Best Current Practice, in section 2.1.1. Authorization Code Grant:

Note: although PKCE so far was recommended as a mechanism to protect
native apps, this advice applies to all kinds of OAuth clients,
including web applications.

It can also be leveraged for confidential clients for an added layer of security.

Given this, we should consider adding this support. From initial research, Okta does support this scenario where a confidential client is used to authenticate with the Token Endpoint and the code_verifier is sent as a parameter.

We should conduct further research to see which other providers support this client configuration/registration.

See this comment for further details.

Related #6446

[dfcoffin]

IETF RFC 8252 "OAuth 2.0 for Native Apps" modifies RFC 6749 "The OAuth 2.0 Authorization Framework" and states in Section

[Appendix A. Server Support Checklist

OAuth servers that support native apps must:

1. Support private-use URI scheme redirect URIs. This is required
   to support mobile operating systems. See Section 7.1.

2. Support "https" scheme redirect URIs for use with public native
   app clients. This is used by apps on advanced mobile operating
   systems that allow app-claimed "https" scheme URIs. See
   Section 7.2.

3. Support loopback IP redirect URIs. This is required to support
   desktop operating systems. See Section 7.3.

4. Not assume that native app clients can keep a secret. If secrets
   are distributed to multiple installs of the same native app, they
   should not be treated as confidential. See Section 8.5.

5. Support PKCE [RFC7636]. Required to protect authorization code
   grants sent to public clients over inter-app communication
   channels. See Section 8.1](https://tools.ietf.org/html/rfc8252#section-4.1)

[jgrandja]

@sdoxsee Thanks for the offer on taking on this ticket. It would be appreciated. But before we do any work here...

We should conduct further research to see which other providers support this client configuration/registration.

So when you have some spare cycles, can you research to see which other providers support this? More importantly, we should try it with the provider to ensure that a confidential client can authenticate with both it's credentials and the code_verifier.

[sdoxsee]

Thanks @jgrandja. This research would be worthwhile but, in the end, I think it would only(?) confirm whether or not PKCE should be defaulted to being "on"? I would argue that its absence from the oauth2 spec itself is reason enough to keep it "off" by default. However, the ClientRegistration could have a flag "withPkce"(?) that would determine if it should be applied or not. All that to say, perhaps the research isn't a pre-requisite for beginning work on this.

What do you think?

[jgrandja]

Good point @sdoxsee. I think you're right as far as the research not being a pre-requisite to start this task. And I agree, PKCE for confidential clients should default to "off". The user has to opt-in for this added level of security (knowing that the target provider supports this). However, I'm not sure I like the setting in ClientRegistration. Let's give this some further thought on where this configuration point could go.

[sdoxsee]

Hi @jgrandja. I've been giving some more though as to where the PKCE setting should go. A couple thoughts to consider:

1. PKCE should only be an option when authorization code grant is used. Since, ClientRegistration has authorizationGrantType at the top level, I don't see where PKCE would fit on ClientRegistration
2. PKCE should still be the default for public clients without explicitly setting it. That is, if the ClientAuthenticationMethod is set to none, then PKCE gets chosen

What do you think about having a .pkce() on the AuthorizationCodeGrantConfigurer so that it would look something like this:

public class MySecurityConfig extends WebSecurityConfigurerAdapter {

    @Override
    protected void configure(HttpSecurity http) throws Exception {
        http
            .oauth2Client()
                 .authorizationCodeGrant()
                    .pkce()
    }
}

The method would set the pkce flag to true and pass it a second constructor on DefaultOAuth2AuthorizationRequestResolver that takes the flag. The flag will explicitly set PKCE in addition to case where it is the default when ClientAuthenticationMethod.NONE.

[jgrandja]

@sdoxsee oauth2Client().authorizationCodeGrant().pkce() seems like the right place to have this configuration point but on second look it wouldn't quite work. Let's say the user has 2x ClientRegistration and both are confidential clients with authorization_code, however, they only want PKCE enabled for one of the clients because the other client (or provider) does not support PKCE for confidential clients - therefore this setting would not allow for this. We definitely need to provide the ability to set PKCE for a confidential client per ClientRegistration.

I'm thinking that if the user wants to enable PKCE for a specific confidential ClientRegistration than they can do that using a custom OAuth2AuthorizationRequestResolver as documented in the reference. It would be a similar implementation to CustomAuthorizationRequestResolver but with the addition of what DefaultOAuth2AuthorizationRequestResolver.addPkceParameters() does.

What do you think about this approach?

[sdoxsee]

@jgrandja great point about the 2x ClientRegistration. Your alternative idea sounds good. I'll give it some thought and get back to you. Thanks!

[sdoxsee]

Hey @jgrandja. Sorry for the delay. I've given your idea some thought. If I understand what you're suggesting, it would be to create something like a PkceOAuth2AuthorizationRequestResolver (and a reactive version) similar to the CustomAuthorizationRequestResolver in the reference documentation.

If that's the case then, after building the OAuth2AuthorizationRequest in the resolve method via the composed DefaultOAuth2AuthorizationRequestResolver member, it would be enhanced with the extra PKCE params.

In order to determine which ClientRegistrations should use PKCE, the authorizationRequestResolver would be created with an array of pkceRegistrationIds for clients wished to be PKCE-enabled. It would use something like this:

	.oauth2Login()
		.authorizationEndpoint()
			.authorizationRequestResolver(
				new PkceOAuth2AuthorizationRequestResolver(
					clientRegistrationRepository,
					"my-pkce-registration-id-1", "my-pkce-registration-id-2", ...
				)
			);

In the resolve method of PkceOAuth2AuthorizationRequestResolver, we'd see if the current registrationId matches any of the pkceRegistrationIds--if so, enhance the OAuth2AuthorizationRequest with PKCE params.

If this works for you, there's also how those PKCE parameters should be added. The addPkceParameters method is already duplicated in DefaultOAuth2AuthorizationRequestResolver and DefaultServerOAuth2AuthorizationRequestResolver. Unless I expose this private method to the new PkceOAuth2AuthorizationRequestResolver (which I don't like), that would make it repeated in 4 places. I'm all for not over-re-using-up-front but, at this point, I'm again tempted to put this logic somewhere else where it could be re-used. I'd originally thought of putting it in the OAuth2AuthorizationRequest itself--after all, it's now been made the default behaviour for authorization_code public clients. It could go also go in some other utility class that takes a OAuth2AuthorizationRequest and adds parameters to it but I like to avoid "utility" classes. The addPkceParameters method (perhaps with different arguments) would be called from at least 4 places from 4 classes (public client reactive and servlet, and confidential client reactive and servlet).

Shall I create a PR somewhere along these lines as a starting point or do you think it would be better to think about this a little more first?

Thanks!

[jgrandja]

@sdoxsee I've been giving this some further thought and I'm not sure if a new implementation of OAuth2AuthorizationRequestResolver (being PkceOAuth2AuthorizationRequestResolver) is the right approach to solve this issue. With this approach we still need to solve the reusability issue with addPkceParameters() and I don't want to duplicate this more than the 2 times we have now. Also, I'm pretty confident there will be more use cases for enhancing the Authorization Request...a recent one #6608.

I think the approach I would like to look into further is a similar pattern found in WebClient.Builder, specifically the operation Builder apply(Consumer<Builder> builderConsumer).

I like this approach as WebClient can be configured with any Consumer that takes a WebClient.Builder. This is definitely very powerful and I like that it takes the composition approach rather than the inheritance approach.

We could have something like apply(Consumer<OAuth2AuthorizationRequest.Builder> builderConsumer) in both DefaultOAuth2AuthorizationRequestResolver and DefaultServerOAuth2AuthorizationRequestResolver. This would allow us to move the addPkceParameters() and createCodeChallenge() into a new class that looks similar to:

public class PkceAuthorizationRequestBuilder implements Consumer<AuthorizationRequest.Builder> {

	@Override
	public void accept(AuthorizationRequest.Builder builder) {

	}
}

This would remove the existing duplication of addPkceParameters() and createCodeChallenge() and allow it to be reused in DefaultOAuth2AuthorizationRequestResolver and DefaultServerOAuth2AuthorizationRequestResolver as a default Consumer in both.

Now to solve this issue, a user could provide a Consumer<AuthorizationRequest.Builder> that determines if Pkce should be applied to a confidential client by inspecting the AuthorizationRequest.Builder and than simply delegating to the new PkceAuthorizationRequestBuilder.

I think some more thought needs to be put into this but I'm feeling this approach could be quite flexible.

What do you think?

[dfcoffin]

@sdoxsee @jgrandja Since the original purpose of this issue was to implement PKCE for public clients, which implements an IETF RFC, I am concerned that it is now attempting to allow the creation of an implementation that is in the "wild" and no longer supports an IETF published RFC. While this may be reasonable, it should be first brought to the attention of the IETF OAuth WG for implementation, as I can see it opening potential security issues not being considered by this request.

[jgrandja]

@dfcoffin As an FYI, we are not planning on changing the existing implementation of DefaultOAuth2AuthorizationRequestResolver that added support for PKCE via #6485.

Also, please see this comment as Okta does support PKCE for confidential clients.

The goal of this issue is to provide support for users that want PKCE enabled for confidential clients as an added layer security on top of client authentication. This will likely be an "opt-in" feature rather than a default setting. Again, there is no intention to change the existing behaviour of PKCE for public clients.