Actually return Intermediate certificates bundled with a leaf!

agent/connect/ca/provider.go

@@ -13,15 +13,23 @@ type Provider interface {
 	// ActiveIntermediate()
 	ActiveRoot() (string, error)
 
-	// ActiveIntermediate returns the current signing cert used by this
-	// provider for generating SPIFFE leaf certs.
+	// ActiveIntermediate returns the current signing cert used by this provider
+	// for generating SPIFFE leaf certs. Note that this must not change except
+	// when Consul requests the change via GenerateIntermediate. Changing the
+	// signing cert will break Consul's assumptions about which validation paths
+	// are active.
 	ActiveIntermediate() (string, error)
 
-	// GenerateIntermediate returns a new intermediate signing cert and
-	// sets it to the active intermediate.
+	// GenerateIntermediate returns a new intermediate signing cert and sets it to
+	// the active intermediate. If multiple intermediates are needed to complete
+	// the chain from the signing certificate back to the active root, they should
+	// all by bundled here.
 	GenerateIntermediate() (string, error)
 
-	// Sign signs a leaf certificate used by Connect proxies from a CSR.
+	// Sign signs a leaf certificate used by Connect proxies from a CSR. The PEM
+	// returned should include only the leaf certificate as all Intermediates
+	// needed to validate it will be added by Consul based on the active
+	// intemediate and any cross-signed intermediates managed by Consul.
 	Sign(*x509.CertificateRequest) (string, error)
 
 	// CrossSignCA must accept a CA certificate signed by another CA's key

agent/consul/connect_ca_endpoint.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"strings"
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/connect"
@@ -122,7 +123,7 @@ func (s *ConnectCA) ConfigurationSet(
 		}
 
 		// If the config has been committed, update the local provider instance
-		s.srv.setCAProvider(newProvider)
+		s.srv.setCAProvider(newProvider, newActiveRoot)
 
 		s.srv.logger.Printf("[INFO] connect: CA provider config updated")
 
@@ -150,7 +151,7 @@ func (s *ConnectCA) ConfigurationSet(
 	}
 
 	// Have the old provider cross-sign the new intermediate
-	oldProvider := s.srv.getCAProvider()
+	oldProvider, _ := s.srv.getCAProvider()
 	if oldProvider == nil {
 		return fmt.Errorf("internal error: CA provider is nil")
 	}
@@ -191,7 +192,7 @@ func (s *ConnectCA) ConfigurationSet(
 
 	// If the config has been committed, update the local provider instance
 	// and call teardown on the old provider
-	s.srv.setCAProvider(newProvider)
+	s.srv.setCAProvider(newProvider, newActiveRoot)
 
 	if err := oldProvider.Cleanup(); err != nil {
 		s.srv.logger.Printf("[WARN] connect: failed to clean up old provider %q", config.Provider)
@@ -309,7 +310,7 @@ func (s *ConnectCA) Sign(
 		return fmt.Errorf("SPIFFE ID in CSR must be a service ID")
 	}
 
-	provider := s.srv.getCAProvider()
+	provider, caRoot := s.srv.getCAProvider()
 	if provider == nil {
 		return fmt.Errorf("internal error: CA provider is nil")
 	}
@@ -348,6 +349,11 @@ func (s *ConnectCA) Sign(
 		return err
 	}
 
+	// Append any intermediates needed by this root.
+	for _, p := range caRoot.IntermediateCerts {
+		pem = strings.TrimSpace(pem) + "\n" + p
+	}
+
 	// TODO(banks): when we implement IssuedCerts table we can use the insert to
 	// that as the raft index to return in response. Right now we can rely on only
 	// the built-in provider being supported and the implementation detail that we

agent/consul/connect_ca_endpoint_test.go

@@ -2,6 +2,7 @@ package consul
 
 import (
 	"crypto/x509"
+	"encoding/pem"
 	"fmt"
 	"os"
 	"testing"
@@ -138,6 +139,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 	t.Parallel()
 
 	assert := assert.New(t)
+	require := require.New(t)
 	dir1, s1 := testServer(t)
 	defer os.RemoveAll(dir1)
 	defer s1.Shutdown()
@@ -151,7 +153,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 		Datacenter: "dc1",
 	}
 	var rootList structs.IndexedCARoots
-	assert.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList))
+	require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", rootReq, &rootList))
 	assert.Len(rootList.Roots, 1)
 	oldRoot := rootList.Roots[0]
 
@@ -174,17 +176,18 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 		}
 		var reply interface{}
 
-		assert.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply))
+		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationSet", args, &reply))
 	}
 
 	// Make sure the new root has been added along with an intermediate
 	// cross-signed by the old root.
