-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ca: add support for an external trusted CA #11910
Changes from all commits
c1c1580
8699481
71f3ae0
42ec34d
5e8ea2a
1853a32
12f12d5
6b679aa
6021105
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:feature | ||
ca: support using an external root CA with the vault CA provider | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -279,7 +279,7 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { | |
if err != nil { | ||
return RootResult{}, err | ||
} | ||
_, err = v.client.Logical().Write(v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{ | ||
resp, err := v.client.Logical().Write(v.config.RootPKIPath+"root/generate/internal", map[string]interface{}{ | ||
"common_name": connect.CACN("vault", uid, v.clusterID, v.isPrimary), | ||
"uri_sans": v.spiffeID.URI().String(), | ||
"key_type": v.config.PrivateKeyType, | ||
|
@@ -288,12 +288,10 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { | |
if err != nil { | ||
return RootResult{}, err | ||
} | ||
|
||
// retrieve the newly generated cert so that we can return it | ||
// TODO: is this already available from the Local().Write() above? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it was. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya, the test case for external CA uses this response |
||
rootPEM, err = v.getCA(v.config.RootPKIPath) | ||
if err != nil { | ||
return RootResult{}, err | ||
var ok bool | ||
rootPEM, ok = resp.Data["certificate"].(string) | ||
if !ok { | ||
return RootResult{}, fmt.Errorf("unexpected response from Vault: %v", resp.Data["certificate"]) | ||
} | ||
|
||
default: | ||
|
@@ -302,7 +300,18 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) { | |
} | ||
} | ||
|
||
return RootResult{PEM: rootPEM}, nil | ||
rootChain, err := v.getCAChain(v.config.RootPKIPath) | ||
if err != nil { | ||
return RootResult{}, err | ||
} | ||
|
||
// Workaround for a bug in the Vault PKI API. | ||
// See https://github.com/hashicorp/vault/issues/13489 | ||
if rootChain == "" { | ||
rootChain = rootPEM | ||
} | ||
|
||
return RootResult{PEM: rootChain}, nil | ||
} | ||
|
||
// GenerateIntermediateCSR creates a private key and generates a CSR | ||
|
@@ -402,8 +411,7 @@ func (v *VaultProvider) SetIntermediate(intermediatePEM, rootPEM string) error { | |
return fmt.Errorf("cannot set an intermediate using another root in the primary datacenter") | ||
} | ||
|
||
// the private key is in vault, so we can't use it in this validation | ||
err := validateSetIntermediate(intermediatePEM, rootPEM, "", v.spiffeID) | ||
err := validateSetIntermediate(intermediatePEM, rootPEM, v.spiffeID) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -468,6 +476,29 @@ func (v *VaultProvider) getCA(path string) (string, error) { | |
return root, nil | ||
} | ||
|
||
// TODO: refactor to remove duplication with getCA | ||
func (v *VaultProvider) getCAChain(path string) (string, error) { | ||
req := v.client.NewRequest("GET", "/v1/"+path+"/ca_chain") | ||
resp, err := v.client.RawRequest(req) | ||
if resp != nil { | ||
defer resp.Body.Close() | ||
} | ||
if resp != nil && resp.StatusCode == http.StatusNotFound { | ||
return "", ErrBackendNotMounted | ||
} | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
raw, err := ioutil.ReadAll(resp.Body) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
root := EnsureTrailingNewline(string(raw)) | ||
return root, nil | ||
} | ||
|
||
// GenerateIntermediate mounts the configured intermediate PKI backend if | ||
// necessary, then generates and signs a new CA CSR using the root PKI backend | ||
// and updates the intermediate backend to use that new certificate. | ||
|
@@ -529,12 +560,7 @@ func (v *VaultProvider) Sign(csr *x509.CertificateRequest) (string, error) { | |
if !ok { | ||
return "", fmt.Errorf("certificate was not a string") | ||
} | ||
ca, ok := response.Data["issuing_ca"].(string) | ||
if !ok { | ||
return "", fmt.Errorf("issuing_ca was not a string") | ||
} | ||
|
||
return EnsureTrailingNewline(cert) + EnsureTrailingNewline(ca), nil | ||
return EnsureTrailingNewline(cert), nil | ||
Comment on lines
-532
to
+563
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is more context in the commit message, but I believe this change is correct because it aligns the vault provider with the other providers, and because we append the full chain to this cert in the Sign RPC endpoint. So previous it would have been adding the same CA cert twice. That was fine before when it was just a single root cert, but it produces an unexpected chain when we have multiple certs in the chain. The tests added in subsequent commits should exercise this code extensively in both primary and secondary DC. |
||
} | ||
|
||
// SignIntermediate returns a signed CA certificate with a path length constraint | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,21 @@ func ParseLeafCerts(pemValue string) (*x509.Certificate, *x509.CertPool, error) | |
return leaf, intermediates, nil | ||
} | ||
|
||
// CertSubjects can be used in debugging to return the subject of each | ||
// certificate in the PEM bundle. Each subject is separated by a newline. | ||
func CertSubjects(pem string) string { | ||
certs, err := parseCerts(pem) | ||
if err != nil { | ||
return err.Error() | ||
} | ||
var buf strings.Builder | ||
for _, cert := range certs { | ||
buf.WriteString(cert.Subject.String()) | ||
buf.WriteString("\n") | ||
} | ||
return buf.String() | ||
} | ||
|
||
// ParseCerts parses the all x509 certificates from a PEM-encoded value. | ||
// The first returned cert is a leaf cert and any other ones are intermediates. | ||
// | ||
|
@@ -90,21 +105,10 @@ func parseCerts(pemValue string) ([]*x509.Certificate, error) { | |
return out, nil | ||
} | ||
|
||
// CalculateCertFingerprint parses the x509 certificate from a PEM-encoded value | ||
// and calculates the SHA-1 fingerprint. | ||
func CalculateCertFingerprint(pemValue string) (string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Possibly dumb question, but did you triple check that the right data is flowing into the newly refactored method for back compat? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! I think I did, but it's been a while so I tried it again to be sure. I copied the new I believe this confirms that the new method works the same way. |
||
// The _ result below is not an error but the remaining PEM bytes. | ||
block, _ := pem.Decode([]byte(pemValue)) | ||
if block == nil { | ||
return "", fmt.Errorf("no PEM-encoded data found") | ||
} | ||
|
||
if block.Type != "CERTIFICATE" { | ||
return "", fmt.Errorf("first PEM-block should be CERTIFICATE type") | ||
} | ||
|
||
hash := sha1.Sum(block.Bytes) | ||
return HexString(hash[:]), nil | ||
// CalculateCertFingerprint calculates the SHA-1 fingerprint from the cert bytes. | ||
func CalculateCertFingerprint(cert []byte) string { | ||
hash := sha1.Sum(cert) | ||
return HexString(hash[:]) | ||
} | ||
|
||
// ParseSigner parses a crypto.Signer from a PEM-encoded key. The private key | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this can return multiple certificates in a chain now, could that cause any problems for downstream things that call the
/ca/roots
endpoint?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! While working on this I had a version that stored only the single root cert in
RootCert
and moved the rest toIntermediateCerts
, but I realized that was more work and ended up being unnecessary in the end. That change has been rebased out now , so I don't think it's visible in the history anymore.I think this is entirely a change in godoc. If someone had configured the built-in Consul provider with a
RootCert
PEM with multiple certs, this would have already been multiple certs.The
PEM
field here will only include multiple certs if the user set it up that way. If the user allows Consul to generate a root cert this will continue to be a single cert. So I think this is backwards compatible, the contents of the field only changes when the user changes the config, not when Consul is upgarded.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Along a similar line of reasoning, if someone doesn't switch to an external trusted CA immediately, would upgrading to a patched version of Consul ever start returning multiple certs in a chain? If so that may break during the rolling upgrade of consul from a non-patched to a patched version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it won't change in that case. The only way this starts returning multiple certs is when the user updates the CA config to do so. Upgrading Consul continues to return a single cert in this PEM.
Existing versions of Consul also return multiple certs here if the
RootCert
is configured that way. I think that might already work with the built-in provider. That's why I think this is mostly just a docs change. It could have already been returning multiple certs, it just wasn't obvious before.