Skip to content

Added option clockSkewInSeconds to allow setting clock_skew_in_seconds parameter for token verification #625

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

Closed
wants to merge 4 commits into from

Conversation

fschaeck
Copy link

Adding the optional parameter clock_skew_in_seconds=60 to the call to google.oauth2.id_token.verify_token now allows for the token-issuing server's clock to be off by up to a minute without the token becoming invalid due to a 'issued-at-time' timestamp that is in the future.

Discussion

Issue #624

Testing

No additional tests required.

API Changes

n/a

Adding the optional parameter clock_skew_in_seconds=60 to the call to google.oauth2.id_token.verify_token now allows for the token-issuing server's clock to be off by up to a minute without the token becoming invalid due to a 'issued-at-time' timestamp that is in the future.
@google-cla
Copy link

google-cla bot commented Jul 14, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@lahirumaramba lahirumaramba self-assigned this Jul 14, 2022
@DanielJerrehian
Copy link

Hi, I wanted to follow up on this issue and ask when it will be merged?

This option value is used for the token verification instead of the fixed 60 seconds from
the earlier commit.

This way, the user of firebase_admin can decide if he/she wants to set that value or not.
Also all existing uses of firebase_admin won't suddenly change behaviour, since if the
option is not specified, it's default of 0 is equivalent to what was used before the
introduction of the new option.
@fschaeck fschaeck changed the title Added clock_skew_in_seconds=60 to token verification Added option clockSkewInSeconds to allow setting clock_skew_in_seconds parameter for token verification Aug 6, 2022
@fschaeck
Copy link
Author

fschaeck commented Aug 6, 2022

I changed the pull request a little. The parameter clock_skew_in_seconds for the Google token verification API call is no longer hard-coded with 60 seconds but can be set as an App option. If that option is not set, a default of 0 is used.

This makes all uses of firebase_admin work the same way as before. So the change won't introduce a change of behaviour for those existing applications. But users that need the clock skew setting can add an option to their App and overwrite the default of 0 that way.

I think that is the better way to introduce using the clock_skew_in_seconds parameter of the Google API.

Copy link

@timur737 timur737 left a comment

Choose a reason for hiding this comment

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

lets merge this changes
many users in stuck
because of this bug

Copy link

@DanielJerrehian DanielJerrehian left a comment

Choose a reason for hiding this comment

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

A default clock skew is set to 0 seconds and the server's clock is allowed to be off by up to 60 seconds

@Nakachi-S
Copy link

Why is this PR not merge?
I am facing this problem too.

@lahirumaramba
Copy link
Member

lahirumaramba commented Oct 19, 2022

Thank you @fschaeck for your contribution! The changes to App options require an internal API review and might take a while.

Hi, @prameshj, @renkelvin let me know your thoughts on this, I can initiate the internal changes. Thank you!

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you @fschaeck for your contribution! This looks good to me, but as an alternative, what do you think about adding clock_skew_in_seconds =0 to verify_id_token() and verify_session_cookie as an optional parameter?

def verify_id_token(id_token, app=None, check_revoked=False):

@Yudai-Saito
Copy link

Yudai-Saito commented Nov 3, 2022

@lahirumaramba @fschaeck
Hello.
I hope this PR will be merged because I had the same problem with the same bug.

By the way, we have a similar bug with this method.

auth.verify_session_cookie(session, check_revoked=True)

I solved it this way.

session_cookie_request = requests.Request()
CERTS_URL = "https://www.googleapis.com/identitytoolkit/v3/relyingparty/publicKeys"
id_token.verify_token(session_token=session, request=session_cookie_request, certs_url=CERTS_URL, clock_skew_in_seconds=10)

Copy link

@Yudai-Saito Yudai-Saito left a comment

Choose a reason for hiding this comment

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

LGTM!

@ryanlinnane
Copy link

@fschaeck are you able to merge this now? Subscribing.

@smartexpert
Copy link

appreciate if this could be merged

@lahirumaramba
Copy link
Member

lahirumaramba commented Nov 29, 2022

Thank you for your patience everyone. We just completed the internal API review and we think a better approach would be to add a new optional clock_skew_seconds parameter to verify_id_token() and verify_session_cookie() APIs.

API:

def verify_id_token(id_token, app=None, check_revoked=False, clock_skew_seconds=0):