+	var newRootPEM string
 	{
 		args := &structs.DCSpecificRequest{
 			Datacenter: "dc1",
 		}
 		var reply structs.IndexedCARoots
-		assert.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply))
+		require.Nil(msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", args, &reply))
 		assert.Len(reply.Roots, 2)
 
 		for _, r := range reply.Roots {
@@ -197,6 +200,7 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 				assert.Equal(r.SigningCert, oldRoot.SigningCert)
 				assert.Equal(r.IntermediateCerts, oldRoot.IntermediateCerts)
 			} else {
+				newRootPEM = r.RootCert
 				// The new root should have a valid cross-signed cert from the old
 				// root as an intermediate.
 				assert.True(r.Active)
@@ -225,15 +229,65 @@ func TestConnectCAConfig_TriggerRotation(t *testing.T) {
 			Datacenter: "dc1",
 		}
 		var reply structs.CAConfiguration
-		assert.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply))
+		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", args, &reply))
 
 		actual, err := ca.ParseConsulCAConfig(reply.Config)
-		assert.NoError(err)
+		require.NoError(err)
 		expected, err := ca.ParseConsulCAConfig(newConfig.Config)
-		assert.NoError(err)
+		require.NoError(err)
 		assert.Equal(reply.Provider, newConfig.Provider)
 		assert.Equal(actual, expected)
 	}
+
+	// Verify that new leaf certs get the cross-signed intermediate bundled
+	{
+		// Generate a CSR and request signing
+		spiffeId := connect.TestSpiffeIDService(t, "web")
+		csr, _ := connect.TestCSR(t, spiffeId)
+		args := &structs.CASignRequest{
+			Datacenter: "dc1",
+			CSR:        csr,
+		}
+		var reply structs.IssuedCert
+		require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply))
+
+		// Verify that the cert is signed by the new CA
+		{
+			roots := x509.NewCertPool()
+			require.True(roots.AppendCertsFromPEM([]byte(newRootPEM)))
+			leaf, err := connect.ParseCert(reply.CertPEM)
+			require.NoError(err)
+			_, err = leaf.Verify(x509.VerifyOptions{
+				Roots: roots,
+			})
+			require.NoError(err)
+		}
+
+		// And that it validates via the intermediate
+		{
+			roots := x509.NewCertPool()
+			assert.True(roots.AppendCertsFromPEM([]byte(oldRoot.RootCert)))
+			leaf, err := connect.ParseCert(reply.CertPEM)
+			require.NoError(err)
+
+			// Make sure the intermediate was returned as well as leaf
+			_, rest := pem.Decode([]byte(reply.CertPEM))
+			require.NotEmpty(rest)
+
+			intermediates := x509.NewCertPool()
+			require.True(intermediates.AppendCertsFromPEM(rest))
+
+			_, err = leaf.Verify(x509.VerifyOptions{
+				Roots:         roots,
+				Intermediates: intermediates,
+			})
+			require.NoError(err)
+		}
+
+		// Verify other fields
+		assert.Equal("web", reply.Service)
+		assert.Equal(spiffeId.URI().String(), reply.ServiceURI)
+	}
 }
 
 // Test CA signing

agent/consul/leader.go

@@ -236,7 +236,7 @@ func (s *Server) revokeLeadership() error {
 		return err
 	}
 
-	s.setCAProvider(nil)
+	s.setCAProvider(nil, nil)
 
 	s.resetConsistentReadReady()
 	s.autopilot.Stop()
@@ -422,8 +422,6 @@ func (s *Server) initializeCA() error {
 		return err
 	}
 
-	s.setCAProvider(provider)
-
 	// Get the active root cert from the CA
 	rootPEM, err := provider.ActiveRoot()
 	if err != nil {
@@ -435,6 +433,8 @@ func (s *Server) initializeCA() error {
 		return err
 	}
 
+	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.
@@ -507,12 +507,14 @@ func (s *Server) createCAProvider(conf *structs.CAConfiguration) (ca.Provider, e
 	}
 }
 
-func (s *Server) getCAProvider() ca.Provider {
+func (s *Server) getCAProvider() (ca.Provider, *structs.CARoot) {
 	retries := 0
 	var result ca.Provider
+	var resultRoot *structs.CARoot
 	for result == nil {
 		s.caProviderLock.RLock()
 		result = s.caProvider
+		resultRoot = s.caProviderRoot
 		s.caProviderLock.RUnlock()
 
 		// In cases where an agent is started with managed proxies, we may ask
@@ -527,13 +529,14 @@ func (s *Server) getCAProvider() ca.Provider {
 		break
 	}
 
-	return result
+	return result, resultRoot
 }
 
-func (s *Server) setCAProvider(newProvider ca.Provider) {
+func (s *Server) setCAProvider(newProvider ca.Provider, root *structs.CARoot) {
 	s.caProviderLock.Lock()
 	defer s.caProviderLock.Unlock()
 	s.caProvider = newProvider
+	s.caProviderRoot = root
 }
 
 // reconcileReaped is used to reconcile nodes that have failed and been reaped

agent/consul/server.go

@@ -100,6 +100,11 @@ type Server struct {
 	// caProvider is the current CA provider in use for Connect. This is
 	// only non-nil when we are the leader.
 	caProvider ca.Provider
+	// caProviderRoot is the CARoot that was stored along with the ca.Provider
+	// active. It's only updated in lock-step with the caProvider. This prevents
+	// races between state updates to active roots and the fetch of the provider
+	// instance.
+	caProviderRoot *structs.CARoot
 	caProviderLock sync.RWMutex
 
 	// Consul configuration