Added OIDC support for UserInfo Endpoint Email Verification #481

dcwangmit01 · 2017-10-19T23:39:50Z

Current OIDC implementation asserts that user email check must come
from JWT token claims. OIDC specification also allows for source
of user email to be fetched from userinfo profile endpoint.
http://openid.net/specs/openid-connect-core-1_0.html#UserInfo
Modified current code to search for email in JWT token claims
first, and then fall back to requesting email from userinfo
endpoint.

dcwangmit01 · 2017-10-19T23:42:44Z

I noticed that the two of you were responsible for adding the most recent OIDC provider. I was hoping for feedback on this patch which was required to make the OIDC provider work for me. I'm new to OIDC/oauth, so feedback is appreciated.

Thanks!
-dave

ericchiang

UserInfo endpoint is optional.[1] Not all providers support it. Why not get the email from the id_token?

[1] https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata

ericchiang · 2017-10-20T00:05:47Z

providers/provider_default.go

-	return a.String()
+
+	// Default golang libs Encode space( ) as plus (+) instead of '%20'
+	// which is more proper.  Some OpenID servers do not accept this


Which providers?

I'm integrating with the Ping Federate OIDC provider. For some reason, their API is not lenient on this. It'll totally fail with '+' as space. It's totally a purist golang library encoding thing. Check out golang/go#4013

Regarding the Email from JWT claims...

I would have preferred JWT, which would have worked with less code, but I don't have access to the Ping Federate admin panel to enable the extensions that set the openid claims in the JWT token. The documentation for the Ping Federate provider is here.

Regardless, I think the OIDC spec allows for email to be returned in either JWT claims OR the UserInfo endpoint. The code changes involving the UserInfo endpoint follow the same pattern as the LinkedIn, Gitlab, and Azure providers.

I'm going to remove the rewriting the "+" signs in the next commit. I'll put that in a separate PR as a command line option, in hopes of increasing the chances of this PR getting merged.

dcwangmit01 · 2017-10-20T17:30:25Z

Adding this comment hidden by code updates...

I think the OIDC spec allows for email to be returned in either JWT claims OR the UserInfo endpoint. The code changes involving the UserInfo endpoint follow the same pattern as the LinkedIn, Gitlab, and Azure providers. It first searches JWT token for the email. If it cannot find it in the JWT token, it falls back to searching the UserInfo endpoint.

ericchiang · 2017-10-20T17:33:54Z

providers/oidc.go

+	if s.AccessToken == "" {
+		return "", errors.New("missing access token")
+	}
+	req, err := http.NewRequest("GET", p.ProfileURL.String(), nil)


How is ProfileURL set? I would have expected this to use the discovery endpoint https://accounts.google.com/.well-known/openid-configuration

Currently, the profile_url is hard-coded in each provider.

Facebook: https://github.com/bitly/oauth2_proxy/blob/master/providers/facebook.go#L32
Azure: https://github.com/bitly/oauth2_proxy/blob/master/providers/azure.go#L22
LinkedIn: https://github.com/bitly/oauth2_proxy/blob/master/providers/linkedin.go#L29

For the OIDC provider in its current state (before I submitted this PR... I left it untouched), the profile_url is untouched. In fact, it defaults to empty (""). I feed it in currently through the command line option:

-profile-url string Profile access endpoint

ericchiang · 2017-10-20T17:34:17Z

providers/oidc.go

