Salt: BUG: Vault with KV Secrets Engine - Version 2 breaks SDB Vault

[klafkoff]

Description of Issue/Question

I am using Vault with KV Secrets Engine - Version 2. This breaks things with SDB since the response from vault is different than KV Secrets Engine - Version 1.

Setup

Make sure you have Vault configured and enable KV Secrets Engine - Version 2:

vault secrets enable -version=2 kv
vault kv enable-versioning secret/
vault kv patch secret/salt/pillar_data wew=lad

/etc/salt/master.d/vault.conf:

sdb_vault:
  driver: vault

vault:
  url: https://vault_host:8200
  verify: False
  auth:
      method: token
      token: wewladwewladwewlad
  policies:
      - saltstack/minions
      - saltstack/minions/{minion}

This patches the sdb.get function to work correctly. I haven't looked into the other sdb functions yet.

diff --git a/salt/sdb/vault.py b/salt/sdb/vault.py
index 5980543..81fedbb 100644
--- a/salt/sdb/vault.py
+++ b/salt/sdb/vault.py
@@ -108,7 +108,9 @@ def get(key, profile=None):
             response.raise_for_status()
         data = response.json()['data']

-        return data[key]
+        # If you're using KV Secrets Engine - Version 2
+        # The data is inside the 'data' key
+        return data[key] if data.get(key) else data['data'][key]
     except Exception as e:
         log.error('Failed to read secret! %s: %s', type(e).__name__, e)
         raise salt.exceptions.CommandExecutionError(e)

Here's what the response looks like from Vault with kv2, from the data = response.json()['data'] in /opt/saltstack/lib/python2.7/site-packages/salt/sdb/vault.py:get()

{u'data': {u'wew': u'lad',}, u'metadata': {u'created_time': u'2018-10-23T20:21:55.042755098Z', u'destroyed': False, u'version': 13, u'deletion_time': u''}}

You'll get an error like the following when running the command below:

$ sudo salt-run sdb.get 'sdb://sdb_vault/secret/data/salt/pillar_data/wew'
[ERROR   ] Failed to read secret! KeyError: u'wew'
Exception occurred in runner sdb.get: Traceback (most recent call last):
  File "/opt/saltstack/lib/python2.7/site-packages/salt/client/mixins.py", line 387, in _low
    data['return'] = self.functions[fun](*args, **kwargs)
  File "/opt/saltstack/lib/python2.7/site-packages/salt/runners/sdb.py", line 27, in get
    return salt.utils.sdb.sdb_get(uri, __opts__, __utils__)
  File "/opt/saltstack/lib/python2.7/site-packages/salt/utils/sdb.py", line 46, in sdb_get
    return loaded_db[fun](query, profile=profile)
  File "/opt/saltstack/lib/python2.7/site-packages/salt/sdb/vault.py", line 122, in get
    raise salt.exceptions.CommandExecutionError(e)
CommandExecutionError: u'wew'
[INFO    ] Runner completed: 20181023165316837868

Steps to Reproduce Issue

sudo salt-run sdb.get 'sdb://sdb_vault/secret/data/salt/pillar_data/wew'

Versions Report

Salt Version:
Salt: 2018.3.2-1735-g84e2dcf

Dependency Versions:
cffi: 1.11.5
cherrypy: Not Installed
dateutil: 2.7.3
docker-py: 3.5.0
gitdb: 2.0.4
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.10
libgit2: 0.27.4
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: 2.18
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: 0.27.2
Python: 2.7.15 (default, Aug 24 2018, 16:24:40)
python-gnupg: 0.4.3
PyYAML: 3.13
PyZMQ: 17.1.2
RAET: Not Installed
smmap: 2.0.4
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.2.5

System Versions:
dist: centos 7.5.1804 Core
locale: UTF-8
machine: x86_64
release: 3.10.0-862.11.6.el7.x86_64
system: Linux
version: CentOS Linux 7.5.1804 Core

[gtmanfred]

i don't understand what has changed? because we are already pulling a data key off the top of the response, is it now two nested dicts called data?

         data = response.json()['data']  # We always pull the "data" key off the response dictionary.

-        return data[key]
+        # If you're using KV Secrets Engine - Version 2
+        # The data is inside the 'data' key
+        return data[key] if data.get(key) else data['data'][key]

[gtmanfred]

Also, i ran this through our tests, and it does not allow us to upgrade our vault container for tests to the newest vault version.

https://github.com/saltstack/salt/blob/fluorine/tests/integration/sdb/test_vault.py#L40

If you can submit a PR that allows us to upgrade that to the latest version of vault i will love you forever <3

Thanks,
Daniel

[klafkoff]

Yeap, it's nested data inside data. If we dump the full json back from Vault, it shows this:

/opt/saltstack/lib/python2.7/site-packages/salt/sdb/vault.py:get()

        ... existing code
        log.error('%s', response.json())  <----- lets see what's here
        data = response.json()['data']

{u'lease_id': u'', u'warnings': None, u'wrap_info': None, u'auth': None, u'lease_duration': 0, u'request_id': u'5345ac76-0b6d-0988-8e1f-1da33466a50b', u'data': {u'data': {u'wew': u'lad'}, u'metadata': {u'created_time': u'2018-10-23T20:21:55.042755098Z', u'destroyed': False, u'version': 13, u'deletion_time': u''}}, u'renewable': False}

Note the u'data': {u'data':

Also I should note, I had to change the path for the pillar/vault config in /etc/salt/master.d/ext_pillar.conf:

ext_pillar:
 - vault: path=secret/data/salt/pillar_data

where before with the v1 engine, I was using

vault: path=secret/salt/pillar_data

[gtmanfred]

ugh, ok thanks vault... i am working on fixing the tests for this, then we can test and make sure they still pass with the change

[gtmanfred]

Yeah, so the problem with the way we use vault is that we hard code and only use v1 of k/v pairs.

https://github.com/saltstack/salt/blob/2018.3/salt/modules/vault.py#L138

So even with your change, the tests still fail, because the vault module needs to be updated to support using k/v version 2

[joe-bowman]

I've come across the same issue; adding:
data = data.get('data', data) on line 110, seems to do the trick, and is backwards compatible with v1 k/v engine.

EDIT: I didn't see the path suggested above :)

[joe-bowman]

fyi @gtmanfred - that refers to v1 of the vault API, not the version of the kv secret engine.

Bumping the vault container to 0.11.5 (latest) still won't help, as the default secret engine is still v1; we'll need to instantiate a second KV store engine and run the battery of sdb.get/set tests again.

[gtmanfred]

a pr to do that would be greatly appreciated.

[gtmanfred]

@saltstack/team-triage

[fsuste]

@gtmanfred Is there any progress on this or any way one can help?

[joe-bowman]

Sorry, I haven't got anywhere with this yet; very bandwidth constrained right now.

As a quick nasty workaround, I put this together: https://github.com/ChorusOne/saltstack-module-vaultkv2

It's a c&p of the vault sdb module, with the fix; add that repo to your gitfs_remotes and specify vaultkv2 as the sdb driver instead of vault.

Comes with no assurances, but I'm using it in prod :)

[giorgiosironi]

FWIW, I am using directly salt['vault'].read_secret() and for that you can work around this problem by using secret/data/ prefixes rather than secret/ ones; the output is a complex dictionary.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[mchugh19]

Not stale

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[mchugh19]

This is corrected in #55842

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[mchugh19]

Don't close until PR is merged

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[sagetherage]

silly stale bot you're so not just for kids