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.

[jeroenheijmans]

Ah, sorry for the confusion @BarsikV. The feature is certainly bugged at the moment, as indicated by the various replies in this issue. I didn't mean to say that "all is well".

Instead, I was specifically responding to the comment above mine. Getting the "current time" from an API or a HTTP header is not the way to go, I meant. I'll edit my previous comment to make that clearer.

[BarsikV]

@jeroenheijmans ok, it was a bit confusing, sorry, and thanks for clarifying.

[LordMidi]

Sorry If I messed things up here. We just have this problem now and got the token expired error and I thought our "research" could help in the overall context. And this is the only thread I found which is active and seems to have a similar cause for the error to occur. Thanks for your answers @jeroenheijmans we will try consider your comment.

[manfredsteyer]

Perhaps there is a misunderstanding, but IMHO the code does what it is supposed to do: Making the token be valid a bit longer b/c clocks might be not in sync.

This is how the check works:

function demo(expiresAt, now, skew) {
  if (expiresAt && parseInt(expiresAt, 10) < now - skew) {
    return false;
  }
  return true;
}

• 5 minutes after expiration still true (b/c of skew of 10):
  demo(100, 105, 10)

• 10 minutes after expiration still true (b/c of skew of 10):
  demo(100, 110, 10)

• 11 minutes after expiration already false (b/c of skew of 10):
  demo(100, 111, 10)

[LordMidi]

As some time has went by and we went into deeper researches we identified the client time as the source of error for us. If the users local time is completely out of sync validation of the token fails (of course). The only bullet proof solution would be getting the current time from the server that issued the token to use this for validation. But we decided to show a error message instead.

[Shne]

Perhaps there is a misunderstanding, but IMHO the code does what it is supposed to do: Making the token be valid a bit longer b/c clocks might be not in sync.

Maybe I'm just missing the use case of that.

I figured the intend would be to consider it invalid a bit earlier, because clocks might not be in sync.
I can see use cases for this:

1. I want to check validity of my access token before use, perhaps because I want to refresh the token beforehand when it has expired. If the clocks are out of sync, I want to consider it invalid earlier, because I would rather refresh earlier than needed, than call backend with an expired token (for whatever reason).
2. I have already called my backend and have not been given access. Now I want to check whether the reason could be that my token has expired. If clocks are out of sync, the backend might consider it expired before I do, so I want to consider it invalid earlier.
3. Network transmission delay is high, and even if the token is valid when I send my request from frontend, by the time the backend receives my token, it can be expired. I might want the clockSkew parameter to also help me in this case, by considering a token invalid a bit earlier.

And then there's also the issue that setting clockSkew to 0 actually makes the default 10 minutes be used instead. (this might have been fixed, I haven't checked)
And maybe it's just me, but 10 minutes seems like an extremely long default skew. I would maybe have expected 10 seconds.

[manfredsteyer]

Hi @Shne,

ah, I see. I did have this in mind when adding the skew. I think, for this use case, you could set it to a negative value. I also check for 0 in the next version (will land soon) so that setting it to 0 works.

[Shne]

Hi @manfredsteyer, thanks for taking the time to look at this.

I think, for this use case, you could set it to a negative value. I also check for 0 in the next version (will land soon) so that setting it to 0 works.

As described in the original post:

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.

But maybe that is also changing in the next version?

[pKrupaJunior]

And maybe it's just me, but 10 minutes seems like an extremely long default skew. I would maybe have expected 10 seconds.

I agree. This can be abused, and potentially a lot of harm could be done in 10 minutes. I think the sensible default value is below 30 seconds. It is especially important to mention this default value in the documentation, because it may be surprising for library consumers to see their tokens valid for 10 minutes after they should have expired.

[manfredsteyer]

Hi @Shne,

ah, I see. I could add an optional property decreaseExpirationBySec. Would this help?

Best wishes,
Manfred

[manfredsteyer]

@pKrupaJunior - yes, that's a good point. I'm a bit afraid of changing it now. Let's spread the word via the docu.

[manfredsteyer]

Just added the new decreaseExpirationBySec.

Feel free to reopen this issue if this doesn't solve the situation for you.

[rasifix]

Perhaps there is a misunderstanding, but IMHO the code does what it is supposed to do: Making the token be valid a bit longer b/c clocks might be not in sync.

This is how the check works:

function demo(expiresAt, now, skew) {
  if (expiresAt && parseInt(expiresAt, 10) < now - skew) {
    return false;
  }
  return true;
}

• 5 minutes after expiration still true (b/c of skew of 10):
  demo(100, 105, 10)
• 10 minutes after expiration still true (b/c of skew of 10):
  demo(100, 110, 10)
• 11 minutes after expiration already false (b/c of skew of 10):
  demo(100, 111, 10)

This is exactly the issue. The token is considered valid when it shouldn't because it is expired. I don't get it why this should be ok in any case!? If the token is within the clockSkew but already expired, the Angular application will happily use it / not perform a new login and the calls fail.

[yuriyward]

@rasifix If the client's time is not in sync with the current time he will always have an invalid token. For me, an idea of skew is to forgive small differences, to make it possible to work with such users (wrong time or slow internet).

[rasifix]

@rasifix If the client's time is not in sync with the current time he will always have an invalid token. For me, an idea of skew is to forgive small differences, to make it possible to work with such users (wrong time or slow internet).

that is not the issue. if the client time is perfectly in sync, the token will be used even after effective expiry for another 10 minutes. Setting the clock skew to -10 minutes fixes the problem. The client will think the token is expired, even if it has not expired, but that is perfectly fine as we can simply login again and get a new token.

[LordMidi]

@rasifix If the client's time is not in sync with the current time he will always have an invalid token. For me, an idea of skew is to forgive small differences, to make it possible to work with such users (wrong time or slow internet).

When the client has an invalid time (more than the skew time - believe me there are a lot) the validation will always fail. So users with invalid time ended up in a loop in our application. Token invalid -> get new token -> Token invalid -> get new token -> ...
So we added like a counter which will break out the loop after some validation fails and redirect to the error page saying "please check you time settings"

[buchatsky]

@manfredsteyer decreaseExpirationBySec is declared as "number of seconds" but it takes part in calculations as milliseconds

[LosD]

@manfredsteyer I'm sorry, but why hasn't this been fixed properly instead of adding optional parameters to work around an obvious bug?

[ValentinFunk]

Just ran into this as well, hasValidAccessToken() is returning true for tokens that are expired

[mmanista-bynd]

@manfredsteyer decreaseExpirationBySec is declared as "number of seconds" but it takes part in calculations as milliseconds

i think this is still a bug as in the quoted comment