Add lookup_key function to get at a specific key value #21

jadestorm · 2019-05-23T21:02:19Z

Pull Request (PR) description

After trying this module out -- I found I could not use it in actual "useful" way yet. I have a path ssl/test which has two keys under it -- cert and key. When I retrieve that, I am unable to get at the either of the subkeys in any way I could figure out. If I am just missing it, please let me know how. =) But I couldn't, for example, do $d['cert'] or any form of that that I could discover. Puppet rejected it as "this is not a hash, it is an object". I saw a few semi weird "double deferred" solutions but they weren't quite working either. I whipped up what is basically a copy of the lookup function but that accepts a key argument. It is working great in my setup so far.

Please note I do not want you to merge/accept this as is -- I would like a chance to clean it up, add some tests, etc before it is potentially merged.

I don't really like doubling up on the functions within the Puppet function definition -- in fact at first I tried to do a quick lookup_key that actually called lookup, unwrapped the return, pulled the key, rewrapped that, and returned it. However I was in a bit of a hurry and did it this way for the moment.

Good? Bad? Meh? Seemed like a quick way to get at what I needed. =) If you like the idea but have some suggestions on how to improve it please send them my way. I've also left it open to maintainer edits so have at it. (I'm using master in production currently)

This Pull Request (PR) fixes the following issues

n/a

ndelic0 · 2019-09-03T09:17:12Z

Anything blocking this PR @jadestorm ?

jadestorm · 2019-09-03T15:27:14Z

@ndelic0 Not really. =) The main reason I said do not merge is because I didn't know if y'all thought lookup_key was a good concept since it's a lot of duplicated code. But if you like it as-is, feel free to merge!

dhollinger

LGTM.

Have a couple minor comments, but neither are blockers

dhollinger · 2019-09-04T14:38:14Z

lib/puppet/functions/vault_lookup/lookup_key.rb

+    if vault_url.nil?
+      Puppet.debug 'No Vault address was set on function, defaulting to value from VAULT_ADDR env value'
+      vault_url = ENV['VAULT_ADDR']
+      raise Puppet::Error, 'No vault_url given and VAULT_ADDR env variable not set' if vault_url.nil?


Might want to also check for an empty string here, not just nil?

dhollinger · 2019-09-04T14:38:36Z

lib/puppet/functions/vault_lookup/lookup_key.rb

+    # and port; it's possible to generate a URI::Generic when a scheme
+    # is not defined, so double check here to make sure at least
+    # host is defined.
+    raise Puppet::Error, "Unable to parse a hostname from #{vault_url}" unless uri.hostname


Unless this accounts for my above concerns with an empty string.

It does in that it freaks out if it's not a valid url (and an empty string qualifies) -- it's not really the cleanest implementation. That said it mirrors what is (or was if it's changed) in lookup.rb =) I somewhat wonder if this would be better served as an additional optional argument to lookup itself. However it was a quick solution and I'm using it in production so it's got that goin' for it. =D

Going to add another comment related to this kinda.

jadestorm

I guess some main things to think about here are --

Would you prefer that I look into implementing this as a third optional param instead of a separate call entirely?
I just noticed I didn't include any documentation what-so-ever. I can certainly do that.

I'm absolutely not opposed to you accepting it as is -- but I'm also happy to put some work into "cleaning it up" before you merge it. I've even learned quite a bit of interesting tidbits about Deferred since I did this originally that may be worth mentioning in the README.

I didn't want to write any README type stuff until #1 was answered. (meaning -- do you like it as a separate call, or would you prefer me to work out having it part of lookup.rb)

ndelic0 · 2019-09-04T15:51:13Z

I'd rather go with separate call. That's just my 2 cents.

dhollinger · 2019-09-04T15:57:12Z

A separate call should be fine, but adding the documentation would be great. Also, if you could add some tests (if you haven't already), that should also probably be done

dhollinger

Needs tests and docs

dhollinger · 2019-09-04T15:58:03Z

Added labels for tests and docs

jadestorm · 2019-09-04T17:35:54Z

Sounds good! Will add those hopefully today. =)

jadestorm · 2019-09-04T17:36:17Z

I went ahead and took out the DO NOT MERGE note as well. =)

