From c82d0e16dcd7c2b3907866fc2124725806a3ed67 Mon Sep 17 00:00:00 2001 From: aeitzman Date: Wed, 15 Feb 2023 00:24:46 +0000 Subject: [PATCH] google/internal/externalaccount: Removed URL validation for google URLs in ADC files Removes URL validation for token_url, service_account_impersonation_url to allow for TPC urls and adds line to the docs to warn users. See https://github.com/googleapis/google-auth-library-nodejs/pull/1517 for same change in node.js library. Change-Id: I85fa67ee0b99deed2adb75668a1b5501851c499c GitHub-Last-Rev: 15d7759884817d0f835768bdb5e5b3fa86e8fdbf GitHub-Pull-Request: golang/oauth2#627 Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/465696 Reviewed-by: Cody Oss Run-TryBot: Cody Oss TryBot-Result: Gopher Robot Reviewed-by: Leo Siracusa --- google/doc.go | 6 + .../externalaccount/basecredentials.go | 32 +---- .../externalaccount/basecredentials_test.go | 135 ------------------ .../externalaccount/impersonate_test.go | 4 +- 4 files changed, 9 insertions(+), 168 deletions(-) diff --git a/google/doc.go b/google/doc.go index b3e7bc85..8a3349fc 100644 --- a/google/doc.go +++ b/google/doc.go @@ -57,6 +57,11 @@ // executable-sourced credentials), please check out: // https://cloud.google.com/iam/docs/using-workload-identity-federation#oidc // +// Note that this library does not perform any validation on the token_url, token_info_url, +// or service_account_impersonation_url fields of the credential configuration. +// It is not recommended to use a credential configuration that you did not generate with +// the gcloud CLI unless you verify that the URL fields point to a googleapis.com domain. +// // # Credentials // // The Credentials type represents Google credentials, including Application Default @@ -81,4 +86,5 @@ // same as the one obtained from the oauth2.Config returned from ConfigFromJSON or // JWTConfigFromJSON, but the Credentials may contain additional information // that is useful is some circumstances. +// package google // import "golang.org/x/oauth2/google" diff --git a/google/internal/externalaccount/basecredentials.go b/google/internal/externalaccount/basecredentials.go index 3eab8df7..dcd252a6 100644 --- a/google/internal/externalaccount/basecredentials.go +++ b/google/internal/externalaccount/basecredentials.go @@ -67,22 +67,6 @@ type Config struct { // that include all elements in a given list, in that order. var ( - validTokenURLPatterns = []*regexp.Regexp{ - // The complicated part in the middle matches any number of characters that - // aren't period, spaces, or slashes. - regexp.MustCompile(`(?i)^[^\.\s\/\\]+\.sts\.googleapis\.com$`), - regexp.MustCompile(`(?i)^sts\.googleapis\.com$`), - regexp.MustCompile(`(?i)^sts\.[^\.\s\/\\]+\.googleapis\.com$`), - regexp.MustCompile(`(?i)^[^\.\s\/\\]+-sts\.googleapis\.com$`), - regexp.MustCompile(`(?i)^sts-[^\.\s\/\\]+\.p\.googleapis\.com$`), - } - validImpersonateURLPatterns = []*regexp.Regexp{ - regexp.MustCompile(`^[^\.\s\/\\]+\.iamcredentials\.googleapis\.com$`), - regexp.MustCompile(`^iamcredentials\.googleapis\.com$`), - regexp.MustCompile(`^iamcredentials\.[^\.\s\/\\]+\.googleapis\.com$`), - regexp.MustCompile(`^[^\.\s\/\\]+-iamcredentials\.googleapis\.com$`), - regexp.MustCompile(`^iamcredentials-[^\.\s\/\\]+\.p\.googleapis\.com$`), - } validWorkforceAudiencePattern *regexp.Regexp = regexp.MustCompile(`//iam\.googleapis\.com/locations/[^/]+/workforcePools/`) ) @@ -110,25 +94,13 @@ func validateWorkforceAudience(input string) bool { // TokenSource Returns an external account TokenSource struct. This is to be called by package google to construct a google.Credentials. func (c *Config) TokenSource(ctx context.Context) (oauth2.TokenSource, error) { - return c.tokenSource(ctx, validTokenURLPatterns, validImpersonateURLPatterns, "https") + return c.tokenSource(ctx, "https") } // tokenSource is a private function that's directly called by some of the tests, // because the unit test URLs are mocked, and would otherwise fail the // validity check. -func (c *Config) tokenSource(ctx context.Context, tokenURLValidPats []*regexp.Regexp, impersonateURLValidPats []*regexp.Regexp, scheme string) (oauth2.TokenSource, error) { - valid := validateURL(c.TokenURL, tokenURLValidPats, scheme) - if !valid { - return nil, fmt.Errorf("oauth2/google: invalid TokenURL provided while constructing tokenSource") - } - - if c.ServiceAccountImpersonationURL != "" { - valid := validateURL(c.ServiceAccountImpersonationURL, impersonateURLValidPats, scheme) - if !valid { - return nil, fmt.Errorf("oauth2/google: invalid ServiceAccountImpersonationURL provided while constructing tokenSource") - } - } - +func (c *Config) tokenSource(ctx context.Context, scheme string) (oauth2.TokenSource, error) { if c.WorkforcePoolUserProject != "" { valid := validateWorkforceAudience(c.Audience) if !valid { diff --git a/google/internal/externalaccount/basecredentials_test.go b/google/internal/externalaccount/basecredentials_test.go index 05e0127f..bf6be321 100644 --- a/google/internal/externalaccount/basecredentials_test.go +++ b/google/internal/externalaccount/basecredentials_test.go @@ -9,7 +9,6 @@ import ( "io/ioutil" "net/http" "net/http/httptest" - "strings" "testing" "time" @@ -208,140 +207,6 @@ func TestNonworkforceWithWorkforcePoolUserProject(t *testing.T) { } } -func TestValidateURLTokenURL(t *testing.T) { - var urlValidityTests = []struct { - tokURL string - expectSuccess bool - }{ - {"https://east.sts.googleapis.com", true}, - {"https://sts.googleapis.com", true}, - {"https://sts.asfeasfesef.googleapis.com", true}, - {"https://us-east-1-sts.googleapis.com", true}, - {"https://sts.googleapis.com/your/path/here", true}, - {"https://.sts.googleapis.com", false}, - {"https://badsts.googleapis.com", false}, - {"https://sts.asfe.asfesef.googleapis.com", false}, - {"https://sts..googleapis.com", false}, - {"https://-sts.googleapis.com", false}, - {"https://us-ea.st-1-sts.googleapis.com", false}, - {"https://sts.googleapis.com.evil.com/whatever/path", false}, - {"https://us-eas\\t-1.sts.googleapis.com", false}, - {"https:/us-ea/st-1.sts.googleapis.com", false}, - {"https:/us-east 1.sts.googleapis.com", false}, - {"https://", false}, - {"http://us-east-1.sts.googleapis.com", false}, - {"https://us-east-1.sts.googleapis.comevil.com", false}, - {"https://sts-xyz.p.googleapis.com", true}, - {"https://sts.pgoogleapis.com", false}, - {"https://p.googleapis.com", false}, - {"https://sts.p.com", false}, - {"http://sts.p.googleapis.com", false}, - {"https://xyz-sts.p.googleapis.com", false}, - {"https://sts-xyz.123.p.googleapis.com", false}, - {"https://sts-xyz.p1.googleapis.com", false}, - {"https://sts-xyz.p.foo.com", false}, - {"https://sts-xyz.p.foo.googleapis.com", false}, - } - ctx := context.Background() - for _, tt := range urlValidityTests { - t.Run(" "+tt.tokURL, func(t *testing.T) { // We prepend a space ahead of the test input when outputting for sake of readability. - config := testConfig - config.TokenURL = tt.tokURL - _, err := config.TokenSource(ctx) - - if tt.expectSuccess && err != nil { - t.Errorf("got %v but want nil", err) - } else if !tt.expectSuccess && err == nil { - t.Errorf("got nil but expected an error") - } - }) - } - for _, el := range urlValidityTests { - el.tokURL = strings.ToUpper(el.tokURL) - } - for _, tt := range urlValidityTests { - t.Run(" "+tt.tokURL, func(t *testing.T) { // We prepend a space ahead of the test input when outputting for sake of readability. - config := testConfig - config.TokenURL = tt.tokURL - _, err := config.TokenSource(ctx) - - if tt.expectSuccess && err != nil { - t.Errorf("got %v but want nil", err) - } else if !tt.expectSuccess && err == nil { - t.Errorf("got nil but expected an error") - } - }) - } -} - -func TestValidateURLImpersonateURL(t *testing.T) { - var urlValidityTests = []struct { - impURL string - expectSuccess bool - }{ - {"https://east.iamcredentials.googleapis.com", true}, - {"https://iamcredentials.googleapis.com", true}, - {"https://iamcredentials.asfeasfesef.googleapis.com", true}, - {"https://us-east-1-iamcredentials.googleapis.com", true}, - {"https://iamcredentials.googleapis.com/your/path/here", true}, - {"https://.iamcredentials.googleapis.com", false}, - {"https://badiamcredentials.googleapis.com", false}, - {"https://iamcredentials.asfe.asfesef.googleapis.com", false}, - {"https://iamcredentials..googleapis.com", false}, - {"https://-iamcredentials.googleapis.com", false}, - {"https://us-ea.st-1-iamcredentials.googleapis.com", false}, - {"https://iamcredentials.googleapis.com.evil.com/whatever/path", false}, - {"https://us-eas\\t-1.iamcredentials.googleapis.com", false}, - {"https:/us-ea/st-1.iamcredentials.googleapis.com", false}, - {"https:/us-east 1.iamcredentials.googleapis.com", false}, - {"https://", false}, - {"http://us-east-1.iamcredentials.googleapis.com", false}, - {"https://us-east-1.iamcredentials.googleapis.comevil.com", false}, - {"https://iamcredentials-xyz.p.googleapis.com", true}, - {"https://iamcredentials.pgoogleapis.com", false}, - {"https://p.googleapis.com", false}, - {"https://iamcredentials.p.com", false}, - {"http://iamcredentials.p.googleapis.com", false}, - {"https://xyz-iamcredentials.p.googleapis.com", false}, - {"https://iamcredentials-xyz.123.p.googleapis.com", false}, - {"https://iamcredentials-xyz.p1.googleapis.com", false}, - {"https://iamcredentials-xyz.p.foo.com", false}, - {"https://iamcredentials-xyz.p.foo.googleapis.com", false}, - } - ctx := context.Background() - for _, tt := range urlValidityTests { - t.Run(" "+tt.impURL, func(t *testing.T) { // We prepend a space ahead of the test input when outputting for sake of readability. - config := testConfig - config.TokenURL = "https://sts.googleapis.com" // Setting the most basic acceptable tokenURL - config.ServiceAccountImpersonationURL = tt.impURL - _, err := config.TokenSource(ctx) - - if tt.expectSuccess && err != nil { - t.Errorf("got %v but want nil", err) - } else if !tt.expectSuccess && err == nil { - t.Errorf("got nil but expected an error") - } - }) - } - for _, el := range urlValidityTests { - el.impURL = strings.ToUpper(el.impURL) - } - for _, tt := range urlValidityTests { - t.Run(" "+tt.impURL, func(t *testing.T) { // We prepend a space ahead of the test input when outputting for sake of readability. - config := testConfig - config.TokenURL = "https://sts.googleapis.com" // Setting the most basic acceptable tokenURL - config.ServiceAccountImpersonationURL = tt.impURL - _, err := config.TokenSource(ctx) - - if tt.expectSuccess && err != nil { - t.Errorf("got %v but want nil", err) - } else if !tt.expectSuccess && err == nil { - t.Errorf("got nil but expected an error") - } - }) - } -} - func TestWorkforcePoolCreation(t *testing.T) { var audienceValidatyTests = []struct { audience string diff --git a/google/internal/externalaccount/impersonate_test.go b/google/internal/externalaccount/impersonate_test.go index 17e2f6d7..8c7f6a9a 100644 --- a/google/internal/externalaccount/impersonate_test.go +++ b/google/internal/externalaccount/impersonate_test.go @@ -9,7 +9,6 @@ import ( "io/ioutil" "net/http" "net/http/httptest" - "regexp" "testing" ) @@ -114,8 +113,7 @@ func TestImpersonation(t *testing.T) { defer targetServer.Close() testImpersonateConfig.TokenURL = targetServer.URL - allURLs := regexp.MustCompile(".+") - ourTS, err := testImpersonateConfig.tokenSource(context.Background(), []*regexp.Regexp{allURLs}, []*regexp.Regexp{allURLs}, "http") + ourTS, err := testImpersonateConfig.tokenSource(context.Background(), "http") if err != nil { t.Fatalf("Failed to create TokenSource: %v", err) }