Shouldn't time skew be subtracted in isTokenExpired?

[spedy]

Before reporting an issue

• I have read and understood the above terms for submitting issues, and I understand that my issue may be closed without action if I do not follow them.

Area

token-exchange

Describe the bug

Timeskew is a non negative value and is the difference between the server and the client in seconds. Then it should be subtracted not added like in your isTokenExpired code here:

kc.isTokenExpired = function(minValidity) {
        if (!kc.tokenParsed || (!kc.refreshToken && kc.flow != 'implicit' )) {
            throw 'Not authenticated';
        }

        if (kc.timeSkew == null) {
            logInfo('[KEYCLOAK] Unable to determine if token is expired as timeskew is not set');
            return true;
        }

        var expiresIn = kc.tokenParsed['exp'] - Math.ceil(new Date().getTime() / 1000) + kc.timeSkew;
        if (minValidity) {
            if (isNaN(minValidity)) {
                throw 'Invalid minValidity';
            }
            expiresIn -= minValidity;
        }
        return expiresIn < 0;
    }

https://github.com/keycloak/keycloak/blob/67b6cf7eac986f8247f77b0a51c66a1fc4151a38/js/libs/keycloak-js/src/keycloak.js#L617C13-L617C23

Version

19.0.3

Regression

• The issue is a regression

Expected behavior

isTokenExpired should be truthfull

Actual behavior

isTokenExpired sometimes give me false instead of true

How to Reproduce?

Set tokenExpiration to 1 minute in keycloak admin, let the time skew be around 46 seconds, then login to your web app with keycloak-js. Close the computer for a minute. Then open it and you'll get 401 unauthorized requests on your non-keycloak server requests.

Anything else?

No response

[spedy]

Also an issue with latest release

[sschu]

@spedy The goal of this setting is to allow for some time skew between client and server by giving some extra time when deciding if a token is already expired. The goal is not to precisely estimate which skew is there and then correct for it.

[spedy]

But shouldn't this be consolidated with your backend code?

For instance here: https://github.com/keycloak/keycloak/blob/841e2790ad3d58c2ae12cb5325908e6477d9b2ed/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/RefreshTokenTest.java#L270

You are checking if the expiration of the token is within this interval, so in your JS code you should probably do the same. Like so

const currentTime = Math.ceil(new Date().getTime() / 1000);
if (!(kc.tokenParsed['exp'] > currentTime - this.timeSkew && kc.tokenParsed['exp'] < currentTime + this.timeSkew)) {
   return true;
}

[sschu]

When deciding if a token is expired, you just need to check if the expired timestamp is smaller than some given value, not if it is bigger than some given value.

[spedy]

But you are not taking into account that time skew is non-negative and that either the client can lag or the server can lag. That's why there is the interval.

[sschu]

Again, the goal is not to estimate the exact skew. You just want to know if a token expired in the past and for that, you just need to check if the expired timestamp is smaller than some given value. The other direction would be interesting for the iatclaim when you want to make sure that a token was not generated in the future.

[spedy]

Ok makes sense, hopefully BE and FE code are consolidated.

Here is my specific case when putting laptop for sleep and logging in the computer after 60 seconds.

const currentDate = new Date()
const tokenExpiration = new Date(currentDate)
const serverTime = new Date(currentDate)
const clientTime = new Date(currentDate)
const currentTime = clientTime
const timeSkew = 61

tokenExpiration.setHours(12)
tokenExpiration.setMinutes(30)
tokenExpiration.setSeconds(27)

serverTime.setHours(12)
serverTime.setMinutes(30)
serverTime.setSeconds(33)

clientTime.setHours(12)
clientTime.setMinutes(30)
clientTime.setSeconds(33)


console.log('server time', serverTime.getTime())
console.log('client time', clientTime.getTime())
console.log('tokenExpiration', tokenExpiration.getTime())


const expiresIn = tokenExpiration.getTime() - clientTime.getTime() + (timeSkew * 1000)
console.log('expiresIn', expiresIn)

TimeSkew before logout was 0, after logging in after cca 60s it was 61. But the server and client time were synchronized.

Value sources:

• server time - after 401 unauthorized taken from the response header date field
• client time - chrome logging with timestamps
• timeSkew - keycloak.timeSkew
• token expiration - keycloak.tokenParsed?.exp

So it might look like the timeSkew is not reinitiated properly in my case.

@sschu so the problem is here
https://github.com/keycloak/keycloak/blob/8e0436715c77f9ea05a0e58b076c89024e11390e/js/libs/keycloak-js/src/keycloak.js#L1032

In our code we use onTokenExpired to update the token, so when browser tab is running in the background the events are still firing. Consequently the timeSkew variable is updated here:

https://github.com/keycloak/keycloak/blob/b6a82964edf98222341c883aad9e16a93c1c252c/js/libs/keycloak-js/src/keycloak.js#L1020

But when we put the computer to sleep, and we wake it up, the iat is set 60 seconds ago, so the timeSkew is around 60. The expiration time of our token is set to around 1 minute for testing purposes and our token has actually expired but isTokenExpired() returns false, because iat was from before we put the computer to sleep.

So I believe clearing timeSkew somehow would solve it

[jonkoops]

This is certainly a bug. The setTimeout() function should not be used as a means to reliably schedule a timer. Instead we should have a timer fire at an interval and check the expiry. I'm triaging this, a PR would be welcomed.

[keycloak-github-bot]

Due to the amount of issues reported by the community we are not able to prioritise resolving this issue at the moment.

If you are affected by this issue, upvote it by adding a 👍 to the description. We would also welcome a contribution to fix the issue.

[keycloak-github-bot]

Due to lack of updates in the last 180 days this issue will be automatically closed.