diff --git a/.changelog/10500.txt b/.changelog/10500.txt new file mode 100644 index 0000000000..ee19666c53 --- /dev/null +++ b/.changelog/10500.txt @@ -0,0 +1,3 @@ +```release-note:bug +check root and intermediate CA expiry before using it to sign a leaf certificate. +``` diff --git a/agent/connect/ca/provider_aws.go b/agent/connect/ca/provider_aws.go index e03f35984d..92d19267cf 100644 --- a/agent/connect/ca/provider_aws.go +++ b/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" diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index fe510c72fe..51d157a61b 100644 --- a/agent/consul/leader_connect_ca.go +++ b/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 +} diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 4ced2c0f23..3bdb1f3a52 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/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 - rootPEM string + callbackCh chan 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. - time.Sleep(500 * time.Millisecond) + // 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. @@ -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}