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 security@ory.sh) 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]

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

[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:

fosite/handler/rfc8628/token_endpoint_handler.go

Lines 56 to 58 in b1fbd36

	func (c *DeviceAuthorizeHandler) CanSkipClientAuth(ctx context.Context, requester fosite.AccessRequester) bool {
	return requester.GetGrantTypes().ExactOne(string(fosite.GrantTypeDeviceCode))
	}

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 ?