wrong invalid token because of system time issues

[wachri]

In processIdToken() in the oauth-service.ts there is a check wether the token has been expired or is issued in the future. The implementation is based on new Date(), which depends on system time of the user. So when the user has a wrong system time, the promise is rejected and the hole login flow fails.

We use the library now for a while and every time a customer contacted our support, we told them to check the system time. Now we run into a special case with a user from the Canary Islands. It seems that on the Canary Islands the time is "official" one hour behind and has the time from Spain. So in this case the user has to set the "wrong" time to login.

So what I like to discuss is how useful is this check or can this be removed?

When it is done in the client, with the client time, what brings it us? I think when a token is generated the expiry date is generated with the time on the server (which can be theoretically wrong, too), so the server is the only authority which really can check the expiry. So when I do a request with a invalid token the server has to check it anyway and should response with a unauthorised status code when the token has been expired. Currently in my oppinion it makes more problems than it solves.

[jeroenheijmans]

Let's split the issue between whether there's a bug in the library or not, and whether checks "can [...] be removed", if that's ok? About the first one:

The implementation is based on new Date(), which depends on system time of the user.

Hmm, don't think it should be? I think it should be the number of milliseconds sinds the UNIX epoch. Or as this answer puts it:

if the local time is correct (for whatever time zone the computer's set to), it should be the same in any time zone.

The iat value against which it's being compared is a "NumericDate", which also works with the UNIX epoch.

So I think the library should just work, if the time of a user's computer is set correctly.

This means you could try to re-check with your users in the Canary Islands, see if their regional settings on their OS are correct (they should have (UTC +00:00) Dublin, London, ... selected I think?

In my experience, with modern software, including web applications, things can get pretty weird if clients have wrong time settings. So fixing that would probably be the best way to tackle things, if possible. However, I also know it sometimes doesn't work that way, coming to the second part.

Is the check useful, or can it be removed?

I think it's probably very useful, at least to me it is. Even if you'd remove the check, any timers listening for expiration (e.g. to do a silent refresh) would break down since they'd also rely on expires_at and iat.

So I don't think removing the check is a good thing to do.

Possibly, making the check optional, via a configuration option, could help for edge cases. But that would also bloat the library with even more options and logic (as well as having possibly weird side effects on things like the refresh timers), and I'm not sure if that's for the better.

PS. I'm not trying to invalidate your use case, and I've also been there with similar weird timezone issues, so I sympathize. Hopefully the above helps you figure out a good scenario. Good luck!

[wachri]

I understand your arguments, especially the point with the silent refresh, too. But I'm still not sure if it is necessary to do the validation in the frontend, when the backend has to validate it, too.

In our case we disabled the validation with the null-validation-handler.ts and removed jsrsasign (which had really a huge impact on our bundle size) stuff by mocking it out with webpack, because as I argued the backend is responsible for the validation.

So what do you think about moving the expire check into the validation layer, so every body can decide if it is needed or not?

When we open a MR request for this, how big is the chance to get this merged?

[manfredsteyer]

The good message is that the next version of this lib will work without jsrasign.

Would it work to make more methods protected so that one can override it as needed?

[wachri]

Issue is getting a bit worser and now I have some more information from a customer. In Morocco the gouvernement decided to keep the summer time (+1 Hour) the hole year. So now they have to change there system time to a wrong time zone to get access...

@manfredsteyer don't know if it would be the best solution in extending the class. Following the architecture of the null-validation-handler.ts I think this would be a better place?

[manfredsteyer]

Normally, the exp date should contain a timezone offset to prevent sth like this. Can you check whether your server is adding it to the date? which server do you use?

[wachri]

Asked the backend team. We are using https://github.com/bshaffer/oauth2-server-php. And the expire date is sent as timestamp

public $exp =>
  int(1542274087)

[jeroenheijmans]

Related, even though there's no supporting issue logged yet, PR #596 might be interesting here?

[wachri]

Nice yes looks promising :)

[oleksii-bielik]

Hello @manfredsteyer @jeroenheijmans!

I have the same issue with some of our customers when they use custom time, do we have any update on this problem? Will it be fixed in the near future? Thanks!

Regards, Oleksii

[jeroenheijmans]

I'll stick with what I wrote before for the moment. Although I sympathize, I think specs like OIDC rely on involved servers and clients have correct clocks/times to prevent a whole class of attacks. I think (presume) you will see similar behavior with other implementations too.

[bmerigan]

We also experience this issue, and while it's technically true that the issue is the customer's to solve, if we had an opportunity to make our software more resilient it's good development practice to do so.
Any reduction in service calls is good.

[buymoney1]

Hi @jeroenheijmans , I have the same problem, and in similar packages, usually, the time is not checked, and this task is examined by the backend to prevent such problems. Please add a feature to add an offsetTime or dateTime provider.
ThankYou

[jeroenheijmans]

@buymoney1 Noticed your at-mention but just for clarity, it would be up to the currently active maintainers; in #1280 I announced to take a step back. Hope that makes sense!