Skip to content
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

Add ClientRegistration.codeChallengeMethod to Enable PKCE #16382

Open
Tracked by #16378
rwinch opened this issue Jan 9, 2025 · 0 comments · May be fixed by #16386
Open
Tracked by #16378

Add ClientRegistration.codeChallengeMethod to Enable PKCE #16382

rwinch opened this issue Jan 9, 2025 · 0 comments · May be fixed by #16386
Labels
for: team-attention This ticket should be discussed as a team before proceeding in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement

Comments

@rwinch
Copy link
Member

rwinch commented Jan 9, 2025

PKCE is recommended to prevent CSRF and authorization code injection attacks. We should further simplify enabling PKCE for Confidential Clients.

Current Behavior

The simplest Spring Boot application that requires PKCE is shown below:

@Bean
SecurityFilterChain springSecurity(HttpSecurity http, OAuth2AuthorizationRequestResolver resolver) throws Exception {
	http
		.authorizeHttpRequests( r -> r
			.anyRequest().authenticated()
		)
		.oauth2Login(l -> l
			.authorizationEndpoint(a -> a
				.authorizationRequestResolver(resolver)
			)
		);
	return http.build();
}

@Bean
OAuth2AuthorizationRequestResolver pkceResolver(ClientRegistrationRepository clientRegistrationRepository) {
	DefaultOAuth2AuthorizationRequestResolver resolver = 
		new DefaultOAuth2AuthorizationRequestResolver(clientRegistrationRepository,"/oauth2/authorization");
	resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce());
	return resolver;
}

Shortcomings

This will be simplified to just a Bean declaration when gh-16380 is implemented, but this still has the following downsides:

  • It is not discoverable from the configuration of a typical Spring Security application
  • It is not declarative

For something that is considered a best security practice (rather than an edge case), we must do better.

Solution

I think what makes the most sense is adding a property ClientRegistration.codeChallengeMethod=S256 (default is null or disabled) similar to what was proposed in gh-12219.

Note that this is on the ClientRegistration because this is how Spring Security decides which grant type to use. If a single application has multiple grant types for the same provider (or different providers), then it is possible that PKCE is not even valid for a specific registration.

This also has the advantage of making it simple for users to define a property using Spring Boot to declaratively enable PKCE with Spring Security.

Previous Discussions

Perhaps this is no longer a problem, but I wanted to bring it up proactively. I know that this was previously declined because:

  • PKCE is only applicable for authorization_code, so there was hesitation around introducing a PKCE property. I do not find this a valid argument. Spring Security already has properties that are only used for specific grant types and custom validation based upon the grant type. For example, the authorizationUri and tokenUri only make sense for the authorization_code grant type and not for client_credentials. A property for enabling PKCE is just another property, that is validated differently by the grant type.
  • Enabling PKCE makes more sense with Spring Boot auto-configuration. However, Spring Boot rejected the proposal.
  • There are concerns that PKCE may not be something that is configured per client. I'd argue that it needs to be per registration because this is how Spring Security decides which grant type to use. If a single application has multiple grant types for the same provider (or different providers), then it is possible that PKCE is not even valid for a specific registration.

cc @jgrandja @sjohnr

@rwinch rwinch added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jan 9, 2025
@rwinch rwinch added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 9, 2025
kse-music added a commit to kse-music/spring-security that referenced this issue Jan 9, 2025
kse-music added a commit to kse-music/spring-security that referenced this issue Jan 9, 2025
@rwinch rwinch added the for: team-attention This ticket should be discussed as a team before proceeding label Jan 9, 2025
kse-music added a commit to kse-music/spring-security that referenced this issue Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention This ticket should be discussed as a team before proceeding in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant