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: ID Token Caching for GCECredentials #510

Merged
merged 3 commits into from
Jan 3, 2024
Merged

Conversation

yash30201
Copy link
Contributor

@yash30201 yash30201 commented Dec 27, 2023

Fixes #503

Copy link
Contributor Author

@yash30201 yash30201 left a comment

Choose a reason for hiding this comment

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

We don't need to check validity of cached ID tokens as the cache config has a lifetime of 1500 seconds whereas id tokens have lifetime of 3600 seconds.

Copy link

@Daniel-I-Am Daniel-I-Am left a comment

Choose a reason for hiding this comment

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

Sorry about the delay on my side, I didn't get the chance to fully test my proposed changes due to the Holidays. Just as I was about to open a PR, I see you've applied the same changes. FWIW, looks good to me 🙂

I installed this version using composer require google/auth:dev-fix-token-caching-issue-503 to check it out where I first discovered the issue and that seems to no longer be causing an issue.

* Implement logic
* Test logic
@yash30201 yash30201 force-pushed the fix-token-caching-issue-503 branch from 8f1dbd3 to 6765476 Compare December 29, 2023 09:24
@yash30201 yash30201 marked this pull request as ready for review December 29, 2023 09:24
@yash30201 yash30201 requested a review from a team as a code owner December 29, 2023 09:24
@yash30201 yash30201 requested a review from bshaffer December 29, 2023 09:26
@bshaffer
Copy link
Contributor

We don't need to check validity of cached ID tokens as the cache config has a lifetime of 1500 seconds whereas id tokens have lifetime of 3600 seconds.

Don't forget that the cache config is a user supplied value and can be changed.

@yash30201
Copy link
Contributor Author

We don't need to check validity of cached ID tokens as the cache config has a lifetime of 1500 seconds whereas id tokens have lifetime of 3600 seconds.

Don't forget that the cache config is a user supplied value and can be changed.

Yes this seems concerning. We should either have a id token validity check or an ability to clear the cache. Other creds id token caching also suffers from same problem as we have been expecting that id tokens have null lifetime in our library.

https://github.com/googleapis/google-auth-library-php/blob/main/src/FetchAuthTokenCache.php#L276-L280

@bshaffer I have a couple of ideas on how to solve them. Shall we take that up on another PR? (Since it's scope would be more that just fixing GCECreds)

Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

This looks great! Just two minor suggestions

src/Credentials/GCECredentials.php Outdated Show resolved Hide resolved
src/Credentials/GCECredentials.php Outdated Show resolved Hide resolved
@bshaffer bshaffer added the release blocking Required feature/issue must be fixed prior to next release. label Jan 2, 2024
@yash30201 yash30201 force-pushed the fix-token-caching-issue-503 branch from c22fc3a to 57ce568 Compare January 3, 2024 14:31
@bshaffer bshaffer merged commit 3222f9e into main Jan 3, 2024
13 checks passed
@bshaffer bshaffer deleted the fix-token-caching-issue-503 branch January 3, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release blocking Required feature/issue must be fixed prior to next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: OIDC identity tokens no longer cached when using AuthTokenMiddleware/FetchAuthTokenCache
3 participants