Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial master-port of #54039 (support Vault kv version2) #55842

Merged
merged 19 commits into from
Apr 13, 2020

Conversation

mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Jan 11, 2020

What does this PR do?

Attempts to port #54039 to master while adding a unit test for the v2 functionality
This is @thenoid's work to allow salt to support both version 1 and version 2 of Vault's kv secrets backend.

What issues does this PR fix or reference?

See notes on #54039
Also fixes #50186

Previous vs New Behavior

Salt's Vault support really only handled the v1 kv interface. @thenoid's PR added logic to support the correct URLs for both v1 and v2 kv backends.

Vault also changed the return from v1 and v2 kv backends. In v1, secrets were returned. In v2, a data structure is returned like the following:

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

The secret data that vault.read_secret used to return is now behind another ['data'] lookup. Since this was a change vault-side, this PR does not attempt to retain the old output (maybe users want some of the new data?), and instead publishes what was supplied by vault.

Thus for users on the v1 kv, running a command like vault.read_secret 'secret/super/secret returns a result like: akey: avalue
While users of the v2 kv backend running the same command will get a result like displayed above and will need to walk the returned 'data' key to get the same result.
{% set supersecret = salt['vault'].read_secret('secret/my/secret')['data'] %} (note the trailing 'data')

That said, the execution module's read_secret, and the sdb module's get, both support filtering on the key of the vault secret. So this PR also extends those functions to support the v1 and v2 returns to work as expected when a key is specified.

Tests written?

Yes
There should really be some docs on the docker setup. I tried to run the integration tests locally so I could investigate updating the vault image, and also running a version 2 kv backend for awesome actual tests, but I'm not sure where the vault docker image is being pulled from.
Figured it out! Added module and sdb integration tests with the current version of vault's docker image as it's needed for kv v2 support.

@mchugh19 mchugh19 requested a review from a team as a code owner January 11, 2020 18:05
@ghost ghost requested a review from cmcmarrow January 11, 2020 18:05
@thenoid
Copy link

thenoid commented Jan 13, 2020

Thank you for doing the needful and for crediting. <3

@mchugh19
Copy link
Contributor Author

mchugh19 commented Jan 16, 2020

All green tests! In your face flaky decorator!

@thenoid
Copy link

thenoid commented Mar 16, 2020

@Ch3LL ???

salt/modules/vault.py Outdated Show resolved Hide resolved
salt/modules/vault.py Outdated Show resolved Hide resolved
tests/integration/modules/test_vault.py Outdated Show resolved Hide resolved
response = __utils__['vault.make_request'](
'POST',
url,
profile,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate why we are removing profile here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unused in the code. If that's a standard kwarg I'm happy to leave it in.

@Ch3LL Ch3LL self-assigned this Mar 25, 2020
@mchugh19
Copy link
Contributor Author

mchugh19 commented Apr 7, 2020

Before merging, it's probably worth discussing the topic brought up in the description. Today with vault kv v1, vault.read_secret 'secret/super/secret returns a result like: akey: avalue
While users of the v2 kv backend running the same command will get a result like displayed above and will need to walk the returned 'data' key to get the same result.
{% set supersecret = salt['vault'].read_secret('secret/my/secret')['data'] %} (note the trailing 'data')

This is done because vault changed the output when using kv v2 to

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

Where there are separated data and metadata keys. Maybe it makes sense to return data by default, and to have a toggle on metadata?
vault.read_secret /secret/my/path metadata=True

@thenoid
Copy link

thenoid commented Apr 7, 2020

So with metadata=True it'd return kv2 style {'data': {whatever}, 'metadata': {something}}?

metadata=False it'd be a kv1 style?

I think that's a fair abstraction

Before merging, it's probably worth discussing the topic brought up in the description. Today with vault kv v1, vault.read_secret 'secret/super/secret returns a result like: akey: avalue
While users of the v2 kv backend running the same command will get a result like displayed above and will need to walk the returned 'data' key to get the same result.
{% set supersecret = salt['vault'].read_secret('secret/my/secret')['data'] %} (note the trailing 'data')

This is done because vault changed the output when using kv v2 to

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

Where there are separated data and metadata keys. Maybe it makes sense to return data by default, and to have a toggle on metadata?
vault.read_secret /secret/my/path metadata=True

@mchugh19
Copy link
Contributor Author

mchugh19 commented Apr 8, 2020

As described in the comments, the previous behavior of vault.read_secret has been retained. Now read_secret will return the secret data on both kv v1 and v2. Anyone needing to access the full return, can use vault.read_secret /secret/path metadata=True

@dwoz dwoz merged commit 1588d87 into saltstack:master Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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