-
Notifications
You must be signed in to change notification settings - Fork 332
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
Conversation
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.
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. |
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.
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. |
There was a problem hiding this 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
There was a problem hiding this 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
Why is this PR not merge? |
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! |
There was a problem hiding this 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): |
@lahirumaramba @fschaeck 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@fschaeck are you able to merge this now? Subscribing. |
appreciate if this could be merged |
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 API:
Usage:
@fschaeck would you be able to make the changes? Thank you! |
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. |
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! |
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. |
@lahirumaramba Any news here? |
Hi @fschaeck thank you for being so thoughtful about this change. Since |
@lahirumaramba when will this pr get reviewed/approved to be merged? this change would be so nice to have for firebase |
Any updates on this? Or how to avoid it? |
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
per feedback in firebase#625 (comment) adds unit and integration tests as well. unit tests and lint pass.
* 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
Addressed in #714 |
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