feat: implement RFC 8628 #3912

nsklikas · 2024-12-18T09:08:55Z

This PR is a continuation of #3851. I have created it from my own personal repo and I have invited people from Ory to contribute so that we can speed up things. I think that most of the comments in the old PR were resolved, but I can copy them to this PR if we wish to keep the discussion history.

Implements the Device Authorization Grant to enable authentication for headless machines (see https://datatracker.ietf.org/doc/html/rfc8628)

Related issue(s)

Implements RFC 8628.

This PR is based on the work done on #3252, by @supercairos and @BuzzBumbleBee. That PR was based on an older version of Hydra and was missing some features/tests.

We have prepared a spec, that describes our design and implementation. We have tried to mimic the existing logic in Hydra and not make changes that would disrupt the existing workflows

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

Notes:

The current implementation has been manually tested only for memory and postgres databases. The tests pass all of them.
Fosite is installed from our fork to ease testing. Once the relevant PR in fosite is merged, we will update go.mod.

Testing

To test this you need to built the hydra image:

make docker

This will create an image with the name: oryd/hydra:latest-sqlite

To run the flow you can use our UI, from https://github.com/canonical/identity-platform-login-ui/tree/hydra-device-test:

git clone [email protected]:canonical/identity-platform-login-ui.git -b hydra-device-test
cd identity-platform-login-ui/
# The image name is hard-coded in the docker-compose file
docker compose up --remove-orphans --force-recreate -d

Create a client for Hydra:

docker exec -it identity-platform-login-ui-hydra-1 hydra create client   --endpoint http://localhost:4445   --grant-type authorization_code,refresh_token,urn:ietf:params:oauth:grant-type:device_code --scope openid,offline_access,email,profile --token-endpoint-auth-method client_secret_post

Use that client to perform the device flow:

docker exec -it identity-platform-login-ui-hydra-1 hydra perform device-code --client-id <client-id> --client-secret <client-secret> -e http://localhost:4444 --scope openid,offline_access,email,profile

The user for logging in is:

username: [email protected]
password: test

aeneasr

Very nice! I'll push some minor changes from my side and left a couple of comments.

I primarily cleaned up error handling to make it standards compliant

aeneasr · 2025-01-16T11:17:13Z

consent/handler.go

+		h.r.Writer().WriteError(w, r, errorsx.WithStack(fosite.ErrNotFound.WithWrap(err).WithHint(`'user_code' session not found`)))
+		return
+	}
+	err = h.r.RFC8628HMACStrategy().ValidateUserCode(ctx, userCodeRequest, reqBody.UserCode)


reqBody.UserCode is not actually being validated by validateusercode. We can probably remove it? Or should it be validated?

As discussed in the fosite PR, this method makes sure that the user_code has not expired. The authenticity of the code has been verified by the call to GetUserCodeSession above.

aeneasr · 2025-01-16T11:17:41Z

consent/handler.go

+		q := reqURL.Query()
+		q.Add("client_id", userCodeRequest.GetClient().GetID())
+		reqURL.RawQuery = q.Encode()
+	}


Should we not always use the client from the usercode request? Why is it possible to override the client here? This is a bit confusing to me as to why we need this.

The initial request should not contain the client_id. We don't know the client_id when the user gets to the device verification endpoint, we need to fetch the user_code from the database to get it.

The only reason we are adding the client_id to the request is to make the existing endpoints work without too many changes. The client_id is parsed in https://github.com/nsklikas/hydra/blob/canonical-master/consent/strategy_default.go#L1211 to construct an authorization request that will be used to do the rest of the flow. If we didn't have this hack, we would have to refactor part of the existing default_strategy. IMHO this is work that should be done after this PR is merged.

I will add a comment to make this clearer.

aeneasr · 2025-01-16T11:20:29Z

consent/handler_test.go

+	require.Contains(t, result.RedirectTo, "device_verifier")
+}
+
+func TestAcceptDuplicateDeviceRequest(t *testing.T) {


Is this testing if we can accept the same device challenge twice? If so, we need two tests:

Test if we can accept the same device challenge twice and that the device verifier is valid

Test if we can accept the device challenge once, use it, then try again, and fail

I think that 1 is handled by this test

About 2, a device challenge can be used multiple times. The reason for that is that only the consent challenge ID is unique in the database. Multiple flows with the same device challenge can be persisted in the database, as long as they have a different consent challenge ID and are associated with a different user_code. I think that the same is true for the login_challenge.

Do you think that the device_challenge should be unique?

aeneasr · 2025-01-16T11:20:58Z

consent/handler_test.go

+			validateResponse: func(resp *http.Response) {
+				require.EqualValues(t, http.StatusBadRequest, resp.StatusCode)
+			},
+		},


