From 10e736401a81f8128b20591cd906e5e1f372fafe Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Fri, 1 Nov 2024 15:58:09 -0600 Subject: [PATCH] auth(fix): remove incorrect validation from impersonate.CredentialsOptions * add credentials validation to idtoken.Options --- auth/credentials/idtoken/idtoken.go | 33 ++++++++++----- auth/credentials/idtoken/idtoken_test.go | 39 ++++++++++++++++++ auth/credentials/impersonate/idtoken.go | 41 ++++++++----------- auth/credentials/impersonate/impersonate.go | 31 +++++--------- .../impersonate/impersonate_test.go | 10 ----- 5 files changed, 91 insertions(+), 63 deletions(-) diff --git a/auth/credentials/idtoken/idtoken.go b/auth/credentials/idtoken/idtoken.go index b66c6551e6ee..a79890be9151 100644 --- a/auth/credentials/idtoken/idtoken.go +++ b/auth/credentials/idtoken/idtoken.go @@ -51,6 +51,12 @@ const ( ComputeTokenFormatFullWithLicense ) +var ( + errMissingOpts = errors.New("idtoken: opts must be provided") + errMissingAudience = errors.New("idtoken: Audience must be provided") + errBothFileAndJSON = errors.New("idtoken: CredentialsFile and CredentialsJSON must not both be provided") +) + // Options for the configuration of creation of an ID token with // [NewCredentials]. type Options struct { @@ -64,14 +70,14 @@ type Options struct { // Optional. CustomClaims map[string]interface{} - // CredentialsFile overrides detection logic and sources a credential file - // from the provided filepath. Optional. + // CredentialsFile sources a JSON credential file from the provided + // filepath. If provided, do not provide CredentialsJSON. Optional. CredentialsFile string - // CredentialsJSON overrides detection logic and uses the JSON bytes as the - // source for the credential. Optional. + // CredentialsJSON sources a JSON credential file from the provided bytes. + // If provided, do not provide CredentialsJSON. Optional. CredentialsJSON []byte // Client configures the underlying client used to make network requests - // when fetching tokens. If provided this should be a fully authenticated + // when fetching tokens. If provided this should be a fully-authenticated // client. Optional. Client *http.Client } @@ -85,17 +91,24 @@ func (o *Options) client() *http.Client { func (o *Options) validate() error { if o == nil { - return errors.New("idtoken: opts must be provided") + return errMissingOpts } if o.Audience == "" { - return errors.New("idtoken: audience must be specified") + return errMissingAudience + } + if o.CredentialsFile != "" && len(o.CredentialsJSON) > 0 { + return errBothFileAndJSON } return nil } -// NewCredentials creates a [cloud.google.com/go/auth.Credentials] that -// returns ID tokens configured by the opts provided. The parameter -// opts.Audience may not be empty. +// NewCredentials creates a [cloud.google.com/go/auth.Credentials] that returns +// ID tokens configured by the opts provided. The parameter opts.Audience must +// not be empty. If both opts.CredentialsFile and opts.CredentialsJSON are +// empty, an attempt will be made to detect credentials from the environment +// (see [cloud.google.com/go/auth/credentials.DetectDefault]). Only service +// account, impersonated service account, external account and Compute +// credentials are supported. func NewCredentials(opts *Options) (*auth.Credentials, error) { if err := opts.validate(); err != nil { return nil, err diff --git a/auth/credentials/idtoken/idtoken_test.go b/auth/credentials/idtoken/idtoken_test.go index bc14b1b5c3a6..e40b2920a544 100644 --- a/auth/credentials/idtoken/idtoken_test.go +++ b/auth/credentials/idtoken/idtoken_test.go @@ -27,6 +27,45 @@ import ( "cloud.google.com/go/auth/internal/credsfile" ) +func TestNewCredentials_Validate(t *testing.T) { + tests := []struct { + name string + opts *Options + wantErr error + }{ + { + name: "missing opts", + wantErr: errMissingOpts, + }, + { + name: "missing audience", + opts: &Options{}, + wantErr: errMissingAudience, + }, + { + name: "both credentials", + opts: &Options{ + Audience: "aud", + CredentialsFile: "creds.json", + CredentialsJSON: []byte{0,1}, + }, + wantErr: errBothFileAndJSON, + }, + } + for _, tt := range tests { + name := tt.name + t.Run(name, func(t *testing.T) { + err := tt.opts.validate() + if err == nil { + t.Fatalf("error expected: %s", tt.wantErr) + } + if err != tt.wantErr { + t.Errorf("got %v, want %v", err, tt.wantErr) + } + }) + } +} + func TestNewCredentials_ServiceAccount(t *testing.T) { wantTok, _ := createRS256JWT(t) b, err := os.ReadFile("../../internal/testdata/sa.json") diff --git a/auth/credentials/impersonate/idtoken.go b/auth/credentials/impersonate/idtoken.go index e51bee7d8764..9d44c91cb0d9 100644 --- a/auth/credentials/impersonate/idtoken.go +++ b/auth/credentials/impersonate/idtoken.go @@ -47,13 +47,13 @@ type IDTokenOptions struct { // chain. Optional. Delegates []string - // Credentials used to fetch the ID token. If not provided, and a Client is - // also not provided, base credentials will try to be detected from the - // environment. Optional. + // Credentials used in generating the impersonated ID token. If empty, an + // attempt will be made to detect credentials from the environment (see + // [cloud.google.com/go/auth/credentials.DetectDefault]). Optional. Credentials *auth.Credentials // Client configures the underlying client used to make network requests - // when fetching tokens. If provided the client should provide it's own - // base credentials at call time. Optional. + // when fetching tokens. If provided this should be a fully-authenticated + // client. Optional. Client *http.Client } @@ -83,17 +83,20 @@ func NewIDTokenCredentials(opts *IDTokenOptions) (*auth.Credentials, error) { if err := opts.validate(); err != nil { return nil, err } - var client *http.Client - var creds *auth.Credentials - if opts.Client == nil && opts.Credentials == nil { + + client := opts.Client + creds := opts.Credentials + if client == nil { var err error - // TODO: test not signed jwt more - creds, err = credentials.DetectDefault(&credentials.DetectOptions{ - Scopes: []string{defaultScope}, - UseSelfSignedJWT: true, - }) - if err != nil { - return nil, err + if creds == nil { + // TODO: test not signed jwt more + creds, err = credentials.DetectDefault(&credentials.DetectOptions{ + Scopes: []string{defaultScope}, + UseSelfSignedJWT: true, + }) + if err != nil { + return nil, err + } } client, err = httptransport.NewClient(&httptransport.Options{ Credentials: creds, @@ -101,14 +104,6 @@ func NewIDTokenCredentials(opts *IDTokenOptions) (*auth.Credentials, error) { if err != nil { return nil, err } - } else if opts.Client == nil { - creds = opts.Credentials - client = internal.DefaultClient() - if err := httptransport.AddAuthorizationMiddleware(client, opts.Credentials); err != nil { - return nil, err - } - } else { - client = opts.Client } itp := impersonatedIDTokenProvider{ diff --git a/auth/credentials/impersonate/impersonate.go b/auth/credentials/impersonate/impersonate.go index df306057b49c..66ee03af9959 100644 --- a/auth/credentials/impersonate/impersonate.go +++ b/auth/credentials/impersonate/impersonate.go @@ -38,7 +38,6 @@ var ( errMissingTargetPrincipal = errors.New("impersonate: target service account must be provided") errMissingScopes = errors.New("impersonate: scopes must be provided") errLifetimeOverMax = errors.New("impersonate: max lifetime is 12 hours") - errClientAndCredentials = errors.New("impersonate: client and credentials must not both be provided") errUniverseNotSupportedDomainWideDelegation = errors.New("impersonate: service account user is configured for the credential. " + "Domain-wide delegation is not supported in universes other than googleapis.com") ) @@ -64,11 +63,11 @@ func NewCredentials(opts *CredentialsOptions) (*auth.Credentials, error) { isStaticToken = true } - var client *http.Client - var creds *auth.Credentials - if opts.Client == nil { + client := opts.Client + creds := opts.Credentials + if client == nil { var err error - if opts.Credentials == nil { + if creds == nil { creds, err = credentials.DetectDefault(&credentials.DetectOptions{ Scopes: []string{defaultScope}, UseSelfSignedJWT: true, @@ -76,8 +75,6 @@ func NewCredentials(opts *CredentialsOptions) (*auth.Credentials, error) { if err != nil { return nil, err } - } else { - creds = opts.Credentials } client, err = httptransport.NewClient(&httptransport.Options{ Credentials: creds, @@ -86,8 +83,6 @@ func NewCredentials(opts *CredentialsOptions) (*auth.Credentials, error) { if err != nil { return nil, err } - } else { - client = opts.Client } universeDomainProvider := resolveUniverseDomainProvider(creds) @@ -163,18 +158,17 @@ type CredentialsOptions struct { // wide delegation. Optional. Subject string - // Credentials is the provider of the credentials used to fetch the ID - // token. If not provided, and a Client is also not provided, credentials - // will try to be detected from the environment. Optional. + // Credentials used in generating the impersonated token. If empty, an + // attempt will be made to detect credentials from the environment (see + // [cloud.google.com/go/auth/credentials.DetectDefault]). Optional. Credentials *auth.Credentials // Client configures the underlying client used to make network requests - // when fetching tokens. If provided the client should provide its own - // credentials at call time. Optional. + // when fetching tokens. If provided this should be a fully-authenticated + // client. Optional. Client *http.Client // UniverseDomain is the default service domain for a given Cloud universe. - // The default value is "googleapis.com". This is the universe domain - // configured for the client, which will be compared to the universe domain - // that is separately configured for the credentials. Optional. + // This field has no default value, and only if provided will it be used to + // verify the universe domain from the credentials. Optional. UniverseDomain string } @@ -191,9 +185,6 @@ func (o *CredentialsOptions) validate() error { if o.Lifetime.Hours() > 12 { return errLifetimeOverMax } - if o.Client != nil && o.Credentials != nil { - return errClientAndCredentials - } return nil } diff --git a/auth/credentials/impersonate/impersonate_test.go b/auth/credentials/impersonate/impersonate_test.go index 777b3a1e02c6..df9cade64ab6 100644 --- a/auth/credentials/impersonate/impersonate_test.go +++ b/auth/credentials/impersonate/impersonate_test.go @@ -57,16 +57,6 @@ func TestNewCredentials_serviceAccount(t *testing.T) { }, wantErr: errLifetimeOverMax, }, - { - name: "credentials and client", - config: CredentialsOptions{ - TargetPrincipal: "foo@project-id.iam.gserviceaccount.com", - Scopes: []string{"scope"}, - Client: &http.Client{}, - Credentials: staticCredentials("googleapis.com"), - }, - wantErr: errClientAndCredentials, - }, { name: "works", config: CredentialsOptions{