FIX: Make vault kv module compatible with v2

[thenoid]

What does this PR do?

Hashicorp vault introduced 'Key Value Store Version 2.0' which dramatically modified the path/structure of the KV secrets. However both are accessed using the same calls.

This changes makes the vault module compatible with both version 1 and version 2 of the vault KV secret store

This module uses the same method/calls that the vault client does to determine if the requested path is v1 or v2 (validated via vault audit logs).

Note: Some blog posts and the GitHub issue below suggest adding extra data or metadata to your path manually. Which "worked" but was super not user friendly and could lead to interesting results if you needed both the data and metadata.

What issues does this PR fix or reference?

Some of the "modify the path" suggestions where here....though looks like I need to port the v2 logic over to the sub module as well. #50186

Previous Behavior

When attempting to access a KV2 secret, you would receive a 403 because the path was incorrect.

sudo salt-call vault.read_secret secrets/test 
.......
ERROR   ] Failed to read secret! HTTPError: 403 Client Error: Forbidden for url: https://example.com:8200/v1/secrets/test
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 204, in call
    ret['return'] = func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/modules/vault.py", line 113, in read_secret
    raise salt.exceptions.CommandExecutionError(e)
CommandExecutionError: 403 Client Error: Forbidden for url: https://example.com:8200/v1/secrets/test
Error running 'vault.read_secret': 403 Client Error: Forbidden for url: https://example.com:8200/v1/secrets/test

New Behavior

You can actually read v2 secrets!

sudo salt-call vault.read_secret secrets/test 
....
[DEBUG   ] Converting path to v2 secrets/test => secrets/data/test
[DEBUG   ] Converting path to v2 secrets/test => secrets/metadata/test
[DEBUG   ] Reading Vault secret for example.com at secrets/data/test
....
[DEBUG   ] Starting new HTTPS connection (1): example.com:8200
[DEBUG   ] https://example.com:8200 "GET /v1/secrets/data/test HTTP/1.1" 200 312

[DEBUG   ] LazyLoaded nested.output
local:
    ----------
    data:
        ----------
        i am a teapot:
            short and stout
    metadata:
        ----------
        created_time:
            2019-04-25T04:46:01.903118162Z
        deletion_time:
        destroyed:
            False
        version:
            1

Tests written?

No

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[mchugh19]

Might it make sense to use __context__ to cache the version number for some time, rather than always needing a metadata lookup on very call?

[thenoid]

Might it make sense to use context to cache the version number for some time, rather than always needing a metadata lookup on very call?

Hehe, I actually just committed something similar to that in our testing environment, once I realized the helper functions would need to be available to also fix the SDB issue. I plan to push that up tomorrow after some testing.

Throwing the WIP subject on there until I finish that up.

[twangboy]

Could we get some tests for this please?

[thenoid]

Okay, cache'ing is now working - here is example debug output fetching the same secret twice.

...
[DEBUG   ] Reading secret a first time
[DEBUG   ] Fetching metadata for infdev-vault1.hirevue.net in secrets/test
...
[DEBUG   ] Got metadata for infdev-vault1.hirevue.net in secrets/test
[DEBUG   ] Converting path to v2 secrets/test => secrets/data/test
[DEBUG   ] Converting path to v2 secrets/test => secrets/metadata/test
[DEBUG   ] Reading Vault secret for example.com at secrets/data/test
.....
[DEBUG   ] https://example.com:8200 "GET /v1/secrets/data/test HTTP/1.1" 200 312
[DEBUG   ] Reading secret a second time
[DEBUG   ] Found cached metadata for example.com in secrets/test
[DEBUG   ] Converting path to v2 secrets/test => secrets/data/test
[DEBUG   ] Converting path to v2 secrets/test => secrets/metadata/test
[DEBUG   ] Reading Vault secret for example.com at secrets/data/test
....
[DEBUG   ] https://example:8200 "GET /v1/secrets/data/test HTTP/1.1" 200 312
.....

[thenoid]

Could we get some tests for this please?

@twangboy Not going to lie, the salt's python testing framework and I are not friends. I can munge other peoples tests well enough....but new ones are my kryptonite.

[rares-pop]

you could have a get_info() that's doing the if, to avoid these 3 lines everywhere. That can return path & data (hence calling it get_info as opposed to get_path).

[Ch3LL]

@thenoid there are some vault tests here:

tests/integration/modules/test_vault.py
tests/unit/runners/test_vault.py

Do those tests help give you direction on writing tests for this fix? If not anything else I can help answer or give direction with here?

[bluemalkin]

Hi, any change of getting this fix merged ? This is blocking us from using Vault kv with salt :-)

[rares-pop]

@bluemalkin - things is, in the meanwhile, saltstack changed the branching strategy, and I'm afraid there are two things that are needed in order for this to get merged:

• port this to 'master' branch
• find some existing tests that are exercising these code and modify them, or add new tests

[bluemalkin]

@thenoid @rares-pop sure - how can we please expedite this ?

[thenoid]

In all honesty, I'm super frustrated with saltstack's QA/Community/etc and not motivated to waste more time on this right now. If you want to write some tests i'll gladly update this PR with them.

If not, just do what we've done with 21+ modules, copy my updated code to _{utils,pillar,modules}, rename it to my_vault.py or something, and then use my_vault.read_secret

[rares-pop]

@bluemalkin , @thenoid - I'm not working at saltstack and I don't have push permissions here, so I can't help you directly. I'm contributing as you guys, and I'm doing reviews now and there, in an attempt to pay back the huge benefits that salt gave us.
That being said, I encourage you to join saltstack slack channels and ask there what's going on, and ask for help: https://saltstackcommunity.herokuapp.com/

In fact I went ahead and posted a message on 'develop' channel to ask for someone from saltstack to review this and take any actions if that's wanted/possible.

[waynew]

@thenoid Hey, thanks for the PR!

Like @rares-pop said, we have changed the branching/release strategy, which can be frustrating - that's totally understandable. It's also entirely understandable if you're not super into contributing to Salt right now - life happens 🙂We absolutely understand that. We welcome and appreciate whatever level of contribution you're up for right now.

---

To get the PR merged there are basically two options:

1. You could rebase this PR on master and add some tests.

   You've said that you're not 100% confident writing tests, but if you're interested in writing them, someone could help train you there. This week is pretty busy for the whole team, and I'm out the next week - Thanksgiving holiday in the US - but I could totally be available the first week in December. Might be someone else available before then, though with SaltConf is happening today, and Thanksgiving next week, delays should be expected.

2. You could do nothing, and either the core team or someone else in the community will write the tests.

   We've added the Needs Testcase label - we know that we will need to write tests, and as soon as time allows we will - or another community member could.

If you or anyone else is interested in rebasing, I've recorded a little screencast: https://asciinema.org/a/PWyeqbfV7ww5sT3AaX9yoIbF4

If you rebase, you'll need to edit this PR to point towards master - there's a button at the top right.

Are you at all interested in finishing this up, or would you like to let someone else take it over?

[Ch3LL]

Looks like this was added to master here: #55842 so im going to close here.

thanks @mchugh19 !!