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 10 forks source link

Allow slash in credential names #26

Closed DrDaveD closed 3 years ago

DrDaveD commented 3 years ago

This allows slash to be in credential names.

As I first mentioned in pr #18:

After the initial OIDC authentication we plan to renew our vault credentials weekly with kerberos. In addition to the user's regular kerberos credentials, we have the ability to create "robot" kerberos credentials of the form /cron/ which are to be used for automated operations via cron. We can translate a credential of that form to a subpath in vault under creds using a vault policy we apply to the auth-kerberos plugin.

@impl replied:

I am OK with that change to the acceptable path tokens as long as the intention is to escape the / -- e.g., would need to be creds/foo%2fbar and not creds/foo/bar.

I would like to do that but the problem is that there are no controls in the kerberos plugin to edit the kerberos credential name when creating a vault policy file. As you pointed out later in the same comment, I can insert {{ identity.entity.name }} into the policy anywhere I want, but there's no provision for editing it to change slashes into %2f. I understand that allowing slashes in the credential name makes things more complicated to look for the "/tokens/" separator being discussed in that PR, but I think it can still be made to work OK. There is a risk that the separator may be accidentally included as part of the credential name, so maybe the separator would need to include a special character not allowed in a credential name.

Since with this PR both "." and "/" would be allowed in a credential name, I tested what happens with "/../". I found out that vault canonicalizes that and removes such constructs (going up a level in the path like a Unix filesystem) so it would never reach the plugin and we don't need to worry about that at least.

impl commented 3 years ago

You know, it would be a big surprise to me if they weren't escaping the slashes for you already. I'll have to double check how the policy checks are layered on, but if you had a policy rule against creds/{{ identity.entity.name }} by itself, it would be super weird for someone to be able to auth with, say, an identity with a name of foo/tokens/bar and have that policy check then succeed against creds/foo/tokens/bar. I would expect Vault upstream to pass that through encoded as creds/foo%2ftokens%2fbar.

DrDaveD commented 3 years ago

I'm quite sure that at least by the time the plugin sees it there are slashes there. First, I needed this PR to make it work. Next, with oidc authentication I have a policy "template" that includes access to <vpath>/creds/{{identity.entity.aliases.<oidc>.name}}/* where I replace <vpath> with the path of the vault oauthapp method and I replace <oidc> with the vault accessor (e.g. auth_oidc_6c8817d1 which I look up by reading sys/auth) of the enabled oidc auth method (which is a mode of vault-plugins-auth-jwt). That mess in curly brackets translates to the username, so it allows for example oauthapp/creds/dwd/*. Then the kerberos policy template allows <vpath>/creds/{{identity.entity.aliases.<kerberos>.name}}:* where I replace <vpath> with the path of the kerberos method and replace <kerberos> with the accessor of the kerberos auth method. This allows me to store from oidc at oauthapp/creds/dwd/cron/machine.domain:default and read it back via authentication from the kerberos plugin made with a name of dwd/cron/machine.domain.

I don't see anything with '%2f".

DrDaveD commented 3 years ago

I force-pushed this to rebase on current master

DrDaveD commented 3 years ago

I updated the PR to based on the latest master, adding a new parameter to the newly named nameRegex() function that allows adding optional characters to the expression, so the extra slash can be applied only to credentials.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

impl commented 3 years ago

Hey @DrDaveD,

I've been thinking about this one lately and I think I'd actually be fine with loosening the restriction on credential names to any name (i.e., any character anywhere, same as KV engine) other than a name that ends with a :. I've struggled with the way this would work from a permissions perspective, because for example we'd have to redo the STS URLs to look something like sts/a/b/c/:/x/y/z where a/b/c is the credential name and x/y/z is the token name. In this case it would be impossible to construct a sane policy that would let users create a token called x/y/z under any credential (e.g., the policy path you'd want would be "sts/*/:/x/y/z" which isn't supported; right now we can work around this with a + instead of a *). However, to be honest I think the burden here probably falls on the Vault administrator and not us as plugin maintainers to make sure the layout of their storage makes sense for their use case.

I will have to update the config path for self-creds to look like config/self/<name> instead of self/<name>/config, which will be a breaking change and require a major version bump, but I can't imagine users will have a hard time adjusting that path, and it's probably a better pattern anyway -- it mirrors the way other plugins work (e.g., metadata/ vs data/ for KV v2).

DrDaveD commented 3 years ago

That all sounds good to me.

DrDaveD commented 3 years ago

The force push was to rebase on the current main, v1.10.0

impl commented 3 years ago

Going to take this into a new branch and work out the regex to allow any characters other than ":/" in order.