Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nonce to OIDC Authentication Request #7337

Closed
wants to merge 7 commits into from
Closed

Add nonce to OIDC Authentication Request #7337

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8ca42a4
gh4442 https://github.com/spring-projects/spring-security/issues/4442…
mkheck Sep 1, 2019
68c1cbf
Github issue 4442 (https://github.com/spring-projects/spring-security…
mkheck Sep 2, 2019
ab39bd4
Reworked gh-4442 per feedback from Joe Grandja
mkheck Sep 9, 2019
595ab60
Added nonce creation, verification, & handling
mkheck Sep 11, 2019
60cdf3e
Fixed outlier tests in OAuth2AuthorizationRequestRedirectFilterTests
mkheck Sep 11, 2019
3441389
Resolves requested changes to PR for gh-4442
mkheck Sep 20, 2019
bf73846
Merge branch 'master' into master
mkheck Sep 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...security/config/annotation/web/configurers/oauth2/client/OAuth2ClientConfigurerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
* Tests for {@link OAuth2ClientConfigurer}.
*
* @author Joe Grandja
* @author Mark Heckler
*/
public class OAuth2ClientConfigurerTests {
private static ClientRegistrationRepository clientRegistrationRepository;
Expand Down Expand Up @@ -138,7 +139,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
Expand All @@ -151,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
Expand Down Expand Up @@ -203,7 +206,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));
}
Expand Down
27 changes: 27 additions & 0 deletions ...curity/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -61,6 +66,7 @@
* to complete the authentication.
*
* @author Joe Grandja
* @author Mark Heckler
* @since 5.0
* @see OAuth2LoginAuthenticationToken
* @see OAuth2AccessTokenResponseClient
Expand All @@ -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";
mkheck marked this conversation as resolved.
Show resolved Hide resolved
private final OAuth2AccessTokenResponseClient<OAuth2AuthorizationCodeGrantRequest> accessTokenResponseClient;
private final OAuth2UserService<OidcUserRequest, OidcUser> userService;
private JwtDecoderFactory<ClientRegistration> jwtDecoderFactory = new OidcIdTokenDecoderFactory();
Expand Down Expand Up @@ -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));
mkheck marked this conversation as resolved.
Show resolved Hide resolved
} catch (NoSuchAlgorithmException e) {
OAuth2Error nonceVerificationError = new OAuth2Error(UNVERIFIABLE_NONCE, e.getMessage(), null);
mkheck marked this conversation as resolved.
Show resolved Hide resolved
throw new OAuth2AuthenticationException(nonceVerificationError);
}
String nonceClaim = idToken.getClaim(IdTokenClaimNames.NONCE);
if (nonceClaim == null || !nonceClaim.equals(nonceHash)) {
mkheck marked this conversation as resolved.
Show resolved Hide resolved
OAuth2Error nonceError = new OAuth2Error(INVALID_NONCE);
throw new OAuth2AuthenticationException(nonceError);
}

OidcUser oidcUser = this.userService.loadUser(new OidcUserRequest(
clientRegistration, accessTokenResponse.getAccessToken(), idToken, additionalParameters));
Collection<? extends GrantedAuthority> mappedAuthorities =
Expand Down Expand Up @@ -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 {
mkheck marked this conversation as resolved.
Show resolved Hide resolved
MessageDigest md = MessageDigest.getInstance("SHA-256");
byte[] digest = md.digest(codeVerifier.getBytes(StandardCharsets.US_ASCII));
return Base64.getUrlEncoder().withoutPadding().encodeToString(digest);
}
}
3 changes: 3 additions & 0 deletions ...springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
*
* @author Joe Grandja
* @author Rafael Dominguez
* @author Mark Heckler
* @since 5.2
* @see JwtDecoderFactory
* @see ClientRegistration
Expand Down Expand Up @@ -88,12 +89,14 @@ public final class OidcIdTokenDecoderFactory implements JwtDecoderFactory<Client
Converter<Object, ?> booleanConverter = getConverter(TypeDescriptor.valueOf(Boolean.class));
Converter<Object, ?> instantConverter = getConverter(TypeDescriptor.valueOf(Instant.class));
Converter<Object, ?> urlConverter = getConverter(TypeDescriptor.valueOf(URL.class));
Converter<Object, ?> stringConverter = getConverter(TypeDescriptor.valueOf(String.class));
Converter<Object, ?> collectionStringConverter = getConverter(
TypeDescriptor.collection(Collection.class, TypeDescriptor.valueOf(String.class)));

