[Feedback Wanted] upcoming kv modules, path, and mount_point

[briantist]

I'd like to introduce modules and plugins for the kv secrets engine soon, but I'm debating about how to specify the mount point and path.

In most of the Vault libraries (include the hvac library we use in this collection), the mount point of the secret engine must be specified separately, and the path is specified relative to that, for example:

https://hvac.readthedocs.io/en/stable/source/hvac_api_secrets_engines.html#hvac.api.secrets_engines.KvV2.read_secret_version

read_secret_version(path, version=None, mount_point='secret')

So if you want to read a secret at the path secret/something/else/password, you would call that as:

read_secret_version('something/else/password', mount_point='secret')

(or in this case you can leave out mount_point because the default mount is used).

Notably though, the Vault CLI does not make this separation:

vault kv get secret/something/else/password

---

As it relates to us, the way of calling as a module could be like this:

- community.hashi_vault.vault_kv_get: 
    backend_mount_point: secret  # optional with default
    path: something/else/password

or like this:

- community.hashi_vault.vault_kv_get: 
    path: secret/something/else/password

The way the CLI does it is probably more user friendly in the case of say, copy/pasting your path from the Vault Web UI into a command, but other modules dealing with secret backends will likely need to take a mount point parameter of some kind, and consistency with those might be better, especially when you consider module_defaults and being able to set it once.

I'm somewhat torn on which direction to go.

There's also the option to take both forms, but this becomes a parsing and testing burden. Any form of accepting a single complete path requires internal parsing anyway because we need to separate it to pass it to hvac, but when the path is always assumed full, we can always assume the first component behind / is the mount. Actually, we can't assume this, because mounts can contain zero or more slashes /, see #229 (comment)

If a path can be either full or not full, and we take a mount point option, we need a way to determine whether or not to use the mount option. If the mount had no default, that would be relatively straightforward, but having no default runs counter to both the CLI and most of the libraries out there.

Having a default means we can't tell whether a given path is meant to be full unless we take another option to control that (use_mount_point? path_is_full?), or we assume that if the first component matches the default that the path is full, but that is error prone: if you have a secret path under the secret mount, then path: secret/my_secret with mount: secret could not be used to access it.

Also for consideration: if we want to define a backend_mount_point option in one place for say, all kv content, we can put that in a doc fragment. But if we have a module like this that won't use it, because it parses it out of path, we can't opt-out for that one option, so we either show the option in docs with a note not to use it, or we make a redundant doc fragments, or we define this option separately in every module and plugin.

---

So I'm opening this discussion in the hopes of getting feedback from users of the collection!

[claco]

I don't use the Ansible vault collection things much any more, and even then, not much either. But I have been doing a lot of command line building lately using hvac. The cli had a ton of work around making things being named consistently with respect to type, mount_point (which is type of mount_point isn't set), and the backend configuration paths.

Being from a Rails background before my Python time, I'm rather fond of "batteries included, but swappable".

- community.hashi_vault.vault_kv_get: 
    path: secret/something/else/password

I'm a +1 for convention over configuration here, especially if it means there is no more "where does data go" in the middle of path any more. That still hurts my brain even though it's been forever since KVv2 dropped.

In this case, if it's secret or team-secrets or any other custom mount path, one can always just use mount_path = path.split("/")[0]

If someone needs to set it because convention is failing us, backend_mount_point: can help out.

My only concern then is one of naming, and consistency with hvac that people might be familiar with. In that case, in hvac:

read_secret_version(path, version=None, mount_point='secret')

I would lobby for the same key/variable names in ansible/yaml:

- community.hashi_vault.vault_kv_get: 
    mount_point: secret
    path: secret/something/else/password

But then, I get stuck on path here not being the same path that we would pass right to hvac. path has secret/, but I think path in hvac then is something/else/password. Unless of course, we pop that first level of the path off if it matches something explicitly set in path in ansible before passing it to path in hvac.

mp, path = ansible_path_value.split('/', 1)

Generally for the idea though!

[briantist]

@claco thanks!

