support for id_token_hint #1125

srenatus · 2017-11-13T10:38:56Z

The spec (see #990) is a bit vague on this. I suppose you could interpret it as follows: id_token_hint SHOULD be there when prompt=none. However, when prompt=none is passed, dex should error out when it cannot fulfill the request in a promptless way. Now, it's obvious that PasswordConnector can't do prompt=none -- for others, however, it's less obvious. It's perhaps possible to "forward" the prompt=none to OIDC connectors, but for SAML AuthnRequests, I'm not sure if that's a thing.

However, what this does is opportunistically select the login provider based on the id_token_hint. If this results in a prompt or not depends on the provider, and we don't promise anything. So, the case here is prompt=none is not set, id_token_hint is OPTIONAL. We make use of that optional hint.

To test-drive this, checkout the branch and start the example-app against a dex instance with the static passwordDB and example-connector, and follow along:

After a Login using Example, there's a button "use ID token hint":
You'll be redirected to /auth/local, and it will log you in without any need for interaction, so you'll be redirected to the callback:
If you start a new process, by logging in from http://127.0.0.1:5555, and select the email provider, clicking "login with id token hint" gets you back straight away to the password prompt:

Now, technically, we'd probably have enough information to pre-fill that username field.

However, there might also be ways to forward the "requested subject" to OIDC and SAML -- and I believe this is what we should do? The edge case I've got in mind is this:

dex gets an auth request with an id_token_hint for alice (using SAML, for example)
the browser is redirected to SAML (authn request, no constraints whatsoever)
the users logs in as bob
the browser returns to dex, it creates a session for bob, as per SAML Assertion.

While this isn't a security hazard, it's potentially a UX issue.

What do you think? I suppose there's two options here:

clean up this PR, leave it as-is
Do (1.), and fix the password connector username pre-fill.
Do (1.), and proceed down the road of request-a-specific-user-to-login-using-any-supported-connector.

(3.) is too much for a single PR, and I cannot commit to getting this done in all connectors.
(2.) seems OK though, but I'd prefer to split this in two PRs.

Ok-ish way to proceed? If so, I'm going to add tests.

TODOs

Tests
Have I screwed up the vendored dependencies?! That change seems way to big for just additionally using go-jose.v2/jwt 🤔 ✅ make revendor, all good now.

ericchiang

Seems reasonable. What do we do if the key's been rotated? Do we really need to verify the signature?

ericchiang · 2017-11-13T23:38:02Z

server/oauth2.go

@@ -20,6 +20,7 @@ import (
 	"time"

 	jose "gopkg.in/square/go-jose.v2"
+	"gopkg.in/square/go-jose.v2/jwt"


we don't need to use the jwt package for this. The jose.ParseSigned works fine https://godoc.org/gopkg.in/square/go-jose.v2#ParseSigned

ericchiang · 2017-11-13T23:38:26Z

server/oauth2.go

@@ -376,6 +377,8 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (req storage.AuthReq
 	scopes := strings.Fields(q.Get("scope"))
 	responseTypes := strings.Fields(q.Get("response_type"))

+	connectorID := s.connectorIDFromIDTokenHint(q.Get("id_token_hint"))


Shouldn't we error if the token hint is incorrect?

OPTIONAL. ID Token previously issued by the Authorization Server being passed as a hint about the End-User's current or past authenticated session with the Client. If the End-User identified by the ID Token is logged in or is logged in by the request, then the Authorization Server returns a positive response; otherwise, it SHOULD return an error. When possible, an id_token_hint SHOULD be present when prompt=none is used and an invalid_request error MAY be returned if it is not; however, the server SHOULD respond successfully when possible, even if it is not present. The Authorization Server need not be listed as an audience of the ID Token when it is used as an id_token_hint value.

Hmm not sure if the spec is clear there. Anyways, I can make this error out if reading the connectorID from the token fails.

ericchiang · 2017-11-13T23:39:18Z

server/oauth2.go

+	// figure out key used for signing this
+	var usedKey string
+	for _, hdr := range tok.Headers {
+		if jose.SignatureAlgorithm(hdr.Algorithm) != jose.RS256 {


In some distant future, I'd like to not hard code jose.RS256 everywhere. Maybe we can take this from a centrally configured place?

ericchiang · 2017-11-13T23:40:51Z

cmd/example-app/main.go

 			return
 		}
+		if idTokenHint != "" {
+			// redirect to auth URL with the hint, using default scopes
+			scopes := []string{"openid", "profile", "email"}


Seems odd that these aren't taken from the first page.

Yeah I was a bit lazy here. I'm going to try to fix this.

It's actually not that simple -- we don't know much of the granted scopes when retrieving the id_token 🤔 Since this is the example app, I haven't spent too much time on implementing this.

np. it's not critical.

srenatus · 2017-11-14T08:40:45Z

Seems reasonable. What do we do if the key's been rotated? Do we really need to verify the signature?

@ericchiang Actually, when it comes to what jose exports, I'm having trouble extracting claims from the payload -- unless I'm verifying the token (since Verify() returns its payload). I'll go look for another way, though 👍

On the issue, I'm torn. I've had another look at the rotation logic (please correct me if I'm wrong): keys get dropped when there's no possibility to have a non-expired ID token signed with that key around.
With this in mind, I suppose checking the signatures is not what we should do.

This opens up the question of what could possibly go wrong? Basically, we're opening up a way for someone (Bob) to send someone else (Cathy) to a certain connector by tricking her into opening /dex/auth?id_token_hint=CRAFTEDIDTOKEN. Now, what would happen there is that Cathy would either be able to login or not. ⏩ I don't think this is too risky. After all, we're only allowing for connector IDs that dex knows (and serves), and we don't redirect anywhere else.

srenatus · 2017-11-14T11:27:31Z

Ok, I've updated the branch. Remaining issues/questions:

It seems that jumping directly to the right connector, if that connector is a password connector, breaks the "back" link (from show "back" link for password connectors #1123), by going back to the example-app, not the page we've been 302-redirected from.
There's some potential for UX improvements from reading the UserID from the id_token_hint, too -- however, I wasn't sure how to put that into storage.AuthRequest. There's a Claims.UserID that might be usable, but I can't tell if this is a terrible thing to do. (Is it?)
- Also, this would affect step (2.) in the list of the top post, so it could be tackled independently.

ericchiang · 2017-11-29T18:24:31Z

It seems that jumping directly to the right connector, if that connector is a password connector, breaks the "back" link (from #1123), by going back to the example-app, not the page we've been 302-redirected from.

This seems like the biggest outstanding concern.

There's some potential for UX improvements from reading the UserID from the id_token_hint, too -- however, I wasn't sure how to put that into storage.AuthRequest. There's a Claims.UserID that might be usable, but I can't tell if this is a terrible thing to do. (Is it?)

We're not guaranteed that the UserID actually corresponds to the username that the user entered to login. In LDAP your username might be "jane" but your userID would be "cn=jane,dn=people,..."

srenatus · 2017-12-04T14:28:44Z

This seems like the biggest outstanding concern.

Alright, I'll take care of this 😄

Signed-off-by: Stephan Renatus <srenatus@chef.io>

This way, we automatically get nicer output that has the test case name prefixed: --- PASS: TestParseAuthorizationRequest (0.01s) --- PASS: TestParseAuthorizationRequest/normal_request (0.00s) --- PASS: TestParseAuthorizationRequest/POST_request (0.00s) --- PASS: TestParseAuthorizationRequest/invalid_client_id (0.00s) --- PASS: TestParseAuthorizationRequest/invalid_redirect_uri (0.00s) --- PASS: TestParseAuthorizationRequest/implicit_flow (0.00s) --- PASS: TestParseAuthorizationRequest/unsupported_response_type (0.00s) --- PASS: TestParseAuthorizationRequest/only_token_response_type (0.00s) Signed-off-by: Stephan Renatus <srenatus@chef.io>

Signed-off-by: Stephan Renatus <srenatus@chef.io>

srenatus · 2017-12-04T15:57:12Z

@ericchiang I'm afraid e8fe811 is a bit of a blunt solution -- but I haven't found a better one. I'd appreciate any input on this.

Also, I'd like to add tests. Currently, it seems like it might make most sense to add some unit tests for handleAuthorization to handlers_test.go? Do you agree? (I'm thinking of simple checks like "server with one connector, it forwards"; "server with two connectors, it doesn't", "server with two connectors + id_token_hint, it forwards".)

deric · 2019-01-22T14:08:03Z

Any progress on this?

srenatus · 2019-01-22T14:31:37Z

@deric I've afraid I've dropped the ball on this. I'll try to pick it up 😅 -- no promises that it'll happen this week, though.

srenatus · 2019-01-22T14:32:02Z

@deric if you'd like to take over, be my guest!

srenatus changed the title ~~[WIP] support for id_token_hint~~ [WIP+question] support for id_token_hint Nov 13, 2017

srenatus force-pushed the sr/id_token_hint branch 2 times, most recently from 6d13ae3 to f3147c2 Compare November 13, 2017 12:54

ericchiang reviewed Nov 13, 2017

View reviewed changes

srenatus force-pushed the sr/id_token_hint branch from f3147c2 to a2423e4 Compare November 14, 2017 11:18

srenatus changed the title ~~[WIP+question] support for id_token_hint~~ support for id_token_hint Nov 14, 2017

srenatus added 5 commits December 4, 2017 15:29

handlers/oauth2: take id_token_hint

9502a33

Signed-off-by: Stephan Renatus <srenatus@chef.io>

example-app: add id_token_hint "support"

326064f

Signed-off-by: Stephan Renatus <srenatus@chef.io>

oauth2_test: add tests for id_token_hint functionality

1e82ba5

Signed-off-by: Stephan Renatus <srenatus@chef.io>

handlers/auth: don't display backlink when passing backlink=none

e8fe811

Signed-off-by: Stephan Renatus <srenatus@chef.io>

srenatus force-pushed the sr/id_token_hint branch from a2423e4 to e8fe811 Compare December 4, 2017 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for id_token_hint #1125

support for id_token_hint #1125

srenatus commented Nov 13, 2017 •

edited

Loading

ericchiang left a comment

ericchiang Nov 13, 2017

ericchiang Nov 13, 2017

srenatus Nov 14, 2017

ericchiang Nov 13, 2017

ericchiang Nov 13, 2017

srenatus Nov 14, 2017

srenatus Nov 14, 2017

ericchiang Nov 14, 2017

srenatus commented Nov 14, 2017 •

edited

Loading

srenatus commented Nov 14, 2017 •

edited

Loading

ericchiang commented Nov 29, 2017

srenatus commented Dec 4, 2017

srenatus commented Dec 4, 2017

deric commented Jan 22, 2019

srenatus commented Jan 22, 2019

srenatus commented Jan 22, 2019

support for id_token_hint #1125

Are you sure you want to change the base?

support for id_token_hint #1125

Conversation

srenatus commented Nov 13, 2017 • edited Loading

TODOs

ericchiang 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

srenatus commented Nov 14, 2017 • edited Loading

srenatus commented Nov 14, 2017 • edited Loading

ericchiang commented Nov 29, 2017

srenatus commented Dec 4, 2017

srenatus commented Dec 4, 2017

deric commented Jan 22, 2019

srenatus commented Jan 22, 2019

srenatus commented Jan 22, 2019

srenatus commented Nov 13, 2017 •

edited

Loading

srenatus commented Nov 14, 2017 •

edited

Loading

srenatus commented Nov 14, 2017 •

edited

Loading