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 refresh_token option to creds write #6

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

DrDaveD
Copy link
Contributor

@DrDaveD DrDaveD commented Mar 20, 2020

I would like to be able to use a single oauth2 authentication to both authenticate to vault and to store the refresh token as a secret. This plugin does a great job on the latter, but it doesn't authenticate vault. This PR adds an option to the creds write function to accept a refresh_token instead of the response code from an oauth2 authorize step. My plan is to next develop a PR for vault-plugins-auth-jwt to store the refresh token at a configured secrets path, to be able to use the two plugins together.

Copy link
Member

@impl impl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello!

Thank your for your PR! I agree that having the exchange done externally could make sense in some use cases, although it may be worth noting in the docs that this does decrease the security of the plugin because it implies that the client secret is known outside of Vault itself.

I left a few points of minor feedback, but in general the implementation looks sane to me.

That said, looking at your use case, I wonder if there is an opportunity for cascading the access token to other configurable endpoints from internally within this plugin. Alternatively, we could implement an authentication backend for this plugin. (It wasn't necessary for our use case, but I'm happy to entertain the idea.) Other than not explicitly supporting a callback on the Vault server, I think this plugin implements most of the required bits from the OIDC plugin. What do you think?

pkg/backend/path_creds.go Outdated Show resolved Hide resolved
pkg/backend/path_creds.go Outdated Show resolved Hide resolved
pkg/backend/path_creds.go Show resolved Hide resolved
@DrDaveD
Copy link
Contributor Author

DrDaveD commented Mar 26, 2020

Thank your for your PR! I agree that having the exchange done externally could make sense in some use cases, although it may be worth noting in the docs that this does decrease the security of the plugin because it implies that the client secret is known outside of Vault itself.

How so? Assuming you're talking about the Oauth2 client secret, that's easy to handle because it would just need to be configured into vault separately for the two plugins.

That said, looking at your use case, I wonder if there is an opportunity for cascading the access token to other configurable endpoints from internally within this plugin.

I don't really see how that would help.

Alternatively, we could implement an authentication backend for this plugin. (It wasn't necessary for our use case, but I'm happy to entertain the idea.)

Now that's a very intriguing idea! You mean that one module could function as both a secrets and auth plugin? I hadn't thought about that possibility.

Other than not explicitly supporting a callback on the Vault server, I think this plugin implements most of the required bits from the OIDC plugin. What do you think?

I actually don't care about the missing callback web server, because what I really want is the OIDC device flow which isn't present in the builtin plugin yet anyway although it likely will be one day). I just thought that it would be best from a reuse point of view to use as much of the base functionality as possible, but it looks like it is harder for two plugins to communicate with each other than I thought.

Maybe the best thing to do would be as you suggest and make this plugin support a vault auth backend, and then add OIDC device flow support. Another thing that I recently realized we'll be needing is an option to get an access token with a subset of the scopes in the original token. Would you be open to accepting PRs for all of those things?

@impl
Copy link
Member

impl commented Mar 26, 2020

Thank your for your PR! I agree that having the exchange done externally could make sense in some use cases, although it may be worth noting in the docs that this does decrease the security of the plugin because it implies that the client secret is known outside of Vault itself.

How so? Assuming you're talking about the Oauth2 client secret, that's easy to handle because it would just need to be configured into vault separately for the two plugins.

I think I phrased this poorly, but what I meant was that you'd have to pull the client secret into memory, at least temporarily, outside of Vault to perform the exchange of the code for a refresh token, unless I'm misunderstanding part of the flow.

That said, looking at your use case, I wonder if there is an opportunity for cascading the access token to other configurable endpoints from internally within this plugin.

I don't really see how that would help.

I was thinking that this plugin could somehow forward the refresh token to the JWT plugin by configuration. I'm not sure if that's possible though.

Alternatively, we could implement an authentication backend for this plugin. (It wasn't necessary for our use case, but I'm happy to entertain the idea.)

Now that's a very intriguing idea! You mean that one module could function as both a secrets and auth plugin? I hadn't thought about that possibility.

Yeah -- not sure about the feasibility of that, but if you could use the same storage backend for both, it could solve this problem.

Other than not explicitly supporting a callback on the Vault server, I think this plugin implements most of the required bits from the OIDC plugin. What do you think?

I actually don't care about the missing callback web server, because what I really want is the OIDC device flow which isn't present in the builtin plugin yet anyway although it likely will be one day). I just thought that it would be best from a reuse point of view to use as much of the base functionality as possible, but it looks like it is harder for two plugins to communicate with each other than I thought.

Maybe the best thing to do would be as you suggest and make this plugin support a vault auth backend, and then add OIDC device flow support. Another thing that I recently realized we'll be needing is an option to get an access token with a subset of the scopes in the original token. Would you be open to accepting PRs for all of those things?

For sure. Subsetting scopes seems straightforward and I'd think we can get that in right away. For the auth backend, it might be worth doing a quick experiment to even see if it's possible before we commit to a full-fledged feature for it. If you'd like to chat about what an implementation plan might look like, you can find me in the Puppet Community Slack channel #relay. Or if you feel comfortable with trying it now, I'd say just go for it and ping us with a PR when you've got something!

@DrDaveD
Copy link
Contributor Author

DrDaveD commented Mar 31, 2020

I have investigated the possibility of having more than one backend in a single plugin, and unfortunately found out that it is not feasible, for two reasons:

  1. Although the underlying Hashicorp plugin library Serve() function supports multiple backends per plugin server, the vault plugin.Serve() function only supports a single backend.
  2. An external plugin can be enabled multiple times at different mountpoints, and each time it is started with a separate process. So even if there were two backends in one plugin, they wouldn't be able to talk to each other.

So I'm back to trying to implement a vault client in vault-plugin-auth-jwt.

@DrDaveD
Copy link
Contributor Author

DrDaveD commented Apr 2, 2020

It turned out that it was easier than I thought for a vault plugin to act as a vault client. I have submitted a vault-plugin-auth-jwt PR to do the other portion.

I think this PR is ready for merging.

Copy link
Member

@impl impl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me from here! @DrDaveD, would you mind squashing your commits and including the prefix New: or Update: at the beginning of the commit message? This will let our tooling automatically package and release a new plugin binary.

@DrDaveD
Copy link
Contributor Author

DrDaveD commented Apr 6, 2020

would you mind squashing your commits and including the prefix New: or Update: at the beginning of the commit message?

Done

@impl
Copy link
Member

impl commented Apr 6, 2020

Perfect, thank you!

@impl impl merged commit c91611f into puppetlabs:master Apr 6, 2020
@DrDaveD DrDaveD deleted the add-refresh-token-opt branch April 6, 2020 19:56
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.

2 participants