So the issue with having to include /data/ in the original single plugin (hashi_vault) is because that plugin was always doing a generic "read" on any given path, so you need to give it direct API paths (that it then later tried to interpret the result as a kv v2 value is something that I'm looking to remedy by having separate plugins with defined purposes).

If you'd like more about the history and such of the hashi_vault lookup, I have a whole page on it here!

So no matter what option we go with concerning path and mount point, these will be kv-focused plugins and modules, so no having to use /data/. There may even be separate kv v1 and v2 plugins, or perhaps more likely, accepting a version option to ensure the right conventions are used (topic for a different discussion though 😉).

---

Regarding the option names not matching hvac, this will be somewhat unavoidable. mount_point had been used for a long time previously to refer mount points as they relate to auth methods, in this collection, and those are shared between every plugin, so that's pretty set unless we make a breaking change with a lot of announcement.

For path, well, yeah that's going to probably have even more overlapping use cases in the future, so it may be unavoidable to sometimes qualify it, or to have it mean something different.

I tend to follow hvac's conventions where possible, but what's more important is for the option names to be clear to Ansible users and consistent within the collection, where it makes sense.

Also, in some ways the original plugin and the collection have had options that try to mimic or make use of the CLI's conventions and (notably) environment variables, but I am also trying to move away from that (which is just to say: alignment with the CLI is not a goal, except where alignment happens to also be a good option for the collection).

[dr4Ke]

Thanks for working on this.

Path parsing

I see another catch: the mount point name may contain the / character too. Although most people would consider it's a bad idea to use it, we can't ignore the possibility. It makes virtually impossible to extract the mount point from the path if there is a default mount point value.

I see two viable alternatives:

• an optional mount_point parameter, without default value. The path parameter would either be the full path (including the mount point, that MUST not contain the / character) or the relative path if the mount_point parameter is set.

These two tasks would do exactly the same:

- community.hashi_vault.vault_kv_get: 
    mount_point: secret
    path: relative/path/to/secret

- community.hashi_vault.vault_kv_get: 
    path: secret/relative/path/to/secret

When using a / in the mount point, the user would be forced to use the mount_point parameter:

- community.hashi_vault.vault_kv_get: 
    mount_point: some/mount/point
    path: relative/path/to/secret

• mount_point and path (relative) both mandatory.

Naming

I would use mount_point as the mount point parameter name for kv stores, too. I suppose that any vault user understands that the mount point is context related. And then that the mount point for authenticating is different than the mount point to manipulate a secret.

[briantist]

Also for other readers, want to point out that the reason this suggestion states that a mount point with a / MUST use the mount_point parameter is because the API path is NOT simple concatenation of mount_point + path; it's the insertion of the /data/ or /metadata component, that must be put in between the mount point and the path, and that's why those two components must be separated, because we otherwise don't know where to put the other component.

So that got me wondering, how does the CLI know? Because it does work correctly.

Using MMock (the same same API mocking/proxy we use in CI), I was able to see what it's doing (this was much easier than trying to follow the source code tbh).

I created a kv v2 backend mounted at sec/ver/2 as its mount point.

I created a secret at path new/one under that mount, then read it from the CLI, as in

vault kv get sec/ver/2/new/one

It first makes a call to /v1/sys/internal/ui/mounts/sec/ver/2/new/one, and that call returns data that includes the mount path.

That's how it's able to then make a direct call to /v1/sec/ver/2/data/new/one without any trial and error.

We could do the same when mount_point is unspecified, but I am worried about API stability:
https://www.vaultproject.io/api-docs/system/internal-ui-mounts

Due to the nature of its intended usage, there is no guarantee on backwards compatibility for this endpoint.

They also don't even document the form used by the CLI, which adds the full provided path to the end of the request to have it return the mount info for that one path instead of returning info on all mounts.

I will reach out to HashiCorp to see if I can get any info on that.

We could possibly call sys/mounts to get a listing and then compare, but I'm concerned about permissions requirements for that. The UI path mentions being unauthenticated but I'm not able to read it without a token as far as I can tell.

I also don't like additional round trips, but permissions and round trip issues are things that can be avoided by specifying the optional mount_point option, as we won't need to ask Vault in that case.

I have confirmed as well that you can't mount a backend in a path that could result in ambiguous full paths, for example I tried to enable another kv backend on sec/ver and it didn't allow it because sec/ver/2 is already mounted.

We could do a procession of attempts:

No mount point specified -> call /sys/internal/ui/mounts/<path> -> on failure: try to call /sys/mounts/ -> on failure: try to assume first component?

[briantist]

For posterity, posting the request and response details of the UI call:

Request:

{
    "scheme": "http",
    "host": "127.0.0.1",
    "port": "8900",
    "method": "GET",
    "path": "/v1/sys/internal/ui/mounts/sec/ver/2/new/one",
    "queryStringParameters": {},
    "fragment": "",
    "headers": {
        "Accept-Encoding": [
            "gzip"
        ],
        "User-Agent": [
            "Go-http-client/1.1"
        ],
        "X-Vault-Request": [
            "true"
        ],
        "X-Vault-Token": [
            "47542cbc-6bf8-4fba-8eda-02e0a0d29a0a"
        ]
    },
    "cookies": {},
    "body": ""
}

Response:

{
    "statusCode": 200,
    "headers": {
        "Cache-Control": [
            "no-store"
        ],
        "Content-Length": [
            "443"
        ],
        "Content-Type": [
            "application/json"
        ],
        "Date": [
            "Sun, 27 Mar 2022 15:41:56 GMT"
        ],
        "Strict-Transport-Security": [
            "max-age=31536000; includeSubDomains"
        ]
    },
    "cookies": {},
    "body": "{\"request_id\":\"930cbeab-7816-8f37-eac3-bef9fcbab509\",\"lease_id\":\"\",\"renewable\":false,\"lease_duration\":0,\"data\":{\"accessor\":\"kv_3702f484\",\"config\":{\"default_lease_ttl\":0,\"force_no_cache\":false,\"max_lease_ttl\":0},\"description\":\"\",\"external_entropy_access\":false,\"local\":false,\"options\":{\"version\":\"2\"},\"path\":\"sec/ver/2/\",\"seal_wrap\":false,\"type\":\"kv\",\"uuid\":\"aae6bc83-c2dc-d154-d99b-6edcfe28e39b\"},\"wrap_info\":null,\"warnings\":null,\"auth\":null}\n"
}

And the response body separately:

{
  "request_id": "930cbeab-7816-8f37-eac3-bef9fcbab509",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "accessor": "kv_3702f484",
    "config": {
      "default_lease_ttl": 0,
      "force_no_cache": false,
      "max_lease_ttl": 0
    },
    "description": "",
    "external_entropy_access": false,
    "local": false,
    "options": {
      "version": "2"
    },
    "path": "sec/ver/2/",
    "seal_wrap": false,
    "type": "kv",
    "uuid": "aae6bc83-c2dc-d154-d99b-6edcfe28e39b"
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

[briantist]

Actually the body response is making me realize something else: I was convinced that kv version would have to either ne specified as an option, or that we'd have separate plugins for dealing with kvV1 vs kvV2, but the mount response does contain the version, which is also how the CLI knows to construct the request, and interpret the result.

We could do the same, if we do inferencing like this.

When mount_point is specified though, kv_version would also need to be specified, or we would still do inferencing on mount_point unless both are supplied, something like that...

[a01fe]

For good or bad, we've chosen to organize paths like org/env/app/kind/thing which means we have lots of mount points. I've written a wrapper around vault_kv_get that finds the mount point by comparing the secret with mount points returned by /sys/internal/ui/mounts as well as adding /data for kv2 mounts. This is consistent with vault CLI and web UI, and users don't have to know what a secret's mount point is.

There's a prefix option so we can have:

vars:
  ansible_vault_kv_get_prefix: "org/env/app"

And look up secrets with lookup('kind/thing:field").

[briantist]

Thanks @a01fe ! That's something I'm considering (above), my main concern will be API stability. If we rely solely on sys/internal/ui/mounts with no fallback option, then we run the risk of our stuff breaking at any time. Being an internal/unsupported API, it could be changed at any time in any release of Vault and there isn't even a guarantee it would be announced.

I do like the idea of using it, but I have to weigh that against the additional burden of maintaining a fallback, documenting what could go wrong for end users, and how a fallback would behave differently.

I'm also going to see if HashiCorp has any additional advice on this or plans in their roadmap to provide a supported API with the same functionality.

[digivava]

Hello from the Vault Developer Experience team at HashiCorp! I wanted to chime in on this discussion since our team has actually been talking about the pathing differences between KV v1 and v2 a lot these days.

My team has actually observed that the shortened path used by the KV CLI commands (my-kv/foo) has been causing a significant amount of confusion for new Vault users (since policy files, vault read, and our official Go client library all use the full path my-kv/data/foo to refer to secrets.) Because of this, we are actively trying to move away from this syntax while still maintaining backwards compatibility, so we're in the process of creating a new syntax for the CLI that behaves more like HVAC, VaultSharp, and our other community libraries, where the mount path is separate from the secret path.

While vault kv get my-kv/foo arguably looks cleaner, we want to mitigate the confusion caused by the fact that my-kv/foo looks like a path but actually isn't, so vault kv get -mount=my-kv foo will be the new syntax that (while entirely optional to adopt) will be promoted in our Getting Started guides and other introductory material.

Because of that work which is currently in progress, it might be better for other tooling to avoid following the "shortened path" style of the current CLI. If you feel it works better for you then of course feel free to pursue it, but you will likely run into what we did with the CLI: that even though it looks simpler, you may find many users bang their heads against it as they wonder why the real path my-kv/data/foo that they grabbed from their policy file or other API call doesn't work in this circumstance.

Something like:

- community.hashi_vault.vault_kv_get: 
    mount_path: secret
    secret_path: password
    version: 2

could be a more similar approach to what we're trying to do going forward, and similar to what HVAC and other libraries do today.

On the Ansible side, when you see that version is "2", that means the Ansible plugin will insert the /data in between the mount path and secret path in the API call. When version is "1", no need to add /data in there.

Metadata could be worked with the same way but with community.hashi_vault.vault_kv_metadata_get, similar to the vault kv metadata get command in the CLI.

(I'm personally not very familiar with Ansible so forgive me if I've missed anything here or said something that obviously wouldn't work with Ansible collection syntax!)

[briantist]

@digivava welcome! Thank you for this insight; this really helps clear up the path (😏) going forward in my mind.

I think then I will not seek to support the CLI path format, and will instead look to take separate mount point and path parameters to construct an API path.

[claco]

I see another catch: the mount point name may contain the / character too.

Is that true and tested? I swear I tried mount points with / in them a few weeks ago, and it wasn't allowed in the vault cli, as the enabling if auth methods griped about the value. If I changed / to -, it was fine. Now I have to revisit this myself. :-) Maybe this was only true for some backends and not others?

[briantist]

It is true and I did test it with the CLI. I did not test it with auth methods only secrets backends.

[briantist]

Update: it works with auth methods too:

vault auth enable -path aws/region/us-east-1 aws
Success! Enabled aws auth method at: aws/region/us-east-1/

[jmarcelletti]

I hope I am misunderstanding this because this feels worse. You currently have 2 ways of doing this.. the CLI path and the API path.. and instead of solving this by unifying to the proper (API) path, you add a 3rd way to add more confusion?

Please just add a new subcommand "vault kv2" that allows you to use the API path but still does the pretty presentation of data.. this means if I want to list I would do "vault kv2 list kv/metadata/blah" and if I want to read it's "vault kv2 get kv/data/blah". Uniform paths are the number 1 complaint from my users and this simple backward compatible solution solves it.

[briantist]

Hi @jmarcelletti , it seems like this was meant to be a reply to #229 (comment) , but is probably better raised through HashiCorp's channels.

Personally though, I think it's a step in the right direction to start making it clearer that the mount point and the (rest of the) path are distinct, rather than having a single opaque path where it isn't obvious.

[jmarcelletti]

I did thanks/sorry about that.

[briantist]

For anyone who was interested, I have started implementation of the first KV content (for reads):

• add content for KV get operations #246