From 7a83d18f231a4d77c5e0a5b687694ad10f21b7a6 Mon Sep 17 00:00:00 2001 From: Moraru Costel Date: Tue, 29 Jun 2021 20:37:03 +0200 Subject: [PATCH] Extend email-domain validation with sub-domain capability (#1233) * Extend email-domain validation with sub-domain capability * Adding the CHANGELOG entry * Fixing lint erros * Fixing lint erros * Renamed the emailDomains to allowedDomains, plus tests * Bringing together all basic test-cases * Fixing unit tests * Add unit tests to validate additional vulnerability concerns --- CHANGELOG.md | 2 +- validator.go | 25 ++- validator_test.go | 436 ++++++++++++++++++++++++++++++++-------------- 3 files changed, 327 insertions(+), 136 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52139ef8..94b02409 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ ## Breaking Changes ## Changes since v7.1.3 - +- [#1233](https://github.com/oauth2-proxy/oauth2-proxy/pull/1233) Extend email-domain validation with sub-domain capability (@morarucostel) - [#1060](https://github.com/oauth2-proxy/oauth2-proxy/pull/1060) Implement RewriteTarget to allow requests to be rewritten before proxying to upstream servers (@JoelSpeed) - [#1086](https://github.com/oauth2-proxy/oauth2-proxy/pull/1086) Refresh sessions before token expiration if configured (@NickMeves) - [#1226](https://github.com/oauth2-proxy/oauth2-proxy/pull/1226) Move app redirection logic to its own package (@JoelSpeed) diff --git a/validator.go b/validator.go index 71f32a2d..6d2a9b68 100644 --- a/validator.go +++ b/validator.go @@ -2,7 +2,6 @@ package main import ( "encoding/csv" - "fmt" "io" "os" "strings" @@ -83,7 +82,7 @@ func newValidatorImpl(domains []string, usersFile string, allowAll = true continue } - domains[i] = fmt.Sprintf("@%s", strings.ToLower(domain)) + domains[i] = strings.ToLower(domain) } validator := func(email string) (valid bool) { @@ -91,9 +90,7 @@ func newValidatorImpl(domains []string, usersFile string, return } email = strings.ToLower(email) - for _, domain := range domains { - valid = valid || strings.HasSuffix(email, domain) - } + valid = isEmailValidWithDomains(email, domains) if !valid { valid = validUsers.IsValid(email) } @@ -109,3 +106,21 @@ func newValidatorImpl(domains []string, usersFile string, func NewValidator(domains []string, usersFile string) func(string) bool { return newValidatorImpl(domains, usersFile, nil, func() {}) } + +// isEmailValidWithDomains checks if the authenticated email is validated against the provided domain +func isEmailValidWithDomains(email string, allowedDomains []string) bool { + for _, domain := range allowedDomains { + // allow if the domain is perfect suffix match with the email + if strings.HasSuffix(email, "@"+domain) { + return true + } + + // allow if the domain is prefixed with . and + // the last element (split on @) has the suffix as the domain + atoms := strings.Split(email, "@") + if strings.HasPrefix(domain, ".") && strings.HasSuffix(atoms[len(atoms)-1], domain) { + return true + } + } + return false +} diff --git a/validator_test.go b/validator_test.go index 58253e8e..5ea8ebf2 100644 --- a/validator_test.go +++ b/validator_test.go @@ -5,6 +5,8 @@ import ( "os" "strings" "testing" + + . "github.com/onsi/gomega" ) type ValidatorTest struct { @@ -59,118 +61,50 @@ func (vt *ValidatorTest) WriteEmails(t *testing.T, emails []string) { } } -func TestValidatorEmpty(t *testing.T) { - vt := NewValidatorTest(t) - defer vt.TearDown() - - vt.WriteEmails(t, []string(nil)) - domains := []string(nil) - validator := vt.NewValidator(domains, nil) - - if validator("foo.bar@example.com") { - t.Error("nothing should validate when the email and " + - "domain lists are empty") - } -} - -func TestValidatorSingleEmail(t *testing.T) { - vt := NewValidatorTest(t) - defer vt.TearDown() - - vt.WriteEmails(t, []string{"foo.bar@example.com"}) - domains := []string(nil) - validator := vt.NewValidator(domains, nil) - - if !validator("foo.bar@example.com") { - t.Error("email should validate") - } - if validator("baz.quux@example.com") { - t.Error("email from same domain but not in list " + - "should not validate when domain list is empty") - } -} - -func TestValidatorSingleDomain(t *testing.T) { - vt := NewValidatorTest(t) - defer vt.TearDown() - - vt.WriteEmails(t, []string(nil)) - domains := []string{"example.com"} - validator := vt.NewValidator(domains, nil) - - if !validator("foo.bar@example.com") { - t.Error("email should validate") - } - if !validator("baz.quux@example.com") { - t.Error("email from same domain should validate") - } -} - -func TestValidatorMultipleEmailsMultipleDomains(t *testing.T) { - vt := NewValidatorTest(t) - defer vt.TearDown() - - vt.WriteEmails(t, []string{ - "xyzzy@example.com", - "plugh@example.com", - }) - domains := []string{"example0.com", "example1.com"} - validator := vt.NewValidator(domains, nil) - - if !validator("foo.bar@example0.com") { - t.Error("email from first domain should validate") - } - if !validator("baz.quux@example1.com") { - t.Error("email from second domain should validate") - } - if !validator("xyzzy@example.com") { - t.Error("first email in list should validate") - } - if !validator("plugh@example.com") { - t.Error("second email in list should validate") - } - if validator("xyzzy.plugh@example.com") { - t.Error("email not in list that matches no domains " + - "should not validate") - } -} - -func TestValidatorComparisonsAreCaseInsensitive(t *testing.T) { - vt := NewValidatorTest(t) - defer vt.TearDown() - - vt.WriteEmails(t, []string{"Foo.Bar@Example.Com"}) - domains := []string{"Frobozz.Com"} - validator := vt.NewValidator(domains, nil) - - if !validator("foo.bar@example.com") { - t.Error("loaded email addresses are not lower-cased") - } - if !validator("Foo.Bar@Example.Com") { - t.Error("validated email addresses are not lower-cased") - } - if !validator("foo.bar@frobozz.com") { - t.Error("loaded domains are not lower-cased") - } - if !validator("foo.bar@Frobozz.Com") { - t.Error("validated domains are not lower-cased") - } -} - -func TestValidatorIgnoreSpacesInAuthEmails(t *testing.T) { - vt := NewValidatorTest(t) - defer vt.TearDown() - - vt.WriteEmails(t, []string{" foo.bar@example.com "}) - domains := []string(nil) - validator := vt.NewValidator(domains, nil) - - if !validator("foo.bar@example.com") { - t.Error("email should validate") - } -} - func TestValidatorOverwriteEmailListDirectly(t *testing.T) { + testCasesPreUpdate := []struct { + name string + email string + expectedAuthZ bool + }{ + { + name: "FirstEmailInList", + email: "xyzzy@example.com", + expectedAuthZ: true, + }, + { + name: "SecondEmailInList", + email: "plugh@example.com", + expectedAuthZ: true, + }, + { + name: "EmailNotInListThatMatchesNoDomains", + email: "xyzzy.plugh@example.com", + expectedAuthZ: false, + }, + } + testCasesPostUpdate := []struct { + name string + email string + expectedAuthZ bool + }{ + { + name: "email removed from list", + email: "xyzzy@example.com", + expectedAuthZ: false, + }, + { + name: "email retained in list", + email: "plugh@example.com", + expectedAuthZ: true, + }, + { + name: "email added to list", + email: "xyzzy.plugh@example.com", + expectedAuthZ: true, + }, + } + vt := NewValidatorTest(t) defer vt.TearDown() @@ -178,19 +112,15 @@ func TestValidatorOverwriteEmailListDirectly(t *testing.T) { "xyzzy@example.com", "plugh@example.com", }) - domains := []string(nil) updated := make(chan bool) - validator := vt.NewValidator(domains, updated) + validator := vt.NewValidator([]string(nil), updated) - if !validator("xyzzy@example.com") { - t.Error("first email in list should validate") - } - if !validator("plugh@example.com") { - t.Error("second email in list should validate") - } - if validator("xyzzy.plugh@example.com") { - t.Error("email not in list that matches no domains " + - "should not validate") + for _, tc := range testCasesPreUpdate { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + authorized := validator(tc.email) + g.Expect(authorized).To(Equal(tc.expectedAuthZ)) + }) } vt.WriteEmails(t, []string{ @@ -199,13 +129,259 @@ func TestValidatorOverwriteEmailListDirectly(t *testing.T) { }) <-updated - if validator("xyzzy@example.com") { - t.Error("email removed from list should not validate") - } - if !validator("plugh@example.com") { - t.Error("email retained in list should validate") - } - if !validator("xyzzy.plugh@example.com") { - t.Error("email added to list should validate") + for _, tc := range testCasesPostUpdate { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + authorized := validator(tc.email) + g.Expect(authorized).To(Equal(tc.expectedAuthZ)) + }) + } +} + +func TestValidatorCases(t *testing.T) { + testCases := []struct { + name string + allowedEmails []string + allowedDomains []string + email string + expectedAuthZ bool + }{ + { + name: "EmailNotInCorrect1stSubDomainsNotInEmails", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "foo.bar@example0.com", + expectedAuthZ: false, + }, + { + name: "EmailInFirstDomain", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "foo@bar.example0.com", + expectedAuthZ: true, + }, + { + name: "EmailNotInCorrect2ndSubDomainsNotInEmails", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "baz.quux@example1.com", + expectedAuthZ: false, + }, + { + name: "EmailInSecondDomain", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "baz@quux.example1.com", + expectedAuthZ: true, + }, + { + name: "EmailInFirstEmailList", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "xyzzy@example.com", + expectedAuthZ: true, + }, + { + name: "EmailNotInDomainsNotInEmails", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "xyzzy.plugh@example.com", + expectedAuthZ: false, + }, + { + name: "EmailInLastEmailList", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{".example0.com", ".example1.com"}, + email: "plugh@example.com", + expectedAuthZ: true, + }, + { + name: "EmailIn1stSubdomain", + allowedEmails: nil, + allowedDomains: []string{"us.example.com", "de.example.com", "example.com"}, + email: "xyzzy@us.example.com", + expectedAuthZ: true, + }, + { + name: "EmailIn2ndSubdomain", + allowedEmails: nil, + allowedDomains: []string{"us.example.com", "de.example.com", "example.com"}, + email: "xyzzy@de.example.com", + expectedAuthZ: true, + }, + { + name: "EmailNotInAnySubdomain", + allowedEmails: nil, + allowedDomains: []string{"us.example.com", "de.example.com", "example.com"}, + email: "global@au.example.com", + expectedAuthZ: false, + }, + { + name: "EmailInLastSubdomain", + allowedEmails: nil, + allowedDomains: []string{"us.example.com", "de.example.com", "example.com"}, + email: "xyzzy@example.com", + expectedAuthZ: true, + }, + { + name: "EmailDomainNotCompletelyMatch", + allowedEmails: nil, + allowedDomains: []string{".example.com", ".example1.com"}, + email: "something@fooexample.com", + expectedAuthZ: false, + }, + { + name: "HackerExtraDomainPrefix1", + allowedEmails: nil, + allowedDomains: []string{".mycompany.com"}, + email: "something@evilhackmycompany.com", + expectedAuthZ: false, + }, + { + name: "HackerExtraDomainPrefix2", + allowedEmails: nil, + allowedDomains: []string{".mycompany.com"}, + email: "something@ext.evilhackmycompany.com", + expectedAuthZ: false, + }, + { + name: "EmptyDomainAndEmailList", + allowedEmails: []string(nil), + allowedDomains: []string(nil), + email: "foo.bar@example.com", + expectedAuthZ: false, + }, + { + name: "EmailMatchWithAllowedEmails", + email: "foo.bar@example.com", + allowedEmails: []string{"foo.bar@example.com"}, + allowedDomains: []string{"example.com"}, + expectedAuthZ: true, + }, + { + name: "EmailFromSameDomainButNotInList", + email: "baz.quux@example.com", + allowedEmails: []string{"foo.bar@example.com"}, + allowedDomains: []string(nil), + expectedAuthZ: false, + }, + { + name: "EmailMatchOnDomain", + email: "foo.bar@example.com", + allowedEmails: []string(nil), + allowedDomains: []string{"example.com"}, + expectedAuthZ: true, + }, + { + name: "EmailMatchOnDomain2", + email: "baz.quux@example.com", + allowedEmails: []string(nil), + allowedDomains: []string{"example.com"}, + expectedAuthZ: true, + }, + { + name: "EmailFromFirstDomainShouldValidate", + email: "foo.bar@example0.com", + allowedEmails: []string{"Foo.Bar@Example.Com"}, + allowedDomains: []string{"example0.com", "example1.com"}, + expectedAuthZ: true, + }, + { + name: "EmailFromSecondDomainShouldValidate", + email: "baz.quux@example1.com", + allowedEmails: []string{"Foo.Bar@Example.Com"}, + allowedDomains: []string{"example0.com", "example1.com"}, + expectedAuthZ: true, + }, + { + name: "FirstEmailInListShouldValidate", + email: "xyzzy@example.com", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{"example0.com", "example1.com"}, + expectedAuthZ: true, + }, + { + name: "SecondEmailInListShouldValidate", + email: "plugh@example.com", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{"example0.com", "example1.com"}, + expectedAuthZ: true, + }, + { + name: "EmailNotInListThatMatchesNoDomains ", + email: "xyzzy.plugh@example.com", + allowedEmails: []string{"xyzzy@example.com", "plugh@example.com"}, + allowedDomains: []string{"example0.com", "example1.com"}, + expectedAuthZ: false, + }, + { + name: "LoadedEmailAddressesAreNotLowerCased", + email: "foo.bar@example.com", + allowedEmails: []string{"Foo.Bar@Example.Com"}, + allowedDomains: []string{"Frobozz.Com"}, + expectedAuthZ: true, + }, + { + name: "ValidatedEmailAddressesAreNotLowerCased", + email: "Foo.Bar@Example.Com", + allowedEmails: []string{"Foo.Bar@Example.Com"}, + allowedDomains: []string{"Frobozz.Com"}, + expectedAuthZ: true, + }, + { + name: "LoadedDomainsAreNotLowerCased", + email: "foo.bar@frobozz.com", + allowedEmails: []string{"Foo.Bar@Example.Com"}, + allowedDomains: []string{"Frobozz.Com"}, + expectedAuthZ: true, + }, + { + name: "ValidatedDomainsAreNotLowerCased", + email: "foo.bar@Frobozz.Com", + allowedEmails: []string{"Foo.Bar@Example.Com"}, + allowedDomains: []string{"Frobozz.Com"}, + expectedAuthZ: true, + }, + { + name: "IgnoreSpacesInAuthEmails", + email: "foo.bar@example.com", + allowedEmails: []string{" foo.bar@example.com "}, + allowedDomains: []string(nil), + expectedAuthZ: true, + }, + { + name: "IgnorePrefixSpacesInAuthEmails", + email: "foo.bar@example.com", + allowedEmails: []string{" foo.bar@example.com"}, + allowedDomains: []string(nil), + expectedAuthZ: true, + }, + { + name: "CheckForEqualityNotSuffix", + email: "foo@evilcompany.com", + allowedEmails: []string(nil), + allowedDomains: []string{".company.com"}, + expectedAuthZ: false, + }, + { + name: "CheckForEqualityNotSuffix2", + email: "foo@evilcompany.com", + allowedEmails: []string(nil), + allowedDomains: []string{"company.com"}, + expectedAuthZ: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + vt := NewValidatorTest(t) + defer vt.TearDown() + + g := NewWithT(t) + vt.WriteEmails(t, tc.allowedEmails) + validator := vt.NewValidator(tc.allowedDomains, nil) + authorized := validator(tc.email) + g.Expect(authorized).To(Equal(tc.expectedAuthZ)) + }) } }