From 1f670c22f5bfc3c3e03d8a5a768e073814c0a5e9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 23 Nov 2021 14:15:08 -0500 Subject: [PATCH] ca: remove one call to provider.ActiveRoot ActiveRoot should not be called from the secondary DC, because there should not be a requirement to run the same Vault instance in a secondary DC. SignIntermediate is called in a secondary DC, so it should not call ActiveRoot We would also like to change the interface of ActiveRoot so that we can support using an intermediate cert as the primary CA in Consul. In preparation for making that change I am reducing the number of calls to ActiveRoot, so that there are fewer code paths to modify when the interface changes. This change required a change to the mockCAServerDelegate we use in tests. It was returning the RootCert for SignIntermediate, but that is not an accurate fake of production. In production this would also be a separate cert. --- agent/consul/leader_connect_ca.go | 22 ++++++++-------------- agent/consul/leader_connect_ca_test.go | 24 +++++++++++++++--------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 3087fb84a..0a6b0b47a 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -1463,28 +1463,22 @@ 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) + // TODO: we store NotBefore and NotAfter on this struct, so we could avoid + // parsing the cert here. + err = c.checkExpired(caRoot.RootCert) 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) + if c.isIntermediateUsedToSignLeaf() && len(caRoot.IntermediateCerts) > 0 { + inter := caRoot.IntermediateCerts[len(caRoot.IntermediateCerts)-1] + if err := c.checkExpired(inter); err != nil { + return nil, fmt.Errorf("intermediate expired: %w", err) + } } // All seems to be in order, actually sign it. - pem, err := provider.Sign(csr) if err == ca.ErrRateLimited { return nil, ErrRateLimited diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 869459cb7..5e5e1de59 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -12,6 +12,7 @@ import ( "fmt" "math/big" "net/rpc" + "net/url" "testing" "time" @@ -131,11 +132,12 @@ func verifyLeafCert(t *testing.T, root *structs.CARoot, leafCertPEM string) { } type mockCAServerDelegate struct { - t *testing.T - config *Config - store *state.Store - primaryRoot *structs.CARoot - callbackCh chan string + t *testing.T + config *Config + store *state.Store + primaryRoot *structs.CARoot + secondaryIntermediate string + callbackCh chan string } func NewMockCAServerDelegate(t *testing.T, config *Config) *mockCAServerDelegate { @@ -198,7 +200,7 @@ func (m *mockCAServerDelegate) forwardDC(method, dc string, args interface{}, re roots.ActiveRootID = m.primaryRoot.ID case "ConnectCA.SignIntermediate": r := reply.(*string) - *r = m.primaryRoot.RootCert + *r = m.secondaryIntermediate default: return fmt.Errorf("received call to unsupported method %q", method) } @@ -305,13 +307,14 @@ func initTestManager(t *testing.T, manager *CAManager, delegate *mockCAServerDel } func TestCAManager_Initialize(t *testing.T) { - conf := DefaultConfig() conf.ConnectEnabled = true conf.PrimaryDatacenter = "dc1" conf.Datacenter = "dc2" delegate := NewMockCAServerDelegate(t, conf) + delegate.secondaryIntermediate = delegate.primaryRoot.RootCert manager := NewCAManager(delegate, nil, testutil.Logger(t), conf) + manager.providerShim = &mockCAProvider{ callbackCh: delegate.callbackCh, rootPEM: delegate.primaryRoot.RootCert, @@ -356,6 +359,7 @@ func TestCAManager_UpdateConfigWhileRenewIntermediate(t *testing.T) { conf.PrimaryDatacenter = "dc1" conf.Datacenter = "dc2" delegate := NewMockCAServerDelegate(t, conf) + delegate.secondaryIntermediate = delegate.primaryRoot.RootCert manager := NewCAManager(delegate, nil, testutil.Logger(t), conf) manager.providerShim = &mockCAProvider{ callbackCh: delegate.callbackCh, @@ -447,6 +451,8 @@ func TestCAManager_SignCertificate_WithExpiredCert(t *testing.T) { intermediatePEM := generateCertPEM(t, caPrivKey, arg.notBeforeIntermediate, arg.notAfterIntermediate) delegate := NewMockCAServerDelegate(t, conf) + delegate.primaryRoot.RootCert = rootPEM + delegate.secondaryIntermediate = intermediatePEM manager := NewCAManager(delegate, nil, testutil.Logger(t), conf) manager.providerShim = &mockCAProvider{ @@ -458,14 +464,13 @@ func TestCAManager_SignCertificate_WithExpiredCert(t *testing.T) { // Simulate Wait half the TTL for the cert to need renewing. manager.timeNow = func() time.Time { - return time.Now().Add(500 * time.Millisecond) + return time.Now().UTC().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) @@ -494,6 +499,7 @@ func generateCertPEM(t *testing.T, caPrivKey *rsa.PrivateKey, notBefore time.Tim ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, BasicConstraintsValid: true, + URIs: []*url.URL{connect.SpiffeIDAgent{Host: "foo"}.URI()}, } caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caPrivKey.PublicKey, caPrivKey)