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(auth): add universe domain support to idtoken #11059

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

quartzmo
Copy link
Member

@quartzmo quartzmo commented Oct 29, 2024

Summary

IdToken flow can be done in two ways: 1) OAuth2 server calls or 2) IAM service calls. The majority of implementations went with the OAuth2 server calls. In non-GDU environments, however, there is no OAuth2 server, so the IAM implementation must be used. This is enabled as follows:

  • Move impersonatedIDTokenProvider.Token in auth/credentials/impersonate/idtoken.go to IDTokenIAMOptions.Token in auth/credentials/internal/impersonate/idtoken.go to enable reuse in auth/credentials/idtoken/iam.go.
  • Move formatIAMServiceAccountName in auth/credentials/impersonate/impersonate.go to FormatIAMServiceAccountResource in auth/internal/internal.go, for reuse.
  • In auth/credentials/idtoken/file.go introduce a conditional (resolveUniverseDomain(f) == internal.DefaultUniverseDomain) which if true sustains the existing behavior (auth.New2LOTokenProvider) and if false introduces the new non-GDU behavior in IDTokenIAMOptions.Token

Note that to use the IAM endpoint in a non-GDU environment, the service account must have the iam.serviceAccountTokenCreator role because the service account is not only the target service account, but also the caller; the Oauth2 endpoint doesn’t need this.

@quartzmo quartzmo force-pushed the auth-idtoken-universe-domain branch from ee34736 to 260aaaa Compare October 30, 2024 16:00
@quartzmo quartzmo force-pushed the auth-idtoken-universe-domain branch from 2b32c34 to 06969bb Compare October 30, 2024 16:02
@quartzmo quartzmo marked this pull request as ready for review October 31, 2024 17:01
@quartzmo quartzmo requested a review from a team as a code owner October 31, 2024 17:01
auth/internal/internal.go Outdated Show resolved Hide resolved
auth/credentials/idtoken/file.go Outdated Show resolved Hide resolved
auth/credentials/idtoken/file.go Outdated Show resolved Hide resolved
auth/credentials/idtoken/file.go Outdated Show resolved Hide resolved
auth/credentials/idtoken/file.go Show resolved Hide resolved
auth/credentials/idtoken/file.go Outdated Show resolved Hide resolved
auth/credentials/impersonate/idtoken.go Outdated Show resolved Hide resolved
auth/credentials/impersonate/idtoken.go Show resolved Hide resolved
auth/credentials/impersonate/user.go Show resolved Hide resolved
@quartzmo quartzmo added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 1, 2024
@quartzmo quartzmo removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 3, 2025
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

LGTM to me overall. Just refactor a little to remove some now unnecessary structs.

// This TokenProvider is primarily intended for use in non-GDU universes, which
// do not have access to the oauth2.googleapis.com/token endpoint, and thus must
// use IAM generateIdToken instead.
type iamIDTokenProvider struct {
Copy link
Member

Choose a reason for hiding this comment

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

I think this struct can go away completely. should now be able to use impersonate.IDTokenIAMOptions directly.

type impersonatedIDTokenProvider struct {
client *http.Client
logger *slog.Logger
client *http.Client
Copy link
Member

Choose a reason for hiding this comment

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

Same with my other comment, this structure should go away in favor of the common code

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.

2 participants