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

Add device flow after code flow callbacks direct to vault #131

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DrDaveD
Copy link

@DrDaveD DrDaveD commented Aug 17, 2020

Overview

This adds support for OIDC device flow on top of pr #130 as an alternative to the implementation in pr #122. #130 has to be committed first and all its changes are included here. If you'd like to see just the changes compared to that pr, see my own pr 1.

Device flow has several advantages over direct callback mode:

  1. There's no need to configure allowed redirect uris for the client.
  2. There's no need to configure firewalls to allow the Authorization Server to call back to Vault.
  3. There's no need for the Authorization Server to recognize the CA cert for Vault.
  4. The URL that the user sees is simpler.

So it's worth having device flow even with direct callback mode, although direct callback mode is good when Authorization Servers don't support device flow.

Design of Change

Device flow is enabled with this implementation by setting the role configuration callback_mode=device. The device authorization endpoint is auto-discovered. This also adds an additional optional role configuration option poll_interval which defaults to 5.

The client API is slightly extended, to add an optional user_code option in the auth response, and to add a slow_down reply to a poll request. A redirect_uri passed in to the auth API is ignored in device flow.

Related Issues/Pull Requests

#103, #122 and #130

Test results

ok  	github.com/hashicorp/vault-plugin-auth-jwt	4.932s

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
Will do if the PR is acceptable
[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible

@DrDaveD
Copy link
Author

DrDaveD commented Mar 22, 2021

Updated for version 0.9.2

DrDaveD added a commit to DrDaveD/vault-plugin-auth-jwt that referenced this pull request Mar 22, 2021
@DrDaveD
Copy link
Author

DrDaveD commented Jun 17, 2021

Updated for version 0.9.4

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from 92b2d28 to 6281f1c Compare August 5, 2021 20:11
@DrDaveD
Copy link
Author

DrDaveD commented Aug 5, 2021

Force-pushed to be a single commit after direct callback mode, and updating to the master branch at the time of the 0.10.1 tag.

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from 6281f1c to 81612a2 Compare November 3, 2021 21:24
@DrDaveD
Copy link
Author

DrDaveD commented Nov 3, 2021

Force pushed after updating for 0.11.1

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from 81612a2 to 213ab3c Compare November 3, 2021 21:36
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from 213ab3c to 08044b7 Compare March 23, 2022 21:13
@DrDaveD
Copy link
Author

DrDaveD commented Mar 23, 2022

Force-pushed after updating for 0.12.1

@sepiroth887
Copy link

sepiroth887 commented Apr 6, 2022

Really excited for this PR to merge. What's the path here to completion and inclusion in vault (or is it directly usable as a plugin as well?)

@DrDaveD
Copy link
Author

DrDaveD commented Apr 6, 2022

I do use it as a plugin to vault for now. I include it in an rpm that I build based on my htvault-config package. It ends up forking a process for every issuer so it would be better if it were included internally.

@DrDaveD
Copy link
Author

DrDaveD commented May 23, 2022

Updated after #204 merged into main

@jmls
Copy link

jmls commented Dec 15, 2022

is this pr ever going to make it in ? Would really like to see it

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from 7b23f46 to 3fce815 Compare January 12, 2023 22:28
@DrDaveD
Copy link
Author

DrDaveD commented Jan 12, 2023

Rebased on main after the commits of October 12, 2022

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from 3fce815 to fce2aa0 Compare January 19, 2023 23:25
@DrDaveD
Copy link
Author

DrDaveD commented Jan 19, 2023

I just did another force push to add support for "verification_url" in addition to "verifcation_uri", since some older server implementations such as Google use the former despite what's in the RFC.

@tmaroschik
Copy link

Thanks @DrDaveD for keeping this up to date. Hopefully it goes in at some point finally...

@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from fce2aa0 to 7c8e5c9 Compare May 2, 2023 19:21
@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch 2 times, most recently from e8e209a to eecc4ea Compare May 17, 2023 17:38
@DrDaveD DrDaveD force-pushed the device-flow-after-server-code-flow branch from eecc4ea to b4937a7 Compare May 17, 2023 17:55
@Zlaticanin
Copy link

@DrDaveD Thank you for submitting this. We’re discussing it internally and will keep you updated.

@42wim
Copy link

42wim commented Dec 3, 2023

@DrDaveD I've tested the PR and both flows and it works fine 👍

Some thoughts:

I think it would be also useful if there's an option for a manual flow too, like in #125

This makes it a bit more secure for possible phishing attacks where an attacker can generate an URL, get a user to click on it (which may be automatically executed because of SSO) and the attacker then can get the token. Of course this makes it less userfriendly. I've hoped the device flow would've solved that, but keycloak for example will return an URL with the code in, so this also can be done automatically with the device flow.

And/or exposing the oidcRequestTimeout so that this can be brought down to seconds in direct mode instead of 10 minutes. (should probably be also have another cache that doesn't allow repopulating with the same clientnonce and escape the cleanup)

@DrDaveD
Copy link
Author

DrDaveD commented Dec 5, 2023

And/or exposing the oidcRequestTimeout so that this can be brought down to seconds in direct mode instead of 10 minutes. (should probably be also have another cache that doesn't allow repopulating with the same clientnonce and escape the cleanup)

This option sounds more useful to me than manual mode.

Signed-off-by: Dave Dykstra <2129743+DrDaveD@users.noreply.github.com>
Signed-off-by: Dave Dykstra <2129743+DrDaveD@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants