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 oauth2_metadata option to add oauth2 tokens to metadata in login response #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DrDaveD
Copy link

@DrDaveD DrDaveD commented Jun 19, 2020

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

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

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

@DrDaveD
Copy link
Author

DrDaveD commented Mar 22, 2021

Updated for version 0.9.2

@DrDaveD
Copy link
Author

DrDaveD commented Jun 17, 2021

Updated for version 0.9.4

@DrDaveD
Copy link
Author

DrDaveD commented Aug 5, 2021

Force-pushed to be a single commit.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@DrDaveD
Copy link
Author

DrDaveD commented May 23, 2022

Rebased after #204 merged

@DrDaveD
Copy link
Author

DrDaveD commented Jan 12, 2023

@impl is this waiting on something?

@impl
Copy link

impl commented Jan 12, 2023

@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! :-)

@DrDaveD
Copy link
Author

DrDaveD commented Jan 12, 2023

@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! :-)

I'm sorry! I should have said @kalafut

@kalafut
Copy link
Contributor

kalafut commented Jan 12, 2023

Sorry, I'm no longer at HashiCorp.

@DrDaveD
Copy link
Author

DrDaveD commented Jan 12, 2023

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.

@austingebauer
Copy link
Member

austingebauer commented Jan 13, 2023

@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?

@DrDaveD
Copy link
Author

DrDaveD commented Jan 13, 2023

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?

@austingebauer
Copy link
Member

@DrDaveD - Thank you for the explanation. I agree with Kalafut's assessment of #107. Particularly this bit.

The basic roles of Vault's auth methods are authenticating to Vault and establishing identity, and the use case you've described does seem to go beyond that. Data from the auth process is generally not returned, though in some cases portions of it are attached to identity metadata if appropriate. The data retained isn't secret, and is often descriptive properties such as the user's region, or resource group.

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.

@DrDaveD
Copy link
Author

DrDaveD commented Jan 25, 2023

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.

@DrDaveD
Copy link
Author

DrDaveD commented Jan 26, 2023

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.

@impl
Copy link

impl commented Jan 26, 2023

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

  1. User authenticates as usual to this plugin, and they get a valid Vault token.
  2. We add a /sts path to this plugin that the token owner can call to request a newly minted token for their own authenticated credentials. Parameters could include requested_token_type, scope, and so on. Whenever a client requests the path, the plugin sends a request to the authorization server token endpoint with something like (roughly abbreviated) grant_type=token-exchange&subject_token_type=access_token&subject_token=<their access_token>&requested_token_type=refresh_token.
  3. The plugin returns the authorization server response pretty much verbatim, e.g. access_token, token_type and so forth, directly to the caller. These tokens will be different than the ones stored by Vault itself.

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:

  • The underlying access/refresh/ID tokens are never directly exposed to the client
  • If you don't want a user to be able to access the /sts path, just don't give put them in an ACL with it
  • The authorization server has the final say in whether the operation is allowed at all

@DrDaveD I wonder if this addresses your use case, or if it just pushes complexity down into another system?

@DrDaveD
Copy link
Author

DrDaveD commented Jan 26, 2023

@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 grant_type=token-exchange&subject_token_type=refresh_token&subject_token=<the saved refresh_token>&requested_token_type=refresh_token (where token_exchange is actually urn:ietf:params:oauth:grant-type:token-exchange and refresh_token is actually urn:ietf:params:oauth:token-type:refresh_token). I guess the plugin would need to associate the refresh token with the provided vault token, since it is not a secrets plugin.

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.

@DrDaveD
Copy link
Author

DrDaveD commented Jan 26, 2023

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.

@impl
Copy link

impl commented Jan 26, 2023

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 /sts path without reauthing.

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.

None yet

5 participants