petems / petems-hiera_vault

A hiera backend for access to secrets being stored in HashiCorp Vault
Apache License 2.0
44 stars 45 forks source link

If the Vault token becomes unavailable to the Hiera/Vault module, a secret is substituted with an empty string #54

Closed jhur7 closed 4 years ago

jhur7 commented 4 years ago

If the Vault token becomes unavailable, a secret is substituted with an empty string. If you have thousands of systems, all configuration files would be updated with an empty string which could be a huge issue.

Here is my Hiera configuration file. I was able to recreate the issue by moving the token file to a .old extension.

   - name: "Hiera-vault lookup"
    lookup_key: hiera_vault
    options:
      confine_to_keys:
        - '^vault_.*'
        - '^.*_password$'
        - '^password.*'
      address: https://hostname.example.com
      ssl_ca_cert: /etc/pki/tls/certs/example-root.pem
      token: /etc/puppetlabs/tokens/tokens_ro.txt
      default_field: value
      mounts:
        secrets/puppet_nogroup:
          - nodes/%{::trusted.certname}
          - common

In the nodes directory, I have a hostname1.net.yaml Hiera data file. This has the following secret key lookup using the Hiera/Vault integration:

virtual::secret_key: "%{lookup('password_virtual_key')}"

This is the example of a Puppet run where the secret was substituted for an empty Puppet string:


@@ -163,7 +163,7 @@
 # SECRET_KEY for all of them.
 #SECRET_KEY = secret_key.generate_or_read_from_file(
 #    os.path.join(LOCAL_PATH, '.secret_key_store'))
-SECRET_KEY = ‘REDACTED’
+SECRET_KEY = ''```

I am running Puppet 6.7 with petems-hiera_vault (v1.0.0).

Please advise on the best approach to resolve this.
petems commented 4 years ago

Hi, let me look into this for you. If it cant reach Vault, it should just error out and then hiera should return an error.

Q: Is there a reason you're using hiera over the new lookup avaliable with Puppet 6? This: https://forge.puppet.com/puppet/vault_lookup

petems commented 4 years ago

So from my testing, this isn't reproducible with the documented method of using lookup, either an automatic parameter lookup or explicit lookup in Puppet code:

---
version: 5
hierarchy:
  - name: "Hiera-vault lookup"
    lookup_key: hiera_vault
    options:
      confine_to_keys:
        - '^vault_.*'
        - '^.*_password$'
        - '^password.*'
      ssl_verify: false
      address: http://puppet:8200
      token: /etc/does_not_exist.txt
      default_field: value
      mounts:
        puppet:
          - '%{::trusted.certname}/'
# profile to deploy a puppet vault_message

class profile::vault_message {

  $vault_notify = lookup({"name" => "vault_notify"})
  notify { "testing vault ${vault_notify}":}

}

Error when running from Puppet:

Info: Retrieving locales
Info: Loading facts
Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Function lookup() did not find a value for the name 'vault_notify' on node node1.vm
Warning: Not using cache on failed catalog
Error: Could not retrieve catalog; skipping run

It's been a long time, but I do remember you being able to have functions within yaml files but it being heavily not recommended to do, because of issues like this. Is there a reason you're doing the lookup within the yaml file, rather than either doing an explicit lookup or using automatic parameter lookup?

petems commented 4 years ago

Ah yes, so this would've been an interpolation function... never really understood the benefits of that, but if you do have a need to use it, I would defend against the empty string by strongly typing the parameter to prevent the empty string issue, or just a function to check it's not an empty string.

The error isnt from hiera_vault but how interpolation functions from Hiera handle errors, the same thing would happen with any backend if it couldnt find a value

jhur7 commented 4 years ago

Thank you so much for getting back to me @petems . I was out of town hence my delayed response. I don't necessarily have a need to use the interpolation function. I have tried the following, but an empty string still gets substituted. How can I interpolate this value without using the lookup function? It appears the lookup doesn't go to Vault at all.

virtual::secret_key: "%{password_virtual_key)}"

As a test, I interpolated a fact, and is working fine. I've perused the documentation a number of times, and I don't see a clear cut way to interpolate a string without using the lookup function in Hiera.

This works (test only). virtual::secret_key: "%{facts.hostname)}"

petems commented 4 years ago

Interpolation itself isn't the issue, it's how interpolation deals with errors. It appears that if there's an issue with doing a lookup (such as the token being unavailable so the Vault process returns nothing) then it returns an empty string. So a fact lookup would have the same thing if theres an error returning a fact.

In fact, you're doubling up a little, as you dont need to use a yaml data file for vault lookup at all.

You dont need to do a lookup in the yaml at all: Puppet will use Vault directly via the plugin.

So with automatic parameter lookup: when the class is compiled, it will try and do a hiera lookup for the parameter secret_key.

So you could simply have the secret as a value at the mount, in your case that would be on your vault as a key entry like this:

secrets/puppet_nogroup/nodes/app01.example.com/virtual::secret_key

One thing to highlight: Vault will only do a lookup for keys that match your restrictions, with your current config it would not do that lookup, you could add in secret or key regex:

- '^vault_.*'
- '^.*_password$'
- '^password.*'
- '^.*_key$'

If you still wanted to use lookup within the yaml code, you could simply change your code so an empty string would cause an error:

class virtual (
  String[1]  $secret_key,
  # ...
) {
  # ...
}

Does that all make sense?

jhur7 commented 4 years ago

Thank You @petems . We've sorted out our issues. For reference, I decided to not use APM so I can confine my variables to a specific key ie vault_. Here is what I did to work around this in my YAML data file.

lookup_options:
  something::secret:
    convert_to: "String[1]"