Maybe you can add a test case here where a device auth code is already used

aeneasr · 2025-01-16T11:21:30Z

consent/strategy_default.go

@@ -41,6 +41,7 @@ import (
 )

 const (
+	DeviceVerificationPath      = "/oauth2/device/verify"


Is this URL coming from some spec?

No, the spec does not specify how the user_code should be verified.

aeneasr · 2025-01-16T11:37:27Z

oauth2/handler.go

+	// If there were multiple flows created for the same user_code then we may end up with multiple flow objects
+	// persisted to the database, while only one of them was actually used to validate the user_code
+	// (see https://github.com/ory/hydra/pull/3851#discussion_r1843678761)
+	// TODO: We should wrap these queries in a transaction


TODO is open here

Yes, we discussed this with @zepatrik. We decided that there are no major issues with this and that it should be fixed later on

aeneasr · 2025-01-16T11:39:25Z

oauth2/handler.go

+		x.LogError(r, err, h.r.Logger())
+		h.r.Writer().WriteError(w, r, err)
+		return
+	}


Is this necessary? The audit logger is really not something we are using/maintaining. You can probably just log to the regular logger

Are referring to the logs on https://github.com/ory/hydra/pull/3912/files/14cf7cdc370102da2f487f12d7cdf4945b3e0f48#diff-dc5158991c1ce571f0a59c32b86d6fc64c55fbe11d67cdee44b1b0ae436569dfR743 and https://github.com/ory/hydra/pull/3912/files/14cf7cdc370102da2f487f12d7cdf4945b3e0f48#diff-dc5158991c1ce571f0a59c32b86d6fc64c55fbe11d67cdee44b1b0ae436569dfR748 (lines 743 and 748)?

I can change it, only reason I used that logger is because it is also used in oAuth2Authorize as well

aeneasr · 2025-01-16T11:40:20Z

oauth2/handler.go

+	if err != nil {
+		h.r.Writer().WriteError(w, r, err)
+		return
+	}


Change this to UPDATE ... SET accepted WHER nid=... AND request_id = ? ?

Oh, I'm sorry, it appears that there are quite a lot of values being copied around here

Do you know which values need to be updated here? Is it all of them? Or just some?

I'm not sure to which line you are referring to.

aeneasr · 2025-01-16T11:41:16Z

oauth2/handler.go

+			"prompt",
+			"acr_values",
+			"id_token_hint",
+			"nonce",


This looks copy pasted - could you make this list a variable for re-use?

It's taken from https://github.com/ory/fosite/blob/master/handler/openid/flow_explicit_auth.go#L29. I will make it a variable here, should I add a comment with a link to fosite?

aeneasr · 2025-01-16T11:53:17Z

persistence/sql/migrations/20241609000001000000_device_flow.cockroach.up.sql

+CREATE INDEX hydra_oauth2_device_auth_codes_request_id_idx ON hydra_oauth2_device_auth_codes (request_id, nid);
+CREATE INDEX hydra_oauth2_device_auth_codes_client_id_idx ON hydra_oauth2_device_auth_codes (client_id, nid);
+CREATE INDEX hydra_oauth2_device_auth_codes_challenge_id_idx ON hydra_oauth2_device_auth_codes (challenge_id);
+CREATE UNIQUE INDEX hydra_oauth2_device_auth_codes_user_code_signature_idx ON hydra_oauth2_device_auth_codes (user_code_signature, nid);


I only found queries for device_code_signature (primary key) and request_id) - do we need the challenge-id?

Also are we not missing a foreign key onto the oauth2 flow table (I'm not sure if we have that in the other tables)

Since you're referencing nid as a foreign key, it will also need an index where it's the prefix.

I only found queries for device_code_signature (primary key) and request_id) - do we need the challenge-id?

The challenge_id is not used as an index, the only reason it is there is to be uniform with the other oauth2_token/code tables. AFAICT it is not used there either, but thought maybe there is another reason for it.

