Add -skip-oidc-discovery option #41

marratj · 2019-02-05T16:01:59Z

Description

This PR adds the option to skip OIDC discovery by go-oidc and instead manually configure Authorization, Token and JWKS URI endpoints.

Motivation and Context

Fixes #27

Some OIDC providers either do not have a discovery endpoint (non-standard Enterprise solutions) or require additional URL parameters for specific user journey policies to be supplied (Microsoft Azure AD B2C).

go-oidc calls the fixed /.well-known/openid-configuration endpoint of the configured OIDC issuer. When there is no issuer or when there are multiple policies defined via additional parameters, this method cannot be relied upon.

Fortunately, go-oidc provides the possibility to use a custom HTTP client via a client context. With this, we supply go-oidc with a "fake" HTTP client that always returns the manually configured Auth,Token and JWKS endpoints in a JSON response when it calls the discovery endpoint.

How Has This Been Tested?

I tested this with Microsoft Azure AD B2C as OIDC Provider which uses an additional URL parameter for custom User Journeys. Manually setting the OIDC endpoints to a particular policy returns the proper user journey

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.

marratj · 2019-02-05T16:03:16Z

@tlawrie care to test this PR in your environment?

options.go

JoelSpeed

Please see my comment about rewriting this using the suggestion from go-oidc.

Would you be able to add some documentation on manually configuring the OIDC provider to the ReadMe please? Also, please add a suitable note to the changelog

JoelSpeed · 2019-02-06T10:17:04Z

options.go

