From fa32c784295bf1b0e4c41da09668c75c141f7eb1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 26 Nov 2021 17:31:23 -0500 Subject: [PATCH] ca: set the correct SigningKeyID after config update with Vault provider The test added in this commit shows the problem. Previously the SigningKeyID was set to the RootCert not the local leaf signing cert. This same bug was fixed in two other places back in 2019, but this last one was missed. While fixing this bug I noticed I had the same few lines of code in 3 places, so I extracted a new function for them. There would be 4 places, but currently the InitializeCA flow sets this SigningKeyID in a different way, so I've left that alone for now. --- .changelog/11672.txt | 3 ++ agent/consul/leader_connect_ca.go | 38 +++++++++-------- agent/consul/leader_connect_ca_test.go | 58 +++++++++++++++++++++++++- 3 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 .changelog/11672.txt diff --git a/.changelog/11672.txt b/.changelog/11672.txt new file mode 100644 index 000000000..d14f74b16 --- /dev/null +++ b/.changelog/11672.txt @@ -0,0 +1,3 @@ +```release-note:bug +ca: fixes a bug that caused the SigningKeyID to be wrong in the primary DC, when the Vault provider is used, after a CA config creates a new root. +``` diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index c27e55c3f..dc712a9e5 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -1005,7 +1005,9 @@ func (c *CAManager) primaryUpdateRootCA(newProvider ca.Provider, args *structs.C return err } if intermediate != newRootPEM { - newActiveRoot.IntermediateCerts = append(newActiveRoot.IntermediateCerts, intermediate) + if err := setLeafSigningCert(newActiveRoot, intermediate); err != nil { + return err + } } // Update the roots and CA config in the state store at the same time @@ -1060,16 +1062,10 @@ func (c *CAManager) primaryRenewIntermediate(provider ca.Provider, newActiveRoot return fmt.Errorf("error generating new intermediate cert: %v", err) } - intermediateCert, err := connect.ParseCert(intermediatePEM) - if err != nil { - return fmt.Errorf("error parsing intermediate cert: %v", err) + if err := setLeafSigningCert(newActiveRoot, intermediatePEM); err != nil { + return err } - // Append the new intermediate to our local active root entry. This is - // where the root representations start to diverge. - newActiveRoot.IntermediateCerts = append(newActiveRoot.IntermediateCerts, intermediatePEM) - newActiveRoot.SigningKeyID = connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) - c.logger.Info("generated new intermediate certificate for primary datacenter") return nil } @@ -1093,20 +1089,28 @@ func (c *CAManager) secondaryRenewIntermediate(provider ca.Provider, newActiveRo return fmt.Errorf("Failed to set the intermediate certificate with the CA provider: %v", err) } - intermediateCert, err := connect.ParseCert(intermediatePEM) - if err != nil { - return fmt.Errorf("error parsing intermediate cert: %v", err) + if err := setLeafSigningCert(newActiveRoot, intermediatePEM); err != nil { + return err } - // Append the new intermediate to our local active root entry. This is - // where the root representations start to diverge. - newActiveRoot.IntermediateCerts = append(newActiveRoot.IntermediateCerts, intermediatePEM) - newActiveRoot.SigningKeyID = connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId) - c.logger.Info("received new intermediate certificate from primary datacenter") return nil } +// setLeafSigningCert updates the CARoot by appending the pem to the list of +// intermediate certificates, and setting the SigningKeyID to the encoded +// SubjectKeyId of the certificate. +func setLeafSigningCert(caRoot *structs.CARoot, pem string) error { + cert, err := connect.ParseCert(pem) + if err != nil { + return fmt.Errorf("error parsing leaf signing cert: %w", err) + } + + caRoot.IntermediateCerts = append(caRoot.IntermediateCerts, pem) + caRoot.SigningKeyID = connect.EncodeSigningKeyID(cert.SubjectKeyId) + return nil +} + // intermediateCertRenewalWatch periodically attempts to renew the intermediate cert. func (c *CAManager) intermediateCertRenewalWatch(ctx context.Context) error { isPrimary := c.serverConf.Datacenter == c.serverConf.PrimaryDatacenter diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 176eb2f55..846144460 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -14,6 +14,8 @@ import ( "testing" "time" + "github.com/hashicorp/consul/testrpc" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -24,7 +26,6 @@ import ( "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" - "github.com/hashicorp/consul/testrpc" ) // TODO(kyhavlov): replace with t.Deadline() @@ -495,3 +496,58 @@ func TestCAManager_Initialize_Logging(t *testing.T) { require.Contains(t, buf.String(), "consul CA provider configured") } + +func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { + ca.SkipIfVaultNotPresent(t) + vault := ca.NewTestVaultServer(t) + + _, s1 := testServerWithConfig(t, func(c *Config) { + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + }, + } + }) + defer func() { + s1.Shutdown() + s1.leaderRoutineManager.Wait() + }() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + _, origRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, origRoot.IntermediateCerts, 1) + + cert, err := connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(origRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), origRoot.SigningKeyID) + + err = s1.caManager.UpdateConfiguration(&structs.CARequest{ + Config: &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root-2/", + "IntermediatePKIPath": "pki-intermediate-2/", + }, + }, + }) + require.NoError(t, err) + + _, newRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, newRoot.IntermediateCerts, 2, + "expected one cross-sign cert and one local leaf sign cert") + require.NotEqual(t, origRoot.ID, newRoot.ID) + + cert, err = connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(newRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) +}