Map<String, Converter<Object, ?>> claimTypeConverters = new HashMap<>();
claimTypeConverters.put(IdTokenClaimNames.ISS, urlConverter);
claimTypeConverters.put(IdTokenClaimNames.AUD, collectionStringConverter);
claimTypeConverters.put(IdTokenClaimNames.NONCE, stringConverter);
jgrandja marked this conversation as resolved.
Show resolved Hide resolved
claimTypeConverters.put(IdTokenClaimNames.EXP, instantConverter);
claimTypeConverters.put(IdTokenClaimNames.IAT, instantConverter);
claimTypeConverters.put(IdTokenClaimNames.AUTH_TIME, instantConverter);
Expand Down
3 changes: 2 additions & 1 deletion .../org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +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.
// TODO Depends on gh-4442
// gh-4442 implemented elsewhere
mkheck marked this conversation as resolved.
Show resolved Hide resolved

if (!invalidClaims.isEmpty()) {
return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims));
Expand Down Expand Up @@ -167,4 +167,5 @@ private static Map<String, Object> validateRequiredClaims(Jwt idToken) {

return requiredClaims;
}

}
3 changes: 3 additions & 0 deletions ...amework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
*
* @author Joe Grandja
* @author Rafael Dominguez
* @author Mark Heckler
* @since 5.2
* @see ReactiveJwtDecoderFactory
* @see ClientRegistration
Expand Down Expand Up @@ -88,12 +89,14 @@ public final class ReactiveOidcIdTokenDecoderFactory implements ReactiveJwtDecod
Converter<Object, ?> booleanConverter = getConverter(TypeDescriptor.valueOf(Boolean.class));
Converter<Object, ?> instantConverter = getConverter(TypeDescriptor.valueOf(Instant.class));
Converter<Object, ?> urlConverter = getConverter(TypeDescriptor.valueOf(URL.class));
Converter<Object, ?> stringConverter = getConverter(TypeDescriptor.valueOf(String.class));
Converter<Object, ?> collectionStringConverter = getConverter(
TypeDescriptor.collection(Collection.class, TypeDescriptor.valueOf(String.class)));

Map<String, Converter<Object, ?>> claimTypeConverters = new HashMap<>();
claimTypeConverters.put(IdTokenClaimNames.ISS, urlConverter);
claimTypeConverters.put(IdTokenClaimNames.AUD, collectionStringConverter);
claimTypeConverters.put(IdTokenClaimNames.NONCE, stringConverter);
jgrandja marked this conversation as resolved.
Show resolved Hide resolved
claimTypeConverters.put(IdTokenClaimNames.EXP, instantConverter);
claimTypeConverters.put(IdTokenClaimNames.IAT, instantConverter);
claimTypeConverters.put(IdTokenClaimNames.AUTH_TIME, instantConverter);
Expand Down
40 changes: 34 additions & 6 deletions ...springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +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.web.util.UrlUtils;
import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
import org.springframework.util.Assert;
Expand Down Expand Up @@ -51,6 +52,7 @@
* @author Joe Grandja
* @author Rob Winch
* @author Eddú Meléndez
* @author Mark Heckler
* @since 5.1
* @see OAuth2AuthorizationRequestResolver
* @see OAuth2AuthorizationRequestRedirectFilter
Expand All @@ -61,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 codeVerifierGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96);
private final StringKeyGenerator randomKeyGenerator = new Base64StringKeyGenerator(Base64.getUrlEncoder().withoutPadding(), 96);
mkheck marked this conversation as resolved.
Show resolved Hide resolved

