From 28a8a6401930f4f629af2278584d861764258d7f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 30 Nov 2021 19:26:36 -0500 Subject: [PATCH] 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