jadestorm · 2019-09-05T18:28:07Z

Ok! How's that looking?

mjtice · 2019-10-16T12:54:47Z

This would be great to have. I'm curious as to the status of this PR.

alexjfisher · 2020-02-15T17:25:59Z

diff -Naur lookup.rb lookup_key.rb 
--- lookup.rb	2020-02-15 17:14:36.778840013 +0000
+++ lookup_key.rb	2020-02-15 17:14:36.778840013 +0000
@@ -1,10 +1,11 @@
-Puppet::Functions.create_function(:'vault_lookup::lookup') do
-  dispatch :lookup do
+Puppet::Functions.create_function(:'vault_lookup::lookup_key') do
+  dispatch :lookup_key do
     param 'String', :path
+    param 'String', :key
     optional_param 'String', :vault_url
   end
 
-  def lookup(path, vault_url = nil)
+  def lookup_key(path, key, vault_url = nil)
     if vault_url.nil?
       Puppet.debug 'No Vault address was set on function, defaulting to value from VAULT_ADDR env value'
       vault_url = ENV['VAULT_ADDR']
@@ -35,7 +36,8 @@
       raise Puppet::Error, 'Error parsing json secret data from vault response'
     end
 
-    Puppet::Pops::Types::PSensitiveType::Sensitive.new(data)
+    data_from_key = data[key]
+    Puppet::Pops::Types::PSensitiveType::Sensitive.new(data_from_key)
   end
 
   private

Given the very small difference with the existing lookup function, I don't think we should create this near duplicate.

Puppet rejected it as "this is not a hash, it is an object".

vault_lookup::lookup($path)['some_key']

might not work, but does

unwrap(vault_lookup::lookup($path))['some_key']

?

alexjfisher

I think we should come up with a solution that doesn't duplicate so much code.

Possible alternative to voxpupuli#21 This solution uses a second dispatch instead of having either an almost entirely duplicated function or introducing a 2nd optional parameter.

jadestorm · 2020-02-21T19:52:07Z

I definitely agree that I would prefer it not be an entirely separate function that does almost the same thing. Originally I whipped up this solution due to a need and not being familiar enough with it to go for some sort of option/param based solution. I'm all for whatever route you are going down as long as it accomplishes the same thing.

As far as the unwrap goes -- question, does that make the vault lookup come from the client or from the puppet master? I'm going to test it in a moment here.

jadestorm · 2020-02-21T19:58:22Z

Ok so -- this works fantastically actually. (unwrap) And if I wrap that whole thing in a Deferred I believe that makes sure the client is making the call rather than the Puppet master, and it still works in that scenario as well. =) So y'all can free free to reject/close this if you want!

alexjfisher · 2020-02-21T20:27:12Z

@jadestorm Cool. Fantastic you got it working! A doc PR would be really helpful though ;) Other people have been struggling with how to use this module.

jadestorm · 2020-02-24T15:43:58Z

@alexjfisher hrm I might have jumped the gun a little. I think I ran my test puppet run before my masters had updated their copy of my environment.

Basically, while the unwrap line works -- I am 90% sure that means the lookup is coming from the Puppet master instead of the agent. (we don't actually restrict any secrets specifically to an agent, but I know some folk that do, plus it seems to be in the spirit of what this module was after unless I'm misreading) Any attempts at deferring the whole thing to the agent seem to be failing. Still trying some things though.

jadestorm · 2020-02-24T15:48:05Z

Yeah if I try to Defer an unwrap, I get issues where it's not able to use []s to refer to a key. If I try to unwrap a Defer, it yells because it's not a Sensitive type. To get this to work from an agent, I may still need a way to get at the key directly. However I like your approach better @alexjfisher -- I'm happy to update the docs but right now I don't have anything I'd consider particularly useful to tell folk. =/

markri · 2020-04-01T15:17:52Z