/**
* Constructs a {@code DefaultOAuth2AuthorizationRequestResolver} using the provided parameters.
Expand Down Expand Up @@ -118,11 +120,15 @@ private OAuth2AuthorizationRequest resolve(HttpServletRequest request, String re
OAuth2AuthorizationRequest.Builder builder;
if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) {
builder = OAuth2AuthorizationRequest.authorizationCode();
Map<String, Object> additionalParameters = new HashMap<>();

createNonceAndHashForRequest(attributes, additionalParameters);

if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) {
Map<String, Object> additionalParameters = new HashMap<>();
addPkceParameters(attributes, additionalParameters);
builder.additionalParameters(additionalParameters);
}

builder.additionalParameters(additionalParameters);
} else if (AuthorizationGrantType.IMPLICIT.equals(clientRegistration.getAuthorizationGrantType())) {
builder = OAuth2AuthorizationRequest.implicit();
} else {
Expand Down Expand Up @@ -201,6 +207,28 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist
.toUriString();
}

/**
* 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
*
* @since 5.2
* @see <a target="_blank" href="https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes">15.5.2. Nonce Implementation Notes</a>
* @see <a target="_blank" href="https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation">3.1.3.7. ID Token Validation</a>
*/
private void createNonceAndHashForRequest(Map<String, Object> attributes, Map<String, Object> additionalParameters) {
mkheck marked this conversation as resolved.
Show resolved Hide resolved
String nonce = this.randomKeyGenerator.generateKey();
attributes.put(IdTokenClaimNames.NONCE, nonce);

try {
String nonceHash = createHash(nonce);
jgrandja marked this conversation as resolved.
Show resolved Hide resolved
additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash);
mkheck marked this conversation as resolved.
Show resolved Hide resolved
} catch (NoSuchAlgorithmException e) {
// MAH: TODO...but what?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the nonce hash could not be created because of JCE crypto lib missing than we should not add the nonce to attributes, effectively creating an Authentication Request without a nonce as is the current impl.

}
}

/**
* Creates and adds additional PKCE parameters for use in the OAuth 2.0 Authorization and Access Token Requests
*
Expand All @@ -214,18 +242,18 @@ private static String expandRedirectUri(HttpServletRequest request, ClientRegist
* @see <a target="_blank" href="https://tools.ietf.org/html/rfc7636#section-4.2">4.2. Client Creates the Code Challenge</a>
*/
private void addPkceParameters(Map<String, Object> attributes, Map<String, Object> 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) {
additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeVerifier);
}
}

private String createCodeChallenge(String codeVerifier) throws NoSuchAlgorithmException {
private String createHash(String codeVerifier) throws NoSuchAlgorithmException {
mkheck marked this conversation as resolved.
Show resolved Hide resolved
MessageDigest md = MessageDigest.getInstance("SHA-256");
byte[] digest = md.digest(codeVerifier.getBytes(StandardCharsets.US_ASCII));
return Base64.getUrlEncoder().withoutPadding().encodeToString(digest);
Expand Down
46 changes: 36 additions & 10 deletions ...rk/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +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.web.server.util.matcher.PathPatternParserServerWebExchangeMatcher;
import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher;
import org.springframework.util.Assert;
Expand All @@ -52,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
mkheck marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -75,7 +77,7 @@ public class DefaultServerOAuth2AuthorizationRequestResolver

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);

/**
* Creates a new instance
Expand Down Expand Up @@ -132,16 +134,18 @@ private OAuth2AuthorizationRequest authorizationRequest(ServerWebExchange exchan
OAuth2AuthorizationRequest.Builder builder;
if (AuthorizationGrantType.AUTHORIZATION_CODE.equals(clientRegistration.getAuthorizationGrantType())) {
builder = OAuth2AuthorizationRequest.authorizationCode();
Map<String, Object> additionalParameters = new HashMap<>();

createNonceAndHashForRequest(attributes, additionalParameters);

if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())) {
Map<String, Object> 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());
Expand Down Expand Up @@ -207,6 +211,28 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr
.toUriString();
}

/**
* 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
*
* @since 5.2
* @see <a target="_blank" href="https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes">15.5.2. Nonce Implementation Notes</a>
* @see <a target="_blank" href="https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation">3.1.3.7. ID Token Validation</a>
*/
private void createNonceAndHashForRequest(Map<String, Object> attributes, Map<String, Object> additionalParameters) {
String nonce = this.randomKeyGenerator.generateKey();
attributes.put(IdTokenClaimNames.NONCE, nonce);

try {
String nonceHash = createHash(nonce);
additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash);
} 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
*
Expand All @@ -220,18 +246,18 @@ private static String expandRedirectUri(ServerHttpRequest request, ClientRegistr
* @see <a target="_blank" href="https://tools.ietf.org/html/rfc7636#section-4.2">4.2. Client Creates the Code Challenge</a>
*/
private void addPkceParameters(Map<String, Object> attributes, Map<String, Object> 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) {
additionalParameters.put(PkceParameterNames.CODE_CHALLENGE, codeVerifier);
}
}

private String createCodeChallenge(String codeVerifier) throws NoSuchAlgorithmException {
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);
Expand Down
Loading