From a5d9b1d32264de932902806a3ecc7fc5a7669210 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 28 Nov 2021 14:11:03 -0500 Subject: [PATCH 1/7] ca: Add CARoots.Active method Which will be used in the next commit. --- agent/consul/state/connect_ca.go | 12 +----------- agent/structs/connect_ca.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/agent/consul/state/connect_ca.go b/agent/consul/state/connect_ca.go index 0b35d03934..156c79f463 100644 --- a/agent/consul/state/connect_ca.go +++ b/agent/consul/state/connect_ca.go @@ -252,18 +252,8 @@ func caRootsTxn(tx ReadTxn, ws memdb.WatchSet) (uint64, structs.CARoots, error) func (s *Store) CARootActive(ws memdb.WatchSet) (uint64, *structs.CARoot, error) { // Get all the roots since there should never be that many and just // do the filtering in this method. - var result *structs.CARoot idx, roots, err := s.CARoots(ws) - if err == nil { - for _, r := range roots { - if r.Active { - result = r - break - } - } - } - - return idx, result, err + return idx, roots.Active(), err } // CARootSetCAS sets the current CA root state using a check-and-set operation. diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 9da766d757..a556df0745 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -145,6 +145,20 @@ func (c *CARoot) Clone() *CARoot { // CARoots is a list of CARoot structures. type CARoots []*CARoot +// Active returns the single CARoot that is marked as active, or nil if there +// is no active root (ex: when they are no roots). +func (c CARoots) Active() *CARoot { + if c == nil { + return nil + } + for _, r := range c { + if r.Active { + return r + } + } + return nil +} + // CASignRequest is the request for signing a service certificate. type CASignRequest struct { // Datacenter is the target for this request. From a4ba1f348ddbba770a876d32469fdbc981f24bfe Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 6 Dec 2021 19:05:35 -0500 Subject: [PATCH 2/7] ca: add a test for Vault in secondary DC --- agent/consul/leader_connect_ca_test.go | 122 +++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 82dabe87cf..18c12473eb 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -11,9 +11,11 @@ import ( "errors" "fmt" "math/big" + "net/rpc" "testing" "time" + msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -31,6 +33,107 @@ import ( // TODO(kyhavlov): replace with t.Deadline() const CATestTimeout = 7 * time.Second +func TestCAManager_Initialize_Vault_Secondary_SharedVault(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + ca.SkipIfVaultNotPresent(t) + + vault := ca.NewTestVaultServer(t) + + _, serverDC1 := testServerWithConfig(t, func(c *Config) { + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-primary/", + }, + } + }) + + runStep(t, "check primary DC", func(t *testing.T) { + testrpc.WaitForTestAgent(t, serverDC1.RPC, "dc1") + + codec := rpcClient(t, serverDC1) + defer codec.Close() + + roots := structs.IndexedCARoots{} + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(t, err) + require.Len(t, roots.Roots, 1) + + leafPEM := getLeafCert(t, codec, roots.TrustDomain, "dc1") + verifyLeafCert(t, roots.Roots[0], leafPEM) + }) + + runStep(t, "start secondary DC", func(t *testing.T) { + _, serverDC2 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "dc2" + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-secondary/", + }, + } + }) + defer serverDC2.Shutdown() + joinWAN(t, serverDC2, serverDC1) + testrpc.WaitForActiveCARoot(t, serverDC2.RPC, "dc2", nil) + + codec := rpcClient(t, serverDC2) + defer codec.Close() + + roots := structs.IndexedCARoots{} + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(t, err) + require.Len(t, roots.Roots, 1) + + leafPEM := getLeafCert(t, codec, roots.TrustDomain, "dc2") + verifyLeafCert(t, roots.Roots[0], leafPEM) + }) +} + +func verifyLeafCert(t *testing.T, root *structs.CARoot, leafCertPEM string) { + t.Helper() + leaf, intermediates, err := connect.ParseLeafCerts(leafCertPEM) + require.NoError(t, err) + + pool := x509.NewCertPool() + ok := pool.AppendCertsFromPEM([]byte(root.RootCert)) + if !ok { + t.Fatalf("Failed to add root CA PEM to cert pool") + } + + // verify with intermediates from leaf CertPEM + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: pool, + Intermediates: intermediates, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + require.NoError(t, err, "failed to verify using intermediates from leaf cert PEM") + + // verify with intermediates from the CARoot + intermediates = x509.NewCertPool() + for _, intermediate := range root.IntermediateCerts { + c, err := connect.ParseCert(intermediate) + require.NoError(t, err) + intermediates.AddCert(c) + } + + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: pool, + Intermediates: intermediates, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + require.NoError(t, err, "failed to verify using intermediates from CARoot list") +} + type mockCAServerDelegate struct { t *testing.T config *Config @@ -515,3 +618,22 @@ func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { require.NoError(t, err) require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) } + +func getLeafCert(t *testing.T, codec rpc.ClientCodec, trustDomain string, dc string) string { + pk, _, err := connect.GeneratePrivateKey() + require.NoError(t, err) + spiffeID := &connect.SpiffeIDService{ + Host: trustDomain, + Service: "srv1", + Datacenter: dc, + } + csr, err := connect.CreateCSR(spiffeID, pk, nil, nil) + require.NoError(t, err) + + req := structs.CASignRequest{CSR: csr} + cert := structs.IssuedCert{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) + require.NoError(t, err) + + return cert.CertPEM +} From 2e4e8bd791a66831e7375de72f4f8c73ef8f0820 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 7 Dec 2021 16:21:23 -0500 Subject: [PATCH 3/7] ca: improve RenewIntermediate tests Use the new verifyLearfCert to show the cert verifies with intermediates from both sources. This required using the RPC interface so that the leaf pem was constructed correctly. Add IndexedCARoots.Active since that is a common operation we see in a few places. --- agent/consul/leader_connect_ca_test.go | 4 -- agent/consul/leader_connect_test.go | 79 ++++++++------------------ agent/structs/connect_ca.go | 9 +++ 3 files changed, 34 insertions(+), 58 deletions(-) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 18c12473eb..470bcdd986 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -57,8 +57,6 @@ func TestCAManager_Initialize_Vault_Secondary_SharedVault(t *testing.T) { testrpc.WaitForTestAgent(t, serverDC1.RPC, "dc1") codec := rpcClient(t, serverDC1) - defer codec.Close() - roots := structs.IndexedCARoots{} err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) require.NoError(t, err) @@ -87,8 +85,6 @@ func TestCAManager_Initialize_Vault_Secondary_SharedVault(t *testing.T) { testrpc.WaitForActiveCARoot(t, serverDC2.RPC, "dc2", nil) codec := rpcClient(t, serverDC2) - defer codec.Close() - roots := structs.IndexedCARoots{} err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) require.NoError(t, err) diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index abaa45b6b9..210c27cafa 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -420,8 +420,13 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { intermediatePEM = newIntermediatePEM }) - _, activeRoot, err = store.CARootActive(nil) + codec := rpcClient(t, s1) + roots := structs.IndexedCARoots{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) require.NoError(err) + require.Len(roots.Roots, 1) + + activeRoot = roots.Active() require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) @@ -431,36 +436,18 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { // Have the new intermediate sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: roots.TrustDomain, Namespace: "default", Datacenter: "dc1", Service: "foo", } - raw, _ := connect.TestCSR(t, spiffeService) + csr, _ := connect.TestCSR(t, spiffeService) - leafCsr, err := connect.ParseCSR(raw) - require.NoError(err) - - leafPEM, err := provider.Sign(leafCsr) - require.NoError(err) - - cert, 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{ - Intermediates: intermediatePool, - Roots: rootPool, - }) + req := structs.CASignRequest{CSR: csr} + cert := structs.IssuedCert{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) require.NoError(err) + verifyLeafCert(t, caRoot, cert.CertPEM) } func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { @@ -570,47 +557,31 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { intermediateCert = cert }) + codec := rpcClient(t, s2) + roots := structs.IndexedCARoots{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(err) + require.Len(roots.Roots, 1) + _, activeRoot, err = store.CARootActive(nil) require.NoError(err) 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: - // dc2 leaf -> dc2 intermediate -> dc1 root - _, caRoot := getCAProviderWithLock(s1) - // Have dc2 sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: roots.TrustDomain, Namespace: "default", - Datacenter: "dc1", + Datacenter: "dc2", Service: "foo", } - raw, _ := connect.TestCSR(t, spiffeService) + csr, _ := connect.TestCSR(t, spiffeService) - leafCsr, err := connect.ParseCSR(raw) - require.NoError(err) - - leafPEM, err := secondaryProvider.Sign(leafCsr) - require.NoError(err) - - 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 = intermediateCert.Verify(x509.VerifyOptions{ - Intermediates: intermediatePool, - Roots: rootPool, - }) + req := structs.CASignRequest{CSR: csr} + cert := structs.IssuedCert{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) require.NoError(err) + verifyLeafCert(t, activeRoot, cert.CertPEM) } func TestConnectCA_ConfigurationSet_RootRotation_Secondary(t *testing.T) { diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index a556df0745..f3d175d350 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -55,6 +55,15 @@ type IndexedCARoots struct { QueryMeta `json:"-"` } +func (r IndexedCARoots) Active() *CARoot { + for _, root := range r.Roots { + if root.ID == r.ActiveRootID { + return root + } + } + return nil +} + // CARoot represents a root CA certificate that is trusted. type CARoot struct { // ID is a globally unique ID (UUID) representing this CA root. From 1dec6bb815fd75e6afa675a56ed8d585fe1accc1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Dec 2021 12:21:52 -0500 Subject: [PATCH 4/7] ca: fix flakes in RenewIntermediate tests I suspect one problem was that we set structs.IntermediateCertRenewInterval to 1ms, which meant that in some cases the intermediate could renew before we stored the original value. Another problem was that the 'wait for intermediate' loop was calling the provider.ActiveIntermediate, but the comparison needs to use the RPC endpoint to accurately represent a user request. So changing the 'wait for' to use the state store ensures we don't race. Also moves the patching into a separate function. Removes the addition of ca.CertificateTimeDriftBuffer as part of calculating halfTime. This was added in a previous commit to attempt to fix the flake, but it did not appear to fix the problem. Adding the time here was making the tests fail when using the shared patch function. It's not clear to me why, but there's no reason we should be including this time in the halfTime calculation. --- agent/consul/leader_connect_ca.go | 9 ++- agent/consul/leader_connect_ca_test.go | 10 +--- agent/consul/leader_connect_test.go | 76 +++++++++++--------------- 3 files changed, 38 insertions(+), 57 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index dc712a9e55..82956bed2c 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -347,7 +347,7 @@ func (c *CAManager) startPostInitializeRoutines(ctx context.Context) { c.leaderRoutineManager.Start(ctx, secondaryCARootWatchRoutineName, c.secondaryCARootWatch) } - c.leaderRoutineManager.Start(ctx, intermediateCertRenewWatchRoutineName, c.intermediateCertRenewalWatch) + c.leaderRoutineManager.Start(ctx, intermediateCertRenewWatchRoutineName, c.runRenewIntermediate) } func (c *CAManager) backgroundCAInitialization(ctx context.Context) error { @@ -1111,8 +1111,8 @@ func setLeafSigningCert(caRoot *structs.CARoot, pem string) error { return nil } -// intermediateCertRenewalWatch periodically attempts to renew the intermediate cert. -func (c *CAManager) intermediateCertRenewalWatch(ctx context.Context) error { +// runRenewIntermediate periodically attempts to renew the intermediate cert. +func (c *CAManager) runRenewIntermediate(ctx context.Context) error { isPrimary := c.serverConf.Datacenter == c.serverConf.PrimaryDatacenter for { @@ -1180,8 +1180,7 @@ func (c *CAManager) RenewIntermediate(ctx context.Context, isPrimary bool) error return fmt.Errorf("error parsing active intermediate cert: %v", err) } - if lessThanHalfTimePassed(c.timeNow(), intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer), - intermediateCert.NotAfter) { + if lessThanHalfTimePassed(c.timeNow(), intermediateCert.NotBefore, intermediateCert.NotAfter) { return nil } diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 470bcdd986..fc2150eea8 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -349,15 +349,7 @@ func TestCAManager_Initialize(t *testing.T) { func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) { // No parallel execution because we change globals - // Set the interval and drift buffer low for renewing the cert. - origInterval := structs.IntermediateCertRenewInterval - origDriftBuffer := ca.CertificateTimeDriftBuffer - defer func() { - structs.IntermediateCertRenewInterval = origInterval - ca.CertificateTimeDriftBuffer = origDriftBuffer - }() - structs.IntermediateCertRenewInterval = time.Millisecond - ca.CertificateTimeDriftBuffer = 0 + patchIntermediateCertRenewInterval(t) conf := DefaultConfig() conf.ConnectEnabled = true diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 210c27cafa..9ae5a9aaec 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -338,19 +338,7 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { ca.SkipIfVaultNotPresent(t) // no parallel execution because we change globals - origInterval := structs.IntermediateCertRenewInterval - origMinTTL := structs.MinLeafCertTTL - origDriftBuffer := ca.CertificateTimeDriftBuffer - defer func() { - structs.IntermediateCertRenewInterval = origInterval - structs.MinLeafCertTTL = origMinTTL - ca.CertificateTimeDriftBuffer = origDriftBuffer - }() - - // Vault backdates certs by 30s by default. - ca.CertificateTimeDriftBuffer = 30 * time.Second - structs.IntermediateCertRenewInterval = time.Millisecond - structs.MinLeafCertTTL = time.Second + patchIntermediateCertRenewInterval(t) require := require.New(t) testVault := ca.NewTestVaultServer(t) @@ -380,7 +368,7 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { s1.leaderRoutineManager.Wait() }() - testrpc.WaitForLeader(t, s1.RPC, "dc1") + testrpc.WaitForActiveCARoot(t, s1.RPC, "dc1", nil) // Capture the current root. var originalRoot *structs.CARoot @@ -390,6 +378,7 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { require.Len(rootList.Roots, 1) originalRoot = activeRoot } + t.Log("original SigningKeyID", originalRoot.SigningKeyID) // Get the original intermediate. waitForActiveCARoot(t, s1, originalRoot) @@ -403,15 +392,16 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { store := s1.caManager.delegate.State() _, activeRoot, err := store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) // 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() + store := s1.caManager.delegate.State() + _, storedRoot, err := store.CARootActive(nil) r.Check(err) + + newIntermediatePEM := s1.caManager.getLeafSigningCertFromRoot(storedRoot) if newIntermediatePEM == intermediatePEM { r.Fatal("not a renewed intermediate") } @@ -427,8 +417,8 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { require.Len(roots.Roots, 1) activeRoot = roots.Active() - require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) // Get the root from dc1 and validate a chain of: // dc1 leaf -> dc1 intermediate -> dc1 root @@ -450,21 +440,26 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { verifyLeafCert(t, caRoot, cert.CertPEM) } +func patchIntermediateCertRenewInterval(t *testing.T) { + origInterval := structs.IntermediateCertRenewInterval + origMinTTL := structs.MinLeafCertTTL + + structs.IntermediateCertRenewInterval = 200 * time.Millisecond + structs.MinLeafCertTTL = time.Second + + t.Cleanup(func() { + structs.IntermediateCertRenewInterval = origInterval + structs.MinLeafCertTTL = origMinTTL + }) +} + func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } // no parallel execution because we change globals - origInterval := structs.IntermediateCertRenewInterval - origMinTTL := structs.MinLeafCertTTL - defer func() { - structs.IntermediateCertRenewInterval = origInterval - structs.MinLeafCertTTL = origMinTTL - }() - - structs.IntermediateCertRenewInterval = time.Millisecond - structs.MinLeafCertTTL = time.Second + patchIntermediateCertRenewInterval(t) require := require.New(t) dir1, s1 := testServerWithConfig(t, func(c *Config) { @@ -517,8 +512,6 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { require.NoError(err) intermediateCert, err := connect.ParseCert(intermediatePEM) require.NoError(err) - currentCertSerialNumber := intermediateCert.SerialNumber - currentCertAuthorityKeyId := intermediateCert.AuthorityKeyId // Capture the current root var originalRoot *structs.CARoot @@ -528,6 +521,7 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { require.Len(rootList.Roots, 1) originalRoot = activeRoot } + t.Log("original SigningKeyID", originalRoot.SigningKeyID) waitForActiveCARoot(t, s1, originalRoot) waitForActiveCARoot(t, s2, originalRoot) @@ -539,22 +533,18 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { 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 - // however, defaultQueryTime will be configurable and we con lower it - // so that it returns for sure. retry.Run(t, func(r *retry.R) { - secondaryProvider, _ = getCAProviderWithLock(s2) - intermediatePEM, err = secondaryProvider.ActiveIntermediate() + store := s2.caManager.delegate.State() + _, storedRoot, err := store.CARootActive(nil) r.Check(err) - cert, err := connect.ParseCert(intermediatePEM) - r.Check(err) - if cert.SerialNumber.Cmp(currentCertSerialNumber) == 0 || !reflect.DeepEqual(cert.AuthorityKeyId, currentCertAuthorityKeyId) { - currentCertSerialNumber = cert.SerialNumber - currentCertAuthorityKeyId = cert.AuthorityKeyId + + newIntermediatePEM := s2.caManager.getLeafSigningCertFromRoot(storedRoot) + if newIntermediatePEM == intermediatePEM { r.Fatal("not a renewed intermediate") } - intermediateCert = cert + intermediateCert, err = connect.ParseCert(newIntermediatePEM) + r.Check(err) + intermediatePEM = newIntermediatePEM }) codec := rpcClient(t, s2) @@ -565,8 +555,8 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { _, activeRoot, err = store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) // Have dc2 sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ From 305655a8b13de2f8e9c6a58bdc1965e76029a790 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Dec 2021 12:35:36 -0500 Subject: [PATCH 5/7] ca: remove duplicate WaitFor function --- agent/consul/connect_ca_endpoint_test.go | 4 ++-- agent/consul/leader_connect_test.go | 17 ++--------------- testrpc/wait.go | 11 +++-------- 3 files changed, 7 insertions(+), 25 deletions(-) diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index d5e52f9e3f..40a384f902 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -697,8 +697,8 @@ func TestConnectCAConfig_UpdateSecondary(t *testing.T) { require.Len(rootList.Roots, 1) rootCert := activeRoot - waitForActiveCARoot(t, s1, rootCert) - waitForActiveCARoot(t, s2, rootCert) + testrpc.WaitForActiveCARoot(t, s1.RPC, "primary", rootCert) + testrpc.WaitForActiveCARoot(t, s2.RPC, "secondary", rootCert) // Capture the current intermediate rootList, activeRoot, err = getTestRoots(s2, "secondary") diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 9ae5a9aaec..2e7d8b9710 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -314,18 +314,6 @@ func TestCAManager_Initialize_Secondary(t *testing.T) { } } -func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) { - retry.Run(t, func(r *retry.R) { - _, root := getCAProviderWithLock(srv) - if root == nil { - r.Fatal("no root") - } - if root.ID != expect.ID { - r.Fatalf("current active root is %s; waiting for %s", root.ID, expect.ID) - } - }) -} - func getCAProviderWithLock(s *Server) (ca.Provider, *structs.CARoot) { return s.caManager.getCAProvider() } @@ -381,7 +369,6 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { t.Log("original SigningKeyID", originalRoot.SigningKeyID) // Get the original intermediate. - waitForActiveCARoot(t, s1, originalRoot) provider, _ := getCAProviderWithLock(s1) intermediatePEM, err := provider.ActiveIntermediate() require.NoError(err) @@ -523,8 +510,8 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { } t.Log("original SigningKeyID", originalRoot.SigningKeyID) - waitForActiveCARoot(t, s1, originalRoot) - waitForActiveCARoot(t, s2, originalRoot) + testrpc.WaitForActiveCARoot(t, s1.RPC, "dc1", originalRoot) + testrpc.WaitForActiveCARoot(t, s2.RPC, "dc2", originalRoot) store := s2.fsm.State() _, activeRoot, err := store.CARootActive(nil) diff --git a/testrpc/wait.go b/testrpc/wait.go index 63f6b603ea..a872668cea 100644 --- a/testrpc/wait.go +++ b/testrpc/wait.go @@ -3,9 +3,10 @@ package testrpc import ( "testing" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/testutil/retry" - "github.com/stretchr/testify/require" ) type rpcFn func(string, interface{}, interface{}) error @@ -152,13 +153,7 @@ func WaitForActiveCARoot(t *testing.T, rpc rpcFn, dc string, expect *structs.CAR r.Fatalf("err: %v", err) } - var root *structs.CARoot - for _, r := range reply.Roots { - if r.ID == reply.ActiveRootID { - root = r - break - } - } + root := reply.Active() if root == nil { r.Fatal("no active root") } From 968aeff1bbf119d3c4e3238aa035e8a453b5e0ed Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Dec 2021 12:50:58 -0500 Subject: [PATCH 6/7] ca: prune some unnecessary lookups in the tests --- agent/consul/leader_connect_test.go | 87 +++++++---------------------- 1 file changed, 20 insertions(+), 67 deletions(-) diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 2e7d8b9710..3257e9f79b 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -330,10 +330,8 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { require := require.New(t) testVault := ca.NewTestVaultServer(t) - defer testVault.Stop() - dir1, s1 := testServerWithConfig(t, func(c *Config) { - c.Build = "1.6.0" + _, s1 := testServerWithConfig(t, func(c *Config) { c.PrimaryDatacenter = "dc1" c.CAConfig = &structs.CAConfiguration{ Provider: "vault", @@ -342,15 +340,11 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { "Token": testVault.RootToken, "RootPKIPath": "pki-root/", "IntermediatePKIPath": "pki-intermediate/", - "LeafCertTTL": "1s", - // The retry loop only retries for 7sec max and - // the ttl needs to be below so that it - // triggers definitely. - "IntermediateCertTTL": "5s", + "LeafCertTTL": "2s", + "IntermediateCertTTL": "7s", }, } }) - defer os.RemoveAll(dir1) defer func() { s1.Shutdown() s1.leaderRoutineManager.Wait() @@ -358,27 +352,15 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { testrpc.WaitForActiveCARoot(t, s1.RPC, "dc1", nil) - // Capture the current root. - var originalRoot *structs.CARoot - { - rootList, activeRoot, err := getTestRoots(s1, "dc1") - require.NoError(err) - require.Len(rootList.Roots, 1) - originalRoot = activeRoot - } - t.Log("original SigningKeyID", originalRoot.SigningKeyID) - - // Get the original intermediate. - provider, _ := getCAProviderWithLock(s1) - intermediatePEM, err := provider.ActiveIntermediate() - require.NoError(err) - 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) + t.Log("original SigningKeyID", activeRoot.SigningKeyID) + + intermediatePEM := s1.caManager.getLeafSigningCertFromRoot(activeRoot) + intermediateCert, err := connect.ParseCert(intermediatePEM) + require.NoError(err) + require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) @@ -407,10 +389,6 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) - // Get the root from dc1 and validate a chain of: - // dc1 leaf -> dc1 intermediate -> dc1 root - provider, caRoot := getCAProviderWithLock(s1) - // Have the new intermediate sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ Host: roots.TrustDomain, @@ -424,7 +402,7 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { cert := structs.IssuedCert{} err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) require.NoError(err) - verifyLeafCert(t, caRoot, cert.CertPEM) + verifyLeafCert(t, activeRoot, cert.CertPEM) } func patchIntermediateCertRenewInterval(t *testing.T) { @@ -449,7 +427,7 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { patchIntermediateCertRenewInterval(t) require := require.New(t) - dir1, s1 := testServerWithConfig(t, func(c *Config) { + _, s1 := testServerWithConfig(t, func(c *Config) { c.Build = "1.6.0" c.CAConfig = &structs.CAConfiguration{ Provider: "consul", @@ -468,7 +446,6 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { }, } }) - defer os.RemoveAll(dir1) defer func() { s1.Shutdown() s1.leaderRoutineManager.Wait() @@ -477,12 +454,10 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { testrpc.WaitForLeader(t, s1.RPC, "dc1") // dc2 as a secondary DC - dir2, s2 := testServerWithConfig(t, func(c *Config) { + _, s2 := testServerWithConfig(t, func(c *Config) { c.Datacenter = "dc2" c.PrimaryDatacenter = "dc1" - c.Build = "1.6.0" }) - defer os.RemoveAll(dir2) defer func() { s2.Shutdown() s2.leaderRoutineManager.Wait() @@ -490,32 +465,17 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { // Create the WAN link joinWAN(t, s2, s1) - testrpc.WaitForLeader(t, s2.RPC, "dc2") - - // Get the original intermediate - // TODO: Wait for intermediate instead of wait for leader - secondaryProvider, _ := getCAProviderWithLock(s2) - intermediatePEM, err := secondaryProvider.ActiveIntermediate() - require.NoError(err) - intermediateCert, err := connect.ParseCert(intermediatePEM) - require.NoError(err) - - // Capture the current root - var originalRoot *structs.CARoot - { - rootList, activeRoot, err := getTestRoots(s1, "dc1") - require.NoError(err) - require.Len(rootList.Roots, 1) - originalRoot = activeRoot - } - t.Log("original SigningKeyID", originalRoot.SigningKeyID) - - testrpc.WaitForActiveCARoot(t, s1.RPC, "dc1", originalRoot) - testrpc.WaitForActiveCARoot(t, s2.RPC, "dc2", originalRoot) + testrpc.WaitForActiveCARoot(t, s2.RPC, "dc2", nil) store := s2.fsm.State() _, activeRoot, err := store.CARootActive(nil) require.NoError(err) + t.Log("original SigningKeyID", activeRoot.SigningKeyID) + + intermediatePEM := s2.caManager.getLeafSigningCertFromRoot(activeRoot) + intermediateCert, err := connect.ParseCert(intermediatePEM) + require.NoError(err) + require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) @@ -1136,14 +1096,7 @@ func getTestRoots(s *Server, datacenter string) (*structs.IndexedCARoots, *struc return nil, nil, err } - var active *structs.CARoot - for _, root := range rootList.Roots { - if root.Active { - active = root - break - } - } - + active := rootList.Active() return &rootList, active, nil } From 4116a143e0d27408ac57085daaa780f8087bb0b6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Dec 2021 13:12:07 -0500 Subject: [PATCH 7/7] fix misleading errors on vault shutdown --- agent/connect/ca/testing.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index a1bbc6a4e2..92673dcb9e 100644 --- a/agent/connect/ca/testing.go +++ b/agent/connect/ca/testing.go @@ -1,6 +1,7 @@ package ca import ( + "errors" "fmt" "io/ioutil" "os" @@ -160,7 +161,7 @@ func runTestVault(t testing.T) (*TestVaultServer, error) { } t.Cleanup(func() { if err := testVault.Stop(); err != nil { - t.Log("failed to stop vault server: %w", err) + t.Logf("failed to stop vault server: %v", err) } }) @@ -207,7 +208,7 @@ func (v *TestVaultServer) Stop() error { } if v.cmd.Process != nil { - if err := v.cmd.Process.Signal(os.Interrupt); err != nil { + if err := v.cmd.Process.Signal(os.Interrupt); err != nil && !errors.Is(err, os.ErrProcessDone) { return fmt.Errorf("failed to kill vault server: %v", err) } }