Use fallback key ID when key version is empty

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

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)
 

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)

internal/crypto/keystores/keystore_test.go

@@ -96,12 +96,12 @@ 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,
-	})
+	keystore := keystores.NewKeyStoreFromMap(
+		map[string][]byte{
+			keyID: key,
+		},
+		"",
+	)
 
 	result, err := keystore.GetKey(keyID)
 	require.NoError(t, err)
@@ -110,3 +110,44 @@ func TestLocalFileKeyStore_GetKey(t *testing.T) {
 	_, err = keystore.GetKey("foobar")
 	require.ErrorIs(t, err, keystores.ErrUnknownKeyID)
 }
+
+func TestLocalFileKeyStore_GetKeyEmptyString(t *testing.T) {
+	t.Parallel()
+
+	keystore := keystores.NewKeyStoreFromMap(
+		map[string][]byte{
+			keyID: key,
+		},
+		keyID,
+	)
+
+	result, err := keystore.GetKey("")
+	require.NoError(t, err)
+	require.Equal(t, key, result)
+
+	result, err = keystore.GetKey(keyID)
+	require.NoError(t, err)
+	require.Equal(t, key, result)
+}
+
+func TestLocalFileKeyStore_GetKeyEmptyStringNoFallback(t *testing.T) {
+	t.Parallel()
+
+	keystore := keystores.NewKeyStoreFromMap(
+		map[string][]byte{
+			keyID: key,
+		},
+		"",
+	)
+
+	_, 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}
+)