Fix Vault managed intermediate PKI bug #15525
Conversation
github-actions
bot
added
the
theme/connect
| // If this is the first time calling setupIntermediatePKIPath, the backend | ||
| // will not have been initialized. Since the mount is ready we can suppress | ||
| // this error. |
There was a problem hiding this comment.
This else if block is the bugfix
agent/connect/ca/provider_vault.go
Outdated
| if v.isConsulMountedIntermediate { | ||
| // This codepath requires the Vault policy: | ||
| // | ||
| // path "/sys/mounts/<intermediate_pki_path>/tune" { | ||
| // capabilities = [ "update" ] | ||
| // } | ||
| // | ||
| err := v.tuneMountNamespaced(v.config.IntermediatePKINamespace, v.config.IntermediatePKIPath, &mountConfig) | ||
| if err != nil { | ||
| v.logger.Warn("Intermediate PKI path was mounted by Consul but could not be tuned", | ||
| "namespace", v.config.IntermediatePKINamespace, | ||
| "path", v.config.IntermediatePKIPath, | ||
| "error", err, | ||
| ) | ||
| } | ||
|
|
||
| } else { | ||
| v.logger.Info("Found existing Intermediate PKI path mount", | ||
| "namespace", v.config.IntermediatePKINamespace, | ||
| "path", v.config.IntermediatePKIPath, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Since tuning the mount is only relevant for Consul-managed, I added a conditional to skip since otherwise you'd see an ominous WARN in the logs on startup.
This isn't the cleanest way to determine if the provider is "Consul-managed" or "Vault-managed" but introducing explicit configuration requires some more consideration around default/missing values.
There was a problem hiding this comment.
If MaxLeaseTTL is set by the operator, tuning this mount really only matters if the mount's MaxLeaseTTL is lower than the IntermediateCertTTL. Otherwise, tuning the config has no effect, as the intermediate CA cert will still be issued with the requested TTL.
I think I heard, though haven't tested, that if you ask Vault to generate a cert (i.e., intermediate CA cert) with a TTL greater than MaxLeaseTTL, the operation will still succeed, the TTL will just be truncated to MaxLeaseTTL. So everything will still work, it's just that the intermediate CA cert will expire sooner than configured. (Unless there's something about our "attempt to renew at 50% of lifetime" math that breaks because it depends on IntermediateCertTTL rather than the TTL of the actual issuer cert.)
I don't know what the default behavior of MaxLeaseTTL is if not configured by the Vault operator.
There was a problem hiding this comment.
I think its somewhat short: https://developer.hashicorp.com/vault/docs/concepts/tokens#the-general-case -- 32 days.
You'll definitely want to read the cert's actual validity period rather than assuming the TTL is OK, not sure if that's useful in this PR or a follow-up though.
Also, I think in 1.11+, we've started adding a warning in this event, but obviously that doesn't help Vault <= 1.10...
There was a problem hiding this comment.
I confirmed that we do work based on the cert's actual validity period:
consul/agent/consul/leader_connect_ca.go
Line 1187 in 6b477ce
| } | ||
| } | ||
|
|
||
| // Create the role for issuing leaf certs if it doesn't exist yet | ||
| // Create the role for issuing leaf certs |
There was a problem hiding this comment.
Updating comment to match code, since it no longer checks for existence
kisunji
force-pushed
the
kisunji/vault-managed-inter-pki
branch
from
696693f to
3cb86e6
Compare
kisunji
force-pushed
the
kisunji/vault-managed-inter-pki
branch
from
3cb86e6 to
dfca9ca
Compare
| // Required to determine if we should tune the mount | ||
| // if the VaultProvider is ever reconfigured. | ||
| v.isConsulMountedIntermediate = true | ||
|
|
There was a problem hiding this comment.
On an earlier meeting did we decide that this needed a fallthrough like the root-generate switch had?
There was a problem hiding this comment.
When we talked I thought I should be initializing the mount from within setupIntermediatePKIPath but realized it wasn't necessary to do that here. So all we need to do is prevent error-ing out on ErrBackendNotInitialized; other codepaths like generateIntermediateCSR below will initialize the mount as necessary.
There was a problem hiding this comment.
So if we do end up mounting the intermediate PKI, there's no other action to be done (unlike GenerateRoot which is actually responsible for mounting+initializing+generating).
I thought about making the code paths for root and intermediate more parallel to each other but deferred it to future Chris
| } | ||
| // Required to determine if we should tune the mount | ||
| // if the VaultProvider is ever reconfigured. | ||
| v.isConsulMountedIntermediate = true |
There was a problem hiding this comment.
Note: this would only get set to true the FIRST time a consul instance ran through this code. After the mount existed it wouldn't set this if it matters.
There was a problem hiding this comment.
Yeah I intended it to be a "best guess" field that only sets if the VaultProvider EVER creates the intermediate mount. If the mount ever existed then we'll never attempt to tune the mount but output a log to help operators diagnose it if they're ever confused by the behavior.
kisunji
force-pushed
the
kisunji/vault-managed-inter-pki
branch
from
948bff7 to
227762e
Compare
Description
If you are 1) using Vault as a Consul Connect CA, 2) using Vault-managed policies (i.e. you are bringing external existing Vault mounts to use as CA instead of giving Consul privileges to make mounts itself), and 3) have an empty intermediate PKI mount, Consul will fail to startup the CA Manager.
People have gotten around this issue by adding a dummy cert in the intermediate PKI mount but this PR aims to fix the root cause.
Testing & Reproduction steps
Links
Vault as CA reference: https://developer.hashicorp.com/consul/docs/connect/ca/vault#vault-acl-policies
PR Checklist