From 315d57bfb1c70be2050b618117b02cdfce4c8f79 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 13 Feb 2020 20:35:09 +0100 Subject: [PATCH] agent: sensible keyring error (#7272) Fixes #7231. Before an agent would always emit a warning when there is an encrypt key in the configuration and an existing keyring stored, which is happening on restart. Now it only emits that warning when the encrypt key from the configuration is not part of the keyring. --- agent/agent.go | 25 +++++++++++++++++++++++++ agent/config/builder.go | 10 ---------- agent/config/runtime_test.go | 35 ----------------------------------- agent/keyring.go | 24 ++++++++++++++++++++++-- agent/keyring_test.go | 12 ++++++++++++ 5 files changed, 59 insertions(+), 47 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index aca723e2ea..7c79d202df 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1585,6 +1585,7 @@ func (a *Agent) setupBaseKeyrings(config *consul.Config) error { fileLAN := filepath.Join(a.config.DataDir, SerfLANKeyring) fileWAN := filepath.Join(a.config.DataDir, SerfWANKeyring) + var existingLANKeyring, existingWANKeyring bool if a.config.EncryptKey == "" { goto LOAD } @@ -1592,12 +1593,16 @@ func (a *Agent) setupBaseKeyrings(config *consul.Config) error { if err := initKeyring(fileLAN, a.config.EncryptKey); err != nil { return err } + } else { + existingLANKeyring = true } if a.config.ServerMode && federationEnabled { if _, err := os.Stat(fileWAN); err != nil { if err := initKeyring(fileWAN, a.config.EncryptKey); err != nil { return err } + } else { + existingWANKeyring = true } } @@ -1617,6 +1622,26 @@ LOAD: } } + // Only perform the following checks if there was an encrypt_key + // provided in the configuration. + if a.config.EncryptKey != "" { + msg := " keyring doesn't include key provided with -encrypt, using keyring" + if existingLANKeyring && + keyringIsMissingKey( + config.SerfLANConfig.MemberlistConfig.Keyring, + a.config.EncryptKey, + ) { + a.logger.Warn(msg, "keyring", "LAN") + } + if existingWANKeyring && + keyringIsMissingKey( + config.SerfWANConfig.MemberlistConfig.Keyring, + a.config.EncryptKey, + ) { + a.logger.Warn(msg, "keyring", "WAN") + } + } + return nil } diff --git a/agent/config/builder.go b/agent/config/builder.go index a802cfc78b..94ac6e789a 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -1106,16 +1106,6 @@ func (b *Builder) Validate(rt RuntimeConfig) error { if _, err := decodeBytes(rt.EncryptKey); err != nil { return fmt.Errorf("encrypt has invalid key: %s", err) } - keyfileLAN := filepath.Join(rt.DataDir, SerfLANKeyring) - if _, err := os.Stat(keyfileLAN); err == nil { - b.warn("WARNING: LAN keyring exists but -encrypt given, using keyring") - } - if rt.ServerMode { - keyfileWAN := filepath.Join(rt.DataDir, SerfWANKeyring) - if _, err := os.Stat(keyfileWAN); err == nil { - b.warn("WARNING: WAN keyring exists but -encrypt given, using keyring") - } - } } // Check the data dir for signs of an un-migrated Consul 0.5.x or older diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index a37bb12629..f1289677ac 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -2157,41 +2157,6 @@ func TestConfigFlagsAndEdgecases(t *testing.T) { hcl: []string{` encrypt = "this is not a valid key" `}, err: "encrypt has invalid key: illegal base64 data at input byte 4", }, - { - desc: "encrypt given but LAN keyring exists", - args: []string{ - `-data-dir=` + dataDir, - }, - json: []string{`{ "encrypt": "pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=" }`}, - hcl: []string{` encrypt = "pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=" `}, - patch: func(rt *RuntimeConfig) { - rt.EncryptKey = "pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=" - rt.DataDir = dataDir - }, - pre: func() { - writeFile(filepath.Join(dataDir, SerfLANKeyring), []byte("pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=")) - }, - warns: []string{`WARNING: LAN keyring exists but -encrypt given, using keyring`}, - }, - { - desc: "encrypt given but WAN keyring exists", - args: []string{ - `-data-dir=` + dataDir, - }, - json: []string{`{ "encrypt": "pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=", "server": true }`}, - hcl: []string{` encrypt = "pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=" server = true `}, - patch: func(rt *RuntimeConfig) { - rt.EncryptKey = "pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=" - rt.ServerMode = true - rt.LeaveOnTerm = false - rt.SkipLeaveOnInt = true - rt.DataDir = dataDir - }, - pre: func() { - writeFile(filepath.Join(dataDir, SerfWANKeyring), []byte("pUqJrVyVRj5jsiYEkM/tFQYfWyJIv4s3XkvDwy7Cu5s=")) - }, - warns: []string{`WARNING: WAN keyring exists but -encrypt given, using keyring`}, - }, { desc: "multiple check files", args: []string{ diff --git a/agent/keyring.go b/agent/keyring.go index 6b0c48a5a9..10817f5bd5 100644 --- a/agent/keyring.go +++ b/agent/keyring.go @@ -1,6 +1,7 @@ package agent import ( + "bytes" "encoding/base64" "encoding/json" "fmt" @@ -23,7 +24,7 @@ const ( func initKeyring(path, key string) error { var keys []string - if keyBytes, err := base64.StdEncoding.DecodeString(key); err != nil { + if keyBytes, err := decodeStringKey(key); err != nil { return fmt.Errorf("Invalid key: %s", err) } else if err := memberlist.ValidateKey(keyBytes); err != nil { return fmt.Errorf("Invalid key: %s", err) @@ -87,7 +88,7 @@ func loadKeyringFile(c *serf.Config) error { func loadKeyring(c *serf.Config, keys []string) error { keysDecoded := make([][]byte, len(keys)) for i, key := range keys { - keyBytes, err := base64.StdEncoding.DecodeString(key) + keyBytes, err := decodeStringKey(key) if err != nil { return err } @@ -107,6 +108,10 @@ func loadKeyring(c *serf.Config, keys []string) error { return nil } +func decodeStringKey(key string) ([]byte, error) { + return base64.StdEncoding.DecodeString(key) +} + // keyringProcess is used to abstract away the semantic similarities in // performing various operations on the encryption keyring. func (a *Agent) keyringProcess(args *structs.KeyringRequest) (*structs.KeyringResponses, error) { @@ -172,3 +177,18 @@ func parseKeyringRequest(req *structs.KeyringRequest, token string, relayFactor req.Token = token req.RelayFactor = relayFactor } + +// keyringIsMissingKey checks whether a key is part of a keyring. Returns true +// if it is not included. +func keyringIsMissingKey(keyring *memberlist.Keyring, key string) bool { + k1, err := decodeStringKey(key) + if err != nil { + return true + } + for _, k2 := range keyring.GetKeys() { + if bytes.Equal(k1, k2) { + return false + } + } + return true +} diff --git a/agent/keyring_test.go b/agent/keyring_test.go index c67db8d831..fe93d24cb5 100644 --- a/agent/keyring_test.go +++ b/agent/keyring_test.go @@ -334,3 +334,15 @@ func TestValidateLocalOnly(t *testing.T) { require.Error(t, ValidateLocalOnly(true, false)) } + +func TestAgent_KeyringIsMissingKey(t *testing.T) { + key1 := "tbLJg26ZJyJ9pK3qhc9jig==" + key2 := "4leC33rgtXKIVUr9Nr0snQ==" + decoded1, err := decodeStringKey(key1) + require.NoError(t, err) + keyring, err := memberlist.NewKeyring([][]byte{}, decoded1) + require.NoError(t, err) + + require.True(t, keyringIsMissingKey(keyring, key2)) + require.False(t, keyringIsMissingKey(keyring, key1)) +}