feat(auth): add universe domain support to credentials/impersonate #10953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
quartzmo
commented
Oct 4, 2024
quartzmo
commented
- fix: return err if both Client and Credentials are set in CredentialsOptions
- fix: propagate Credentials universe domain in httptransport.AddAuthorizationMiddleware
* fix: return err if both Client and Credentials are set in CredentialsOptions * fix: propagate Credentials universe domain in httptransport.AddAuthorizationMiddleware
codyoss
reviewed
Oct 7, 2024
| // If a subject is specified a domain-wide delegation auth-flow is initiated | ||
| // to impersonate as the provided subject (user). | ||
| if opts.Subject != "" { | ||
| if !opts.isUniverseDomainGDU() { | ||
| gdu, err := isUniverseDomainGDU(universeDomainProvider) |
There was a problem hiding this comment.
Can this check be moved to the impl for the token provider? Having this code here puts this check at construction time vs usage-time.
| // This is the universe domain configured for the credentials, which will be | ||
| // used in endpoint(s), and compared to the universe domain that is separately | ||
| // configured for the client. | ||
| func resolveUniverseDomainProvider(opts *CredentialsOptions, creds *auth.Credentials) auth.CredentialsPropertyProvider { |
There was a problem hiding this comment.
This seems like common logic that could be broadened and used consistently all over, similar to what we do for quotaproject. Could we do something similar here: https:/googleapis/google-cloud-go/blob/main/auth/internal/internal.go#L100
| @@ -170,10 +171,14 @@ func AddAuthorizationMiddleware(client *http.Client, creds *auth.Credentials) er | |||
| base = http.DefaultTransport | |||
| } | |||
| } | |||
| clientUniverseDomain, err := creds.UniverseDomain(context.Background()) | |||
There was a problem hiding this comment.
This now potentially can hard pull MDS, or at least could in the future. Can this be delayed?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.