Device grant flow (migrate to master)

[BuzzBumbleBee]

Related Issue or Design Document

Checklist

• [ ] I have read the contributing guidelines and signed the CLA.
• [ ] I have referenced an issue containing the design document if my change introduces a new feature.
• [ ] 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 green light (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 necessary documentation within the code base (if appropriate).

Further comments

[CLAassistant]

All committers have signed the CLA.

[supercairos]

Thank you for the tremendous amount of work! I have not yet been able to read through everything and only looked into a few files. What struck me is the question of whether it would be possible to move the device_* files into a separate module in the handler directory? Similar to e.g. jwt-bearer

I've refactored all the code that was in handler/oauth2/flow_device_* to be in a separate folder handler/rfc8628/* following what was done for jwt-bearer.

I also took the liberty to remove code duplicate that was present. For exemple in PKCE and OAUTH2 files were duplicated with very minor changes to fit better the device flow. They are now respectively only one file as originaly that switch based on the request content.

[supercairos]

I haven't tested to hook it up with Hydra to make the Github CI pass, I've only run test on my local computer

[supercairos]

@aeneasr Can you review again? I've fixed the code to work with Hydra and it seams to be working.

[supercairos]

What I have not yet fully understood yet, who validates the user code? Is that the consumer of Fosite?

Indeed, In our case it's Hydra that handle the UserCode validation. https://github.com/BuzzBumbleBee/hydra/blob/feat_dev_grants_2x/consent/handler.go#L931-L948

[supercairos]

@aeneasr Any chance you had time to check this out ? :)

[juguhan]

May I know if there is any plan to merge this PR to support rfc8628?

[supercairos]

rfc8628

From what I know, Ory don't have any plans to merge this PR as it's not a requested feature by any of their customer and this PR has quite a huge change. I'll do my best to maintain this updated on every new Ory/Hydra updates.

[supercairos]

Let me check that later this week. I haven't coded in this project for a while now :)

[supercairos]

There is a also a refactoring I would like to perform on the PKCE, let me know if it's needed: https://github.com/ory/fosite/pull/701#discussion_r1081536194

[hperl]

There is a also a refactoring I would like to perform on the PKCE, let me know if it's needed: #701 (comment)

I'm still trying to understand why PKCE would be needed for the device authorization grant. PKCE would protect the device code, but the code is returned in-band in the initial response to the device authorization request. If an attacker could read that response, it could surely also read the final response from the token request. How does PKCE add more security here?

[supercairos]

There is a also a refactoring I would like to perform on the PKCE, let me know if it's needed: #701 (comment)

I'm still trying to understand why PKCE would be needed for the device authorization grant. PKCE would protect the device code, but the code is returned in-band in the initial response to the device authorization request. If an attacker could read that response, it could surely also read the final response from the token request. How does PKCE add more security here?

I do agree that PKCE is a bit redondant as it's done in-band. But it was a request by @BuzzBumbleBee and I saw no reason why not implement it :)

Maybe he can explain better than I (cc @BuzzBumbleBee)

[BuzzBumbleBee]

There is a also a refactoring I would like to perform on the PKCE, let me know if it's needed: #701 (comment)

I'm still trying to understand why PKCE would be needed for the device authorization grant. PKCE would protect the device code, but the code is returned in-band in the initial response to the device authorization request. If an attacker could read that response, it could surely also read the final response from the token request. How does PKCE add more security here?

I do agree that PKCE is a bit redondant as it's done in-band. But it was a request by @BuzzBumbleBee and I saw no reason why not implement it :)

Maybe he can explain better than I (cc @BuzzBumbleBee)

In a lot of cases this type of flow is used on IoT devices to login to service (think BBC iPlayer / Disney+ / Netflix on android TV) those would likely be using an Auth fow with PKCE (client credentials could be extracted from the application).

These services almost always have a fallback method of logging in that would be a normal username and password so not having PKCE support on this flow but available on others would add complexity to the end user application.

I can see the limitations tho.

Also another providers (like forgerock) support device flow with PKCE.

[hperl]

There is a also a refactoring I would like to perform on the PKCE, let me know if it's needed: #701 (comment)

I'm still trying to understand why PKCE would be needed for the device authorization grant. PKCE would protect the device code, but the code is returned in-band in the initial response to the device authorization request. If an attacker could read that response, it could surely also read the final response from the token request. How does PKCE add more security here?

I do agree that PKCE is a bit redondant as it's done in-band. But it was a request by @BuzzBumbleBee and I saw no reason why not implement it :) Maybe he can explain better than I (cc @BuzzBumbleBee)

In a lot of cases this type of flow is used on IoT devices to login to service (think BBC iPlayer / Disney+ / Netflix on android TV) those would likely be using an Auth fow with PKCE (client credentials could be extracted from the application).

These services almost always have a fallback method of logging in that would be a normal username and password so not having PKCE support on this flow but available on others would add complexity to the end user application.

I can see the limitations tho.

Also another providers (like forgerock) support device flow with PKCE.

Sorry, I didn't follow that argument. I agree that for the authorization code flow you need PKCE, and I also saw forgerock implementing PKCE for the device flow.

As I see it:

