From a2ffc882949ae3a4f495b646b56fc2a478e168ed Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Tue, 15 Mar 2022 10:25:17 -0400 Subject: [PATCH] Allow configuring PKCE for confidential clients Closes gh-6548 --- .../oauth2/client/authorization-grants.adoc | 3 + .../oauth2/client/authorization-grants.adoc | 3 + docs/modules/ROOT/pages/whats-new.adoc | 20 +++- ...ultOAuth2AuthorizationRequestResolver.java | 86 +++++--------- ...OAuth2AuthorizationRequestCustomizers.java | 111 ++++++++++++++++++ ...verOAuth2AuthorizationRequestResolver.java | 88 +++++--------- ...uth2AuthorizationRequestResolverTests.java | 111 +++++++++++++++--- ...uth2AuthorizationRequestResolverTests.java | 94 +++++++++++++-- 8 files changed, 369 insertions(+), 147 deletions(-) create mode 100644 oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestCustomizers.java diff --git a/docs/modules/ROOT/pages/reactive/oauth2/client/authorization-grants.adoc b/docs/modules/ROOT/pages/reactive/oauth2/client/authorization-grants.adoc index a2a7a77cd4..6e42dcb698 100644 --- a/docs/modules/ROOT/pages/reactive/oauth2/client/authorization-grants.adoc +++ b/docs/modules/ROOT/pages/reactive/oauth2/client/authorization-grants.adoc @@ -72,6 +72,9 @@ If the client is running in an untrusted environment (eg. native application or . `client-secret` is omitted (or empty) . `client-authentication-method` is set to "none" (`ClientAuthenticationMethod.NONE`) +[TIP] +If the OAuth 2.0 Provider supports PKCE for https://tools.ietf.org/html/rfc6749#section-2.1[Confidential Clients], you may (optionally) configure it using `DefaultServerOAuth2AuthorizationRequestResolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce())`. + [[oauth2Client-auth-code-redirect-uri]] The `DefaultServerOAuth2AuthorizationRequestResolver` also supports `URI` template variables for the `redirect-uri` using `UriComponentsBuilder`. diff --git a/docs/modules/ROOT/pages/servlet/oauth2/client/authorization-grants.adoc b/docs/modules/ROOT/pages/servlet/oauth2/client/authorization-grants.adoc index 01626cf7f7..aea17b02a9 100644 --- a/docs/modules/ROOT/pages/servlet/oauth2/client/authorization-grants.adoc +++ b/docs/modules/ROOT/pages/servlet/oauth2/client/authorization-grants.adoc @@ -72,6 +72,9 @@ If the client is running in an untrusted environment (eg. native application or . `client-secret` is omitted (or empty) . `client-authentication-method` is set to "none" (`ClientAuthenticationMethod.NONE`) +[TIP] +If the OAuth 2.0 Provider supports PKCE for https://tools.ietf.org/html/rfc6749#section-2.1[Confidential Clients], you may (optionally) configure it using `DefaultOAuth2AuthorizationRequestResolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce())`. + [[oauth2Client-auth-code-redirect-uri]] The `DefaultOAuth2AuthorizationRequestResolver` also supports `URI` template variables for the `redirect-uri` using `UriComponentsBuilder`. diff --git a/docs/modules/ROOT/pages/whats-new.adoc b/docs/modules/ROOT/pages/whats-new.adoc index c27a736da9..fb84f98e8f 100644 --- a/docs/modules/ROOT/pages/whats-new.adoc +++ b/docs/modules/ROOT/pages/whats-new.adoc @@ -4,5 +4,21 @@ Spring Security 5.7 provides a number of new features. Below are the highlights of the release. -* xref:servlet/authentication/persistence.adoc#requestattributesecuritycontextrepository[`RequestAttributeSecurityContextRepository`] -* xref:servlet/authentication/persistence.adoc#securitycontextholderfilter[`SecurityContextHolderFilter`] - Ability to require explicit saving of the `SecurityContext`. +[[whats-new-servlet]] +== Servlet + +* Web + +** Introduced xref:servlet/authentication/persistence.adoc#requestattributesecuritycontextrepository[`RequestAttributeSecurityContextRepository`] +** Introduced xref:servlet/authentication/persistence.adoc#securitycontextholderfilter[`SecurityContextHolderFilter`] - Ability to require explicit saving of the `SecurityContext` + +* OAuth 2.0 Client + +** Allow configuring https://github.com/spring-projects/spring-security/issues/6548[PKCE for confidential clients] + +[[whats-new-webflux]] +== WebFlux + +* OAuth 2.0 Client + +** Allow configuring https://github.com/spring-projects/spring-security/issues/6548[PKCE for confidential clients] diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java index 7e606eae6b..915730dbea 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -34,7 +34,6 @@ import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; -import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; import org.springframework.security.oauth2.core.oidc.OidcScopes; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.web.util.UrlUtils; @@ -70,15 +69,19 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au private static final char PATH_DELIMITER = '/'; + private static final StringKeyGenerator DEFAULT_STATE_GENERATOR = new Base64StringKeyGenerator( + Base64.getUrlEncoder()); + + private static final StringKeyGenerator DEFAULT_SECURE_KEY_GENERATOR = new Base64StringKeyGenerator( + Base64.getUrlEncoder().withoutPadding(), 96); + + private static final Consumer DEFAULT_PKCE_APPLIER = OAuth2AuthorizationRequestCustomizers + .withPkce(); + private final ClientRegistrationRepository clientRegistrationRepository; private final AntPathRequestMatcher authorizationRequestMatcher; - private final StringKeyGenerator stateGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder()); - - private final StringKeyGenerator secureKeyGenerator = new Base64StringKeyGenerator( - Base64.getUrlEncoder().withoutPadding(), 96); - private Consumer authorizationRequestCustomizer = (customizer) -> { }; @@ -100,7 +103,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au @Override public OAuth2AuthorizationRequest resolve(HttpServletRequest request) { - String registrationId = this.resolveRegistrationId(request); + String registrationId = resolveRegistrationId(request); if (registrationId == null) { return null; } @@ -123,6 +126,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au * @param authorizationRequestCustomizer the {@code Consumer} to be provided the * {@link OAuth2AuthorizationRequest.Builder} * @since 5.3 + * @see OAuth2AuthorizationRequestCustomizers */ public void setAuthorizationRequestCustomizer( Consumer authorizationRequestCustomizer) { @@ -147,9 +151,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au if (clientRegistration == null) { throw new IllegalArgumentException("Invalid Client Registration with Id: " + registrationId); } - Map attributes = new HashMap<>(); - attributes.put(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId()); - OAuth2AuthorizationRequest.Builder builder = getBuilder(clientRegistration, attributes); + OAuth2AuthorizationRequest.Builder builder = getBuilder(clientRegistration); String redirectUriStr = expandRedirectUri(request, clientRegistration, redirectUriAction); @@ -158,8 +160,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au .authorizationUri(clientRegistration.getProviderDetails().getAuthorizationUri()) .redirectUri(redirectUriStr) .scopes(clientRegistration.getScopes()) - .state(this.stateGenerator.generateKey()) - .attributes(attributes); + .state(DEFAULT_STATE_GENERATOR.generateKey()); // @formatter:on this.authorizationRequestCustomizer.accept(builder); @@ -167,23 +168,24 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au return builder.build(); } - private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientRegistration, - Map attributes) { + private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientRegistration) { if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) { - OAuth2AuthorizationRequest.Builder builder = OAuth2AuthorizationRequest.authorizationCode(); - Map additionalParameters = new HashMap<>(); + // @formatter:off + OAuth2AuthorizationRequest.Builder builder = OAuth2AuthorizationRequest.authorizationCode() + .attributes((attrs) -> + attrs.put(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + // @formatter:on if (!CollectionUtils.isEmpty(clientRegistration.getScopes()) && clientRegistration.getScopes().contains(OidcScopes.OPENID)) { // Section 3.1.2.1 Authentication Request - // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest scope // REQUIRED. OpenID Connect requests MUST contain the "openid" scope // value. - addNonceParameters(attributes, additionalParameters); + applyNonce(builder); } if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { - addPkceParameters(attributes, additionalParameters); + DEFAULT_PKCE_APPLIER.accept(builder); } - builder.additionalParameters(additionalParameters); return builder; } if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { @@ -252,57 +254,25 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au /** * Creates nonce and its hash for use in OpenID Connect 1.0 Authentication Requests. - * @param attributes where the {@link OidcParameterNames#NONCE} is stored for the - * authentication request - * @param additionalParameters where the {@link OidcParameterNames#NONCE} hash is - * added for the authentication request + * @param builder where the {@link OidcParameterNames#NONCE} and hash is stored for + * the authentication request * * @since 5.2 * @see 3.1.2.1. * Authentication Request */ - private void addNonceParameters(Map attributes, Map additionalParameters) { + private static void applyNonce(OAuth2AuthorizationRequest.Builder builder) { try { - String nonce = this.secureKeyGenerator.generateKey(); + String nonce = DEFAULT_SECURE_KEY_GENERATOR.generateKey(); String nonceHash = createHash(nonce); - attributes.put(OidcParameterNames.NONCE, nonce); - additionalParameters.put(OidcParameterNames.NONCE, nonceHash); + builder.attributes((attrs) -> attrs.put(OidcParameterNames.NONCE, nonce)); + builder.additionalParameters((params) -> params.put(OidcParameterNames.NONCE, nonceHash)); } catch (NoSuchAlgorithmException ex) { } } - /** - * Creates and adds additional PKCE parameters for use in the OAuth 2.0 Authorization - * and Access Token Requests - * @param attributes where {@link PkceParameterNames#CODE_VERIFIER} is stored for the - * token request - * @param additionalParameters where {@link PkceParameterNames#CODE_CHALLENGE} and, - * usually, {@link PkceParameterNames#CODE_CHALLENGE_METHOD} are added to be used in - * the authorization request. - * - * @since 5.2 - * @see 1.1. - * Protocol Flow - * @see 4.1. - * Client Creates a Code Verifier - * @see 4.2. - * Client Creates the Code Challenge - */ - private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.secureKeyGenerator.generateKey(); - attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); - try { - String codeChallenge = createHash(codeVerifier); - additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeChallenge); - additionalParameters.put(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256"); - } - catch (NoSuchAlgorithmException ex) { - additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeVerifier); - } - } - private static String createHash(String value) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(value.getBytes(StandardCharsets.US_ASCII)); diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestCustomizers.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestCustomizers.java new file mode 100644 index 0000000000..b1c71be8a3 --- /dev/null +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestCustomizers.java @@ -0,0 +1,111 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.oauth2.client.web; + +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; + +import org.springframework.security.crypto.keygen.Base64StringKeyGenerator; +import org.springframework.security.crypto.keygen.StringKeyGenerator; +import org.springframework.security.oauth2.client.web.server.DefaultServerOAuth2AuthorizationRequestResolver; +import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; +import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; + +/** + * A factory of customizers that customize the {@link OAuth2AuthorizationRequest OAuth 2.0 + * Authorization Request} via the {@link OAuth2AuthorizationRequest.Builder}. + * + * @author Joe Grandja + * @since 5.7 + * @see OAuth2AuthorizationRequest.Builder + * @see DefaultOAuth2AuthorizationRequestResolver#setAuthorizationRequestCustomizer(Consumer) + * @see DefaultServerOAuth2AuthorizationRequestResolver#setAuthorizationRequestCustomizer(Consumer) + */ +public final class OAuth2AuthorizationRequestCustomizers { + + private static final StringKeyGenerator DEFAULT_SECURE_KEY_GENERATOR = new Base64StringKeyGenerator( + Base64.getUrlEncoder().withoutPadding(), 96); + + private OAuth2AuthorizationRequestCustomizers() { + } + + /** + * Returns a {@code Consumer} to be provided the + * {@link OAuth2AuthorizationRequest.Builder} that adds the + * {@link PkceParameterNames#CODE_CHALLENGE code_challenge} and, usually, + * {@link PkceParameterNames#CODE_CHALLENGE_METHOD code_challenge_method} parameters + * to the OAuth 2.0 Authorization Request. The {@code code_verifier} is stored in + * {@link OAuth2AuthorizationRequest#getAttribute(String)} under the key + * {@link PkceParameterNames#CODE_VERIFIER code_verifier} for subsequent use in the + * OAuth 2.0 Access Token Request. + * @return a {@code Consumer} to be provided the + * {@link OAuth2AuthorizationRequest.Builder} that adds the PKCE parameters + * @see 1.1. Protocol Flow + * @see 4.1. Client Creates a + * Code Verifier + * @see 4.2. Client Creates the + * Code Challenge + */ + public static Consumer withPkce() { + return OAuth2AuthorizationRequestCustomizers::applyPkce; + } + + private static void applyPkce(OAuth2AuthorizationRequest.Builder builder) { + if (isPkceAlreadyApplied(builder)) { + return; + } + + String codeVerifier = DEFAULT_SECURE_KEY_GENERATOR.generateKey(); + + builder.attributes((attrs) -> attrs.put(PkceParameterNames.CODE_VERIFIER, codeVerifier)); + + builder.additionalParameters((params) -> { + try { + String codeChallenge = createHash(codeVerifier); + params.put(PkceParameterNames.CODE_CHALLENGE, codeChallenge); + params.put(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256"); + } + catch (NoSuchAlgorithmException ex) { + params.put(PkceParameterNames.CODE_CHALLENGE, codeVerifier); + } + }); + } + + private static boolean isPkceAlreadyApplied(OAuth2AuthorizationRequest.Builder builder) { + AtomicBoolean pkceApplied = new AtomicBoolean(false); + builder.additionalParameters((params) -> { + if (params.containsKey(PkceParameterNames.CODE_CHALLENGE)) { + pkceApplied.set(true); + } + }); + return pkceApplied.get(); + } + + private static String createHash(String value) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] digest = md.digest(value.getBytes(StandardCharsets.US_ASCII)); + return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + } + +} diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java index a3a0169189..b5f557bffe 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -32,11 +32,11 @@ import org.springframework.security.crypto.keygen.Base64StringKeyGenerator; import org.springframework.security.crypto.keygen.StringKeyGenerator; import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository; +import org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestCustomizers; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; -import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; import org.springframework.security.oauth2.core.oidc.OidcScopes; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.web.server.util.matcher.PathPatternParserServerWebExchangeMatcher; @@ -59,6 +59,7 @@ import org.springframework.web.util.UriComponentsBuilder; * * @author Rob Winch * @author Mark Heckler + * @author Joe Grandja * @since 5.1 */ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOAuth2AuthorizationRequestResolver { @@ -78,15 +79,19 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA private static final char PATH_DELIMITER = '/'; + private static final StringKeyGenerator DEFAULT_STATE_GENERATOR = new Base64StringKeyGenerator( + Base64.getUrlEncoder()); + + private static final StringKeyGenerator DEFAULT_SECURE_KEY_GENERATOR = new Base64StringKeyGenerator( + Base64.getUrlEncoder().withoutPadding(), 96); + + private static final Consumer DEFAULT_PKCE_APPLIER = OAuth2AuthorizationRequestCustomizers + .withPkce(); + private final ServerWebExchangeMatcher authorizationRequestMatcher; private final ReactiveClientRegistrationRepository clientRegistrationRepository; - private final StringKeyGenerator stateGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder()); - - private final StringKeyGenerator secureKeyGenerator = new Base64StringKeyGenerator( - Base64.getUrlEncoder().withoutPadding(), 96); - private Consumer authorizationRequestCustomizer = (customizer) -> { }; @@ -133,7 +138,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA @Override public Mono resolve(ServerWebExchange exchange, String clientRegistrationId) { - return this.findByRegistrationId(exchange, clientRegistrationId) + return findByRegistrationId(exchange, clientRegistrationId) .map((clientRegistration) -> authorizationRequest(exchange, clientRegistration)); } @@ -143,6 +148,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA * @param authorizationRequestCustomizer the {@code Consumer} to be provided the * {@link OAuth2AuthorizationRequest.Builder} * @since 5.3 + * @see OAuth2AuthorizationRequestCustomizers */ public final void setAuthorizationRequestCustomizer( Consumer authorizationRequestCustomizer) { @@ -159,17 +165,14 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA private OAuth2AuthorizationRequest authorizationRequest(ServerWebExchange exchange, ClientRegistration clientRegistration) { + OAuth2AuthorizationRequest.Builder builder = getBuilder(clientRegistration); String redirectUriStr = expandRedirectUri(exchange.getRequest(), clientRegistration); - Map attributes = new HashMap<>(); - attributes.put(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId()); - OAuth2AuthorizationRequest.Builder builder = getBuilder(clientRegistration, attributes); // @formatter:off builder.clientId(clientRegistration.getClientId()) .authorizationUri(clientRegistration.getProviderDetails().getAuthorizationUri()) .redirectUri(redirectUriStr) .scopes(clientRegistration.getScopes()) - .state(this.stateGenerator.generateKey()) - .attributes(attributes); + .state(DEFAULT_STATE_GENERATOR.generateKey()); // @formatter:on this.authorizationRequestCustomizer.accept(builder); @@ -177,11 +180,13 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA return builder.build(); } - private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientRegistration, - Map attributes) { + private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientRegistration) { if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) { - OAuth2AuthorizationRequest.Builder builder = OAuth2AuthorizationRequest.authorizationCode(); - Map additionalParameters = new HashMap<>(); + // @formatter:off + OAuth2AuthorizationRequest.Builder builder = OAuth2AuthorizationRequest.authorizationCode() + .attributes((attrs) -> + attrs.put(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + // @formatter:on if (!CollectionUtils.isEmpty(clientRegistration.getScopes()) && clientRegistration.getScopes().contains(OidcScopes.OPENID)) { // Section 3.1.2.1 Authentication Request - @@ -189,12 +194,11 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA // scope // REQUIRED. OpenID Connect requests MUST contain the "openid" scope // value. - addNonceParameters(attributes, additionalParameters); + applyNonce(builder); } if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { - addPkceParameters(attributes, additionalParameters); + DEFAULT_PKCE_APPLIER.accept(builder); } - builder.additionalParameters(additionalParameters); return builder; } if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { @@ -261,57 +265,25 @@ public class DefaultServerOAuth2AuthorizationRequestResolver implements ServerOA /** * Creates nonce and its hash for use in OpenID Connect 1.0 Authentication Requests. - * @param attributes where the {@link OidcParameterNames#NONCE} is stored for the - * authentication request - * @param additionalParameters where the {@link OidcParameterNames#NONCE} hash is - * added for the authentication request + * @param builder where the {@link OidcParameterNames#NONCE} and hash is stored for + * the authentication request * * @since 5.2 * @see 3.1.2.1. * Authentication Request */ - private void addNonceParameters(Map attributes, Map additionalParameters) { + private static void applyNonce(OAuth2AuthorizationRequest.Builder builder) { try { - String nonce = this.secureKeyGenerator.generateKey(); + String nonce = DEFAULT_SECURE_KEY_GENERATOR.generateKey(); String nonceHash = createHash(nonce); - attributes.put(OidcParameterNames.NONCE, nonce); - additionalParameters.put(OidcParameterNames.NONCE, nonceHash); + builder.attributes((attrs) -> attrs.put(OidcParameterNames.NONCE, nonce)); + builder.additionalParameters((params) -> params.put(OidcParameterNames.NONCE, nonceHash)); } catch (NoSuchAlgorithmException ex) { } } - /** - * Creates and adds additional PKCE parameters for use in the OAuth 2.0 Authorization - * and Access Token Requests - * @param attributes where {@link PkceParameterNames#CODE_VERIFIER} is stored for the - * token request - * @param additionalParameters where {@link PkceParameterNames#CODE_CHALLENGE} and, - * usually, {@link PkceParameterNames#CODE_CHALLENGE_METHOD} are added to be used in - * the authorization request. - * - * @since 5.2 - * @see 1.1. - * Protocol Flow - * @see 4.1. - * Client Creates a Code Verifier - * @see 4.2. - * Client Creates the Code Challenge - */ - private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.secureKeyGenerator.generateKey(); - attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); - try { - String codeChallenge = createHash(codeVerifier); - additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeChallenge); - additionalParameters.put(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256"); - } - catch (NoSuchAlgorithmException ex) { - additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeVerifier); - } - } - private static String createHash(String value) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); byte[] digest = md.digest(value.getBytes(StandardCharsets.US_ASCII)); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java index 5c91205e4a..0345f55a29 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -57,7 +57,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { private ClientRegistration fineRedirectUriTemplateRegistration; - private ClientRegistration pkceRegistration; + private ClientRegistration publicClientRegistration; private ClientRegistration oidcRegistration; @@ -73,9 +73,9 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { this.registration2 = TestClientRegistrations.clientRegistration2().build(); this.fineRedirectUriTemplateRegistration = fineRedirectUriTemplateClientRegistration().build(); // @formatter:off - this.pkceRegistration = TestClientRegistrations.clientRegistration() - .registrationId("pkce-client-registration-id") - .clientId("pkce-client-id") + this.publicClientRegistration = TestClientRegistrations.clientRegistration() + .registrationId("public-client-registration-id") + .clientId("public-client-id") .clientAuthenticationMethod(ClientAuthenticationMethod.NONE) .clientSecret(null) .build(); @@ -85,7 +85,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { .build(); // @formatter:on this.clientRegistrationRepository = new InMemoryClientRegistrationRepository(this.registration1, - this.registration2, this.fineRedirectUriTemplateRegistration, this.pkceRegistration, + this.registration2, this.fineRedirectUriTemplateRegistration, this.publicClientRegistration, this.oidcRegistration); this.resolver = new DefaultOAuth2AuthorizationRequestResolver(this.clientRegistrationRepository, this.authorizationRequestBaseUri); @@ -371,8 +371,8 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { } @Test - public void resolveWhenAuthorizationRequestWithValidPkceClientThenResolves() { - ClientRegistration clientRegistration = this.pkceRegistration; + public void resolveWhenAuthorizationRequestWithValidPublicClientThenResolves() { + ClientRegistration clientRegistration = this.publicClientRegistration; String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); request.setServletPath(requestUri); @@ -398,10 +398,84 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { assertThat((String) authorizationRequest.getAttribute(PkceParameterNames.CODE_VERIFIER)) .matches("^([a-zA-Z0-9\\-\\.\\_\\~]){128}$"); assertThat(authorizationRequest.getAuthorizationRequestUri()) - .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=pkce-client-id&" - + "scope=read:user&state=.{15,}&" - + "redirect_uri=http://localhost/login/oauth2/code/pkce-client-registration-id&" - + "code_challenge_method=S256&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + .matches("https://example.com/login/oauth/authorize\\?" + + "response_type=code&client_id=public-client-id&" + "scope=read:user&state=.{15,}&" + + "redirect_uri=http://localhost/login/oauth2/code/public-client-registration-id&" + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge_method=S256"); + } + + // gh-6548 + @Test + public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientsThenApplied() { + this.resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce()); + + ClientRegistration clientRegistration = this.registration1; + String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); + MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); + request.setServletPath(requestUri); + OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); + assertPkceApplied(authorizationRequest, clientRegistration); + + clientRegistration = this.registration2; + requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); + request = new MockHttpServletRequest("GET", requestUri); + request.setServletPath(requestUri); + authorizationRequest = this.resolver.resolve(request); + assertPkceApplied(authorizationRequest, clientRegistration); + } + + // gh-6548 + @Test + public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClientThenApplied() { + this.resolver.setAuthorizationRequestCustomizer((builder) -> { + builder.attributes((attrs) -> { + String registrationId = (String) attrs.get(OAuth2ParameterNames.REGISTRATION_ID); + if (this.registration1.getRegistrationId().equals(registrationId)) { + OAuth2AuthorizationRequestCustomizers.withPkce().accept(builder); + } + }); + }); + + ClientRegistration clientRegistration = this.registration1; + String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); + MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); + request.setServletPath(requestUri); + OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); + assertPkceApplied(authorizationRequest, clientRegistration); + + clientRegistration = this.registration2; + requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); + request = new MockHttpServletRequest("GET", requestUri); + request.setServletPath(requestUri); + authorizationRequest = this.resolver.resolve(request); + assertPkceNotApplied(authorizationRequest, clientRegistration); + } + + private void assertPkceApplied(OAuth2AuthorizationRequest authorizationRequest, + ClientRegistration clientRegistration) { + assertThat(authorizationRequest.getAdditionalParameters()).containsKey(PkceParameterNames.CODE_CHALLENGE); + assertThat(authorizationRequest.getAdditionalParameters()) + .contains(entry(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256")); + assertThat(authorizationRequest.getAttributes()).containsKey(PkceParameterNames.CODE_VERIFIER); + assertThat((String) authorizationRequest.getAttribute(PkceParameterNames.CODE_VERIFIER)) + .matches("^([a-zA-Z0-9\\-\\.\\_\\~]){128}$"); + assertThat(authorizationRequest.getAuthorizationRequestUri()) + .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&" + "client_id=" + + clientRegistration.getClientId() + "&" + "scope=read:user&" + "state=.{15,}&" + + "redirect_uri=http://localhost/login/oauth2/code/" + clientRegistration.getRegistrationId() + + "&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge_method=S256"); + } + + private void assertPkceNotApplied(OAuth2AuthorizationRequest authorizationRequest, + ClientRegistration clientRegistration) { + assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(PkceParameterNames.CODE_CHALLENGE); + assertThat(authorizationRequest.getAdditionalParameters()) + .doesNotContainKey(PkceParameterNames.CODE_CHALLENGE_METHOD); + assertThat(authorizationRequest.getAttributes()).doesNotContainKey(PkceParameterNames.CODE_VERIFIER); + assertThat(authorizationRequest.getAuthorizationRequestUri()) + .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&" + "client_id=" + + clientRegistration.getClientId() + "&" + "scope=read:user&" + "state=.{15,}&" + + "redirect_uri=http://localhost/login/oauth2/code/" + clientRegistration.getRegistrationId()); } @Test @@ -444,7 +518,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); request.setServletPath(requestUri); this.resolver.setAuthorizationRequestCustomizer( - (customizer) -> customizer.additionalParameters((params) -> params.remove(OidcParameterNames.NONCE)) + (builder) -> builder.additionalParameters((params) -> params.remove(OidcParameterNames.NONCE)) .attributes((attrs) -> attrs.remove(OidcParameterNames.NONCE))); OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(OidcParameterNames.NONCE); @@ -462,11 +536,10 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); request.setServletPath(requestUri); - this.resolver - .setAuthorizationRequestCustomizer((customizer) -> customizer.authorizationRequestUri((uriBuilder) -> { - uriBuilder.queryParam("param1", "value1"); - return uriBuilder.build(); - })); + this.resolver.setAuthorizationRequestCustomizer((builder) -> builder.authorizationRequestUri((uriBuilder) -> { + uriBuilder.queryParam("param1", "value1"); + return uriBuilder.build(); + })); OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" @@ -481,7 +554,7 @@ public class DefaultOAuth2AuthorizationRequestResolverTests { String requestUri = this.authorizationRequestBaseUri + "/" + clientRegistration.getRegistrationId(); MockHttpServletRequest request = new MockHttpServletRequest("GET", requestUri); request.setServletPath(requestUri); - this.resolver.setAuthorizationRequestCustomizer((customizer) -> customizer.parameters((params) -> { + this.resolver.setAuthorizationRequestCustomizer((builder) -> builder.parameters((params) -> { params.put("appid", params.get("client_id")); params.remove("client_id"); })); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java index 0762eba0b6..464d641c96 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolverTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -29,6 +29,7 @@ import org.springframework.mock.web.server.MockServerWebExchange; import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository; import org.springframework.security.oauth2.client.registration.TestClientRegistrations; +import org.springframework.security.oauth2.client.web.OAuth2AuthorizationRequestCustomizers; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; @@ -41,7 +42,9 @@ import org.springframework.web.server.ServerWebExchange; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.entry; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; /** @@ -106,7 +109,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { } @Test - public void resolveWhenAuthorizationRequestWithValidPkceClientThenResolves() { + public void resolveWhenAuthorizationRequestWithValidPublicClientThenResolves() { given(this.clientRegistrationRepository.findByRegistrationId(any())) .willReturn(Mono.just(TestClientRegistrations.clientRegistration() .clientAuthenticationMethod(ClientAuthenticationMethod.NONE).clientSecret(null).build())); @@ -116,7 +119,79 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { assertThat(request.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id&" - + "code_challenge_method=S256&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge_method=S256"); + } + + // gh-6548 + @Test + public void resolveWhenAuthorizationRequestApplyPkceToConfidentialClientsThenApplied() { + ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build(); + given(this.clientRegistrationRepository.findByRegistrationId(eq(registration1.getRegistrationId()))) + .willReturn(Mono.just(registration1)); + ClientRegistration registration2 = TestClientRegistrations.clientRegistration2().build(); + given(this.clientRegistrationRepository.findByRegistrationId(eq(registration2.getRegistrationId()))) + .willReturn(Mono.just(registration2)); + + this.resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce()); + + OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId()); + assertPkceApplied(request, registration1); + + request = resolve("/oauth2/authorization/" + registration2.getRegistrationId()); + assertPkceApplied(request, registration2); + } + + // gh-6548 + @Test + public void resolveWhenAuthorizationRequestApplyPkceToSpecificConfidentialClientThenApplied() { + ClientRegistration registration1 = TestClientRegistrations.clientRegistration().build(); + given(this.clientRegistrationRepository.findByRegistrationId(eq(registration1.getRegistrationId()))) + .willReturn(Mono.just(registration1)); + ClientRegistration registration2 = TestClientRegistrations.clientRegistration2().build(); + given(this.clientRegistrationRepository.findByRegistrationId(eq(registration2.getRegistrationId()))) + .willReturn(Mono.just(registration2)); + + this.resolver.setAuthorizationRequestCustomizer((builder) -> { + builder.attributes((attrs) -> { + String registrationId = (String) attrs.get(OAuth2ParameterNames.REGISTRATION_ID); + if (registration1.getRegistrationId().equals(registrationId)) { + OAuth2AuthorizationRequestCustomizers.withPkce().accept(builder); + } + }); + }); + + OAuth2AuthorizationRequest request = resolve("/oauth2/authorization/" + registration1.getRegistrationId()); + assertPkceApplied(request, registration1); + + request = resolve("/oauth2/authorization/" + registration2.getRegistrationId()); + assertPkceNotApplied(request, registration2); + } + + private void assertPkceApplied(OAuth2AuthorizationRequest authorizationRequest, + ClientRegistration clientRegistration) { + assertThat(authorizationRequest.getAdditionalParameters()).containsKey(PkceParameterNames.CODE_CHALLENGE); + assertThat(authorizationRequest.getAdditionalParameters()) + .contains(entry(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256")); + assertThat(authorizationRequest.getAttributes()).containsKey(PkceParameterNames.CODE_VERIFIER); + assertThat((String) authorizationRequest.getAttribute(PkceParameterNames.CODE_VERIFIER)) + .matches("^([a-zA-Z0-9\\-\\.\\_\\~]){128}$"); + assertThat(authorizationRequest.getAuthorizationRequestUri()) + .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&" + "client_id=" + + clientRegistration.getClientId() + "&" + "scope=read:user&" + "state=.{15,}&" + + "redirect_uri=/login/oauth2/code/" + clientRegistration.getRegistrationId() + "&" + + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge_method=S256"); + } + + private void assertPkceNotApplied(OAuth2AuthorizationRequest authorizationRequest, + ClientRegistration clientRegistration) { + assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(PkceParameterNames.CODE_CHALLENGE); + assertThat(authorizationRequest.getAdditionalParameters()) + .doesNotContainKey(PkceParameterNames.CODE_CHALLENGE_METHOD); + assertThat(authorizationRequest.getAttributes()).doesNotContainKey(PkceParameterNames.CODE_VERIFIER); + assertThat(authorizationRequest.getAuthorizationRequestUri()) + .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&" + "client_id=" + + clientRegistration.getClientId() + "&" + "scope=read:user&" + "state=.{15,}&" + + "redirect_uri=/login/oauth2/code/" + clientRegistration.getRegistrationId()); } @Test @@ -136,7 +211,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { given(this.clientRegistrationRepository.findByRegistrationId(any())) .willReturn(Mono.just(TestClientRegistrations.clientRegistration().scope(OidcScopes.OPENID).build())); this.resolver.setAuthorizationRequestCustomizer( - (customizer) -> customizer.additionalParameters((params) -> params.remove(OidcParameterNames.NONCE)) + (builder) -> builder.additionalParameters((params) -> params.remove(OidcParameterNames.NONCE)) .attributes((attrs) -> attrs.remove(OidcParameterNames.NONCE))); OAuth2AuthorizationRequest authorizationRequest = resolve("/oauth2/authorization/registration-id"); assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(OidcParameterNames.NONCE); @@ -151,11 +226,10 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { public void resolveWhenAuthorizationRequestCustomizerAddsParameterThenQueryIncludesParameter() { given(this.clientRegistrationRepository.findByRegistrationId(any())) .willReturn(Mono.just(TestClientRegistrations.clientRegistration().scope(OidcScopes.OPENID).build())); - this.resolver - .setAuthorizationRequestCustomizer((customizer) -> customizer.authorizationRequestUri((uriBuilder) -> { - uriBuilder.queryParam("param1", "value1"); - return uriBuilder.build(); - })); + this.resolver.setAuthorizationRequestCustomizer((builder) -> builder.authorizationRequestUri((uriBuilder) -> { + uriBuilder.queryParam("param1", "value1"); + return uriBuilder.build(); + })); OAuth2AuthorizationRequest authorizationRequest = resolve("/oauth2/authorization/registration-id"); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" @@ -167,7 +241,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolverTests { public void resolveWhenAuthorizationRequestCustomizerOverridesParameterThenQueryIncludesParameter() { given(this.clientRegistrationRepository.findByRegistrationId(any())) .willReturn(Mono.just(TestClientRegistrations.clientRegistration().scope(OidcScopes.OPENID).build())); - this.resolver.setAuthorizationRequestCustomizer((customizer) -> customizer.parameters((params) -> { + this.resolver.setAuthorizationRequestCustomizer((builder) -> builder.parameters((params) -> { params.put("appid", params.get("client_id")); params.remove("client_id"); }));