@@ -57,10 +62,10 @@ func (p *OIDCProvider) Redeem(redirectURL, code string) (s *SessionState, err er
 	}

 	if claims.Email == "" {
-		return nil, fmt.Errorf("id_token did not contain an email")
+		log.Printf("id_token did not contain an email. Falling back on userinfo endpoint\n")


Perform the request to the user info endpoint here.

Okay. I have no problem changing it.

I think google does it the way that you're suggeting, but they have full control over where email shows up. It will show up only in the token and no other way (as in no userinfo endpoint).

The other providers don't seem to check email in their Redeem functions. They rely on the Redeem function not returning an email, and then common oauthproxy.go:redeemCode function to notice and then hit the UserInfo endpoint.

I kind of think both places are equivalent, and I'm happy to change it if you prefer it here. Let me know your preference (after this additional information), and I can change it on Monday.

Thanks @ericchiang , for the review!

* Current OIDC implementation asserts that user email check must come from JWT token claims. OIDC specification also allows for source of user email to be fetched from userinfo profile endpoint. http://openid.net/specs/openid-connect-core-1_0.html#UserInfo * Modified current code to search for email in JWT token claims first, and then fall back to requesting email from userinfo endpoint. * Modified URL encoding to use '%20' instead of '+' for encoding spaces, which is more proper. Some OpenID servers do not accept '+' encoding for the http query arg "scope" (e.g. bad: "openid+profile+email" good: "openid%20profilie%20email")

@ericchiang

* The change was requested by @ericchiang in the pull request comments to PR481.

dcwangmit01 · 2018-01-06T07:47:58Z

Hi All,

I'd still like to get this pull request in. I've made the changes that @ericchiang had requested. Sorry it took a few months.

This PR has been rebased on the latest code.

-dave

dcwangmit01 · 2018-01-17T23:45:11Z

Hi @jehiah @ericchiang,

Would you mind having a look to review?

Thanks!
-dave

dcwangmit01 · 2018-05-16T04:25:41Z

@ericchiang I made the changes you've requested. Would you mind reviewing?

Kaszaq · 2018-06-11T12:49:13Z

Hello @jehiah @ericchiang,
Can this please be merged and released?

This does resolve my issue:
#615

Thanks

dcwangmit01 · 2018-06-20T21:00:03Z

Hi @jehiah @mbland @ploxiln,

Would any of you mind reviewing this pull request and potentially merging it? A few people have used this patch successfully. My teams have been using it in multiple deployments successfully for several months now. We'd love to get rid of our fork.

Thanks!

ploxiln · 2018-06-20T21:05:44Z

@mbland and I do not have permissions to merge in this repo, sorry

dcwangmit01 force-pushed the oidc_userinfo_support branch 2 times, most recently from 5f42379 to 4f82de3 Compare October 20, 2017 00:06

ericchiang reviewed Oct 20, 2017

View reviewed changes

dcwangmit01 force-pushed the oidc_userinfo_support branch from 4f82de3 to e17f3e3 Compare October 20, 2017 00:13

ericchiang reviewed Oct 20, 2017

View reviewed changes

jehiah added the enhancement label Oct 23, 2017

jehiah added this to the v2.3 milestone Oct 23, 2017

dcwangmit01 and others added 4 commits January 3, 2018 10:07

Fixing go vet error

5213b7a

Removed url rewrite of '+'

3e112ea

Moved OIDC userinfo email lookup directly into oidc:Redeem function

eb5b18e

* The change was requested by @ericchiang in the pull request comments to PR481.

dcwangmit01 force-pushed the oidc_userinfo_support branch from e277b66 to eb5b18e Compare January 6, 2018 07:45

joejulian mentioned this pull request Mar 2, 2018

Added OIDC support for UserInfo Endpoint Email Verification joejulian/oauth2_proxy#2

Merged

dcwangmit01 mentioned this pull request Jul 16, 2018

Oidc userinfo support cisco-sso/oauth2_proxy#2

Closed

rtluckie mentioned this pull request May 3, 2019

Add OIDC support for UserInfo Endpoint Email Verification oauth2-proxy/oauth2-proxy#145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OIDC support for UserInfo Endpoint Email Verification #481

Added OIDC support for UserInfo Endpoint Email Verification #481

dcwangmit01 commented Oct 19, 2017 •

edited

Loading

dcwangmit01 commented Oct 19, 2017 •

edited

Loading

ericchiang left a comment

ericchiang Oct 20, 2017

dcwangmit01 Oct 20, 2017

dcwangmit01 Oct 20, 2017

dcwangmit01 Oct 20, 2017

dcwangmit01 commented Oct 20, 2017

ericchiang Oct 20, 2017

dcwangmit01 Oct 20, 2017

ericchiang Oct 20, 2017

dcwangmit01 Oct 20, 2017 •

edited

Loading

dcwangmit01 commented Jan 6, 2018 •

edited

Loading

dcwangmit01 commented Jan 17, 2018

dcwangmit01 commented May 16, 2018

Kaszaq commented Jun 11, 2018

dcwangmit01 commented Jun 20, 2018

ploxiln commented Jun 20, 2018

Added OIDC support for UserInfo Endpoint Email Verification #481

Are you sure you want to change the base?

Added OIDC support for UserInfo Endpoint Email Verification #481

Conversation

dcwangmit01 commented Oct 19, 2017 • edited Loading

dcwangmit01 commented Oct 19, 2017 • edited Loading

ericchiang left a comment

Choose a reason for hiding this comment

ericchiang Oct 20, 2017

Choose a reason for hiding this comment

dcwangmit01 Oct 20, 2017

Choose a reason for hiding this comment

dcwangmit01 Oct 20, 2017

Choose a reason for hiding this comment

dcwangmit01 Oct 20, 2017

Choose a reason for hiding this comment

dcwangmit01 commented Oct 20, 2017

ericchiang Oct 20, 2017

Choose a reason for hiding this comment

dcwangmit01 Oct 20, 2017

Choose a reason for hiding this comment

ericchiang Oct 20, 2017

Choose a reason for hiding this comment

dcwangmit01 Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

dcwangmit01 commented Jan 6, 2018 • edited Loading

dcwangmit01 commented Jan 17, 2018

dcwangmit01 commented May 16, 2018

Kaszaq commented Jun 11, 2018

dcwangmit01 commented Jun 20, 2018

ploxiln commented Jun 20, 2018

dcwangmit01 commented Oct 19, 2017 •

edited

Loading

dcwangmit01 commented Oct 19, 2017 •

edited

Loading

dcwangmit01 Oct 20, 2017 •

edited

Loading

dcwangmit01 commented Jan 6, 2018 •

edited

Loading