diff --git a/agent/connect/ca/provider_consul_config.go b/agent/connect/ca/provider_consul_config.go index b290832ac4..48faa0717b 100644 --- a/agent/connect/ca/provider_consul_config.go +++ b/agent/connect/ca/provider_consul_config.go @@ -10,10 +10,7 @@ import ( ) func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderConfig, error) { - config := structs.ConsulCAProviderConfig{ - CommonCAProviderConfig: defaultCommonConfig(), - } - + config := defaultConsulCAProviderConfig() 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, diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index 854e6f90be..13ac312564 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/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"), }, } } diff --git a/agent/connect/ca/provider_test.go b/agent/connect/ca/provider_test.go index 618086b64c..9d0344b31c 100644 --- a/agent/connect/ca/provider_test.go +++ b/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{} { diff --git a/agent/connect/generate.go b/agent/connect/generate.go index 0e9bc911db..04b4f2900a 100644 --- a/agent/connect/generate.go +++ b/agent/connect/generate.go @@ -11,11 +11,13 @@ import ( "encoding/pem" "fmt" "strings" + "time" ) const ( - DefaultPrivateKeyType = "ec" - DefaultPrivateKeyBits = 256 + DefaultPrivateKeyType = "ec" + DefaultPrivateKeyBits = 256 + DefaultIntermediateCertTTL = 24 * 365 * time.Hour ) func pemEncodeKey(key []byte, blockType string) (string, error) { diff --git a/agent/connect/testing_ca.go b/agent/connect/testing_ca.go index 4b0d722886..fdd37ad4c1 100644 --- a/agent/connect/testing_ca.go +++ b/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{ diff --git a/agent/connect_ca_endpoint_test.go b/agent/connect_ca_endpoint_test.go index b5fb061421..72e3155336 100644 --- a/agent/connect_ca_endpoint_test.go +++ b/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", - "RotationPeriod": "1h", + "LeafCertTTL": "72h", + "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", - "RotationPeriod": "1h", + "LeafCertTTL": "72h", + "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", - "RotationPeriod": "1h", + "LeafCertTTL": "72h", + "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", - "RotationPeriod": "1h", + "LeafCertTTL": "72h", + "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 + ` } diff --git a/agent/consul/leader_connect.go b/agent/consul/leader_connect.go index b231329375..a430fe7aa6 100644 --- a/agent/consul/leader_connect.go +++ b/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 { - return config, nil - } - - config = s.config.CAConfig - if config.ClusterID == "" { - id, err := uuid.GenerateUUID() - if err != nil { - return nil, err + if config == nil { + config = s.config.CAConfig + if config.ClusterID == "" { + id, err := uuid.GenerateUUID() + if err != nil { + return nil, err + } + config.ClusterID = id } - 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() diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go index 16712841e8..5378a67ef6 100644 --- a/agent/consul/leader_connect_test.go +++ b/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. diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 5a224895e7..d701805622 100644 --- a/agent/consul/server_test.go +++ b/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", }, } diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 448f753ddc..03261d9b80 100644 --- a/agent/structs/connect_ca.go +++ b/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 { - return fmt.Errorf("leaf cert TTL must be greater than 1h") + if c.LeafCertTTL < MinLeafCertTTL { + return fmt.Errorf("leaf cert TTL must be greater or equal than %s", MinLeafCertTTL) } - if c.LeafCertTTL > 365*24*time.Hour { - return fmt.Errorf("leaf cert TTL must be less than 1 year") + if c.LeafCertTTL > MaxLeafCertTTL { + 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 diff --git a/agent/structs/connect_ca_test.go b/agent/structs/connect_ca_test.go index 8c7910982b..4f79a91eb4 100644 --- a/agent/structs/connect_ca_test.go +++ b/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) + }) + } +} diff --git a/command/connect/ca/set/connect_ca_set_test.go b/command/connect/ca/set/connect_ca_set_test.go index 3e31004041..18c913a955 100644 --- a/command/connect/ca/set/connect_ca_set_test.go +++ b/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) } diff --git a/command/connect/ca/set/test-fixtures/ca_config.json b/command/connect/ca/set/test-fixtures/ca_config.json index b05f14bfad..0ff99f3576 100644 --- a/command/connect/ca/set/test-fixtures/ca_config.json +++ b/command/connect/ca/set/test-fixtures/ca_config.json @@ -4,6 +4,6 @@ "PrivateKey": "", "RootCert": "", "RotationPeriod": "24h", - "IntermediateCertTTL": "36h" + "IntermediateCertTTL": "288h" } }