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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

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

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

@fschaeck
Added clock_skew_in_seconds to token verification
e38f71a 
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?

@fschaeck
Added new app option clockSkewInSeconds
4d14005 
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.

timur737
timur737 approved these changes Aug 28, 2022
View reviewed changes
Copy link

@timur737 timur737 left a comment •
edited
Loading

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

DanielJerrehian
DanielJerrehian approved these changes Aug 31, 2022
View reviewed changes
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 •
edited
Loading

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!

lahirumaramba
lahirumaramba reviewed Oct 21, 2022
View reviewed changes
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?

firebase-admin-python/firebase_admin/auth.py

Line 194 in b9e95e8

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

@Yudai-Saito
Copy link

Yudai-Saito commented Nov 3, 2022 •
edited
Loading

@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)

Yudai-Saito
Yudai-Saito approved these changes Nov 3, 2022
View reviewed changes
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 •
edited
Loading

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 •
edited
Loading

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.