Repository: oauth2-proxy/oauth2-proxy
Pull request #1233 (2021-06-09 15:05:08)

Extend email-domain validation with sub-domain capability
<!--- Provide a general summary of your changes in the Title above -->

## Description
Extend `email-domain` validation with sub-domain capability, by allowing `.` to the domain
Fix for the #927 issue.

<!--- Describe your changes in detail -->

## Motivation and Context

Extremely difficult to configure and mantain in an organisation with 100+ subdomains. Extend the AuthZ capabilities to ease the configuration.

<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## How Has This Been Tested?
Tested in our k8s environments and with the help of unit tests.

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


morarucostel: Hey @JoelSpeed , @NickMeves , 
Can you please have a look on the PR?
morarucostel: @JoelSpeed I do have mixed feelings about the utility method. I think there are quite a few differences between the two scenarios (prefix handling with `/` with `http`, port logic ) for just 1-2 lines of code...

If anyone wants to give it a try to this.
JoelSpeed: The utility method for the subdomain check is by no means a blocker for this PR, can always be done as a follow up later so I wouldn't stress
morarucostel: Hey @NickMeves , 
I see the tests are failing in `TestEncodeAndDecodeSessionState`. I haven't changed anything near that, so most probably would be from another commit that was merged in.
morarucostel: @NickMeves @JoelSpeed any updates on this PR? 
Looks go to be merged?

Patch:
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
+}


Test patch:
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 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,
+		},
 	}
-}
-
-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")
+	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,
+		},
 	}
-}
 
-func TestValidatorOverwriteEmailListDirectly(t *testing.T) {
 	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")
+	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))
+		})
 	}
-	if !validator("plugh@example.com") {
-		t.Error("email retained in list should validate")
+}
+
+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,
+		},
 	}
-	if !validator("xyzzy.plugh@example.com") {
-		t.Error("email added to list should validate")
+
+	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))
+		})
 	}
 }