From 8ca42a43cb80f6676241a8b07d14e7220f126d9e Mon Sep 17 00:00:00 2001 From: Mark Heckler Date: Sun, 1 Sep 2019 18:47:13 -0500 Subject: [PATCH 1/6] gh4442 https://github.com/spring-projects/spring-security/issues/4442 Added nonce to OAuth2 Authentication Request, also added comparison+verification in IdToken --- .../client/OAuth2ClientConfigurerTests.java | 6 ++- .../DefaultOidcIdTokenValidatorFactory.java | 1 + .../authentication/OidcIdTokenValidator.java | 37 +++++++++++++- ...ultOAuth2AuthorizationRequestResolver.java | 39 ++++++++++++--- ...verOAuth2AuthorizationRequestResolver.java | 45 +++++++++++++---- .../OidcIdTokenValidatorTests.java | 48 +++++++++++++++---- ...uth2AuthorizationRequestResolverTests.java | 33 ++++++++----- ...uth2AuthorizationRequestResolverTests.java | 7 ++- .../core/endpoint/NonceParameterNames.java | 17 +++++++ 9 files changed, 191 insertions(+), 42 deletions(-) create mode 100644 oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java index 86e4b4e3c41..3e7b2624a4c 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java @@ -138,7 +138,8 @@ public void configureWhenAuthorizationCodeRequestThenRedirectForAuthorization() assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + - "redirect_uri=http://localhost/client-1"); + "redirect_uri=http://localhost/client-1&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -203,7 +204,8 @@ public void configureWhenRequestCacheProvidedAndClientAuthorizationRequiredExcep assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + - "redirect_uri=http://localhost/client-1"); + "redirect_uri=http://localhost/client-1&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); verify(requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)); } diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java index 70c5a6749c1..5f3fab6f20e 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java @@ -18,6 +18,7 @@ import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.oauth2.core.DelegatingOAuth2TokenValidator; import org.springframework.security.oauth2.core.OAuth2TokenValidator; +import org.springframework.security.oauth2.core.oidc.OidcIdToken; import org.springframework.security.oauth2.jwt.Jwt; import org.springframework.security.oauth2.jwt.JwtTimestampValidator; diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java index 806c837112f..21ce69d4c49 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java @@ -27,8 +27,12 @@ import org.springframework.util.CollectionUtils; import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.time.Duration; import java.time.Instant; +import java.util.Base64; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -47,13 +51,21 @@ public final class OidcIdTokenValidator implements OAuth2TokenValidator { private static final Duration DEFAULT_CLOCK_SKEW = Duration.ofSeconds(60); private final ClientRegistration clientRegistration; + private String nonce = ""; private Duration clockSkew = DEFAULT_CLOCK_SKEW; public OidcIdTokenValidator(ClientRegistration clientRegistration) { + this(clientRegistration, ""); + } + + public OidcIdTokenValidator(ClientRegistration clientRegistration, String nonce) { Assert.notNull(clientRegistration, "clientRegistration cannot be null"); this.clientRegistration = clientRegistration; + + this.nonce = nonce; } + @Override public OAuth2TokenValidatorResult validate(Jwt idToken) { // 3.1.3.7 ID Token Validation @@ -112,7 +124,18 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { // that it is the same value as the one that was sent in the Authentication Request. // The Client SHOULD check the nonce value for replay attacks. // The precise method for detecting replay attacks is Client specific. - // TODO Depends on gh-4442 + // Depends on gh-4442 + String nonceHash = ""; + try { + nonceHash = createHash(nonce); + } catch (NoSuchAlgorithmException e) { + //e.printStackTrace(); + // MH: ??? + } + String nonceClaim = idToken.getClaim(IdTokenClaimNames.NONCE); + if (nonceClaim == null || !nonceClaim.equals(nonceHash)) { + invalidClaims.put(IdTokenClaimNames.NONCE, idToken.getClaim(IdTokenClaimNames.NONCE)); + } if (!invalidClaims.isEmpty()) { return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims)); @@ -156,6 +179,11 @@ private static Map validateRequiredClaims(Jwt idToken) { if (CollectionUtils.isEmpty(audience)) { requiredClaims.put(IdTokenClaimNames.AUD, audience); } + //String nonce = idToken.getNonce(); MH: We should be using OidcToken for this instead of Jwt. See DefaultOidcIdTokenValidatorFactory + String nonce = idToken.getClaim(IdTokenClaimNames.NONCE); + if (nonce == null) { + requiredClaims.put(IdTokenClaimNames.NONCE, nonce); + } Instant expiresAt = idToken.getExpiresAt(); if (expiresAt == null) { requiredClaims.put(IdTokenClaimNames.EXP, expiresAt); @@ -167,4 +195,11 @@ private static Map validateRequiredClaims(Jwt idToken) { return requiredClaims; } + + private String createHash(String nonce) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] digest = md.digest(nonce.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/DefaultOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java index 84e2ec47be8..09dd1a2f918 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 @@ -21,6 +21,7 @@ import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; +import org.springframework.security.oauth2.core.endpoint.NonceParameterNames; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; @@ -61,7 +62,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au private final ClientRegistrationRepository clientRegistrationRepository; private final AntPathRequestMatcher authorizationRequestMatcher; private final StringKeyGenerator stateGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder()); - private final StringKeyGenerator codeVerifierGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + private final StringKeyGenerator randomKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); /** * Constructs a {@code DefaultOAuth2AuthorizationRequestResolver} using the provided parameters. @@ -118,11 +119,15 @@ private OAuth2AuthorizationRequest resolve(HttpServletRequest request, String re OAuth2AuthorizationRequest.Builder builder; if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.authorizationCode(); + Map additionalParameters = new HashMap<>(); + + addNonceHash(attributes, additionalParameters); + if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { - Map additionalParameters = new HashMap<>(); addPkceParameters(attributes, additionalParameters); - builder.additionalParameters(additionalParameters); } + + builder.additionalParameters(additionalParameters); } else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.implicit(); } else { @@ -201,6 +206,28 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist .toUriString(); } + /** + * Created and adds nonce for use in OpenID Connect Authentication Requests + * + * @param attributes where {@link PkceParameterNames#CODE_VERIFIER} is stored for the token request + * @param additionalParameters where {@link NonceParameterNames#NONCE} is added to be used in the authentication request. + * + * @since 5.2 MAH fix links + * @see 15.5.2. Nonce Implementation Notes + * @see 3.1.3.7. ID Token Validation + */ + private void addNonceHash(Map attributes, Map additionalParameters) { + String nonce = this.randomKeyGenerator.generateKey(); + attributes.put(NonceParameterNames.NONCE, nonce); + + try { + String hashNonce = createHash(nonce); + additionalParameters.put(NonceParameterNames.NONCE, hashNonce); + } catch (NoSuchAlgorithmException e) { + // MAH: TODO...but what? + } + } + /** * Creates and adds additional PKCE parameters for use in the OAuth 2.0 Authorization and Access Token Requests * @@ -214,10 +241,10 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist * @see 4.2. Client Creates the Code Challenge */ private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.codeVerifierGenerator.generateKey(); + String codeVerifier = this.randomKeyGenerator.generateKey(); attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); try { - String codeChallenge = createCodeChallenge(codeVerifier); + String codeChallenge = createHash(codeVerifier); additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeChallenge); additionalParameters.put(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256"); } catch (NoSuchAlgorithmException e) { @@ -225,7 +252,7 @@ private void addPkceParameters(Map attributes, Map additionalParameters = new HashMap<>(); + + addNonceHash(attributes, additionalParameters); + if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { - Map additionalParameters = new HashMap<>(); addPkceParameters(attributes, additionalParameters); - builder.additionalParameters(additionalParameters); } - } - else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { + + builder.additionalParameters(additionalParameters); + } else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) { builder = OAuth2AuthorizationRequest.implicit(); - } - else { + } else { throw new IllegalArgumentException( "Invalid Authorization Grant Type (" + clientRegistration.getAuthorizationGrantType().getValue() + ") for Client Registration with Id: " + clientRegistration.getRegistrationId()); @@ -207,6 +210,28 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr .toUriString(); } + /** + * Created and adds nonce for use in OpenID Connect Authentication Requests + * + * @param attributes where {@link PkceParameterNames#CODE_VERIFIER} is stored for the token request + * @param additionalParameters where {@link NonceParameterNames#NONCE} is added to be used in the authentication request. + * + * @since 5.2 MAH fix links + * @see 15.5.2. Nonce Implementation Notes + * @see 3.1.3.7. ID Token Validation + */ + private void addNonceHash(Map attributes, Map additionalParameters) { + String nonce = this.randomKeyGenerator.generateKey(); + attributes.put(NonceParameterNames.NONCE, nonce); + + try { + String hashNonce = createHash(nonce); + additionalParameters.put(NonceParameterNames.NONCE, hashNonce); + } catch (NoSuchAlgorithmException e) { + // MAH: TODO...but what? + } + } + /** * Creates and adds additional PKCE parameters for use in the OAuth 2.0 Authorization and Access Token Requests * @@ -220,10 +245,10 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr * @see 4.2. Client Creates the Code Challenge */ private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.codeVerifierGenerator.generateKey(); + String codeVerifier = this.randomKeyGenerator.generateKey(); attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); try { - String codeChallenge = createCodeChallenge(codeVerifier); + String codeChallenge = createHash(codeVerifier); additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeChallenge); additionalParameters.put(PkceParameterNames.CODE_CHALLENGE_METHOD, "S256"); } catch (NoSuchAlgorithmException e) { @@ -231,7 +256,7 @@ private void addPkceParameters(Map attributes, Map msg.contains(IdTokenClaimNames.AUD)); } + @Test + public void validateWhenNonceNullThenHasErrors() { + this.claims.remove(IdTokenClaimNames.NONCE); + assertThat(this.validateIdToken()) + .hasSize(1) + .extracting(OAuth2Error::getDescription) + .allMatch(msg -> msg.contains(IdTokenClaimNames.NONCE)); + } + @Test public void validateWhenIssuedAtNullThenHasErrors() { this.issuedAt = null; @@ -165,6 +182,13 @@ public void validateWhenAudNotClientIdThenHasErrors() { .allMatch(msg -> msg.contains(IdTokenClaimNames.AUD)); } +/* + @Test + public void validateWhenNonceHashMatchesThenErrors() { + assertThat(this.validateIdToken()).isEmpty(); + } +*/ + @Test public void validateWhenExpiredAnd60secClockSkewThenNoErrors() { this.issuedAt = Instant.now().minus(Duration.ofSeconds(60)); @@ -218,6 +242,7 @@ public void validateWhenExpiresAtBeforeNowThenHasErrors() { public void validateWhenMissingClaimsThenHasErrors() { this.claims.remove(IdTokenClaimNames.SUB); this.claims.remove(IdTokenClaimNames.AUD); + this.claims.remove(IdTokenClaimNames.NONCE); this.issuedAt = null; this.expiresAt = null; assertThat(this.validateIdToken()) @@ -226,7 +251,8 @@ public void validateWhenMissingClaimsThenHasErrors() { .allMatch(msg -> msg.contains(IdTokenClaimNames.SUB)) .allMatch(msg -> msg.contains(IdTokenClaimNames.AUD)) .allMatch(msg -> msg.contains(IdTokenClaimNames.IAT)) - .allMatch(msg -> msg.contains(IdTokenClaimNames.EXP)); + .allMatch(msg -> msg.contains(IdTokenClaimNames.EXP)) + .allMatch(msg -> msg.contains(IdTokenClaimNames.NONCE)); } @Test @@ -241,8 +267,14 @@ public void validateFormatError() { private Collection validateIdToken() { Jwt idToken = new Jwt("token123", this.issuedAt, this.expiresAt, this.headers, this.claims); - OidcIdTokenValidator validator = new OidcIdTokenValidator(this.registration.build()); + OidcIdTokenValidator validator = new OidcIdTokenValidator(this.registration.build(), nonce); validator.setClockSkew(this.clockSkew); return validator.validate(idToken).getErrors(); } + + private String createHash(String nonce) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); + return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + } } 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 665755e58ad..762fd8a0fb6 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 @@ -24,10 +24,7 @@ import org.springframework.security.oauth2.client.registration.TestClientRegistrations; 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.OAuth2AuthorizationResponseType; -import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; -import org.springframework.security.oauth2.core.endpoint.PkceParameterNames; +import org.springframework.security.oauth2.core.endpoint.*; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -119,12 +116,14 @@ public void resolveWhenAuthorizationRequestWithValidClientThenResolves() { assertThat(authorizationRequest.getState()).isNotNull(); assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(OAuth2ParameterNames.REGISTRATION_ID); assertThat(authorizationRequest.getAttributes()) - .containsExactly(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, NonceParameterNames.NONCE) + .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -137,7 +136,8 @@ public void resolveWhenClientAuthorizationRequiredExceptionAvailableThenResolves OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request, clientRegistration.getRegistrationId()); assertThat(authorizationRequest).isNotNull(); assertThat(authorizationRequest.getAttributes()) - .containsExactly(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); + .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, NonceParameterNames.NONCE) + .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); } @Test @@ -259,7 +259,8 @@ public void resolveWhenAuthorizationRequestIncludesPort80ThenExpandedRedirectUri .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -277,7 +278,8 @@ public void resolveWhenAuthorizationRequestIncludesPort443ThenExpandedRedirectUr .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=https://example.com/login/oauth2/code/registration-id"); + "redirect_uri=https://example.com/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -292,7 +294,8 @@ public void resolveWhenClientAuthorizationRequiredExceptionAvailableThenRedirect .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -307,7 +310,8 @@ public void resolveWhenAuthorizationRequestOAuth2LoginThenRedirectUriIsLogin() { .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id-2&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id-2"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -323,7 +327,8 @@ public void resolveWhenAuthorizationRequestHasActionParameterAuthorizeThenRedire .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -339,7 +344,8 @@ public void resolveWhenAuthorizationRequestHasActionParameterLoginThenRedirectUr .matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id-2&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id-2"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id-2&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -375,6 +381,7 @@ public void resolveWhenAuthorizationRequestWithValidPkceClientThenResolves() { "scope=read:user&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/pkce-client-registration-id&" + "code_challenge_method=S256&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } 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 86a722d18ca..8fc504efc91 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 @@ -82,7 +82,8 @@ public void resolveWhenClientRegistrationFoundThenWorks() { 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"); + "redirect_uri=/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } private OAuth2AuthorizationRequest resolve(String path) { @@ -101,7 +102,8 @@ public void resolveWhenForwardedHeadersClientRegistrationFoundThenWorks() { 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"); + "redirect_uri=/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -121,6 +123,7 @@ public void resolveWhenAuthorizationRequestWithValidPkceClientThenResolves() { "scope=read:user&state=.*?&" + "redirect_uri=/login/oauth2/code/registration-id&" + "code_challenge_method=S256&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "code_challenge=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } } diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java new file mode 100644 index 00000000000..b529255633b --- /dev/null +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java @@ -0,0 +1,17 @@ +package org.springframework.security.oauth2.core.endpoint; + +/** + * Standard parameter names defined in the OpenID Connect Core 1.0 + * incorporating errata set 1 and used by the authorization endpoint and + * token endpoint. + * + * @author Mark Heckler + * @since 5.2 + * @see 15.5.2. Nonce Implementation Notes + */ +public interface NonceParameterNames { + /** + * {@code nonce} - used in Authentication Request. + */ + String NONCE = "nonce"; +} From 68c1cbf478fd41687a0d7c95205c1f75f4be14a1 Mon Sep 17 00:00:00 2001 From: Mark Heckler Date: Mon, 2 Sep 2019 10:45:19 -0500 Subject: [PATCH 2/6] Github issue 4442 (https://github.com/spring-projects/spring-security/issues/4442) completion, with follow-on issue to be discussed w/team --- .../oidc/authentication/DefaultOidcIdTokenValidatorFactory.java | 1 - .../oauth2/client/oidc/authentication/OidcIdTokenValidator.java | 1 - 2 files changed, 2 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java index 5f3fab6f20e..70c5a6749c1 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/DefaultOidcIdTokenValidatorFactory.java @@ -18,7 +18,6 @@ import org.springframework.security.oauth2.client.registration.ClientRegistration; import org.springframework.security.oauth2.core.DelegatingOAuth2TokenValidator; import org.springframework.security.oauth2.core.OAuth2TokenValidator; -import org.springframework.security.oauth2.core.oidc.OidcIdToken; import org.springframework.security.oauth2.jwt.Jwt; import org.springframework.security.oauth2.jwt.JwtTimestampValidator; diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java index 21ce69d4c49..4b4367cf7a1 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java @@ -124,7 +124,6 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { // that it is the same value as the one that was sent in the Authentication Request. // The Client SHOULD check the nonce value for replay attacks. // The precise method for detecting replay attacks is Client specific. - // Depends on gh-4442 String nonceHash = ""; try { nonceHash = createHash(nonce); From ab39bd41140b4ee9099f66cf5b7e6603a3b472b0 Mon Sep 17 00:00:00 2001 From: Mark Heckler Date: Mon, 9 Sep 2019 13:36:29 -0500 Subject: [PATCH 3/6] Reworked gh-4442 per feedback from Joe Grandja --- .../client/OAuth2ClientConfigurerTests.java | 4 +- ...thorizationCodeAuthenticationProvider.java | 27 ++++++++ .../OidcIdTokenDecoderFactory.java | 3 + .../authentication/OidcIdTokenValidator.java | 35 +--------- .../ReactiveOidcIdTokenDecoderFactory.java | 3 + ...ultOAuth2AuthorizationRequestResolver.java | 21 +++--- ...verOAuth2AuthorizationRequestResolver.java | 21 +++--- ...zationCodeAuthenticationProviderTests.java | 68 +++++++++++++++---- .../OidcIdTokenValidatorTests.java | 41 +---------- ...uth2AuthorizationRequestResolverTests.java | 6 +- ...uth2AuthorizationRequestResolverTests.java | 1 + .../core/endpoint/NonceParameterNames.java | 17 ----- .../endpoint/OAuth2AuthorizationResponse.java | 24 +++++++ .../TestOAuth2AuthorizationResponses.java | 8 +++ 14 files changed, 152 insertions(+), 127 deletions(-) delete mode 100644 oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java index 3e7b2624a4c..4605bf821d9 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java @@ -75,6 +75,7 @@ * Tests for {@link OAuth2ClientConfigurer}. * * @author Joe Grandja + * @author Mark Heckler */ public class OAuth2ClientConfigurerTests { private static ClientRegistrationRepository clientRegistrationRepository; @@ -152,7 +153,8 @@ public void configureWhenOauth2ClientInLambdaThenRedirectForAuthorization() thro assertThat(mvcResult.getResponse().getRedirectedUrl()).matches("https://provider.com/oauth2/authorize\\?" + "response_type=code&client_id=client-1&" + "scope=user&state=.{15,}&" + - "redirect_uri=http://localhost/client-1"); + "redirect_uri=http://localhost/client-1&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java index 73227b69893..77e675cd53a 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java @@ -33,6 +33,7 @@ import org.springframework.security.oauth2.core.endpoint.OAuth2AccessTokenResponse; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse; +import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; import org.springframework.security.oauth2.core.oidc.OidcIdToken; import org.springframework.security.oauth2.core.oidc.OidcScopes; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; @@ -43,6 +44,10 @@ import org.springframework.security.oauth2.jwt.JwtException; import org.springframework.util.Assert; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; import java.util.Collection; import java.util.Map; @@ -61,6 +66,7 @@ * to complete the authentication. * * @author Joe Grandja + * @author Mark Heckler * @since 5.0 * @see OAuth2LoginAuthenticationToken * @see OAuth2AccessTokenResponseClient @@ -75,6 +81,8 @@ public class OidcAuthorizationCodeAuthenticationProvider implements Authenticati private static final String INVALID_STATE_PARAMETER_ERROR_CODE = "invalid_state_parameter"; private static final String INVALID_REDIRECT_URI_PARAMETER_ERROR_CODE = "invalid_redirect_uri_parameter"; private static final String INVALID_ID_TOKEN_ERROR_CODE = "invalid_id_token"; + private static final String UNVERIFIABLE_NONCE = "unverifiable_nonce"; + private static final String INVALID_NONCE = "invalid_nonce"; private final OAuth2AccessTokenResponseClient accessTokenResponseClient; private final OAuth2UserService userService; private JwtDecoderFactory jwtDecoderFactory = new OidcIdTokenDecoderFactory(); @@ -154,6 +162,19 @@ public Authentication authenticate(Authentication authentication) throws Authent } OidcIdToken idToken = createOidcToken(clientRegistration, accessTokenResponse); + String nonceHash; + try { + nonceHash = createHash(authorizationRequest.getAttribute(IdTokenClaimNames.NONCE)); + } catch (NoSuchAlgorithmException e) { + OAuth2Error nonceVerificationError = new OAuth2Error(UNVERIFIABLE_NONCE, e.getMessage(), null); + throw new OAuth2AuthenticationException(nonceVerificationError); + } + String nonceClaim = idToken.getClaim(IdTokenClaimNames.NONCE); + if (nonceClaim == null || !nonceClaim.equals(nonceHash)) { + OAuth2Error nonceError = new OAuth2Error(INVALID_NONCE); + throw new OAuth2AuthenticationException(nonceError); + } + OidcUser oidcUser = this.userService.loadUser(new OidcUserRequest( clientRegistration, accessTokenResponse.getAccessToken(), idToken, additionalParameters)); Collection mappedAuthorities = @@ -211,4 +232,10 @@ private OidcIdToken createOidcToken(ClientRegistration clientRegistration, OAuth OidcIdToken idToken = new OidcIdToken(jwt.getTokenValue(), jwt.getIssuedAt(), jwt.getExpiresAt(), jwt.getClaims()); return idToken; } + + private String createHash(String codeVerifier) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] digest = md.digest(codeVerifier.getBytes(StandardCharsets.US_ASCII)); + return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + } } diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java index 2ef5d81142b..da51f71d629 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java @@ -57,6 +57,7 @@ * * @author Joe Grandja * @author Rafael Dominguez + * @author Mark Heckler * @since 5.2 * @see JwtDecoderFactory * @see ClientRegistration @@ -88,12 +89,14 @@ public final class OidcIdTokenDecoderFactory implements JwtDecoderFactory booleanConverter = getConverter(TypeDescriptor.valueOf(Boolean.class)); Converter instantConverter = getConverter(TypeDescriptor.valueOf(Instant.class)); Converter urlConverter = getConverter(TypeDescriptor.valueOf(URL.class)); + Converter stringConverter = getConverter(TypeDescriptor.valueOf(String.class)); Converter collectionStringConverter = getConverter( TypeDescriptor.collection(Collection.class, TypeDescriptor.valueOf(String.class))); Map> claimTypeConverters = new HashMap<>(); claimTypeConverters.put(IdTokenClaimNames.ISS, urlConverter); claimTypeConverters.put(IdTokenClaimNames.AUD, collectionStringConverter); + claimTypeConverters.put(IdTokenClaimNames.NONCE, stringConverter); claimTypeConverters.put(IdTokenClaimNames.EXP, instantConverter); claimTypeConverters.put(IdTokenClaimNames.IAT, instantConverter); claimTypeConverters.put(IdTokenClaimNames.AUTH_TIME, instantConverter); diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java index 4b4367cf7a1..ef60eec1e69 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java @@ -27,12 +27,8 @@ import org.springframework.util.CollectionUtils; import java.net.URL; -import java.nio.charset.StandardCharsets; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; import java.time.Duration; import java.time.Instant; -import java.util.Base64; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -51,21 +47,13 @@ public final class OidcIdTokenValidator implements OAuth2TokenValidator { private static final Duration DEFAULT_CLOCK_SKEW = Duration.ofSeconds(60); private final ClientRegistration clientRegistration; - private String nonce = ""; private Duration clockSkew = DEFAULT_CLOCK_SKEW; public OidcIdTokenValidator(ClientRegistration clientRegistration) { - this(clientRegistration, ""); - } - - public OidcIdTokenValidator(ClientRegistration clientRegistration, String nonce) { Assert.notNull(clientRegistration, "clientRegistration cannot be null"); this.clientRegistration = clientRegistration; - - this.nonce = nonce; } - @Override public OAuth2TokenValidatorResult validate(Jwt idToken) { // 3.1.3.7 ID Token Validation @@ -124,17 +112,7 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { // that it is the same value as the one that was sent in the Authentication Request. // The Client SHOULD check the nonce value for replay attacks. // The precise method for detecting replay attacks is Client specific. - String nonceHash = ""; - try { - nonceHash = createHash(nonce); - } catch (NoSuchAlgorithmException e) { - //e.printStackTrace(); - // MH: ??? - } - String nonceClaim = idToken.getClaim(IdTokenClaimNames.NONCE); - if (nonceClaim == null || !nonceClaim.equals(nonceHash)) { - invalidClaims.put(IdTokenClaimNames.NONCE, idToken.getClaim(IdTokenClaimNames.NONCE)); - } + // gh-4442 implemented elsewhere if (!invalidClaims.isEmpty()) { return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims)); @@ -178,11 +156,6 @@ private static Map validateRequiredClaims(Jwt idToken) { if (CollectionUtils.isEmpty(audience)) { requiredClaims.put(IdTokenClaimNames.AUD, audience); } - //String nonce = idToken.getNonce(); MH: We should be using OidcToken for this instead of Jwt. See DefaultOidcIdTokenValidatorFactory - String nonce = idToken.getClaim(IdTokenClaimNames.NONCE); - if (nonce == null) { - requiredClaims.put(IdTokenClaimNames.NONCE, nonce); - } Instant expiresAt = idToken.getExpiresAt(); if (expiresAt == null) { requiredClaims.put(IdTokenClaimNames.EXP, expiresAt); @@ -195,10 +168,4 @@ private static Map validateRequiredClaims(Jwt idToken) { return requiredClaims; } - private String createHash(String nonce) throws NoSuchAlgorithmException { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); - return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); - } - } diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java index 16dfa5461c8..6661efb7edd 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java @@ -57,6 +57,7 @@ * * @author Joe Grandja * @author Rafael Dominguez + * @author Mark Heckler * @since 5.2 * @see ReactiveJwtDecoderFactory * @see ClientRegistration @@ -88,12 +89,14 @@ public final class ReactiveOidcIdTokenDecoderFactory implements ReactiveJwtDecod Converter booleanConverter = getConverter(TypeDescriptor.valueOf(Boolean.class)); Converter instantConverter = getConverter(TypeDescriptor.valueOf(Instant.class)); Converter urlConverter = getConverter(TypeDescriptor.valueOf(URL.class)); + Converter stringConverter = getConverter(TypeDescriptor.valueOf(String.class)); Converter collectionStringConverter = getConverter( TypeDescriptor.collection(Collection.class, TypeDescriptor.valueOf(String.class))); Map> claimTypeConverters = new HashMap<>(); claimTypeConverters.put(IdTokenClaimNames.ISS, urlConverter); claimTypeConverters.put(IdTokenClaimNames.AUD, collectionStringConverter); + claimTypeConverters.put(IdTokenClaimNames.NONCE, stringConverter); claimTypeConverters.put(IdTokenClaimNames.EXP, instantConverter); claimTypeConverters.put(IdTokenClaimNames.IAT, instantConverter); claimTypeConverters.put(IdTokenClaimNames.AUTH_TIME, instantConverter); 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 09dd1a2f918..f25efa08ee0 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 @@ -21,10 +21,10 @@ import org.springframework.security.oauth2.client.registration.ClientRegistrationRepository; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; -import org.springframework.security.oauth2.core.endpoint.NonceParameterNames; 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.IdTokenClaimNames; import org.springframework.security.web.util.UrlUtils; import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.util.Assert; @@ -52,6 +52,7 @@ * @author Joe Grandja * @author Rob Winch * @author Eddú Meléndez + * @author Mark Heckler * @since 5.1 * @see OAuth2AuthorizationRequestResolver * @see OAuth2AuthorizationRequestRedirectFilter @@ -121,7 +122,7 @@ private OAuth2AuthorizationRequest resolve(HttpServletRequest request, String re builder = OAuth2AuthorizationRequest.authorizationCode(); Map additionalParameters = new HashMap<>(); - addNonceHash(attributes, additionalParameters); + createNonceAndHashForRequest(attributes, additionalParameters); if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { addPkceParameters(attributes, additionalParameters); @@ -207,22 +208,22 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist } /** - * Created and adds nonce for use in OpenID Connect Authentication Requests + * Creates nonce and its hash for use in OpenID Connect Authentication Requests * - * @param attributes where {@link PkceParameterNames#CODE_VERIFIER} is stored for the token request - * @param additionalParameters where {@link NonceParameterNames#NONCE} is added to be used in the authentication request. + * @param attributes where {@link IdTokenClaimNames#NONCE} is stored for the token request + * @param additionalParameters where hash of {@link IdTokenClaimNames#NONCE} is added to the authentication request * - * @since 5.2 MAH fix links + * @since 5.2 * @see 15.5.2. Nonce Implementation Notes * @see 3.1.3.7. ID Token Validation */ - private void addNonceHash(Map attributes, Map additionalParameters) { + private void createNonceAndHashForRequest(Map attributes, Map additionalParameters) { String nonce = this.randomKeyGenerator.generateKey(); - attributes.put(NonceParameterNames.NONCE, nonce); + attributes.put(IdTokenClaimNames.NONCE, nonce); try { - String hashNonce = createHash(nonce); - additionalParameters.put(NonceParameterNames.NONCE, hashNonce); + String nonceHash = createHash(nonce); + additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); } catch (NoSuchAlgorithmException e) { // MAH: TODO...but what? } 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 9371e89c709..c80aab181e0 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 @@ -24,10 +24,10 @@ import org.springframework.security.oauth2.client.registration.ReactiveClientRegistrationRepository; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; -import org.springframework.security.oauth2.core.endpoint.NonceParameterNames; 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.IdTokenClaimNames; import org.springframework.security.web.server.util.matcher.PathPatternParserServerWebExchangeMatcher; import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher; import org.springframework.util.Assert; @@ -53,6 +53,7 @@ * used to resolve the {@link ClientRegistration} and create the {@link OAuth2AuthorizationRequest}. * * @author Rob Winch + * @author Mark Heckler * @since 5.1 */ public class DefaultServerOAuth2AuthorizationRequestResolver @@ -135,7 +136,7 @@ private OAuth2AuthorizationRequest authorizationRequest(ServerWebExchange exchan builder = OAuth2AuthorizationRequest.authorizationCode(); Map additionalParameters = new HashMap<>(); - addNonceHash(attributes, additionalParameters); + createNonceAndHashForRequest(attributes, additionalParameters); if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { addPkceParameters(attributes, additionalParameters); @@ -211,22 +212,22 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr } /** - * Created and adds nonce for use in OpenID Connect Authentication Requests + * Creates nonce and its hash for use in OpenID Connect Authentication Requests * - * @param attributes where {@link PkceParameterNames#CODE_VERIFIER} is stored for the token request - * @param additionalParameters where {@link NonceParameterNames#NONCE} is added to be used in the authentication request. + * @param attributes where {@link IdTokenClaimNames#NONCE} is stored for the token request + * @param additionalParameters where hash of {@link IdTokenClaimNames#NONCE} is added to the authentication request * - * @since 5.2 MAH fix links + * @since 5.2 * @see 15.5.2. Nonce Implementation Notes * @see 3.1.3.7. ID Token Validation */ - private void addNonceHash(Map attributes, Map additionalParameters) { + private void createNonceAndHashForRequest(Map attributes, Map additionalParameters) { String nonce = this.randomKeyGenerator.generateKey(); - attributes.put(NonceParameterNames.NONCE, nonce); + attributes.put(IdTokenClaimNames.NONCE, nonce); try { - String hashNonce = createHash(nonce); - additionalParameters.put(NonceParameterNames.NONCE, hashNonce); + String nonceHash = createHash(nonce); + additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); } catch (NoSuchAlgorithmException e) { // MAH: TODO...but what? } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java index 77dba3d87a5..a0b606a1d65 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java @@ -24,6 +24,8 @@ import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.AuthorityUtils; import org.springframework.security.core.authority.mapping.GrantedAuthoritiesMapper; +import org.springframework.security.crypto.keygen.Base64StringKeyGenerator; +import org.springframework.security.crypto.keygen.StringKeyGenerator; import org.springframework.security.oauth2.client.authentication.OAuth2LoginAuthenticationToken; import org.springframework.security.oauth2.client.endpoint.OAuth2AccessTokenResponseClient; import org.springframework.security.oauth2.client.endpoint.OAuth2AuthorizationCodeGrantRequest; @@ -33,10 +35,7 @@ import org.springframework.security.oauth2.core.OAuth2AccessToken; import org.springframework.security.oauth2.core.OAuth2AuthenticationException; import org.springframework.security.oauth2.core.OAuth2ErrorCodes; -import org.springframework.security.oauth2.core.endpoint.OAuth2AccessTokenResponse; -import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationExchange; -import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; -import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse; +import org.springframework.security.oauth2.core.endpoint.*; import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.oauth2.core.oidc.user.OidcUser; @@ -44,14 +43,11 @@ import org.springframework.security.oauth2.jwt.JwtDecoder; import org.springframework.security.oauth2.jwt.JwtException; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.time.Instant; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; +import java.util.*; import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.CoreMatchers.containsString; @@ -60,13 +56,13 @@ import static org.mockito.Mockito.when; import static org.springframework.security.oauth2.client.registration.TestClientRegistrations.clientRegistration; import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationRequests.request; -import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationResponses.error; -import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationResponses.success; +import static org.springframework.security.oauth2.core.endpoint.TestOAuth2AuthorizationResponses.*; /** * Tests for {@link OidcAuthorizationCodeAuthenticationProvider}. * * @author Joe Grandja + * @author Mark Heckler */ public class OidcAuthorizationCodeAuthenticationProviderTests { private ClientRegistration clientRegistration; @@ -77,6 +73,9 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { private OAuth2AccessTokenResponse accessTokenResponse; private OAuth2UserService userService; private OidcAuthorizationCodeAuthenticationProvider authenticationProvider; + private StringKeyGenerator randomKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + private String nonce = this.randomKeyGenerator.generateKey(); + private String nonceHash; @Rule public ExpectedException exception = ExpectedException.none(); @@ -84,9 +83,22 @@ public class OidcAuthorizationCodeAuthenticationProviderTests { @Before @SuppressWarnings("unchecked") public void setUp() { + try { + nonceHash = createHash(nonce); + } catch (NoSuchAlgorithmException e) { + e.printStackTrace(); + } + Map attributes = new HashMap<>(); + Map additionalParameters = new HashMap<>(); + addNonceToRequest(attributes, additionalParameters); + this.clientRegistration = clientRegistration().clientId("client1").build(); - this.authorizationRequest = request().scope("openid", "profile", "email").build(); - this.authorizationResponse = success().build(); + this.authorizationRequest = request() + .scope("openid", "profile", "email") + .attributes(attributes) + .additionalParameters(additionalParameters) + .build(); + this.authorizationResponse = successWithNonce((String) additionalParameters.get(IdTokenClaimNames.NONCE)).build(); this.authorizationExchange = new OAuth2AuthorizationExchange(this.authorizationRequest, this.authorizationResponse); this.accessTokenResponseClient = mock(OAuth2AccessTokenResponseClient.class); this.accessTokenResponse = this.accessTokenSuccessResponse(); @@ -224,6 +236,7 @@ public void authenticateWhenLoginSuccessThenReturnAuthentication() { claims.put(IdTokenClaimNames.SUB, "subject1"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); claims.put(IdTokenClaimNames.AZP, "client1"); + claims.put(IdTokenClaimNames.NONCE, nonceHash); this.setUpIdToken(claims); OidcUser principal = mock(OidcUser.class); @@ -253,6 +266,7 @@ public void authenticateWhenAuthoritiesMapperSetThenReturnMappedAuthorities() { claims.put(IdTokenClaimNames.SUB, "subject1"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); claims.put(IdTokenClaimNames.AZP, "client1"); + claims.put(IdTokenClaimNames.NONCE, nonceHash); this.setUpIdToken(claims); OidcUser principal = mock(OidcUser.class); @@ -282,6 +296,7 @@ public void authenticateWhenTokenSuccessResponseThenAdditionalParametersAddedToU claims.put(IdTokenClaimNames.SUB, "subject1"); claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); claims.put(IdTokenClaimNames.AZP, "client1"); + claims.put(IdTokenClaimNames.NONCE, nonceHash); this.setUpIdToken(claims); OidcUser principal = mock(OidcUser.class); @@ -322,6 +337,7 @@ private OAuth2AccessTokenResponse accessTokenSuccessResponse() { additionalParameters.put("param1", "value1"); additionalParameters.put("param2", "value2"); additionalParameters.put(OidcParameterNames.ID_TOKEN, "id-token"); + additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); return OAuth2AccessTokenResponse .withToken("access-token-1234") @@ -333,4 +349,26 @@ private OAuth2AccessTokenResponse accessTokenSuccessResponse() { .build(); } + + /** + * Adds nonce for use in OpenID Connect Authentication Requests + * + * @param attributes where {@link IdTokenClaimNames#NONCE} is stored for the token request + * @param additionalParameters where the hash of {@link IdTokenClaimNames#NONCE} is added to be used in the authentication request + * + * @since 5.2 + * @see 15.5.2. Nonce Implementation Notes + * @see 3.1.3.7. ID Token Validation + */ + private void addNonceToRequest(Map attributes, Map additionalParameters) { + //String nonce = this.randomKeyGenerator.generateKey(); + attributes.put(IdTokenClaimNames.NONCE, nonce); + additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); + } + + private String createHash(String nonce) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); + return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); + } } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java index f8324eeca33..eb406096238 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java @@ -17,8 +17,6 @@ import org.junit.Before; import org.junit.Test; -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.TestClientRegistrations; import org.springframework.security.oauth2.core.OAuth2Error; @@ -26,9 +24,6 @@ import org.springframework.security.oauth2.jose.jws.JwsAlgorithms; import org.springframework.security.oauth2.jwt.Jwt; -import java.nio.charset.StandardCharsets; -import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; import java.time.Duration; import java.time.Instant; import java.util.*; @@ -48,20 +43,13 @@ public class OidcIdTokenValidatorTests { private Instant issuedAt = Instant.now(); private Instant expiresAt = this.issuedAt.plusSeconds(3600); private Duration clockSkew = Duration.ofSeconds(60); - private StringKeyGenerator randomKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); - private String nonce = this.randomKeyGenerator.generateKey(); + @Before public void setup() { this.headers.put("alg", JwsAlgorithms.RS256); this.claims.put(IdTokenClaimNames.ISS, "https://issuer.example.com"); this.claims.put(IdTokenClaimNames.SUB, "rob"); this.claims.put(IdTokenClaimNames.AUD, Collections.singletonList("client-id")); - try { - this.claims.put(IdTokenClaimNames.NONCE, createHash(nonce)); - } catch (NoSuchAlgorithmException e) { - // MH: Sanity check, please - e.printStackTrace(); - } } @Test @@ -111,15 +99,6 @@ public void validateWhenAudNullThenHasErrors() { .allMatch(msg -> msg.contains(IdTokenClaimNames.AUD)); } - @Test - public void validateWhenNonceNullThenHasErrors() { - this.claims.remove(IdTokenClaimNames.NONCE); - assertThat(this.validateIdToken()) - .hasSize(1) - .extracting(OAuth2Error::getDescription) - .allMatch(msg -> msg.contains(IdTokenClaimNames.NONCE)); - } - @Test public void validateWhenIssuedAtNullThenHasErrors() { this.issuedAt = null; @@ -182,13 +161,6 @@ public void validateWhenAudNotClientIdThenHasErrors() { .allMatch(msg -> msg.contains(IdTokenClaimNames.AUD)); } -/* - @Test - public void validateWhenNonceHashMatchesThenErrors() { - assertThat(this.validateIdToken()).isEmpty(); - } -*/ - @Test public void validateWhenExpiredAnd60secClockSkewThenNoErrors() { this.issuedAt = Instant.now().minus(Duration.ofSeconds(60)); @@ -242,7 +214,6 @@ public void validateWhenExpiresAtBeforeNowThenHasErrors() { public void validateWhenMissingClaimsThenHasErrors() { this.claims.remove(IdTokenClaimNames.SUB); this.claims.remove(IdTokenClaimNames.AUD); - this.claims.remove(IdTokenClaimNames.NONCE); this.issuedAt = null; this.expiresAt = null; assertThat(this.validateIdToken()) @@ -251,8 +222,7 @@ public void validateWhenMissingClaimsThenHasErrors() { .allMatch(msg -> msg.contains(IdTokenClaimNames.SUB)) .allMatch(msg -> msg.contains(IdTokenClaimNames.AUD)) .allMatch(msg -> msg.contains(IdTokenClaimNames.IAT)) - .allMatch(msg -> msg.contains(IdTokenClaimNames.EXP)) - .allMatch(msg -> msg.contains(IdTokenClaimNames.NONCE)); + .allMatch(msg -> msg.contains(IdTokenClaimNames.EXP)); } @Test @@ -267,14 +237,9 @@ public void validateFormatError() { private Collection validateIdToken() { Jwt idToken = new Jwt("token123", this.issuedAt, this.expiresAt, this.headers, this.claims); - OidcIdTokenValidator validator = new OidcIdTokenValidator(this.registration.build(), nonce); + OidcIdTokenValidator validator = new OidcIdTokenValidator(this.registration.build()); validator.setClockSkew(this.clockSkew); return validator.validate(idToken).getErrors(); } - private String createHash(String nonce) throws NoSuchAlgorithmException { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); - return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); - } } 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 762fd8a0fb6..315f485a3bf 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 @@ -25,6 +25,7 @@ import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; import org.springframework.security.oauth2.core.endpoint.*; +import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -34,6 +35,7 @@ * Tests for {@link DefaultOAuth2AuthorizationRequestResolver}. * * @author Joe Grandja + * @author Mark Heckler */ public class DefaultOAuth2AuthorizationRequestResolverTests { private ClientRegistration registration1; @@ -116,7 +118,7 @@ public void resolveWhenAuthorizationRequestWithValidClientThenResolves() { assertThat(authorizationRequest.getState()).isNotNull(); assertThat(authorizationRequest.getAdditionalParameters()).doesNotContainKey(OAuth2ParameterNames.REGISTRATION_ID); assertThat(authorizationRequest.getAttributes()) - .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, NonceParameterNames.NONCE) + .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, IdTokenClaimNames.NONCE) .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); assertThat(authorizationRequest.getAuthorizationRequestUri()) .matches("https://example.com/login/oauth/authorize\\?" + @@ -136,7 +138,7 @@ public void resolveWhenClientAuthorizationRequiredExceptionAvailableThenResolves OAuth2AuthorizationRequest authorizationRequest = this.resolver.resolve(request, clientRegistration.getRegistrationId()); assertThat(authorizationRequest).isNotNull(); assertThat(authorizationRequest.getAttributes()) - .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, NonceParameterNames.NONCE) + .containsOnlyKeys(OAuth2ParameterNames.REGISTRATION_ID, IdTokenClaimNames.NONCE) .contains(entry(OAuth2ParameterNames.REGISTRATION_ID, clientRegistration.getRegistrationId())); } 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 8fc504efc91..60a797ea307 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 @@ -41,6 +41,7 @@ /** * @author Rob Winch + * @author Mark Heckler * @since 5.1 */ @RunWith(MockitoJUnitRunner.class) diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java deleted file mode 100644 index b529255633b..00000000000 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java +++ /dev/null @@ -1,17 +0,0 @@ -package org.springframework.security.oauth2.core.endpoint; - -/** - * Standard parameter names defined in the OpenID Connect Core 1.0 - * incorporating errata set 1 and used by the authorization endpoint and - * token endpoint. - * - * @author Mark Heckler - * @since 5.2 - * @see 15.5.2. Nonce Implementation Notes - */ -public interface NonceParameterNames { - /** - * {@code nonce} - used in Authentication Request. - */ - String NONCE = "nonce"; -} diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java index 1e34de0ac46..41906a4e658 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java @@ -23,6 +23,7 @@ * A representation of an OAuth 2.0 Authorization Response for the authorization code grant type. * * @author Joe Grandja + * @author Mark Heckler * @since 5.0 * @see OAuth2Error * @see Section 4.1.2 Authorization Response @@ -31,6 +32,7 @@ public final class OAuth2AuthorizationResponse { private String redirectUri; private String state; private String code; + private String nonce; private OAuth2Error error; private OAuth2AuthorizationResponse() { @@ -63,6 +65,15 @@ public String getCode() { return this.code; } + /** + * Returns the nonce. + * + * @return the nonce + */ + public String getNonce() { + return this.nonce; + } + /** * Returns the {@link OAuth2Error OAuth 2.0 Error} if the Authorization Request failed, otherwise {@code null}. * @@ -119,6 +130,7 @@ public static class Builder { private String redirectUri; private String state; private String code; + private String nonce; private String errorCode; private String errorDescription; private String errorUri; @@ -159,6 +171,17 @@ public Builder code(String code) { return this; } + /** + * Sets the nonce. + * + * @param nonce the nonce + * @return the {@link Builder} + */ + public Builder nonce(String nonce) { + this.nonce = nonce; + return this; + } + /** * Sets the error code. * @@ -212,6 +235,7 @@ public OAuth2AuthorizationResponse build() { authorizationResponse.error = new OAuth2Error( this.errorCode, this.errorDescription, this.errorUri); } + authorizationResponse.nonce = this.nonce; return authorizationResponse; } } diff --git a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java index 7ad7085c710..de040dde9bc 100644 --- a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java +++ b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java @@ -18,6 +18,7 @@ /** * @author Rob Winch + * @author Mark Heckler * @since 5.1 */ public class TestOAuth2AuthorizationResponses { @@ -28,6 +29,13 @@ public static OAuth2AuthorizationResponse.Builder success() { .redirectUri("https://example.com/authorize/oauth2/code/registration-id"); } + public static OAuth2AuthorizationResponse.Builder successWithNonce(String nonce) { + return OAuth2AuthorizationResponse.success("authorization-code") + .state("state") + .nonce(nonce) + .redirectUri("https://example.com/authorize/oauth2/code/registration-id"); + } + public static OAuth2AuthorizationResponse.Builder error() { return OAuth2AuthorizationResponse.error("error") .redirectUri("https://example.com/authorize/oauth2/code/registration-id") From 595ab60c9286ecf0a94272c133f561bdbfadd3e6 Mon Sep 17 00:00:00 2001 From: Mark Heckler Date: Tue, 10 Sep 2019 19:07:17 -0500 Subject: [PATCH 4/6] Added nonce creation, verification, & handling Implements nonce functionality for OpenID Connect authentication request in OAuth2 Authorization Code flow. This includes the following capabilities: * Generate nonce * Generate hash of nonce * Add nonce to attributes and hash to additional parameters of authentication request * Propagate hash to authentication response * Verify hash upon receipt of response by hashing original nonce * Complete all relevant tests Fixes/implements gh-4442 --- ...thorizationCodeAuthenticationProvider.java | 31 ++++++++++--------- .../authentication/OidcIdTokenValidator.java | 8 +---- ...ultOAuth2AuthorizationRequestResolver.java | 16 +++++----- ...verOAuth2AuthorizationRequestResolver.java | 16 +++++----- ...zationCodeAuthenticationProviderTests.java | 7 ++--- .../endpoint/OAuth2AuthorizationResponse.java | 24 -------------- .../oidc/endpoint/OidcParameterNames.java | 6 ++++ .../TestOAuth2AuthorizationResponses.java | 8 ----- 8 files changed, 42 insertions(+), 74 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java index 77e675cd53a..7a2c4a46321 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java @@ -33,7 +33,6 @@ import org.springframework.security.oauth2.core.endpoint.OAuth2AccessTokenResponse; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationResponse; -import org.springframework.security.oauth2.core.oidc.IdTokenClaimNames; import org.springframework.security.oauth2.core.oidc.OidcIdToken; import org.springframework.security.oauth2.core.oidc.OidcScopes; import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; @@ -81,7 +80,6 @@ public class OidcAuthorizationCodeAuthenticationProvider implements Authenticati private static final String INVALID_STATE_PARAMETER_ERROR_CODE = "invalid_state_parameter"; private static final String INVALID_REDIRECT_URI_PARAMETER_ERROR_CODE = "invalid_redirect_uri_parameter"; private static final String INVALID_ID_TOKEN_ERROR_CODE = "invalid_id_token"; - private static final String UNVERIFIABLE_NONCE = "unverifiable_nonce"; private static final String INVALID_NONCE = "invalid_nonce"; private final OAuth2AccessTokenResponseClient accessTokenResponseClient; private final OAuth2UserService userService; @@ -162,17 +160,20 @@ public Authentication authenticate(Authentication authentication) throws Authent } OidcIdToken idToken = createOidcToken(clientRegistration, accessTokenResponse); - String nonceHash; - try { - nonceHash = createHash(authorizationRequest.getAttribute(IdTokenClaimNames.NONCE)); - } catch (NoSuchAlgorithmException e) { - OAuth2Error nonceVerificationError = new OAuth2Error(UNVERIFIABLE_NONCE, e.getMessage(), null); - throw new OAuth2AuthenticationException(nonceVerificationError); - } - String nonceClaim = idToken.getClaim(IdTokenClaimNames.NONCE); - if (nonceClaim == null || !nonceClaim.equals(nonceHash)) { - OAuth2Error nonceError = new OAuth2Error(INVALID_NONCE); - throw new OAuth2AuthenticationException(nonceError); + String requestNonce = authorizationRequest.getAttribute(OidcParameterNames.NONCE); + if (requestNonce != null) { + String nonceHash; + + try { + nonceHash = createHash(requestNonce); + } catch (NoSuchAlgorithmException e) { + throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE)); + } + + String nonceHashClaim = idToken.getClaim(OidcParameterNames.NONCE); + if (nonceHashClaim == null || !nonceHashClaim.equals(nonceHash)) { + throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE)); + } } OidcUser oidcUser = this.userService.loadUser(new OidcUserRequest( @@ -233,9 +234,9 @@ private OidcIdToken createOidcToken(ClientRegistration clientRegistration, OAuth return idToken; } - private String createHash(String codeVerifier) throws NoSuchAlgorithmException { + private String createHash(String nonce) throws NoSuchAlgorithmException { MessageDigest md = MessageDigest.getInstance("SHA-256"); - byte[] digest = md.digest(codeVerifier.getBytes(StandardCharsets.US_ASCII)); + byte[] digest = md.digest(nonce.getBytes(StandardCharsets.US_ASCII)); return Base64.getUrlEncoder().withoutPadding().encodeToString(digest); } } diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java index ef60eec1e69..79b96fc18d5 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java @@ -39,6 +39,7 @@ * * @author Rob Winch * @author Joe Grandja + * @author Mark Heckler * @since 5.1 * @see OAuth2TokenValidator * @see Jwt @@ -107,13 +108,6 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { invalidClaims.put(IdTokenClaimNames.IAT, idToken.getIssuedAt()); } - // 11. If a nonce value was sent in the Authentication Request, - // a nonce Claim MUST be present and its value checked to verify - // that it is the same value as the one that was sent in the Authentication Request. - // The Client SHOULD check the nonce value for replay attacks. - // The precise method for detecting replay attacks is Client specific. - // gh-4442 implemented elsewhere - if (!invalidClaims.isEmpty()) { return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims)); } 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 f25efa08ee0..a641e783e33 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 @@ -63,7 +63,7 @@ public final class DefaultOAuth2AuthorizationRequestResolver implements OAuth2Au private final ClientRegistrationRepository clientRegistrationRepository; private final AntPathRequestMatcher authorizationRequestMatcher; private final StringKeyGenerator stateGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder()); - private final StringKeyGenerator randomKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + private final StringKeyGenerator stringKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); /** * Constructs a {@code DefaultOAuth2AuthorizationRequestResolver} using the provided parameters. @@ -122,7 +122,7 @@ private OAuth2AuthorizationRequest resolve(HttpServletRequest request, String re builder = OAuth2AuthorizationRequest.authorizationCode(); Map additionalParameters = new HashMap<>(); - createNonceAndHashForRequest(attributes, additionalParameters); + addNonceParameters(attributes, additionalParameters); if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { addPkceParameters(attributes, additionalParameters); @@ -217,15 +217,15 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist * @see 15.5.2. Nonce Implementation Notes * @see 3.1.3.7. ID Token Validation */ - private void createNonceAndHashForRequest(Map attributes, Map additionalParameters) { - String nonce = this.randomKeyGenerator.generateKey(); + private void addNonceParameters(Map attributes, Map additionalParameters) { + String nonce = this.stringKeyGenerator.generateKey(); attributes.put(IdTokenClaimNames.NONCE, nonce); try { String nonceHash = createHash(nonce); additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); } catch (NoSuchAlgorithmException e) { - // MAH: TODO...but what? + // } } @@ -242,7 +242,7 @@ private void createNonceAndHashForRequest(Map attributes, Map4.2. Client Creates the Code Challenge */ private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.randomKeyGenerator.generateKey(); + String codeVerifier = this.stringKeyGenerator.generateKey(); attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); try { String codeChallenge = createHash(codeVerifier); @@ -253,9 +253,9 @@ private void addPkceParameters(Map attributes, Map additionalParameters = new HashMap<>(); - createNonceAndHashForRequest(attributes, additionalParameters); + addNonceParameters(attributes, additionalParameters); if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) { addPkceParameters(attributes, additionalParameters); @@ -221,15 +221,15 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr * @see 15.5.2. Nonce Implementation Notes * @see 3.1.3.7. ID Token Validation */ - private void createNonceAndHashForRequest(Map attributes, Map additionalParameters) { - String nonce = this.randomKeyGenerator.generateKey(); + private void addNonceParameters(Map attributes, Map additionalParameters) { + String nonce = this.stringKeyGenerator.generateKey(); attributes.put(IdTokenClaimNames.NONCE, nonce); try { String nonceHash = createHash(nonce); additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); } catch (NoSuchAlgorithmException e) { - // MAH: TODO...but what? + // } } @@ -246,7 +246,7 @@ private void createNonceAndHashForRequest(Map attributes, Map4.2. Client Creates the Code Challenge */ private void addPkceParameters(Map attributes, Map additionalParameters) { - String codeVerifier = this.randomKeyGenerator.generateKey(); + String codeVerifier = this.stringKeyGenerator.generateKey(); attributes.put(PkceParameterNames.CODE_VERIFIER, codeVerifier); try { String codeChallenge = createHash(codeVerifier); @@ -257,9 +257,9 @@ private void addPkceParameters(Map attributes, Map userService; private OidcAuthorizationCodeAuthenticationProvider authenticationProvider; - private StringKeyGenerator randomKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); - private String nonce = this.randomKeyGenerator.generateKey(); + private StringKeyGenerator stringKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96); + private String nonce = this.stringKeyGenerator.generateKey(); private String nonceHash; @Rule @@ -98,7 +98,7 @@ public void setUp() { .attributes(attributes) .additionalParameters(additionalParameters) .build(); - this.authorizationResponse = successWithNonce((String) additionalParameters.get(IdTokenClaimNames.NONCE)).build(); + this.authorizationResponse = success().build(); this.authorizationExchange = new OAuth2AuthorizationExchange(this.authorizationRequest, this.authorizationResponse); this.accessTokenResponseClient = mock(OAuth2AccessTokenResponseClient.class); this.accessTokenResponse = this.accessTokenSuccessResponse(); @@ -361,7 +361,6 @@ private OAuth2AccessTokenResponse accessTokenSuccessResponse() { * @see 3.1.3.7. ID Token Validation */ private void addNonceToRequest(Map attributes, Map additionalParameters) { - //String nonce = this.randomKeyGenerator.generateKey(); attributes.put(IdTokenClaimNames.NONCE, nonce); additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); } diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java index 41906a4e658..1e34de0ac46 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java @@ -23,7 +23,6 @@ * A representation of an OAuth 2.0 Authorization Response for the authorization code grant type. * * @author Joe Grandja - * @author Mark Heckler * @since 5.0 * @see OAuth2Error * @see Section 4.1.2 Authorization Response @@ -32,7 +31,6 @@ public final class OAuth2AuthorizationResponse { private String redirectUri; private String state; private String code; - private String nonce; private OAuth2Error error; private OAuth2AuthorizationResponse() { @@ -65,15 +63,6 @@ public String getCode() { return this.code; } - /** - * Returns the nonce. - * - * @return the nonce - */ - public String getNonce() { - return this.nonce; - } - /** * Returns the {@link OAuth2Error OAuth 2.0 Error} if the Authorization Request failed, otherwise {@code null}. * @@ -130,7 +119,6 @@ public static class Builder { private String redirectUri; private String state; private String code; - private String nonce; private String errorCode; private String errorDescription; private String errorUri; @@ -171,17 +159,6 @@ public Builder code(String code) { return this; } - /** - * Sets the nonce. - * - * @param nonce the nonce - * @return the {@link Builder} - */ - public Builder nonce(String nonce) { - this.nonce = nonce; - return this; - } - /** * Sets the error code. * @@ -235,7 +212,6 @@ public OAuth2AuthorizationResponse build() { authorizationResponse.error = new OAuth2Error( this.errorCode, this.errorDescription, this.errorUri); } - authorizationResponse.nonce = this.nonce; return authorizationResponse; } } diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java index 13c37441c88..a0f58aaec7d 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java @@ -20,6 +20,7 @@ * and used by the authorization endpoint and token endpoint. * * @author Joe Grandja + * @author Mark Heckler * @since 5.0 * @see 18.2 OAuth Parameters Registration */ @@ -30,4 +31,9 @@ public interface OidcParameterNames { */ String ID_TOKEN = "id_token"; + /** + * {@code nonce} - used in the Access Token Request and Response. + */ + String NONCE = "nonce"; + } diff --git a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java index de040dde9bc..7ad7085c710 100644 --- a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java +++ b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/endpoint/TestOAuth2AuthorizationResponses.java @@ -18,7 +18,6 @@ /** * @author Rob Winch - * @author Mark Heckler * @since 5.1 */ public class TestOAuth2AuthorizationResponses { @@ -29,13 +28,6 @@ public static OAuth2AuthorizationResponse.Builder success() { .redirectUri("https://example.com/authorize/oauth2/code/registration-id"); } - public static OAuth2AuthorizationResponse.Builder successWithNonce(String nonce) { - return OAuth2AuthorizationResponse.success("authorization-code") - .state("state") - .nonce(nonce) - .redirectUri("https://example.com/authorize/oauth2/code/registration-id"); - } - public static OAuth2AuthorizationResponse.Builder error() { return OAuth2AuthorizationResponse.error("error") .redirectUri("https://example.com/authorize/oauth2/code/registration-id") From 60cdf3e8eeb535d71e78e5083b3b6b7966a1cd40 Mon Sep 17 00:00:00 2001 From: Mark Heckler Date: Tue, 10 Sep 2019 20:02:59 -0500 Subject: [PATCH 5/6] Fixed outlier tests in OAuth2AuthorizationRequestRedirectFilterTests --- ...OAuth2AuthorizationRequestRedirectFilterTests.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java index bbc47c9a56e..ca55e2451c3 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java @@ -49,6 +49,7 @@ * Tests for {@link OAuth2AuthorizationRequestRedirectFilter}. * * @author Joe Grandja + * @author Mark Heckler */ public class OAuth2AuthorizationRequestRedirectFilterTests { private ClientRegistration registration1; @@ -154,7 +155,8 @@ public void doFilterWhenAuthorizationRequestOAuth2LoginThenRedirectForAuthorizat assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -234,7 +236,8 @@ public void doFilterWhenCustomAuthorizationRequestBaseUriThenRedirectForAuthoriz assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/login/oauth2/code/registration-id"); + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); } @Test @@ -255,7 +258,8 @@ public void doFilterWhenNotAuthorizationRequestAndClientAuthorizationRequiredExc assertThat(response.getRedirectedUrl()).matches("https://example.com/login/oauth/authorize\\?" + "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + - "redirect_uri=http://localhost/authorize/oauth2/code/registration-id"); + "redirect_uri=http://localhost/authorize/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}"); verify(this.requestCache).saveRequest(any(HttpServletRequest.class), any(HttpServletResponse.class)); } @@ -359,6 +363,7 @@ public void doFilterWhenAuthorizationRequestAndCustomAuthorizationRequestUriSetT "response_type=code&client_id=client-id&" + "scope=read:user&state=.{15,}&" + "redirect_uri=http://localhost/login/oauth2/code/registration-id&" + + "nonce=([a-zA-Z0-9\\-\\.\\_\\~]){43}&" + "login_hint=user@provider\\.com"); } } From 34413890bd1000d7b03ca83bd98017f62df5fd2f Mon Sep 17 00:00:00 2001 From: Mark Heckler Date: Fri, 20 Sep 2019 15:08:35 -0600 Subject: [PATCH 6/6] Resolves requested changes to PR for gh-4442 Made changes requested by Joe Grandja to PR, including addition of test when nonce isn't present in request. Fixes gh-4442 and addresses changes requested to PR --- ...thorizationCodeAuthenticationProvider.java | 8 ++-- ...tionCodeReactiveAuthenticationManager.java | 43 +++++++++++++++++-- ...ultOAuth2AuthorizationRequestResolver.java | 17 ++++---- ...verOAuth2AuthorizationRequestResolver.java | 17 ++++---- ...zationCodeAuthenticationProviderTests.java | 33 +++++++++++++- ...thorizationRequestRedirectFilterTests.java | 2 +- .../oidc/endpoint/OidcParameterNames.java | 2 +- 7 files changed, 93 insertions(+), 29 deletions(-) diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java index 7a2c4a46321..d61f270ec70 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java @@ -80,7 +80,7 @@ public class OidcAuthorizationCodeAuthenticationProvider implements Authenticati private static final String INVALID_STATE_PARAMETER_ERROR_CODE = "invalid_state_parameter"; private static final String INVALID_REDIRECT_URI_PARAMETER_ERROR_CODE = "invalid_redirect_uri_parameter"; private static final String INVALID_ID_TOKEN_ERROR_CODE = "invalid_id_token"; - private static final String INVALID_NONCE = "invalid_nonce"; + private static final String INVALID_NONCE_ERROR_CODE = "invalid_nonce"; private final OAuth2AccessTokenResponseClient accessTokenResponseClient; private final OAuth2UserService userService; private JwtDecoderFactory jwtDecoderFactory = new OidcIdTokenDecoderFactory(); @@ -158,7 +158,7 @@ public Authentication authenticate(Authentication authentication) throws Authent null); throw new OAuth2AuthenticationException(invalidIdTokenError, invalidIdTokenError.toString()); } - OidcIdToken idToken = createOidcToken(clientRegistration, accessTokenResponse); + OidcIdToken idToken = createOidcToken(clientRegistration, accessTokenResponse); String requestNonce = authorizationRequest.getAttribute(OidcParameterNames.NONCE); if (requestNonce != null) { @@ -167,12 +167,12 @@ public Authentication authenticate(Authentication authentication) throws Authent try { nonceHash = createHash(requestNonce); } catch (NoSuchAlgorithmException e) { - throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE)); + throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); } String nonceHashClaim = idToken.getClaim(OidcParameterNames.NONCE); if (nonceHashClaim == null || !nonceHashClaim.equals(nonceHash)) { - throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE)); + throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); } } diff --git a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java index 8a215efdf8d..bf4e71a45db 100644 --- a/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java +++ b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java @@ -43,13 +43,17 @@ import org.springframework.util.Assert; import reactor.core.publisher.Mono; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; import java.util.Collection; import java.util.Map; /** * An implementation of an {@link org.springframework.security.authentication.AuthenticationProvider} for OAuth 2.0 Login, * which leverages the OAuth 2.0 Authorization Code Grant Flow. - * + *

