From 74f2a80a428a171c0375c8c39efa0b1a37bd63f2 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Thu, 13 Sep 2018 15:43:00 +0100 Subject: [PATCH] Fix CA pruning when CA config uses string durations. (#4669) * Fix CA pruning when CA config uses string durations. The tl;dr here is: - Configuring LeafCertTTL with a string like "72h" is how we do it by default and should be supported - Most of our tests managed to escape this by defining them as time.Duration directly - Out actual default value is a string - Since this is stored in a map[string]interface{} config, when it is written to Raft it goes through a msgpack encode/decode cycle (even though it's written from server not over RPC). - msgpack decode leaves the string as a `[]uint8` - Some of our parsers required string and failed - So after 1 hour, a default configured server would throw an error about pruning old CAs - If a new CA was configured that set LeafCertTTL as a time.Duration, things might be OK after that, but if a new CA was just configured from config file, intialization would cause same issue but always fail still so would never prune the old CA. - Mostly this is just a janky error that got passed tests due to many levels of complicated encoding/decoding. tl;dr of the tl;dr: Yay for type safety. Map[string]interface{} combined with msgpack always goes wrong but we somehow get bitten every time in a new way :D We already fixed this once! The main CA config had the same problem so @kyhavlov already wrote the mapstructure DecodeHook that fixes it. It wasn't used in several places it needed to be and one of those is notw in `structs` which caused a dependency cycle so I've moved them. This adds a whole new test thta explicitly tests the case that broke here. It also adds tests that would have failed in other places before (Consul and Vaul provider parsing functions). I'm not sure if they would ever be affected as it is now as we've not seen things broken with them but it seems better to explicitly test that and support it to not be bitten a third time! * Typo fix * Fix bad Uint8 usage --- agent/connect/ca/provider_consul_config.go | 45 +---------------- agent/connect/ca/provider_consul_test.go | 5 +- agent/connect/ca/provider_vault.go | 2 +- agent/connect/ca/provider_vault_test.go | 2 + agent/connect_ca_endpoint.go | 3 +- agent/consul/leader_test.go | 4 +- agent/consul/server_test.go | 3 +- agent/structs/connect_ca.go | 48 +++++++++++++++++- agent/structs/connect_ca_test.go | 58 ++++++++++++++++++++++ 9 files changed, 117 insertions(+), 53 deletions(-) create mode 100644 agent/structs/connect_ca_test.go diff --git a/agent/connect/ca/provider_consul_config.go b/agent/connect/ca/provider_consul_config.go index 9c94f0e62a..f3af3a4898 100644 --- a/agent/connect/ca/provider_consul_config.go +++ b/agent/connect/ca/provider_consul_config.go @@ -2,7 +2,6 @@ package ca import ( "fmt" - "reflect" "time" "github.com/hashicorp/consul/agent/structs" @@ -15,7 +14,7 @@ func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderC } decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: ParseDurationFunc(), + DecodeHook: structs.ParseDurationFunc(), Result: &config, WeaklyTypedInput: true, } @@ -40,48 +39,6 @@ func ParseConsulCAConfig(raw map[string]interface{}) (*structs.ConsulCAProviderC return &config, nil } -// ParseDurationFunc is a mapstructure hook for decoding a string or -// []uint8 into a time.Duration value. -func ParseDurationFunc() mapstructure.DecodeHookFunc { - return func( - f reflect.Type, - t reflect.Type, - data interface{}) (interface{}, error) { - var v time.Duration - if t != reflect.TypeOf(v) { - return data, nil - } - - switch { - case f.Kind() == reflect.String: - if dur, err := time.ParseDuration(data.(string)); err != nil { - return nil, err - } else { - v = dur - } - return v, nil - case f == reflect.SliceOf(reflect.TypeOf(uint8(0))): - s := Uint8ToString(data.([]uint8)) - if dur, err := time.ParseDuration(s); err != nil { - return nil, err - } else { - v = dur - } - return v, nil - default: - return data, nil - } - } -} - -func Uint8ToString(bs []uint8) string { - b := make([]byte, len(bs)) - for i, v := range bs { - b[i] = byte(v) - } - return string(b) -} - 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 2a37f1b94f..2092d934d4 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/agent/connect/ca/provider_consul_test.go @@ -65,7 +65,10 @@ func testConsulCAConfig() *structs.CAConfiguration { return &structs.CAConfiguration{ ClusterID: "asdf", Provider: "consul", - Config: map[string]interface{}{}, + Config: map[string]interface{}{ + // Tests duration parsing after msgpack type mangling during raft apply. + "LeafCertTTL": []uint8("72h"), + }, } } diff --git a/agent/connect/ca/provider_vault.go b/agent/connect/ca/provider_vault.go index 743ea8957e..eaf3646090 100644 --- a/agent/connect/ca/provider_vault.go +++ b/agent/connect/ca/provider_vault.go @@ -289,7 +289,7 @@ func ParseVaultCAConfig(raw map[string]interface{}) (*structs.VaultCAProviderCon } decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + DecodeHook: structs.ParseDurationFunc(), Result: &config, WeaklyTypedInput: true, } diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 5c248e8dc4..05f8c36448 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -32,6 +32,8 @@ func testVaultClusterWithConfig(t *testing.T, rawConf map[string]interface{}) (* "Token": token, "RootPKIPath": "pki-root/", "IntermediatePKIPath": "pki-intermediate/", + // Tests duration parsing after msgpack type mangling during raft apply. + "LeafCertTTL": []uint8("72h"), } for k, v := range rawConf { conf[k] = v diff --git a/agent/connect_ca_endpoint.go b/agent/connect_ca_endpoint.go index 82d1233699..402797a8fa 100644 --- a/agent/connect_ca_endpoint.go +++ b/agent/connect_ca_endpoint.go @@ -4,7 +4,6 @@ import ( "fmt" "net/http" - "github.com/hashicorp/consul/agent/connect/ca" "github.com/hashicorp/consul/agent/structs" ) @@ -81,7 +80,7 @@ func (s *HTTPServer) ConnectCAConfigurationSet(resp http.ResponseWriter, req *ht func fixupConfig(conf *structs.CAConfiguration) { for k, v := range conf.Config { if raw, ok := v.([]uint8); ok { - strVal := ca.Uint8ToString(raw) + strVal := structs.Uint8ToString(raw) conf.Config[k] = strVal switch conf.Provider { case structs.ConsulCAProvider: diff --git a/agent/consul/leader_test.go b/agent/consul/leader_test.go index fe967c2f5c..ca42a5404a 100644 --- a/agent/consul/leader_test.go +++ b/agent/consul/leader_test.go @@ -1038,10 +1038,10 @@ func TestLeader_CARootPruning(t *testing.T) { newConfig := &structs.CAConfiguration{ Provider: "consul", Config: map[string]interface{}{ - "LeafCertTTL": 500 * time.Millisecond, + "LeafCertTTL": "500ms", "PrivateKey": newKey, "RootCert": "", - "RotationPeriod": 90 * 24 * time.Hour, + "RotationPeriod": "2160h", "SkipValidate": true, }, } diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index 43dcd13ff2..bbd3ef60d9 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -100,7 +100,8 @@ func testServerConfig(t *testing.T) (string, *Config) { Config: map[string]interface{}{ "PrivateKey": "", "RootCert": "", - "RotationPeriod": 90 * 24 * time.Hour, + "RotationPeriod": "2160h", + "LeafCertTTL": "72h", }, } diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 1e869fd45d..76162c2f65 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -2,6 +2,7 @@ package structs import ( "fmt" + "reflect" "time" "github.com/mitchellh/mapstructure" @@ -202,8 +203,9 @@ func (c *CAConfiguration) GetCommonConfig() (*CommonCAProviderConfig, error) { var config CommonCAProviderConfig decodeConf := &mapstructure.DecoderConfig{ - DecodeHook: mapstructure.StringToTimeDurationHookFunc(), - Result: &config, + DecodeHook: ParseDurationFunc(), + Result: &config, + WeaklyTypedInput: true, } decoder, err := mapstructure.NewDecoder(decodeConf) @@ -265,3 +267,45 @@ type VaultCAProviderConfig struct { RootPKIPath string IntermediatePKIPath string } + +// ParseDurationFunc is a mapstructure hook for decoding a string or +// []uint8 into a time.Duration value. +func ParseDurationFunc() mapstructure.DecodeHookFunc { + return func( + f reflect.Type, + t reflect.Type, + data interface{}) (interface{}, error) { + var v time.Duration + if t != reflect.TypeOf(v) { + return data, nil + } + + switch { + case f.Kind() == reflect.String: + if dur, err := time.ParseDuration(data.(string)); err != nil { + return nil, err + } else { + v = dur + } + return v, nil + case f == reflect.SliceOf(reflect.TypeOf(uint8(0))): + s := Uint8ToString(data.([]uint8)) + if dur, err := time.ParseDuration(s); err != nil { + return nil, err + } else { + v = dur + } + return v, nil + default: + return data, nil + } + } +} + +func Uint8ToString(bs []uint8) string { + b := make([]byte, len(bs)) + for i, v := range bs { + b[i] = byte(v) + } + return string(b) +} diff --git a/agent/structs/connect_ca_test.go b/agent/structs/connect_ca_test.go new file mode 100644 index 0000000000..dd185ebe1f --- /dev/null +++ b/agent/structs/connect_ca_test.go @@ -0,0 +1,58 @@ +package structs + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestCAConfiguration_GetCommonConfig(t *testing.T) { + tests := []struct { + name string + cfg *CAConfiguration + want *CommonCAProviderConfig + wantErr bool + }{ + { + name: "basic defaults", + cfg: &CAConfiguration{ + Config: map[string]interface{}{ + "RotationPeriod": "2160h", + "LeafCertTTL": "72h", + }, + }, + want: &CommonCAProviderConfig{ + LeafCertTTL: 72 * time.Hour, + }, + }, + { + // Note that this is currently what is actually stored in MemDB, I think + // due to a trip through msgpack somewhere but I'm not really sure why + // since the defaults are applied on the server and so should probably use + // direct RPC that bypasses encoding? Either way this case is important + // because it reflects the actual data as it's stored in state which is + // what matters in real life. + name: "basic defaults after encoding fun", + cfg: &CAConfiguration{ + Config: map[string]interface{}{ + "RotationPeriod": []uint8("2160h"), + "LeafCertTTL": []uint8("72h"), + }, + }, + want: &CommonCAProviderConfig{ + LeafCertTTL: 72 * time.Hour, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.cfg.GetCommonConfig() + if (err != nil) != tt.wantErr { + t.Errorf("CAConfiguration.GetCommonConfig() error = %v, wantErr %v", err, tt.wantErr) + return + } + require.Equal(t, tt.want, got) + }) + } +}