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_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.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/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", diff --git a/internal/proxy/options.go b/internal/proxy/options.go index dab86d1a..c5e4d878 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,42 @@ 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 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 { + 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(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 in the following upstreams: %v", - invalidUpstreams)) + `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( + "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)) } } diff --git a/internal/proxy/options_test.go b/internal/proxy/options_test.go index e2fdc2f1..bd00fbc9 100644 --- a/internal/proxy/options_test.go +++ b/internal/proxy/options_test.go @@ -16,7 +16,7 @@ func testOptions() *Options { o.CookieSecret = testEncodedCookieSecret o.ClientID = "bazquux" o.ClientSecret = "xyzzyplugh" - o.DefaultAllowedEmailDomains = []string{"*"} + o.AllowNoValidators = true o.DefaultProviderSlug = "idp" o.ProviderURLString = "https://www.example.com" o.UpstreamConfigsFile = "testdata/upstream_configs.yml" @@ -50,6 +50,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 +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 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()) @@ -70,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 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..62961e20 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, }, @@ -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",