Hi, this issue is already taking a while...
So I was trying to use my own fork with #21 and #18 merged.

Finally I was getting some "green" results in my puppet. Then I tried running this using the latest kv2 backend as well, but as it turns out this is not supported.

Forming the request is not a problem, instead of doing
$d = Deferred('vault_lookup::lookup', ["secret/test"])
we need to do
$d = Deferred('vault_lookup::lookup', ["secret/data/test"])

But the needed secret is buried away in a more nested structure, which is a problem for this PR.
Example response:

{
  "request_id": "4e87981e-a2b5-b62a-5a34-23fccf6e7220",
  "lease_id": "",
  "renewable": false,
  "lease_duration": 0,
  "data": {
    "data": {
      "password": "my-secret"
    },
    "metadata": {
      "created_time": "2020-04-01T14:55:00.747555293Z",
      "deletion_time": "",
      "destroyed": false,
      "version": 1
    }
  },
  "wrap_info": null,
  "warnings": null,
  "auth": null
}

So maybe this needs to be mentioned in the docs that you're only supporting kv1 as a backend. (or extend it with an extra lookup method with support for kv2). Maybe just have 3 methods

vault_lookup::lookup
vault_lookup::lookup_kv1
vault_lookup::lookup_kv2

with the appropriate inheritence to prevent code duplication, but I'm no Ruby expert..

zigsphere · 2020-09-26T00:58:24Z

Also, to kind of echo what Markri was saying, assuming you are using v2, you also need to add an additional ['data'] to line 34 in lookup_key.rb like what I did in my fork: https://github.com/zigsphere/puppet-vault_lookup/blob/master/lib/puppet/functions/vault_lookup/lookup_key.rb#L34

If you do not do this, then v2 will return a nil.

By doing this and what Markri suggested, a lookup is successful:
$d = Deferred('vault_lookup::lookup_key', ['secret/data/sre/linux, 'db_passwd', 'https://vault.domain.com:8200'])

2fa · 2020-11-17T17:08:29Z

Hello.

Is there any progress on this one? I've read all the comments and seems like @alexjfisher solution is preferred to be the one.

I did some improvements to his fork so now vault_lookup supports both kv-v1 and kv-v2 data format (you still need to write correct path to secret though). Here's proposed change.

Also, and this is really important: because i unified returned data format for both engines, folks who's using current version of vault_lookup with kv-v2 should remove all ['data'] sections to make it work again. This needs to be done only for lookup for kv-v2 without the key. I'm afraid that this is the only right decision for moving forward.

jadestorm · 2020-11-17T17:47:09Z

I will add that I would just close this PR in lieu of @alexjfisher 's version. However there's a decent amount of discussion here so I figure the maintainers will close it when they are ready.

jadestorm · 2022-03-03T21:19:34Z

@2fa Have you poked around with this anymore? It seems to have gone stagnant. =)

2fa · 2022-03-09T12:33:11Z

Sorry, i was on a vacation =)

Right now we're using @alexjfisher fork with my additions for about 2 years now. I didn't update it so i probably should check what's new and merge it with the current master. I'll try to look at this today and will update this thread when i'm done.

jadestorm · 2022-03-14T15:22:12Z

Sorry, i was on a vacation =)

Right now we're using @alexjfisher fork with my additions for about 2 years now. I didn't update it so i probably should check what's new and merge it with the current master. I'll try to look at this today and will update this thread when i'm done.

That would be awesome, thank you! I'm hoping to get back to using upstream code instead of my craptastic fork. ;)

ncstate-daniel added 2 commits May 23, 2019 16:25

Add a quick and dirty lookup_key function to see if it works.

2a63dff

Moving this into a separate function for the moment for testing.

fc9a56e

dhollinger approved these changes Sep 4, 2019

View reviewed changes

jadestorm commented Sep 4, 2019

View reviewed changes

dhollinger requested changes Sep 4, 2019

View reviewed changes

dhollinger added needs-docs needs-tests labels Sep 4, 2019

