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.

Sign up for GitHub

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

google/internal/externalaccount: Removed URL validation for google URLs in ADC files #627

Closed
wants to merge 2 commits into from
Closed

google/internal/externalaccount: Removed URL validation for google URLs in ADC files #627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45e2b9c
Removed URL validation for google URLs in ADC files
aeitzman Feb 6, 2023
15d7759
Moved warning message in doc
aeitzman Feb 15, 2023
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
6 changes: 6 additions & 0 deletions google/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
32 changes: 2 additions & 30 deletions google/internal/externalaccount/basecredentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/`)
)

Expand Down Expand Up @@ -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 {
Expand Down
135 changes: 0 additions & 135 deletions google/internal/externalaccount/basecredentials_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"io/ioutil"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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
Expand Down
4 changes: 1 addition & 3 deletions google/internal/externalaccount/impersonate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"io/ioutil"
"net/http"
"net/http/httptest"
"regexp"
"testing"
)

Expand Down Expand Up @@ -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)
}
Expand Down