ID Token validation should support clock skew

[zcwang3]

Summary

I am using spring security 5.0.7 with spring boot 2.0.4.RELEASE for login with Google and Login with Microsoft feature.

I meet the same issue of OidcAuthorizationCodeAuthenticationProvider maxIssuedAt which is closed without any enhancement

Actual Behavior

Today, when i login with Google account in one of my testing machine, I meet the issue with follow error message:
c.c.w.g.c.SecurityConfiguration$2 - Authentication request failed: org.springframework.security.oauth2.core.OAuth2AuthenticationException: [invalid_id_token]

it does not output the detail reason why id token is invalid: like aud is null or audience is not same with client id or idToken is expired etc.

After debug, I find that the failure reason is because of issuedAt.isAfter(maxIssuedAt) check failure, you can see detail error reason in the following screen shots:

Expected Behavior

Login with Microsoft and Google account should be successful in all different machines with some grace time

Please notice
Login with Microsoft account with ID token is successful in the same machine.
Login with Google account with ID token is failure because of issuedAt.isAfter(maxIssuedAt) check failure
Login with Microsoft and Google account with ID token are both successful in other testing machines

Since we can not control the issuedAt value which generated by Microsoft or Google and issuedAt generated by Microsoft and Google is obviously different in my testing machine.

Enhance request 1: add detail error reason in exception when id_tioken validate failure
Enhance request 2: default 30 second in the check of issuedAt.isAfter(maxIssuedAt) becomes configurable
Suggestion to fix with existing spring security version : And for current spring security version without previous enhance support, how can I do to change the value of 30 to 300s, do we have some extension point for it without change sourcecode in jar?

Configuration

no special configuration, the issue is just related with id token check logic in OidcAuthorizationCodeAuthenticationProvider

Version

org.springframework.boot spring-boot-starter-parent 2.0.4.RELEASE

 <dependency>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-security</artifactId>
    </dependency>
    <dependency>
        <groupId>org.springframework.security</groupId>
        <artifactId>spring-security-oauth2-client</artifactId>
    </dependency>
    <dependency>
        <groupId>org.springframework.security</groupId>
        <artifactId>spring-security-oauth2-jose</artifactId>
    </dependency>

Sample

no special configuration, the issue is just related with id token check logic in OidcAuthorizationCodeAuthenticationProvider

[jgrandja]

@zcwang3 You've tested 3 different scenarios with 2 passing and 1 failing so this suggests a possible environment issue. I took a look at the debug screenshot you provided and it looks like you have a clock sync issue between the computer you're testing on and Google's clock.

