From fb90449a289afe76de9818c964b1582ece624a18 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Fri, 31 May 2024 11:23:27 +0100 Subject: [PATCH 1/4] Use fallback key ID when key version is empty This addresses a bug preventing the decryption of old secrets when the default key has been rotated. --- internal/crypto/engine.go | 6 --- internal/crypto/keystores/keystore.go | 19 +++++++-- internal/crypto/keystores/keystore_test.go | 47 ++++++++++++++++++++-- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/internal/crypto/engine.go b/internal/crypto/engine.go index 2c77a312b4..34a0fd130d 100644 --- a/internal/crypto/engine.go +++ b/internal/crypto/engine.go @@ -194,12 +194,6 @@ func (e *engine) decrypt(data EncryptedData) ([]byte, error) { return nil, fmt.Errorf("%w: %s", algorithms.ErrUnknownAlgorithm, e.defaultAlgorithm) } - // for backwards compatibility with encrypted data which doesn't have the - // key ID stored in the DB. - if data.KeyVersion == "" { - data.KeyVersion = e.defaultKeyID - } - key, err := e.keystore.GetKey(data.KeyVersion) if err != nil { // error from keystore is good enough - we do not need more context diff --git a/internal/crypto/keystores/keystore.go b/internal/crypto/keystores/keystore.go index 371bbb189d..066b6fd37f 100644 --- a/internal/crypto/keystores/keystore.go +++ b/internal/crypto/keystores/keystore.go @@ -55,8 +55,10 @@ func NewKeyStoreFromConfig(config serverconfig.CryptoConfig) (KeyStore, error) { // Join the default key to the fallback keys to assemble the full // set of keys to load. keyIDs := []string{config.Default.KeyID} + fallbackKeyID := "" if config.Fallback.KeyID != "" { keyIDs = append(keyIDs, config.Fallback.KeyID) + fallbackKeyID = config.Fallback.KeyID } keys := make(keysByID, len(keyIDs)) for _, keyID := range keyIDs { @@ -68,21 +70,30 @@ func NewKeyStoreFromConfig(config serverconfig.CryptoConfig) (KeyStore, error) { } return &localFileKeyStore{ - keys: keys, + keys: keys, + fallbackKeyID: fallbackKeyID, }, nil } // NewKeyStoreFromMap constructs a keystore from a map of key ID to key bytes. // This is mostly useful for testing. -func NewKeyStoreFromMap(keys keysByID) KeyStore { - return &localFileKeyStore{keys} +func NewKeyStoreFromMap(keys keysByID, fallbackID string) KeyStore { + return &localFileKeyStore{keys, fallbackID} } type localFileKeyStore struct { - keys keysByID + keys keysByID + fallbackKeyID string } func (l *localFileKeyStore) GetKey(id string) ([]byte, error) { + if id == "" { + if l.fallbackKeyID != "" { + id = l.fallbackKeyID + } else { + return nil, errors.New("empty key ID with no config defined") + } + } key, ok := l.keys[id] if !ok { return nil, fmt.Errorf("%w: %s", ErrUnknownKeyID, id) diff --git a/internal/crypto/keystores/keystore_test.go b/internal/crypto/keystores/keystore_test.go index 2fe0ec4e7a..1acb62adc1 100644 --- a/internal/crypto/keystores/keystore_test.go +++ b/internal/crypto/keystores/keystore_test.go @@ -99,9 +99,12 @@ func TestLocalFileKeyStore_GetKey(t *testing.T) { keyID := "my_key" key := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} - keystore := keystores.NewKeyStoreFromMap(map[string][]byte{ - keyID: key, - }) + keystore := keystores.NewKeyStoreFromMap( + map[string][]byte{ + keyID: key, + }, + "", + ) result, err := keystore.GetKey(keyID) require.NoError(t, err) @@ -110,3 +113,41 @@ func TestLocalFileKeyStore_GetKey(t *testing.T) { _, err = keystore.GetKey("foobar") require.ErrorIs(t, err, keystores.ErrUnknownKeyID) } + +func TestLocalFileKeyStore_GetKeyEmptyString(t *testing.T) { + t.Parallel() + + keyID := "my_key" + key := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + + keystore := keystores.NewKeyStoreFromMap( + map[string][]byte{ + keyID: key, + }, + keyID, + ) + + result, err := keystore.GetKey("") + require.NoError(t, err) + require.Equal(t, key, result) + + _, err = keystore.GetKey("foobar") + require.ErrorIs(t, err, keystores.ErrUnknownKeyID) +} + +func TestLocalFileKeyStore_GetKeyEmptyStringNoFallback(t *testing.T) { + t.Parallel() + + keyID := "my_key" + key := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + + keystore := keystores.NewKeyStoreFromMap( + map[string][]byte{ + keyID: key, + }, + "", + ) + + _, err := keystore.GetKey("") + require.ErrorContains(t, err, "empty key ID with no config defined") +} From e025936985bce24b9bea3bbc83a1e418140a0705 Mon Sep 17 00:00:00 2001 From: Don Browne Date: Fri, 31 May 2024 11:25:26 +0100 Subject: [PATCH 2/4] fix lint --- internal/crypto/keystores/keystore_test.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/crypto/keystores/keystore_test.go b/internal/crypto/keystores/keystore_test.go index 1acb62adc1..55fb59dd05 100644 --- a/internal/crypto/keystores/keystore_test.go +++ b/internal/crypto/keystores/keystore_test.go @@ -96,9 +96,6 @@ func TestNewKeyStoreFromConfig(t *testing.T) { func TestLocalFileKeyStore_GetKey(t *testing.T) { t.Parallel() - keyID := "my_key" - key := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} - keystore := keystores.NewKeyStoreFromMap( map[string][]byte{ keyID: key, @@ -117,9 +114,6 @@ func TestLocalFileKeyStore_GetKey(t *testing.T) { func TestLocalFileKeyStore_GetKeyEmptyString(t *testing.T) { t.Parallel() - keyID := "my_key" - key := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} - keystore := keystores.NewKeyStoreFromMap( map[string][]byte{ keyID: key, @@ -138,9 +132,6 @@ func TestLocalFileKeyStore_GetKeyEmptyString(t *testing.T) { func TestLocalFileKeyStore_GetKeyEmptyStringNoFallback(t *testing.T) { t.Parallel() - keyID := "my_key" - key := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} - keystore := keystores.NewKeyStoreFromMap( map[string][]byte{ keyID: key, @@ -151,3 +142,11 @@ func TestLocalFileKeyStore_GetKeyEmptyStringNoFallback(t *testing.T) { _, err := keystore.GetKey("") require.ErrorContains(t, err, "empty key ID with no config defined") } + +const ( + keyID = "my_key" +) + +var ( + key = []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} +) From ff3bc054fe2b7bc0226ace0c040681794d16560e Mon Sep 17 00:00:00 2001 From: Don Browne Date: Fri, 31 May 2024 11:35:19 +0100 Subject: [PATCH 3/4] fix engine tests --- internal/crypto/engine_test.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/internal/crypto/engine_test.go b/internal/crypto/engine_test.go index 9da832a145..8bccbe7623 100644 --- a/internal/crypto/engine_test.go +++ b/internal/crypto/engine_test.go @@ -176,6 +176,22 @@ func TestDecryptBadAlgorithm(t *testing.T) { func TestDecryptBadEncoding(t *testing.T) { t.Parallel() + engine, err := NewEngineFromConfig(config) + require.NoError(t, err) + encryptedToken := EncryptedData{ + Algorithm: algorithms.Aes256Cfb, + EncodedData: "abc", + KeyVersion: "test_encryption_key", + } + require.NoError(t, err) + + _, err = engine.DecryptString(encryptedToken) + require.ErrorContains(t, err, "error decoding secret") +} + +func TestDecryptNoVersionNoFallback(t *testing.T) { + t.Parallel() + engine, err := NewEngineFromConfig(config) require.NoError(t, err) encryptedToken := EncryptedData{ @@ -187,7 +203,7 @@ func TestDecryptBadEncoding(t *testing.T) { require.NoError(t, err) _, err = engine.DecryptString(encryptedToken) - require.ErrorContains(t, err, "error decoding secret") + require.ErrorContains(t, err, "empty key ID with no config defined") } func TestDecryptFailedDecryption(t *testing.T) { @@ -199,7 +215,7 @@ func TestDecryptFailedDecryption(t *testing.T) { Algorithm: algorithms.Aes256Cfb, // too small of a value - will trigger the ciphertext length check EncodedData: "abcdef0123456789", - KeyVersion: "", + KeyVersion: "test_encryption_key", } require.NoError(t, err) From 48df42c7a16578b4c955f5e7113252a5f333c05a Mon Sep 17 00:00:00 2001 From: Don Browne Date: Fri, 31 May 2024 11:37:19 +0100 Subject: [PATCH 4/4] more useful test --- internal/crypto/keystores/keystore_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/crypto/keystores/keystore_test.go b/internal/crypto/keystores/keystore_test.go index 55fb59dd05..e13b6ffd75 100644 --- a/internal/crypto/keystores/keystore_test.go +++ b/internal/crypto/keystores/keystore_test.go @@ -125,8 +125,9 @@ func TestLocalFileKeyStore_GetKeyEmptyString(t *testing.T) { require.NoError(t, err) require.Equal(t, key, result) - _, err = keystore.GetKey("foobar") - require.ErrorIs(t, err, keystores.ErrUnknownKeyID) + result, err = keystore.GetKey(keyID) + require.NoError(t, err) + require.Equal(t, key, result) } func TestLocalFileKeyStore_GetKeyEmptyStringNoFallback(t *testing.T) {