From db254f09914920e6a9c22d9b4100a40bc76255a4 Mon Sep 17 00:00:00 2001
From: Kyle Havlovitz <kylehav@gmail.com>
Date: Wed, 11 Jul 2018 09:44:30 -0700
Subject: [PATCH] connect: persist intermediate CAs on leader change

---
 agent/consul/leader.go      | 21 +++++-----
 agent/consul/leader_test.go | 79 +++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 12 deletions(-)

diff --git a/agent/consul/leader.go b/agent/consul/leader.go
index 8b32226dc..b9c91c9dc 100644
--- a/agent/consul/leader.go
+++ b/agent/consul/leader.go
@@ -448,14 +448,6 @@ func (s *Server) initializeCA() error {
 		return err
 	}
 
-	// TODO(banks): in the case that we've just gained leadership in an already
-	// configured cluster. We really need to fetch RootCA from state to provide it
-	// in setCAProvider. This matters because if the current active root has
-	// intermediates, parsing the rootCA from only the root cert PEM above will
-	// not include them and so leafs we sign will not bundle the intermediates.
-
-	s.setCAProvider(provider, rootCA)
-
 	// Check if the CA root is already initialized and exit if it is.
 	// Every change to the CA after this initial bootstrapping should
 	// be done through the rotation process.
@@ -465,12 +457,15 @@ func (s *Server) initializeCA() error {
 		return err
 	}
 	if activeRoot != nil {
+		// 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 {
-			// TODO(banks): this seems like a pretty catastrophic state to get into.
-			// Shouldn't we do something stronger than warn and continue signing with
-			// a key that's not the active CA according to the state?
-			s.logger.Printf("[WARN] connect: CA root %q is not the active root (%q)", rootCA.ID, activeRoot.ID)
+			return fmt.Errorf("stored CA root %q is not the active root (%s)", rootCA.ID, activeRoot.ID)
 		}
+
+		rootCA.IntermediateCerts = activeRoot.IntermediateCerts
+		s.setCAProvider(provider, rootCA)
+
 		return nil
 	}
 
@@ -494,6 +489,8 @@ func (s *Server) initializeCA() error {
 		return respErr
 	}
 
+	s.setCAProvider(provider, rootCA)
+
 	s.logger.Printf("[INFO] connect: initialized CA with provider %q", conf.Provider)
 
 	return nil
diff --git a/agent/consul/leader_test.go b/agent/consul/leader_test.go
index a685223cb..862e9d68f 100644
--- a/agent/consul/leader_test.go
+++ b/agent/consul/leader_test.go
@@ -12,6 +12,7 @@ import (
 	"github.com/hashicorp/consul/testutil/retry"
 	"github.com/hashicorp/net-rpc-msgpackrpc"
 	"github.com/hashicorp/serf/serf"
+	"github.com/pascaldekloe/goe/verify"
 	"github.com/stretchr/testify/require"
 )
 
@@ -1064,3 +1065,81 @@ func TestLeader_CARootPruning(t *testing.T) {
 	require.True(roots[0].Active)
 	require.NotEqual(roots[0].ID, oldRoot.ID)
 }
+
+func TestLeader_PersistIntermediateCAs(t *testing.T) {
+	t.Parallel()
+
+	require := require.New(t)
+	dir1, s1 := testServer(t)
+	defer os.RemoveAll(dir1)
+	defer s1.Shutdown()
+	codec := rpcClient(t, s1)
+	defer codec.Close()
+
+	dir2, s2 := testServerDCBootstrap(t, "dc1", false)
+	defer os.RemoveAll(dir2)
+	defer s2.Shutdown()
+
+	dir3, s3 := testServerDCBootstrap(t, "dc1", false)
+	defer os.RemoveAll(dir3)
+	defer s3.Shutdown()
+
+	joinLAN(t, s2, s1)
+	joinLAN(t, s3, s1)
+
+	testrpc.WaitForLeader(t, s1.RPC, "dc1")
+
+	// Get the current root
+	rootReq := &structs.DCSpecificRequest{
+		Datacenter: "dc1",
+	}
+	var rootList structs.IndexedCARoots
+	require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList))
+	require.Len(rootList.Roots, 1)
+
+	// Update the provider config to use a new private key, which should
+	// cause a rotation.
+	_, newKey, err := connect.GeneratePrivateKey()
+	require.NoError(err)
+	newConfig := &structs.CAConfiguration{
+		Provider: "consul",
+		Config: map[string]interface{}{
+			"PrivateKey":     newKey,
+			"RootCert":       "",
+			"RotationPeriod": 90 * 24 * time.Hour,
+		},
+	}
+	{
+		args := &structs.CARequest{
+			Datacenter: "dc1",
+			Config:     newConfig,
+		}
+		var reply interface{}
+
+		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply))
+	}
+
+	// Get the active root before leader change.
+	_, root := s1.getCAProvider()
+	require.Len(root.IntermediateCerts, 1)
+
+	// Force a leader change and make sure the root CA values are preserved.
+	s1.Leave()
+	s1.Shutdown()
+
+	retry.Run(t, func(r *retry.R) {
+		var leader *Server
+		for _, s := range []*Server{s2, s3} {
+			if s.IsLeader() {
+				leader = s
+				break
+			}
+		}
+		if leader == nil {
+			r.Fatal("no leader")
+		}
+
+		_, newLeaderRoot := leader.getCAProvider()
+		verify.Values(r, "", root, newLeaderRoot)
+	})
+}