Add nonce to OIDC Authentication Request #7337

mkheck · 2019-09-02T15:58:39Z

This is a PR for gh-4442: Consider adding nonce to OAuth2 Authentication Request (#4442).

I've coded the implementation & tests for creating+passing+verifying the nonce in the ID token, and all tests pass (with the exception of the one that wasn't passing in the codebase before).

I would like to open a follow-on request to push some optimizing changes out a bit further.
I'm getting everything I need within the OidcIdTokenValidator by retrieving the nonce via claims, but it would be cleaner to change from dealing with Jwt to dealing with OIdcIdTokens instead. But that would necessitate reaching into & reworking DefaultOidcIdTokenValidatorFactory (& of course tests associated, etc.) and everything that touches. And it touches quite a lot.

In short, this all works, so I believe the issue to be feature complete. But I would like to extend further changes to present a more specific solution throughout.

jgrandja

@mkheck Thanks for the PR! I left a couple of comments for you. Also, can you please rebase off master and squash commits to one and than force push. Please ensure you do fast-forward merges as we use this git process. Thanks!

...ava/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java

...work/security/oauth2/client/web/reactive/function/client/OAuth2AuthorizedClientResolver.java

...rity/oauth2/client/web/reactive/result/method/annotation/OAuth2AuthorizedClientResolver.java

...ore/src/main/java/org/springframework/security/oauth2/core/endpoint/NonceParameterNames.java

jgrandja · 2019-09-07T13:42:35Z

@mkheck I've been thinking about whether the nonce validation should happen in OidcIdTokenValidator or OidcAuthorizationCodeAuthenticationProvider. Although it seems natural that it should happen in OidcIdTokenValidator, there are some challenges with that since either nonce should be passed in via the constructor, as you have it:

public OidcIdTokenValidator(ClientRegistration clientRegistration, String nonce)

which is not ideal since we would have to create a new OidcIdTokenValidator per request since the specific nonce is request specific.

The other solution is to implement the validation check in OidcAuthorizationCodeAuthenticationProvider, which would work well since we have access to the OidcIdToken and OAuth2AuthorizationRequest. We are also doing validation checks there between the OAuth2AuthorizationRequest and OAuth2AuthorizationResponse. So I say we implement the nonce check directly in OidcAuthorizationCodeAuthenticationProvider.

Makes sense?

…est, also added comparison+verification in IdToken

…ssue to be discussed w/team

jgrandja

Thanks for the updates @mkheck! I left some further feedback. Looks like there are some test failures. You can run ./gradlew --refresh-dependencies clean check to ensure build passes before you push.

Also, can you update the commit messages to follow this format.

.../security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java

...ava/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java

...rg/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java

jgrandja · 2019-09-10T18:21:53Z

...rg/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java

