pkiCert now gets the Cert, Private key and CA #1591
Conversation
eikenb
added
the
hashicat-update-required
eikenb
force-pushed
the
pki-private-key
branch
3 times, most recently
from
3eae49a to
aa19ffe
Compare
There was a problem hiding this comment.
I tried to run this locally with Vault, but got an error indicating that somehow cert was nil:
2022-06-23T12:49:23.717-0700 [INFO] (runner) creating watcher
2022-06-23T12:49:23.717-0700 [INFO] (runner) starting
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) running initial templates
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) initiating run
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) checking template 035fc374ef90b59e0ecad00e3bf14553
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) missing data for 1 dependencies
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) missing dependency: vault.pki(pki/issue/example-dot-com)
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) add used dependency vault.pki(pki/issue/example-dot-com) to missing since isLeader but do not have a watcher
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) was not watching 1 dependencies
2022-06-23T12:49:23.717-0700 [DEBUG] (watcher) adding vault.pki(pki/issue/example-dot-com)
2022-06-23T12:49:23.717-0700 [TRACE] (watcher) vault.pki(pki/issue/example-dot-com) starting
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) diffing and updating dependencies
2022-06-23T12:49:23.717-0700 [DEBUG] (runner) watching 1 dependencies
2022-06-23T12:49:23.717-0700 [TRACE] (view) vault.pki(pki/issue/example-dot-com) starting fetch
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x2b8 pc=0x10759c76c]
goroutine 30 [running]:
github.com/hashicorp/consul-template/dependency.goodFor(0x14000f51300?)
/Users/swenson/projects/consul-template/dependency/vault_pki.go:114 +0x1c
github.com/hashicorp/consul-template/dependency.(*VaultPKIQuery).Fetch.func1(0x0)
/Users/swenson/projects/consul-template/dependency/vault_pki.go:89 +0xc0
github.com/hashicorp/consul-template/dependency.(*VaultPKIQuery).Fetch(0x14000191280, 0x14000be1b90, 0x1400033c788?)
/Users/swenson/projects/consul-template/dependency/vault_pki.go:96 +0xd0
github.com/hashicorp/consul-template/watch.(*View).fetch(0x14000208d00, 0x105048704?, 0x14000f46000?, 0x0?)
/Users/swenson/projects/consul-template/watch/view.go:203 +0xf4
created by github.com/hashicorp/consul-template/watch.(*View).poll
/Users/swenson/projects/consul-template/watch/view.go:117 +0x100
|
That traceback basically says that the response to vault call to get the PKI cert didn't contain the cert data. I'm not sure why this would happen but I think I could add a check so it would return an error in this case and trigger a retry. If you could get a dump of the data fetched when consul-template calls vault here to see what's in the response to the PKI call, it could help shed some light on what's going on. I can't seem to reproduce this with just consul-template (yet). |
Reworked `PKICert` to return an object with the Cert, the CA and the Private Key. They are stored as stings on a returned struct with appropriate names (Cert, Key, CA). Made it so that the `Data` intermediary variable works like secret so the differing behavior between `secret` and `pkiCert` wouldn't be a snag. Includes updated tests and documentation.
|
Just pushed up a tweak to the documentation to call out that you must have the Certificate itself in the rendered template file for it to work as it contains the TTL data. |
|
If you wanted to try my idea of returning an error to trigger the retry, it would go in vault_pki.go, around line 88. Just a check if |
|
I added the retry, but I still get the same error. Here is the response I see from Vault: |
|
Looks like the |
|
Great find. Thanks. Was working through your test PEMs but hadn't got to that point. Is it messing with pem.Decode()? |
|
Yep! Easy fix though. Something like encoded = []byte(strings.TrimSpace(string(encoded)))
if len(encoded) > 0 && encoded[0] == '{' {
encoded = []byte(strings.TrimSpace(string(encoded[1:])))
}at the top of |
|
The pem.Decode() is supposed to handle things like that. Skipping over data until if finds the prefix. To me it looks more like it is that space at the start of the line more than the bracket. It goes line by line and does a HasPrefix for the "-----BEGIN". If I add spaces before the test PEMs (cert, key or ca) I can get it to fail parsing. I'm not sure why it goes line-by-line on a byte stream. I don't remember anything in the spec about it having to be at the start of the line. But it looks like we need to deal with that. I think I'll see if I can add code to scan for the "-----BEGIN" and make that the start of what I feed to pem.Decode(). Going to try this locally and if it seems to help I'll push it so you can test it. |
|
It also requires nothing on the ending line. It uses HasSuffix to look for it. So there can be nothing at the end either. So it looks like the pem library only deals with simply formatted, cert only files. If we stuck to that we could deal with the spaces with a simple bytes.TrimSpace() and that would fix things. But I'm guessing some people will embed these PEMs in other sorts of config files and we'll need to handle stripping any prefixes or suffixes to get that behavior. |
|
I've pushed up the changes to get it working with the PEM embedded in other text. I kept it a separate commit for now so you can see the changes better but I plan to squash it before merging. Let me know how it goes with this. Seems like this should do it. Thanks 💯 for the review and testing BTW! So many weird edge cases. |
|
I filed a bug against the pem.Decode() docs. |
To get fixes in hashicorp/consul-template#1591
Update consul-template to latest for pkiCert fix So that we get the fixes in hashicorp/consul-template#1590 and hashicorp/consul-template#1591. I tested manually that this no longer causes `pkiCert` to get into an infinite failure loop when the cert expires, and that the key and CA certificate are also accessible. Co-authored-by: Theron Voran <[email protected]>
cherry-picked c165363 Update consul-template to latest for pkiCert fix So that we get the fixes in hashicorp/consul-template#1590 and hashicorp/consul-template#1591. I tested manually that this no longer causes `pkiCert` to get into an infinite failure loop when the cert expires, and that the key and CA certificate are also accessible. Co-authored-by: Theron Voran <[email protected]>
cherry-picked c165363 Update consul-template to latest for pkiCert fix So that we get the fixes in hashicorp/consul-template#1590 and hashicorp/consul-template#1591. I tested manually that this no longer causes `pkiCert` to get into an infinite failure loop when the cert expires, and that the key and CA certificate are also accessible. Co-authored-by: Theron Voran <[email protected]> Co-authored-by: Christopher Swenson <[email protected]> Co-authored-by: Theron Voran <[email protected]>
|
Following this Special Note: This function uses the template file destination as a cache for the certificate to prevent Consul-Template from re-fetching it on reload or restart. This special behavior is to better work with Vault’s PKI behavior of always returning a new certificate even if the current one is still good. Using the destination file as a local “cache” allows Consul-Template to check for the certificate in that local file and, if found, parse it and checks it’s valid date range only fetching a new certificate if the local one has expired. It can only get re-populate the fields (Cert, CA, Key) if that data is in the file. Eg. If you don’t include the CA in the file the CA field will be blank when loading from cache. And note that you must include the Certificate itself in this file as it contains the TTL/expiration data, {{ with pkiCert “pki/issue/my-domain-dot-com” “common_name=foo.example.com” }} and have the template saved as .txt it seem that the file is not honored and the cert keep getting generated, anyone experiencing this? |
|
Hey @Siamert, sorry for your issues. I am unable to reproduce it. Once consul-template is started and the PKI certs file is written I can restart/reload consul-template without it changing the certs. I simplified the test case as much as I could, down to this shell script which works for me (ie. doesn't re-create the certs). I run that script then Ctrl-C and restart, or use #!/bin/sh
export VAULT_TOKEN=a_token # <- Put your vault root token here
export VAULT_ADDR='http://127.0.0.1:8200'
cat > template.tmpl << EOF
{{- with pkiCert "pki/issue/example-dot-com" "common_name=foo.example.com" -}}
Certificate: {{ .Cert }}
Private Key: {{ .Key }}
Authority: {{ .CA }}
{{ end }}
EOF
if ! vault secrets list | grep -q 'pki'
then
vault secrets enable pki
vault secrets tune -max-lease-ttl=8760h pki/
vault write -field=certificate pki/root/generate/internal \
common_name="example.com" \
ttl=8760h
vault write pki/roles/example-dot-com \
allowed_domains=example.com \
allow_subdomains=true \
max_ttl=72h
fi
consul-template \
-log-level=warn \
-vault-renew-token=false \
-template template.tmpl:certsIf you still have problems please file an issue or ask on the discuss forum. Comments on already closed issues/PRs hides the discussion that might help someone else. Thanks and I hope this helps! |
|
Thank you eikenb These are for nginx cert.tpl & nginx.tpl ##cert.tpl -- cert {{ with secret "pki_int/issue/nginx" "ttl=30m" "common_name=foo.example.com" }} ##nginx.tpl -- Key {{ with secret "pki_int/issue/nginx" "ttl=30m" "common_name=foo.example.com" }} ##nginx.hcl template { template { template { rendered_cert.tpl contains this:{{ with pkiCert "pki_int/issue/nginx" "ttl=30m" "common_name=foo.example.com" }} The document state that, it use the destination file rendered_cert.txt as local cache but seems it is not read as |
|
Hey @Siamert, if you want more help with this please consider filing a new issue or asking on the discuss forum. Either place will be better than in the comments of a closed PR. Thanks. |
|
Thanks i did already. |
Reworked
PKICertto return an object with the Cert, the CA and the Private Key. They are stored as stings on a returned struct with appropriate names (Cert, Key, CA).Fixes #1567
Note chains were left out as they are difficult/impossible to differentiate from the CA cert and in the file. Plus they are available after the fact from the API.