connect: add validations around intermediate cert ttl (#7213)

agent/connect/ca/provider_consul_config.go

@@ -10,10 +10,7 @@ import (
 )
 
 func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderConfig, error) {
-	config := structs.ConsulCAProviderConfig{
+	config := defaultConsulCAProviderConfig()
-		CommonCAProviderConfig: defaultCommonConfig(),
-	}
-
 	decodeConf := &mapstructure.DecoderConfig{
 		DecodeHook:       structs.ParseDurationFunc(),
 		Result:           &config,
@@ -37,9 +34,19 @@ func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderC
 		return nil, err
 	}
 
+	if err := config.Validate(); err != nil {
+		return nil, err
+	}
+
 	return &config, nil
 }
 
+func defaultConsulCAProviderConfig() structs.ConsulCAProviderConfig {
+	return structs.ConsulCAProviderConfig{
+		CommonCAProviderConfig: defaultCommonConfig(),
+		IntermediateCertTTL:    24 * 365 * time.Hour,
+	}
+}
 func defaultCommonConfig() structs.CommonCAProviderConfig {
 	return structs.CommonCAProviderConfig{
 		LeafCertTTL:    3 * 24 * time.Hour,

agent/connect/ca/provider_consul_test.go

@@ -69,7 +69,7 @@ func testConsulCAConfig() *structs.CAConfiguration {
 		Config: map[string]interface{}{
 			// Tests duration parsing after msgpack type mangling during raft apply.
 			"LeafCertTTL":         []uint8("72h"),
-			"IntermediateCertTTL": []uint8("72h"),
+			"IntermediateCertTTL": []uint8("288h"),
 		},
 	}
 }

agent/connect/ca/provider_test.go

@@ -51,7 +51,7 @@ func TestStructs_CAConfiguration_MsgpackEncodeDecode(t *testing.T) {
 					"PrivateKey":          "key",
 					"RootCert":            "cert",
 					"RotationPeriod":      "5m",
-					"IntermediateCertTTL": "30m",
+					"IntermediateCertTTL": "90h",
 					"DisableCrossSigning": true,
 				},
 			},