• it complicates the current implementation of PKCE for the authorization code grant (minor argument)
• there is no standard describing PKCE with the device code grant (minor argument, we can deviate from standards if needed; please correct me if I'm wrong)
• it does not add to security, as an attacker that can read the in-band device code can also read the in-band access token (major argument; please correct me if I'm wrong)

All in all, I'd not implement PKCE for the device flow.

[supercairos]

There is a also a refactoring I would like to perform on the PKCE, let me know if it's needed: #701 (comment)

I'm still trying to understand why PKCE would be needed for the device authorization grant. PKCE would protect the device code, but the code is returned in-band in the initial response to the device authorization request. If an attacker could read that response, it could surely also read the final response from the token request. How does PKCE add more security here?

I do agree that PKCE is a bit redondant as it's done in-band. But it was a request by @BuzzBumbleBee and I saw no reason why not implement it :) Maybe he can explain better than I (cc @BuzzBumbleBee)

In a lot of cases this type of flow is used on IoT devices to login to service (think BBC iPlayer / Disney+ / Netflix on android TV) those would likely be using an Auth fow with PKCE (client credentials could be extracted from the application). These services almost always have a fallback method of logging in that would be a normal username and password so not having PKCE support on this flow but available on others would add complexity to the end user application. I can see the limitations tho. Also another providers (like forgerock) support device flow with PKCE.

Sorry, I didn't follow that argument. I agree that for the authorization code flow you need PKCE, and I also saw forgerock implementing PKCE for the device flow.

As I see it:

• it complicates the current implementation of PKCE for the authorization code grant (minor argument)
• there is no standard describing PKCE with the device code grant (minor argument, we can deviate from standards if needed; please correct me if I'm wrong)
• it does not add to security, as an attacker that can read the in-band device code can also read the in-band access token (major argument; please correct me if I'm wrong)

All in all, I'd not implement PKCE for the device flow.

I let @BuzzBumbleBee reply as he was the one requesting this feature.

But I would tend to be on @hperl side on that one:

1. it's not really a standard and not supported by many standard client library (you can always hack it in as I did in: https://github.com/supercairos/oauth-device-flow-client-sample/blob/master/src/index.ts#L54).
2. It would make this PR a bit simpler and ca be easily done at a latter time.

[BuzzBumbleBee]

@hperl / @supercairos

I think maybe I'm overcomplicating this, take this example.

App on Android TV, it has the device flow and (as a fallback) has auth code + PKCE flow behind a "login with email button"

I can't store the client secret I'm the application as it's then vulnerable to extraction.

Without code challenge and verifier will hydra allow a client authentication method of none ? I'm pretty sure this was something I hit really early when investigating this (hydra wasn't allowing a none Auth type without PKCE)

I have no objection to removing PKCE as long as it can support implementations where client secret storage isn't available.

Again maybe I'm overcomplicating here :)

[hperl]

@hperl / @supercairos

I think maybe I'm overcomplicating this, take this example.

App on Android TV, it has the device flow and (as a fallback) has auth code + PKCE flow behind a "login with email button"

I can't store the client secret I'm the application as it's then vulnerable to extraction.

Without code challenge and verifier will hydra allow a client authentication method of none ? I'm pretty sure this was something I hit really early when investigating this (hydra wasn't allowing a none Auth type without PKCE)

I have no objection to removing PKCE as long as it can support implementations where client secret storage isn't available.

Again maybe I'm overcomplicating here :)

Thanks for adding the use case. Client authentication is skipped for the device auth grant, see here: https://github.com/ory/fosite/blob/b1fbd361a56bd5c5d24df7e07d2d8aed636f8aa1/handler/rfc8628/token_endpoint_handler.go#L56-L58

Does that resolve your requirement for PKCE?

[BuzzBumbleBee]

@hperl absolutely fine by me in that case 👍

[supercairos]

@hperl absolutely fine by me in that case 👍

So I can remove the PKCE device code ?

[hperl]

@hperl absolutely fine by me in that case 👍

So I can remove the PKCE device code ?

Yes please.

[supercairos]

@hperl absolutely fine by me in that case 👍

So I can remove the PKCE device code ?

Yes please.

PKCE has been removed! ✅

[glerchundi]

I'm thrilled seeing how this three even four legged contribution is going! Keep up the good work! We're super interested in having Device flow in Hydra 💪

[vivshankar]

@BuzzBumbleBee Thanks for a great contribution. A small point for your consideration. I am probably over-thinking this.

On my end, we implemented device flow on our Fosite fork (https://github.com/vivshankar/fosite/tree/v0.44.x) and used the following terminology -

RFC8628DeviceAuthorize RFC8628UserAuthorize

The main motivation was because we anticipated CIBA also requiring a user authorization handler endpoint for cases where the back-channel authentication mechanism is a link that is sent to the user via SMS and other transports, as opposed to a more secure push notification. This would behave exactly like the /user_authorize endpoint you would use for device flow except it is sent through a back channel transport mechanism.

We felt "DeviceUser" didn't necessarily capture that this was the device flow spec. Again, this is an extremely minor point and you can feel free to ignore this 😄 . My motivation here is for us to not stay forked forever when this PR is merged.

[kghost]

This feature is essential, it seems that Canonical already adopt it internally.

Is there anything we can do to move it further ?