diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index 15c903133..fa13ed7be 100644 --- a/agent/connect/ca/provider_consul.go +++ b/agent/connect/ca/provider_consul.go @@ -21,6 +21,13 @@ import ( "github.com/hashicorp/go-hclog" ) +const ( + + // NotBefore will be CertificateTimeDriftBuffer in the past to account for + // time drift between different servers. + CertificateTimeDriftBuffer = time.Minute +) + var ErrNotInitialized = errors.New("provider not initialized") type ConsulProvider struct { @@ -474,7 +481,7 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string, // Sign the certificate valid from 1 minute in the past, this helps it be // accepted right away even when nodes are not in close time sync across the // cluster. A minute is more than enough for typical DC clock drift. - effectiveNow := time.Now().Add(-1 * time.Minute) + effectiveNow := time.Now().Add(-1 * CertificateTimeDriftBuffer) template := x509.Certificate{ SerialNumber: sn, Subject: csr.Subject, diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go index 04baee48e..d512cc276 100644 --- a/agent/consul/leader_connect.go +++ b/agent/consul/leader_connect.go @@ -696,7 +696,7 @@ func (s *Server) secondaryIntermediateCertRenewalWatch(ctx context.Context) erro return fmt.Errorf("error parsing active intermediate cert: %v", err) } - if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore, + if lessThanHalfTimePassed(time.Now(), intermediateCert.NotBefore.Add(ca.CertificateTimeDriftBuffer), intermediateCert.NotAfter) { return nil } diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index c4852d57a..1f22f2734 100644 --- a/agent/consul/leader_connect_test.go +++ b/agent/consul/leader_connect_test.go @@ -100,8 +100,8 @@ func TestLeader_SecondaryCA_Initialize(t *testing.T) { err error ) retry.Run(t, func(r *retry.R) { - _, caRoot = s1.getCAProvider() - secondaryProvider, _ = s2.getCAProvider() + _, caRoot = getCAProviderWithLock(s1) + secondaryProvider, _ = getCAProviderWithLock(s2) intermediatePEM, err = secondaryProvider.ActiveIntermediate() require.NoError(r, err) @@ -165,7 +165,7 @@ func TestLeader_SecondaryCA_Initialize(t *testing.T) { func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) { retry.Run(t, func(r *retry.R) { - _, root := srv.getCAProvider() + _, root := getCAProviderWithLock(srv) if root == nil { r.Fatal("no root") } @@ -175,6 +175,12 @@ func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) { }) } +func getCAProviderWithLock(s *Server) (ca.Provider, *structs.CARoot) { + s.caProviderReconfigurationLock.Lock() + defer s.caProviderReconfigurationLock.Unlock() + return s.getCAProvider() +} + func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { // no parallel execution because we change globals origInterval := structs.IntermediateCertRenewInterval @@ -227,7 +233,8 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { testrpc.WaitForLeader(t, s2.RPC, "dc2") // Get the original intermediate - secondaryProvider, _ := s2.getCAProvider() + // TODO: Wait for intermediate instead of wait for leader + secondaryProvider, _ := getCAProviderWithLock(s2) intermediatePEM, err := secondaryProvider.ActiveIntermediate() require.NoError(err) cert, err := connect.ParseCert(intermediatePEM) @@ -253,7 +260,7 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { // however, defaultQueryTime will be configurable and we con lower it // so that it returns for sure. retry.Run(t, func(r *retry.R) { - secondaryProvider, _ := s2.getCAProvider() + secondaryProvider, _ = getCAProviderWithLock(s2) intermediatePEM, err = secondaryProvider.ActiveIntermediate() r.Check(err) cert, err := connect.ParseCert(intermediatePEM) @@ -266,9 +273,9 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) { }) require.NoError(err) - // Get the new root from dc1 and validate a chain of: + // Get the root from dc1 and validate a chain of: // dc2 leaf -> dc2 intermediate -> dc1 root - _, caRoot := s1.getCAProvider() + _, caRoot := getCAProviderWithLock(s1) // Have dc2 sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ @@ -329,7 +336,7 @@ func TestLeader_SecondaryCA_IntermediateRefresh(t *testing.T) { testrpc.WaitForLeader(t, s2.RPC, "dc2") // Get the original intermediate - secondaryProvider, _ := s2.getCAProvider() + secondaryProvider, _ := getCAProviderWithLock(s2) oldIntermediatePEM, err := secondaryProvider.ActiveIntermediate() require.NoError(err) require.NotEmpty(oldIntermediatePEM) @@ -415,7 +422,7 @@ func TestLeader_SecondaryCA_IntermediateRefresh(t *testing.T) { // Get the new root from dc1 and validate a chain of: // dc2 leaf -> dc2 intermediate -> dc1 root - _, caRoot := s1.getCAProvider() + _, caRoot := getCAProviderWithLock(s1) // Have dc2 sign a leaf cert and make sure the chain is correct. spiffeService := &connect.SpiffeIDService{ @@ -524,7 +531,7 @@ func TestLeader_SecondaryCA_FixSigningKeyID_via_IntermediateRefresh(t *testing.T // the CA provider anyway. retry.Run(t, func(r *retry.R) { // verify that the root is now corrected - provider, activeRoot := s2.getCAProvider() + provider, activeRoot := getCAProviderWithLock(s2) require.NotNil(r, provider) require.NotNil(r, activeRoot) @@ -709,7 +716,7 @@ func TestLeader_SecondaryCA_UpgradeBeforePrimary(t *testing.T) { // Wait for the secondary transition to happen and then verify the secondary DC // has both roots present. - secondaryProvider, _ := s2.getCAProvider() + secondaryProvider, _ := getCAProviderWithLock(s2) retry.Run(t, func(r *retry.R) { state1 := s1.fsm.State() _, roots1, err := state1.CARoots(nil) @@ -730,7 +737,7 @@ func TestLeader_SecondaryCA_UpgradeBeforePrimary(t *testing.T) { require.NotEmpty(r, inter, "should have valid intermediate") }) - _, caRoot := s1.getCAProvider() + _, caRoot := getCAProviderWithLock(s1) intermediatePEM, err := secondaryProvider.ActiveIntermediate() require.NoError(t, err) @@ -1325,7 +1332,7 @@ func TestLeader_PersistIntermediateCAs(t *testing.T) { } // Get the active root before leader change. - _, root := s1.getCAProvider() + _, root := getCAProviderWithLock(s1) require.Len(root.IntermediateCerts, 1) // Force a leader change and make sure the root CA values are preserved. @@ -1344,7 +1351,7 @@ func TestLeader_PersistIntermediateCAs(t *testing.T) { r.Fatal("no leader") } - _, newLeaderRoot := leader.getCAProvider() + _, newLeaderRoot := getCAProviderWithLock(leader) if !reflect.DeepEqual(newLeaderRoot, root) { r.Fatalf("got %v, want %v", newLeaderRoot, root) }