@@ -60,7 +60,7 @@ func TestStructs_CAConfiguration_MsgpackEncodeDecode(t *testing.T) {
 				PrivateKey:             "key",
 				RootCert:               "cert",
 				RotationPeriod:         5 * time.Minute,
-				IntermediateCertTTL:    30 * time.Minute,
+				IntermediateCertTTL:    90 * time.Hour,
 				DisableCrossSigning:    true,
 			},
 			parseFunc: func(t *testing.T, raw map[string]interface{}) interface{} {

agent/connect/generate.go

@@ -11,11 +11,13 @@ import (
 	"encoding/pem"
 	"fmt"
 	"strings"
+	"time"
 )
 
 const (
-	DefaultPrivateKeyType = "ec"
+	DefaultPrivateKeyType      = "ec"
-	DefaultPrivateKeyBits = 256
+	DefaultPrivateKeyBits      = 256
+	DefaultIntermediateCertTTL = 24 * 365 * time.Hour
 )
 
 func pemEncodeKey(key []byte, blockType string) (string, error) {

agent/connect/testing_ca.go

@@ -368,7 +368,7 @@ func testCAConfigSet(t testing.T, a TestAgentRPC,
 			"PrivateKey":          ca.SigningKey,
 			"RootCert":            ca.RootCert,
 			"RotationPeriod":      180 * 24 * time.Hour,
-			"IntermediateCertTTL": 72 * time.Hour,
+			"IntermediateCertTTL": 288 * time.Hour,
 		},
 	}
 	args := &structs.CARequest{

agent/connect_ca_endpoint_test.go

@@ -76,7 +76,8 @@ func TestConnectCAConfig(t *testing.T) {
 				"Provider": "consul",
 				"Config": {
 					"LeafCertTTL": "72h",
-					"RotationPeriod": "1h"
+					"RotationPeriod": "1h",
+					"IntermediateCertTTL": "288h"
 				}
 			}`,
 			wantErr: false,
@@ -84,8 +85,9 @@ func TestConnectCAConfig(t *testing.T) {
 				Provider:  "consul",
 				ClusterID: connect.TestClusterID,
 				Config: map[string]interface{}{
-					"LeafCertTTL":    "72h",
+					"LeafCertTTL":         "72h",
-					"RotationPeriod": "1h",
+					"RotationPeriod":      "1h",
+					"IntermediateCertTTL": "288h",
 				},
 			},
 		},
@@ -97,7 +99,7 @@ func TestConnectCAConfig(t *testing.T) {
 				"Config": {
 					"LeafCertTTL": "72h",
 					"RotationPeriod": "1h",
-					"IntermediateCertTTL": "2h"
+					"IntermediateCertTTL": "288h"
 				}
 			}`,
 			wantErr: false,
@@ -107,7 +109,7 @@ func TestConnectCAConfig(t *testing.T) {
 				Config: map[string]interface{}{
 					"LeafCertTTL":         "72h",
 					"RotationPeriod":      "1h",
-					"IntermediateCertTTL": "2h",
+					"IntermediateCertTTL": "288h",
 				},
 			},
 		},
@@ -118,7 +120,8 @@ func TestConnectCAConfig(t *testing.T) {
 				"Provider": "consul",
 				"Config": {
 					"LeafCertTTL": "72h",
-					"RotationPeriod": "1h"
+					"RotationPeriod": "1h",
+					"IntermediateCertTTL": "288h"
 				},
 				"ForceWithoutCrossSigning": true
 			}`,
@@ -127,8 +130,9 @@ func TestConnectCAConfig(t *testing.T) {
 				Provider:  "consul",
 				ClusterID: connect.TestClusterID,
 				Config: map[string]interface{}{
-					"LeafCertTTL":    "72h",
+					"LeafCertTTL":         "72h",
-					"RotationPeriod": "1h",
+					"RotationPeriod":      "1h",
+					"IntermediateCertTTL": "288h",
 				},
 				ForceWithoutCrossSigning: true,
 			},
@@ -145,7 +149,8 @@ func TestConnectCAConfig(t *testing.T) {
 				"provider": "consul",
 				"config": {
 					"LeafCertTTL": "72h",
-					"RotationPeriod": "1h"
+					"RotationPeriod": "1h",
+					"IntermediateCertTTL": "288h"
 				},
 				"force_without_cross_signing": true
 			}`,
@@ -154,8 +159,9 @@ func TestConnectCAConfig(t *testing.T) {
 				Provider:  "consul",
 				ClusterID: connect.TestClusterID,
 				Config: map[string]interface{}{
-					"LeafCertTTL":    "72h",
+					"LeafCertTTL":         "72h",
-					"RotationPeriod": "1h",
+					"RotationPeriod":      "1h",
+					"IntermediateCertTTL": "288h",
 				},
 				ForceWithoutCrossSigning: true,
 			},
@@ -179,7 +185,8 @@ func TestConnectCAConfig(t *testing.T) {
 				"Provider": "consul",
 				"config": {
 					"LeafCertTTL": "72h",
-					"RotationPeriod": "1h"
+					"RotationPeriod": "1h",
+				        "IntermediateCertTTL": "288h"
 				},
 				"State": {
 					"foo": "bar"
@@ -190,8 +197,9 @@ func TestConnectCAConfig(t *testing.T) {
 				Provider:  "consul",
 				ClusterID: connect.TestClusterID,
 				Config: map[string]interface{}{
-					"LeafCertTTL":    "72h",
+					"LeafCertTTL":         "72h",
-					"RotationPeriod": "1h",
+					"RotationPeriod":      "1h",
+					"IntermediateCertTTL": "288h",
 				},
 				State: map[string]string{
 					"foo": "bar",
@@ -211,6 +219,7 @@ func TestConnectCAConfig(t *testing.T) {
 					enabled = true
 					ca_provider = "consul"
 					ca_config {
+						intermediate_cert_ttl = "288h"
 						test_state {
 							` + tc.initialState + `
 						}

agent/consul/leader_connect.go

@@ -35,10 +35,6 @@ var (
 	// maxRetryBackoff is the maximum number of seconds to wait between failed blocking
 	// queries when backing off.
 	maxRetryBackoff = 256
-
-	// intermediateCertRenewInterval is the interval at which the expiration
-	// of the intermediate cert is checked and renewed if necessary.
-	intermediateCertRenewInterval = time.Hour
 )
 
 // initializeCAConfig is used to initialize the CA config if necessary
@@ -49,25 +45,36 @@ func (s *Server) initializeCAConfig() (*structs.CAConfiguration, error) {
 	if err != nil {
 		return nil, err
 	}
-	if config != nil {
+	if config == nil {
-		return config, nil
+		config = s.config.CAConfig
-	}
+		if config.ClusterID == "" {
-
+			id, err := uuid.GenerateUUID()
-	config = s.config.CAConfig
+			if err != nil {
-	if config.ClusterID == "" {
+				return nil, err
-		id, err := uuid.GenerateUUID()
+			}
-		if err != nil {
+			config.ClusterID = id
-			return nil, err
 		}
-		config.ClusterID = id
+	} else if _, ok := config.Config["IntermediateCertTTL"]; !ok {
+		dup := *config
+		copied := make(map[string]interface{})
+		for k, v := range dup.Config {
+			copied[k] = v
+		}
+		copied["IntermediateCertTTL"] = connect.DefaultIntermediateCertTTL.String()
+		dup.Config = copied
+		config = &dup
+	} else {
+		return config, nil
 	}
 
 	req := structs.CARequest{
 		Op:     structs.CAOpSetConfig,
 		Config: config,
 	}
-	if _, err = s.raftApply(structs.ConnectCARequestType, req); err != nil {
+	if resp, err := s.raftApply(structs.ConnectCARequestType, req); err != nil {
 		return nil, err
+	} else if respErr, ok := resp.(error); ok {
+		return nil, respErr
 	}
 
 	return config, nil
@@ -643,7 +650,7 @@ func (s *Server) secondaryIntermediateCertRenewalWatch(ctx context.Context) erro
 		select {
 		case <-ctx.Done():
 			return nil
-		case <-time.After(intermediateCertRenewInterval):
+		case <-time.After(structs.IntermediateCertRenewInterval):
 			retryLoopBackoff(ctx.Done(), func() error {
 				s.caProviderReconfigurationLock.Lock()
 				defer s.caProviderReconfigurationLock.Unlock()

agent/consul/leader_connect_test.go

@@ -175,9 +175,16 @@ func waitForActiveCARoot(t *testing.T, srv *Server, expect *structs.CARoot) {
 }
 
 func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
-	t.Parallel()
+	// no parallel execution because we change globals
+	origInterval := structs.IntermediateCertRenewInterval
+	origMinTTL := structs.MinLeafCertTTL
+	defer func() {
+		structs.IntermediateCertRenewInterval = origInterval
+		structs.MinLeafCertTTL = origMinTTL
+	}()
 
-	intermediateCertRenewInterval = time.Millisecond
+	structs.IntermediateCertRenewInterval = time.Millisecond
+	structs.MinLeafCertTTL = time.Second
 	require := require.New(t)
 
 	dir1, s1 := testServerWithConfig(t, func(c *Config) {
@@ -188,7 +195,7 @@ func TestLeader_SecondaryCA_IntermediateRenew(t *testing.T) {
 				"PrivateKey":     "",
 				"RootCert":       "",
 				"RotationPeriod": "2160h",
-				"LeafCertTTL":    "72h",
+				"LeafCertTTL":    "5s",
 				// The retry loop only retries for 7sec max and
 				// the ttl needs to be below so that it
 				// triggers definitely.

agent/consul/server_test.go

@@ -160,7 +160,7 @@ func testServerConfig(t *testing.T) (string, *Config) {
 			"RootCert":            "",
 			"RotationPeriod":      "2160h",
 			"LeafCertTTL":         "72h",
-			"IntermediateCertTTL": "72h",
+			"IntermediateCertTTL": "288h",
 		},
 	}
 

agent/structs/connect_ca.go

@@ -351,17 +351,20 @@ type CommonCAProviderConfig struct {
 	PrivateKeyBits int
 }
 
+var MinLeafCertTTL = time.Hour
+var MaxLeafCertTTL = 365 * 24 * time.Hour
+
 func (c CommonCAProviderConfig) Validate() error {
 	if c.SkipValidate {
 		return nil
 	}
 
-	if c.LeafCertTTL < time.Hour {
+	if c.LeafCertTTL < MinLeafCertTTL {
-		return fmt.Errorf("leaf cert TTL must be greater than 1h")
+		return fmt.Errorf("leaf cert TTL must be greater or equal than %s", MinLeafCertTTL)
 	}
 
-	if c.LeafCertTTL > 365*24*time.Hour {
+	if c.LeafCertTTL > MaxLeafCertTTL {
-		return fmt.Errorf("leaf cert TTL must be less than 1 year")
+		return fmt.Errorf("leaf cert TTL must be less than %s", MaxLeafCertTTL)
 	}
 
 	switch c.PrivateKeyType {
@@ -395,6 +398,40 @@ type ConsulCAProviderConfig struct {
 	DisableCrossSigning bool
 }
 
+// intermediateCertRenewInterval is the interval at which the expiration
+// of the intermediate cert is checked and renewed if necessary.
+var IntermediateCertRenewInterval = time.Hour
+
+func (c *ConsulCAProviderConfig) Validate() error {
+	if c.IntermediateCertTTL < (3 * IntermediateCertRenewInterval) {
+		// Intermediate Certificates are checked every
+		// hour(intermediateCertRenewInterval) if they are about to
+		// expire. Recreating an intermediate certs is started once
+		// more than half its lifetime has passed.
+		// If it would be 2h, worst case is that the check happens
+		// right before half time and when the check happens again, the
+		// certificate is very close to expiring, leaving only a small
+		// timeframe to renew. 3h leaves more than 30min to recreate.
+		// Right now the minimum LeafCertTTL is 1h, which means this
+		// check not strictly needed, because the same thing is covered
+		// in the next check too. But just in case minimum LeafCertTTL
+		// changes at some point, this validation must still be
+		// performed.
+		return fmt.Errorf("Intermediate Cert TTL must be greater or equal than %dh", 3*int(IntermediateCertRenewInterval.Hours()))
+	}
+	if c.IntermediateCertTTL < (3 * c.CommonCAProviderConfig.LeafCertTTL) {
+		// Intermediate Certificates are being sent to the proxy when
+		// the Leaf Certificate changes because they are bundled
+		// together.
+		// That means that the Intermediate Certificate TTL must be at
+		// a minimum of 3 * Leaf Certificate TTL to ensure that the new
+		// Intermediate is being set together with the Leaf Certificate
+		// before it expires.
+		return fmt.Errorf("Intermediate Cert TTL must be greater or equal than 3 * LeafCertTTL (>=%s).", 3*c.CommonCAProviderConfig.LeafCertTTL)
+	}
+	return nil
+}
+
 // CAConsulProviderState is used to track the built-in Consul CA provider's state.
 type CAConsulProviderState struct {
 	ID               string

agent/structs/connect_ca_test.go

@@ -59,3 +59,55 @@ func TestCAConfiguration_GetCommonConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestCAProviderConfig_Validate(t *testing.T) {
+	tests := []struct {
+		name    string
+		cfg     *ConsulCAProviderConfig
+		wantErr bool
+		wantMsg string
+	}{
+		{
+			name:    "defaults",
+			cfg:     &ConsulCAProviderConfig{},
+			wantErr: true,
+			wantMsg: "Intermediate Cert TTL must be greater or equal than 3h",
+		},
+		{
+			name: "intermediate cert ttl too short",
+			cfg: &ConsulCAProviderConfig{
+				CommonCAProviderConfig: CommonCAProviderConfig{LeafCertTTL: 2 * time.Hour},
+				IntermediateCertTTL:    4 * time.Hour,
+			},
+			wantErr: true,
+			wantMsg: "Intermediate Cert TTL must be greater or equal than 3 * LeafCertTTL (>=6h0m0s).",
+		},
+		{
+			name: "intermediate cert ttl too short",
+			cfg: &ConsulCAProviderConfig{
+				CommonCAProviderConfig: CommonCAProviderConfig{LeafCertTTL: 5 * time.Hour},
+				IntermediateCertTTL:    15*time.Hour - 1,
+			},
+			wantErr: true,
+			wantMsg: "Intermediate Cert TTL must be greater or equal than 3 * LeafCertTTL (>=15h0m0s).",
+		},
+		{
+			name: "good intermediate and leaf cert TTL",
+			cfg: &ConsulCAProviderConfig{
+				CommonCAProviderConfig: CommonCAProviderConfig{LeafCertTTL: 1 * time.Hour},
+				IntermediateCertTTL:    4 * time.Hour,
+			},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.cfg.Validate()
+			if err == nil {
+				require.False(t, tt.wantErr)
+				return
+			}
+			require.Equal(t, err.Error(), tt.wantMsg)
+		})
+	}
+}

command/connect/ca/set/connect_ca_set_test.go

@@ -50,5 +50,5 @@ func TestConnectCASetConfigCommand(t *testing.T) {
 	parsed, err := ca.ParseConsulCAConfig(reply.Config)
 	require.NoError(err)
 	require.Equal(24*time.Hour, parsed.RotationPeriod)
-	require.Equal(36*time.Hour, parsed.IntermediateCertTTL)
+	require.Equal(288*time.Hour, parsed.IntermediateCertTTL)
 }

command/connect/ca/set/test-fixtures/ca_config.json

@@ -4,6 +4,6 @@
 		"PrivateKey": "",
 		"RootCert": "",
 		"RotationPeriod": "24h",
-		"IntermediateCertTTL": "36h"
+		"IntermediateCertTTL": "288h"
 	}
 }