* This {@link org.springframework.security.authentication.AuthenticationProvider} is responsible for authenticating * an Authorization Code credential with the Authorization Server's Token Endpoint * and if valid, exchanging it for an Access Token credential. @@ -61,7 +65,6 @@ * to complete the authentication. * * @author Rob Winch - * @since 5.1 * @see OAuth2LoginAuthenticationToken * @see ReactiveOAuth2AccessTokenResponseClient * @see ReactiveOAuth2UserService @@ -70,6 +73,7 @@ * @see Section 4.1 Authorization Code Grant Flow * @see Section 4.1.3 Access Token Request * @see Section 4.1.4 Access Token Response + * @since 5.1 */ public class OidcAuthorizationCodeReactiveAuthenticationManager implements ReactiveAuthenticationManager { @@ -77,6 +81,7 @@ public class OidcAuthorizationCodeReactiveAuthenticationManager implements private static final String INVALID_STATE_PARAMETER_ERROR_CODE = "invalid_state_parameter"; private static final String INVALID_REDIRECT_URI_PARAMETER_ERROR_CODE = "invalid_redirect_uri_parameter"; private static final String INVALID_ID_TOKEN_ERROR_CODE = "invalid_id_token"; + private static final String INVALID_NONCE_ERROR_CODE = "invalid_nonce"; private final ReactiveOAuth2AccessTokenResponseClient accessTokenResponseClient; @@ -148,8 +153,8 @@ public Mono authenticate(Authentication authentication) { * Sets the {@link ReactiveJwtDecoderFactory} used for {@link OidcIdToken} signature verification. * The factory returns a {@link ReactiveJwtDecoder} associated to the provided {@link ClientRegistration}. * - * @since 5.2 * @param jwtDecoderFactory the {@link ReactiveJwtDecoderFactory} used for {@link OidcIdToken} signature verification + * @since 5.2 */ public final void setJwtDecoderFactory(ReactiveJwtDecoderFactory jwtDecoderFactory) { Assert.notNull(jwtDecoderFactory, "jwtDecoderFactory cannot be null"); @@ -170,7 +175,8 @@ private Mono authenticationResult(OAuth2Authoriz } return createOidcToken(clientRegistration, accessTokenResponse) - .map(idToken -> new OidcUserRequest(clientRegistration, accessToken, idToken, additionalParameters)) + .doOnNext(idToken -> validateNonce(authorizationCodeAuthentication, idToken)) + .map(idToken -> new OidcUserRequest(clientRegistration, accessToken, idToken, additionalParameters)) .flatMap(this.userService::loadUser) .map(oauth2User -> { Collection mappedAuthorities = @@ -192,4 +198,33 @@ private Mono createOidcToken(ClientRegistration clientRegistration, return jwtDecoder.decode(rawIdToken) .map(jwt -> new OidcIdToken(jwt.getTokenValue(), jwt.getIssuedAt(), jwt.getExpiresAt(), jwt.getClaims())); } + + private Mono validateNonce(OAuth2AuthorizationCodeAuthenticationToken authorizationCodeAuthentication, OidcIdToken idToken) { + String requestNonce = authorizationCodeAuthentication + .getAuthorizationExchange() + .getAuthorizationRequest() + .getAttribute(OidcParameterNames.NONCE); + if (requestNonce != null) { + String nonceHash; + + try { + nonceHash = createHash(requestNonce); + } catch (NoSuchAlgorithmException e) { + throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); + } + + String nonceHashClaim = idToken.getClaim(OidcParameterNames.NONCE); + if (nonceHashClaim == null || !nonceHashClaim.equals(nonceHash)) { + throw new OAuth2AuthenticationException(new OAuth2Error(INVALID_NONCE_ERROR_CODE)); + } + } + + return Mono.just(idToken); + } + + private String createHash(String nonce) throws NoSuchAlgorithmException { + MessageDigest md = MessageDigest.getInstance("SHA-256"); + byte[] digest = md.digest(nonce.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/DefaultOAuth2AuthorizationRequestResolver.java b/oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java index a641e783e33..533d29c3740 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 @@ -24,7 +24,7 @@ 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.IdTokenClaimNames; +import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.web.util.UrlUtils; import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.util.Assert; @@ -210,22 +210,21 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist /** * Creates nonce and its hash for use in OpenID Connect Authentication Requests * - * @param attributes where {@link IdTokenClaimNames#NONCE} is stored for the token request - * @param additionalParameters where hash of {@link IdTokenClaimNames#NONCE} is added to the authentication request + * @param attributes where {@link OidcParameterNames#NONCE} is stored for the token request + * @param additionalParameters where hash of {@link OidcParameterNames#NONCE} is added to the authentication request * * @since 5.2 * @see 15.5.2. Nonce Implementation Notes * @see 3.1.3.7. ID Token Validation */ private void addNonceParameters(Map attributes, Map additionalParameters) { - String nonce = this.stringKeyGenerator.generateKey(); - attributes.put(IdTokenClaimNames.NONCE, nonce); - try { + String nonce = this.stringKeyGenerator.generateKey(); + attributes.put(OidcParameterNames.NONCE, nonce); + String nonceHash = createHash(nonce); - additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); - } catch (NoSuchAlgorithmException e) { - // + additionalParameters.put(OidcParameterNames.NONCE, nonceHash); + } catch (NoSuchAlgorithmException ignored) { } } 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 b2cb1edc79f..6561c517bf6 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 @@ -27,7 +27,7 @@ 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.IdTokenClaimNames; +import org.springframework.security.oauth2.core.oidc.endpoint.OidcParameterNames; import org.springframework.security.web.server.util.matcher.PathPatternParserServerWebExchangeMatcher; import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher; import org.springframework.util.Assert; @@ -214,22 +214,21 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr /** * Creates nonce and its hash for use in OpenID Connect Authentication Requests * - * @param attributes where {@link IdTokenClaimNames#NONCE} is stored for the token request - * @param additionalParameters where hash of {@link IdTokenClaimNames#NONCE} is added to the authentication request + * @param attributes where {@link OidcParameterNames#NONCE} is stored for the token request + * @param additionalParameters where hash of {@link OidcParameterNames#NONCE} is added to the authentication request * * @since 5.2 * @see 15.5.2. Nonce Implementation Notes * @see 3.1.3.7. ID Token Validation */ private void addNonceParameters(Map attributes, Map additionalParameters) { - String nonce = this.stringKeyGenerator.generateKey(); - attributes.put(IdTokenClaimNames.NONCE, nonce); - try { + String nonce = this.stringKeyGenerator.generateKey(); + attributes.put(OidcParameterNames.NONCE, nonce); + String nonceHash = createHash(nonce); - additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash); - } catch (NoSuchAlgorithmException e) { - // + additionalParameters.put(OidcParameterNames.NONCE, nonceHash); + } catch (NoSuchAlgorithmException ignored) { } } diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java index 3afadf01dc2..3afe853967c 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -313,6 +313,37 @@ public void authenticateWhenTokenSuccessResponseThenAdditionalParametersAddedToU this.accessTokenResponse.getAdditionalParameters()); } + // gh-4442 + @Test + public void authenticateWhenTokenSuccessResponseThenAdditionalParametersAddedToUserRequestNoNonce() { + OAuth2AuthorizationRequest authorizationRequestNoNonce = request() + .scope("openid", "profile", "email") + .attributes(new HashMap<>()) + .additionalParameters(new HashMap<>()) + .build(); + OAuth2AuthorizationExchange authorizationExchangeNoNonce = new OAuth2AuthorizationExchange(authorizationRequestNoNonce, this.authorizationResponse); + + Map claims = new HashMap<>(); + claims.put(IdTokenClaimNames.ISS, "https://provider.com"); + claims.put(IdTokenClaimNames.SUB, "subject1"); + claims.put(IdTokenClaimNames.AUD, Arrays.asList("client1", "client2")); + claims.put(IdTokenClaimNames.AZP, "client1"); + this.setUpIdToken(claims); + + OidcUser principal = mock(OidcUser.class); + List authorities = AuthorityUtils.createAuthorityList("ROLE_USER"); + when(principal.getAuthorities()).thenAnswer( + (Answer>) invocation -> authorities); + ArgumentCaptor userRequestArgCaptor = ArgumentCaptor.forClass(OidcUserRequest.class); + when(this.userService.loadUser(userRequestArgCaptor.capture())).thenReturn(principal); + + this.authenticationProvider.authenticate(new OAuth2LoginAuthenticationToken( + this.clientRegistration, authorizationExchangeNoNonce)); + + assertThat(userRequestArgCaptor.getValue().getAdditionalParameters()).containsAllEntriesOf( + this.accessTokenResponse.getAdditionalParameters()); + } + private void setUpIdToken(Map claims) { Instant issuedAt = Instant.now(); Instant expiresAt = Instant.from(issuedAt).plusSeconds(3600); diff --git a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java index ca55e2451c3..4343ec9a00a 100644 --- a/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java +++ b/oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/web/OAuth2AuthorizationRequestRedirectFilterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java index a0f58aaec7d..77f5dbd7e92 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2019 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.