Merge pull request #11718 from hashicorp/dnephin/backport-ca-fixes-1.9

[1.9.x] Backport of CA system bug fixes

.changelog/11671.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+ca: fixes a bug that caused the intermediate cert used to sign leaf certs to be missing from the /connect/ca/roots API response when the Vault provider was used.
+```

agent/connect/ca/provider.go

@@ -16,11 +16,12 @@ import (
 // on servers and CA provider.
 var ErrRateLimited = errors.New("operation rate limited by CA provider")
 
-// PrimaryIntermediateProviders is a list of CA providers that make use use of an
-// intermediate cert in the primary datacenter as well as the secondary. This is used
-// when determining whether to run the intermediate renewal routine in the primary.
-var PrimaryIntermediateProviders = map[string]struct{}{
-	"vault": {},
+// PrimaryUsesIntermediate is an optional interface  that CA providers may implement
+// to indicate that they use an intermediate cert in the primary datacenter as
+// well as the secondary. This is used when determining whether to run the
+// intermediate renewal routine in the primary.
+type PrimaryUsesIntermediate interface {
+	PrimaryUsesIntermediate()
 }
 
 // ProviderConfig encapsulates all the data Consul passes to `Configure` on a

agent/connect/ca/provider_vault.go

@@ -11,12 +11,13 @@ import (
 	"strings"
 	"time"
 
-	"github.com/hashicorp/consul/agent/connect"
-	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/consul/logging"
 	"github.com/hashicorp/go-hclog"
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/mitchellh/mapstructure"
+
+	"github.com/hashicorp/consul/agent/connect"
+	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/consul/logging"
 )
 
 const VaultCALeafCertRole = "leaf-cert"
@@ -518,7 +519,7 @@ func (v *VaultProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
 }
 
 // SupportsCrossSigning implements Provider
-func (c *VaultProvider) SupportsCrossSigning() (bool, error) {
+func (v *VaultProvider) SupportsCrossSigning() (bool, error) {
 	return true, nil
 }
 
@@ -557,6 +558,8 @@ func (v *VaultProvider) Stop() {
 	v.shutdown()
 }
 
+func (v *VaultProvider) PrimaryUsesIntermediate() {}
+
 func ParseVaultCAConfig(raw map[string]interface{}) (*structs.VaultCAProviderConfig, error) {
 	config := structs.VaultCAProviderConfig{
 		CommonCAProviderConfig: defaultCommonConfig(),

agent/consul/leader_connect_ca.go

@@ -9,12 +9,13 @@ import (
 	"sync"
 	"time"
 
+	"github.com/hashicorp/go-hclog"
+	uuid "github.com/hashicorp/go-uuid"
+
 	"github.com/hashicorp/consul/agent/connect"
 	"github.com/hashicorp/consul/agent/connect/ca"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/structs"
-	"github.com/hashicorp/go-hclog"
-	uuid "github.com/hashicorp/go-uuid"
 )
 
 type caState string
@@ -259,7 +260,7 @@ func (c *CAManager) Start() {
 	// Attempt to initialize the Connect CA now. This will
 	// happen during leader establishment and it would be great
 	// if the CA was ready to go once that process was finished.
-	if err := c.InitializeCA(); err != nil {
+	if err := c.Initialize(); err != nil {
 		c.logger.Error("Failed to initialize Connect CA", "error", err)
 
 		// we failed to fully initialize the CA so we need to spawn a
@@ -289,7 +290,7 @@ func (c *CAManager) startPostInitializeRoutines() {
 }
 
 func (c *CAManager) backgroundCAInitialization(ctx context.Context) error {
-	retryLoopBackoffAbortOnSuccess(ctx, c.InitializeCA, func(err error) {
+	retryLoopBackoffAbortOnSuccess(ctx, c.Initialize, func(err error) {
 		c.logger.Error("Failed to initialize Connect CA",
 			"routine", backgroundCAInitializationRoutineName,
 			"error", err,
@@ -306,10 +307,10 @@ func (c *CAManager) backgroundCAInitialization(ctx context.Context) error {
 	return nil
 }
 
-// InitializeCA sets up the CA provider when gaining leadership, either bootstrapping
+// Initialize sets up the CA provider when gaining leadership, either bootstrapping
 // the CA if this is the primary DC or making a remote RPC for intermediate signing
 // if this is a secondary DC.
-func (c *CAManager) InitializeCA() (reterr error) {
+func (c *CAManager) Initialize() (reterr error) {
 	// Bail if connect isn't enabled.
 	if !c.serverConf.ConnectEnabled {
 		return nil
@@ -447,12 +448,26 @@ func (c *CAManager) initializeRootCA(provider ca.Provider, conf *structs.CAConfi
 		}
 	}
 
+	var rootUpdateRequired bool
+
 	// Versions prior to 1.9.3, 1.8.8, and 1.7.12 incorrectly used the primary
 	// rootCA's subjectKeyID here instead of the intermediate. For
 	// provider=consul this didn't matter since there are no intermediates in
 	// the primaryDC, but for vault it does matter.
 	expectedSigningKeyID := connect.EncodeSigningKeyID(intermediateCert.SubjectKeyId)
-	needsSigningKeyUpdate := (rootCA.SigningKeyID != expectedSigningKeyID)
+	if rootCA.SigningKeyID != expectedSigningKeyID {
+		c.logger.Info("Correcting stored CARoot values",
+			"previous-signing-key", rootCA.SigningKeyID, "updated-signing-key", expectedSigningKeyID)
+		rootCA.SigningKeyID = expectedSigningKeyID
+		rootUpdateRequired = true
+	}
+
+	// Add the local leaf signing cert to the rootCA struct. This handles both
+	// upgrades of existing state, and new rootCA.
+	if c.getLeafSigningCertFromRoot(rootCA) != interPEM {
+		rootCA.IntermediateCerts = append(rootCA.IntermediateCerts, interPEM)
+		rootUpdateRequired = true
+	}
 
 	// Check if the CA root is already initialized and exit if it is,
 	// adding on any existing intermediate certs since they aren't directly
@@ -464,26 +479,21 @@ func (c *CAManager) initializeRootCA(provider ca.Provider, conf *structs.CAConfi
 	if err != nil {
 		return err
 	}
-	if activeRoot != nil && needsSigningKeyUpdate {
-		c.logger.Info("Correcting stored SigningKeyID value", "previous", rootCA.SigningKeyID, "updated", expectedSigningKeyID)
-
-	} else if activeRoot != nil && !needsSigningKeyUpdate {
+	if activeRoot != nil && !rootUpdateRequired {
 		// 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 {
 			return fmt.Errorf("stored CA root %q is not the active root (%s)", rootCA.ID, activeRoot.ID)
 		}
 
+		// TODO: why doesn't this c.setCAProvider(provider, activeRoot) ?
 		rootCA.IntermediateCerts = activeRoot.IntermediateCerts
 		c.setCAProvider(provider, rootCA)
 
+		c.logger.Info("initialized primary datacenter CA from existing CARoot with provider", "provider", conf.Provider)
 		return nil
 	}
 
-	if needsSigningKeyUpdate {
-		rootCA.SigningKeyID = expectedSigningKeyID
-	}
-
 	// Get the highest index
 	idx, _, err := state.CARoots(nil)
 	if err != nil {
@@ -511,6 +521,22 @@ func (c *CAManager) initializeRootCA(provider ca.Provider, conf *structs.CAConfi
 	return nil
 }
 
+// getLeafSigningCertFromRoot returns the PEM encoded certificate that should be used to
+// sign leaf certificates in the local datacenter. The SubjectKeyId of the
+// returned cert should always match the SigningKeyID of the CARoot.
+//
+// TODO: fix the data model so that we don't need this complicated lookup to
+// find the leaf signing cert. See github.com/hashicorp/consul/issues/11347.
+func (c *CAManager) getLeafSigningCertFromRoot(root *structs.CARoot) string {
+	if !c.isIntermediateUsedToSignLeaf() {
+		return root.RootCert
+	}
+	if len(root.IntermediateCerts) == 0 {
+		return ""
+	}
+	return root.IntermediateCerts[len(root.IntermediateCerts)-1]
+}
+
 // initializeSecondaryCA runs the routine for generating an intermediate CA CSR and getting
 // it signed by the primary DC if the root CA of the primary DC has changed since the last
 // intermediate. It should only be called while the state lock is held by setting the state
@@ -732,7 +758,7 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error)
 		}
 	}()
 
-	// Attempt to initialize the config if we failed to do so in InitializeCA for some reason
+	// Attempt to initialize the config if we failed to do so in Initialize for some reason
 	_, err = c.initializeCAConfig()
 	if err != nil {
 		return err
@@ -1076,15 +1102,8 @@ func (c *CAManager) RenewIntermediate(ctx context.Context, isPrimary bool) error
 
 	// If this is the primary, check if this is a provider that uses an intermediate cert. If
 	// it isn't, we don't need to check for a renewal.
-	if isPrimary {
-		_, config, err := state.CAConfig(nil)
-		if err != nil {
-			return err
-		}
-
-		if _, ok := ca.PrimaryIntermediateProviders[config.Provider]; !ok {
-			return nil
-		}
+	if isPrimary && !primaryUsesIntermediate(provider) {
+		return nil
 	}
 
 	activeIntermediate, err := provider.ActiveIntermediate()
@@ -1268,3 +1287,16 @@ func (c *CAManager) configuredSecondaryCA() bool {
 	defer c.stateLock.Unlock()
 	return c.actingSecondaryCA
 }
+
+func primaryUsesIntermediate(provider ca.Provider) bool {
+	_, ok := provider.(ca.PrimaryUsesIntermediate)
+	return ok
+}
+
+func (c *CAManager) isIntermediateUsedToSignLeaf() bool {
+	if c.serverConf.Datacenter != c.serverConf.PrimaryDatacenter {
+		return true
+	}
+	provider, _ := c.getCAProvider()
+	return primaryUsesIntermediate(provider)
+}

agent/consul/leader_connect_ca_test.go

@@ -1,22 +1,26 @@
 package consul
 
 import (
+	"bytes"
 	"context"
 	"crypto/x509"
 	"errors"
 	"fmt"
 	"testing"
 	"time"
 
+	"github.com/hashicorp/go-version"
+	"github.com/hashicorp/serf/serf"
+	"github.com/stretchr/testify/require"
+
 	"github.com/hashicorp/consul/agent/connect"
 	ca "github.com/hashicorp/consul/agent/connect/ca"
 	"github.com/hashicorp/consul/agent/consul/state"
 	"github.com/hashicorp/consul/agent/metadata"
 	"github.com/hashicorp/consul/agent/structs"
 	"github.com/hashicorp/consul/sdk/testutil"
-	"github.com/hashicorp/go-version"
-	"github.com/hashicorp/serf/serf"
-	"github.com/stretchr/testify/require"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
+	"github.com/hashicorp/consul/testrpc"
 )
 
 // TODO(kyhavlov): replace with t.Deadline()
@@ -179,7 +183,7 @@ func testCAConfig() *structs.CAConfiguration {
 func initTestManager(t *testing.T, manager *CAManager, delegate *mockCAServerDelegate) {
 	initCh := make(chan struct{})
 	go func() {
-		require.NoError(t, manager.InitializeCA())
+		require.NoError(t, manager.Initialize())
 		close(initCh)
 	}()
 	for i := 0; i < 5; i++ {
@@ -204,12 +208,12 @@ func TestCAManager_Initialize(t *testing.T) {
 	delegate := NewMockCAServerDelegate(t, conf)
 	manager := NewCAManager(delegate, nil, testutil.Logger(t), conf)
 
-	// Call InitializeCA and then confirm the RPCs and provider calls
+	// Call Initialize and then confirm the RPCs and provider calls
 	// happen in the expected order.
 	require.EqualValues(t, caStateUninitialized, manager.state)
 	errCh := make(chan error)
 	go func() {
-		errCh <- manager.InitializeCA()
+		errCh <- manager.Initialize()
 	}()
 
 	waitForCh(t, delegate.callbackCh, "forwardDC/ConnectCA.Roots")
@@ -220,7 +224,7 @@ func TestCAManager_Initialize(t *testing.T) {
 	waitForCh(t, delegate.callbackCh, "raftApply/ConnectCA")
 	waitForEmptyCh(t, delegate.callbackCh)
 
-	// Make sure the InitializeCA call returned successfully.
+	// Make sure the Initialize call returned successfully.
 	select {
 	case err := <-errCh:
 		require.NoError(t, err)
@@ -285,3 +289,34 @@ func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) {
 
 	require.EqualValues(t, caStateInitialized, manager.state)
 }
+
+func TestCAManager_Initialize_Logging(t *testing.T) {
+	if testing.Short() {
+		t.Skip("too slow for testing.Short")
+	}
+
+	t.Parallel()
+	_, conf1 := testServerConfig(t)
+
+	// Setup dummy logger to catch output
+	var buf bytes.Buffer
+	logger := testutil.LoggerWithOutput(t, &buf)
+
+	deps := newDefaultDeps(t, conf1)
+	deps.Logger = logger
+
+	s1, err := NewServer(conf1, deps)
+	require.NoError(t, err)
+	defer s1.Shutdown()
+	testrpc.WaitForLeader(t, s1.RPC, "dc1")
+
+	// Wait til CA root is setup
+	retry.Run(t, func(r *retry.R) {
+		var out structs.IndexedCARoots
+		r.Check(s1.RPC("ConnectCA.Roots", structs.DCSpecificRequest{
+			Datacenter: conf1.Datacenter,
+		}, &out))
+	})
+
+	require.Contains(t, buf.String(), "consul CA provider configured")
+}