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

auth(fix): remove incorrect validation from impersonate.CredentialsOptions #11077

Merged
merged 2 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions auth/credentials/idtoken/idtoken.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand All @@ -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
Expand Down
39 changes: 39 additions & 0 deletions auth/credentials/idtoken/idtoken_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
41 changes: 18 additions & 23 deletions auth/credentials/impersonate/idtoken.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -83,32 +83,27 @@ 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,
})
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{
Expand Down
31 changes: 11 additions & 20 deletions auth/credentials/impersonate/impersonate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)
Expand All @@ -64,20 +63,18 @@ 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,
})
if err != nil {
return nil, err
}
} else {
creds = opts.Credentials
}
client, err = httptransport.NewClient(&httptransport.Options{
Credentials: creds,
Expand All @@ -86,8 +83,6 @@ func NewCredentials(opts *CredentialsOptions) (*auth.Credentials, error) {
if err != nil {
return nil, err
}
} else {
client = opts.Client
}

universeDomainProvider := resolveUniverseDomainProvider(creds)
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
10 changes: 0 additions & 10 deletions auth/credentials/impersonate/impersonate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading