From 6bf8241e298fde977d0b7ffaa2676780b739afa1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 8 Dec 2021 17:49:00 -0500 Subject: [PATCH 1/4] Merge pull request #11721 from hashicorp/dnephin/ca-export-fsm-operation ca: use the real FSM operation in tests Fixed conflicts in tests. --- agent/connect/ca/provider_consul_test.go | 18 +++++++++++---- agent/connect/ca/testing.go | 28 ------------------------ agent/consul/fsm/commands_oss.go | 26 ++++++++++++++-------- agent/consul/leader_connect_ca_test.go | 22 +++++++++++++++---- 4 files changed, 49 insertions(+), 45 deletions(-) diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index 43d5b2fcb013..a9b8bd50d556 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/agent/consul/fsm" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/structs" ) @@ -22,7 +23,16 @@ func (c *consulCAMockDelegate) State() *state.Store { } func (c *consulCAMockDelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) { - return ApplyCARequestToStore(c.state, req) + idx, _, err := c.state.CAConfig(nil) + if err != nil { + return nil, err + } + + result := fsm.ApplyConnectCAOperationFromRequest(c.state, req, idx+1) + if err, ok := result.(error); ok && err != nil { + return nil, err + } + return result, nil } func newMockDelegate(t *testing.T, conf *structs.CAConfiguration) *consulCAMockDelegate { @@ -150,7 +160,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(err) require.Equal(spiffeService.URI(), parsed.URIs[0]) require.Equal(connect.ServiceCN("foo", "default", connect.TestClusterID), parsed.Subject.CommonName) - require.Equal(uint64(2), parsed.SerialNumber.Uint64()) + require.Equal(uint64(3), parsed.SerialNumber.Uint64()) subjectKeyID, err := connect.KeyId(csr.PublicKey) require.NoError(err) require.Equal(subjectKeyID, parsed.SubjectKeyId) @@ -179,7 +189,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(err) require.Equal(spiffeService.URI(), parsed.URIs[0]) require.Equal(connect.ServiceCN("bar", "default", connect.TestClusterID), parsed.Subject.CommonName) - require.Equal(parsed.SerialNumber.Uint64(), uint64(2)) + require.Equal(uint64(4), parsed.SerialNumber.Uint64()) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) @@ -207,7 +217,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) { require.NoError(err) require.Equal(spiffeAgent.URI(), parsed.URIs[0]) require.Equal(connect.AgentCN("uuid", connect.TestClusterID), parsed.Subject.CommonName) - require.Equal(uint64(2), parsed.SerialNumber.Uint64()) + require.Equal(uint64(5), parsed.SerialNumber.Uint64()) requireNotEncoded(t, parsed.SubjectKeyId) requireNotEncoded(t, parsed.AuthorityKeyId) diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index 7b79f62f051a..e977f6d5c0d4 100644 --- a/agent/connect/ca/testing.go +++ b/agent/connect/ca/testing.go @@ -8,8 +8,6 @@ import ( "sync" "github.com/hashicorp/consul/agent/connect" - "github.com/hashicorp/consul/agent/consul/state" - "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/freeport" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/go-hclog" @@ -237,32 +235,6 @@ func (v *TestVaultServer) Stop() error { return nil } -func ApplyCARequestToStore(store *state.Store, req *structs.CARequest) (interface{}, error) { - idx, _, err := store.CAConfig(nil) - if err != nil { - return nil, err - } - - switch req.Op { - case structs.CAOpSetProviderState: - _, err := store.CASetProviderState(idx+1, req.ProviderState) - if err != nil { - return nil, err - } - - return true, nil - case structs.CAOpDeleteProviderState: - if err := store.CADeleteProviderState(idx+1, req.ProviderState.ID); err != nil { - return nil, err - } - - return true, nil - case structs.CAOpIncrementProviderSerialNumber: - return uint64(2), nil - default: - return nil, fmt.Errorf("Invalid CA operation '%s'", req.Op) - } -} func requireTrailingNewline(t testing.T, leafPEM string) { t.Helper() if len(leafPEM) == 0 { diff --git a/agent/consul/fsm/commands_oss.go b/agent/consul/fsm/commands_oss.go index 16e0d7172919..533dab6ebe78 100644 --- a/agent/consul/fsm/commands_oss.go +++ b/agent/consul/fsm/commands_oss.go @@ -420,10 +420,19 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} { []metrics.Label{{Name: "op", Value: string(req.Op)}}) defer metrics.MeasureSinceWithLabels([]string{"fsm", "ca"}, time.Now(), []metrics.Label{{Name: "op", Value: string(req.Op)}}) + + result := ApplyConnectCAOperationFromRequest(c.state, &req, index) + if err, ok := result.(error); ok && err != nil { + c.logger.Warn("Failed to apply CA operation", "operation", req.Op) + } + return result +} + +func ApplyConnectCAOperationFromRequest(state *state.Store, req *structs.CARequest, index uint64) interface{} { switch req.Op { case structs.CAOpSetConfig: if req.Config.ModifyIndex != 0 { - act, err := c.state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) + act, err := state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) if err != nil { return err } @@ -431,29 +440,29 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} { return act } - return c.state.CASetConfig(index, req.Config) + return state.CASetConfig(index, req.Config) case structs.CAOpSetRoots: - act, err := c.state.CARootSetCAS(index, req.Index, req.Roots) + act, err := state.CARootSetCAS(index, req.Index, req.Roots) if err != nil { return err } return act case structs.CAOpSetProviderState: - act, err := c.state.CASetProviderState(index, req.ProviderState) + act, err := state.CASetProviderState(index, req.ProviderState) if err != nil { return err } return act case structs.CAOpDeleteProviderState: - if err := c.state.CADeleteProviderState(index, req.ProviderState.ID); err != nil { + if err := state.CADeleteProviderState(index, req.ProviderState.ID); err != nil { return err } return true case structs.CAOpSetRootsAndConfig: - act, err := c.state.CARootSetCAS(index, req.Index, req.Roots) + act, err := state.CARootSetCAS(index, req.Index, req.Roots) if err != nil { return err } @@ -461,20 +470,19 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} { return act } - act, err = c.state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) + act, err = state.CACheckAndSetConfig(index, req.Config.ModifyIndex, req.Config) if err != nil { return err } return act case structs.CAOpIncrementProviderSerialNumber: - sn, err := c.state.CAIncrementProviderSerialNumber(index) + sn, err := state.CAIncrementProviderSerialNumber(index) if err != nil { return err } return sn default: - c.logger.Warn("Invalid CA operation", "operation", req.Op) return fmt.Errorf("Invalid CA operation '%s'", req.Op) } } diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 90c1077496a7..ebf9fbaa58c9 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/consul/agent/connect" ca "github.com/hashicorp/consul/agent/connect/ca" + "github.com/hashicorp/consul/agent/consul/fsm" "github.com/hashicorp/consul/agent/consul/state" "github.com/hashicorp/consul/agent/metadata" "github.com/hashicorp/consul/agent/structs" @@ -63,10 +64,6 @@ func (m *mockCAServerDelegate) CheckServers(datacenter string, fn func(*metadata }) } -func (m *mockCAServerDelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) { - return ca.ApplyCARequestToStore(m.store, req) -} - func (m *mockCAServerDelegate) createCAProvider(conf *structs.CAConfiguration) (ca.Provider, error) { return &mockCAProvider{ callbackCh: m.callbackCh, @@ -74,6 +71,23 @@ func (m *mockCAServerDelegate) createCAProvider(conf *structs.CAConfiguration) ( }, nil } +// ApplyCARequest mirrors FSM.applyConnectCAOperation because that functionality +// is not exported. +func (m *mockCAServerDelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) { + idx, _, err := m.store.CAConfig(nil) + if err != nil { + return nil, err + } + + m.callbackCh <- fmt.Sprintf("raftApply/ConnectCA") + + result := fsm.ApplyConnectCAOperationFromRequest(m.store, req, idx+1) + if err, ok := result.(error); ok && err != nil { + return nil, err + } + return result, nil +} + func (m *mockCAServerDelegate) forwardDC(method, dc string, args interface{}, reply interface{}) error { switch method { case "ConnectCA.Roots": From 17b791a7b1bd57bcca40bd99e2a002cb672d61bf Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 9 Dec 2021 12:29:43 -0500 Subject: [PATCH 2/4] Merge pull request #11780 from hashicorp/dnephin/ca-test-vault-in-secondary ca: improve test coverage for RenewIntermediate --- agent/connect/ca/testing.go | 14 +- agent/consul/connect_ca_endpoint_test.go | 4 +- agent/consul/leader_connect_ca.go | 23 ++- agent/consul/leader_connect_ca_test.go | 184 +++++++++++++++++- agent/consul/leader_connect_test.go | 237 +++++++---------------- agent/consul/state/connect_ca.go | 12 +- agent/structs/connect_ca.go | 23 +++ testrpc/wait.go | 11 +- 8 files changed, 300 insertions(+), 208 deletions(-) diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index e977f6d5c0d4..adba48388fa7 100644 --- a/agent/connect/ca/testing.go +++ b/agent/connect/ca/testing.go @@ -1,18 +1,20 @@ package ca import ( + "errors" "fmt" "io/ioutil" "os" "os/exec" "sync" - "github.com/hashicorp/consul/agent/connect" - "github.com/hashicorp/consul/sdk/freeport" - "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/go-hclog" vaultapi "github.com/hashicorp/vault/api" "github.com/mitchellh/go-testing-interface" + + "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/sdk/freeport" + "github.com/hashicorp/consul/sdk/testutil/retry" ) // KeyTestCases is a list of the important CA key types that we should test @@ -169,7 +171,9 @@ func runTestVault(t testing.T) (*TestVaultServer, error) { returnPortsFn: returnPortsFn, } t.Cleanup(func() { - testVault.Stop() + if err := testVault.Stop(); err != nil { + t.Logf("failed to stop vault server: %v", err) + } }) return testVault, nil } @@ -217,7 +221,7 @@ func (v *TestVaultServer) Stop() error { } if v.cmd.Process != nil { - if err := v.cmd.Process.Signal(os.Interrupt); err != nil { + if err := v.cmd.Process.Signal(os.Interrupt); err != nil && !errors.Is(err, os.ErrProcessDone) { return fmt.Errorf("failed to kill vault server: %v", err) } } diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 4cfe22ce452a..c57d76b01e32 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -527,8 +527,8 @@ func TestConnectCAConfig_UpdateSecondary(t *testing.T) { require.Len(rootList.Roots, 1) rootCert := activeRoot - waitForActiveCARoot(t, s1, rootCert) - waitForActiveCARoot(t, s2, rootCert) + testrpc.WaitForActiveCARoot(t, s1.RPC, "primary", rootCert) + testrpc.WaitForActiveCARoot(t, s2.RPC, "secondary", rootCert) // Capture the current intermediate rootList, activeRoot, err = getTestRoots(s2, "secondary") diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index bd7fec5d9d95..f732ef3b6b38 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -286,7 +286,7 @@ func (c *CAManager) startPostInitializeRoutines() { c.leaderRoutineManager.Start(secondaryCARootWatchRoutineName, c.secondaryCARootWatch) } - c.leaderRoutineManager.Start(intermediateCertRenewWatchRoutineName, c.intermediateCertRenewalWatch) + c.leaderRoutineManager.Start(intermediateCertRenewWatchRoutineName, c.runRenewIntermediate) } func (c *CAManager) backgroundCAInitialization(ctx context.Context) error { @@ -1051,8 +1051,22 @@ func (c *CAManager) getIntermediateCASigned(provider ca.Provider, newActiveRoot return nil } -// intermediateCertRenewalWatch periodically attempts to renew the intermediate cert. -func (c *CAManager) intermediateCertRenewalWatch(ctx context.Context) error { +// setLeafSigningCert updates the CARoot by appending the pem to the list of +// intermediate certificates, and setting the SigningKeyID to the encoded +// SubjectKeyId of the certificate. +func setLeafSigningCert(caRoot *structs.CARoot, pem string) error { + cert, err := connect.ParseCert(pem) + if err != nil { + return fmt.Errorf("error parsing leaf signing cert: %w", err) + } + + caRoot.IntermediateCerts = append(caRoot.IntermediateCerts, pem) + caRoot.SigningKeyID = connect.EncodeSigningKeyID(cert.SubjectKeyId) + return nil +} + +// runRenewIntermediate periodically attempts to renew the intermediate cert. +func (c *CAManager) runRenewIntermediate(ctx context.Context) error { isPrimary := c.serverConf.Datacenter == c.serverConf.PrimaryDatacenter for { @@ -1120,8 +1134,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), - intermediateCert.NotAfter) { + if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore, intermediateCert.NotAfter) { return nil } diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index ebf9fbaa58c9..df3fdf90c8de 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -6,10 +6,12 @@ import ( "crypto/x509" "errors" "fmt" + "net/rpc" "testing" "time" "github.com/hashicorp/go-version" + msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" "github.com/hashicorp/serf/serf" "github.com/stretchr/testify/require" @@ -27,6 +29,103 @@ import ( // TODO(kyhavlov): replace with t.Deadline() const CATestTimeout = 7 * time.Second +func TestCAManager_Initialize_Vault_Secondary_SharedVault(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + ca.SkipIfVaultNotPresent(t) + + vault := ca.NewTestVaultServer(t) + + _, serverDC1 := testServerWithConfig(t, func(c *Config) { + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-primary/", + }, + } + }) + + runStep(t, "check primary DC", func(t *testing.T) { + testrpc.WaitForTestAgent(t, serverDC1.RPC, "dc1") + + codec := rpcClient(t, serverDC1) + roots := structs.IndexedCARoots{} + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(t, err) + require.Len(t, roots.Roots, 1) + + leafPEM := getLeafCert(t, codec, roots.TrustDomain, "dc1") + verifyLeafCert(t, roots.Roots[0], leafPEM) + }) + + runStep(t, "start secondary DC", func(t *testing.T) { + _, serverDC2 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "dc2" + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-secondary/", + }, + } + }) + defer serverDC2.Shutdown() + joinWAN(t, serverDC2, serverDC1) + testrpc.WaitForActiveCARoot(t, serverDC2.RPC, "dc2", nil) + + codec := rpcClient(t, serverDC2) + roots := structs.IndexedCARoots{} + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(t, err) + require.Len(t, roots.Roots, 1) + + leafPEM := getLeafCert(t, codec, roots.TrustDomain, "dc2") + verifyLeafCert(t, roots.Roots[0], leafPEM) + }) +} + +func verifyLeafCert(t *testing.T, root *structs.CARoot, leafCertPEM string) { + t.Helper() + leaf, intermediates, err := connect.ParseLeafCerts(leafCertPEM) + require.NoError(t, err) + + pool := x509.NewCertPool() + ok := pool.AppendCertsFromPEM([]byte(root.RootCert)) + if !ok { + t.Fatalf("Failed to add root CA PEM to cert pool") + } + + // verify with intermediates from leaf CertPEM + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: pool, + Intermediates: intermediates, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + require.NoError(t, err, "failed to verify using intermediates from leaf cert PEM") + + // verify with intermediates from the CARoot + intermediates = x509.NewCertPool() + for _, intermediate := range root.IntermediateCerts { + c, err := connect.ParseCert(intermediate) + require.NoError(t, err) + intermediates.AddCert(c) + } + + _, err = leaf.Verify(x509.VerifyOptions{ + Roots: pool, + Intermediates: intermediates, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }) + require.NoError(t, err, "failed to verify using intermediates from CARoot list") +} + type mockCAServerDelegate struct { t *testing.T config *Config @@ -251,15 +350,7 @@ func TestCAManager_Initialize(t *testing.T) { func TestCAManager_UpdateConfigWhileRenewIntermediate(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 + patchIntermediateCertRenewInterval(t) conf := DefaultConfig() conf.ConnectEnabled = true @@ -334,3 +425,78 @@ func TestCAManager_Initialize_Logging(t *testing.T) { require.Contains(t, buf.String(), "consul CA provider configured") } + +func TestCAManager_UpdateConfiguration_Vault_Primary(t *testing.T) { + ca.SkipIfVaultNotPresent(t) + vault := ca.NewTestVaultServer(t) + + _, s1 := testServerWithConfig(t, func(c *Config) { + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + }, + } + }) + defer func() { + s1.Shutdown() + }() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + _, origRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, origRoot.IntermediateCerts, 1) + + cert, err := connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(origRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), origRoot.SigningKeyID) + + err = s1.caManager.UpdateConfiguration(&structs.CARequest{ + Config: &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root-2/", + "IntermediatePKIPath": "pki-intermediate-2/", + }, + }, + }) + require.NoError(t, err) + + _, newRoot, err := s1.fsm.State().CARootActive(nil) + require.NoError(t, err) + require.Len(t, newRoot.IntermediateCerts, 2, + "expected one cross-sign cert and one local leaf sign cert") + require.NotEqual(t, origRoot.ID, newRoot.ID) + + cert, err = connect.ParseCert(s1.caManager.getLeafSigningCertFromRoot(newRoot)) + require.NoError(t, err) + require.Equal(t, connect.HexString(cert.SubjectKeyId), newRoot.SigningKeyID) +} + +func getLeafCert(t *testing.T, codec rpc.ClientCodec, trustDomain string, dc string) string { + pk, _, err := connect.GeneratePrivateKey() + require.NoError(t, err) + spiffeID := &connect.SpiffeIDService{ + Host: trustDomain, + Service: "srv1", + Datacenter: dc, + } + cn, err := connect.CNForCertURI(spiffeID) + require.NoError(t, err) + csr, err := connect.CreateCSR(spiffeID, cn, pk, nil, nil) + require.NoError(t, err) + + req := structs.CASignRequest{CSR: csr} + cert := structs.IssuedCert{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) + require.NoError(t, err) + + return cert.CertPEM +} diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 046396241106..ade371f6f42c 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -167,18 +167,6 @@ func TestCAManager_Initialize_Secondary(t *testing.T) { } } -func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) { - retry.Run(t, func(r *retry.R) { - _, root := getCAProviderWithLock(srv) - if root == nil { - r.Fatal("no root") - } - if root.ID != expect.ID { - r.Fatalf("current active root is %s; waiting for %s", root.ID, expect.ID) - } - }) -} - func getCAProviderWithLock(s *Server) (ca.Provider, *structs.CARoot) { return s.caManager.getCAProvider() } @@ -191,26 +179,12 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { ca.SkipIfVaultNotPresent(t) // no parallel execution because we change globals - origInterval := structs.IntermediateCertRenewInterval - origMinTTL := structs.MinLeafCertTTL - origDriftBuffer := ca.CertificateTimeDriftBuffer - defer func() { - structs.IntermediateCertRenewInterval = origInterval - structs.MinLeafCertTTL = origMinTTL - ca.CertificateTimeDriftBuffer = origDriftBuffer - }() - - // Vault backdates certs by 30s by default. - ca.CertificateTimeDriftBuffer = 30 * time.Second - structs.IntermediateCertRenewInterval = time.Millisecond - structs.MinLeafCertTTL = time.Second + patchIntermediateCertRenewInterval(t) require := require.New(t) testVault := ca.NewTestVaultServer(t) - defer testVault.Stop() - dir1, s1 := testServerWithConfig(t, func(c *Config) { - c.Build = "1.6.0" + _, s1 := testServerWithConfig(t, func(c *Config) { c.PrimaryDatacenter = "dc1" c.CAConfig = &structs.CAConfiguration{ Provider: "vault", @@ -219,49 +193,34 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { "Token": testVault.RootToken, "RootPKIPath": "pki-root/", "IntermediatePKIPath": "pki-intermediate/", - "LeafCertTTL": "1s", - // The retry loop only retries for 7sec max and - // the ttl needs to be below so that it - // triggers definitely. - "IntermediateCertTTL": "5s", + "LeafCertTTL": "2s", + "IntermediateCertTTL": "7s", }, } }) - defer os.RemoveAll(dir1) defer s1.Shutdown() - testrpc.WaitForLeader(t, s1.RPC, "dc1") + testrpc.WaitForActiveCARoot(t, s1.RPC, "dc1", nil) - // Capture the current root. - var originalRoot *structs.CARoot - { - rootList, activeRoot, err := getTestRoots(s1, "dc1") - require.NoError(err) - require.Len(rootList.Roots, 1) - originalRoot = activeRoot - } - - // Get the original intermediate. - waitForActiveCARoot(t, s1, originalRoot) - provider, _ := getCAProviderWithLock(s1) - intermediatePEM, err := provider.ActiveIntermediate() + store := s1.caManager.delegate.State() + _, activeRoot, err := store.CARootActive(nil) require.NoError(err) + t.Log("original SigningKeyID", activeRoot.SigningKeyID) + + intermediatePEM := s1.caManager.getLeafSigningCertFromRoot(activeRoot) intermediateCert, err := connect.ParseCert(intermediatePEM) require.NoError(err) - // Check that the state store has the correct intermediate - store := s1.caManager.delegate.State() - _, activeRoot, err := store.CARootActive(nil) - require.NoError(err) - require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) // Wait for dc1's intermediate to be refreshed. - // It is possible that test fails when the blocking query doesn't return. retry.Run(t, func(r *retry.R) { - provider, _ = getCAProviderWithLock(s1) - newIntermediatePEM, err := provider.ActiveIntermediate() + store := s1.caManager.delegate.State() + _, storedRoot, err := store.CARootActive(nil) r.Check(err) + + newIntermediatePEM := s1.caManager.getLeafSigningCertFromRoot(storedRoot) if newIntermediatePEM == intermediatePEM { r.Fatal("not a renewed intermediate") } @@ -270,47 +229,43 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) { intermediatePEM = newIntermediatePEM }) - _, activeRoot, err = store.CARootActive(nil) + codec := rpcClient(t, s1) + roots := structs.IndexedCARoots{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) require.NoError(err) - require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) - require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + require.Len(roots.Roots, 1) - // Get the root from dc1 and validate a chain of: - // dc1 leaf -> dc1 intermediate -> dc1 root - provider, caRoot := getCAProviderWithLock(s1) + activeRoot = roots.Active() + require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) + require.Equal(intermediatePEM, s1.caManager.getLeafSigningCertFromRoot(activeRoot)) // Have the new intermediate sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: roots.TrustDomain, Namespace: "default", Datacenter: "dc1", Service: "foo", } - raw, _ := connect.TestCSR(t, spiffeService) - - leafCsr, err := connect.ParseCSR(raw) - require.NoError(err) + csr, _ := connect.TestCSR(t, spiffeService) - leafPEM, err := provider.Sign(leafCsr) + req := structs.CASignRequest{CSR: csr} + cert := structs.IssuedCert{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) require.NoError(err) + verifyLeafCert(t, activeRoot, cert.CertPEM) +} - cert, err := connect.ParseCert(leafPEM) - require.NoError(err) +func patchIntermediateCertRenewInterval(t *testing.T) { + origInterval := structs.IntermediateCertRenewInterval + origMinTTL := structs.MinLeafCertTTL - // Check that the leaf signed by the new intermediate can be verified using the - // returned cert chain (signed intermediate + remote root). - intermediatePool := x509.NewCertPool() - // TODO: do not explicitly add the intermediatePEM, we should have it available - // from leafPEM. Use connect.ParseLeafCerts to do the right thing. - intermediatePool.AppendCertsFromPEM([]byte(intermediatePEM)) - rootPool := x509.NewCertPool() - rootPool.AppendCertsFromPEM([]byte(caRoot.RootCert)) + structs.IntermediateCertRenewInterval = 200 * time.Millisecond + structs.MinLeafCertTTL = time.Second - _, err = cert.Verify(x509.VerifyOptions{ - Intermediates: intermediatePool, - Roots: rootPool, + t.Cleanup(func() { + structs.IntermediateCertRenewInterval = origInterval + structs.MinLeafCertTTL = origMinTTL }) - require.NoError(err) } func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { @@ -319,18 +274,10 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { } // no parallel execution because we change globals - origInterval := structs.IntermediateCertRenewInterval - origMinTTL := structs.MinLeafCertTTL - defer func() { - structs.IntermediateCertRenewInterval = origInterval - structs.MinLeafCertTTL = origMinTTL - }() - - structs.IntermediateCertRenewInterval = time.Millisecond - structs.MinLeafCertTTL = time.Second + patchIntermediateCertRenewInterval(t) require := require.New(t) - dir1, s1 := testServerWithConfig(t, func(c *Config) { + _, s1 := testServerWithConfig(t, func(c *Config) { c.Build = "1.6.0" c.CAConfig = &structs.CAConfiguration{ Provider: "consul", @@ -350,112 +297,73 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { }, } }) - defer os.RemoveAll(dir1) defer s1.Shutdown() testrpc.WaitForLeader(t, s1.RPC, "dc1") // dc2 as a secondary DC - dir2, s2 := testServerWithConfig(t, func(c *Config) { + _, s2 := testServerWithConfig(t, func(c *Config) { c.Datacenter = "dc2" c.PrimaryDatacenter = "dc1" - c.Build = "1.6.0" }) - defer os.RemoveAll(dir2) defer s2.Shutdown() // Create the WAN link joinWAN(t, s2, s1) - testrpc.WaitForLeader(t, s2.RPC, "dc2") + testrpc.WaitForActiveCARoot(t, s2.RPC, "dc2", nil) - // Get the original intermediate - // TODO: Wait for intermediate instead of wait for leader - secondaryProvider, _ := getCAProviderWithLock(s2) - intermediatePEM, err := secondaryProvider.ActiveIntermediate() + store := s2.fsm.State() + _, activeRoot, err := store.CARootActive(nil) require.NoError(err) + t.Log("original SigningKeyID", activeRoot.SigningKeyID) + + intermediatePEM := s2.caManager.getLeafSigningCertFromRoot(activeRoot) intermediateCert, err := connect.ParseCert(intermediatePEM) require.NoError(err) - currentCertSerialNumber := intermediateCert.SerialNumber - currentCertAuthorityKeyId := intermediateCert.AuthorityKeyId - // Capture the current root - var originalRoot *structs.CARoot - { - rootList, activeRoot, err := getTestRoots(s1, "dc1") - require.NoError(err) - require.Len(rootList.Roots, 1) - originalRoot = activeRoot - } - - waitForActiveCARoot(t, s1, originalRoot) - waitForActiveCARoot(t, s2, originalRoot) - - store := s2.fsm.State() - _, activeRoot, err := store.CARootActive(nil) - require.NoError(err) require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) // Wait for dc2's intermediate to be refreshed. - // It is possible that test fails when the blocking query doesn't return. - // When https://github.com/hashicorp/consul/pull/3777 is merged - // however, defaultQueryTime will be configurable and we con lower it - // so that it returns for sure. retry.Run(t, func(r *retry.R) { - secondaryProvider, _ = getCAProviderWithLock(s2) - intermediatePEM, err = secondaryProvider.ActiveIntermediate() - r.Check(err) - cert, err := connect.ParseCert(intermediatePEM) + store := s2.caManager.delegate.State() + _, storedRoot, err := store.CARootActive(nil) r.Check(err) - if cert.SerialNumber.Cmp(currentCertSerialNumber) == 0 || !reflect.DeepEqual(cert.AuthorityKeyId, currentCertAuthorityKeyId) { - currentCertSerialNumber = cert.SerialNumber - currentCertAuthorityKeyId = cert.AuthorityKeyId + + newIntermediatePEM := s2.caManager.getLeafSigningCertFromRoot(storedRoot) + if newIntermediatePEM == intermediatePEM { r.Fatal("not a renewed intermediate") } - intermediateCert = cert + intermediateCert, err = connect.ParseCert(newIntermediatePEM) + r.Check(err) + intermediatePEM = newIntermediatePEM }) + codec := rpcClient(t, s2) + roots := structs.IndexedCARoots{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(err) + require.Len(roots.Roots, 1) + _, activeRoot, err = store.CARootActive(nil) require.NoError(err) - require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) require.Equal(connect.HexString(intermediateCert.SubjectKeyId), activeRoot.SigningKeyID) - - // Get the root from dc1 and validate a chain of: - // dc2 leaf -> dc2 intermediate -> dc1 root - _, caRoot := getCAProviderWithLock(s1) + require.Equal(intermediatePEM, s2.caManager.getLeafSigningCertFromRoot(activeRoot)) // Have dc2 sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ - Host: "node1", + Host: roots.TrustDomain, Namespace: "default", - Datacenter: "dc1", + Datacenter: "dc2", Service: "foo", } - raw, _ := connect.TestCSR(t, spiffeService) - - leafCsr, err := connect.ParseCSR(raw) - require.NoError(err) - - leafPEM, err := secondaryProvider.Sign(leafCsr) - require.NoError(err) + csr, _ := connect.TestCSR(t, spiffeService) - intermediateCert, err = connect.ParseCert(leafPEM) - require.NoError(err) - - // Check that the leaf signed by the new intermediate can be verified using the - // returned cert chain (signed intermediate + remote root). - intermediatePool := x509.NewCertPool() - // TODO: do not explicitly add the intermediatePEM, we should have it available - // from leafPEM. Use connect.ParseLeafCerts to do the right thing. - intermediatePool.AppendCertsFromPEM([]byte(intermediatePEM)) - rootPool := x509.NewCertPool() - rootPool.AppendCertsFromPEM([]byte(caRoot.RootCert)) - - _, err = intermediateCert.Verify(x509.VerifyOptions{ - Intermediates: intermediatePool, - Roots: rootPool, - }) + req := structs.CASignRequest{CSR: csr} + cert := structs.IssuedCert{} + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) require.NoError(err) + verifyLeafCert(t, activeRoot, cert.CertPEM) } func TestConnectCA_ConfigurationSet_RootRotation_Secondary(t *testing.T) { @@ -1036,14 +944,7 @@ func getTestRoots(s *Server, datacenter string) (*structs.IndexedCARoots, *struc return nil, nil, err } - var active *structs.CARoot - for _, root := range rootList.Roots { - if root.Active { - active = root - break - } - } - + active := rootList.Active() return &rootList, active, nil } diff --git a/agent/consul/state/connect_ca.go b/agent/consul/state/connect_ca.go index ea33b687f28a..8a6172d4d3bd 100644 --- a/agent/consul/state/connect_ca.go +++ b/agent/consul/state/connect_ca.go @@ -260,18 +260,8 @@ func caRootsTxn(tx ReadTxn, ws memdb.WatchSet) (uint64, structs.CARoots, error) func (s *Store) CARootActive(ws memdb.WatchSet) (uint64, *structs.CARoot, error) { // Get all the roots since there should never be that many and just // do the filtering in this method. - var result *structs.CARoot idx, roots, err := s.CARoots(ws) - if err == nil { - for _, r := range roots { - if r.Active { - result = r - break - } - } - } - - return idx, result, err + return idx, roots.Active(), err } // CARootSetCAS sets the current CA root state using a check-and-set operation. diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 5e9c8880d994..26d1c1e289f0 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -54,6 +54,15 @@ type IndexedCARoots struct { QueryMeta `json:"-"` } +func (r IndexedCARoots) Active() *CARoot { + for _, root := range r.Roots { + if root.ID == r.ActiveRootID { + return root + } + } + return nil +} + // CARoot represents a root CA certificate that is trusted. type CARoot struct { // ID is a globally unique ID (UUID) representing this CA root. @@ -144,6 +153,20 @@ func (c *CARoot) Clone() *CARoot { // CARoots is a list of CARoot structures. type CARoots []*CARoot +// Active returns the single CARoot that is marked as active, or nil if there +// is no active root (ex: when they are no roots). +func (c CARoots) Active() *CARoot { + if c == nil { + return nil + } + for _, r := range c { + if r.Active { + return r + } + } + return nil +} + // CASignRequest is the request for signing a service certificate. type CASignRequest struct { // Datacenter is the target for this request. diff --git a/testrpc/wait.go b/testrpc/wait.go index c593f66062ce..34f538e8d3c3 100644 --- a/testrpc/wait.go +++ b/testrpc/wait.go @@ -3,9 +3,10 @@ package testrpc import ( "testing" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/testutil/retry" - "github.com/stretchr/testify/require" ) type rpcFn func(string, interface{}, interface{}) error @@ -152,13 +153,7 @@ func WaitForActiveCARoot(t *testing.T, rpc rpcFn, dc string, expect *structs.CAR r.Fatalf("err: %v", err) } - var root *structs.CARoot - for _, r := range reply.Roots { - if r.ID == reply.ActiveRootID { - root = r - break - } - } + root := reply.Active() if root == nil { r.Fatal("no active root") } From 919a1e93e7500400456491c1984962143016b189 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 11 Jan 2022 18:59:17 -0500 Subject: [PATCH 3/4] fix backport error This line was backported by automation, but it fails the build on this branch. --- agent/proxycfg/manager_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/agent/proxycfg/manager_test.go b/agent/proxycfg/manager_test.go index 2bebeec43511..68dab58406ed 100644 --- a/agent/proxycfg/manager_test.go +++ b/agent/proxycfg/manager_test.go @@ -584,7 +584,6 @@ func TestManager_SyncState_No_Notify(t *testing.T) { m, err := NewManager(ManagerConfig{ Cache: c, - Health: &health.Client{Cache: c, CacheName: cachetype.HealthServicesName}, State: state, Tokens: tokens, Source: &structs.QuerySource{Datacenter: "dc1"}, From 0af407b5864e2bcd07091fcf22eee4870230c88a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 11 Jan 2022 19:33:25 -0500 Subject: [PATCH 4/4] fix test failures For some reason 1.9 seems to require an explicit Datacenter name making these RPC requests. In newer versions it works with the empty string. --- agent/consul/leader_connect_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index ade371f6f42c..a241cfa45c51 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -341,7 +341,9 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { codec := rpcClient(t, s2) roots := structs.IndexedCARoots{} - err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{ + Datacenter: "dc2", + }, &roots) require.NoError(err) require.Len(roots.Roots, 1) @@ -359,7 +361,7 @@ func TestCAManager_RenewIntermediate_Secondary(t *testing.T) { } csr, _ := connect.TestCSR(t, spiffeService) - req := structs.CASignRequest{CSR: csr} + req := structs.CASignRequest{CSR: csr, Datacenter: "dc2"} cert := structs.IssuedCert{} err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", &req, &cert) require.NoError(err)