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

Kv2 support with a specified secret key #54

Merged
merged 8 commits into from
Aug 22, 2022

Conversation

firstnevyn
Copy link
Contributor

Pull Request (PR) description

Adds support for Vault KV2 format secrets and allows addressing a key within a vault secret

With 0.4.0 a kv2 returns a complete hash including creation version and other metadata. this fails to be converted to a string causing an agent compile error

This Pull Request (PR) fixes the following issues

Fixes #21
Fixes #25

@firstnevyn
Copy link
Contributor Author

This adds a new optional parameter 'vault_key' which can address a key within a secret in a kv2 secretengine. it does require padding the unused arguments because deferred takes a list of values. but this means that existing use cases will still work while supporting the current Vault kv secretengine

@firstnevyn firstnevyn marked this pull request as ready for review August 4, 2022 14:06
@firstnevyn
Copy link
Contributor Author

Also fixes #30

@bastelfreak bastelfreak added the enhancement New feature or request label Aug 11, 2022
@@ -5,6 +5,7 @@
optional_param 'Optional[String]', :vault_cert_path_segment
optional_param 'String', :vault_cert_role
optional_param 'String', :vault_namespace
optional_param 'String', :vault_key
Copy link
Member

Choose a reason for hiding this comment

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

DO we need to allow empty strings, or could we also use:

Suggested change
optional_param 'String', :vault_key
optional_param 'String[1]', :vault_key

(which enforces a minimal string length of 1, or no string at all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should... because of how this ends up being called sometime you need to pass in the empty string.
and yes while it's the final argument in the list it doesn't matter.. but if we ever need another argument it's problems.

A better way to handle this that works well with deferred would be nice but isn't obvious to me.

Deferred doesn't take named arguments (for better or mostly worse) so for example when not using namespaces you need to pass the empty string to access the key parameter

@bastelfreak
Copy link
Member

@firstnevyn thanks for the PR! Can you take a look at the existing tests and maybe add one for this feature? We have unit tests in https://github.com/voxpupuli/puppet-vault_lookup/blob/master/spec/functions/lookup_spec.rb and acceptance tests in https://github.com/voxpupuli/puppet-vault_lookup/blob/master/spec/acceptance/lookup_spec.rb

@firstnevyn firstnevyn marked this pull request as draft August 19, 2022 03:39
@firstnevyn firstnevyn marked this pull request as ready for review August 22, 2022 03:54
@firstnevyn firstnevyn changed the title Initial run at kv2 support RFC Kv2 support with a specified secret key Aug 22, 2022
@bastelfreak
Copy link
Member

thanks for the work!

@bastelfreak bastelfreak merged commit dfcf1fe into voxpupuli:master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants