diff --git a/internal/crypto/algorithms/aes256cfb.go b/internal/crypto/algorithms/aes256cfb.go index d1151935cc..ec58be286c 100644 --- a/internal/crypto/algorithms/aes256cfb.go +++ b/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) } diff --git a/internal/crypto/algorithms/aes256cfb_test.go b/internal/crypto/algorithms/aes256cfb_test.go index 6e110adc9e..27817a9662 100644 --- a/internal/crypto/algorithms/aes256cfb_test.go +++ b/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) }) } diff --git a/internal/crypto/algorithms/aes256gcm.go b/internal/crypto/algorithms/aes256gcm.go index 1ec8e57fcc..6f0232f1da 100644 --- a/internal/crypto/algorithms/aes256gcm.go +++ b/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 } diff --git a/internal/crypto/algorithms/aes256gcm_test.go b/internal/crypto/algorithms/aes256gcm_test.go index 0bd785b1d2..db2e19f64e 100644 --- a/internal/crypto/algorithms/aes256gcm_test.go +++ b/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" diff --git a/internal/crypto/algorithms/algorithm.go b/internal/crypto/algorithms/algorithm.go index 5c53db08f3..c6eeb1295d 100644 --- a/internal/crypto/algorithms/algorithm.go +++ b/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 diff --git a/internal/crypto/engine.go b/internal/crypto/engine.go index 7806677563..3a7034c40b 100644 --- a/internal/crypto/engine.go +++ b/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 } diff --git a/internal/crypto/engine_test.go b/internal/crypto/engine_test.go index 9f5ba50a1e..cfcbd81af2 100644 --- a/internal/crypto/engine_test.go +++ b/internal/crypto/engine_test.go @@ -4,6 +4,8 @@ package crypto import ( + "encoding/base64" + "os" "testing" "github.com/stretchr/testify/assert" @@ -14,9 +16,13 @@ import ( "github.com/mindersec/minder/pkg/config/server" ) -//Test both the algorithm and the engine in one test suite -// TODO: if we add additional algorithms in future, we should split up testing +var config = &server.Config{ + Auth: server.AuthConfig{ + TokenKey: "./testdata/test_encryption_key", + }, +} +// Test both the algorithm and the engine in one test suite func TestNewFromCryptoConfig(t *testing.T) { t.Parallel() @@ -211,8 +217,121 @@ func TestDecryptFailedDecryption(t *testing.T) { require.ErrorIs(t, err, ErrDecrypt) } -var config = &server.Config{ - Auth: server.AuthConfig{ - TokenKey: "./testdata/test_encryption_key", - }, +func TestEngineGeneratesUniqueSalts(t *testing.T) { + t.Parallel() + + engine, err := NewEngineFromConfig(config) + require.NoError(t, err) + + const sampleData = "Super Secret Password" + + encrypted1, err := engine.EncryptString(sampleData) + require.NoError(t, err) + + encrypted2, err := engine.EncryptString(sampleData) + require.NoError(t, err) + + assert.NotEqual(t, encrypted1.EncodedData, encrypted2.EncodedData) + assert.NotEqual(t, encrypted1.Salt, encrypted2.Salt) + + decrypted1, err := engine.DecryptString(encrypted1) + require.NoError(t, err) + assert.Equal(t, sampleData, decrypted1) +} + +func TestEngineRoutesLegacyCFBDecryption(t *testing.T) { + t.Parallel() + + engine, err := NewEngineFromConfig(config) + require.NoError(t, err) + + const sampleData = "Legacy CFB Secret" + cfbAlgo := &algorithms.AES256CFBAlgorithm{} + + keyBytes, err := os.ReadFile(config.Auth.TokenKey) + require.NoError(t, err) + + testSalt := []byte("1234567890123456") + rawCiphertext, err := cfbAlgo.Encrypt([]byte(sampleData), keyBytes, testSalt) + require.NoError(t, err) + + legacyPayload := EncryptedData{ + Algorithm: algorithms.Aes256Cfb, + EncodedData: base64.StdEncoding.EncodeToString(rawCiphertext), + KeyVersion: "test_encryption_key", + Salt: base64.StdEncoding.EncodeToString(testSalt), + } + + decrypted, err := engine.DecryptString(legacyPayload) + require.NoError(t, err) + assert.Equal(t, sampleData, decrypted) +} + +func TestGCM_NoDoubleStorage(t *testing.T) { + t.Parallel() + + algo := &algorithms.AES256GCMAlgorithm{} + validKey := []byte("YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWE=") + salt := []byte("1234567890123456") + plaintext := []byte("hello") + + ciphertext, err := algo.Encrypt(plaintext, validKey, salt) + require.NoError(t, err) + + // Verify nonce isn't prepended (Length = plaintext + GCM Tag) + assert.Equal(t, len(plaintext)+16, len(ciphertext)) +} + +func TestGCM_SafelyHandlesKeystoreNewlines(t *testing.T) { + t.Parallel() + + algo := &algorithms.AES256GCMAlgorithm{} + dirtyKey := []byte("YWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWFhYWE=\n") + salt := []byte("1234567890123456") + plaintext := []byte("testing newlines") + + ciphertext, err := algo.Encrypt(plaintext, dirtyKey, salt) + require.NoError(t, err) + + decrypted, err := algo.Decrypt(ciphertext, dirtyKey, salt) + require.NoError(t, err) + assert.Equal(t, plaintext, decrypted) +} + +func TestCFB_LegacySaltFallback(t *testing.T) { + t.Parallel() + + algo := &algorithms.AES256CFBAlgorithm{} + key := []byte("my_raw_password_string\n") + plaintext := []byte("legacy fallback test") + + var emptySalt []byte + ciphertext, err := algo.Encrypt(plaintext, key, emptySalt) + require.NoError(t, err) + + decrypted, err := algo.Decrypt(ciphertext, key, emptySalt) + require.NoError(t, err) + assert.Equal(t, plaintext, decrypted) +} + +func TestCFB_DynamicSalt(t *testing.T) { + t.Parallel() + + algo := &algorithms.AES256CFBAlgorithm{} + + key := []byte("a_very_secure_passphrase\n") + salt := []byte("dynamic_salt_123") + plaintext := []byte("testing dynamic salt with cfb") + + ciphertext, err := algo.Encrypt(plaintext, key, salt) + require.NoError(t, err) + + decrypted, err := algo.Decrypt(ciphertext, key, salt) + require.NoError(t, err) + + assert.Equal(t, string(plaintext), string(decrypted), "CFB should successfully encrypt/decrypt using a dynamic salt") + + wrongSalt := []byte("wrong_salt_45678") + wrongDecrypted, _ := algo.Decrypt(ciphertext, key, wrongSalt) + assert.NotEqual(t, string(plaintext), string(wrongDecrypted), "Decryption with the wrong salt should produce garbage data") } diff --git a/internal/crypto/models.go b/internal/crypto/models.go index a9359168eb..f2b2a32a53 100644 --- a/internal/crypto/models.go +++ b/internal/crypto/models.go @@ -20,6 +20,8 @@ type EncryptedData struct { // An identifier which specifies the key used. // Used to handle multiple keys during key rotation. KeyVersion string + // Salt used during encryption to ensure non-deterministic ciphertexts. + Salt string `json:"salt,omitempty"` } // Serialize converts the contents to JSON. @@ -36,6 +38,7 @@ func NewBackwardsCompatibleEncryptedData(encryptedData string) EncryptedData { Algorithm: algorithms.Aes256Cfb, EncodedData: encryptedData, KeyVersion: "", + Salt: "", } } diff --git a/pkg/config/server/crypto.go b/pkg/config/server/crypto.go index 466db0f0dd..da0fe2e8b1 100644 --- a/pkg/config/server/crypto.go +++ b/pkg/config/server/crypto.go @@ -5,9 +5,10 @@ package server // CryptoConfig is the configuration for the crypto engine type CryptoConfig struct { - KeyStore KeyStoreConfig `mapstructure:"keystore"` - Default DefaultCrypto `mapstructure:"default"` - Fallback FallbackCrypto `mapstructure:"fallback"` + KeyStore KeyStoreConfig `mapstructure:"keystore"` + Default DefaultCrypto `mapstructure:"default"` + Fallback FallbackCrypto `mapstructure:"fallback"` + SupportedAlgorithms []string `mapstructure:"supported_algorithms"` } // KeyStoreConfig specifies the type of keystore to use and its configuration