-
Notifications
You must be signed in to change notification settings - Fork 62
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
base: main
Are you sure you want to change the base?
Add device flow after code flow callbacks direct to vault #131
Conversation
0a614f5
to
2677522
Compare
Updated for version 0.9.2 |
Updated for version 0.9.4 |
92b2d28
to
6281f1c
Compare
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. |
6281f1c
to
81612a2
Compare
Force pushed after updating for 0.11.1 |
81612a2
to
213ab3c
Compare
213ab3c
to
08044b7
Compare
Force-pushed after updating for 0.12.1 |
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?) |
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. |
08044b7
to
7b23f46
Compare
Updated after #204 merged into main |
is this pr ever going to make it in ? Would really like to see it |
7b23f46
to
3fce815
Compare
Rebased on main after the commits of October 12, 2022 |
3fce815
to
fce2aa0
Compare
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. |
Thanks @DrDaveD for keeping this up to date. Hopefully it goes in at some point finally... |
fce2aa0
to
7c8e5c9
Compare
e8e209a
to
eecc4ea
Compare
eecc4ea
to
b4937a7
Compare
@DrDaveD Thank you for submitting this. We’re discussing it internally and will keep you updated. |
@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) |
This option sounds more useful to me than manual mode. |
f4096a2
to
c7c913b
Compare
c7c913b
to
f373af0
Compare
Signed-off-by: Dave Dykstra <[email protected]>
Signed-off-by: Dave Dykstra <[email protected]>
f373af0
to
1db39e6
Compare
Any updates to this PR? Having to port forward a local HTTP server to finalize the auth has been a huge pain for our vault deployment. |
I have been keeping it up to date, but the maintainers haven't moved on it in 4 years. The corresponding PR this depends on has now been merged into the openbao development branch and the developers there plan to include the PR corresponding to this one soon. |
Thanks for the heads up @DrDaveD . We'll evaluate OpenBao as an alternative, seems like it's moving faster. |
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:
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 optionpoll_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 aslow_down
reply to a poll request. Aredirect_uri
passed in to the auth API is ignored in device flow.Related Issues/Pull Requests
#103, #122 and #130
Test results
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