diff --git a/pkg/secret/aes_encoder.go b/pkg/secret/aes_encoder.go index b2dc9de..804750d 100644 --- a/pkg/secret/aes_encoder.go +++ b/pkg/secret/aes_encoder.go @@ -5,17 +5,42 @@ import ( "crypto/aes" "crypto/cipher" "crypto/rand" + "crypto/subtle" "encoding/binary" "encoding/hex" + "errors" "fmt" "io" - "strings" +) + +const ( + formatVersionLegacyCBC uint16 = 16 + formatVersionAesGCM uint16 = 2 + formatVersionAesGCMYaml uint16 = 3 + + formatVersionSize = 2 + gcmNonceSize = 12 + + // The sealed data ends with one byte holding how much filler precedes it, which lets + // the container size dodge the legacy layout. See fillerSizeFor. + gcmFillerSizeLen = 1 + gcmMaxFillerSize = 1 +) + +var ( + errMinimumDataLength = errors.New("minimum required data length") + errUnpadFailed = errors.New("inconsistent data, unpad failed") + errBlockSizeMultiple = errors.New("data isn't a multiple of the block size") + errAuthenticationFailed = errors.New("authentication failed: data has been tampered with or the encryption key is wrong") + errUnsupportedFormatVersion = errors.New("unsupported secret format version") ) type AesEncoder struct { CipherBlock cipher.Block } +var _ formatAwareEncoder = (*AesEncoder)(nil) + func GenerateAesSecretKey() ([]byte, error) { randomBytes := make([]byte, 16) if _, err := rand.Read(randomBytes); err != nil { @@ -43,23 +68,29 @@ func NewAesEncoder(key []byte) (*AesEncoder, error) { } func (s *AesEncoder) Encrypt(data []byte) ([]byte, error) { - dataToEncrypt := pad(data) + return s.encryptWithFormat(data, formatVersionAesGCM) +} - cipherData := make([]byte, aes.BlockSize+len(dataToEncrypt)) - iv := cipherData[:aes.BlockSize] - if _, err := io.ReadFull(rand.Reader, iv); err != nil { - return nil, err +func (s *AesEncoder) encryptYamlScalar(data []byte) ([]byte, error) { + return s.encryptWithFormat(data, formatVersionAesGCMYaml) +} + +func (s *AesEncoder) encryptWithFormat(data []byte, version uint16) ([]byte, error) { + gcm, err := cipher.NewGCM(s.CipherBlock) + if err != nil { + return nil, fmt.Errorf("initialize aes-gcm: %w", err) } - mode := cipher.NewCBCEncrypter(s.CipherBlock, iv) - mode.CryptBlocks(cipherData[aes.BlockSize:], dataToEncrypt) + args := make([]byte, formatVersionSize+gcmNonceSize) + binary.LittleEndian.PutUint16(args[:formatVersionSize], version) - ivSize := make([]byte, 2) - binary.LittleEndian.PutUint16(ivSize, aes.BlockSize) + nonce := args[formatVersionSize:] + if _, err := io.ReadFull(rand.Reader, nonce); err != nil { + return nil, fmt.Errorf("read random nonce: %w", err) + } - var args []byte - args = append(args, ivSize...) - args = append(args, cipherData...) + // The version prefix is authenticated so that it cannot be altered within this format. + args = gcm.Seal(args, nonce, sealedPlainText(data, gcm.Overhead()), args[:formatVersionSize]) result := make([]byte, hex.EncodedLen(len(args))) hex.Encode(result, args) @@ -68,57 +99,157 @@ func (s *AesEncoder) Encrypt(data []byte) ([]byte, error) { } func (s *AesEncoder) Decrypt(data []byte) ([]byte, error) { + result, _, err := s.decryptWithFormat(data) + return result, err +} + +// Empty input is reported as the legacy format so that callers which restore YAML +// scalar metadata treat the result as an unframed plain value. +func (s *AesEncoder) decryptWithFormat(data []byte) ([]byte, uint16, error) { if len(data) == 0 { - return data, nil + return data, formatVersionLegacyCBC, nil } dataToExtract, err := hexToBinary(data) if err != nil { - return nil, err + return nil, 0, err } - ivLengthInfoSize := 2 - ivSize := aes.BlockSize - paddingMaxSize := aes.BlockSize - minimalDataBinarySize := ivLengthInfoSize + ivSize + paddingMaxSize - minimalDataSize := minimalDataBinarySize * 2 - if len(dataToExtract) < minimalDataBinarySize { // iv + padding - return nil, fmt.Errorf("minimum required data length: '%v'", minimalDataSize) + if len(dataToExtract) < formatVersionSize { + return nil, 0, minimumDataLengthError(legacyCBCMinimumDataBinarySize()) } - iv := dataToExtract[ivLengthInfoSize : ivLengthInfoSize+ivSize] - cipherText := dataToExtract[ivLengthInfoSize+ivSize:] + version := binary.LittleEndian.Uint16(dataToExtract[:formatVersionSize]) + + switch version { + case formatVersionLegacyCBC: + result, err := s.decryptLegacyCBC(dataToExtract) + return result, version, err + case formatVersionAesGCM, formatVersionAesGCMYaml: + result, err := s.decryptAesGCM(dataToExtract) + return result, version, err + default: + return nil, version, fmt.Errorf("%w: %d", errUnsupportedFormatVersion, version) + } +} + +func (s *AesEncoder) decryptLegacyCBC(dataToExtract []byte) ([]byte, error) { + if len(dataToExtract) < legacyCBCMinimumDataBinarySize() { + return nil, minimumDataLengthError(legacyCBCMinimumDataBinarySize()) + } + + iv := dataToExtract[formatVersionSize : formatVersionSize+aes.BlockSize] + cipherText := dataToExtract[formatVersionSize+aes.BlockSize:] if len(cipherText)%aes.BlockSize != 0 { - return nil, fmt.Errorf("data isn't a multiple of the block size") + return nil, errBlockSizeMultiple } mode := cipher.NewCBCDecrypter(s.CipherBlock, iv) mode.CryptBlocks(cipherText, cipherText) - result, err := unpad(cipherText) + return unpad(cipherText) +} + +func (s *AesEncoder) decryptAesGCM(dataToExtract []byte) ([]byte, error) { + gcm, err := cipher.NewGCM(s.CipherBlock) if err != nil { - return nil, err + return nil, fmt.Errorf("initialize aes-gcm: %w", err) } - return result, nil + minimumDataBinarySize := gcmContainerSize(0, 0, gcm.Overhead()) + if len(dataToExtract) < minimumDataBinarySize { + return nil, minimumDataLengthError(minimumDataBinarySize) + } + if matchesLegacyLayout(len(dataToExtract)) { + return nil, errAuthenticationFailed + } + + nonce := dataToExtract[formatVersionSize : formatVersionSize+gcmNonceSize] + cipherText := dataToExtract[formatVersionSize+gcmNonceSize:] + + result, err := gcm.Open(nil, nonce, cipherText, dataToExtract[:formatVersionSize]) + if err != nil { + return nil, errAuthenticationFailed + } + + return unfill(result, gcm.Overhead()) +} + +// A container whose size matches the legacy layout could be handed to the CBC reader by +// rewriting its version prefix, and that reader cannot authenticate. Padding the sealed +// data by one byte in exactly those cases keeps every version-2 container off the legacy +// grid, so such a rewrite always fails on size alone. +func sealedPlainText(data []byte, overhead int) []byte { + filler := fillerSizeFor(len(data), overhead) + + result := make([]byte, 0, len(data)+filler+gcmFillerSizeLen) + result = append(result, data...) + result = append(result, bytes.Repeat([]byte{0}, filler)...) + result = append(result, byte(filler)) + + return result +} + +func fillerSizeFor(dataSize, overhead int) int { + if matchesLegacyLayout(gcmContainerSize(dataSize, 0, overhead)) { + return 1 + } + + return 0 +} + +func gcmContainerSize(dataSize, filler, overhead int) int { + return formatVersionSize + gcmNonceSize + dataSize + filler + gcmFillerSizeLen + overhead +} + +func matchesLegacyLayout(containerSize int) bool { + return containerSize >= legacyCBCMinimumDataBinarySize() && + (containerSize-formatVersionSize-aes.BlockSize)%aes.BlockSize == 0 +} + +func unfill(sealed []byte, overhead int) ([]byte, error) { + if len(sealed) < gcmFillerSizeLen { + return nil, errAuthenticationFailed + } + + filler := int(sealed[len(sealed)-1]) + if filler > gcmMaxFillerSize || len(sealed) < filler+gcmFillerSizeLen { + return nil, errAuthenticationFailed + } + + dataSize := len(sealed) - filler - gcmFillerSizeLen + if filler != fillerSizeFor(dataSize, overhead) || !bytes.Equal(sealed[dataSize:len(sealed)-gcmFillerSizeLen], bytes.Repeat([]byte{0}, filler)) { + return nil, errAuthenticationFailed + } + + return sealed[:dataSize], nil +} + +func legacyCBCMinimumDataBinarySize() int { + return formatVersionSize + aes.BlockSize + aes.BlockSize } -func pad(data []byte) []byte { - padding := aes.BlockSize - len(data)%aes.BlockSize - padtext := bytes.Repeat([]byte{byte(padding)}, padding) - return append(data, padtext...) +func minimumDataLengthError(minimumDataBinarySize int) error { + return fmt.Errorf("%w: '%v'", errMinimumDataLength, minimumDataBinarySize*2) } func unpad(data []byte) ([]byte, error) { length := len(data) + if length == 0 { + return nil, errUnpadFailed + } + unpadding := int(data[length-1]) + if unpadding == 0 || unpadding > aes.BlockSize || unpadding > length { + return nil, errUnpadFailed + } - if unpadding > length { - return nil, fmt.Errorf("inconsistent data, unpad failed") + if subtle.ConstantTimeCompare(data[length-unpadding:], bytes.Repeat([]byte{byte(unpadding)}, unpadding)) != 1 { + return nil, errUnpadFailed } - return data[:(length - unpadding)], nil + return data[:length-unpadding], nil } func hexToBinary(data []byte) ([]byte, error) { @@ -131,16 +262,7 @@ func hexToBinary(data []byte) ([]byte, error) { } func IsExtractDataError(err error) bool { - dataErrorPrefixes := []string{ - "minimum required data length", - "encoding/hex: odd length hex string", - } - - for _, prefix := range dataErrorPrefixes { - if strings.HasPrefix(err.Error(), prefix) { - return true - } - } - - return false + return errors.Is(err, errMinimumDataLength) || + errors.Is(err, hex.ErrLength) || + errors.Is(err, errUnsupportedFormatVersion) } diff --git a/pkg/secret/aes_encoder_format_test.go b/pkg/secret/aes_encoder_format_test.go new file mode 100644 index 0000000..0ae36fe --- /dev/null +++ b/pkg/secret/aes_encoder_format_test.go @@ -0,0 +1,375 @@ +package secret + +import ( + "bytes" + "crypto/aes" + "crypto/cipher" + "encoding/binary" + "encoding/hex" + "errors" + "fmt" + "strings" + "testing" +) + +func TestAesEncoderWritesAesGCMFormatVersion(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + encodedData, err := s.Encrypt([]byte("value")) + if err != nil { + t.Fatal(err) + } + + if prefix := string(encodedData[:4]); prefix != "0200" { + t.Errorf("\n[EXPECTED]: %s\n[GOT]: %s", "0200", prefix) + } +} + +// A version-2 ciphertext is shorter than the legacy minimum of 34 binary bytes, so a +// minimum-length check left ahead of the version dispatch would reject valid data. +func TestAesEncoderShortPlaintextRoundTrip(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + for _, plainText := range []string{"", "a", "ab", "abc"} { + t.Run(fmt.Sprintf("%d bytes", len(plainText)), func(t *testing.T) { + encodedData, err := s.Encrypt([]byte(plainText)) + if err != nil { + t.Fatal(err) + } + + if plainText == "" && hex.DecodedLen(len(encodedData)) >= legacyCBCMinimumDataBinarySize() { + t.Errorf("the smallest ciphertext is %d binary bytes, at or above the legacy minimum of %d, so this no longer exercises a below-minimum ciphertext", hex.DecodedLen(len(encodedData)), legacyCBCMinimumDataBinarySize()) + } + + result, err := s.Decrypt(encodedData) + if err != nil { + t.Fatal(err) + } + + if string(result) != plainText { + t.Errorf("\n[EXPECTED]: %q\n[GOT]: %q", plainText, string(result)) + } + }) + } +} + +func TestAesEncoderRejectsEveryBitFlip(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + encodedData, err := s.Encrypt([]byte("postgres://user:hunter2@db:5432/app")) + if err != nil { + t.Fatal(err) + } + + original, err := hexToBinary(encodedData) + if err != nil { + t.Fatal(err) + } + + for bytePos := 0; bytePos < len(original); bytePos++ { + for bit := 0; bit < 8; bit++ { + tampered := make([]byte, len(original)) + copy(tampered, original) + tampered[bytePos] ^= 1 << bit + + tamperedHex := make([]byte, hex.EncodedLen(len(tampered))) + hex.Encode(tamperedHex, tampered) + + // Flips inside the version prefix produce an unsupported-version error rather + // than an authentication failure, so any error is an acceptable rejection. + if _, err := s.Decrypt(tamperedHex); err == nil { + t.Fatalf("tampered ciphertext accepted: byte %d bit %d", bytePos, bit) + } + } + } +} + +// Because no container sits on the legacy grid, rewriting the version prefix is now +// rejected for every plaintext length rather than only for most of them. +// +// The rejection must come from the size or block check, never from unpad: reaching unpad +// would mean the blob was decrypted with an unauthenticated key stream first, and whether +// that lands on valid padding is a matter of chance. Asserting only that some error came +// back would accept that outcome roughly 995 times out of 1000 and hide the regression. +func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + for dataSize := 0; dataSize <= 200; dataSize++ { + plainText := bytes.Repeat([]byte("s"), dataSize) + + encodedData, err := s.Encrypt(plainText) + if err != nil { + t.Fatal(err) + } + + raw, err := hexToBinary(encodedData) + if err != nil { + t.Fatal(err) + } + + binary.LittleEndian.PutUint16(raw[:formatVersionSize], formatVersionLegacyCBC) + + downgraded := make([]byte, hex.EncodedLen(len(raw))) + hex.Encode(downgraded, raw) + + _, err = s.Decrypt(downgraded) + if err == nil { + t.Fatalf("a version-downgraded ciphertext of a %d-byte plaintext was accepted", dataSize) + } + + if !errors.Is(err, errMinimumDataLength) && !errors.Is(err, errBlockSizeMultiple) { + t.Fatalf("a version-downgraded ciphertext of a %d-byte plaintext reached the legacy key stream instead of failing on shape: %v", dataSize, err) + } + } +} + +func TestAesEncoderRejectsNonCanonicalFiller(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + for _, test := range []struct { + name string + sealed []byte + }{ + {name: "missing required filler", sealed: []byte("abc\x00")}, + {name: "missing required filler at the next legacy layout", sealed: append(bytes.Repeat([]byte("a"), 19), 0)}, + {name: "nonzero filler", sealed: []byte("abc\xff\x01")}, + {name: "unnecessary zero filler", sealed: []byte("abcd\x00\x01")}, + } { + t.Run(test.name, func(t *testing.T) { + _, err := s.Decrypt(aesGCMEncoded(t, s, test.sealed)) + if !errors.Is(err, errAuthenticationFailed) { + t.Fatalf("expected an authentication failure, got: %v", err) + } + }) + } +} + +func aesGCMEncoded(t *testing.T, s *AesEncoder, sealed []byte) []byte { + t.Helper() + + gcm, err := cipher.NewGCM(s.CipherBlock) + if err != nil { + t.Fatal(err) + } + + raw := make([]byte, formatVersionSize+gcm.NonceSize()) + binary.LittleEndian.PutUint16(raw[:formatVersionSize], formatVersionAesGCM) + + nonce := raw[formatVersionSize:] + for i := range nonce { + nonce[i] = byte(i) + } + + raw = gcm.Seal(raw, nonce, sealed, raw[:formatVersionSize]) + + encoded := make([]byte, hex.EncodedLen(len(raw))) + hex.Encode(encoded, raw) + + return encoded +} + +func TestAesEncoderRejectsWrongKey(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + other, err := NewAesEncoder([]byte("bc3458408a5687e60b9417adb84e4ad0")) + if err != nil { + t.Fatal(err) + } + + encodedData, err := s.Encrypt([]byte("value")) + if err != nil { + t.Fatal(err) + } + + _, err = other.Decrypt(encodedData) + if !errors.Is(err, errAuthenticationFailed) { + t.Errorf("expected an authentication failure, got: %v", err) + } + + if IsExtractDataError(err) { + t.Error("an authentication failure must not be reported as a data error, so callers keep advising to check the encryption key") + } +} + +func TestAesEncoderRejectsUnsupportedFormatVersion(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + encodedData, err := s.Encrypt([]byte("value")) + if err != nil { + t.Fatal(err) + } + + unsupported := append([]byte("0400"), encodedData[4:]...) + + _, err = s.Decrypt(unsupported) + if !errors.Is(err, errUnsupportedFormatVersion) { + t.Fatalf("expected an unsupported version error, got: %v", err) + } + + if !strings.Contains(err.Error(), "4") { + t.Errorf("expected the rejected version in the message, got: %v", err) + } + + if !IsExtractDataError(err) { + t.Error("an unsupported format version is a data error") + } +} + +func TestAesEncoderDecryptWithFormatReportsVersion(t *testing.T) { + s, err := NewAesEncoder(legacyFixtureKey) + if err != nil { + t.Fatal(err) + } + + _, version, err := s.decryptWithFormat(legacyFixtureSecretFile) + if err != nil { + t.Fatal(err) + } + + if version != formatVersionLegacyCBC { + t.Errorf("\n[EXPECTED]: %d\n[GOT]: %d", formatVersionLegacyCBC, version) + } + + encodedData, err := s.Encrypt([]byte("value")) + if err != nil { + t.Fatal(err) + } + + _, version, err = s.decryptWithFormat(encodedData) + if err != nil { + t.Fatal(err) + } + + if version != formatVersionAesGCM { + t.Errorf("\n[EXPECTED]: %d\n[GOT]: %d", formatVersionAesGCM, version) + } +} + +func TestUnpad(t *testing.T) { + tests := []struct { + name string + data []byte + expected string + expectError bool + }{ + { + name: "valid single byte of padding", + data: append(bytes.Repeat([]byte("a"), aes.BlockSize-1), 0x01), + expected: strings.Repeat("a", aes.BlockSize-1), + }, + { + name: "valid full block of padding", + data: bytes.Repeat([]byte{byte(aes.BlockSize)}, aes.BlockSize), + expected: "", + }, + { + name: "zero padding length", + data: append(bytes.Repeat([]byte("a"), aes.BlockSize-1), 0x00), + expectError: true, + }, + { + name: "padding length above the block size", + data: append(bytes.Repeat([]byte("a"), 2*aes.BlockSize-1), byte(aes.BlockSize+1)), + expectError: true, + }, + { + name: "padding length above the data length", + data: append(bytes.Repeat([]byte("a"), 3), 0x08), + expectError: true, + }, + { + name: "inconsistent padding bytes", + data: append(bytes.Repeat([]byte("a"), aes.BlockSize-4), 0x04, 0xff, 0x04, 0x04), + expectError: true, + }, + { + name: "empty data", + data: []byte{}, + expectError: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result, err := unpad(test.data) + + if test.expectError { + if !errors.Is(err, errUnpadFailed) { + t.Errorf("expected an unpad failure, got: %v", err) + } + return + } + + if err != nil { + t.Fatal(err) + } + + if string(result) != test.expected { + t.Errorf("\n[EXPECTED]: %q\n[GOT]: %q", test.expected, string(result)) + } + }) + } +} + +func TestIsExtractDataError(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + { + name: "minimum data length", + err: minimumDataLengthError(legacyCBCMinimumDataBinarySize()), + expected: true, + }, + { + name: "odd length hex string", + err: fmt.Errorf("wrapped: %w", hex.ErrLength), + expected: true, + }, + { + name: "unsupported format version", + err: fmt.Errorf("%w: %d", errUnsupportedFormatVersion, 3), + expected: true, + }, + { + name: "authentication failure", + err: errAuthenticationFailed, + expected: false, + }, + { + name: "unpad failure", + err: errUnpadFailed, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := IsExtractDataError(test.err); got != test.expected { + t.Errorf("\n[EXPECTED]: %v\n[GOT]: %v", test.expected, got) + } + }) + } +} diff --git a/pkg/secret/doc.go b/pkg/secret/doc.go new file mode 100644 index 0000000..f746e3d --- /dev/null +++ b/pkg/secret/doc.go @@ -0,0 +1,83 @@ +// Package secret encrypts and decrypts werf secrets, either as whole blobs or as the +// individual scalar values of a YAML document. +// +// # Encoded format +// +// An encoded value is the hex encoding of a binary payload that starts with a two-byte +// little-endian format version: +// +// version 16: [version][16-byte IV][AES-CBC ciphertext, PKCS#7 padded] +// version 2: [version][12-byte nonce][AES-GCM whole-blob ciphertext and authentication tag] +// version 3: [version][12-byte nonce][AES-GCM YAML scalar ciphertext and authentication tag] +// +// What each AES-GCM format seals is not the value on its own but: +// +// [value][filler, 0 or 1 zero bytes][one byte holding the filler size] +// +// The filler is present only when the container would otherwise be the size of a legacy +// container, and exists purely to keep the two formats apart; see Authentication below. +// It is inside the sealed data, so it is authenticated along with the value. +// +// Version 16 is the legacy format. Those two bytes originally held the CBC IV size, were +// always written as 16, and were never read back, which is why the field could be +// repurposed as a version without changing the layout of existing data. It is read-only: +// it still decrypts exactly as it always did, but it is never written any more. +// +// Version 2 is AES-GCM and is what whole-blob Encrypt writes. EncryptYamlData writes +// version 3, whose plaintext also holds the scalar metadata. Any other version is rejected +// with an error rather than being decrypted as CBC. +// +// Both AES-GCM versions are read with the same secret key, so upgrading needs no new key and no +// migration. The key format is unchanged: 16, 24 or 32 random bytes hex-encoded, as +// produced by GenerateAesSecretKey. +// +// # Compatibility +// +// Reading is backward compatible, but writing is not forward compatible: a value written +// in version 2 or 3 cannot be read by a werf or nelm release that predates AES-GCM support, +// because those releases ignore the version field and decrypt everything as CBC. There is +// no way to write the legacy format any more, so a repository whose secrets have been +// re-encrypted needs every consumer, including CI jobs and saved deploy plans, to +// understand AES-GCM formats. Re-encrypting everything at once is what rotate-secret-key does. +// +// # Authentication +// +// The legacy CBC format has no integrity check, so a corrupted or deliberately modified +// ciphertext could decrypt to garbage instead of failing, and a wrong key was often +// accepted. AES-GCM authenticates, so tampering and wrong keys are reported as errors. +// This protects newly written values only; existing values gain it once re-encrypted. +// +// The version prefix of an AES-GCM value is authenticated, so it cannot be altered within +// that format. On its own that would not be enough, because rewriting the prefix +// to 16 hands the value to the legacy CBC reader, which by definition does not +// authenticate and would never consult it. That is what the filler is for: an AES-GCM +// container is never the size of a legacy one, so a rewritten prefix always fails the +// legacy size and block checks. Such a value is rejected outright, for every possible +// value length, rather than merely most of the time. +// +// What remains is that the legacy format itself stays readable. Anyone able to rewrite +// those two bytes can instead replace the whole value with a legacy blob of their own, +// and that has a small chance of being accepted, returning unpredictable garbage rather +// than anything they choose, since they do not hold the key. So the exposure is the +// readable unauthenticated format, not any particular way of reaching it, and that is the +// price of not breaking existing data. Re-encrypting with rotate-secret-key does not +// change it either, because the legacy reader has to stay for as long as any legacy value +// might exist anywhere. +// +// # YAML scalars +// +// EncryptYamlData and DecryptYamlData encrypt each scalar leaf of a document in place. +// From version 3 on, the YAML tag and the scalar style are stored inside the encrypted +// payload, so a number stays a number and a block scalar keeps its style across a round +// trip. Whole-blob Encrypt and Decrypt never add this framing. +// +// Values encrypted before version 3 did not store a tag, so that information does not +// exist anywhere and cannot be recovered: they keep decrypting as strings. Re-encrypting +// does not help, because by then the original type is already lost. The only way to give +// such a value its intended type is to re-enter it, for example through +// "werf helm secret values edit". +// +// A comment attached to an encrypted value is preserved, which means it stays cleartext in +// the encrypted file, the same way mapping keys already do. Do not put secrets in +// comments. +package secret diff --git a/pkg/secret/encoder.go b/pkg/secret/encoder.go index 9814ff0..9b4b3c8 100644 --- a/pkg/secret/encoder.go +++ b/pkg/secret/encoder.go @@ -4,3 +4,11 @@ type Encoder interface { Encrypt(data []byte) ([]byte, error) Decrypt(encodedData []byte) ([]byte, error) } + +// formatAwareEncoder is implemented by encoders whose ciphertext carries a format +// version. YamlEncoder only stores and restores YAML scalar metadata when its Encoder +// implements this, so that any other Encoder keeps producing and consuming plain values. +type formatAwareEncoder interface { + encryptYamlScalar(data []byte) ([]byte, error) + decryptWithFormat(encodedData []byte) ([]byte, uint16, error) +} diff --git a/pkg/secret/legacy_compat_test.go b/pkg/secret/legacy_compat_test.go new file mode 100644 index 0000000..86b7881 --- /dev/null +++ b/pkg/secret/legacy_compat_test.go @@ -0,0 +1,76 @@ +package secret + +import ( + "testing" +) + +// Ciphertexts and key below are copied verbatim from werf's committed e2e fixtures +// (test/e2e/converge/_fixtures/complex/state0). They are the backward-compatibility +// contract for the legacy AES-CBC format and must never be regenerated. +var ( + legacyFixtureKey = []byte("bc3458408a5687e60b9417adb84e4ad0") + + legacyFixtureSecretValues = "added_via_secret_values: 10007b717b44ec49b722c5d517cf6259bd20c93ea5ef2dcfc3934bccee59613d0ac07f5f39835d30f3736fc5d3edc10335bb\n" + + "overridden_via_secret_values: 1000ad8725d33000e7bf81a5b65851003b9ca35e999b617ccff1fe402c7a941382f4fc5c9257de633a42427958b5fb43b2e3\n" + + legacyFixtureSecretValuesExtra = "added_via_secret_values_extra: 1000dc58aec55b8ce919d24fd1a9c5435f983d9a39f1fba0971d2452ae30d03788dffd4ff2d179b426e62badec66c12e2f7d\n" + + "overridden_via_secret_values_extra: 1000b52f32b4f9a78fb8b53e2a4d4211408b0de9c9bc83a8cda7a6be77e288f307bf90d016b476960f3c99ab51be48b318fcb4f404a4559bec045631902ec5f49728\n" + + legacyFixtureSecretFile = []byte("100052fb0fa1cc8ef1cb123be089cdb853cc153772691b8fb743d612a7b64d65614d4d8745250752564941227eb8d7161523") +) + +func TestLegacyFixtureYamlDataDecrypts(t *testing.T) { + tests := []struct { + name string + data string + expected string + }{ + { + name: "secret-values.yaml", + data: legacyFixtureSecretValues, + expected: "added_via_secret_values: added_via_secret_values\noverridden_via_secret_values: overridden_via_secret_values\n", + }, + { + name: "secret-values-extra.yaml", + data: legacyFixtureSecretValuesExtra, + expected: "added_via_secret_values_extra: added_via_secret_values_extra\noverridden_via_secret_values_extra: overridden_via_secret_values_extra\n", + }, + } + + encoder, err := NewAesEncoder(legacyFixtureKey) + if err != nil { + t.Fatal(err) + } + + yamlEncoder := NewYamlEncoder(encoder) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result, err := yamlEncoder.DecryptYamlData([]byte(test.data)) + if err != nil { + t.Fatal(err) + } + + if string(result) != test.expected { + t.Errorf("\n[EXPECTED]: %q\n[GOT]: %q", test.expected, string(result)) + } + }) + } +} + +func TestLegacyFixtureSecretFileDecrypts(t *testing.T) { + encoder, err := NewAesEncoder(legacyFixtureKey) + if err != nil { + t.Fatal(err) + } + + result, err := encoder.Decrypt(legacyFixtureSecretFile) + if err != nil { + t.Fatal(err) + } + + expected := "secretConfigContent\n" + if string(result) != expected { + t.Errorf("\n[EXPECTED]: %q\n[GOT]: %q", expected, string(result)) + } +} diff --git a/pkg/secret/yaml_encoder.go b/pkg/secret/yaml_encoder.go index d2995e8..7ddfdb6 100644 --- a/pkg/secret/yaml_encoder.go +++ b/pkg/secret/yaml_encoder.go @@ -3,6 +3,7 @@ package secret import ( "bytes" "fmt" + "strconv" yaml_v3 "gopkg.in/yaml.v3" ) @@ -11,8 +12,9 @@ import ( type YamlEncoder struct { Encoder Encoder - generateFunc func([]byte) ([]byte, error) - extractFunc func([]byte) ([]byte, error) + generateFunc func([]byte) ([]byte, error) + extractFunc func([]byte) ([]byte, error) + formatAware formatAwareEncoder } func NewYamlEncoder(encoder Encoder) *YamlEncoder { @@ -21,6 +23,10 @@ func NewYamlEncoder(encoder Encoder) *YamlEncoder { if encoder != nil { yamlEncoder.generateFunc = encoder.Encrypt yamlEncoder.extractFunc = encoder.Decrypt + + if formatAware, ok := encoder.(formatAwareEncoder); ok { + yamlEncoder.formatAware = formatAware + } } else { yamlEncoder.generateFunc = doNothing yamlEncoder.extractFunc = doNothing @@ -39,7 +45,12 @@ func (s *YamlEncoder) Encrypt(data []byte) ([]byte, error) { } func (s *YamlEncoder) EncryptYamlData(data []byte) ([]byte, error) { - resultData, err := doYamlDataV2(s.generateFunc, data, encryptYamlMode) + generateFunc := s.generateFunc + if s.formatAware != nil { + generateFunc = s.formatAware.encryptYamlScalar + } + + resultData, err := doYamlDataV2(generateFunc, s.formatAware, data, encryptYamlMode) if err != nil { return nil, fmt.Errorf("encryption failed: check encryption key and data: %w", err) } @@ -61,7 +72,7 @@ func (s *YamlEncoder) Decrypt(data []byte) ([]byte, error) { } func (s *YamlEncoder) DecryptYamlData(data []byte) ([]byte, error) { - resultData, err := doYamlDataV2(s.extractFunc, data, decryptYamlMode) + resultData, err := doYamlDataV2(s.extractFunc, s.formatAware, data, decryptYamlMode) if err != nil { if IsExtractDataError(err) { return nil, fmt.Errorf("decryption failed: check data `%s`: %w", string(data), err) @@ -73,14 +84,14 @@ func (s *YamlEncoder) DecryptYamlData(data []byte) ([]byte, error) { return resultData, nil } -func doYamlDataV2(doFunc func([]byte) ([]byte, error), data []byte, mode yamlProcessorMode) ([]byte, error) { +func doYamlDataV2(doFunc func([]byte) ([]byte, error), formatAware formatAwareEncoder, data []byte, mode yamlProcessorMode) ([]byte, error) { var config yaml_v3.Node if err := yaml_v3.Unmarshal(data, &config); err != nil { return nil, fmt.Errorf("unable to unmarshal config data: %w", err) } - resultConfig, err := doYamlValueSecretV2(doFunc, deepCopyNode(&config), mode) + resultConfig, err := doYamlValueSecretV2(doFunc, formatAware, deepCopyNode(&config), mode) if err != nil { return nil, fmt.Errorf("unable to process config secrets: %w", err) } @@ -132,11 +143,11 @@ func deepCopyNode(node *yaml_v3.Node) *yaml_v3.Node { return copyNode } -func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node, mode yamlProcessorMode) (*yaml_v3.Node, error) { +func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), formatAware formatAwareEncoder, node *yaml_v3.Node, mode yamlProcessorMode) (*yaml_v3.Node, error) { switch node.Kind { case yaml_v3.DocumentNode: for pos := 0; pos < len(node.Content); pos += 1 { - newValueNode, err := doYamlValueSecretV2(doFunc, deepCopyNode(node.Content[pos]), mode) + newValueNode, err := doYamlValueSecretV2(doFunc, formatAware, deepCopyNode(node.Content[pos]), mode) if err != nil { return nil, fmt.Errorf("unable to process document key %d: %w", pos, err) } @@ -147,7 +158,7 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node for pos := 0; pos < len(node.Content); pos += 2 { keyNode := node.Content[pos] valueNode := node.Content[pos+1] - newValueNode, err := doYamlValueSecretV2(doFunc, deepCopyNode(valueNode), mode) + newValueNode, err := doYamlValueSecretV2(doFunc, formatAware, deepCopyNode(valueNode), mode) if err != nil { return nil, fmt.Errorf("unable to process map key %q value=%v: %w", keyNode.Value, valueNode.Value, err) } @@ -156,7 +167,7 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node case yaml_v3.SequenceNode: for pos := 0; pos < len(node.Content); pos += 1 { - newValueNode, err := doYamlValueSecretV2(doFunc, deepCopyNode(node.Content[pos]), mode) + newValueNode, err := doYamlValueSecretV2(doFunc, formatAware, deepCopyNode(node.Content[pos]), mode) if err != nil { return nil, fmt.Errorf("unable to process array key %d: %w", pos, err) } @@ -164,7 +175,7 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node } case yaml_v3.AliasNode: - newAliasNode, err := doYamlValueSecretV2(doFunc, deepCopyNode(node.Alias), mode) + newAliasNode, err := doYamlValueSecretV2(doFunc, formatAware, deepCopyNode(node.Alias), mode) if err != nil { return nil, fmt.Errorf("unable to process an alias node %q: %w", node.Value, err) } @@ -184,40 +195,43 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node return nil, fmt.Errorf("unable to decode string value %q: %w", node.Value, err) } + if formatAware != nil { + return node, decryptScalarWithMetadata(formatAware, node, value) + } + newValue, err := doFunc([]byte(value)) if err != nil { return nil, err } - if err := node.Encode(string(newValue)); err != nil { - return nil, fmt.Errorf("unable to encode string value %q: %w", string(newValue), err) + if err := encodeScalarPreservingComments(node, string(newValue)); err != nil { + return nil, err } default: return nil, fmt.Errorf("unable to decode non string value %q: expected encoded value as hex string", node.Value) } case encryptYamlMode: - // FIXME: support all types, by node.ShortTag() - switch node.ShortTag() { case "!!null": // ignore default: - var value interface{} - - if err := node.Decode(&value); err != nil { - return nil, fmt.Errorf("unable to decode string value %q: %w", node.Value, err) + plainText, err := scalarPlainText(formatAware, node) + if err != nil { + return nil, err } - // FIXME: this is compatibility mode with previous werf version - newValue, err := doFunc([]byte(fmt.Sprintf("%v", value))) + newValue, err := doFunc(plainText) if err != nil { return nil, err } - if err := node.Encode(string(newValue)); err != nil { - return nil, fmt.Errorf("unable to encode string value %q: %w", string(newValue), err) + // The ciphertext is always emitted as an ordinary string scalar. Carrying the + // original tag over would make older readers reject the node, and carrying a + // folded style over would let the emitter fold line breaks into the hex. + if err := encodeScalarPreservingComments(node, string(newValue)); err != nil { + return nil, err } } } @@ -226,4 +240,85 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node return node, nil } +func scalarPlainText(formatAware formatAwareEncoder, node *yaml_v3.Node) ([]byte, error) { + if formatAware != nil { + return frameScalar(node.ShortTag(), node.Style, node.Value), nil + } + + var value interface{} + if err := node.Decode(&value); err != nil { + return nil, fmt.Errorf("unable to decode string value %q: %w", node.Value, err) + } + + return []byte(fmt.Sprintf("%v", value)), nil +} + +func decryptScalarWithMetadata(formatAware formatAwareEncoder, node *yaml_v3.Node, value string) error { + plainText, version, err := formatAware.decryptWithFormat([]byte(value)) + if err != nil { + return err + } + + if version != formatVersionAesGCMYaml { + return encodeScalarPreservingComments(node, string(plainText)) + } + + tag, style, originalValue, err := unframeScalar(plainText) + if err != nil { + return err + } + + node.Kind = yaml_v3.ScalarNode + node.Tag = tag + node.Style = style + node.Value = originalValue + node.Content = nil + node.Alias = nil + + return nil +} + +func encodeScalarPreservingComments(node *yaml_v3.Node, value string) error { + headComment, lineComment, footComment := node.HeadComment, node.LineComment, node.FootComment + + if err := node.Encode(value); err != nil { + return fmt.Errorf("unable to encode string value %q: %w", value, err) + } + + node.HeadComment, node.LineComment, node.FootComment = headComment, lineComment, footComment + + return nil +} + +const scalarFrameSeparator = 0 + +// frameScalar stores the YAML metadata of a scalar next to its raw value so that the tag +// and style survive a round trip. The value comes last and is treated as opaque bytes, so +// it may contain separators, newlines or anything else. +func frameScalar(shortTag string, style yaml_v3.Style, value string) []byte { + var payload bytes.Buffer + + payload.WriteString(shortTag) + payload.WriteByte(scalarFrameSeparator) + payload.WriteString(strconv.Itoa(int(style))) + payload.WriteByte(scalarFrameSeparator) + payload.WriteString(value) + + return payload.Bytes() +} + +func unframeScalar(payload []byte) (string, yaml_v3.Style, string, error) { + parts := bytes.SplitN(payload, []byte{scalarFrameSeparator}, 3) + if len(parts) != 3 { + return "", 0, "", fmt.Errorf("malformed encrypted scalar payload: expected tag, style and value") + } + + style, err := strconv.Atoi(string(parts[1])) + if err != nil { + return "", 0, "", fmt.Errorf("unable to parse scalar style %q: %w", string(parts[1]), err) + } + + return string(parts[0]), yaml_v3.Style(style), string(parts[2]), nil +} + func doNothing(data []byte) ([]byte, error) { return data, nil } diff --git a/pkg/secret/yaml_encoder_fidelity_test.go b/pkg/secret/yaml_encoder_fidelity_test.go new file mode 100644 index 0000000..a5337b5 --- /dev/null +++ b/pkg/secret/yaml_encoder_fidelity_test.go @@ -0,0 +1,179 @@ +package secret + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + yaml_v3 "gopkg.in/yaml.v3" +) + +func scalarOf(data string) *yaml_v3.Node { + var document yaml_v3.Node + Expect(yaml_v3.Unmarshal([]byte(data), &document)).To(Succeed()) + return document.Content[0].Content[1] +} + +var _ = Describe("YamlEncoder scalar fidelity", func() { + var encoder *YamlEncoder + + BeforeEach(func() { + aesEncoder, err := NewAesEncoder(AesSecretKey) + Expect(err).NotTo(HaveOccurred()) + encoder = NewYamlEncoder(aesEncoder) + }) + + DescribeTable("tag, style and value of a scalar survive an encrypt then decrypt round trip", + func(data string) { + encrypted, err := encoder.EncryptYamlData([]byte(data)) + Expect(err).NotTo(HaveOccurred()) + + decrypted, err := encoder.DecryptYamlData(encrypted) + Expect(err).NotTo(HaveOccurred()) + + original := scalarOf(data) + restored := scalarOf(string(decrypted)) + + Expect(restored.Value).To(Equal(original.Value), "value") + Expect(restored.ShortTag()).To(Equal(original.ShortTag()), "tag") + Expect(restored.Style).To(Equal(original.Style), "style") + }, + Entry("integer", "v: 123\n"), + Entry("negative integer", "v: -7\n"), + Entry("boolean", "v: true\n"), + Entry("float", "v: 64.5\n"), + Entry("timestamp", "v: 2022-07-15\n"), + Entry("binary", "v: !!binary R0lGODlhDAAMAIQAAP//9/X17unp5WZmZgAAAOfn515eXg==\n"), + Entry("plain string", "v: hello\n"), + Entry("numeric string", "v: \"123\"\n"), + Entry("boolean-like string", "v: \"true\"\n"), + Entry("folded block string", "v: >-\n hello\n world\n"), + Entry("literal block string", "v: |\n line1\n line2\n"), + Entry("empty string", "v: \"\"\n"), + Entry("only newlines", "v: \"\\n\\n\"\n"), + Entry("embedded separator byte", "v: \"a\\0b\\0c\"\n"), + Entry("tabs and carriage returns", "v: \"a\\tb\\r\\nc\"\n"), + Entry("leading whitespace", "v: \" indented\"\n"), + Entry("unicode with combining marks and astral characters", "v: \"паро́ль→\\U0001F510\"\n"), + Entry("colon and hash", "v: \"a: b # c\"\n"), + Entry("long value with double spaces", "v: \"word word word word word word word word word word word word word word word word x y\"\n"), + ) + + It("keeps a null value untouched", func() { + data := "v:\n" + + encrypted, err := encoder.EncryptYamlData([]byte(data)) + Expect(err).NotTo(HaveOccurred()) + Expect(string(encrypted)).To(Equal(data)) + + decrypted, err := encoder.DecryptYamlData(encrypted) + Expect(err).NotTo(HaveOccurred()) + Expect(string(decrypted)).To(Equal(data)) + }) + + It("emits the ciphertext as an ordinary string scalar", func() { + encrypted, err := encoder.EncryptYamlData([]byte("v: 123\n")) + Expect(err).NotTo(HaveOccurred()) + + cipherScalar := scalarOf(string(encrypted)) + Expect(cipherScalar.ShortTag()).To(Equal("!!str")) + Expect(cipherScalar.Style).To(Equal(yaml_v3.Style(0))) + Expect(cipherScalar.Value).To(HavePrefix("0300")) + }) + + It("preserves comments attached to a value and to a key", func() { + data := "# head comment on key\nv: 123 # line comment on value\n" + + encrypted, err := encoder.EncryptYamlData([]byte(data)) + Expect(err).NotTo(HaveOccurred()) + + decrypted, err := encoder.DecryptYamlData(encrypted) + Expect(err).NotTo(HaveOccurred()) + + var document yaml_v3.Node + Expect(yaml_v3.Unmarshal(decrypted, &document)).To(Succeed()) + + keyNode := document.Content[0].Content[0] + valueNode := document.Content[0].Content[1] + + Expect(keyNode.HeadComment).To(Equal("# head comment on key")) + Expect(valueNode.LineComment).To(Equal("# line comment on value")) + Expect(valueNode.Value).To(Equal("123")) + Expect(valueNode.ShortTag()).To(Equal("!!int")) + }) + + It("restores types through nested mappings, sequences and anchors", func() { + data := "root: &anchor\n count: 3\n enabled: false\n items:\n - 1\n - two\n - 3.5\nalias: *anchor\n" + + encrypted, err := encoder.EncryptYamlData([]byte(data)) + Expect(err).NotTo(HaveOccurred()) + + decrypted, err := encoder.DecryptYamlData(encrypted) + Expect(err).NotTo(HaveOccurred()) + + var restored map[string]interface{} + Expect(yaml_v3.Unmarshal(decrypted, &restored)).To(Succeed()) + + root := restored["root"].(map[string]interface{}) + Expect(root["count"]).To(Equal(3)) + Expect(root["enabled"]).To(Equal(false)) + Expect(root["items"]).To(Equal([]interface{}{1, "two", 3.5})) + }) + + It("leaves values untouched and unframed without an encoder", func() { + data := "v: 0200abcdef\n" + + decrypted, err := NewYamlEncoder(nil).DecryptYamlData([]byte(data)) + Expect(err).NotTo(HaveOccurred()) + Expect(string(decrypted)).To(Equal(data)) + }) + + It("does not frame a whole blob, so a secret file round trips unchanged", func() { + aesEncoder, err := NewAesEncoder(AesSecretKey) + Expect(err).NotTo(HaveOccurred()) + + content := "line1\nline2\n" + + encoded, err := aesEncoder.Encrypt([]byte(content)) + Expect(err).NotTo(HaveOccurred()) + + decoded, err := aesEncoder.Decrypt(encoded) + Expect(err).NotTo(HaveOccurred()) + Expect(string(decoded)).To(Equal(content)) + Expect(string(decoded)).NotTo(ContainSubstring(string([]byte{scalarFrameSeparator}))) + }) + + It("decrypts an unframed whole blob stored in a YAML value", func() { + aesEncoder, err := NewAesEncoder(AesSecretKey) + Expect(err).NotTo(HaveOccurred()) + + blob, err := aesEncoder.Encrypt([]byte("no framing here")) + Expect(err).NotTo(HaveOccurred()) + + decrypted, err := NewYamlEncoder(aesEncoder).DecryptYamlData([]byte("v: " + string(blob) + "\n")) + Expect(err).NotTo(HaveOccurred()) + Expect(scalarOf(string(decrypted)).Value).To(Equal("no framing here")) + }) + + It("does not interpret whole blobs as scalar frames", func() { + aesEncoder, err := NewAesEncoder(AesSecretKey) + Expect(err).NotTo(HaveOccurred()) + + for _, content := range []string{"!!str\x000\x00hello", "!!str\x00"} { + blob, err := aesEncoder.Encrypt([]byte(content)) + Expect(err).NotTo(HaveOccurred()) + + decrypted, err := NewYamlEncoder(aesEncoder).DecryptYamlData([]byte("v: " + string(blob) + "\n")) + Expect(err).NotTo(HaveOccurred()) + Expect(scalarOf(string(decrypted)).Value).To(Equal(content)) + } + }) + + It("still decrypts a legacy ciphertext as a plain string", func() { + legacyEncoder, err := NewAesEncoder(legacyFixtureKey) + Expect(err).NotTo(HaveOccurred()) + + decrypted, err := NewYamlEncoder(legacyEncoder).DecryptYamlData([]byte(legacyFixtureSecretValues)) + Expect(err).NotTo(HaveOccurred()) + + Expect(string(decrypted)).To(Equal("added_via_secret_values: added_via_secret_values\noverridden_via_secret_values: overridden_via_secret_values\n")) + }) +}) diff --git a/pkg/secret/yaml_encoder_test.go b/pkg/secret/yaml_encoder_test.go index 7e94deb..26113aa 100644 --- a/pkg/secret/yaml_encoder_test.go +++ b/pkg/secret/yaml_encoder_test.go @@ -104,8 +104,10 @@ image: `), ) - // TODO: support restoring of original type during decode - It("should encode integer, bool, float, timestamp and binary as string, then convert to string during decode", func() { + // An Encoder without the format-aware capability, such as EncoderMock here, keeps the + // original behaviour of stringifying every scalar. AesEncoder stores the tag and the + // style inside the payload instead, which is covered by "YamlEncoder scalar fidelity". + It("should encode integer, bool, float, timestamp and binary as string, then convert to string during decode, for an encoder without format support", func() { originalData := ` mystring: value mybool: !!bool true diff --git a/pkg/secret/yaml_helpers.go b/pkg/secret/yaml_helpers.go index 2290b99..90c5ae1 100644 --- a/pkg/secret/yaml_helpers.go +++ b/pkg/secret/yaml_helpers.go @@ -101,7 +101,14 @@ func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConf newEncodedConfig.Alias = newAliasNode case yaml_v3.ScalarNode: - if oldConfig.Value == newConfig.Value { + // The tag and the style are part of what gets encrypted, so a change to either of + // them has to produce new ciphertext even when the raw value is untouched. + if oldConfig.Value == newConfig.Value && + oldConfig.ShortTag() == newConfig.ShortTag() && + oldConfig.Style == newConfig.Style { + oldEncodedConfig.HeadComment = newEncodedConfig.HeadComment + oldEncodedConfig.LineComment = newEncodedConfig.LineComment + oldEncodedConfig.FootComment = newEncodedConfig.FootComment return oldEncodedConfig, nil } return newEncodedConfig, nil diff --git a/pkg/secret/yaml_helpers_test.go b/pkg/secret/yaml_helpers_test.go index 29f5d93..0de669a 100644 --- a/pkg/secret/yaml_helpers_test.go +++ b/pkg/secret/yaml_helpers_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + yaml_v3 "gopkg.in/yaml.v3" ) type MergeEncodedYamlTest struct { @@ -480,3 +481,48 @@ hosts: }), ) }) + +var _ = Describe("MergeEncodedYaml scalar metadata", func() { + DescribeTable("reuse the old encoded value only when the value, the tag and the style are all unchanged", + func(oldData, newData, expectedResult string) { + const oldEncodedData = "v: OLD\n" + const newEncodedData = "v: NEW\n" + + res, err := MergeEncodedYaml([]byte(oldData), []byte(newData), []byte(oldEncodedData), []byte(newEncodedData)) + Expect(err).To(Succeed()) + Expect(string(res)).To(Equal(expectedResult)) + }, + Entry("nothing changed", "v: 123\n", "v: 123\n", "v: OLD\n"), + Entry("value changed", "v: 123\n", "v: 124\n", "v: NEW\n"), + Entry("type changed from string to integer", "v: \"123\"\n", "v: 123\n", "v: NEW\n"), + Entry("type changed from integer to string", "v: 123\n", "v: \"123\"\n", "v: NEW\n"), + Entry("style changed from folded block to plain", "v: >-\n hi\n", "v: hi\n", "v: NEW\n"), + Entry("style changed from plain to folded block", "v: hi\n", "v: >-\n hi\n", "v: NEW\n"), + Entry("style changed from plain to double quoted", "v: hi\n", "v: \"hi\"\n", "v: NEW\n"), + Entry("explicit and implicit string tags are equal", "v: hi\n", "v: hi\n", "v: OLD\n"), + ) + + DescribeTable("keeps a comment-only edit while reusing ciphertext", + func(oldData, newData, oldEncodedData, newEncodedData, expected string) { + merged, err := MergeEncodedYaml([]byte(oldData), []byte(newData), []byte(oldEncodedData), []byte(newEncodedData)) + Expect(err).NotTo(HaveOccurred()) + Expect(string(merged)).To(Equal(expected)) + }, + Entry("replaced line comment", "v: value # old\n", "v: value # new\n", "v: OLD # old\n", "v: NEW # new\n", "v: OLD # new\n"), + Entry("removed line comment", "v: value # old\n", "v: value\n", "v: OLD # old\n", "v: NEW\n", "v: OLD\n"), + ) + + It("preserves comments when reusing ciphertext", func() { + oldConfig := &yaml_v3.Node{Kind: yaml_v3.ScalarNode, Tag: "!!str", Value: "value"} + newConfig := &yaml_v3.Node{Kind: yaml_v3.ScalarNode, Tag: "!!str", Value: "value"} + oldEncodedConfig := &yaml_v3.Node{Kind: yaml_v3.ScalarNode, Tag: "!!str", Value: "OLD", HeadComment: "old head", LineComment: "old line", FootComment: "old foot"} + newEncodedConfig := &yaml_v3.Node{Kind: yaml_v3.ScalarNode, Tag: "!!str", Value: "NEW", HeadComment: "new head", LineComment: "new line", FootComment: "new foot"} + + merged, err := MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConfig) + Expect(err).NotTo(HaveOccurred()) + Expect(merged.Value).To(Equal("OLD")) + Expect(merged.HeadComment).To(Equal("new head")) + Expect(merged.LineComment).To(Equal("new line")) + Expect(merged.FootComment).To(Equal("new foot")) + }) +})