IdentityModel / oidc-client-js

OpenID Connect (OIDC) and OAuth2 protocol support for browser-based JavaScript applications
Apache License 2.0
2.43k stars 841 forks source link

"iat is in the future" for a significant portion of users after daylight savings time #1254

Closed lwansbrough closed 3 years ago

lwansbrough commented 4 years ago

We're seeing a significant number of users run into the error "iat is in the future" after Nov 1st in regions that observe daylight savings time. I believe this is likely due to people not having their computer clocks automatically set. One possible solution as I see it would be adjusting the acceptable clock skew out to 1:05 from 0:05 mins. Is there a better way to handle this?

coolhome commented 3 years ago

I would imagine the best solution would be to have the users correct their system datetime. A lot of software requires synchronized system clocks to operate. Given the nature of how clocks work there are bound to have consistency issues over time. Clock skew is usually intended to only be a few minutes to allow for clocks being slightly off. My reasoning for this is adding to the clock slew would only trick the frontend in thinking the token was valid longer than it was intended for.

https://en.wikipedia.org/wiki/Clock_skew#On_a_network RFC-7519 4.1.4-4.4.6

lwansbrough commented 3 years ago

Unfortunately when you have millions of users this is a common problem (which may come as a surprise), so simply asking people to fix their time comes at a cost to business. Can we disable IAT checks on the client side somehow?

brockallen commented 3 years ago

There is a PR already merged that allows you to inject your own clock service. It's just waiting for me to have time to do a release.

pmoleri commented 3 years ago

Hi @brockallen, this is a nice addition.

I have 2 questions though:

  1. Will silent-renew work properly?

As @coolhome pointed out adding to the clock slew would only trick the frontend in thinking the token was valid longer than it was intended for In the current state ClockService can be used to fix "iat is in the future", but the silent-renew service still uses Date.now() so it may stop working or renew constantly depending on which kind of offset the client has. Isn't it so?

  1. Do you have any suggested way of Initializing the Clock Service?

I guess the class implementation could look like this:

class ServerClock {
    constructor(serverTime) {
        this.offset = Date.now() - serverTime;
    }

    getEpochTime() {
        return Promise.resolve((Date.now() - this.offset) / 1000 | 0);
    }
}

Then, how to feed the correct time?

I guess injecting the Date in the HTML is the most efficient way if you don't need to serve it as a static file, i.e. it can be manipulated in every request and caching for the main file is turned off.

Are there any other ideas?

brockallen commented 3 years ago

@pmoleri not sure on all of your questions -- i'd have to think thru, but quick note is that silent renew passes the results of the iframe to the parent window, and they're validated in the parent window context. so any services there are what will do the validation, and not in the iframe.

pmoleri commented 3 years ago

Hi Brock, regarding Silent Renew, my question was more about if automaticSilentRenew will be able to trigger the renew at the proper time.

E.g.

I haven't check the code in depth, but as I saw Date.now() is still used in most places other than the initial token validation.

brockallen commented 3 years ago

Hi Brock, regarding Silent Renew, my question was more about if automaticSilentRenew will be able to trigger the renew at the proper time.

I think that will be ok, since the token response has an expired_in, and so we use the current clock to calculate the future clock time the token will expire. We don't care the actual time (or if it's off/incorrect).

pmoleri commented 3 years ago

since the token response has an expired_in, and so we use the current clock to calculate the future clock time the token will expire

I see, thanks for clarifying this. I assumed that it was using token.exp.

So, considering:

I could just use a huge clock_skew, like 24hs or 1 week and forget about these errors, right? Would that be insecure somehow?

Thanks

brockallen commented 3 years ago

I see, thanks for clarifying this. I assumed that it was using token.exp.

Access tokens are opaque to the client, thus the protocol has the expires_in response param.

I could just use a huge clock_skew, like 24hs or 1 week and forget about these errors, right?

I'd consult the spec for guidance on that.