jadestorm changed the title ~~DO NOT MERGE (yet): Add lookup_key function to get at a specific key value~~ Add lookup_key function to get at a specific key value Sep 4, 2019

ncstate-daniel added 2 commits September 5, 2019 13:55

Added documentation and test for lookup_key (and a bit of extra info).

0e8883c

Missed adding the keys for a couple of calls.

bdd6f92

alexjfisher requested a review from dhollinger October 16, 2019 13:50

nick-george mentioned this pull request Jan 20, 2020

Getting the following puppet deprecation when reusing your code #22

Closed

bastelfreak approved these changes Jan 20, 2020

View reviewed changes

alexjfisher requested changes Feb 15, 2020

View reviewed changes

alexjfisher mentioned this pull request Feb 15, 2020

WIP: RFC: Support looking up with a key #25

Closed

firstnevyn mentioned this pull request Aug 4, 2022

Kv2 support with a specified secret key #54

Merged

bastelfreak closed this in #54 Aug 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lookup_key function to get at a specific key value #21

Add lookup_key function to get at a specific key value #21

jadestorm commented May 23, 2019

ndelic0 commented Sep 3, 2019

jadestorm commented Sep 3, 2019

dhollinger left a comment

dhollinger Sep 4, 2019

dhollinger Sep 4, 2019

jadestorm Sep 4, 2019

jadestorm left a comment

ndelic0 commented Sep 4, 2019

dhollinger commented Sep 4, 2019

dhollinger left a comment

dhollinger commented Sep 4, 2019

jadestorm commented Sep 4, 2019

jadestorm commented Sep 4, 2019

jadestorm commented Sep 5, 2019

mjtice commented Oct 16, 2019

alexjfisher commented Feb 15, 2020

alexjfisher left a comment

jadestorm commented Feb 21, 2020

jadestorm commented Feb 21, 2020

alexjfisher commented Feb 21, 2020

jadestorm commented Feb 24, 2020

jadestorm commented Feb 24, 2020

markri commented Apr 1, 2020 •

edited

Loading

zigsphere commented Sep 26, 2020 •

edited

Loading

2fa commented Nov 17, 2020

jadestorm commented Nov 17, 2020

jadestorm commented Mar 3, 2022

2fa commented Mar 9, 2022

jadestorm commented Mar 14, 2022

Add lookup_key function to get at a specific key value #21

Add lookup_key function to get at a specific key value #21

Conversation

jadestorm commented May 23, 2019

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

ndelic0 commented Sep 3, 2019

jadestorm commented Sep 3, 2019

dhollinger left a comment

Choose a reason for hiding this comment

dhollinger Sep 4, 2019

Choose a reason for hiding this comment

dhollinger Sep 4, 2019

Choose a reason for hiding this comment

jadestorm Sep 4, 2019

Choose a reason for hiding this comment

jadestorm left a comment

Choose a reason for hiding this comment

ndelic0 commented Sep 4, 2019

dhollinger commented Sep 4, 2019

dhollinger left a comment

Choose a reason for hiding this comment

dhollinger commented Sep 4, 2019

jadestorm commented Sep 4, 2019

jadestorm commented Sep 4, 2019

jadestorm commented Sep 5, 2019

mjtice commented Oct 16, 2019

alexjfisher commented Feb 15, 2020

alexjfisher left a comment

Choose a reason for hiding this comment

jadestorm commented Feb 21, 2020

jadestorm commented Feb 21, 2020

alexjfisher commented Feb 21, 2020

jadestorm commented Feb 24, 2020

jadestorm commented Feb 24, 2020

markri commented Apr 1, 2020 • edited Loading

zigsphere commented Sep 26, 2020 • edited Loading

2fa commented Nov 17, 2020

jadestorm commented Nov 17, 2020

jadestorm commented Mar 3, 2022

2fa commented Mar 9, 2022

jadestorm commented Mar 14, 2022

markri commented Apr 1, 2020 •

edited

Loading

zigsphere commented Sep 26, 2020 •

edited

Loading