Add userinfo_endpoint #1454

mdbraber · 2019-05-27T09:20:30Z

Supersedes #1453

mdbraber · 2019-05-27T09:28:27Z

@srenatus would be great if you could take a look at this (build checks out)!

srenatus · 2019-05-28T07:23:50Z

I'll try to get this in this week 🤞

srenatus · 2019-06-03T08:34:13Z

Sorry about that. This week it is, then. 😅

srenatus

This is looking good so far -- thank you!

Besides my inline commits, a few points of note:

Would you mind throwing in a test or three for the new HTTP handler, please?
@jackielii: Are you OK with @mdbraber adopting your PR? @mdbraber could you add a Co-authored-by line with @jackielii on the relevant commits, please? (Or massage the history to include the commit of A simple implementation of userinfo endpoint #1201?)
Oh this has quite some history.... @fjbsantiago had a proposal up, too, in Added User Info endpoint #1133. I suppose we'll better put a bunch of hat-tips into the merge commit 🤔

Thanks again! 😃

srenatus · 2019-06-05T16:38:53Z

server/oauth2.go

@@ -265,6 +265,11 @@ type federatedIDClaims struct {
 	UserID      string `json:"user_id,omitempty"`
 }

+func (s *Server) newAccessToken(clientID string, claims storage.Claims, scopes []string, nonce, connID string) (accessToken string, err error) {
+	idToken, _, err := s.newIDToken(clientID, claims, scopes, nonce, storage.NewID(), connID)


Hmm! So we'll be using a JWT for access_token... but it's a different one; we don't hand out the same we give as id_token. 💭

Maybe @jackielii can answer this one?

Like in my original PR comment: "this is a stop gap solution". In my understanding, access_token is a "key" that should be saved away and later used to retrieve "user_info". Here I'm abusing the ID token signature verification: Use an ID token as the access token, and use signature verification to verify the JWT and just return the claims as the user_info response.

Because the access_token is opaque to the user, so this change is forward compatible with the "proper solution". I even created this function to later swap out for the proper "save to db" solution.

@srenatus sorry forgot to answer your original question. Within one auth request, the id_token won't be exactly the same as the access_token, albeit access_token looks and "can" behave like a jwt id_token. We're just abusing the fact that access_token is just a "random string".

Quite clever! And thanks for considering "forward compat with a proper solution" 😄

srenatus · 2019-06-05T16:41:16Z

server/handlers.go

+		return nil, fmt.Errorf("unable to parse signed message")
+	}
+
+	// Parse the message to check expiry, as it jose doesn't distinguish expiry error from others


I don't understand how the comment relates to the code below. 🤔

This might be me - doing some too agressive butchering of the code. Sorry.

srenatus · 2019-06-05T16:41:42Z

server/handlers.go

+	// Parse the message to check expiry, as it jose doesn't distinguish expiry error from others
+	parts := strings.Split(token, ".")
+	if len(parts) != 3 {
+		return nil, fmt.Errorf("square/go-jose: compact JWS format must have three parts")


Also, we should probably not claim we're square/go-jose. We aren't 😄

Well that seems an easy one to fix

mdbraber · 2019-06-05T18:56:49Z

This is looking good so far -- thank you!

Thanks for taking the time to review!

Would you mind throwing in a test or three for the new HTTP handler, please?

I'm not proficient in writing any sort of tests so that'll take me a lot of figuring out, I'm happy to try if someone might have some pointers how to do this.

@jackielii: Are you OK with @mdbraber adopting your PR? @mdbraber could you add a Co-authored-by line with @jackielii on the relevant commits, please? (Or massage the history to include the commit of A simple implementation of userinfo endpoint #1201?)

Well I think @jackielii did most of the work as it's their patch. I only butchered the patch to make it fit the current code base :-) Happy to add a Co-authored line on the relevant commit. Massaging the history seems more work so I'll just do the first.

Oh this has quite some history.... @fjbsantiago had a proposal up, too, in Added User Info endpoint #1133. I suppose we'll better put a bunch of hat-tips into the merge commit 🤔

So it seems, so more hat tips for everyone :-) 🎩

jackielii · 2019-06-05T20:00:04Z

@mdbraber & @srenatus absolutely. Feel free to use my name or otherwise, no probs for me. I was lazy and looking for a solution hastily, and just figured this trick should be the easiest one without changing too much. It utilises existing id_token signature verification mechanism to implement the access_token + user_info verification. It has been in semi-production in our internal system for since I submitted the other PR over a year ago.

BTW, I realised on that my PR wasn't similar to the original #1133 . That one I still couldn't fully understand, and mine definitely took a different approach. But it helped to understand the code base a bit better.

Thanks & Regards,

jackielii · 2019-06-05T20:06:31Z

server/handlers.go

+		return nil, err
+	}
+
+	// TODO: check other claims


This could be a "big" TODO. I realised later on that the jose or any other JWT token lib should do all the checks, but I'm not sure they do all the needed. Or maybe they do, and I'm just being over paranoid here to check again. Need a JWT expert or jose expert here to answer the question.

Co-authored-by: Yuxing Li <[email protected]> Co-authored-by: Francisco Santiago <[email protected]>

mdbraber · 2019-06-05T20:16:53Z

Added the right attribution: added Co-authored-by to @jackielii and @fjbsantiago acknowledging their work.

srenatus · 2019-06-08T17:20:46Z

I'm not proficient in writing any sort of tests so that'll take me a lot of figuring out, I'm happy to try if someone might have some pointers how to do this.

https://github.com/dexidp/dex/blob/master/server/server_test.go has some tests that could be added to or expanded, I suppose. LMK if you need help. 😃

alindeman · 2019-06-17T16:46:49Z

How's this coming along? Is there anything I can do to help? I'd like to see this land in Dex so I can better integrate it with services like AWS ALBs that require userinfo endpoints.

srenatus · 2019-06-18T05:30:42Z

@alindeman This needs tests. If you'd like to contribute, you could PR against mdbraber:add-user-endpoint or a open a new one against dexidp/dex with these commits included. (We can then merge this PR first and rebase the other one then.)

alindeman · 2019-06-18T23:45:34Z

@alindeman This needs tests. If you'd like to contribute, you could PR against mdbraber:add-user-endpoint or a open a new one against dexidp/dex with these commits included. (We can then merge this PR first and rebase the other one then.)

Sounds good. I will plan to tackle adding some tests tomorrow.

This was referenced May 27, 2019

Add userinfo_endpoint #1453

Closed

UserInfo endpoint #376

Closed

mdbraber changed the title ~~Add user endpoint~~ Add userinfo_endpoint May 27, 2019

srenatus mentioned this pull request Jun 4, 2019

make userName configurable #1459

Merged

srenatus reviewed Jun 5, 2019

View reviewed changes

jackielii reviewed Jun 5, 2019

View reviewed changes

mdbraber and others added 4 commits June 5, 2019 22:11

Add userinfo endpoint

a8d059a

Co-authored-by: Yuxing Li <[email protected]> Co-authored-by: Francisco Santiago <[email protected]>

Fix changes

d7750b1

Formatting

74f4e74

Fix comments

3dd1bac

mdbraber force-pushed the add-user-endpoint branch from e6f4c27 to 3dd1bac Compare June 5, 2019 20:15

drekle mentioned this pull request Jun 6, 2019

OIDC groups and userinfo support #1463

Closed

alindeman mentioned this pull request Jun 20, 2019

Add UserInfo endpoint #1473

Merged

srenatus closed this in #1473 Jul 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add userinfo_endpoint #1454

Add userinfo_endpoint #1454

mdbraber commented May 27, 2019

mdbraber commented May 27, 2019

srenatus commented May 28, 2019

srenatus commented Jun 3, 2019

srenatus left a comment

srenatus Jun 5, 2019

mdbraber Jun 5, 2019

jackielii Jun 5, 2019

jackielii Jun 5, 2019

srenatus Jun 7, 2019

srenatus Jun 5, 2019

mdbraber Jun 5, 2019

srenatus Jun 5, 2019

mdbraber Jun 5, 2019

mdbraber commented Jun 5, 2019

jackielii commented Jun 5, 2019

jackielii Jun 5, 2019

mdbraber commented Jun 5, 2019

srenatus commented Jun 8, 2019

alindeman commented Jun 17, 2019

srenatus commented Jun 18, 2019

alindeman commented Jun 18, 2019

Add userinfo_endpoint #1454

Add userinfo_endpoint #1454

Conversation

mdbraber commented May 27, 2019

mdbraber commented May 27, 2019

srenatus commented May 28, 2019

srenatus commented Jun 3, 2019

srenatus 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

mdbraber commented Jun 5, 2019

jackielii commented Jun 5, 2019

Choose a reason for hiding this comment

mdbraber commented Jun 5, 2019

srenatus commented Jun 8, 2019

alindeman commented Jun 17, 2019

srenatus commented Jun 18, 2019

alindeman commented Jun 18, 2019