openid / AppAuth-Android

Android client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
https://openid.github.io/AppAuth-Android
Apache License 2.0
2.82k stars 881 forks source link

Parameter for setting an allowed clock skew / time difference #830

Open sezabass opened 2 years ago

sezabass commented 2 years ago

Feature Request

Motivation

We use AppAuth on a released app, to provide authentication to its users. Some users complain that they just cannot log in, and it often happens because they do not have their phones' clocks synchronizing automatically. They typically have a client time that is at least a little different from the authentication server's, and the AppAuth plugin returns an error.

Description

A parameter that would now be available for setting a more (or less) permissive allowed time for OpenID Connect Core Section 3.1.3.7 rules #9 and #10. e.g:


        // OpenID Connect Core Section 3.1.3.7. rule #9
        // Validates that the current time is before the expiry time.
        Long allowedFutureSkewParam = null; // <-- this would come as a parameter

        Long nowInSeconds = clock.getCurrentTimeMillis() / MILLIS_PER_SECOND;
        // The next two lines would be modified
        Long allowedFutureSkew = allowedFutureSkewParam != null ? allowedFutureSkewParam : 0L;
        if (nowInSeconds - allowedFutureSkew > this.expiration) {
            throw AuthorizationException.fromTemplate(GeneralErrors.ID_TOKEN_VALIDATION_ERROR,
                new IdTokenException("ID Token expired"));
        }

        // OpenID Connect Core Section 3.1.3.7. rule #10
        // Validates that the issued at time is not more than +/- 10 minutes on the current
        // time.
        Long allowedSkewParam = null; // <-- this would come as a parameter

        // The next two lines would be modified
        Long allowedSkew = allowedSkewParam != null ? allowedSkewParam : TEN_MINUTES_IN_SECONDS;
        if (Math.abs(nowInSeconds - this.issuedAt) > allowedSkew) {

            throw AuthorizationException.fromTemplate(GeneralErrors.ID_TOKEN_VALIDATION_ERROR,
                new IdTokenException("Issued at time is more than 10 minutes "
                    + "before or after the current time"));
        }

One thing that is important to mention is that on OpenId's specs, the time is not determined, and in our case we wanted to make it a bit more permissive than TEN_MINUTES_IN_SECONDS.

It will not break anything as the default behavior would be to keep the current behavior. The only drawback I can see here is maybe less security measures - but since this is an opt-in measure, we can assume that one that uses this knows what is being done.

Alternatives or Workarounds

We made this modification locally on a forked repo.
We wanted to know if a PR making this change would be accepted by the lib's maintainers and the community.

MahmoudOsamaAli commented 2 years ago

i also need this feature as i faced this issue recently, which can force me to change using this lib for authentication

MahmoudOsamaAli commented 2 years ago

please consider this enhancement

ALTELMA commented 1 year ago

Oh, I think I am the only one who facing this problem. Thanks for more information to understand the exact problem.

However, We just need this one, too.

stantronic commented 1 year ago

Also having this issue. remaining on an older version of this lib until it is fixed.

PauloHFS commented 1 year ago

@stantronic what version are you using? I'm having this same issue.

brighthr-stanton commented 1 year ago

Hi Still on 0.7.1 here. Looks like there haven't been any releases since 2021 though.

brighthr-stanton commented 1 year ago

@PauloHFS we're stick on 0.7.1 until this gets fixed. We cant count on users having the correct time set on their phone

PauloHFS commented 1 year ago

@brighthr-stanton I follow your choice and also fixed the version and we are in it waiting for the correction. I intend to get involved with this problem and if possible bring a solution in a PR when I have time. This problem affects the IOS version as well, in addition to affecting libraries that port both versions to javascript such as react-native-app-auth, which is very used.

brighthr-stanton commented 1 year ago

Great. Let us know if and when you do that PR so we can vote on it :)

KamenGoranchev commented 1 year ago

I am facing the same issue and it is not only at id token verification but also at the check if the access token is still valid. One possible solution to how this can be solved in a generic manner is to allow injecting a custom Clock instance. I know having the server time always sync on client side so I could potentially create such clock

Shaun0703 commented 1 year ago

It's me sep1972 you do know my account is still hacked do you??


From: Kamen Goranchev @.> Sent: 25 July 2023 21:47 To: openid/AppAuth-Android @.> Cc: Subscribed @.***> Subject: Re: [openid/AppAuth-Android] Parameter for setting an allowed clock skew / time difference (Issue #830)

I am facing the same issue and it is not only at id token verification but also at the check if the access token is still valid. One possible solution to how this can be solved in a generic manner is to allow injecting a custom Clock instance. I know having the server time always sync on client side so I could potentially create such clock

— Reply to this email directly, view it on GitHubhttps://github.com/openid/AppAuth-Android/issues/830#issuecomment-1650537739, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AWZYHTITCTRRLNSQY7RMJK3XSAWEZANCNFSM5XLLA7PQ. You are receiving this because you are subscribed to this thread.Message ID: @.***>

Mardaneus86 commented 9 months ago

I created a fork where it is possible to change the allowed time skew for the ID token validation. As an additional change I also added an option to completely disable ID token issue time validation. The change itself follows a similar pattern to what was done for #662.

See https://github.com/Mardaneus86/AppAuth-Android/tree/idtoken-time-skew for the changes I made. Would this have any chance to get accepted as a PR? If so, I'm happy to create one for this.