luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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>
This commit is contained in:
Dhia Ayachi 2021-07-13 12:15:06 -04:00 committed by GitHub
parent ae8b526be8
commit 53b45a8441
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 170 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/10500.txt Normal file
View File

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

1
agent/connect/ca/provider_aws.go
View File

@ -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"

49
agent/consul/leader_connect_ca.go
View File

@ -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
}

133
agent/consul/leader_connect_ca_test.go
View File

@ -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"
@ -150,6 +156,7 @@ func (m *mockCAServerDelegate) generateCASignRequest(csr string) *structs.CASign
type mockCAProvider struct {
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) {
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}