Improve error messages in OidcIdTokenValidator #6349
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * 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. | ||
|
|
@@ -27,7 +27,10 @@ | |
|
|
||
| import java.net.URL; | ||
| import java.time.Instant; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| /** | ||
| * An {@link OAuth2TokenValidator} responsible for | ||
|
|
@@ -41,7 +44,6 @@ | |
| * @see <a target="_blank" href="http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation">ID Token Validation</a> | ||
| */ | ||
| public final class OidcIdTokenValidator implements OAuth2TokenValidator<Jwt> { | ||
| private final ClientRegistration clientRegistration; | ||
|
|
||
| public OidcIdTokenValidator(ClientRegistration clientRegistration) { | ||
|
|
@@ -53,27 +55,10 @@ public OidcIdTokenValidator(ClientRegistration clientRegistration) { | |
| public OAuth2TokenValidatorResult validate(Jwt idToken) { | ||
| // 3.1.3.7 ID Token Validation | ||
| // http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation | ||
| Map<String, Object> invalidClaims = validateRequiredClaims(idToken); | ||
|
|
||
| if (!invalidClaims.isEmpty()){ | ||
| return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims)); | ||
| } | ||
|
|
||
| // 2. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) | ||
|
|
@@ -85,21 +70,21 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { | |
| // The aud (audience) Claim MAY contain an array with more than one element. | ||
| // The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience, | ||
| // or if it contains additional audiences not trusted by the Client. | ||
| if (!idToken.getAudience().contains(this.clientRegistration.getClientId())) { | ||
| invalidClaims.put(IdTokenClaimNames.AUD, idToken.getAudience()); | ||
| } | ||
|
|
||
| // 4. If the ID Token contains multiple audiences, | ||
| // the Client SHOULD verify that an azp Claim is present. | ||
| String authorizedParty = idToken.getClaimAsString(IdTokenClaimNames.AZP); | ||
| if (idToken.getAudience().size() > 1 && authorizedParty == null) { | ||
| invalidClaims.put(IdTokenClaimNames.AZP, authorizedParty); | ||
| } | ||
|
|
||
| // 5. If an azp (authorized party) Claim is present, | ||
| // the Client SHOULD verify that its client_id is the Claim Value. | ||
| if (authorizedParty != null && !authorizedParty.equals(this.clientRegistration.getClientId())) { | ||
| invalidClaims.put(IdTokenClaimNames.AZP, authorizedParty); | ||
| } | ||
|
|
||
| // 7. The alg value SHOULD be the default of RS256 or the algorithm sent by the Client | ||
|
|
@@ -108,16 +93,16 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { | |
|
|
||
| // 9. The current time MUST be before the time represented by the exp Claim. | ||
| Instant now = Instant.now(); | ||
| if (!now.isBefore(idToken.getExpiresAt())) { | ||
| invalidClaims.put(IdTokenClaimNames.EXP, idToken.getExpiresAt()); | ||
| } | ||
|
|
||
| // 10. The iat Claim can be used to reject tokens that were issued too far away from the current time, | ||
| // limiting the amount of time that nonces need to be stored to prevent attacks. | ||
| // The acceptable range is Client specific. | ||
| Instant maxIssuedAt = now.plusSeconds(30); | ||
| if (idToken.getIssuedAt().isAfter(maxIssuedAt)) { | ||
| invalidClaims.put(IdTokenClaimNames.IAT, idToken.getIssuedAt()); | ||
| } | ||
|
|
||
| // 11. If a nonce value was sent in the Authentication Request, | ||
|
|
@@ -127,10 +112,45 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) { | |
| // The precise method for detecting replay attacks is Client specific. | ||
| // TODO Depends on gh-4442 | ||
|
|
||
| if (!invalidClaims.isEmpty()) { | ||
| return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims)); | ||
| } | ||
|
|
||
| return OAuth2TokenValidatorResult.success(); | ||
| } | ||
|
|
||
| private static OAuth2Error invalidIdToken(Map<String, Object> invalidClaims) { | ||
|
There was a problem hiding this comment. Let's remove the methods There was a problem hiding this comment. Also, can you update the tests to assert on the There was a problem hiding this comment. Ok, I've added the changes :) |
||
| String claimsDetail = invalidClaims.entrySet().stream() | ||
| .map(it -> it.getKey()+ "("+it.getValue()+")") | ||
| .collect(Collectors.joining(", ")); | ||
|
|
||
| return new OAuth2Error("invalid_id_token", "The ID Token contains invalid claims: "+claimsDetail, null); | ||
| } | ||
|
|
||
| private static Map<String, Object> validateRequiredClaims(Jwt idToken){ | ||
| Map<String, Object> requiredClaims = new HashMap<>(); | ||
|
|
||
| URL issuer = idToken.getIssuer(); | ||
| if (issuer == null) { | ||
| requiredClaims.put(IdTokenClaimNames.ISS, issuer); | ||
| } | ||
| String subject = idToken.getSubject(); | ||
| if (subject == null) { | ||
| requiredClaims.put(IdTokenClaimNames.SUB, subject); | ||
| } | ||
| List<String> audience = idToken.getAudience(); | ||
| if (CollectionUtils.isEmpty(audience)) { | ||
| requiredClaims.put(IdTokenClaimNames.AUD, audience); | ||
| } | ||
| Instant expiresAt = idToken.getExpiresAt(); | ||
| if (expiresAt == null) { | ||
| requiredClaims.put(IdTokenClaimNames.EXP, expiresAt); | ||
| } | ||
| Instant issuedAt = idToken.getIssuedAt(); | ||
| if (issuedAt == null) { | ||
| requiredClaims.put(IdTokenClaimNames.IAT, issuedAt); | ||
| } | ||
|
|
||
| return requiredClaims; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change the method signature to
invalidIdToken(Map<String, Object> invalidClaims). This method would return oneOAuth2Errorwith a generic message that lists theinvalidClaimsin comma-delimited format.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, here I use the keys and join them with a comma to create an OAuth2Error like "Required claims were not found: issuer, iat".
What about the rest of the validations that are performed once we are sure that the required claims are present? Should we keep the message stating why the validation failed?
I submitted new changes... let me know what you think, as always I'm happy to make changes if needed.
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the message generic.
For example:
If one of the required claims is not present than fail-fast and return this message:
The ID Token contains invalid claims: sub (null), aud (null)NOTE: Check all required claims before failing fast
If all required claims are present than proceed with the rest of the validation and if there is a failure than display the same generic message:
The ID Token contains invalid claims: aud (invalid-aud), azp (invalid-azp)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok Joe, I submitted new changes