Add allowed_email_domains on auth_request endpoint #1286

armandpicard · 2021-07-28T08:30:42Z

Description

Add the allowed_email_domains on the auth_request endpoint to restrict access per ingress.

Motivation and Context

Some users may want to have one oauth2 proxy with multiple allowed email domains but restrict access to some ressource to only one or multiple of these emails domains

How Has This Been Tested?

Unit Tests and test on Kubernees with an Nginx ingress.

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

NickMeves · 2021-08-07T17:10:33Z

oauthproxy.go

@@ -961,15 +961,57 @@ func (p *OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req *http.R
 //
 //nolint:gosimple


We can remove this nolint & TODO above now that you are adding more functionality 👍

NickMeves · 2021-08-07T17:12:41Z

oauthproxy.go

+	return false
+}
+
+func extractAllowedEmailDomains(req *http.Request) map[string]struct{} {


This and extractAllowedGroups are nearly identical -- maybe we should generalize it and use the querystring param as an extra argument (appears to be the only difference)?

NickMeves · 2021-08-07T17:15:58Z

oauthproxy.go

+		return false
+	}
+
+	if _, ok := allowedEmailDomains[emailSplited[1]]; ok {


Throughout the rest of the codebase, there's support for broad subdomains (.example.com would allow foo.example.com and bar.example.com). I think that support should be extended here too.

I believe there's common helper methods for this logic since it is an error-prone implementation that leads to security vulnerabilities; hopefully we can use those here?

It support now broad subdomains starting with . and *. and sharing the same code base through an util.

Agree but I didn't find out something relevant as a trusted common helper method. Do you have something in mind?

NickMeves

Thanks for the PR! I think this will be a worthwhile addition.

I've left some notes on areas to align it more with the rest of the codebase.

This needs a CHANGELOG entry as well.

w3st3ry · 2021-09-02T12:38:30Z

Hi @NickMeves, WDYT about the change? Should I add something else?

docs/docs/features/endpoints.md

NickMeves · 2021-09-05T16:39:06Z

oauthproxy.go

-		for _, group := range strings.Split(allowedGroups, ",") {
-			if group != "" {
-				groups[group] = struct{}{}
+	for _, group := range s.Groups {


What's the reasoning for moving away from Loop + Set (with O(NlogN) time complexity) to double looping to find matches (O(N^2) time complexity?

I know in small cases (< 10), double looping is fine and potentially faster. But since we don't know how large our worst case users will be using this feature -- we leaned toward O(NlogN) Loop + Set in the initial implementation.

I recommend switching the extractAllowedEntities implementation to return a map[string]struct{} instead of []string and doing a set membership check in your checker methods.

Yes you right, I fixed it with the recommended implementation.

pkg/util/util.go

NickMeves · 2021-09-05T16:49:42Z

pkg/util/util.go

@@ -23,3 +25,82 @@ func GetCertPool(paths []string) (*x509.CertPool, error) {
 	}
 	return pool, nil
 }
+
+func IsEndpointAllowed(endpoint *url.URL, allowedDomains []string) bool {


This is a great refactor! Thanks!

Let's slide it to a more specific utils area though.

Can we move this to here: https://github.com/oauth2-proxy/oauth2-proxy/blob/master/pkg/validation/utils.go

Since this is all validation related?

Yes sure, done.

JoelSpeed

@NickMeves For the group authentication, didn't we have some requirement about having to have the allowed_group be defined on the config first to be allowed to be added in the query params?

JoelSpeed · 2021-09-17T14:35:45Z

oauthproxy.go

 	}

 	return true
 }

+func extractAllowedEntities(req *http.Request, field string) map[string]struct{} {


I was a bit confused about what we were trying to achieve here, can we add a comment that explains what the inputs and outputs are at a minimum

I added a comment based on @NickMeves recommendations.
See #1286 (comment)

JoelSpeed · 2021-09-17T14:38:22Z

oauthproxy.go

+		return true
+	}
+
+	emailSplited := strings.Split(s.Email, "@")


Splitted is a bit jarring to me, what about

Suggested change

emailSplited := strings.Split(s.Email, "@")

splitEmail := strings.Split(s.Email, "@")

JoelSpeed · 2021-09-17T14:42:20Z

pkg/app/redirect/validator_test.go

+			Entry("Valid Wildcard Subdomain", "foo.wildcard.bar/redirect", true),
+			Entry("Valid Wildcard Subdomain Root", "wildcard.bar/redirect", true),
+			Entry("Valid Wildcard Subdomain anyport", "foo.wildcard.sub.anyport.bar:4242/redirect", true),
+			Entry("Valid Wildcard Subdomain Anyport Root", "wildcard.sub.anyport.bar:4242/redirect", true),
+			Entry("Valid Wildcard Subdomain Defined Port", "foo.wildcard.sub.port.bar:8080/redirect", true),
+			Entry("Valid Wildcard Subdomain Defined Port Root", "wildcard.sub.port.bar:8080/redirect", true),
+			Entry("Invalid HTTP subdomain", "foo.bar/redirect", false),
+			Entry("Invalid HTTP subdomain", "proxy.foo.bar/redirect", false),


Are we sure about this? I don't think redirects are allowed without a scheme prefixing them currently, this could lead to open redirects which I really would like to avoid

Thanks, forget to commit the fix.
Done.

JoelSpeed · 2021-09-17T14:43:01Z

pkg/validation/utils.go

@@ -9,3 +16,45 @@ func prefixValues(prefix string, values ...string) []string {
 	}
 	return msgs
 }
+
+func IsEndpointAllowed(endpoint *url.URL, allowedDomains []string) bool {


Please add a doc string for this function

I actually don't think this is the right place to put this function, the validation package is all about configuration validation rather than runtime validation like this. Can we move it to pkg/utils until we find a better home please

JoelSpeed · 2021-09-17T14:46:05Z

pkg/validation/utils.go

+	return false
+}
+
+func isHostnameBelongsToAllowedHost(hostname, allowedHost string) bool {


I think this could be simpler

Suggested change

func isHostnameBelongsToAllowedHost(hostname, allowedHost string) bool {

func isHostnameAllowed(hostname, allowedHost string) bool {

NickMeves · 2021-09-26T00:11:07Z

@NickMeves For the group authentication, didn't we have some requirement about having to have the allowed_group be defined on the config first to be allowed to be added in the query params?

@JoelSpeed I don't recall this being a requirement -- The architecture I built this for was a central Authentication Proxy (BeyondCorp style) where all the consuming applications (nginx, k8s ingress) could specificy any groups they cared about for AuthZ

w3st3ry · 2021-10-05T21:15:36Z

@JoelSpeed @NickMeves I normally solved the required fixes/changes + solved conflicts by rebasing to master.

JoelSpeed

Few more comments, otherwise I think I'm happy for this to merge

JoelSpeed · 2021-10-16T16:43:26Z

CHANGELOG.md

 - [#1210](https://github.com/oauth2-proxy/oauth2-proxy/pull/1210) A new `keycloak-oidc` provider has been added with support for role based authentication. The existing keycloak auth provider will eventually be deprecated and removed. Please switch to the new provider `keycloak-oidc`.
+- [#1286](https://github.com/oauth2-proxy/oauth2-proxy/pull/1286) Add the `allowed_email_domains` and the `allowed_groups` on the `auth_request` endpoint to restrict access per ingress + support standard wildcard char for validation with sub-domain. (@w3st3ry @armandpicard)


This should be moved into the changes since v7.1.3 section and typically should just be the PR title, we can leave this here if you add the extra one to the changes since section

JoelSpeed · 2021-10-16T16:48:09Z

pkg/validation/utils.go

@@ -9,3 +16,45 @@ func prefixValues(prefix string, values ...string) []string {
 	}
 	return msgs
 }
+
+func IsEndpointAllowed(endpoint *url.URL, allowedDomains []string) bool {


I actually don't think this is the right place to put this function, the validation package is all about configuration validation rather than runtime validation like this. Can we move it to pkg/utils until we find a better home please

JoelSpeed · 2021-10-16T16:49:02Z

pkg/validation/utils.go

+			// if the whitelisted domain's port is '*', allow all ports
+			// if the whitelisted domain contains a specific port, only allow that port
+			// if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https


We are trying to move away from this term when we touch code that uses it

Suggested change

// if the whitelisted domain's port is '*', allow all ports

// if the whitelisted domain contains a specific port, only allow that port

// if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https

// if the allowed domain's port is '*', allow all ports

// if the allowed domain contains a specific port, only allow that port

// if the allowed domain doesn't contain a port at all, only allow empty redirect ports ie http and https

w3st3ry · 2021-10-28T15:04:38Z

@JoelSpeed @NickMeves I pushed the latest requested changes + rebased to master/fixed conflicts.

JoelSpeed

Changes look great, I think there's a conflict in the changelog now though. We are planning to do a bug release in the next couple of weeks, if you don't mind holding until we've done that release, then rebasing this to fix the changelog, that would be appreciated. Then we can get you merged as soon as we are open for features

github-actions · 2022-02-05T00:03:45Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

JoelSpeed · 2022-02-06T12:57:05Z

Apologies for letting this get stale, are you able to resolve the conflicts and we can get this merged?

JoelSpeed

Looks good, apologies for the delay, lets get this merged

JoelSpeed · 2022-02-14T12:57:01Z

@armandpicard GitHub seems to think there are conflicts on this branch, do you think you could try rebasing it?

w3st3ry · 2022-02-14T14:55:44Z

@JoelSpeed conflicts are now fixed + branch rebased.

JoelSpeed · 2022-02-14T16:26:08Z

CHANGELOG.md

@@ -37,6 +37,7 @@ N/A
 - [#1444](https://github.com/oauth2-proxy/oauth2-proxy/pull/1444) Update LinkedIn provider validate URL (@jkandasa)
 - [#1471](https://github.com/oauth2-proxy/oauth2-proxy/pull/1471) Update alpine to 3.15 (@AlexanderBabel)
 - [#1479](https://github.com/oauth2-proxy/oauth2-proxy/pull/1479) Update to Go 1.17 (@polarctos)
+- [#1286](https://github.com/oauth2-proxy/oauth2-proxy/pull/1286) Add the `allowed_email_domains` and the `allowed_groups` on the `auth_request` + support standard wildcard char for validation with sub-domain and email-domain. (@w3st3ry @armandpicard)


Sorry, this should be moved up to the Changes since v7.2.1 section now, closer to line 9

@JoelSpeed my bad, fixed!

…est endpoint + support standard wildcard char for validation with sub-domain and email-domain. Signed-off-by: Valentin Pichard <github@w3st.fr>

JoelSpeed

LGTM, thanks

armandpicard marked this pull request as ready for review July 28, 2021 09:19

armandpicard requested a review from a team as a code owner July 28, 2021 09:19

NickMeves reviewed Aug 7, 2021

View reviewed changes

NickMeves requested changes Aug 7, 2021

View reviewed changes

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch 4 times, most recently from 10e6c24 to 16dd3fb Compare August 13, 2021 10:39

armandpicard requested a review from NickMeves August 13, 2021 10:55

NickMeves reviewed Sep 5, 2021

View reviewed changes

docs/docs/features/endpoints.md Outdated Show resolved Hide resolved

NickMeves reviewed Sep 5, 2021

View reviewed changes

docs/docs/features/endpoints.md Outdated Show resolved Hide resolved

NickMeves reviewed Sep 5, 2021

View reviewed changes

pkg/util/util.go Show resolved Hide resolved

NickMeves reviewed Sep 5, 2021

View reviewed changes

JoelSpeed reviewed Sep 17, 2021

View reviewed changes

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch 6 times, most recently from f60ec03 to eda65c6 Compare October 5, 2021 21:11

JoelSpeed reviewed Oct 16, 2021

View reviewed changes

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch 3 times, most recently from 170cb53 to b7bd0c1 Compare October 28, 2021 14:59

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch from b7bd0c1 to 22b70d8 Compare October 28, 2021 15:01

JoelSpeed previously approved these changes Nov 19, 2021

View reviewed changes

JoelSpeed added the enhancement label Nov 19, 2021

JoelSpeed added this to In progress in 7.3.0 via automation Nov 19, 2021

w3st3ry dismissed JoelSpeed’s stale review via b6454c9 December 6, 2021 18:09

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch from 22b70d8 to b6454c9 Compare December 6, 2021 18:09

github-actions bot added the Stale label Feb 5, 2022

JoelSpeed removed the Stale label Feb 6, 2022

JoelSpeed previously approved these changes Feb 14, 2022

View reviewed changes

JoelSpeed requested a review from NickMeves February 14, 2022 12:56

w3st3ry dismissed JoelSpeed’s stale review via 299e51c February 14, 2022 14:53

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch from b6454c9 to 299e51c Compare February 14, 2022 14:53

JoelSpeed reviewed Feb 14, 2022

View reviewed changes

Add the allowed_email_domains and the allowed_groups on the auth_requ…

2b4c8a9

…est endpoint + support standard wildcard char for validation with sub-domain and email-domain. Signed-off-by: Valentin Pichard <github@w3st.fr>

w3st3ry force-pushed the allowed_email_domains-on-auth_request-endpoint branch from 299e51c to 2b4c8a9 Compare February 14, 2022 17:03

JoelSpeed approved these changes Feb 14, 2022

View reviewed changes

JoelSpeed merged commit 263a5df into oauth2-proxy:master Feb 17, 2022

7.3.0 automation moved this from In progress to Done Feb 17, 2022

w3st3ry deleted the allowed_email_domains-on-auth_request-endpoint branch February 21, 2022 14:45

JoelSpeed mentioned this pull request Dec 23, 2022

Added AllowedGroupsHeader option #1727

Closed

3 tasks

		@@ -961,15 +961,57 @@ func (p OAuthProxy) getAuthenticatedSession(rw http.ResponseWriter, req http.R
		//
		//nolint:gosimple

	emailSplited := strings.Split(s.Email, "@")
	splitEmail := strings.Split(s.Email, "@")

	func isHostnameBelongsToAllowedHost(hostname, allowedHost string) bool {
	func isHostnameAllowed(hostname, allowedHost string) bool {

		- [#1210](https://github.com/oauth2-proxy/oauth2-proxy/pull/1210) A new `keycloak-oidc` provider has been added with support for role based authentication. The existing keycloak auth provider will eventually be deprecated and removed. Please switch to the new provider `keycloak-oidc`.
		- [#1286](https://github.com/oauth2-proxy/oauth2-proxy/pull/1286) Add the `allowed_email_domains` and the `allowed_groups` on the `auth_request` endpoint to restrict access per ingress + support standard wildcard char for validation with sub-domain. (@w3st3ry @armandpicard)

Add allowed_email_domains on auth_request endpoint #1286

Add allowed_email_domains on auth_request endpoint #1286

Conversation

armandpicard commented Jul 28, 2021 • edited

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves Aug 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves left a comment

Choose a reason for hiding this comment

w3st3ry commented Sep 2, 2021

NickMeves Sep 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMeves commented Sep 26, 2021

w3st3ry commented Oct 5, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w3st3ry commented Oct 28, 2021

JoelSpeed left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 5, 2022

JoelSpeed commented Feb 6, 2022

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed commented Feb 14, 2022

w3st3ry commented Feb 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

armandpicard commented Jul 28, 2021 •

edited

NickMeves Aug 7, 2021 •

edited

NickMeves Sep 5, 2021 •

edited