From 32ef9c5d5cc4cbc18d339c660d574ea736bed7ff Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 23 Nov 2021 12:49:43 -0500 Subject: [PATCH 1/3] ca: add some godoc and func for finding leaf signing cert This will be used in a follow up commit. --- agent/consul/leader_connect_ca.go | 19 +++++++++++++++---- agent/structs/connect_ca.go | 13 +++++++++++-- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 180f488cb8..10013cf8f6 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -1149,10 +1149,8 @@ func (c *CAManager) RenewIntermediate(ctx context.Context, isPrimary bool) error // If this is the primary, check if this is a provider that uses an intermediate cert. If // it isn't, we don't need to check for a renewal. - if isPrimary { - if _, ok := provider.(ca.PrimaryUsesIntermediate); !ok { - return nil - } + if isPrimary && !primaryUsesIntermediate(provider) { + return nil } activeIntermediate, err := provider.ActiveIntermediate() @@ -1536,3 +1534,16 @@ func (c *CAManager) checkExpired(pem string) error { } return nil } + +func primaryUsesIntermediate(provider ca.Provider) bool { + _, ok := provider.(ca.PrimaryUsesIntermediate) + return ok +} + +func (c *CAManager) isIntermediateUsedToSignLeaf() bool { + if c.serverConf.Datacenter != c.serverConf.PrimaryDatacenter { + return true + } + provider, _ := c.getCAProvider() + return primaryUsesIntermediate(provider) +} \ No newline at end of file diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 91898e666b..9da766d757 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -86,11 +86,20 @@ type CARoot struct { NotBefore time.Time NotAfter time.Time - // RootCert is the PEM-encoded public certificate. + // RootCert is the PEM-encoded public certificate for the root CA. The + // certificate is the same for all federated clusters. RootCert string // IntermediateCerts is a list of PEM-encoded intermediate certs to - // attach to any leaf certs signed by this CA. + // attach to any leaf certs signed by this CA. The list may include a + // certificate cross-signed by an old root CA, any subordinate CAs below the + // root CA, and the intermediate CA used to sign leaf certificates in the + // local Datacenter. + // + // If the provider which created this root uses an intermediate to sign + // leaf certificates (Vault provider), or this is a secondary Datacenter then + // the intermediate used to sign leaf certificates will be the last in the + // list. IntermediateCerts []string // SigningCert is the PEM-encoded signing certificate and SigningKey From b29faa3e505b4ebbdf8a5314d388a73640cc809b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 25 Nov 2021 19:12:49 -0500 Subject: [PATCH 2/3] 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 0000000000..0f97366fb5 --- /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 10013cf8f6..ef931b360e 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 f4a65bf9e3..3c19edbe96 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 9da766d757..46c7e21327 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 From 28a8a6401930f4f629af2278584d861764258d7f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 30 Nov 2021 19:26:36 -0500 Subject: [PATCH 3/3] ca: make getLeafSigningCertFromRoot safer As a method on the struct type this would not be safe to call without first checking c.isIntermediateUsedToSignLeaf. So for now, move this logic to the CAMananger, so that it is always correct. --- agent/consul/leader_connect_ca.go | 20 ++++++++++++++++++-- agent/consul/leader_connect_test.go | 8 ++++---- agent/structs/connect_ca.go | 17 ----------------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index ef931b360e..198830ab52 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -536,7 +536,7 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf // Add the local leaf signing cert to the rootCA struct. This handles both // upgrades of existing state, and new rootCA. - if rootCA.LeafSigningCert() != interPEM { + if c.getLeafSigningCertFromRoot(rootCA) != interPEM { rootCA.IntermediateCerts = append(rootCA.IntermediateCerts, interPEM) rootUpdateRequired = true } @@ -593,6 +593,22 @@ func (c *CAManager) primaryInitialize(provider ca.Provider, conf *structs.CAConf return nil } +// getLeafSigningCertFromRoot 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. +// +// TODO: fix the data model so that we don't need this complicated lookup to +// find the leaf signing cert. See github.com/hashicorp/consul/issues/11347. +func (c *CAManager) getLeafSigningCertFromRoot(root *structs.CARoot) string { + if !c.isIntermediateUsedToSignLeaf() { + return root.RootCert + } + if len(root.IntermediateCerts) == 0 { + return "" + } + return root.IntermediateCerts[len(root.IntermediateCerts)-1] +} + // secondaryInitializeIntermediateCA runs the routine for generating an intermediate CA CSR and getting // it signed by the primary DC if the root CA of the primary DC has changed since the last // intermediate. It should only be called while the state lock is held by setting the state @@ -1555,4 +1571,4 @@ func (c *CAManager) isIntermediateUsedToSignLeaf() bool { } provider, _ := c.getCAProvider() return primaryUsesIntermediate(provider) -} \ No newline at end of file +} diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 3c19edbe96..a6716deaf6 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -403,7 +403,7 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) { store := s1.caManager.delegate.State() _, activeRoot, err := store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Wait for dc1's intermediate to be refreshed. @@ -422,7 +422,7 @@ func TestLeader_Vault_PrimaryCA_IntermediateRenew(t *testing.T) { _, activeRoot, err = store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Get the root from dc1 and validate a chain of: @@ -548,7 +548,7 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { store := s2.fsm.State() _, activeRoot, err := store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Wait for dc2's intermediate to be refreshed. @@ -572,7 +572,7 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { _, activeRoot, err = store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, activeRoot.LeafSigningCert()) + require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Get the root from dc1 and validate a chain of: diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 46c7e21327..9da766d757 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -142,23 +142,6 @@ 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