google/externalaccount: validate tokenURL and ServiceAccountImpersonationURL

Change-Id: Iab70cc967fd97ac8e349a14760df0f8b02ddf074
GitHub-Last-Rev: ddf4dbd0b7096a0d34677047b9c3992bb6ed359b
GitHub-Pull-Request: golang/oauth2#514
Reviewed-on: https://go-review.googlesource.com/c/oauth2/+/340569
Reviewed-by: Patrick Jones <ithuriel@google.com>
Reviewed-by: Cody Oss <codyoss@google.com>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Trust: Cody Oss <codyoss@google.com>
Run-TryBot: Cody Oss <codyoss@google.com>
TryBot-Result: Go Bot <gobot@golang.org>

google/google.go

@@ -177,7 +177,7 @@ func (f *credentialsFile) tokenSource(ctx context.Context, params CredentialsPar
 			QuotaProjectID:                 f.QuotaProjectID,
 			Scopes:                         params.Scopes,
 		}
-		return cfg.TokenSource(ctx), nil
+		return cfg.TokenSource(ctx)
 	case "":
 		return nil, errors.New("missing 'type' field in credentials")
 	default:

google/internal/externalaccount/aws_test.go

@@ -28,8 +28,7 @@ func setTime(testTime time.Time) func() time.Time {
 
 func setEnvironment(env map[string]string) func(string) string {
 	return func(key string) string {
-		value, _ := env[key]
+		return env[key]
-		return value
 	}
 }
 

google/internal/externalaccount/basecredentials.go

@@ -7,10 +7,14 @@ package externalaccount
 import (
 	"context"
 	"fmt"
-	"golang.org/x/oauth2"
 	"net/http"
+	"net/url"
+	"regexp"
 	"strconv"
+	"strings"
 	"time"
+
+	"golang.org/x/oauth2"
 )
 
 // now aliases time.Now for testing
@@ -51,14 +55,72 @@ type Config struct {
 	Scopes []string
 }
 
+// Each element consists of a list of patterns.  validateURLs checks for matches
+// 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$`),
+	}
+	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$`),
+	}
+)
+
+func validateURL(input string, patterns []*regexp.Regexp, scheme string) bool {
+	parsed, err := url.Parse(input)
+	if err != nil {
+		return false
+	}
+	if !strings.EqualFold(parsed.Scheme, scheme) {
+		return false
+	}
+	toTest := parsed.Host
+
+	for _, pattern := range patterns {
+
+		if valid := pattern.MatchString(toTest); valid {
+			return true
+		}
+	}
+	return false
+}
+
 // 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 {
+func (c *Config) TokenSource(ctx context.Context) (oauth2.TokenSource, error) {
+	return c.tokenSource(ctx, validTokenURLPatterns, validImpersonateURLPatterns, "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")
+		}
+	}
+
 	ts := tokenSource{
 		ctx:  ctx,
 		conf: c,
 	}
 	if c.ServiceAccountImpersonationURL == "" {
-		return oauth2.ReuseTokenSource(nil, ts)
+		return oauth2.ReuseTokenSource(nil, ts), nil
 	}
 	scopes := c.Scopes
 	ts.conf.Scopes = []string{"https://www.googleapis.com/auth/cloud-platform"}
@@ -68,7 +130,7 @@ func (c *Config) TokenSource(ctx context.Context) oauth2.TokenSource {
 		scopes: scopes,
 		ts:     oauth2.ReuseTokenSource(nil, ts),
 	}
-	return oauth2.ReuseTokenSource(nil, imp)
+	return oauth2.ReuseTokenSource(nil, imp), nil
 }
 
 // Subject token file types.

google/internal/externalaccount/basecredentials_test.go

@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 )
@@ -95,3 +96,117 @@ func TestToken(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},
+	}
+	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},
+	}
+	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")
+			}
+		})
+	}
+}

google/internal/externalaccount/clientauth.go

@@ -6,9 +6,10 @@ package externalaccount
 
 import (
 	"encoding/base64"
-	"golang.org/x/oauth2"
 	"net/http"
 	"net/url"
+
+	"golang.org/x/oauth2"
 )
 
 // clientAuthentication represents an OAuth client ID and secret and the mechanism for passing these credentials as stated in rfc6749#2.3.1.

google/internal/externalaccount/clientauth_test.go

@@ -5,11 +5,12 @@
 package externalaccount
 
 import (
-	"golang.org/x/oauth2"
 	"net/http"
 	"net/url"
 	"reflect"
 	"testing"
+
+	"golang.org/x/oauth2"
 )
 
 var clientID = "rbrgnognrhongo3bi4gb9ghg9g"

google/internal/externalaccount/impersonate.go

@@ -9,11 +9,12 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"golang.org/x/oauth2"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"time"
+
+	"golang.org/x/oauth2"
 )
 
 // generateAccesstokenReq is used for service account impersonation

google/internal/externalaccount/impersonate_test.go

@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"regexp"
 	"testing"
 )
 
@@ -76,7 +77,11 @@ func TestImpersonation(t *testing.T) {
 	defer targetServer.Close()
 
 	testImpersonateConfig.TokenURL = targetServer.URL
-	ourTS := testImpersonateConfig.TokenSource(context.Background())
+	allURLs := regexp.MustCompile(".+")
+	ourTS, err := testImpersonateConfig.tokenSource(context.Background(), []*regexp.Regexp{allURLs}, []*regexp.Regexp{allURLs}, "http")
+	if err != nil {
+		t.Fatalf("Failed to create TokenSource: %v", err)
+	}
 
 	oldNow := now
 	defer func() { now = oldNow }()

google/internal/externalaccount/sts_exchange.go

@@ -65,6 +65,9 @@ func exchangeToken(ctx context.Context, endpoint string, request *stsTokenExchan
 	defer resp.Body.Close()
 
 	body, err := ioutil.ReadAll(io.LimitReader(resp.Body, 1<<20))
+	if err != nil {
+		return nil, err
+	}
 	if c := resp.StatusCode; c < 200 || c > 299 {
 		return nil, fmt.Errorf("oauth2/google: status code %d: %s", c, body)
 	}

google/internal/externalaccount/sts_exchange_test.go

@@ -7,12 +7,13 @@ package externalaccount
 import (
 	"context"
 	"encoding/json"
-	"golang.org/x/oauth2"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
+
+	"golang.org/x/oauth2"
 )
 
 var auth = clientAuthentication{
@@ -127,6 +128,9 @@ func TestExchangeToken_Opts(t *testing.T) {
 		}
 		var opts map[string]interface{}
 		err = json.Unmarshal([]byte(strOpts[0]), &opts)
+		if err != nil {
+			t.Fatalf("Couldn't parse received \"options\" field.")
+		}
 		if len(opts) < 2 {
 			t.Errorf("Too few options received.")
 		}

google/internal/externalaccount/urlcredsource.go

@@ -9,10 +9,11 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"golang.org/x/oauth2"
 	"io"
 	"io/ioutil"
 	"net/http"
+
+	"golang.org/x/oauth2"
 )
 
 type urlCredentialSource struct {