2018-09-13T08:55:26.893Z - Now (Testing machine clock)
2018-09-13T08:56:02Z - IssuedAt (Google's clock)

As you can see, the clock on your testing machine is not in sync with Google's clock. Looks like the testing machine clock is approx. 36 secs behind Google's clock. Can you adjust the clock on the testing machine and re-test?

Please also see this comment for more info.

There is no current way for a user to configure maxIssuedAt at the moment. So to get around this issue you'll need to ensure the clocks in your application environments are in sync (or close) with the Provider environments.

We can look at adding this support in a future release. And I also feel that a more detailed exception message for ID Token validation failures is needed.

[zcwang3]

@jgrandja Thanks for you checking.

Please notice: in the same test machine, Microsoft ID token verification is pass and Google ID token verification is failure because of maxIssuedAt check issue.
it means that the clock on my testing machine is sync with Microsoft's clock, and it is not sync with Google's clock.

After I have sync clock on my testing machine with ntp command, the same issue still exists.

So i think clock out of sync issue should exist in Google and I can do nothing for that.

So, I need increate grace time of maxIssuedAt check to avoid potential production issue caused by clock out of sync in OpenID provider (like Microsoft / Google / etc...)

[jgrandja]

@zcwang3

in the same test machine, Microsoft ID token verification is pass and Google ID token verification is failure because of maxIssuedAt check issue.
it means that the clock on my testing machine is sync with Microsoft's clock, and it is not sync with Google's clock.

Can you please provide another sceenshot from the test machine for Microsoft and Google. I'd like to see what the issueAt and now times are.
Thanks.

[zcwang3]

Hi @jgrandja

here is the attached screen shots for Microsoft / Google id token verification

Google ID token verification: Google clock is about 40s faster than our server

Microsoft ID token verification: Microsoft clock is about 5 minutes slower than our server

BTW:
as work around solution to avoid potential production issue, i copy code from OidcAuthorizationCodeAuthenticationProvider and then generate a new class MyOidcAuthorizationCodeAuthenticationProvider, in this new class i have change value from 30 to 300 and add detail error reason when token is invalid. You can refer to the following source code

private void validateIdToken(OidcIdToken idToken, ClientRegistration clientRegistration) {
// 3.1.3.7 ID Token Validation
// http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation

    // Validate REQUIRED Claims
    URL issuer = idToken.getIssuer();
    if (issuer == null) {
        this.throwInvalidIdTokenException("issuer is null");
    }
    String subject = idToken.getSubject();
    if (subject == null) {
        this.throwInvalidIdTokenException("subject is null");
    }
    List<String> audience = idToken.getAudience();
    if (CollectionUtils.isEmpty(audience)) {
        this.throwInvalidIdTokenException("audience is empty");
    }
    Instant expiresAt = idToken.getExpiresAt();
    if (expiresAt == null) {
        this.throwInvalidIdTokenException("expiresAt is null");
    }
    Instant issuedAt = idToken.getIssuedAt();
    if (issuedAt == null) {
        this.throwInvalidIdTokenException("issuedAt is null");
    }

    // 2. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery)
    // MUST exactly match the value of the iss (issuer) Claim.
    // TODO Depends on gh-4413

    // 3. The Client MUST validate that the aud (audience) Claim contains its client_id value
    // registered at the Issuer identified by the iss (issuer) Claim as an audience.
    // 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 (!audience.contains(clientRegistration.getClientId())) {
        this.throwInvalidIdTokenException("client id does not match with audience, audience is " + audience);
    }

    // 4. If the ID Token contains multiple audiences,
    // the Client SHOULD verify that an azp Claim is present.
    String authorizedParty = idToken.getAuthorizedParty();
    if (audience.size() > 1 && authorizedParty == null) {
        this.throwInvalidIdTokenException("authorizedParty is null");
    }

    // 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(clientRegistration.getClientId())) {
        this.throwInvalidIdTokenException("client id does not match with authorizedParty, authorizedParty is " + authorizedParty);
    }

    // 7. The alg value SHOULD be the default of RS256 or the algorithm sent by the Client
    // in the id_token_signed_response_alg parameter during Registration.
    // TODO Depends on gh-4413

    // 9. The current time MUST be before the time represented by the exp Claim.
    Instant now = Instant.now();
    if (!now.isBefore(expiresAt)) {
        this.throwInvalidIdTokenException(**"token is expired, now is " + now + ", expiresAt is " + expiresAt**);
    }

    // 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 = Instant.now().plusSeconds(**300**);
    if (issuedAt.isAfter(maxIssuedAt)) {
        this.throwInvalidIdTokenException(**"maxIssuedAt check failure, now is " + now + ", issuedAt is " + issuedAt  + ", maxIssuedAt is " + maxIssuedAt**);
    }

    // 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.
    // TODO Depends on gh-4442

}

private void throwInvalidIdTokenException(**String errorReason**) {
    OAuth2Error invalidIdTokenError = new OAuth2Error(INVALID_ID_TOKEN_ERROR_CODE);
    throw new OAuth2AuthenticationException(invalidIdTokenError, invalidIdTokenError.toString() + ":" + errorReason);
}

[zcwang3]

Hi @jgrandja

Do we have any conclusion/plan for this issue?

[jgrandja]

@zcwang3 Apologies for the delayed response. We were quite busy leading up to the 5.1 release and SpringOne last week.

