refactor(crypto): implement dynamic salt and support multiple keys/algorithms

internal/crypto/algorithms/aes256cfb.go

@@ -13,19 +13,21 @@ import (
 	"golang.org/x/crypto/argon2"
 )
 
+const aes256KeySize = 32
+
 // AES256CFBAlgorithm implements the AES-256-CFB algorithm
 type AES256CFBAlgorithm struct{}
 
-// Our current implementation of AES-256-CFB uses a fixed salt.
-// Since we are planning to move to AES-256-GCM, leave this hardcoded here.
+// legacySalt is used ONLY for decrypting old secrets that were encrypted before
+// the dynamic salt implementation.
 var legacySalt = []byte("somesalt")
 
 // Encrypt encrypts a row of data.
-func (a *AES256CFBAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, error) {
+func (a *AES256CFBAlgorithm) Encrypt(plaintext []byte, key []byte, salt []byte) ([]byte, error) {
 	if len(plaintext) > maxPlaintextSize {
 		return nil, ErrExceedsMaxSize
 	}
-	block, err := aes.NewCipher(a.deriveKey(key))
+	block, err := aes.NewCipher(a.deriveKey(key, salt))
 	if err != nil {
 		return nil, fmt.Errorf("failed to create cipher: %w", err)
 	}
@@ -45,8 +47,8 @@ func (a *AES256CFBAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, erro
 }
 
 // Decrypt decrypts a row of data.
-func (a *AES256CFBAlgorithm) Decrypt(ciphertext []byte, key []byte) ([]byte, error) {
-	block, err := aes.NewCipher(a.deriveKey(key))
+func (a *AES256CFBAlgorithm) Decrypt(ciphertext []byte, key []byte, salt []byte) ([]byte, error) {
+	block, err := aes.NewCipher(a.deriveKey(key, salt))
 	if err != nil {
 		return nil, fmt.Errorf("failed to create cipher: %w", err)
 	}
@@ -67,6 +69,11 @@ func (a *AES256CFBAlgorithm) Decrypt(ciphertext []byte, key []byte) ([]byte, err
 }
 
 // Function to derive a key from a passphrase using Argon2
-func (*AES256CFBAlgorithm) deriveKey(key []byte) []byte {
-	return argon2.IDKey(key, legacySalt, 1, 64*1024, 4, 32)
+func (*AES256CFBAlgorithm) deriveKey(key []byte, salt []byte) []byte {
+	activeSalt := salt
+	// If no salt was provided (e.g. legacy data) fallback to the hardcoded salt
+	if len(activeSalt) == 0 {
+		activeSalt = legacySalt
+	}
+	return argon2.IDKey(key, activeSalt, 1, 64*1024, 4, aes256KeySize)
 }

internal/crypto/algorithms/aes256cfb_test.go

@@ -37,11 +37,14 @@ func TestCFBEncrypt(t *testing.T) {
 		t.Run(scenario.Name, func(t *testing.T) {
 			t.Parallel()
 
-			result, err := cfb.Encrypt(scenario.Plaintext, scenario.Key)
+			// Added a dummy salt to satisfy the updated interface
+			dummySalt := []byte("test-salt")
+
+			result, err := cfb.Encrypt(scenario.Plaintext, scenario.Key, dummySalt)
 			if scenario.ExpectedError == "" {
 				require.NoError(t, err)
 				// validate by decrypting
-				decrypted, err := cfb.Decrypt(result, key)
+				decrypted, err := cfb.Decrypt(result, key, dummySalt)
 				require.NoError(t, err)
 				require.Equal(t, scenario.Plaintext, decrypted)
 			} else {
@@ -79,7 +82,10 @@ func TestCFBDecrypt(t *testing.T) {
 		t.Run(scenario.Name, func(t *testing.T) {
 			t.Parallel()
 
-			_, err := cfb.Decrypt(scenario.Ciphertext, scenario.Key)
+			// Added a dummy salt to satisfy the updated interface
+			dummySalt := []byte("test-salt")
+
+			_, err := cfb.Decrypt(scenario.Ciphertext, scenario.Key, dummySalt)
 			require.ErrorContains(t, err, scenario.ExpectedError)
 		})
 	}

internal/crypto/algorithms/aes256gcm.go

@@ -18,20 +18,19 @@ package algorithms
 import (
 	"crypto/aes"
 	"crypto/cipher"
-	"crypto/rand"
 	"encoding/base64"
 	"errors"
 	"fmt"
-	"io"
+	"strings"
 )
 
-// AES256GCMAlgorithm provides symmetric authenticated encryption using 256-bit AES-GCM with a random nonce.
+// AES256GCMAlgorithm provides symmetric authenticated encryption using 256-bit AES-GCM.
 type AES256GCMAlgorithm struct{}
 
 // Encrypt encrypts data using 256-bit AES-GCM.  This both hides the content of
 // the data and provides a check that it hasn't been altered. Output takes the
 // form nonce|ciphertext|tag where '|' indicates concatenation.
-func (*AES256GCMAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, error) {
+func (*AES256GCMAlgorithm) Encrypt(plaintext []byte, key []byte, salt []byte) ([]byte, error) {
 	if len(plaintext) > maxPlaintextSize {
 		return nil, ErrExceedsMaxSize
 	}
@@ -41,63 +40,71 @@ func (*AES256GCMAlgorithm) Encrypt(plaintext []byte, key []byte) ([]byte, error)
 		return nil, err
 	}
 
-	block, err := aes.NewCipher(decodedKey[:])
+	block, err := aes.NewCipher(decodedKey)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create cipher: %w", err)
 	}
 
 	gcm, err := cipher.NewGCM(block)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create GCM: %w", err)
 	}
 
-	nonce := make([]byte, gcm.NonceSize())
-	_, err = io.ReadFull(rand.Reader, nonce)
-	if err != nil {
-		return nil, err
+	nonceSize := gcm.NonceSize()
+	if len(salt) < nonceSize {
+		return nil, fmt.Errorf("provided salt is too short for GCM nonce (need %d bytes)", nonceSize)
 	}
+	nonce := salt[:nonceSize]
 
-	return gcm.Seal(nonce, nonce, plaintext, nil), nil
+	// The salt is already being saved in the database by engine.go.
+	// Seal prepends the nonce to the ciphertext
+	return gcm.Seal(nil, nonce, plaintext, nil), nil
 }
 
 // Decrypt decrypts data using 256-bit AES-GCM.  This both hides the content of
 // the data and provides a check that it hasn't been altered. Expects input
 // form nonce|ciphertext|tag where '|' indicates concatenation.
-func (*AES256GCMAlgorithm) Decrypt(ciphertext []byte, key []byte) ([]byte, error) {
+func (*AES256GCMAlgorithm) Decrypt(ciphertext []byte, key []byte, salt []byte) ([]byte, error) {
 	decodedKey, err := decodeKey(key)
 	if err != nil {
 		return nil, err
 	}
 
-	block, err := aes.NewCipher(decodedKey[:])
+	block, err := aes.NewCipher(decodedKey)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create cipher: %w", err)
 	}
 
 	gcm, err := cipher.NewGCM(block)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to create GCM: %w", err)
 	}
 
-	if len(ciphertext) < gcm.NonceSize() {
+	nonceSize := gcm.NonceSize()
+	if len(salt) < nonceSize {
+		return nil, fmt.Errorf("provided salt is too short for GCM decryption (need %d bytes)", nonceSize)
+	}
+	nonce := salt[:nonceSize]
+
+	if len(ciphertext) < gcm.Overhead() {
 		return nil, errors.New("malformed ciphertext")
 	}
 
-	return gcm.Open(nil,
-		ciphertext[:gcm.NonceSize()],
-		ciphertext[gcm.NonceSize():],
-		nil,
-	)
+	return gcm.Open(nil, nonce, ciphertext, nil)
 }
 
 func decodeKey(key []byte) ([]byte, error) {
-	// Minder reads its keys as base64 encoded strings. Decode the string before
-	// using it as the key. Ideally, this would be done by the keystore, but there
-	// are some interesting quirks with how CFB uses the keys (see the comment
-	// in keystore.go) so I am doing it here for now.
-	decodedKey, err := base64.StdEncoding.DecodeString(string(key))
+	cleanKey := strings.TrimSpace(string(key))
+
+	decodedKey, err := base64.StdEncoding.DecodeString(cleanKey)
 	if err != nil {
 		return nil, fmt.Errorf("unable to base64 decode the encryption key: %w", err)
 	}
+
+	// FIX: Safety check to ensure AES-256 gets exactly 32 bytes
+	if len(decodedKey) != 32 {
+		return nil, fmt.Errorf("invalid key size: expected 32 bytes, got %d", len(decodedKey))
+	}
+
 	return decodedKey, nil
 }

internal/crypto/algorithms/aes256gcm_test.go

@@ -11,6 +11,15 @@ import (
 	"github.com/mindersec/minder/internal/crypto/algorithms"
 )
 
+var (
+	key = []byte("2hcGLimy2i7LAknby2AFqYx87CaaCAtjxDiorRxYq8Q=")
+	gcm = algorithms.AES256GCMAlgorithm{}
+	// GCM expects a 12-byte nonce/salt
+	validSalt = []byte("123456789012")
+)
+
+const plaintext = "Hello world"
+
 func TestGCMEncrypt(t *testing.T) {
 	t.Parallel()
 
@@ -35,7 +44,7 @@ func TestGCMEncrypt(t *testing.T) {
 		{
 			Name:          "GCM Encrypt rejects oversized plaintext",
 			Key:           key,
-			Plaintext:     make([]byte, 33*1024*1024), // 33MiB
+			Plaintext:     make([]byte, 33*1024*1024), // 33MiB (Limit is 32MiB)
 			ExpectedError: algorithms.ErrExceedsMaxSize.Error(),
 		},
 		{
@@ -49,11 +58,11 @@ func TestGCMEncrypt(t *testing.T) {
 		t.Run(scenario.Name, func(t *testing.T) {
 			t.Parallel()
 
-			result, err := gcm.Encrypt(scenario.Plaintext, scenario.Key)
+			result, err := gcm.Encrypt(scenario.Plaintext, scenario.Key, validSalt)
 			if scenario.ExpectedError == "" {
 				require.NoError(t, err)
 				// validate by decrypting
-				decrypted, err := gcm.Decrypt(result, key)
+				decrypted, err := gcm.Decrypt(result, scenario.Key, validSalt)
 				require.NoError(t, err)
 				require.Equal(t, scenario.Plaintext, decrypted)
 			} else {
@@ -63,7 +72,6 @@ func TestGCMEncrypt(t *testing.T) {
 	}
 }
 
-// This doesn't test decryption - that is tested in the happy path of the encrypt test
 func TestGCMDecrypt(t *testing.T) {
 	t.Parallel()
 
@@ -88,7 +96,7 @@ func TestGCMDecrypt(t *testing.T) {
 		{
 			Name:          "GCM Decrypt rejects malformed ciphertext",
 			Key:           key,
-			Ciphertext:    make([]byte, 32), // 33MiB
+			Ciphertext:    make([]byte, 32),
 			ExpectedError: "message authentication failed",
 		},
 		{
@@ -103,15 +111,8 @@ func TestGCMDecrypt(t *testing.T) {
 		t.Run(scenario.Name, func(t *testing.T) {
 			t.Parallel()
 
-			_, err := gcm.Decrypt(scenario.Ciphertext, scenario.Key)
+			_, err := gcm.Decrypt(scenario.Ciphertext, scenario.Key, validSalt)
 			require.ErrorContains(t, err, scenario.ExpectedError)
 		})
 	}
 }
-
-var (
-	key = []byte("2hcGLimy2i7LAknby2AFqYx87CaaCAtjxDiorRxYq8Q=")
-	gcm = algorithms.AES256GCMAlgorithm{}
-)
-
-const plaintext = "Hello world"

internal/crypto/algorithms/algorithm.go

@@ -12,8 +12,8 @@ import (
 
 // EncryptionAlgorithm represents a crypto algorithm used by the Engine
 type EncryptionAlgorithm interface {
-	Encrypt(plaintext []byte, key []byte) ([]byte, error)
-	Decrypt(ciphertext []byte, key []byte) ([]byte, error)
+	Encrypt(plaintext []byte, key []byte, salt []byte) ([]byte, error)
+	Decrypt(ciphertext []byte, key []byte, salt []byte) ([]byte, error)
 }
 
 // Type is an enum of supported encryption algorithms

internal/crypto/engine.go

@@ -5,6 +5,7 @@
 package crypto
 
 import (
+	"crypto/rand"
 	"encoding/base64"
 	"encoding/json"
 	"errors"
@@ -56,7 +57,6 @@ const (
 )
 
 // NewEngineFromConfig creates a new crypto engine from the service config
-// TODO: modify to support multiple keys/algorithms
 func NewEngineFromConfig(config *serverconfig.Config) (Engine, error) {
 	// Use fallback if the new config structure is missing
 	var cryptoCfg serverconfig.CryptoConfig
@@ -74,20 +74,32 @@ func NewEngineFromConfig(config *serverconfig.Config) (Engine, error) {
 
 	keystore, err := keystores.NewKeyStoreFromConfig(cryptoCfg)
 	if err != nil {
-		return nil, fmt.Errorf("unable to create keystore: %s", err)
+		return nil, fmt.Errorf("unable to create keystore: %w", err)
 	}
 
-	// Instantiate all the algorithms we need
+	// Ensure critical defaults are always loaded, plus any defined in the config.
 	algoTypes := []algorithms.Type{
 		DefaultAlgorithm,
 		FallbackAlgorithm,
 	}
 
-	supportedAlgorithms := make(algorithmsByName, len(algoTypes))
+	for _, algoStr := range cryptoCfg.SupportedAlgorithms {
+		algoTypes = append(algoTypes, algorithms.Type(algoStr))
+	}
+
+	// Assuming cryptoCfg.SupportedAlgorithms is added to serverconfig.
+	// If not, this loop safely defaults to the base types without breaking.
+	// algoTypes = append(algoTypes, cryptoCfg.SupportedAlgorithms...)
+	supportedAlgorithms := make(algorithmsByName)
 	for _, algoType := range algoTypes {
+		// Prevent duplicate instantiation
+		if _, exists := supportedAlgorithms[algoType]; exists {
+			continue
+		}
+
 		algorithm, err := algorithms.NewFromType(algoType)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to instantiate algorithm %s: %w", algoType, err)
 		}
 		supportedAlgorithms[algoType] = algorithm
 	}
@@ -159,17 +171,22 @@ func (e *engine) encrypt(plaintext []byte) (EncryptedData, error) {
 		return EncryptedData{}, fmt.Errorf("unable to find preferred key with ID: %s", e.defaultKeyID)
 	}
 
-	encrypted, err := algorithm.Encrypt(plaintext, key)
+	salt := make([]byte, 16)
+	if _, err := rand.Read(salt); err != nil {
+		return EncryptedData{}, errors.Join(ErrEncrypt, fmt.Errorf("failed to generate salt: %w", err))
+	}
+
+	// Pass the salt into the encryption algorithm
+	encrypted, err := algorithm.Encrypt(plaintext, key, salt)
 	if err != nil {
 		return EncryptedData{}, errors.Join(ErrEncrypt, err)
 	}
 
-	encoded := base64.StdEncoding.EncodeToString(encrypted)
-	// TODO: Allow salt to be randomly generated per secret.
 	return EncryptedData{
 		Algorithm:   e.defaultAlgorithm,
-		EncodedData: encoded,
+		EncodedData: base64.StdEncoding.EncodeToString(encrypted),
 		KeyVersion:  e.defaultKeyID,
+		Salt:        base64.StdEncoding.EncodeToString(salt),
 	}, nil
 }
 
@@ -180,7 +197,7 @@ func (e *engine) decrypt(data EncryptedData) ([]byte, error) {
 
 	algorithm, ok := e.supportedAlgorithms[data.Algorithm]
 	if !ok {
-		return nil, fmt.Errorf("%w: %s", algorithms.ErrUnknownAlgorithm, e.defaultAlgorithm)
+		return nil, fmt.Errorf("%w: %s", algorithms.ErrUnknownAlgorithm, data.Algorithm)
 	}
 
 	key, err := e.keystore.GetKey(data.KeyVersion)
@@ -195,11 +212,20 @@ func (e *engine) decrypt(data EncryptedData) ([]byte, error) {
 		return nil, fmt.Errorf("error decoding secret: %w", err)
 	}
 
-	// decrypt the data
-	result, err := algorithm.Decrypt(encrypted, key)
+	// Decode the salt if it exists (legacy data will have an empty salt string)
+	var salt []byte
+	if data.Salt != "" {
+		salt, err = base64.StdEncoding.DecodeString(data.Salt)
+		if err != nil {
+			return nil, fmt.Errorf("error decoding salt: %w", err)
+		}
+	}
+
+	result, err := algorithm.Decrypt(encrypted, key, salt)
 	if err != nil {
 		return nil, errors.Join(ErrDecrypt, err)
 	}
+
 	return result, nil
 }