Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design Proposal: Implementation of OAuth 2.0 Device Authorization Grant #6

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@wadahiro
Copy link

commented May 1, 2019

This pr is a design proposal which supports KEYCLOAK-7675 Decoupled channel authentication mechanism partially (OAuth 2.0 Device Authorization Grant only).

Mailing list: http://lists.jboss.org/pipermail/keycloak-dev/2019-March/011843.html

@stianst
Copy link
Contributor

left a comment

  • Missing details around how authentication will be done
  • Missing details around rate limiting / brute force protection
  • Consent section needs to be clarified/discussed
Show resolved Hide resolved design/oauth2-device-authorization-grant.md

## User Code Format

The spec says about user code format patterns in [6.1. User Code Recommendations](https://tools.ietf.org/html/draft-ietf-oauth-device-flow-15#section-6.1). It's good to be able to provide an SPI for user code format customization. We implement a case-insensitive eight-letter format as a default implementation of the SPI.

This comment has been minimized.

Copy link
@stianst

stianst May 8, 2019

Contributor

Default implementation should be "four-letters space four-letters" (WDYG MFKS). The space makes it more readable. When validating the code the space should be ignored so the user can enter "WDYG MFKS" or "WDYGMFKS".

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 10, 2019

Why not use WDYG-MFKS ? It looks better than space, IMO. The code is not subject to doubts whether or not you have spaces in code.

This comment has been minimized.

Copy link
@stianst

stianst May 13, 2019

Contributor

What doubts? Space is optional so it doesn't matter. You can type it as [xxxxyyyy] [xxxx yyyy] or even [xxxx] [yyyy].

Google uses spaces, Apple uses hyphens. I find spaces more readable. Further, with optional spaces the input can be split into multiple inputs if wanted and just joined together without having to consider what the delimeter is as it is optional:

[XXXX] [YYYY] - instead of [XXXX-YYYY]

This comment has been minimized.

Copy link
@wadahiro

wadahiro May 15, 2019

Author

When I refer to Google documents, it seems to use dashes as a delimiter.
Also dashes is used in the spec 6.1 as an example such as "WDJB-MJHT". Therefore, I feel that dashes is preferable as default. Also, in my proposal this format is customizable with SPI.

This comment has been minimized.

Copy link
@stianst

stianst May 15, 2019

Contributor

OK, let's go with dashes then. I was thinking about application passwords feature that Google has, where it uses optional spaces.


In this case, the end-user need to enter a user code first in the user interaction. It means the authorization server can provide different authentication flow per client because the server can detect the client ID from the user code. However, the risk getting user code brute force attack is higher than Login First as attackers can try to enter user codes freely.

### Which user interaction does keycloak should support?

This comment has been minimized.

Copy link
@stianst

stianst May 8, 2019

Contributor

I don't have a strong preference to either login or user code first, so going with user code first works for me.

Please elaborate on how the user will be authenticated though. The user should authenticate through the regular browser flow as usual. If already authenticated this is obviously not required. What is the plans around this?

The spec also mentions that rate limiting should be implemented. How are you planning on tackling that?

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 10, 2019

I agree that user code first is what we should consider. However, if the user is using the verification_uri_complete we can prompt for credentials beforehand given that the user_code is already available from the URL.

Are you considering Non-textual Verification URI Optimization (verification_uri_complete) in this proposal?

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 10, 2019

Regarding rate limiting, I guess we can reuse some stuff that we use in Security Defenses/Brute Force ?

This comment has been minimized.

Copy link
@stianst

stianst May 13, 2019

Contributor

verification_uri_complete should be implemented as it allows displaying a QR code with the link and not require users to enter user code manually. It should show a first page asking the user to consent to linking the device prior to continuing (this page should be displayed first in place of the page asking for the user code with verification_uri)

This comment has been minimized.

Copy link
@wadahiro

wadahiro May 15, 2019

Author

Thank you for feedback. I'll add an explanation of authentication to my proposal. In my proposal, the regular browser flow is used for the user authentication after user code verification.

verification_uri_complete

In 3.3.1, the spec says The server SHOULD display the "user_code" to the user and ask them to verify that it matches the "user_code" being displayed on the device, to confirm they are authorizing the correct device. So I agree that showing a page first in place of the page asking for the user code with verification_uri.

Note: the first page when verification_uri_complete is used is not implemented in my implementation yet.

rate limiting / brute force protection

Unlike the existing Brute Force protection, the user ID is unknown at that time.
How about using the value of Cookie AUTH_SESSION_ID as the key for handling rate limiting / brute force protection? Or should the source IP be the key?

This comment has been minimized.

Copy link
@stianst

stianst May 15, 2019

Contributor

I'd say anyone trying to brute force something would clear cookies after each attempt. At least that's what I'd do ;)

Source IP is also a bit questionable as most likely a brute force attack would require significant attempts here, meaning a bot net would be used.

To be honest I'm not sure how best to proceed with this right now, but I don't think the brute force protection we have today can be directly used here and we'd need something different (and smarter?).

This comment has been minimized.

Copy link
@wadahiro

wadahiro May 23, 2019

Author

How about implementing rate limiting user code attempts, which is described in the spec 5.1. User Code Brute Forcing? I think we can achieve rate limiting with the following logic.

  • When attacker enters an invalid user code, an error event is recorded into EVENT_ENTITY table.
  • If the code is valid, keycloak check whether the attempts exceeds the threshold number of times from the code issued time referring EVENT_ENTITY table.
  • If the attempt exceeds the limit, reject the verification process of user code.

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 23, 2019

Maybe we should, just in the same way that we control the interval which a client can pool the AS for user approval, control the interval that the user can submit the user_code. In combination with the code lifetime, this should make life really hard for an attacker.

Beyond that, admins should take into account any network tool to mitigate suspicious behavior.


User Code Verification First is adopted by major IdPs like Google, Microsoft, and Salesforce. So it seems to be a common practice. Also, it can provide a more flexible authentication flow per device client. Keycloak already provides a feature that a client can use specific browser flow by using the Authentication Flow Overrides option. Therefore, it's suitable for supporting User Code Verification First.

## Consent

This comment has been minimized.

Copy link
@stianst

stianst May 8, 2019

Contributor

I would think that consent should be optional like it is for other flows. If the user visits verification_uri the user will manually copy the user code from the device so consent is implied here. If the user visits verification_uri_complete (perhaps by scanning a QR code) the user should have a page displayed with details about the client and user code visible before continuing to authentication (and optionally giving specific consent for scopes to the client).

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 10, 2019

The fact that the user types the user_code when doing verification_uri does not imply the user is also consenting access to a device. The device can ask for scopes when making authorization requests and the user should be aware of the scopes he will grant to the device.

In both cases, the consent page should be displayed accordingly with the client configuration in regards to whether or not consent should be displayed. Just like it works today.

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 10, 2019

The way I see the user verification flow is as follows:

  • The user provides the code (directly or indirectly when doing verification_uri_complete) at /realms/{realms}/device`
  • Once the code is submitted by the user, the /realms/{realms}/device will redirect the user to the login page. At this point, the device endpoint will set some state in a way that after the user is authenticated, we can mark the code as approved.

My point here is that the device endpoint is basically reusing and relying on the regular authentication flow configured to the realm with a caveat that we need some implicit execution during the flow that is capable of checking the user code being approced to allow the device to obtain an access token.

Another approach is supporting a callback URI to the device endpoint. In this case, once the user authenticates, Keycloak will redirect the user back to the device endpoint in order to complete the user code approval.

This comment has been minimized.

Copy link
@stianst

stianst May 13, 2019

Contributor

I believe we are saying the same thing, but neither of us are saying it 100% clearly. This is the flow I have in mind:

  1. If verification_uri then user is asked to enter user code. If verification_uri_complete the user is asked to confirm if they want to link/authenticate the device.
  2. User is asked to login if not already logged-in (through the regular browser flow)
  3. User is asked to consent to scopes if client requires consent (through the regular browser flow)
  4. Code is marked as approved

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 13, 2019

Sure Stian, in fact, the flow is part of the standard :)

The point I'm trying to make is more related to the implementation details. Although I didn't look at the current implementation yet.

This comment has been minimized.

Copy link
@wadahiro

wadahiro May 15, 2019

Author

One of the reasons for displaying the consent page always is to prevent remote phishing, as mentioned below in the spec 5.4.

5.4.  Remote Phishing

   It is possible for the device flow to be initiated on a device in an
   attacker's possession.  For example, an attacker might send an email
   instructing the target user to visit the verification URL and enter
   the user code.  To mitigate such an attack, it is RECOMMENDED to
   inform the user that they are authorizing a device during the user
   interaction step (see Section 3.3), and to confirm that the device is
   in their possession.  The authorization server SHOULD display
   information about the device so that the person can notice if a
   software client was attempting to impersonating a hardware device.

But, I agree to use the existing Consent Required option. However, in that case, I would like to propose adding the Display Consent Always sub-option which is configurable when Consent Required is true. In the current implementation, once the user agrees, it will be skipped in the subsequent authentication.

This comment has been minimized.

Copy link
@stianst

stianst May 15, 2019

Contributor

I'd say that is not needed as there is a separate consent to authorize the device already present, which is either user entering the user code manually or confirming the user code. When entering the code manually the user knows the device as the code is coming from the device. When reviewing the code we know the client so can display info about the client alongside the user code.

This comment has been minimized.

Copy link
@wadahiro

wadahiro May 22, 2019

Author

@stianst

When reviewing the code we know the client so can display info about the client alongside the user code.

You mean the end-user can see the screen for reviewing after entering user code even if verification_uri is used? The flow is as follows. If so, I agree.

  1. User is asked to enter user code. then enter user code.
  2. If the user code is valid, user is asked to continue with information including the device (client information).
  3. If user click continue button, user is asked to login if not already logged-in (through the regular browser flow).
  4. User is asked to consent to scopes if client requires consent (through the regular browser flow).
  5. Device code is marked as approved.

Also, to implement review screen in step 2 above, should we add User Code Review Screen Text option in client config page?

@stianst stianst requested a review from pedroigor May 8, 2019

@stianst stianst self-assigned this May 8, 2019

@pedroigor
Copy link

left a comment

Overall it looks good, I would like to try your implementation next week and see how you are doing this, especially in regards to the user verification process.


## Endpoint

### Device Authorization Endpoint

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 10, 2019

I would suggest /realms/{realm}/openid-connect/device/code.

However, this proposal highlights an issue in our resource path format, which I hope we will improve with the new REST guidelines we are defining. Ideally, the path should be /realms/{realm}/oauth/device/code.

This comment has been minimized.

Copy link
@stianst

stianst May 13, 2019

Contributor

Why code and not auth? The step is called "Device Authorization Request", so auth makes more sense to me, as it is the equivalent of the regular OAuth auth endpoint.

This comment has been minimized.

Copy link
@pedroigor

pedroigor May 13, 2019

You are obtaining a code, both device and user codes.

This comment has been minimized.

Copy link
@wadahiro

wadahiro May 23, 2019

Author

The regular authorization endpoint also returns a code which is called Authorization Code. So I think it is better to match the existing endpoint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.