From b441bc2a966e43c145d35631582b27e8c080202e Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Fri, 6 Mar 2020 09:15:39 +0000 Subject: [PATCH 1/6] sso_proxy: temporary fix for when group validation is not used --- internal/proxy/providers/sso.go | 3 +++ internal/proxy/providers/sso_test.go | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/proxy/providers/sso.go b/internal/proxy/providers/sso.go index 1aca089c..08a496a6 100644 --- a/internal/proxy/providers/sso.go +++ b/internal/proxy/providers/sso.go @@ -181,6 +181,9 @@ func (p *SSOProvider) ValidateGroup(email string, allowedGroups []string, access logger.WithUser(email).WithAllowedGroups(allowedGroups).Info("validating groups") inGroups := []string{} + if len(allowedGroups) == 0 { + return inGroups, true, nil + } userGroups, err := p.UserGroups(email, allowedGroups, accessToken) if err != nil { diff --git a/internal/proxy/providers/sso_test.go b/internal/proxy/providers/sso_test.go index 1b9a2244..58a00eff 100644 --- a/internal/proxy/providers/sso_test.go +++ b/internal/proxy/providers/sso_test.go @@ -144,11 +144,11 @@ func TestSSOProviderGroups(t *testing.T) { ProfileStatus int }{ { - Name: "invalid when no group id set", + Name: "valid when no group id set", Email: "michael.bland@gsa.gov", Groups: []string{}, ProxyGroupIds: []string{}, - ExpectedValid: false, + ExpectedValid: true, ExpectedInGroups: []string{}, ExpectError: nil, }, From d8f02299f0e9e2ceb4c1b90a15fabc5428f735ae Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Tue, 10 Mar 2020 13:00:38 +0000 Subject: [PATCH 2/6] test update --- internal/proxy/providers/sso_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/proxy/providers/sso_test.go b/internal/proxy/providers/sso_test.go index 58a00eff..62961e20 100644 --- a/internal/proxy/providers/sso_test.go +++ b/internal/proxy/providers/sso_test.go @@ -319,7 +319,7 @@ func TestSSOProviderValidateSessionState(t *testing.T) { ProviderResponse: http.StatusOK, Groups: []string{}, ProxyGroupIds: []string{}, - ExpectedValid: false, + ExpectedValid: true, }, { Name: "invalid when response is is not 200", From c957a28150e678ff60f9e595ddc3d1cb446fb6e6 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Wed, 11 Mar 2020 12:04:18 +0000 Subject: [PATCH 3/6] some quick additions to test potential logic --- .../pkg/options/email_address_validator.go | 6 ----- .../pkg/options/email_domain_validator.go | 14 ++--------- internal/proxy/options.go | 25 +++++++++++++------ 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/internal/pkg/options/email_address_validator.go b/internal/pkg/options/email_address_validator.go index 02c6195e..e679c805 100644 --- a/internal/pkg/options/email_address_validator.go +++ b/internal/pkg/options/email_address_validator.go @@ -25,8 +25,6 @@ type EmailAddressValidator struct { // - is non-empty // - matches one of the originally passed in email addresses // (case insensitive) -// - if the originally passed in list of emails consists only of "*", then all emails -// are considered valid based on their domain. // If valid, nil is returned in place of an error. func NewEmailAddressValidator(allowedEmails []string) EmailAddressValidator { var emailAddresses []string @@ -50,10 +48,6 @@ func (v EmailAddressValidator) Validate(session *sessions.SessionState) error { return ErrEmailAddressDenied } - if len(v.AllowedEmails) == 1 && v.AllowedEmails[0] == "*" { - return nil - } - err := v.validate(session) if err != nil { return err diff --git a/internal/pkg/options/email_domain_validator.go b/internal/pkg/options/email_domain_validator.go index 4afdf372..92f15c07 100644 --- a/internal/pkg/options/email_domain_validator.go +++ b/internal/pkg/options/email_domain_validator.go @@ -25,19 +25,13 @@ type EmailDomainValidator struct { // - is non-empty // - the domain of the email address matches one of the originally passed in domains. // (case insensitive) -// - if the originally passed in list of domains consists only of "*", then all emails -// are considered valid based on their domain. // If valid, nil is returned in place of an error. func NewEmailDomainValidator(allowedDomains []string) EmailDomainValidator { emailDomains := make([]string, 0, len(allowedDomains)) for _, domain := range allowedDomains { - if domain == "*" { - emailDomains = append(emailDomains, domain) - } else { - emailDomain := fmt.Sprintf("@%s", strings.ToLower(domain)) - emailDomains = append(emailDomains, emailDomain) - } + emailDomain := fmt.Sprintf("@%s", strings.ToLower(domain)) + emailDomains = append(emailDomains, emailDomain) } return EmailDomainValidator{ AllowedDomains: emailDomains, @@ -53,10 +47,6 @@ func (v EmailDomainValidator) Validate(session *sessions.SessionState) error { return ErrEmailDomainDenied } - if len(v.AllowedDomains) == 1 && v.AllowedDomains[0] == "*" { - return nil - } - err := v.validate(session) if err != nil { return err diff --git a/internal/proxy/options.go b/internal/proxy/options.go index dab86d1a..630da088 100644 --- a/internal/proxy/options.go +++ b/internal/proxy/options.go @@ -64,6 +64,7 @@ type Options struct { DefaultAllowedEmailDomains []string `envconfig:"DEFAULT_ALLOWED_EMAIL_DOMAINS"` DefaultAllowedEmailAddresses []string `envconfig:"DEFAULT_ALLOWED_EMAIL_ADDRESSES"` DefaultAllowedGroups []string `envconfig:"DEFAULT_ALLOWED_GROUPS"` + AllowNoValidators bool `envconfig:"ALLOW_NO_VALIDATORS" default:"false"` ClientID string `envconfig:"CLIENT_ID"` ClientSecret string `envconfig:"CLIENT_SECRET"` @@ -196,20 +197,30 @@ func (o *Options) Validate() error { } if o.upstreamConfigs != nil { - invalidUpstreams := []string{} + noValidatorsDefined := []string{} + wildcardUsed := []string{} for _, uc := range o.upstreamConfigs { if uc.Timeout > o.TCPWriteTimeout { o.TCPWriteTimeout = uc.Timeout } - - if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 && len(uc.AllowedGroups) == 0 { - invalidUpstreams = append(invalidUpstreams, uc.Service) + if !o.AllowNoValidators { + if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 && len(uc.AllowedGroups) == 0 { + noValidatorsDefined = append(noValidatorsDefined, uc.Service) + } else if uc.AllowedEmailDomains[0] == "*" || uc.AllowedEmailAddresses[0] == "*" || uc.AllowedGroups[0] == "*" { + wildcardUsed = append(wildcardUsed, uc.Service) + } } } - if len(invalidUpstreams) != 0 { + if len(noValidatorsDefined) != 0 { + msgs = append(msgs, fmt.Sprintf( + `missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config. + If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: %v`, + noValidatorsDefined)) + } + if len(wildcardUsed) != 0 { msgs = append(msgs, fmt.Sprintf( - "missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config in the following upstreams: %v", - invalidUpstreams)) + "invalid setting: a wildcard cannot be used within validators. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected upstreams: %v", + wildcardUsed)) } } From 8c75d96f5f13aba11a18887e43501ef6adde1ad6 Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Wed, 11 Mar 2020 12:26:49 +0000 Subject: [PATCH 4/6] some test modifications --- internal/proxy/options_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/proxy/options_test.go b/internal/proxy/options_test.go index e2fdc2f1..41b45362 100644 --- a/internal/proxy/options_test.go +++ b/internal/proxy/options_test.go @@ -16,7 +16,8 @@ func testOptions() *Options { o.CookieSecret = testEncodedCookieSecret o.ClientID = "bazquux" o.ClientSecret = "xyzzyplugh" - o.DefaultAllowedEmailDomains = []string{"*"} + o.AllowNoValidators = true + //o.DefaultAllowedEmailDomains = []string{"*"} o.DefaultProviderSlug = "idp" o.ProviderURLString = "https://www.example.com" o.UpstreamConfigsFile = "testdata/upstream_configs.yml" @@ -50,6 +51,7 @@ func TestNewOptions(t *testing.T) { err := o.Validate() testutil.NotEqual(t, nil, err) + //TODO: invalid setting: wildcards used in validators not tested expected := errorMsg([]string{ "missing setting: cluster", "missing setting: provider-url", @@ -59,7 +61,7 @@ func TestNewOptions(t *testing.T) { "missing setting: client-secret", "missing setting: statsd-host", "missing setting: statsd-port", - "missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config in the following upstreams: [testService]", + "missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config. \n\t\t\t\tIf no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: [testService]", "Invalid value for COOKIE_SECRET; must decode to 32 or 64 bytes, but decoded to 0 bytes", }) testutil.Equal(t, expected, err.Error()) From bc1852ef9740860ce95dd9de7cc069510600a60c Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Wed, 11 Mar 2020 18:25:30 +0000 Subject: [PATCH 5/6] fixup current tests --- .../options/email_address_validator_test.go | 18 +++++++++--------- .../pkg/options/email_domain_validator_test.go | 18 +++++++++--------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/internal/pkg/options/email_address_validator_test.go b/internal/pkg/options/email_address_validator_test.go index aef92f75..ca226c99 100644 --- a/internal/pkg/options/email_address_validator_test.go +++ b/internal/pkg/options/email_address_validator_test.go @@ -95,28 +95,28 @@ func TestEmailAddressValidatorValidator(t *testing.T) { expectedErr: nil, }, { - name: "single wildcard allows all", + name: "single wildcard is denied", AllowedEmails: []string{"*"}, session: &sessions.SessionState{ Email: "foo@example.com", }, - expectedErr: nil, + expectedErr: ErrEmailAddressDenied, }, { - name: "single wildcard allows all", - AllowedEmails: []string{"*"}, + name: "wildcard is ignored if other domains included are invalid", + AllowedEmails: []string{"foo@example.com", "*"}, session: &sessions.SessionState{ - Email: "bar@gmail.com", + Email: "foo@gmail.com", }, - expectedErr: nil, + expectedErr: ErrEmailAddressDenied, }, { - name: "wildcard is ignored if other domains included", + name: "wildcard is ignored if other domains included are valid", AllowedEmails: []string{"foo@example.com", "*"}, session: &sessions.SessionState{ - Email: "foo@gmail.com", + Email: "foo@example.com", }, - expectedErr: ErrEmailAddressDenied, + expectedErr: nil, }, { name: "empty email rejected", diff --git a/internal/pkg/options/email_domain_validator_test.go b/internal/pkg/options/email_domain_validator_test.go index e2eb407f..0ac33e6b 100644 --- a/internal/pkg/options/email_domain_validator_test.go +++ b/internal/pkg/options/email_domain_validator_test.go @@ -95,28 +95,28 @@ func TestEmailDomainValidatorValidator(t *testing.T) { expectedErr: nil, }, { - name: "single wildcard allows all", + name: "single wildcard is denied", allowedDomains: []string{"*"}, session: &sessions.SessionState{ Email: "foo@example.com", }, - expectedErr: nil, + expectedErr: ErrEmailDomainDenied, }, { - name: "single wildcard allows all", - allowedDomains: []string{"*"}, + name: "wildcard is ignored if other domains are included are invalid", + allowedDomains: []string{"*", "example.com"}, session: &sessions.SessionState{ - Email: "bar@gmail.com", + Email: "foo@gmal.com", }, - expectedErr: nil, + expectedErr: ErrEmailDomainDenied, }, { - name: "wildcard is ignored if other domains are included", + name: "wildcard is ignored if other domains are included are valid", allowedDomains: []string{"*", "example.com"}, session: &sessions.SessionState{ - Email: "foo@gmal.com", + Email: "foo@example.com", }, - expectedErr: ErrEmailDomainDenied, + expectedErr: nil, }, { name: "empty email rejected", From f5836accf722293c10ad429b917edbd67b256b0d Mon Sep 17 00:00:00 2001 From: Jusshersmith Date: Mon, 16 Mar 2020 12:55:17 +0000 Subject: [PATCH 6/6] fix up and add tests --- internal/proxy/options.go | 18 ++++++-- internal/proxy/options_test.go | 77 +++++++++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/internal/proxy/options.go b/internal/proxy/options.go index 630da088..c5e4d878 100644 --- a/internal/proxy/options.go +++ b/internal/proxy/options.go @@ -203,17 +203,29 @@ func (o *Options) Validate() error { if uc.Timeout > o.TCPWriteTimeout { o.TCPWriteTimeout = uc.Timeout } + + // If we don't explicity accept 'no validators' then ensure defined validator values aren't len(0) or len(1) with a wildcard. + // Not accepting wildcards is a new restriction, so we largely test against this for now to raise awareness and avoid unexpected outcomes. if !o.AllowNoValidators { if len(uc.AllowedEmailDomains) == 0 && len(uc.AllowedEmailAddresses) == 0 && len(uc.AllowedGroups) == 0 { noValidatorsDefined = append(noValidatorsDefined, uc.Service) - } else if uc.AllowedEmailDomains[0] == "*" || uc.AllowedEmailAddresses[0] == "*" || uc.AllowedGroups[0] == "*" { - wildcardUsed = append(wildcardUsed, uc.Service) + } else { + validatorValues := make(map[string][]string) + validatorValues["Addresses"] = uc.AllowedEmailAddresses + validatorValues["Domains"] = uc.AllowedEmailDomains + validatorValues["Groups"] = uc.AllowedGroups + for _, v := range validatorValues { + if len(v) != 0 && v[0] == "*" { + wildcardUsed = append(wildcardUsed, uc.Service) + break + } + } } } } if len(noValidatorsDefined) != 0 { msgs = append(msgs, fmt.Sprintf( - `missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config. + `missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: %v`, noValidatorsDefined)) } diff --git a/internal/proxy/options_test.go b/internal/proxy/options_test.go index 41b45362..bd00fbc9 100644 --- a/internal/proxy/options_test.go +++ b/internal/proxy/options_test.go @@ -17,7 +17,6 @@ func testOptions() *Options { o.ClientID = "bazquux" o.ClientSecret = "xyzzyplugh" o.AllowNoValidators = true - //o.DefaultAllowedEmailDomains = []string{"*"} o.DefaultProviderSlug = "idp" o.ProviderURLString = "https://www.example.com" o.UpstreamConfigsFile = "testdata/upstream_configs.yml" @@ -61,7 +60,7 @@ func TestNewOptions(t *testing.T) { "missing setting: client-secret", "missing setting: statsd-host", "missing setting: statsd-port", - "missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config. \n\t\t\t\tIf no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: [testService]", + "missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config.\n\t\t\t\tIf no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: [testService]", "Invalid value for COOKIE_SECRET; must decode to 32 or 64 bytes, but decoded to 0 bytes", }) testutil.Equal(t, expected, err.Error()) @@ -72,6 +71,80 @@ func TestInitializedOptions(t *testing.T) { testutil.Equal(t, nil, o.Validate()) } +func TestValidatorOptions(t *testing.T) { + testCases := []struct { + name string + upstreamConfig *UpstreamConfig + AllowNoValidators bool + expectedErrors []string + }{ + { + name: "no validators error", + upstreamConfig: &UpstreamConfig{ + Service: "testService", + AllowedEmailDomains: []string{}, + AllowedEmailAddresses: []string{}, + AllowedGroups: []string{}, + }, + expectedErrors: []string{ + "missing setting: ALLOWED_EMAIL_DOMAINS, ALLOWED_EMAIL_ADDRESSES, ALLOWED_GROUPS default in environment or override in upstream config.\n\t\t\t\tIf no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected usptreams: [testService]", + }, + }, + { + name: "wildcard error", + upstreamConfig: &UpstreamConfig{ + Service: "testService", + AllowedEmailDomains: []string{"*"}, + AllowedEmailAddresses: []string{"*"}, + AllowedGroups: []string{"*"}, + }, + expectedErrors: []string{ + "invalid setting: a wildcard cannot be used within validators. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected upstreams: [testService]", + }, + }, + { + name: "validators defined, one using wildcard returns error", + upstreamConfig: &UpstreamConfig{ + Service: "testService", + AllowedEmailDomains: []string{""}, + AllowedEmailAddresses: []string{"*"}, + AllowedGroups: []string{"foo"}, + }, + expectedErrors: []string{ + "invalid setting: a wildcard cannot be used within validators. If no extra validators are required, set 'ALLOW_NO_VALIDATORS' to 'true'. Affected upstreams: [testService]", + }, + }, + { + name: "AllowNoValidators is true. No validators defined returns success", + upstreamConfig: &UpstreamConfig{ + Service: "testService", + AllowedEmailDomains: []string{}, + AllowedEmailAddresses: []string{}, + AllowedGroups: []string{}, + }, + AllowNoValidators: true, + expectedErrors: []string{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + o := testOptions() + o.AllowNoValidators = tc.AllowNoValidators + o.UpstreamConfigsFile = "" + if !tc.AllowNoValidators { + o.upstreamConfigs = []*UpstreamConfig{tc.upstreamConfig} + } + + err := o.Validate() + // Above we override the o.UpstreamConfigsFile setting to ensure we test these upstream configs with a clean slate, + // but this means we always get the below 'missing setting: upstream-configs' error. + tc.expectedErrors = append([]string{"missing setting: upstream-configs"}, tc.expectedErrors...) + testutil.Equal(t, errorMsg(tc.expectedErrors), err.Error()) + }) + } +} + func TestProviderURLValidation(t *testing.T) { testCases := []struct { name string