-
Notifications
You must be signed in to change notification settings - Fork 61
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 oauth2_metadata option to add oauth2 tokens to metadata in login response #119
base: main
Are you sure you want to change the base?
Conversation
3f8d02e
to
e74fccf
Compare
Updated for version 0.9.2 |
Updated for version 0.9.4 |
74489eb
to
d24f121
Compare
Force-pushed to be a single commit. |
d24f121
to
71ffe6c
Compare
Rebased after #204 merged |
@impl is this waiting on something? |
I don't have commit access to this repository, but if I did, I would surely merge it for you! :-) |
Sorry, I'm no longer at HashiCorp. |
Oh. So who is maintaining this plugin now? @austingebauer ? @benashz ? This minor PR has been waiting for 2-1/2 years, along with my major PRs that other people have also been asking for. |
@DrDaveD - I apologize for the delay on some of the PRs here. The Vault ecosystem maintains the plugin. The team has a broad scope of ownership and competing priorities. We often wait for some sense of demand/enthusiasm for the feature from the oss community to feed into prioritization. In this case, it hasn't been clear that there is general demand outside of your specific use case. I'm also unsure if this something we want to support. Today, the tokens that result from the authentication are only known to Vault for a short period of time. With this PR, they could be used beyond that scope. Would this new field need some sort of security disclaimer? I'm slightly hesitant to introduce this capability unless it's acceptable and useful to those using the plugin. Does the benefit of being able to obtain the tokens outweigh the cost of adding a new, potentially security-sensitive parameter to the API? |
It's an important feature to be able to store refresh tokens in vault, which is the whole purpose for why I am using vault. This was the way that was least objectionable to transfer the refresh token out of the auth plugin into a secrets plugin. Previously I had proposed #107 to directly transfer it which would have been a bit more secure, but this is secure enough because the refresh token is useless without the corresponding oauth id and secret, which stay in vault. Maybe you don't object to #107 the way that kalafut did? |
@DrDaveD - Thank you for the explanation. I agree with Kalafut's assessment of #107. Particularly this bit.
This PR can expose the ID token, access token, and refresh token from what I see. I discussed this with my team, and there was consensus that we don't want these tokens to be made available in Vault token metadata. Vault is the confidential client to whom the tokens are issued. Supporting this doesn't seem to align with security recommendations in rfc6749#section-10.3 and rfc6749#section-10.4. I understand this is less convenient, but perhaps a different client application could hand these tokens off to the secrets engine? We have an OIDC client library at hashicorp/cap/oidc that's fairly easy to use. If that's not an option, maintaining a fork is the recommended path. |
How will that help both get a Vault auth token and store the refresh token in a Vault secrets engine? Are you suggesting that the users go through OIDC authentication twice? I need some way for the users to authenticate with OIDC once and both get a Vault token and store the refresh token in Vault. |
Regarding the RFC's security recommendations, I think of the vault client as an extension of vault itself in the Oauth2 model. It is not like a web browser, which never gets any tokens. The vault client supplied with this plugin uses that model too, because the vault client is what receives the callback from the token issuer! (Mitigated in my other long-waiting PR #130.) So I think it's OK for that vital security information to flow through the vault client, if the client does not store the information and never has access to the OIDC client secret. I don't think doing that is a significant security risk. Ideally I would rather use #107 because it avoids sending the refresh token through the client, but you and others in your group have rejected that. This PR was a suggestion of Kalifut as a replacement. If I can't do the approach of either #107 or this PR, do you have any other suggestion for how to obtain and store refresh tokens in vault? I don't have need of the ID token or access token, and my first attempt at Kalifut's suggestion was #113 which only exposed the refresh token, but he suggested to generalize it, which this PR does. Although I have been the only one commenting on this issue so far, I do represent a quite large number (tens of thousands) of users in the High Energy Physics research community who are in the process of transitioning from using X.509-based authentication to JWT authentication. We send short-lived access tokens all over, although we get them from the vault secrets plugin where the refresh tokens are stored, not from this plugin. We use this plugin for OIDC authentication. For reference, the vault configurator we use is here and the vault client we use is here. |
@DrDaveD @austingebauer I wonder if there's any appetite for including STS/RFC 8693 support in this plugin? You could conceivably implement it in such a way where a user could request a valid refresh token without ever exposing the tokens stored internally by the plugin. E.g., I'm thinking:
With this you have a bunch of layers of security that I think could help assuage any concerns about the integrity of the authenticated credentials themselves:
@DrDaveD I wonder if this addresses your use case, or if it just pushes complexity down into another system? |
@impl, thanks for the suggestion. I could work with that, although it seems like an unnecessary complexity. This plugin does not currently (without this PR) give out the access token at all either, so I think a better variation would be for the plugin to store the refresh token and use I would also point out that RFC8693 token exchange is supported by the golang oauth2 library, although they don't admit it. I have tried getting them to accept a documentation update saying so, but they have taken no action. |
Oh, I see you were suggesting having the plugin store the access_token instead of the refresh_token. I was thinking that wouldn't be sufficient because it is typically short-lived, but that would be OK too because I would have the client immediately do the exchange, so it wouldn't have time to expire. |
I think either way (access token or refresh token) could work -- depends on what the plugin has access to and what most servers that implement the RFC provide as to which one you'd want to use. The validity of the Vault token probably shouldn't exceed the expiry time of the returned access token anyway, right? So you'd only have that long to access the |
71ffe6c
to
fe40c1c
Compare
Overview
This PR adds a configuration option oauth2_metadata which is a list of token types that can be returned in metadata. The list can be any of access_token, id_token, and refresh_token, and they are returned in metadata names oauth2_access_token, oauth2_id_token, and oauth2_refresh_token, respectively.
This option makes it possible for a user to authenticate with vault once and also use the tokens directly from the oauth2 issuer. With some help from a client application to store the refresh token back into a vault secrets plugin like the Puppet labs oauthapp, from then on access tokens can be read from the secrets plugin. This is needed by the High Energy Physics science community including the thousands of members of the collaborations running experiments at the Large Hadron Colider.
Design of Change
The new option simply puts the selected tokens into the returned metadata in a similar way to the claims_mapping option returns things in metadata.
Related Issues/Pull Requests
This is another way of addressing issue #101 that is simpler than the rejected pull request #107 and more flexible than pull request #113.
Test results
Contributor Checklist
[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
Will add the docs once the PR is deemed acceptable
[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible