Skip to content
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

Merged
merged 9 commits into from
Feb 22, 2022
3 changes: 3 additions & 0 deletions .changelog/11910.txt
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
```
52 changes: 27 additions & 25 deletions agent/connect/ca/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,38 +9,14 @@ import (
"github.com/hashicorp/consul/agent/connect"
)

func validateSetIntermediate(
intermediatePEM, rootPEM string,
currentPrivateKey string, // optional
spiffeID *connect.SpiffeIDSigning,
) error {
func validateSetIntermediate(intermediatePEM, rootPEM string, spiffeID *connect.SpiffeIDSigning) error {
// Get the key from the incoming intermediate cert so we can compare it
// to the currently stored key.
intermediate, err := connect.ParseCert(intermediatePEM)
if err != nil {
return fmt.Errorf("error parsing intermediate PEM: %v", err)
}

if currentPrivateKey != "" {
privKey, err := connect.ParseSigner(currentPrivateKey)
if err != nil {
return err
}

// Compare the two keys to make sure they match.
b1, err := x509.MarshalPKIXPublicKey(intermediate.PublicKey)
if err != nil {
return err
}
b2, err := x509.MarshalPKIXPublicKey(privKey.Public())
if err != nil {
return err
}
if !bytes.Equal(b1, b2) {
return fmt.Errorf("intermediate cert is for a different private key")
}
}

// Validate the remaining fields and make sure the intermediate validates against
// the given root cert.
if !intermediate.IsCA {
Expand All @@ -65,6 +41,32 @@ func validateSetIntermediate(
return nil
}

func validateIntermediateSignedByPrivateKey(intermediatePEM string, privateKey string) error {
intermediate, err := connect.ParseCert(intermediatePEM)
if err != nil {
return fmt.Errorf("error parsing intermediate PEM: %v", err)
}

privKey, err := connect.ParseSigner(privateKey)
if err != nil {
return err
}

// Compare the two keys to make sure they match.
b1, err := x509.MarshalPKIXPublicKey(intermediate.PublicKey)
if err != nil {
return err
}
b2, err := x509.MarshalPKIXPublicKey(privKey.Public())
if err != nil {
return err
}
if !bytes.Equal(b1, b2) {
return fmt.Errorf("intermediate cert is for a different private key")
}
return nil
}

func validateSignIntermediate(csr *x509.CertificateRequest, spiffeID *connect.SpiffeIDSigning) error {
// We explicitly _don't_ require that the CSR has a valid SPIFFE signing URI
// SAN because AWS PCA doesn't let us set one :(. We need to relax it here
Expand Down
26 changes: 19 additions & 7 deletions agent/connect/ca/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ type PrimaryProvider interface {
// the active intermediate. If multiple intermediates are needed to complete
// the chain from the signing certificate back to the active root, they should
// all by bundled here.
// TODO: replace with GenerateLeafSigningCert (https://github.com/hashicorp/consul/issues/12386)
GenerateIntermediate() (string, error)

// SignIntermediate will validate the CSR to ensure the trust domain in the
Expand Down Expand Up @@ -171,22 +172,33 @@ type PrimaryProvider interface {
}

type SecondaryProvider interface {
// GenerateIntermediateCSR generates a CSR for an intermediate CA
// certificate, to be signed by the root of another datacenter. If IsPrimary was
// set to true with Configure(), calling this is an error.
// GenerateIntermediateCSR should return a CSR for an intermediate CA
// certificate. The intermediate CA will be signed by the primary CA and
// should be used by the provider to sign leaf certificates in the local
// datacenter.
//
// After the certificate is signed, SecondaryProvider.SetIntermediate will
// be called to store the intermediate CA.
GenerateIntermediateCSR() (string, error)

// SetIntermediate sets the provider to use the given intermediate certificate
// as well as the root it was signed by. This completes the initialization for
// a provider where IsPrimary was set to false in Configure().
// SetIntermediate is called to store a newly signed leaf signing certificate and
// the chain of certificates back to the root CA certificate.
//
// The provider should save the certificates and use them to
// Provider.Sign leaf certificates.
SetIntermediate(intermediatePEM, rootPEM string) error
}

// RootResult is the result returned by PrimaryProvider.GenerateRoot.
//
// TODO: rename this struct
type RootResult struct {
// PEM encoded certificate that will be used as the primary CA.
// PEM encoded bundle of CA certificates. The first certificate must be the
// primary CA used to sign intermediates for secondary datacenters, and the
// last certificate must be the trusted CA.
Copy link
Contributor

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?

Copy link
Contributor Author

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 to IntermediateCerts, 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.

Copy link
Member

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.

Copy link
Contributor Author

@dnephin dnephin Feb 17, 2022

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.

//
// If there is only a single certificate in the bundle then it will be used
// as both the primary CA and the trusted CA.
PEM string
}

Expand Down
10 changes: 4 additions & 6 deletions agent/connect/ca/provider_consul.go
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,10 @@ func (c *ConsulProvider) SetIntermediate(intermediatePEM, rootPEM string) error
return fmt.Errorf("cannot set an intermediate using another root in the primary datacenter")
}

err = validateSetIntermediate(
intermediatePEM, rootPEM,
providerState.PrivateKey,
c.spiffeID,
)
if err != nil {
if err = validateSetIntermediate(intermediatePEM, rootPEM, c.spiffeID); err != nil {
return err
}
if err := validateIntermediateSignedByPrivateKey(intermediatePEM, providerState.PrivateKey); err != nil {
return err
}

Expand Down
58 changes: 42 additions & 16 deletions agent/connect/ca/provider_vault.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
10 changes: 5 additions & 5 deletions agent/connect/generate.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ const (
DefaultIntermediateCertTTL = 24 * 365 * time.Hour
)

func pemEncodeKey(key []byte, blockType string) (string, error) {
func pemEncode(value []byte, blockType string) (string, error) {
var buf bytes.Buffer

if err := pem.Encode(&buf, &pem.Block{Type: blockType, Bytes: key}); err != nil {
return "", fmt.Errorf("error encoding private key: %s", err)
if err := pem.Encode(&buf, &pem.Block{Type: blockType, Bytes: value}); err != nil {
return "", fmt.Errorf("error encoding value %v: %s", blockType, err)
}
return buf.String(), nil
}
Expand All @@ -38,7 +38,7 @@ func generateRSAKey(keyBits int) (crypto.Signer, string, error) {
}

bs := x509.MarshalPKCS1PrivateKey(pk)
pemBlock, err := pemEncodeKey(bs, "RSA PRIVATE KEY")
pemBlock, err := pemEncode(bs, "RSA PRIVATE KEY")
if err != nil {
return nil, "", err
}
Expand Down Expand Up @@ -73,7 +73,7 @@ func generateECDSAKey(keyBits int) (crypto.Signer, string, error) {
return nil, "", fmt.Errorf("error marshaling ECDSA private key: %s", err)
}

pemBlock, err := pemEncodeKey(bs, "EC PRIVATE KEY")
pemBlock, err := pemEncode(bs, "EC PRIVATE KEY")
if err != nil {
return nil, "", err
}
Expand Down
34 changes: 19 additions & 15 deletions agent/connect/parsing.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 TestNewCARoot (which is much more strict about assertions) from this branch and pasted into main. I deleted the extra test cases I added (the ones with extra certs). The only diff was in the name field (Root -> Primary). Once I changed the expected value of the Name to be Root once again the test passed.

I believe this confirms that the new method works the same way. newCARoot (previous parseCARoot) is the only function that calls this one outside of test code.

// 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
Expand Down
5 changes: 1 addition & 4 deletions agent/connect/testing_ca.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,7 @@ func testCA(t testing.T, xc *structs.CARoot, keyType string, keyBits int, ttl ti
t.Fatalf("error encoding private key: %s", err)
}
result.RootCert = buf.String()
result.ID, err = CalculateCertFingerprint(result.RootCert)
if err != nil {
t.Fatalf("error generating CA ID fingerprint: %s", err)
}
result.ID = CalculateCertFingerprint(bs)
result.SerialNumber = uint64(sn.Int64())
result.NotBefore = template.NotBefore.UTC()
result.NotAfter = template.NotAfter.UTC()
Expand Down