Also are we not missing a foreign key onto the oauth2 flow table (I'm not sure if we have that in the other tables)

Thought that there was some issue with adding that fk, but it seems to work now. Will try to add it.

Since you're referencing nid as a foreign key, it will also need an index where it's the prefix.

Not sure why we need an index for that key. When will it ever be used? Do other tables have this index?

Instead of updating the device session, we were over-writing it causing existing session info that were created from fosite to be lost.

nsklikas requested review from aeneasr, hperl and alnr as code owners December 18, 2024 09:08

nsklikas mentioned this pull request Dec 18, 2024

feat: implement RFC 8628 #3851

Open

7 tasks

nsklikas force-pushed the canonical-master branch from baaef03 to 469c5e1 Compare December 18, 2024 12:13

nsklikas force-pushed the canonical-master branch from 469c5e1 to 7970287 Compare January 8, 2025 15:49

nsklikas requested a review from a team as a code owner January 8, 2025 15:49

nsklikas force-pushed the canonical-master branch from 7970287 to 8790af4 Compare January 9, 2025 09:13

aeneasr force-pushed the canonical-master branch from 8790af4 to 14cf7cd Compare January 16, 2025 10:39

aeneasr requested changes Jan 16, 2025

View reviewed changes

nsklikas and others added 20 commits January 24, 2025 14:29

chore: install fosite from branch (remove)

c69a37f

fix: set utc expires_at

a6dc4d7

fix: add redirect_uri to test

112ab9c

fix: add rfc8628 providers to registry

01a6d41

fix: update database schema

9a9a145

fix: update oauth persister logic

9e73ae6

feat: add device authorization endpoint handler

f049cf8

refactor: move logic to updateSessionWithRequest method

e0857b4

fix: rename device auth endpoint handler

c569aab

feat: add device user verification handler

d99822d

fix: implement device user verification logic

9f8469a

feat: update flow

4db4c50

fix: add post device auth handler

8ea3699

feat: add consent handler for accepting a user_code

defaa6a

chore: add post_device_done to config schema

44b13ff

chore: add e2e tests

ea252d9

feat: token request handling for device flow

5110906

chore: update config

3989888

fix: fix the OIDC token and refresh token issue for device flow

06644cd

fix: update OpenID Connect session after user consent

662a4d2

nsklikas and others added 25 commits January 24, 2025 14:30

fix: add GetDeviceCodeSessionByRequestID method

9939877

fix: return client_id to post_device page

e743087

fix: update existing device session

75eef96

Instead of updating the device session, we were over-writing it causing existing session info that were created from fosite to be lost.

fix: update tests

daa5432

fix: add device auth endpoint in discovery metadata

d5ef647

fix: make device grant lifetimes configurable

36dde01

test: update sql fixtures

6461417

fix: perform device flow from CLI

d1311a2

fix: wrap db calls in transaction

430a1ef

chore: fix license

af048c8

chore: update sdk

9854e30

fix: duplicate user_code update

e89a78c

refactor: merge user and device code tables

92f3baa

fix: create openid session when log in succeeds

352294f

refactor: update device session persistence logic

78efb52

fix: update oauth persister logic

d6fe23a

fix: handle user_code collisions

6521272

chore: code review

08f2c53

fixup! dfa4c99

7c99ba2

fixup! dfa4c99

a3b2c68

fixup! 04c25d8

09881fe

chore: add more tests

c04492a

fixup! c69a37f

4ff77fe

fixup! c69a37f

2ff1ebd

fix: update device session fields

2e8cf7c

nsklikas force-pushed the canonical-master branch from 0a4fa13 to 2e8cf7c Compare January 24, 2025 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement RFC 8628 #3912

feat: implement RFC 8628 #3912

nsklikas commented Dec 18, 2024

aeneasr left a comment

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

aeneasr Jan 16, 2025

nsklikas Jan 24, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

aeneasr Jan 16, 2025

nsklikas Jan 24, 2025

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

aeneasr Jan 16, 2025

nsklikas Jan 23, 2025

feat: implement RFC 8628 #3912

Are you sure you want to change the base?

feat: implement RFC 8628 #3912

Conversation

nsklikas commented Dec 18, 2024

Related issue(s)

Checklist

Further Comments

Testing

aeneasr 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

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