Clock skew is in wrong direction

[Shne]

Describe the bug
Unless the intent is to consider an access/id token valid for 10 minutes (by default) after it has expired, the clock skew calculation is in the wrong direction.

This, somewhat recent, commit seems to be the culprit: 68238fb#diff-685e26c24f3c008856834f8b4a350d5472b6aea01a623c31a8513f6cc599c57fR2391
The pluses should not have been changed to minuses.

It causes the current time to be considered 10 minutes (by default) in the past, causing the token to be considered valid for longer.
I.e.: with the current code, the current time needs to be 10 minutes after the token expiration time for the comparison to turn true and the return value (indicating validity of token) to become false.
Perhaps it could be worth refactoring this code for readability. At least, I had some difficulty understanding it.

Expected behavior
Tokens should be considered expired once they expire.

Versions
Library version 12.1.0

Additional context
I was experiencing problems with access tokens considered valid by the hasValidAccessToken() method, where I knew they were expired.

I can't even use negative values to fix it, because of these lines: https://github.com/manfredsteyer/angular-oauth2-oidc/blob/12.1/projects/lib/src/oauth-service.ts#L2233 that will throw 'Token has expired' Errors if I use a reasonable, but negative, value for clockSkewInSec.

I also consider it a bug that setting the clock skew to 0 causes the default to be used, because the value is checked for truthiness, instead of compared against undefined and/or null. This is also a new change, here: 68238fb#diff-685e26c24f3c008856834f8b4a350d5472b6aea01a623c31a8513f6cc599c57fR2121

[pKrupaJunior]

I agree with all the issues raised above.
It is difficult to understand why the default value is 10 minutes since it is not mentioned in the documentation. The tsdoc for the AuthConfig#clockSkewInSec parameter does not mention the default value and it only states it used for id tokens ("validating id_token's iat and exp values").
Moreover it is difficult to locate this default value since it is buried in getClockSkewInMsec private method

I also consider it a bug that setting the clock skew to 0 causes the default to be used, because the value is checked for truthiness, instead of compared against undefined and/or null.

I also think this is a bug since I do not see a reason why 0 would be an invalid value for the clock skew.

[ddoerr]

Im new to OAuth in my applications, but isnt this a serious issue? How can I determine if a user is authenticated if not by hasValidAccessToken()? If I can't trust hasValidAccessToken() I have to use refreshToken() every time I have to check auth status. Am I missing something?

[Shne]

Yeah, I'm surprised this hasn't been fixed yet.

I ended up writing my own short method for checking whether access token is expired:

private get isAccessTokenExpired() {
  // custom check of token expiration to avoid bug in library: https://github.com/manfredsteyer/angular-oauth2-oidc/issues/1135
  const expiresAt = this.oAuthService.getAccessTokenExpiration() as number | null; // can return null https://github.com/manfredsteyer/angular-oauth2-oidc/blob/12.1/projects/lib/src/oauth-service.ts#L2354
  return Date.now() > (expiresAt ?? 0) - (this.oAuthService.clockSkewInSec ?? 0);
}

[rasifix]

why has this bug not been fixed so far? Because nobody created a PR? Or does somebody have reservations about fixing this?!

[andreagrossetti]

I've wasted sooooo much time trying to figure out why sometimes I was logged out with no reason. This is a MAJOR bug, please fix it as soon as possible.

[buchatsky]

If the "skew" is like a token response flight time, the sign must be definitely "+". Expired condition:

now > Svr.expAt
now > Cli.expAt - skew
Cli.expAt < now + skew

[BarsikV]

I have run into a similar problem with this variable being used wrongly.
I also was relying on it to check if the computer time was wrong, and show a page to the user if they need to fix their computer time. However, this is being calculated wrongly here

angular-oauth2-oidc/projects/lib/src/oauth-service.ts

Lines 2266 to 2267 in d95d7da

	issuedAtMSec - clockSkewInMSec >= now ||
	expiresAtMSec + clockSkewInMSec <= now

It first checks for the issued at time - clockskew, then for the expired at time + the clock skew, and throws a 'Token expired exception'.
There is more than one problem between those lines. It tries to do two different checks at once, and both of them are incomplete.
Yes, if the expired at time + the clock skew is greater than the current time, the token is really expired. But what is really needed is to check if the issued at time + the clock skew is greater than the current time, so that we can know if the computer time is off.
If the computer time is off, the token expiration is still wrong, but we will just notice it later.

Also, this check explained above only checks the ID token, but the access token never get checked like that, which is also weird.

[LordMidi]

A problem that we face is that if you the client's system time is not in sync the users will run into a never ending loop.
"Token expired"-error -> login tried again -> "Token expired"-error -> ...

I know it's possible to provide a custom date-time-service - https://github.com/manfredsteyer/angular-oauth2-oidc/blob/2c0d16bbe20136cfbd3ebc6babdc49dac1aa7d78/docs-src/custom-date-time-provider.md
but wouldn't it an idea to get the "current" time from the HTTP-Header response of the token-request? In this request the time of the token issuer server is included and can not colide with wrong client settings.

Another idea is to implement an HTTP-call to get the current time from a date service or the token-issuer inside of the custom date-time-provider. But would be an overheader as this value is already part of the HTTP-responses of the id-provider. I'm wondering if this value is accessible inside of the now()-function of the date-time-provider...

[jeroenheijmans]

💡 EDIT: This comment is specifically meant as a reply to the one immediately above, sharing my thoughts about the suggestion to get the "current time" / clock in the library from an HTTP Header or via an API Call from a server. It's not so much about the bug with the clock skew feature.

---

Thx for the additional insights @LordMidi. You pose some interesting ideas. I do think if you want to implement those I recommend using a custom date-time service to do so, and I would caution strongly against it.

Security features like these rely on both parties having a decently accurate clock on their ends. They prevent tampering with data by folks in the middle or otherwise compromised setups. If you ask the server (via a header or an api) for "the current time" to do checks on tokens, you give away some control over security.

Of course the bug with clock skew needs to be fixed at some point, because some "bandwidth"/skew will in practice happen. But apart from that I think the check is good as is. If you cannot rely on accurate clocks it is perhaps better to completely turn off these security features (e.g. via a custom datetime provider) as opposed to relying on another party for providing time to your app.

In short: if you want to use your suggestion as a workaround, because you understand the security implications, I'd say "go for it", using the existing escape hatches. But I'd be weary to add features to the library of the sort.

Hope that makes sense?

[BarsikV]

Of course the bug with clock skew needs to be fixed at some point, because some "bandwidth"/skew will in practice happen. But apart from that I think the check is good as is. If you cannot rely on accurate clocks it is perhaps better to completely turn off these security features (e.g. via a custom datetime provider) as opposed to relying on another party for providing time to your app.

I'm sorry, are we talking about the same thing? What check is good as is? It's not only a security check, since you need to check when the token expires. The check clearly doesn't work as expected. It has many issues, as described above. Of course, you may have a different idea about how to use the library "correctly", but it's completely fair to ask for a proper clock skew check. And it feels a bit degrading, when you say it's all good as is.