@@ -70,6 +73,9 @@ type Options struct {
 	// potential overrides.
 	Provider          string `flag:"provider" cfg:"provider"`
 	OIDCIssuerURL     string `flag:"oidc-issuer-url" cfg:"oidc_issuer_url"`
+	SkipOidcDiscovery bool   `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`
+	OIDCUserInfoURL   string `flag:"oidc-userinfo-url" cfg:"oidc_userinfo_url"`


I don't think this is needed? Is the userinfo URL actually used anywhere?

agreed, have removed it

JoelSpeed · 2019-02-06T10:17:38Z

options.go

@@ -70,6 +73,9 @@ type Options struct {
 	// potential overrides.
 	Provider          string `flag:"provider" cfg:"provider"`
 	OIDCIssuerURL     string `flag:"oidc-issuer-url" cfg:"oidc_issuer_url"`
+	SkipOidcDiscovery bool   `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`


Can we capitalise OIDC to be consistent

Suggested change

SkipOidcDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`

SkipOIDCDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`

agreed, have changed it

marratj · 2019-02-20T09:36:33Z

rebased to master, using oidc.NewVerifier now (which was actually pretty simple and already documented with an example in go-oidc itself).

@tlawrie care to test the new changes?

tlawrie · 2019-02-20T09:44:22Z

Fantastic. Myself and my colleague @costelmoraru will test

JoelSpeed

One minor nit then good to go

JoelSpeed · 2019-02-20T10:08:37Z

options.go

+
+		ctx := context.Background()
+
+		// Override discoverable provider data with a custom http.Client that fakes


This comment is now out of date, could you update it please

marratj · 2019-02-25T07:42:33Z

@tlawrie did you have any chance to test yet?
@JoelSpeed when @tlawrie finished his tests, this should be good to merge?

Co-Authored-By: marratj <marrat@marrat.de>

JoelSpeed

Yep, I think this looks good, I'm out of office all week so definitely won't have time to test it myself so if someone else could verify the changes that would be good!

costelmoraru · 2019-02-25T14:13:45Z

Hey @marratj , we tested out the -skip-oidc-discovery option on a ICP 2.3.0.1 and ICP 3.1.1 and it works correctly.

Thank you for your help and contribution.

marratj · 2019-03-04T10:45:58Z

@JoelSpeed @costelmoraru @tlawrie

I guess we are good to go then?

JoelSpeed · 2019-03-04T13:54:52Z

@marratj Thanks for your work on this! 🎉

* added karrieretutor go-oidc fork for using an AAD B2C Policy * added karrieretutor go-oidc fork for using an AAD B2C Policy * added --skip-oidc-discovery option * added --skip-oidc-discovery option * add simple test for skip-oidc-discovery option * revert Dockerfile to pusher upstream * revert Dockerfile to pusher upstream * remove karrieretutor b2c option leftover * remove karrieretutor b2c option leftover * Fix typo (missing letters) Co-Authored-By: marratj <marrat@marrat.de> * Fix typo (missing letters) Co-Authored-By: marratj <marrat@marrat.de> * replace fake http client with NewProvider() from go-oidc * remove OIDC UserInfo URL option (not required) * add info about -skip-oidc-discovery to README * add note to changelog * Update outdated comment

This reverts commit 8816a2a.

harindaka · 2019-07-31T04:07:55Z

@marratj does this mean oauth2_proxy fully supports Azure AD B2C now? Is there any documentation on how I can setup oauth2_proxy to work with B2C? Or should I just refer the documentation for the Azure AD provider at https://pusher.github.io/oauth2_proxy/auth-configuration#azure-auth-provider

marratj · 2019-07-31T06:34:46Z

@harindaka yes, this fully works now, provider to use is the OIDC one, here is a config that we use:

provider = "oidc"
oidc_issuer_url = "https://login.microsoftonline.com/{tenant-id}/v2.0/"
scope = "{B2C-app-id} openid offline_access"

skip_oidc_discovery = true
login_url = "https://login.microsoftonline.com/{tenant-id}/oauth2/v2.0/authorize?p=b2c_1a_policyname"
redeem_url = "https://login.microsoftonline.com/{tenant-id}/oauth2/v2.0/token?p=b2c_1a_policyname"
oidc_jwks_url = "https://login.microsoftonline.com/{tenant-id}/discovery/v2.0/keys?p=b2c_1a_policyname"

For {tenant-id}, use the ID of the tenant (the long UUID one), not the tenant domain (although the latter should work, but we had some issues with that in the past).

marratj requested a review from a team February 5, 2019 16:02

aslafy-z reviewed Feb 5, 2019

View reviewed changes

options.go Outdated Show resolved Hide resolved

JoelSpeed reviewed Feb 6, 2019

View reviewed changes

marratj mentioned this pull request Feb 8, 2019

Current OIDC implementation requires provider to implement the /.well-known/openid-configuration endpoint #27

Closed

marratj force-pushed the manual-oidc branch from c2b6285 to f0bf2ae Compare February 20, 2019 09:35

JoelSpeed reviewed Feb 20, 2019

View reviewed changes

ploxiln mentioned this pull request Feb 20, 2019

Microsoft Azure AD B2C Support ploxiln/oauth2_proxy#9

Closed

marratj and others added 16 commits February 25, 2019 08:43

added karrieretutor go-oidc fork for using an AAD B2C Policy

b9c1dea

added --skip-oidc-discovery option

09feba6

add simple test for skip-oidc-discovery option

16f9632

revert Dockerfile to pusher upstream

d7fda79

remove karrieretutor b2c option leftover

dcf42f9

Fix typo (missing letters)

f89a779

Co-Authored-By: marratj <marrat@marrat.de>

added karrieretutor go-oidc fork for using an AAD B2C Policy

8df3d08

added --skip-oidc-discovery option

2f4c3ec

revert Dockerfile to pusher upstream

8b8a5e7

remove karrieretutor b2c option leftover

0d45c4e

Fix typo (missing letters)

81f3475

Co-Authored-By: marratj <marrat@marrat.de>

replace fake http client with NewProvider() from go-oidc

02bea6d

remove OIDC UserInfo URL option (not required)

084caf5

add info about -skip-oidc-discovery to README

f8df27f

add note to changelog

68b49bc

Update outdated comment

fea2e3b

marratj force-pushed the manual-oidc branch from d50dbe4 to fea2e3b Compare February 25, 2019 07:44

JoelSpeed reviewed Feb 25, 2019

View reviewed changes

JoelSpeed approved these changes Mar 4, 2019

View reviewed changes

JoelSpeed merged commit 8816a2a into oauth2-proxy:master Mar 4, 2019

ploxiln mentioned this pull request Mar 4, 2019

OIDC provider: add -skip-oidc-discovery option ploxiln/oauth2_proxy#21

Merged

MXClyde added a commit to MXClyde/oauth2_proxy that referenced this pull request Apr 2, 2019

Revert "Add -skip-oidc-discovery option (oauth2-proxy#41)"

7e7f742

This reverts commit 8816a2a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add -skip-oidc-discovery option #41

Add -skip-oidc-discovery option #41

marratj commented Feb 5, 2019 •

edited

marratj commented Feb 5, 2019

JoelSpeed left a comment

JoelSpeed Feb 6, 2019

marratj Feb 20, 2019

JoelSpeed Feb 6, 2019

marratj Feb 20, 2019

marratj commented Feb 20, 2019

tlawrie commented Feb 20, 2019

JoelSpeed left a comment

JoelSpeed Feb 20, 2019

marratj Feb 20, 2019

marratj commented Feb 25, 2019

JoelSpeed left a comment

costelmoraru commented Feb 25, 2019

marratj commented Mar 4, 2019

JoelSpeed commented Mar 4, 2019

harindaka commented Jul 31, 2019

marratj commented Jul 31, 2019

	SkipOidcDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`
	SkipOIDCDiscovery bool `flag:"skip-oidc-discovery" cfg:"skip_oidc_discovery"`


		ctx := context.Background()

		// Override discoverable provider data with a custom http.Client that fakes

Add -skip-oidc-discovery option #41

Add -skip-oidc-discovery option #41

Conversation

marratj commented Feb 5, 2019 • edited

Description

Motivation and Context

How Has This Been Tested?

Checklist:

marratj commented Feb 5, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Feb 6, 2019

Choose a reason for hiding this comment

marratj Feb 20, 2019

Choose a reason for hiding this comment

JoelSpeed Feb 6, 2019

Choose a reason for hiding this comment

marratj Feb 20, 2019

Choose a reason for hiding this comment

marratj commented Feb 20, 2019

tlawrie commented Feb 20, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

JoelSpeed Feb 20, 2019

Choose a reason for hiding this comment

marratj Feb 20, 2019

Choose a reason for hiding this comment

marratj commented Feb 25, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

costelmoraru commented Feb 25, 2019

marratj commented Mar 4, 2019

JoelSpeed commented Mar 4, 2019

harindaka commented Jul 31, 2019

marratj commented Jul 31, 2019

marratj commented Feb 5, 2019 •

edited