From 64532ef6362e8875a1ce8c467dda4acf0d1a74ca Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 Nov 2021 19:12:49 -0500 Subject: [PATCH] ca: fix stored CARoot representation with Vault provider We were not adding the local signing cert to the CARoot. This commit fixes that bug, and also adds support for fixing existing CARoot on upgrade. Also update the tests for both primary and secondary to be more strict. Check the SigningKeyID is correct after initialization and rotation. --- .changelog/11671.txt | 3 +++ agent/consul/leader_connect_ca.go | 27 ++++++++++++------- agent/consul/leader_connect_test.go | 42 +++++++++++++++++++++++------ agent/structs/connect_ca.go | 17 ++++++++++++ 4 files changed, 72 insertions(+), 17 deletions(-) create mode 100644 .changelog/11671.txt diff --git a/.changelog/11671.txt b/.changelog/11671.txt new file mode 100644 index 000000000..0f97366fb --- /dev/null +++ b/.changelog/11671.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: fixes a bug that caused the intermediate cert used to sign leaf certs to be missing from the /connect/ca/roots API response when the Vault provider was used. +``` diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 10013cf8f..ef931b360 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -520,12 +520,26 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf } } + var rootUpdateRequired bool + // Versions prior to 1.9.3, 1.8.8, and 1.7.12 incorrectly used the primary // rootCA's subjectKeyID here instead of the intermediate. For // provider=consul this didn't matter since there are no intermediates in // the primaryDC, but for vault it does matter. expectedSigningKeyID := connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) - needsSigningKeyUpdate := (rootCA.SigningKeyID != expectedSigningKeyID) + if rootCA.SigningKeyID != expectedSigningKeyID { + c.logger.Info("Correcting stored CARoot values", + "previous-signing-key", rootCA.SigningKeyID, "updated-signing-key", expectedSigningKeyID) + rootCA.SigningKeyID = expectedSigningKeyID + rootUpdateRequired = true + } + + // Add the local leaf signing cert to the rootCA struct. This handles both + // upgrades of existing state, and new rootCA. + if rootCA.LeafSigningCert() != interPEM { + rootCA.IntermediateCerts = append(rootCA.IntermediateCerts, interPEM) + rootUpdateRequired = true + } // Check if the CA root is already initialized and exit if it is, // adding on any existing intermediate certs since they aren't directly @@ -537,26 +551,21 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf if err != nil { return err } - if activeRoot != nil && needsSigningKeyUpdate { - c.logger.Info("Correcting stored SigningKeyID value", "previous", rootCA.SigningKeyID, "updated", expectedSigningKeyID) - - } else if activeRoot != nil && !needsSigningKeyUpdate { + if activeRoot != nil && !rootUpdateRequired { // This state shouldn't be possible to get into because we update the root and // CA config in the same FSM operation. if activeRoot.ID != rootCA.ID { return fmt.Errorf("stored CA root %q is not the active root (%s)", rootCA.ID, activeRoot.ID) } + // TODO: why doesn't this c.setCAProvider(provider, activeRoot) ? rootCA.IntermediateCerts = activeRoot.IntermediateCerts c.setCAProvider(provider, rootCA) + c.logger.Info("initialized primary datacenter CA from existing CARoot with provider", "provider", conf.Provider) return nil } - if needsSigningKeyUpdate { - rootCA.SigningKeyID = expectedSigningKeyID - } - // Get the highest index idx, _, err := state.CARoots(nil) if err != nil { diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index f4a65bf9e..3c19edbe9 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -396,23 +396,34 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) { provider, _ := getCAProviderWithLock(s1) intermediatePEM, err := provider.ActiveIntermediate() require.NoError(err) - _, err = connect.ParseCert(intermediatePEM) + intermediateCert, err := connect.ParseCert(intermediatePEM) require.NoError(err) + // Check that the state store has the correct intermediate + store := s1.caManager.delegate.State() + _, activeRoot, err := store.CARootActive(nil) + require.NoError(err) + require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + // Wait for dc1's intermediate to be refreshed. // It is possible that test fails when the blocking query doesn't return. retry.Run(t, func(r *retry.R) { provider, _ = getCAProviderWithLock(s1) newIntermediatePEM, err := provider.ActiveIntermediate() r.Check(err) - _, err = connect.ParseCert(intermediatePEM) - r.Check(err) if newIntermediatePEM == intermediatePEM { r.Fatal("not a renewed intermediate") } + intermediateCert, err = connect.ParseCert(newIntermediatePEM) + r.Check(err) intermediatePEM = newIntermediatePEM }) + + _, activeRoot, err = store.CARootActive(nil) require.NoError(err) + require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Get the root from dc1 and validate a chain of: // dc1 leaf -> dc1 intermediate -> dc1 root @@ -439,6 +450,8 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) { // Check that the leaf signed by the new intermediate can be verified using the // returned cert chain (signed intermediate + remote root). intermediatePool := x509.NewCertPool() + // TODO: do not explicitly add the intermediatePEM, we should have it available + // from leafPEM. Use connect.ParseLeafCerts to do the right thing. intermediatePool.AppendCertsFromPEM([]byte(intermediatePEM)) rootPool := x509.NewCertPool() rootPool.AppendCertsFromPEM([]byte(caRoot.RootCert)) @@ -515,10 +528,10 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { secondaryProvider, _ := getCAProviderWithLock(s2) intermediatePEM, err := secondaryProvider.ActiveIntermediate() require.NoError(err) - cert, err := connect.ParseCert(intermediatePEM) + intermediateCert, err := connect.ParseCert(intermediatePEM) require.NoError(err) - currentCertSerialNumber := cert.SerialNumber - currentCertAuthorityKeyId := cert.AuthorityKeyId + currentCertSerialNumber := intermediateCert.SerialNumber + currentCertAuthorityKeyId := intermediateCert.AuthorityKeyId // Capture the current root var originalRoot *structs.CARoot @@ -532,6 +545,12 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { waitForActiveCARoot(t, s1, originalRoot) waitForActiveCARoot(t, s2, originalRoot) + store := s2.fsm.State() + _, activeRoot, err := store.CARootActive(nil) + require.NoError(err) + require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + // Wait for dc2's intermediate to be refreshed. // It is possible that test fails when the blocking query doesn't return. // When https://github.com/hashicorp/consul/pull/3777 is merged @@ -548,8 +567,13 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { currentCertAuthorityKeyId = cert.AuthorityKeyId r.Fatal("not a renewed intermediate") } + intermediateCert = cert }) + + _, activeRoot, err = store.CARootActive(nil) require.NoError(err) + require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Get the root from dc1 and validate a chain of: // dc2 leaf -> dc2 intermediate -> dc1 root @@ -570,17 +594,19 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { leafPEM, err := secondaryProvider.Sign(leafCsr) require.NoError(err) - cert, err = connect.ParseCert(leafPEM) + intermediateCert, err = connect.ParseCert(leafPEM) require.NoError(err) // Check that the leaf signed by the new intermediate can be verified using the // returned cert chain (signed intermediate + remote root). intermediatePool := x509.NewCertPool() + // TODO: do not explicitly add the intermediatePEM, we should have it available + // from leafPEM. Use connect.ParseLeafCerts to do the right thing. intermediatePool.AppendCertsFromPEM([]byte(intermediatePEM)) rootPool := x509.NewCertPool() rootPool.AppendCertsFromPEM([]byte(caRoot.RootCert)) - _, err = cert.Verify(x509.VerifyOptions{ + _, err = intermediateCert.Verify(x509.VerifyOptions{ Intermediates: intermediatePool, Roots: rootPool, }) diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 9da766d75..46c7e2132 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -142,6 +142,23 @@ func (c *CARoot) Clone() *CARoot { return &newCopy } +// LeafSigningCert returns the PEM encoded certificate that should be used to +// sign leaf certificates in the local datacenter. The SubjectKeyId of the +// returned cert should always match the SigningKeyID of the CARoot. +// +// This method has no knowledge of the provider, so it makes assumptions about +// which cert is used based on established convention. Generally you should +// check CAManager.isIntermediateUsedToSignLeaf first. +// +// If there are no IntermediateCerts the RootCert is returned. If there are +// IntermediateCerts the last one in the list is returned. +func (c *CARoot) LeafSigningCert() string { + if len(c.IntermediateCerts) == 0 { + return c.RootCert + } + return c.IntermediateCerts[len(c.IntermediateCerts)-1] +} + // CARoots is a list of CARoot structures. type CARoots []*CARoot