Allow additional time for PKI certificates to valid

[jasonodonnell]

Vault adds 30 seconds to pad NotBefore (configurable) field for certificates issued by the PKI secret engine. This pad compensates for clockskew.

An issue I'm seeing with rendering certificates is Consul Template calculates the sleep time using cert.NotAfter - cert.NotBefore of the certificate and configures the read loop for 85-95% of that value, which has the padding included. In some situations, specifically when TTL is low, the certificate expires before the application serving it has time to react.

This PR changes Consul Template to instead use the expiration value sent along with the certificates. This means no cert inspection is required and we can do our calculations based on expiration - time.Now. This will give us a better duration to wait.

If the client has significant clockskew and requested certificates have a low TTL (maybe ~1m), this could still result in rendering new certs after the cert has already expired. As TTL grows, though, this issue quickly goes away.

This logic only applies to certificates that do not have a lease (generate_lease=false is the default). Leased certs use a different code path and renew instead of updating.

[eikenb]

Is this 30second padding documented? Maybe as part of the API, SDK or configuration?

I ask as my one concern with this is that we are working around an internal detail of Vault that could change in the future without notice.

[jasonodonnell]

Hi @eikenb, it's not documented but I agree working around this internal detail isn't great. One thing nice about this code path, though, is if the duration goes below zero, it will default to the 5 minute loop.

Happy to discuss different changes but we're seeing reports of this causing applications to crash because the short time between being valid and rendering can cause errors. It's possible we could tune the random variance of the final duration calculation specifically for certs to be lower? Maybe 65%-75%.

[eikenb]

In that second suggestion, were you talking about the block in that code where it does?

`sleep = sleep * (.85 + rand.Float64()*0.1)`

[jasonodonnell]

That's what I was thinking @eikenb.

[eikenb]

Bah... I think I'll just accept this as is.. Lowering the duration of those certs by an arbitrary % seems almost like obfuscating the real reason. Better ugly and obvious than clean but hidden.

I was trying to find where in Vault it sets the 30 second padding without luck. Could you point me in the right direction?

[jasonodonnell]

@eikenb I was a little wrong about it being added to validTo field. The extra 30 seconds is actually coming from a padding of the NotBefore field: https://github.com/hashicorp/vault/blob/dcf2308b5cf4f09e6013ba1ce718212f87e0a880/sdk/helper/certutil/helpers.go#L514
https://www.vaultproject.io/api/secret/pki#not_before_duration

Since we're doing validTo - notBefore, we get an extra 30 seconds by default. I don't think we want to recommend tuning this to be lower because it can introduce subtle clock skew bugs.

I think the better solution here is to change the durationFromCert function to do validTo - time.Now. This will give us a more realistic base duration to do additional calculations from.

Thoughts?

[eikenb]

This sounds promising, but I'd like to see it in code to help me fully get it.
Could you update the PR with your idea?

[calvn]

Since the padding adjustment is a fixed value on consul template's end, can this issue be re-introduced if the not_before_duration is a non-default value? For instance, if it's 60s instead, there's now again a difference of 30s between when CT renders and when the cert actually expires.

[jasonodonnell]

@calvn That's correct based on what I found. I suggested using cert.NotAfter - time.Now. It seems to work but I'm still fixing broken tests that have hardcoded certificates from 2018.

[jasonodonnell]

@calvn @eikenb I think I found a better path forward. After inspecting Vault's reply to a PKI issue request, I noticed one of the fields is expiration. We can use this instead of inspecting certificates.

I updated the code, removed the cert inspection code and added a test.

Sample of the returned values from Vault if you're curious:

/ $ vault write pki/issue/hashicorp-com common_name=www.hashicorp.com ttl=43799h
Key                 Value
---                 -----
certificate         -----BEGIN CERTIFICATE-----
...
-----END CERTIFICATE-----
expiration          1753195959
issuing_ca          -----BEGIN CERTIFICATE-----
...
Lf3EhhGsihUqD25mptQt
-----END CERTIFICATE-----
private_key         -----BEGIN RSA PRIVATE KEY-----
...
-----END RSA PRIVATE KEY-----
private_key_type    rsa
serial_number       6b:09:69:d5:8e:8d:e9:05:72:66:16:b7:20:79:d8:50:22:a3:6e:53

[eikenb]

Nice. Less code, no hacks. Tests.
LGTM!