From 2984186a39321766f55e24a10e7ee41ebc6ef4d4 Mon Sep 17 00:00:00 2001 From: Ryan Kohler Date: Fri, 22 Apr 2022 15:21:36 -0700 Subject: [PATCH] Changes requested by @lsirac --- .../externalaccount/basecredentials.go | 4 +- .../externalaccount/executablecredsource.go | 25 +++- .../executablecredsource_test.go | 129 ++++++++++++++---- 3 files changed, 128 insertions(+), 30 deletions(-) diff --git a/google/internal/externalaccount/basecredentials.go b/google/internal/externalaccount/basecredentials.go index 45cb7cb..4251c44 100644 --- a/google/internal/externalaccount/basecredentials.go +++ b/google/internal/externalaccount/basecredentials.go @@ -183,7 +183,7 @@ type CredentialSource struct { type ExecutableConfig struct { Command string `json:"command"` - TimeoutMillis int `json:"timeout_millis"` + TimeoutMillis *int `json:"timeout_millis"` OutputFile string `json:"output_file"` } @@ -214,7 +214,7 @@ func (c *Config) parse(ctx context.Context) (baseCredentialSource, error) { } else if c.CredentialSource.URL != "" { return urlCredentialSource{URL: c.CredentialSource.URL, Headers: c.CredentialSource.Headers, Format: c.CredentialSource.Format, ctx: ctx}, nil } else if c.CredentialSource.Executable != nil { - return CreateExecutableCredential(ctx, c.CredentialSource.Executable, c), nil + return CreateExecutableCredential(ctx, c.CredentialSource.Executable, c) } return nil, fmt.Errorf("oauth2/google: unable to parse credential source") } diff --git a/google/internal/externalaccount/executablecredsource.go b/google/internal/externalaccount/executablecredsource.go index 09dd7fb..e43f950 100644 --- a/google/internal/externalaccount/executablecredsource.go +++ b/google/internal/externalaccount/executablecredsource.go @@ -20,6 +20,8 @@ var serviceAccountImpersonationCompiler = regexp.MustCompile("https://iamcredent const ( executableSupportedMaxVersion = 1 defaultTimeout = 30 * time.Second + timeoutMinimum = 5 * time.Second + timeoutMaximum = 120 * time.Second ) func missingFieldError(field string) error { @@ -63,7 +65,15 @@ func executableError(err error) error { } func executablesDisallowedError() error { - return errors.New("Executables need to be explicitly allowed (set GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES to '1') to run") + return errors.New("oauth2/google: Executables need to be explicitly allowed (set GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES to '1') to run") +} + +func timeoutRangeError() error { + return errors.New("oauth2/google: Invalid `timeout_millis` field. Executable timeout must be between 5 and 120 seconds.") +} + +func commandMissingError() error { + return errors.New("oauth2/google: Missing `command` field. Executable command must be provided.") } // baseEnv is an alias of os.Environ used for testing @@ -100,12 +110,19 @@ type executableCredentialSource struct { // CreateExecutableCredential creates an executableCredentialSource given an ExecutableConfig. // It also performs defaulting and type conversions. -func CreateExecutableCredential(ctx context.Context, ec *ExecutableConfig, config *Config) (result executableCredentialSource) { +func CreateExecutableCredential(ctx context.Context, ec *ExecutableConfig, config *Config) (result executableCredentialSource, err error) { + if ec.Command == "" { + err = commandMissingError() + } result.Command = ec.Command - if ec.TimeoutMillis == 0 { + if ec.TimeoutMillis == nil { result.Timeout = defaultTimeout } else { - result.Timeout = time.Duration(ec.TimeoutMillis) * time.Millisecond + result.Timeout = time.Duration(*ec.TimeoutMillis) * time.Millisecond + if result.Timeout < timeoutMinimum || result.Timeout > timeoutMaximum { + err = timeoutRangeError() + return + } } result.OutputFile = ec.OutputFile result.ctx = ctx diff --git a/google/internal/externalaccount/executablecredsource_test.go b/google/internal/externalaccount/executablecredsource_test.go index 1d6f66e..3b29201 100644 --- a/google/internal/externalaccount/executablecredsource_test.go +++ b/google/internal/externalaccount/executablecredsource_test.go @@ -31,10 +31,13 @@ func String(s string) *string { func TestCreateExecutableCredential(t *testing.T) { ec := ExecutableConfig{ Command: "blarg", - TimeoutMillis: 50000, + TimeoutMillis: Int(50000), } - ecs := CreateExecutableCredential(context.Background(), &ec, nil) + ecs, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err != nil { + t.Fatalf("creation failed %v", err) + } if ecs.Command != "blarg" { t.Errorf("ecs.Command got %v but want %v", ecs.Command, "blarg") } @@ -48,7 +51,10 @@ func TestCreateExecutableCredential_WithoutTimeout(t *testing.T) { Command: "blarg", } - ecs := CreateExecutableCredential(context.Background(), &ec, nil) + ecs, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err != nil { + t.Fatalf("creation failed %v", err) + } if ecs.Command != "blarg" { t.Errorf("ecs.Command got %v but want %v", ecs.Command, "blarg") } @@ -57,6 +63,72 @@ func TestCreateExecutableCredential_WithoutTimeout(t *testing.T) { } } +func TestCreateExectuableCredential_WithoutCommand(t *testing.T) { + ec := ExecutableConfig{} + + _, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err == nil { + t.Fatalf("Expected error but found none") + } + if got, want := err.Error(), commandMissingError().Error(); got != want { + t.Errorf("Incorrect error received.\nReceived: %s\nExpected: %s", got, want) + } +} + +func TestCreateExectuableCredential_TimeoutTooLow(t *testing.T) { + ec := ExecutableConfig{ + Command: "blarg", + TimeoutMillis: Int(4999), + } + + _, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err == nil { + t.Fatalf("Expected error but found none") + } + if got, want := err.Error(), timeoutRangeError().Error(); got != want { + t.Errorf("Incorrect error received.\nReceived: %s\nExpected: %s", got, want) + } +} + +func TestCreateExectuableCredential_TimeoutLow(t *testing.T) { + ec := ExecutableConfig{ + Command: "blarg", + TimeoutMillis: Int(5000), + } + + _, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err != nil { + t.Fatalf("creation failed %v", err) + } +} + +func TestCreateExectuableCredential_TimeoutHigh(t *testing.T) { + ec := ExecutableConfig{ + Command: "blarg", + TimeoutMillis: Int(120000), + } + + _, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err != nil { + t.Fatalf("creation failed %v", err) + } +} + +func TestCreateExectuableCredential_TimeoutTooHigh(t *testing.T) { + ec := ExecutableConfig{ + Command: "blarg", + TimeoutMillis: Int(120001), + } + + _, err := CreateExecutableCredential(context.Background(), &ec, nil) + if err == nil { + t.Fatalf("Expected error but found none") + } + if got, want := err.Error(), timeoutRangeError().Error(); got != want { + t.Errorf("Incorrect error received.\nReceived: %s\nExpected: %s", got, want) + } +} + func areSlicesEquivalent(a, b []string) bool { if len(a) != len(b) { return false @@ -85,7 +157,10 @@ func TestMinimalExecutableCredentialGetEnvironment(t *testing.T) { }, } - ecs := CreateExecutableCredential(context.Background(), config.CredentialSource.Executable, &config) + ecs, err := CreateExecutableCredential(context.Background(), config.CredentialSource.Executable, &config) + if err != nil { + t.Fatalf("creation failed %v", err) + } oldBaseEnv := baseEnv defer func() { baseEnv = oldBaseEnv }() @@ -118,7 +193,10 @@ func TestExectuableCredentialGetEnvironmentMalformedImpersonationUrl(t *testing. }, } - ecs := CreateExecutableCredential(context.Background(), config.CredentialSource.Executable, &config) + ecs, err := CreateExecutableCredential(context.Background(), config.CredentialSource.Executable, &config) + if err != nil { + t.Fatalf("creation failed %v", err) + } oldBaseEnv := baseEnv defer func() { baseEnv = oldBaseEnv }() @@ -153,7 +231,10 @@ func TestExectuableCredentialGetEnvironment(t *testing.T) { }, } - ecs := CreateExecutableCredential(context.Background(), config.CredentialSource.Executable, &config) + ecs, err := CreateExecutableCredential(context.Background(), config.CredentialSource.Executable, &config) + if err != nil { + t.Fatalf("creation failed %v", err) + } oldBaseEnv := baseEnv defer func() { baseEnv = oldBaseEnv }() @@ -178,7 +259,7 @@ func TestRetrieveExecutableSubjectTokenWithoutEnvironmentVariablesSet(t *testing cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -207,7 +288,7 @@ func TestRetrieveExecutableSubjectTokenInvalidFormat(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -251,7 +332,7 @@ func TestRetrieveExecutableSubjectTokenMissingVersion(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -297,7 +378,7 @@ func TestRetrieveExecutableSubjectTokenMissingSuccess(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -343,7 +424,7 @@ func TestRetrieveExecutableSubjectTokenUnsuccessfulResponseWithFields(t *testing cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -392,7 +473,7 @@ func TestRetrieveExecutableSubjectTokenUnsuccessfulResponseWithCode(t *testing.T cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -440,7 +521,7 @@ func TestRetrieveExecutableSubjectTokenUnsuccessfulResponseWithMessage(t *testin cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -488,7 +569,7 @@ func TestRetrieveExecutableSubjectTokenUnsuccessfulResponseWithoutFields(t *test cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -535,7 +616,7 @@ func TestRetrieveExecutableSubjectTokenNewerVersion(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -582,7 +663,7 @@ func TestRetrieveExecutableSubjectTokenMissingExpiration(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -630,7 +711,7 @@ func TestRetrieveExecutableSubjectTokenTokenTypeMissing(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -678,7 +759,7 @@ func TestRetrieveExecutableSubjectTokenInvalidTokenType(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -727,7 +808,7 @@ func TestRetrieveExecutableSubjectTokenExpired(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -776,7 +857,7 @@ func TestRetrieveExecutableSubjectTokenJwt(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -827,7 +908,7 @@ func TestRetrieveExecutableSubjectTokenJwtMissingIdToken(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -876,7 +957,7 @@ func TestRetrieveExecutableSubjectTokenIdToken(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -927,7 +1008,7 @@ func TestRetrieveExecutableSubjectTokenSaml(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, } @@ -978,7 +1059,7 @@ func TestRetrieveExecutableSubjectTokenSamlMissingResponse(t *testing.T) { cs := CredentialSource{ Executable: &ExecutableConfig{ Command: "blarg", - TimeoutMillis: 5000, + TimeoutMillis: Int(5000), }, }