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: Invalidate the SA's AccessToken when createScoped() is called #1489

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented Aug 29, 2024

See b/353616562 for more information. This seems to be a very similar issue to #1387.

Invalidating the Access Token via setAccessToken(null) from createScoped(...) should not break any existing workflows. Copying the existing Access Token while modifying the interval fields that make up the Credentials is a regression in behavior that was introduced in v1.14.0 of the Auth-Library.

Example flow for the current state (pseudocode):

Credentials credentials = GoogleCredentials.getADC();
Storage.create(..., credentials) // Access Token is retrieved with the scopes from ADC
System.out.println(credentials.getAccessToken()) // This is a valid access token with the ADC's scopes

credentials = credentials.createScoped("{NEW SCOPES}");
Storage.update(..., credentials) // Access Token is cached and re-used, but the scopes are different
System.out.println(credentials.getAccessToken()) // This is an invalid access token as it still has the ADC's scopes

Without this change, users would be recieving a 403 Access Denied error message. This change to invalidate the access token would force the auth library to retrieve a new valid access token.

Example flow for the new state (pseudocode):

Credentials credentials = GoogleCredentials.getADC();
Storage.create(..., credentials) // Access Token is retrieved with the scopes from ADC
System.out.println(credentials.getAccessToken()) // This is a valid access token with the ADC's scopes

credentials = credentials.createScoped("{NEW SCOPES}");
Storage.update(..., credentials) // Access Token is retrieved with the new scopes
System.out.println(credentials.getAccessToken()) // This is an valid access token as it has the new scopes

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Aug 29, 2024
@lqiu96 lqiu96 requested a review from zhumin8 August 30, 2024 16:42
@lqiu96 lqiu96 marked this pull request as ready for review August 30, 2024 16:42
@lqiu96 lqiu96 requested a review from a team as a code owner August 30, 2024 16:42
Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

LGTM.
Just a small comment with the test.

@lqiu96 lqiu96 requested a review from a team as a code owner September 3, 2024 20:35
Copy link

sonarqubecloud bot commented Sep 3, 2024

@lqiu96 lqiu96 merged commit f26fee7 into main Sep 3, 2024
18 checks passed
@lqiu96 lqiu96 deleted the invalidate-accessToken-SA branch September 3, 2024 20:48
zhumin8 added a commit that referenced this pull request Oct 18, 2024
…verse domain (#1528)

for context: b/340602527

Changes in this pr:
- Override `getUniverseDomain()` to grab source credentials’s universe domain (UD) by default. Always use source credentials UD, not explicit provided UD. (In current design, impersonated credentials may not have universe domain in the outer layer. relay on UD from source credential. This may change in future)
- Fix `isDefaultUniverseDomain()` in `GoogleCredentials` to account for `getUniverseDomain()` overrides in child classes.
- In refreshAccessToken(), use endpoint url pattern to account for TPC case.
  - note that I choose to bypass this refreshIfExpired step because it wrongly steps into code path meant only for OAuth2 token request (GDU flow). Filed #1534 to address this separately. But for GDU flow here, this refresh step is redundant because the SSJ will get re-generated at [initialize request](https://github.com/googleapis/google-auth-library-java/blob/a987ecd06fd25a0048cdb3da6d1df4d029d85d79/oauth2_http/java/com/google/auth/oauth2/ImpersonatedCredentials.java#L558). Also skip this step for SA GDU with SSJ flow.
- Throw IllegalStateException if UD is explicitly set (with parent class setter) and not matching source credential's UD

- Fix toBuilder() to invoke super, and fix related issue with createScoped. (see #1489, #1428); Also fix equals() to compare super first.


Not in this pr: 
- idtoken and signBlob endpoint changes are out-of-scope for this pr, will raise separate pr for it.

sa-to-sa impersonation is successfully E2E tested for TPC usage according to [go/prptst-testing-service-account-impersonation](http://goto.google.com/prptst-testing-service-account-impersonation).



---------

Co-authored-by: Blake Li <blakeli@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants