Merge pull request #11780 from hashicorp/dnephin/ca-test-vault-in-secondary

ca: improve test coverage for RenewIntermediate

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)
 		}
 	}

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)
+	testrpc.WaitForActiveCARoot(t, s1.RPC, "primary", rootCert)
-	waitForActiveCARoot(t, s2, rootCert)
+	testrpc.WaitForActiveCARoot(t, s2.RPC, "secondary", rootCert)
 
 	// Capture the current intermediate
 	rootList, activeRoot, err = getTestRoots(s2, "secondary")

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.
+// runRenewIntermediate periodically attempts to renew the intermediate cert.
-func (c *CAManager) intermediateCertRenewalWatch(ctx context.Context) error {
+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),
+	if lessThanHalfTimePassed(c.timeNow(), intermediateCert.NotBefore, intermediateCert.NotAfter) {
-		intermediateCert.NotAfter) {
 		return nil
 	}
 

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,103 @@ 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)
+		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)
+		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
@@ -250,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.
+	patchIntermediateCertRenewInterval(t)
-	origInterval := structs.IntermediateCertRenewInterval
-	origDriftBuffer := ca.CertificateTimeDriftBuffer
-	defer func() {
-		structs.IntermediateCertRenewInterval = origInterval
-		ca.CertificateTimeDriftBuffer = origDriftBuffer
-	}()
-	structs.IntermediateCertRenewInterval = time.Millisecond
-	ca.CertificateTimeDriftBuffer = 0
 
 	conf := DefaultConfig()
 	conf.ConnectEnabled = true
@@ -515,3 +606,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
+}

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()
 }
@@ -338,26 +326,12 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) {
 	ca.SkipIfVaultNotPresent(t)
 
 	// no parallel execution because we change globals
-	origInterval := structs.IntermediateCertRenewInterval
+	patchIntermediateCertRenewInterval(t)
-	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
 	require := require.New(t)
 
 	testVault := ca.NewTestVaultServer(t)
-	defer testVault.Stop()
 
-	dir1, s1 := testServerWithConfig(t, func(c *Config) {
+	_, s1 := testServerWithConfig(t, func(c *Config) {
-		c.Build = "1.6.0"
 		c.PrimaryDatacenter = "dc1"
 		c.CAConfig = &structs.CAConfiguration{
 			Provider: "vault",
@@ -366,52 +340,37 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) {
 				"Token":               testVault.RootToken,
 				"RootPKIPath":         "pki-root/",
 				"IntermediatePKIPath": "pki-intermediate/",
-				"LeafCertTTL":         "1s",
+				"LeafCertTTL":         "2s",
-				// The retry loop only retries for 7sec max and
+				"IntermediateCertTTL": "7s",
-				// the ttl needs to be below so that it
-				// triggers definitely.
-				"IntermediateCertTTL": "5s",
 			},
 		}
 	})
-	defer os.RemoveAll(dir1)
 	defer func() {
 		s1.Shutdown()
 		s1.leaderRoutineManager.Wait()
 	}()
 
-	testrpc.WaitForLeader(t, s1.RPC, "dc1")
+	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
-	}
-
-	// Get the original intermediate.
-	waitForActiveCARoot(t, s1, originalRoot)
-	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)
-	require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot))
+	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))
 
 	// 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)
+		store := s1.caManager.delegate.State()
-		newIntermediatePEM, err := provider.ActiveIntermediate()
+		_, storedRoot, err := store.CARootActive(nil)
 		r.Check(err)
+
+		newIntermediatePEM := s1.caManager.getLeafSigningCertFromRoot(storedRoot)
 		if newIntermediatePEM == intermediatePEM {
 			r.Fatal("not a renewed intermediate")
 		}
@@ -420,47 +379,43 @@ 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.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot))
+	require.Len(roots.Roots, 1)
-	require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID)
 
-	// Get the root from dc1 and validate a chain of:
+	activeRoot = roots.Active()
-	// dc1 leaf -> dc1 intermediate -> dc1 root
+	require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID)
-	provider, caRoot := getCAProviderWithLock(s1)
+	require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot))
 
 	// 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)
+	req := structs.CASignRequest{CSR: csr}
+	cert := structs.IssuedCert{}
+	err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert)
 	require.NoError(err)
+	verifyLeafCert(t, activeRoot, cert.CertPEM)
+}
 
-	leafPEM, err := provider.Sign(leafCsr)
+func patchIntermediateCertRenewInterval(t *testing.T) {
-	require.NoError(err)
+	origInterval := structs.IntermediateCertRenewInterval
+	origMinTTL := structs.MinLeafCertTTL
 
-	cert, err := connect.ParseCert(leafPEM)
+	structs.IntermediateCertRenewInterval = 200 * time.Millisecond
-	require.NoError(err)
+	structs.MinLeafCertTTL = time.Second
 
-	// Check that the leaf signed by the new intermediate can be verified using the
+	t.Cleanup(func() {
-	// returned cert chain (signed intermediate + remote root).
+		structs.IntermediateCertRenewInterval = origInterval
-	intermediatePool := x509.NewCertPool()
+		structs.MinLeafCertTTL = origMinTTL
-	// 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,
 	})
-	require.NoError(err)
 }
 
 func TestCAManager_RenewIntermediate_Secondary(t *testing.T) {
@@ -469,18 +424,10 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) {
 	}
 
 	// no parallel execution because we change globals
-	origInterval := structs.IntermediateCertRenewInterval
+	patchIntermediateCertRenewInterval(t)
-	origMinTTL := structs.MinLeafCertTTL
-	defer func() {
-		structs.IntermediateCertRenewInterval = origInterval
-		structs.MinLeafCertTTL = origMinTTL
-	}()
-
-	structs.IntermediateCertRenewInterval = time.Millisecond
-	structs.MinLeafCertTTL = time.Second
 	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",
@@ -499,7 +446,6 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) {
 			},
 		}
 	})
-	defer os.RemoveAll(dir1)
 	defer func() {
 		s1.Shutdown()
 		s1.leaderRoutineManager.Wait()
@@ -508,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()
@@ -521,96 +465,60 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) {
 
 	// Create the WAN link
 	joinWAN(t, s2, s1)
-	testrpc.WaitForLeader(t, s2.RPC, "dc2")
+	testrpc.WaitForActiveCARoot(t, s2.RPC, "dc2", nil)
-
-	// 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)
-	currentCertSerialNumber := intermediateCert.SerialNumber
-	currentCertAuthorityKeyId := intermediateCert.AuthorityKeyId
-
-	// Capture the current root
-	var originalRoot *structs.CARoot
-	{
-		rootList, activeRoot, err := getTestRoots(s1, "dc1")
-		require.NoError(err)
-		require.Len(rootList.Roots, 1)
-		originalRoot = activeRoot
-	}
-
-	waitForActiveCARoot(t, s1, originalRoot)
-	waitForActiveCARoot(t, s2, originalRoot)
 
 	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)
 
 	// 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)
+		store := s2.caManager.delegate.State()
-		intermediatePEM, err = secondaryProvider.ActiveIntermediate()
+		_, storedRoot, err := store.CARootActive(nil)
 		r.Check(err)
-		cert, err := connect.ParseCert(intermediatePEM)
+
-		r.Check(err)
+		newIntermediatePEM := s2.caManager.getLeafSigningCertFromRoot(storedRoot)
-		if cert.SerialNumber.Cmp(currentCertSerialNumber) == 0 || !reflect.DeepEqual(cert.AuthorityKeyId, currentCertAuthorityKeyId) {
+		if newIntermediatePEM == intermediatePEM {
-			currentCertSerialNumber = cert.SerialNumber
-			currentCertAuthorityKeyId = cert.AuthorityKeyId
 			r.Fatal("not a renewed intermediate")
 		}
-		intermediateCert = cert
+		intermediateCert, err = connect.ParseCert(newIntermediatePEM)
+		r.Check(err)
+		intermediatePEM = newIntermediatePEM
 	})
 
+	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)
-
+	require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot))
-	// 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)
+	req := structs.CASignRequest{CSR: csr}
-	require.NoError(err)
+	cert := structs.IssuedCert{}
-
+	err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert)
-	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,
-	})
 	require.NoError(err)
+	verifyLeafCert(t, activeRoot, cert.CertPEM)
 }
 
 func TestConnectCA_ConfigurationSet_RootRotation_Secondary(t *testing.T) {
@@ -1188,14 +1096,7 @@ func getTestRoots(s *Server, datacenter string) (*structs.IndexedCARoots, *struc
 		return nil, nil, err
 	}
 
-	var active *structs.CARoot
+	active := rootList.Active()
-	for _, root := range rootList.Roots {
-		if root.Active {
-			active = root
-			break
-		}
-	}
-
 	return &rootList, active, nil
 }
 

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 {
+	return idx, roots.Active(), err
-		for _, r := range roots {
-			if r.Active {
-				result = r
-				break
-			}
-		}
-	}
-
-	return idx, result, err
 }
 
 // CARootSetCAS sets the current CA root state using a check-and-set operation.

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.
@@ -145,6 +154,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.

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
+		root := reply.Active()
-		for _, r := range reply.Roots {
-			if r.ID == reply.ActiveRootID {
-				root = r
-				break
-			}
-		}
 		if root == nil {
 			r.Fatal("no active root")
 		}