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

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

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?

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.

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

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?

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

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

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?

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?

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.

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

nsklikas and others added 22 commits January 16, 2025 11:39

chore: install fosite from branch (remove)

c6e06cc

fix: set utc expires_at

c5456e9

fix: add redirect_uri to test

5d65b78

fix: add rfc8628 providers to registry

390d9fb

fix: update database schema

79e63d8

fix: update oauth persister logic

909999a

feat: add device authorization endpoint handler

82ef5e6

refactor: move logic to updateSessionWithRequest method

76696dd

fix: rename device auth endpoint handler

122da39

feat: add device user verification handler

66bacf8

fix: implement device user verification logic

04c25d8

feat: update flow

6b7810a

fix: add post device auth handler

1a40e99

feat: add consent handler for accepting a user_code

3bf1986

chore: add post_device_done to config schema

45db132

chore: add e2e tests

8484d58

feat: token request handling for device flow

7fb2d45

chore: update config

f5ddf04

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

f35cd27

fix: update OpenID Connect session after user consent

efd42df

fix: add GetDeviceCodeSessionByRequestID method

dbce6e3

fix: return client_id to post_device page

a765730

nsklikas added 15 commits January 16, 2025 11:39

fix: update existing device session

9cbbdb4

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

10b66cc

fix: add device auth endpoint in discovery metadata

4761c44

fix: make device grant lifetimes configurable

935ffa3

test: update sql fixtures

bc51324

fix: perform device flow from CLI

ed2ccce

fix: wrap db calls in transaction

9bb1634

chore: fix license

1611d7f

chore: update sdk

eadbd60

fix: duplicate user_code update

467091a

refactor: merge user and device code tables

dfa4c99

fix: create openid session when log in succeeds

6309d8d

refactor: update device session persistence logic

21c29d2

fix: update oauth persister logic

8da4859

fix: handle user_code collisions

14cf7cd

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

chore: code review

0a4fa13

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

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 2025

aeneasr Jan 16, 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