Spring-security: Clock skew to check access token expiration has wrong sign

[shimikano]

Summary

After upgrading to 5.2.0.RELEASE, we noticed that the clock skew used to calculate an access token's expiration in conjunction with ServerOAuth2AuthorizedClientExchangeFilterFunction seems to have the wrong sign.

E.g., compare the hasTokenExpired method in the various AuthorizedClientProvider implementations of 5.2.0.RELEASE with the implementation of 5.1.6.RELEASE.

Though consistent with the javadoc, the skew should be added to the current timestamp in order to conservatively consider an access token expired.

Actual Behavior

token has expired <==> expiration time < now - skew

Expected Behavior

token has expired <==> expiration time < now + skew

Version

5.2.0.RELEASE

[jzheaux]

Thank you, @shimikano for checking on this.

I believe the arithmetic is correct as-is.

Consider an expiration time at time 10. At time 9, it is not yet expired: 10 < 9 is false. So far so good.

But, with a skew of 2, 10 < 9 + 2 would be true, meaning that it is expired. This is wrong since the intent of the skew is to give the token 2 units of additional leeway, not 2 units less.

Using this same example of an expiration time at time 10, let's say that it is now time 11, so the token is expired. But, we have a skew of 2, so 10 < 11 - 2 would mean that it is not yet expired, which is correct. 10 < 11 + 2 would state that it is expired, which is not correct.

[shimikano]

Thank you for looking into this.

This is wrong since _the intent of the skew is to give the token 2 units of additional leeway, not 2 units less_.

This is the crucial observation.

The semantics of a skew may depend on the context, but I'd argue that in the specific use case of ServerOAuth2AuthorizedClientExchangeFilterFunction, where the token is relayed to make requests to a resource server, the intent of the skew should be to "shorten" the token's life time, in order to preemptively refresh the token before making the actual request.

Going back to your example, if the token is considered not yet expired at time 11 (and hence wouldn't get refreshed), the request to the resource server would fail due to an expired token. On the other hand, I would like it to be considered expired at time 9.

This used to be the case (and was documented accordingly) pre 5.2.0.

We tried to recreate the old behavior by passing a negative skew to RefreshTokenReactiveOAuth2AuthorizedClientProvider, but there's a check against that.

If the logic you described ("skew = extending a token's life time") is really needed, then I'd suggest removing the negative skew check of RefreshTokenReactiveOAuth2AuthorizedClientProvider for backwards compatibility (and mark this as a breaking change for users of ServerOAuth2AuthorizedClientExchangeFilterFunction).

[philsttr]

@jzheaux I hate adding +1 comments, but I really think this issue needs to be reopened, as the current behavior does not allow clients to preemptively refresh tokens before they expire. Essentially the current behavior guarantees that some client requests will fail due to using an expired token. I'd like to avoid failed client requests.

I 100% agree with @shimikano 's assertion that the skew should give 2 units _less_ leeway on the client side, so that the client will refresh the token _before_ it expires, not _after_ it expires.

On the resource-server side, however, I can totally see a clock skew giving 2 units _more_ leeway (which is what is currently done in the JwtTimestampValidator)

[jgrandja]

@shimikano

where the token is relayed to make requests to a resource server, the intent of the skew should be to "shorten" the token's life time, in order to preemptively refresh the token before making the actual request

This makes sense and it looks like there is indeed an issue with the AuthorizedClientProvider implementations. Let me investigate this further.

[jgrandja]

@shimikano @philsttr I just pushed the fix. Please try it and let me know how it goes. Thanks.