def verify_session_cookie(session_cookie, check_revoked=False, app=None, clock_skew_seconds=0):

Usage:

import firebase_admin
from firebase_admin import auth

app = firebase_admin.initialize_app()

decoded_token = auth.verify_id_token(id_token, clock_skew_seconds=60)
decoded_claims = auth.verify_session_cookie(session_cookie, check_revoked=True, clock_skew_seconds=10)

@fschaeck would you be able to make the changes? Thank you!

@barel-mishal
Copy link

barel-mishal commented Dec 2, 2022

Hi mean while this need to be changed what the work around? could not figure out yet how to skew my clock with node js

@ryanlinnane
Copy link

Hi mean while this need to be changed what the work around? could not figure out yet how to skew my clock with node js

Fork the fix and install the git repo with pip works for now.

@fschaeck
Copy link
Author

fschaeck commented Dec 3, 2022

@fschaeck would you be able to make the changes? Thank you!

@lahirumaramba

You do realize, that this means to change the signatures of methods TokenVerifier.verfify_id_token, TokenVerifier.verify_session_cookie and _JWTVerifier.verify from firebase_admin/_token_gen.py by adding the optional parameter clock_skew_seconds there as well, right?

Since TokenVerifier and _JWTVerifier are classes (doubly) abstracting the token verification and I was not able to assess, if there are any other implementations that might be used here, I chose the way of the additional app property instead of changing the signature of those methods, because other such abstracting classes may not understand the additional parameter and then apps using this optional parameter all the sudden stop working.

If you okay THOSE change as well, I will change this pull request right the way!

@beef-stek
Copy link

verify_token

Can you please go into more detail on your fix? What class is id_token? Thanks.

This bug has been annoying as it would happen randomly. Please merge this as soon as possible! THANKS GOOGLE.

@fschaeck
Copy link
Author

@lahirumaramba Any news here?

@lahirumaramba
Copy link
Member

lahirumaramba commented Dec 13, 2022

You do realize, that this means to change the signatures of methods TokenVerifier.verfify_id_token, TokenVerifier.verify_session_cookie and _JWTVerifier.verify from firebase_admin/_token_gen.py by adding the optional parameter clock_skew_seconds there as well, right?

Since TokenVerifier and _JWTVerifier are classes (doubly) abstracting the token verification and I was not able to assess, if there are any other implementations that might be used here, I chose the way of the additional app property instead of changing the signature of those methods, because other such abstracting classes may not understand the additional parameter and then apps using this optional parameter all the sudden stop working.

If you okay THOSE change as well, I will change this pull request right the way!

Hi @fschaeck thank you for being so thoughtful about this change. Since _JWTVerifier and TokenVerifier are internal classes and not exposed in the public API surface, we are safe to change the method signature by adding the optional parameter clock_skew_seconds. Please add unit tests (and integration tests) where possible. Thanks!

@karkir0003
Copy link

karkir0003 commented Jan 6, 2023

@lahirumaramba when will this pr get reviewed/approved to be merged? this change would be so nice to have for firebase

@theobouwman
Copy link

Any updates on this? Or how to avoid it?

stillmatic added a commit to stillmatic/firebase-admin-python that referenced this pull request Aug 23, 2023
per feedback in firebase#625 (comment)

adds unit and integration tests as well. unit tests and lint pass.
stillmatic added a commit to stillmatic/firebase-admin-python that referenced this pull request Sep 26, 2023
per feedback in firebase#625 (comment)

adds unit and integration tests as well. unit tests and lint pass.
@anroopak anroopak mentioned this pull request Oct 3, 2023
stillmatic added a commit to stillmatic/firebase-admin-python that referenced this pull request Oct 12, 2023
per feedback in firebase#625 (comment)

adds unit and integration tests as well. unit tests and lint pass.
jonathanedey pushed a commit that referenced this pull request Oct 26, 2023
* feat: add clockSkewSeconds

per feedback in #625 (comment)

adds unit and integration tests as well. unit tests and lint pass.

* fix: test

* chore: version bump for testing

* chore: address CR

* fix:lint

* chore: address CR

* chore: remove test

* fix: remove more tests

* chore: address CR
@jonathanedey
Copy link
Contributor

Addressed in #714

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.