check expiry date of the root/intermediate before using it to sign a leaf (#10500)

* ca: move provider creation into CAManager

This further decouples the CAManager from Server. It reduces the interface between them and
removes the need for the SetLogger method on providers.

* ca: move SignCertificate to CAManager

To reduce the scope of Server, and keep all the CA logic together

* ca: move SignCertificate to the file where it is used

* auto-config: move autoConfigBackend impl off of Server

Most of these methods are used exclusively for the AutoConfig RPC
endpoint. This PR uses a pattern that we've used in other places as an
incremental step to reducing the scope of Server.

* fix linter issues

* check error when `raftApplyMsgpack`

* ca: move SignCertificate to CAManager

To reduce the scope of Server, and keep all the CA logic together

* check expiry date of the intermediate before using it to sign a leaf

* fix typo in comment

Co-authored-by: Kyle Havlovitz <kylehav@gmail.com>

* Fix test name

* do not check cert start date

* wrap error to mention it is the intermediate expired

* Fix failing test

* update comment

Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>

* use shim to avoid sleep in test

* add root cert validation

* remove duplicate code

* Revert "fix linter issues"

This reverts commit 6356302b54f06c8f2dee8e59740409d49e84ef24.

* fix import issue

* gofmt leader_connect_ca

* add changelog entry

* update error message

Co-authored-by: Freddy <freddygv@users.noreply.github.com>

* fix error message in test

Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>
Co-authored-by: Kyle Havlovitz <kylehav@gmail.com>
Co-authored-by: Freddy <freddygv@users.noreply.github.com>

.changelog/10500.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+check root and intermediate CA expiry before using it to sign a leaf certificate.
+```

agent/connect/ca/provider_aws.go

@@ -12,6 +12,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/acmpca"
+
 	"github.com/hashicorp/go-hclog"
 	"github.com/mitchellh/mapstructure"
 

agent/consul/leader_connect_ca.go

@@ -76,6 +76,9 @@ type CAManager struct {
 	leaderRoutineManager *routine.Manager
 	// providerShim is used to test CAManager with a fake provider.
 	providerShim ca.Provider
+
+	// shim time.Now for testing
+	timeNow func() time.Time
 }
 
 type caDelegateWithState struct {
@@ -131,6 +134,7 @@ func NewCAManager(delegate caServerDelegate, leaderRoutineManager *routine.Manag
 		serverConf:           config,
 		state:                caStateUninitialized,
 		leaderRoutineManager: leaderRoutineManager,
+		timeNow:              time.Now,
 	}
 }
 
@@ -740,7 +744,7 @@ func (c *CAManager) persistNewRootAndConfig(provider ca.Provider, newActiveRoot
 		newRoot := *r
 		if newRoot.Active && newActiveRoot != nil {
 			newRoot.Active = false
-			newRoot.RotatedOutAt = time.Now()
+			newRoot.RotatedOutAt = c.timeNow()
 		}
 		if newRoot.ExternalTrustDomain == "" {
 			newRoot.ExternalTrustDomain = newConf.ClusterID
@@ -991,7 +995,7 @@ func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error)
 		newRoot := *r
 		if newRoot.Active {
 			newRoot.Active = false
-			newRoot.RotatedOutAt = time.Now()
+			newRoot.RotatedOutAt = c.timeNow()
 		}
 		newRoots = append(newRoots, &newRoot)
 	}
@@ -1154,7 +1158,7 @@ func (c *CAManager) RenewIntermediate(ctx context.Context, isPrimary bool) error
 		return fmt.Errorf("error parsing active intermediate cert: %v", err)
 	}
 
-	if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer),
+	if lessThanHalfTimePassed(c.timeNow(), intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer),
 		intermediateCert.NotAfter) {
 		return nil
 	}
@@ -1453,6 +1457,26 @@ func (c *CAManager) SignCertificate(csr *x509.CertificateRequest, spiffeID conne
 
 	connect.HackSANExtensionForCSR(csr)
 
+	root, err := provider.ActiveRoot()
+	if err != nil {
+		return nil, err
+	}
+	// Check if the root expired before using it to sign.
+	err = c.checkExpired(root)
+	if err != nil {
+		return nil, fmt.Errorf("root expired: %w", err)
+	}
+
+	inter, err := provider.ActiveIntermediate()
+	if err != nil {
+		return nil, err
+	}
+	// Check if the intermediate expired before using it to sign.
+	err = c.checkExpired(inter)
+	if err != nil {
+		return nil, fmt.Errorf("intermediate expired: %w", err)
+	}
+
 	// All seems to be in order, actually sign it.
 
 	pem, err := provider.Sign(csr)
@@ -1469,14 +1493,6 @@ func (c *CAManager) SignCertificate(csr *x509.CertificateRequest, spiffeID conne
 	}
 
 	// Append our local CA's intermediate if there is one.
-	inter, err := provider.ActiveIntermediate()
-	if err != nil {
-		return nil, err
-	}
-	root, err := provider.ActiveRoot()
-	if err != nil {
-		return nil, err
-	}
 	if inter != root {
 		pem = pem + ca.EnsureTrailingNewline(inter)
 	}
@@ -1513,3 +1529,14 @@ func (c *CAManager) SignCertificate(csr *x509.CertificateRequest, spiffeID conne
 
 	return &reply, nil
 }
+
+func (ca *CAManager) checkExpired(pem string) error {
+	cert, err := connect.ParseCert(pem)
+	if err != nil {
+		return err
+	}
+	if cert.NotAfter.Before(ca.timeNow()) {
+		return fmt.Errorf("certificate expired, expiration date: %s ", cert.NotAfter.String())
+	}
+	return nil
+}

agent/consul/leader_connect_ca_test.go

@@ -1,10 +1,16 @@
 package consul
 
 import (
+	"bytes"
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
 	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
 	"errors"
 	"fmt"
+	"math/big"
 	"testing"
 	"time"
 
@@ -148,8 +154,9 @@ func (m *mockCAServerDelegate) generateCASignRequest(csr string) *structs.CASign
 // mockCAProvider mocks an empty provider implementation with a channel in order to coordinate
 // waiting for certain methods to be called.
 type mockCAProvider struct {
-	callbackCh chan string
+	callbackCh      chan string
-	rootPEM    string
+	rootPEM         string
+	intermediatePem string
 }
 
 func (m *mockCAProvider) Configure(cfg ca.ProviderConfig) error { return nil }
@@ -167,7 +174,10 @@ func (m *mockCAProvider) SetIntermediate(intermediatePEM, rootPEM string) error
 	return nil
 }
 func (m *mockCAProvider) ActiveIntermediate() (string, error) {
-	return m.rootPEM, nil
+	if m.intermediatePem == "" {
+		return m.rootPEM, nil
+	}
+	return m.intermediatePem, nil
 }
 func (m *mockCAProvider) GenerateIntermediate() (string, error)                     { return "", nil }
 func (m *mockCAProvider) Sign(*x509.CertificateRequest) (string, error)             { return "", nil }
@@ -231,9 +241,6 @@ func initTestManager(t *testing.T, manager *CAManager, delegate *mockCAServerDel
 }
 
 func TestCAManager_Initialize(t *testing.T) {
-	if testing.Short() {
-		t.Skip("too slow for testing.Short")
-	}
 
 	conf := DefaultConfig()
 	conf.ConnectEnabled = true
@@ -276,9 +283,6 @@ func TestCAManager_Initialize(t *testing.T) {
 }
 
 func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) {
-	if testing.Short() {
-		t.Skip("too slow for testing.Short")
-	}
 
 	// No parallel execution because we change globals
 	// Set the interval and drift buffer low for renewing the cert.
@@ -303,8 +307,10 @@ func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) {
 	}
 	initTestManager(t, manager, delegate)
 
-	// Wait half the TTL for the cert to need renewing.
+	// Simulate Wait half the TTL for the cert to need renewing.
-	time.Sleep(500 * time.Millisecond)
+	manager.timeNow = func() time.Time {
+		return time.Now().Add(500 * time.Millisecond)
+	}
 
 	// Call RenewIntermediate and then confirm the RPCs and provider calls
 	// happen in the expected order.
@@ -338,6 +344,117 @@ func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) {
 	require.EqualValues(t, caStateInitialized, manager.state)
 }
 
+func TestCAManager_SignLeafWithExpiredCert(t *testing.T) {
+
+	args := []struct {
+		testName              string
+		notBeforeRoot         time.Time
+		notAfterRoot          time.Time
+		notBeforeIntermediate time.Time
+		notAfterIntermediate  time.Time
+		isError               bool
+		errorMsg              string
+	}{
+		{"intermediate valid", time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), false, ""},
+		{"intermediate expired", time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(-2, 0, 0), time.Now().AddDate(0, 0, -1), true, "intermediate expired: certificate expired, expiration date"},
+		{"root expired", time.Now().AddDate(-2, 0, 0), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), true, "root expired: certificate expired, expiration date"},
+		// a cert that is not yet valid is ok, assume it will be valid soon enough
+		{"intermediate in the future", time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(0, 0, 1), time.Now().AddDate(0, 0, 2), false, ""},
+		{"root in the future", time.Now().AddDate(0, 0, 1), time.Now().AddDate(0, 0, 2), time.Now().AddDate(0, 0, -1), time.Now().AddDate(0, 0, 2), false, ""},
+	}
+
+	for _, arg := range args {
+
+		t.Run(arg.testName, func(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
+
+			conf := DefaultConfig()
+			conf.ConnectEnabled = true
+			conf.PrimaryDatacenter = "dc1"
+			conf.Datacenter = "dc2"
+			delegate := NewMockCAServerDelegate(t, conf)
+			manager := NewCAManager(delegate, nil, testutil.Logger(t), conf)
+
+			err, rootPEM := generatePem(arg.notBeforeRoot, arg.notAfterRoot)
+			require.NoError(t, err)
+			err, intermediatePEM := generatePem(arg.notBeforeIntermediate, arg.notAfterIntermediate)
+			require.NoError(t, err)
+			manager.providerShim = &mockCAProvider{
+				callbackCh:      delegate.callbackCh,
+				rootPEM:         rootPEM,
+				intermediatePem: intermediatePEM,
+			}
+			initTestManager(t, manager, delegate)
+
+			// Simulate Wait half the TTL for the cert to need renewing.
+			manager.timeNow = func() time.Time {
+				return time.Now().Add(500 * time.Millisecond)
+			}
+
+			// Call RenewIntermediate and then confirm the RPCs and provider calls
+			// happen in the expected order.
+
+			_, err = manager.SignCertificate(&x509.CertificateRequest{}, &connect.SpiffeIDAgent{})
+
+			if arg.isError {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), arg.errorMsg)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func generatePem(notBefore time.Time, notAfter time.Time) (error, string) {
+	ca := &x509.Certificate{
+		SerialNumber: big.NewInt(2019),
+		Subject: pkix.Name{
+			Organization:  []string{"Company, INC."},
+			Country:       []string{"US"},
+			Province:      []string{""},
+			Locality:      []string{"San Francisco"},
+			StreetAddress: []string{"Golden Gate Bridge"},
+			PostalCode:    []string{"94016"},
+		},
+		NotBefore:             notBefore,
+		NotAfter:              notAfter,
+		IsCA:                  true,
+		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth},
+		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
+		BasicConstraintsValid: true,
+	}
+	caPrivKey, err := rsa.GenerateKey(rand.Reader, 4096)
+	if err != nil {
+		return err, ""
+	}
+	caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey)
+	if err != nil {
+		return err, ""
+	}
+	caPEM := new(bytes.Buffer)
+	pem.Encode(caPEM, &pem.Block{
+		Type:  "CERTIFICATE",
+		Bytes: caBytes,
+	})
+
+	caPrivKeyPEM := new(bytes.Buffer)
+	pem.Encode(caPrivKeyPEM, &pem.Block{
+		Type:  "RSA PRIVATE KEY",
+		Bytes: x509.MarshalPKCS1PrivateKey(caPrivKey),
+	})
+	return err, caPEM.String()
+}
+
 func TestCADelegateWithState_GenerateCASignRequest(t *testing.T) {
 	s := Server{config: &Config{PrimaryDatacenter: "east"}, tokens: new(token.Store)}
 	d := &caDelegateWithState{Server: &s}