I do have a plan on how to provide a configurable clock skew value for maxIssuedAt. I'm quite busy with other priorities at the moment though.

Would you be interested in submitting a PR for this? I could guide you through the process and detail the plan/idea I have for this change.

[zcwang3]

@jgrandja Sorry for late response because of holiday leave. I can have a try to fix this issue.
Could you please help to share the information you mentioned?
And which location is suggested for configuration of maxIssuedAt?

[jgrandja]

@zcwang3 There are 2 issues that will address this #5930 and #5717.

I started on #5717 but ran into a design issue so it didn't make it into 5.1.
I'll get back to this one soon.

What are your thoughts on working on #5930?

[zcwang3]

@jgrandja I am work overtime for my daily work and not have time to work on #5930 now.
May be I will try to pick it up later if I have time.

Thanks

[jgrandja]

@zcwang3 No worries. I'll try to get to it soon as well. If you get to it before me please let me know. Thanks.

[rwinch]

@jgrandja In the meantime should we rename this to be something like...OidcAuthorizationCodeAuthenticationProvider should support clock skew?

[jgrandja]

@rwinch Yes, thanks for pointing that out. I renamed it to be more specific to ID Token validation.

[gburboz]

@jgrandja clock skew is just one of the many factors due to which this check would fail (e.g. network latency). I'd recommend to name this client config as issuedWithin

Additionally, clock skew is applicable not just to issuedAt but also to expiresAt. I'd recommend to have additional provider level property allowedClockSkew with some default value which can be overridden from config and be used anywhere such comparison is required.

Instant now = Instant.now();
...
Instant maxExpiresAt = now.plusSeconds(allowedClockSkew);
...
Instant maxIssuedAt = now.plusSeconds(issuedWithin + allowedClockSkew);
...

[jgrandja]

@gburboz Thanks for the feedback. I'll consider your points when I get to this issue.

[yannick-fernand]

Hi @jgrandja i have the same issue. when do you think you will resolve it.
thanks

[jgrandja]

@yannick-fernand This is scheduled for 5.2.0.M1 as indicated in the ticket. However, given that M1 is scheduled for Jan 15, it might not make it until M2. I'm trying to catch up with things since the break. I'll do my best.

[jgrandja]

@zcwang3 @gburboz @yannick-fernand Clock skew can now be configured via OidcIdTokenValidator.setClockSkew(). However, it's not that easy to configure at the moment as it will require some code duplication on the user's end. This is being addressed in #6379. I'm in the process of putting together a PR for this. After #6379 is resolved, it should be fairly straight forward to configure a clock skew per ClientRegistration - which implicitly supports per provider.

[jgrandja]

@zcwang3 @gburboz @yannick-fernand #6379 has been resolved which allows the user to configure the clock skew (per provider) in a straight forward way. The following example demonstrates a custom clock skew setting for google and okta - all other providers default to 60.

@Configuration
public class OAuth2LoginConfig {

	@Bean
	public JwtDecoderFactory<ClientRegistration> idTokenDecoderFactory() {
		OidcIdTokenDecoderFactory idTokenDecoderFactory = new OidcIdTokenDecoderFactory();

		idTokenDecoderFactory.setJwtValidatorFactory(clientRegistration -> {
			OidcIdTokenValidator idTokenValidator = new OidcIdTokenValidator(clientRegistration);
			if (clientRegistration.getRegistrationId().equals("google")) {
				idTokenValidator.setClockSkew(Duration.ofSeconds(30));
			} else if (clientRegistration.getRegistrationId().equals("okta")) {
				idTokenValidator.setClockSkew(Duration.ofSeconds(45));
			}
			return idTokenValidator;
		});

		return idTokenDecoderFactory;
	}
}

NOTE: Please be aware that OidcIdTokenDecoderFactory may be renamed depending on the outcome of #6425. This will be decided before 5.2.0.RC1.

[OdDev26]

Hello @jgrandja, how can this new method be used?: idTokenDecoderFactory()