+			String nonceHash = createHash(nonce);
+			additionalParameters.put(IdTokenClaimNames.NONCE, nonceHash);
+		} catch (NoSuchAlgorithmException e) {
+			// MAH: TODO...but what?


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.

...rg/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java

...ework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java

...main/java/org/springframework/security/oauth2/core/endpoint/OAuth2AuthorizationResponse.java

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 spring-projectsgh-4442

mkheck · 2019-09-11T00:23:41Z

Hi @jgrandja,

Good catches one and all. I've reworked per your recommendations and all build tests pass. LDAP integration tests are failing, but I suppose those are resolved by Travis?

I've also formatted the commit message per the guidelines you provided. 😃

Please let me know of any additional changes required or desired. Thanks!

mkheck · 2019-09-11T01:06:00Z

Missed 4 unit tests in OAuth2AuthorizationRequestRedirectFilterTests, fixed & re-pushed. Please let me know if I need to repackage in some way. 😑

Still getting local failures with various LDAP tests under org.springframework.security.config.ldap, but I did not modify those. Integration tests to complete successfully in CI pipeline?

jgrandja

Thanks for the updates @mkheck. Please see comments inline.

...rg/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java

.../security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java

...ework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java

.../security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java

...rg/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java

...src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java

Made changes requested by Joe Grandja to PR, including addition of test when nonce isn't present in request. Fixes spring-projectsgh-4442 and addresses changes requested to PR

mkheck

Please let me know Joe, thanks!

jgrandja

Thanks for the updates @mkheck. Please see my comments.

Also, can you add a test in OidcAuthorizationCodeReactiveAuthenticationManagerTests

.../security/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProvider.java

...ty/oauth2/client/oidc/authentication/OidcAuthorizationCodeReactiveAuthenticationManager.java

...rg/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java

...rg/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenDecoderFactory.java

...gframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java

...rity/oauth2/client/oidc/authentication/OidcAuthorizationCodeAuthenticationProviderTests.java

...src/main/java/org/springframework/security/oauth2/core/oidc/endpoint/OidcParameterNames.java

jgrandja · 2019-09-26T12:35:36Z

Thanks for the PR @mkheck! This is now in master via da9f027. Please take a look at the polish commit d3b7a47 and let me know if you have any questions.
Thanks again!

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 2, 2019

jgrandja requested changes Sep 7, 2019

View reviewed changes

jgrandja self-assigned this Sep 7, 2019

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 7, 2019

jgrandja changed the title ~~PR for gh-4442: Consider adding nonce to OAuth2 Authentication Request~~ Add nonce to OAuth2 Authentication Request Sep 7, 2019

mkheck added 2 commits September 7, 2019 17:13

gh4442 spring-projects#4442 Added nonce to OAuth2 Authentication Requ…

8ca42a4

…est, also added comparison+verification in IdToken

Github issue 4442 (spring-projects#4442) completion, with follow-on i…

68c1cbf

…ssue to be discussed w/team

mkheck force-pushed the master branch from 975b329 to 68c1cbf Compare September 7, 2019 23:24

Reworked spring-projectsgh-4442 per feedback from Joe Grandja

ab39bd4

jgrandja requested changes Sep 10, 2019

View reviewed changes

Fixed outlier tests in OAuth2AuthorizationRequestRedirectFilterTests

60cdf3e

jgrandja requested changes Sep 17, 2019

View reviewed changes

mkheck added 2 commits September 20, 2019 15:08

Resolves requested changes to PR for spring-projectsgh-4442

3441389

Made changes requested by Joe Grandja to PR, including addition of test when nonce isn't present in request. Fixes spring-projectsgh-4442 and addresses changes requested to PR

Merge branch 'master' into master

bf73846

mkheck commented Sep 20, 2019

View reviewed changes

jgrandja requested changes Sep 23, 2019

View reviewed changes

jgrandja added this to the 5.2.0 milestone Sep 23, 2019

jgrandja changed the title ~~Add nonce to OAuth2 Authentication Request~~ Add nonce to OIDC Authentication Request Sep 26, 2019

jgrandja closed this Sep 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nonce to OIDC Authentication Request #7337

Add nonce to OIDC Authentication Request #7337

mkheck commented Sep 2, 2019

jgrandja left a comment •

edited

Loading

jgrandja commented Sep 7, 2019

jgrandja left a comment

jgrandja Sep 10, 2019

mkheck commented Sep 11, 2019

mkheck commented Sep 11, 2019

jgrandja left a comment

mkheck left a comment

jgrandja left a comment •

edited

Loading

jgrandja commented Sep 26, 2019 •

edited

Loading

Add nonce to OIDC Authentication Request #7337

Add nonce to OIDC Authentication Request #7337

Conversation

mkheck commented Sep 2, 2019

jgrandja left a comment • edited Loading

Choose a reason for hiding this comment

jgrandja commented Sep 7, 2019

jgrandja left a comment

Choose a reason for hiding this comment

jgrandja Sep 10, 2019

Choose a reason for hiding this comment

mkheck commented Sep 11, 2019

mkheck commented Sep 11, 2019

jgrandja left a comment

Choose a reason for hiding this comment

mkheck left a comment

Choose a reason for hiding this comment

jgrandja left a comment • edited Loading

Choose a reason for hiding this comment

jgrandja commented Sep 26, 2019 • edited Loading

jgrandja left a comment •

edited

Loading

jgrandja left a comment •

edited

Loading

jgrandja commented Sep 26, 2019 •

edited

Loading