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

feat(rest): support impersonated ADC #14815

Merged

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Oct 31, 2024

Part of the work for #12497

Support ADC impersonation over REST.

We need to feed the source_credentials back into the thing that parses the ADC json, then fit that into our existing impersonation code.

Testing

I am not sure how to turn this into an integration test. If I figure it out, I will add one in a future PR.

To test locally I...

  • created a SA (1) with permissions to read a GCS bucket
  • created a SA (2) with roles/ServiceAccountTokenCreator on (1)
    • Note that (2) does not have permission to read from GCS
  • created ADC with impersonation
    • the source_credentials are (2)
    • service_account_impersonation_url points to (1)
  • successfully ran the GCS quickstart!

This change is Reviewable

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.27%. Comparing base (a41a136) to head (e925c95).
Report is 21 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14815   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files        2319     2319           
  Lines      208410   208481   +71     
=======================================
+ Hits       194390   194459   +69     
- Misses      14020    14022    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dbolduc dbolduc marked this pull request as ready for review October 31, 2024 17:55
@dbolduc dbolduc requested a review from a team as a code owner October 31, 2024 17:55
@cuiy0006
Copy link
Collaborator

If the iamcredentials url created in MinimalIamCredentialsRestStub is different from service_account_impersonation_url, which one should we use?

Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dbolduc)


google/cloud/internal/oauth2_google_credentials.cc line 71 at r1 (raw file):

  }
  if (cred_type == "impersonated_service_account") {
    auto info = ParseImpersonatedServiceAccountCredentials(contents, path);

I would have thought that info would contain the service_account_impersonation_url and that it would be used as part of the request.

@dbolduc dbolduc added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Nov 7, 2024
@dbolduc
Copy link
Member Author

dbolduc commented Nov 13, 2024

If the iamcredentials url created in MinimalIamCredentialsRestStub is different from service_account_impersonation_url, which one should we use?

Ultimately, we want to use service_account_impersonation_url.

What we have will create the service_account_impersonation_url unless

  • it is using :generateIdToken instead of :generateAccessToken
  • the base credentials universe domain differs from the universe domain in service_account_impersonation_url. (I don't think this can happen, but I am not 100% sure).

I would have thought that info would contain the service_account_impersonation_url and that it would be used as part of the request.

I parsed out the service account, because that is what we (currently) feed into the thing that makes the base credentials. I looked into plumbing the full url but it got annoying quickly.

As discussed in the team meeting, I think we should go ahead with this PR (adding functionality) then look into revising the plumbing as part of the ID Token flow work.

Copy link
Member

@scotthart scotthart left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dbolduc)

@dbolduc dbolduc merged commit 12acccd into googleapis:main Nov 14, 2024
75 of 76 checks passed
@dbolduc dbolduc deleted the adc-impersonated-service-account-rest branch November 14, 2024 16:41
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.

3 participants