connect: intermediate CA certs generated with the vault provider lack URI SANs #6491
Conversation
agent/connect/ca/provider_vault.go
Outdated
| @@ -201,7 +201,17 @@ func (v *VaultProvider) SetIntermediate(intermediatePEM, rootPEM string) error { | |||
| return fmt.Errorf("cannot set an intermediate using another root in the primary datacenter") | |||
| } | |||
|
|
|||
| _, err := v.client.Logical().Write(v.config.IntermediatePKIPath+"intermediate/set-signed", map[string]interface{}{ | |||
| spiffeID := connect.SpiffeIDSigning{ClusterID: v.clusterId, Domain: "consul"} | |||
There was a problem hiding this comment.
Shouldn't the domain be the one that is configured by the user?
There was a problem hiding this comment.
Nope. There's a different helper function that's used over on the consul provider that does this exact computation for the same purpose (SetIntermediate) that has a helpful @banks comment:
// SpiffeIDSigningForCluster returns the SPIFFE signing identifier (trust
// domain) representation of the given CA config. If config is nil this function
// will panic.
//
// NOTE(banks): we intentionally fix the tld `.consul` for now rather than tie
// this to the `domain` config used for DNS because changing DNS domain can't
// break all certificate validation. That does mean that DNS prefix might not
// match the identity URIs and so the trust domain might not actually resolve
// which we would like but don't actually need.
func SpiffeIDSigningForCluster(config *structs.CAConfiguration) *SpiffeIDSigning {
return &SpiffeIDSigning{ClusterID: config.ClusterID, Domain: "consul"}
}
For clarity I'm going to rearrange stuff so that function can be called instead though.
rboyer
force-pushed
the
fix-vault-ca-provider-uri-sans
branch
from
a5033c1 to
8ab24e7
Compare
|
Extremely superficial LGTM. I'll let Hans approve when he's satisfied! |
There was a problem hiding this comment.
The existing tests should have caught this, but they were using a
vendored copy of vault version 0.10.3. This fixes the tests by running
an actual copy of vault instead of an in-process copy.
Does that mean that the Vault tests won't run in CI at all?
Should they? Or do we have other Integration tests against Vault that would catch issues here?
Approving as the code change looks solid but IMO it would be best to not end up with CI skipping any tests around Vault compat. I think we have others though but would they catch issues like this in future?
This PR also ensures that we download a copy of vault before running the tests so that the vault tests execute. |
… URI SANs This only affects vault versions >=1.1.1 because the prior code accidentally relied upon a bug that was fixed in hashicorp/vault#6505 The existing tests should have caught this, but they were using a vendored copy of vault version 0.10.3. This fixes the tests by running an actual copy of vault instead of an in-process copy. This has the added benefit of changing the dependency on vault to just vault/api. Also update VaultProvider to use similar SetIntermediate validation code as the ConsulProvider implementation.
rboyer
force-pushed
the
fix-vault-ca-provider-uri-sans
branch
from
8ab24e7 to
0c3e22b
Compare
… URI SANs (#6491) This only affects vault versions >=1.1.1 because the prior code accidentally relied upon a bug that was fixed in hashicorp/vault#6505 The existing tests should have caught this, but they were using a vendored copy of vault version 0.10.3. This fixes the tests by running an actual copy of vault instead of an in-process copy. This has the added benefit of changing the dependency on vault to just vault/api. Also update VaultProvider to use similar SetIntermediate validation code as the ConsulProvider implementation.
This only affects vault versions >=1.1.1 because the prior code
accidentally relied upon a bug that was fixed in
hashicorp/vault#6505
The existing tests should have caught this, but they were using a
vendored copy of vault version 0.10.3. This fixes the tests by running
an actual copy of vault instead of an in-process copy. This has the
added benefit of changing the dependency on vault to just vault/api.
Also update VaultProvider to use similar SetIntermediate validation code
as the ConsulProvider implementation.
The key detail is in needing to set
use_csr_values = trueto have CSR URIs used for the cert.