puppetlabs / vault-plugin-secrets-oauthapp

OAuth 2.0 secrets plugin for HashiCorp Vault supporting a variety of grant types
Apache License 2.0
94 stars 11 forks source link

Implement RFC 8693 (STS/token exchange) #41

Closed DrDaveD closed 1 year ago

DrDaveD commented 3 years ago

The code in this branch is exactly the same as #40 but it is a much cleaner single commit on top of the current main branch instead of the mess in the current feature/exchange plus #40. I recommend closing #40, replacing the features/exchange branch with a branch off of main, and merging this into it. Currently if I try to get github to create a diff off of #40 it does not cleanly apply as a patch to the main branch.

impl commented 3 years ago

Makes sense. I'll close #40 and #24 and we can start over. Sorry for the churn on this one.

DrDaveD commented 3 years ago

Force-pushed after rebasing on 2.1.0

DrDaveD commented 3 years ago

Also see long, related discussion in #18.

DrDaveD commented 3 years ago

Force-pushed after rebasing on 2.2.0

DrDaveD commented 3 years ago

Force-pushed after rebasing on 3.0.0-beta.3

DrDaveD commented 3 years ago

Oops, I thought I had run make check and make test but apparently not. Force-pushed again after fixing those.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

DrDaveD commented 3 years ago

Force-pushed after rebasing on 3.0.0-beta.4

DrDaveD commented 3 years ago

Force-pushed after rebasing on 3.0.0.

impl commented 3 years ago

Hey @DrDaveD,

I'm probably going to take a pass at finally implementing this in my free time in the near future, so I wanted to check in to see whether the strategy we discussed in #26 will still work for you here:

Does that all make sense? Any concerns about implementation in your environment?

DrDaveD commented 3 years ago

That all sounds fine for my environment. I will have to extend the vault policies to allow access to the additional paths, and change my client to know about them, but I can do that.

Since we last discussed, I have learned more about RFC8693 and think there may be a desire for additional options to set the requested token type to be urn:ietf:params:oauth:token-type:refresh_token or urn:ietf:params:oauth:token-type:access_token (or leave it unset to take the token issuer's default). If a refresh token is requested I would expect that to stay in the plugin and an access token returned. Also there may be a desire to select between those two subject token types. The plugin holds both the refresh token and access token so it can include the appropriate subject token in the request. The RFC also talks about actor token and actor token type but I really don't understand what use case there might be for that or how it could be included so I would suggest leaving that as an exercise for the future if ever.

DrDaveD commented 3 years ago

I just remembered that I have a second client that was depending on the very simple API that I now have implemented. It is delivered a valid vault token, a URL, and list of scopes & audiences and does a single read from that API. I will have to change it to write to the new location and read it back. I can do that, but it will take some time to deploy to the installations that are already testing with this. This other client is also part of a much bigger project that I am only a small contributor to, it's not completely under my control. I will need to merge my current PR with what you come up with and maintain that for a while until all the clients are updated.

DrDaveD commented 3 years ago

That other client I'm talking about reads the URL from my separately packaged primary client. So if there were a way to have an API that would do the put & read in one step, compatible with what I am doing in my PR, it would be a lot easier for me because I wouldn't have to change the other client.

impl commented 1 year ago

OK, I've spent enough time noodling on this PR and not doing anything about it that I've come to a conclusion on what I want to do now vs. later.

I'm going to leave the sts/<cred-name>/:/<token-name> work for later (if ever, perhaps as demanded by interest from others), and that will potentially support some of the other requested_token_type options from RFC 8693. Instead I'm just going to add a read endpoint like sts/<cred-name> which will only support access token retrieval as you've implemented. I want it to be a separate endpoint so that users can have more fine-grained ACLs (some users may not want to expose this operation to their clients for reads, I think).

So you should end up with a single-endpoint STS exchange that you can hopefully swap over to without much difficulty. As always, thanks for your hard work on and input on the design of this plugin!

DrDaveD commented 1 year ago

By this point I have a fairly extensive installed base, so I will need a plugin that supports both modes for a transition period. I expect to make a PR to your implementation once it is finished to add a compatibility interface to look like as it is in this PR, so I can continue to support current clients until they all get upgraded to the new interface. It helps my build process for it to be a PR, but you won't ever need to merge it.

DrDaveD commented 1 year ago

Note: as of v3.1.1, this PR still applies cleanly to the main branch even though its functionality has been moved to the /sts path instead of /creds in #78. So in my build of fermitools/htvault-config I am still applying the patch of this PR as it is on top of the v3.1.1 source code, so I can have both paths for a transition period.