Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix clock skew to right direction #1237

Closed

Conversation

tobias-trame
Copy link

Fixes #1135

@santam85
Copy link

@manfredsteyer Would be great if we could get this one checked and merged, we are running into #1135 too :(

@Feffy
Copy link

Feffy commented Sep 22, 2022

@manfredsteyer Any idea when we could get this one merged and released? I can see a lot of people are running into #1135

@@ -2263,8 +2263,8 @@ export class OAuthService extends AuthConfig implements OnDestroy {
const clockSkewInMSec = this.getClockSkewInMsec(); // (this.getClockSkewInMsec() || 600) * 1000;

if (
issuedAtMSec - clockSkewInMSec >= now ||
expiresAtMSec + clockSkewInMSec <= now
issuedAtMSec >= now - clockSkewInMSec ||
Copy link

@vincent-petit vincent-petit Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sign of the clockSkew is wrong when comparing with the issuedAt.
In the case when the Identity Provider server clock is the same as the the browser clock, the issuedAt of a new/refreshed access_token/id_token will be very close to the server time.
Reducing the browser time with the clock skew, it will lead to the "token has expired" error each time, no?

Suggested change
issuedAtMSec >= now - clockSkewInMSec ||
issuedAtMSec >= now + clockSkewInMSec ||

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right. The original code is correct here.

Copy link

@vincent-petit vincent-petit Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code was correct only for issuedAt attribute.
The updates you suggest on the expiresAt seem correct to me

@tobias-trame tobias-trame deleted the fix-clock-skew branch September 29, 2022 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clock skew is in wrong direction
4 participants