From 78f2d1f1d5a547bfe779d56b258c2330ab292631 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:31:57 +0100 Subject: [PATCH 01/12] test(secret): pin legacy AES-CBC format with fixture golden vectors Add regression coverage for the legacy ciphertext format using real ciphertexts and the key copied verbatim from werf's committed e2e fixtures (test/e2e/converge/_fixtures/complex/state0). These vectors are the backward-compatibility contract for the upcoming format-version work: every existing ciphertext on disk starts with the 2-byte little-endian prefix 16 and must keep decrypting unchanged. They are written before any production change so the contract is established against known-good behaviour, and must never be regenerated. Signed-off-by: Aleksei Igrychev --- pkg/secret/legacy_compat_test.go | 76 ++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 pkg/secret/legacy_compat_test.go 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)) + } +} From 03bb3ce01c6530ace7d06e4280634765e16a7cd7 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:36:45 +0100 Subject: [PATCH 02/12] feat(secret): add format version discriminator and AES-GCM encryption The 2-byte little-endian prefix of a ciphertext used to hold the CBC IV size, was always written as 16, and was never read back. Repurpose it as a format version: 16 keeps meaning the legacy AES-CBC layout and 2 now means AES-GCM, which is written by default. Unknown versions are rejected explicitly instead of falling through to CBC. CBC provided no integrity check, so a tampered ciphertext was accepted and returned garbled plaintext, and a wrong key was silently accepted roughly 19% of the time. AES-GCM authenticates, and the surviving legacy read path gets a hardened unpad that rejects a zero padding length, a length above the block size, and inconsistent padding bytes. The minimum-length check now lives inside each version branch: a version-2 ciphertext is only 30 binary bytes at minimum, below the legacy 34, so a shared check would have rejected valid short ciphertexts. Error classification moves from message-prefix matching to sentinel errors with errors.Is, keeping every existing message byte-identical so callers that match on them keep working. An authentication failure stays outside IsExtractDataError so callers keep advising to check the encryption key. Key handling, NewAesEncoder's signature and the public API are unchanged. Signed-off-by: Aleksei Igrychev --- pkg/secret/aes_encoder.go | 148 +++++++++----- pkg/secret/aes_encoder_format_test.go | 283 ++++++++++++++++++++++++++ pkg/secret/encoder.go | 7 + 3 files changed, 391 insertions(+), 47 deletions(-) create mode 100644 pkg/secret/aes_encoder_format_test.go diff --git a/pkg/secret/aes_encoder.go b/pkg/secret/aes_encoder.go index b2dc9de..dce566e 100644 --- a/pkg/secret/aes_encoder.go +++ b/pkg/secret/aes_encoder.go @@ -5,17 +5,36 @@ import ( "crypto/aes" "crypto/cipher" "crypto/rand" + "crypto/subtle" "encoding/binary" "encoding/hex" + "errors" "fmt" "io" - "strings" +) + +const ( + formatVersionLegacyCBC uint16 = 16 + formatVersionAesGCM uint16 = 2 + + formatVersionSize = 2 + gcmNonceSize = 12 +) + +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 _ formatAwareDecrypter = (*AesEncoder)(nil) + func GenerateAesSecretKey() ([]byte, error) { randomBytes := make([]byte, 16) if _, err := rand.Read(randomBytes); err != nil { @@ -43,23 +62,20 @@ func NewAesEncoder(key []byte) (*AesEncoder, error) { } func (s *AesEncoder) Encrypt(data []byte) ([]byte, error) { - dataToEncrypt := pad(data) - - cipherData := make([]byte, aes.BlockSize+len(dataToEncrypt)) - iv := cipherData[:aes.BlockSize] - if _, err := io.ReadFull(rand.Reader, iv); err != nil { - return nil, err + 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], formatVersionAesGCM) - 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...) + args = gcm.Seal(args, nonce, data, nil) result := make([]byte, hex.EncodedLen(len(args))) hex.Encode(result, args) @@ -68,57 +84,104 @@ 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()) + } + + version := binary.LittleEndian.Uint16(dataToExtract[:formatVersionSize]) + + switch version { + case formatVersionLegacyCBC: + result, err := s.decryptLegacyCBC(dataToExtract) + return result, version, err + case formatVersionAesGCM: + 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[ivLengthInfoSize : ivLengthInfoSize+ivSize] - cipherText := dataToExtract[ivLengthInfoSize+ivSize:] + 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) + } + + minimumDataBinarySize := formatVersionSize + gcmNonceSize + gcm.Overhead() + if len(dataToExtract) < minimumDataBinarySize { + return nil, minimumDataLengthError(minimumDataBinarySize) + } + + nonce := dataToExtract[formatVersionSize : formatVersionSize+gcmNonceSize] + cipherText := dataToExtract[formatVersionSize+gcmNonceSize:] + + result, err := gcm.Open(nil, nonce, cipherText, nil) + if err != nil { + return nil, errAuthenticationFailed } return result, nil } -func pad(data []byte) []byte { - padding := aes.BlockSize - len(data)%aes.BlockSize - padtext := bytes.Repeat([]byte{byte(padding)}, padding) - return append(data, padtext...) +func legacyCBCMinimumDataBinarySize() int { + return formatVersionSize + aes.BlockSize + aes.BlockSize +} + +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 +194,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..86e537c --- /dev/null +++ b/pkg/secret/aes_encoder_format_test.go @@ -0,0 +1,283 @@ +package secret + +import ( + "bytes" + "crypto/aes" + "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 binarySize := hex.DecodedLen(len(encodedData)); binarySize >= legacyCBCMinimumDataBinarySize() { + t.Logf("ciphertext is %d binary bytes, legacy minimum is %d", binarySize, 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) + } + } + } +} + +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("0300"), 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(), "3") { + 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/encoder.go b/pkg/secret/encoder.go index 9814ff0..369f877 100644 --- a/pkg/secret/encoder.go +++ b/pkg/secret/encoder.go @@ -4,3 +4,10 @@ type Encoder interface { Encrypt(data []byte) ([]byte, error) Decrypt(encodedData []byte) ([]byte, error) } + +// formatAwareDecrypter 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 formatAwareDecrypter interface { + decryptWithFormat(encodedData []byte) ([]byte, uint16, error) +} From e3d363eb2866b296764d9772d4d02b921ab2ea07 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:40:02 +0100 Subject: [PATCH 03/12] feat(secret): preserve YAML scalar type, style and comments Encrypting a YAML value used to stringify it with fmt.Sprintf, so the tag was never stored and decryption always produced a string: foo: 123 came back as foo: "123". node.Encode also replaced the value node wholesale, discarding the block scalar style and any comment attached to it. Store the short tag and the style alongside the raw value inside the encrypted payload and restore them on decryption. The value is framed last and treated as opaque bytes, so it may contain separators or newlines. Framing is gated on the encoder implementing formatAwareDecrypter, and the gate covers both directions: an Encoder without it keeps producing and consuming plain values exactly as before, so third-party encoders never receive a framed payload they cannot interpret. The ciphertext itself is still emitted as an ordinary plain 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 and corrupt it. Comments are now restored around node.Encode. Note that a comment attached to a value stays cleartext in the encrypted file, as keys already do. Signed-off-by: Aleksei Igrychev --- pkg/secret/yaml_encoder.go | 132 +++++++++++++++++++---- pkg/secret/yaml_encoder_fidelity_test.go | 130 ++++++++++++++++++++++ 2 files changed, 241 insertions(+), 21 deletions(-) create mode 100644 pkg/secret/yaml_encoder_fidelity_test.go diff --git a/pkg/secret/yaml_encoder.go b/pkg/secret/yaml_encoder.go index d2995e8..b0a41f2 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" ) @@ -13,6 +14,7 @@ type YamlEncoder struct { generateFunc func([]byte) ([]byte, error) extractFunc func([]byte) ([]byte, error) + formatAware formatAwareDecrypter } 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.(formatAwareDecrypter); ok { + yamlEncoder.formatAware = formatAware + } } else { yamlEncoder.generateFunc = doNothing yamlEncoder.extractFunc = doNothing @@ -39,7 +45,7 @@ func (s *YamlEncoder) Encrypt(data []byte) ([]byte, error) { } func (s *YamlEncoder) EncryptYamlData(data []byte) ([]byte, error) { - resultData, err := doYamlDataV2(s.generateFunc, data, encryptYamlMode) + resultData, err := doYamlDataV2(s.generateFunc, s.formatAware, data, encryptYamlMode) if err != nil { return nil, fmt.Errorf("encryption failed: check encryption key and data: %w", err) } @@ -61,7 +67,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 +79,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 formatAwareDecrypter, 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 +138,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 formatAwareDecrypter, 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 +153,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 +162,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 +170,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 +190,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 +235,85 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), node *yaml_v3.Node return node, nil } +func scalarPlainText(formatAware formatAwareDecrypter, 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 formatAwareDecrypter, node *yaml_v3.Node, value string) error { + plainText, version, err := formatAware.decryptWithFormat([]byte(value)) + if err != nil { + return err + } + + if version != formatVersionAesGCM { + 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..7228248 --- /dev/null +++ b/pkg/secret/yaml_encoder_fidelity_test.go @@ -0,0 +1,130 @@ +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 so that older readers accept it", 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("0200")) + }) + + 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("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")) + }) +}) From 335f04096e5d1f8128df79e005243f131cebfd0b Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:41:27 +0100 Subject: [PATCH 04/12] fix(secret): re-encrypt a scalar when only its tag or style changes MergeEncodedYamlNode reused the old ciphertext whenever the raw value matched, which kept stale ciphertext after an edit that only changed a value's type: "123" and 123 both carry the value 123 and differed only by tag, so the edit was silently dropped. Compare the short tag and the style as well. The style matters because it is now part of the encrypted payload, so switching a folded block scalar to a plain one has to produce new ciphertext too. Using ShortTag rather than Tag keeps an explicitly written !!str equal to an implicit one. Also retitle the YamlEncoder spec that pins scalar stringification, so it describes an encoder without format support rather than reading as the intended end state, and drop its obsolete TODO. Signed-off-by: Aleksei Igrychev --- pkg/secret/yaml_encoder_test.go | 6 ++++-- pkg/secret/yaml_helpers.go | 6 +++++- pkg/secret/yaml_helpers_test.go | 21 +++++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/pkg/secret/yaml_encoder_test.go b/pkg/secret/yaml_encoder_test.go index f98a3de..5219cdc 100644 --- a/pkg/secret/yaml_encoder_test.go +++ b/pkg/secret/yaml_encoder_test.go @@ -103,8 +103,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..0f5a928 100644 --- a/pkg/secret/yaml_helpers.go +++ b/pkg/secret/yaml_helpers.go @@ -101,7 +101,11 @@ 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 { return oldEncodedConfig, nil } return newEncodedConfig, nil diff --git a/pkg/secret/yaml_helpers_test.go b/pkg/secret/yaml_helpers_test.go index 29f5d93..d3ca958 100644 --- a/pkg/secret/yaml_helpers_test.go +++ b/pkg/secret/yaml_helpers_test.go @@ -480,3 +480,24 @@ 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"), + ) +}) From de21346175395c93f2d68d92c12989cda7859e3d Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:42:22 +0100 Subject: [PATCH 05/12] docs(secret): document the encoded format and its compatibility limits Add a package doc describing both format versions and their layout, that version 16 is read-only while version 2 is what gets written, and that the secret key is unchanged so no migration is needed. Spell out the two things that are easy to get wrong. Writing is not forward compatible: older werf and nelm releases ignore the version field and decrypt everything as CBC, so every consumer including CI jobs and saved plans has to understand version 2. And a value encrypted before version 2 never stored its tag, so its original type is gone for good and can only be restored by re-entering the value. Also note that a comment attached to an encrypted value stays cleartext. Signed-off-by: Aleksei Igrychev --- pkg/secret/doc.go | 56 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 pkg/secret/doc.go diff --git a/pkg/secret/doc.go b/pkg/secret/doc.go new file mode 100644 index 0000000..c0d65f8 --- /dev/null +++ b/pkg/secret/doc.go @@ -0,0 +1,56 @@ +// 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 ciphertext and authentication tag] +// +// 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 Encrypt writes. Any other version is rejected with an +// error rather than being decrypted as CBC. +// +// Both 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 cannot be read by a werf or nelm release that predates version 2 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 version 2. 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. +// +// # YAML scalars +// +// EncryptYamlData and DecryptYamlData encrypt each scalar leaf of a document in place. +// From version 2 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 2 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 From 291ec25557371e83cddf9c78cd1e7f8020b0579f Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:44:27 +0100 Subject: [PATCH 06/12] test(secret): cover the boundaries of scalar framing Three cases had no coverage. Framing must not run without an encoder, so --no-decrypt-secrets keeps passing ciphertext through untouched. Framing must not reach whole-blob encryption, or a decrypted secret file would gain separator bytes. And an unframed payload appearing in a YAML value must be reported rather than silently mangled into a tag and a style. Signed-off-by: Aleksei Igrychev --- pkg/secret/yaml_encoder_fidelity_test.go | 34 ++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pkg/secret/yaml_encoder_fidelity_test.go b/pkg/secret/yaml_encoder_fidelity_test.go index 7228248..f1dfdc8 100644 --- a/pkg/secret/yaml_encoder_fidelity_test.go +++ b/pkg/secret/yaml_encoder_fidelity_test.go @@ -118,6 +118,40 @@ var _ = Describe("YamlEncoder scalar fidelity", func() { 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("reports an unframed payload found in a YAML value instead of corrupting it", func() { + aesEncoder, err := NewAesEncoder(AesSecretKey) + Expect(err).NotTo(HaveOccurred()) + + blob, err := aesEncoder.Encrypt([]byte("no framing here")) + Expect(err).NotTo(HaveOccurred()) + + _, err = NewYamlEncoder(aesEncoder).DecryptYamlData([]byte("v: " + string(blob) + "\n")) + Expect(err).To(MatchError(ContainSubstring("malformed encrypted scalar payload"))) + }) + It("still decrypts a legacy ciphertext as a plain string", func() { legacyEncoder, err := NewAesEncoder(legacyFixtureKey) Expect(err).NotTo(HaveOccurred()) From bc5582f004361abbab47db5418601e61da0cc577 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:52:00 +0100 Subject: [PATCH 07/12] fix(secret): authenticate the format version prefix Bind the two version bytes as additional authenticated data so they cannot be altered within the AES-GCM format. This does not stop a rewrite of the prefix to the legacy version, which routes the value to the CBC reader that by definition does not authenticate. Measured, such a value is still rejected unless its length happens to suit CBC and the decrypted tail happens to form valid padding, and the result is unpredictable garbage rather than anything the attacker chooses. Closing that gap entirely would mean refusing to read legacy values, which is the one thing that must not break, so it is documented instead and pinned by a test over non-aligned lengths. Also turn the dead size guard in the short-plaintext round trip into a real failure, so the test cannot silently stop covering a ciphertext below the legacy minimum length. Signed-off-by: Aleksei Igrychev --- pkg/secret/aes_encoder.go | 7 +++-- pkg/secret/aes_encoder_format_test.go | 37 ++++++++++++++++++++++++++- pkg/secret/doc.go | 10 ++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/pkg/secret/aes_encoder.go b/pkg/secret/aes_encoder.go index dce566e..5942548 100644 --- a/pkg/secret/aes_encoder.go +++ b/pkg/secret/aes_encoder.go @@ -75,7 +75,10 @@ func (s *AesEncoder) Encrypt(data []byte) ([]byte, error) { return nil, fmt.Errorf("read random nonce: %w", err) } - args = gcm.Seal(args, nonce, data, nil) + // The version prefix is authenticated so that it cannot be altered within this + // format. It cannot stop a rewrite to the legacy version, which routes to the + // unauthenticated CBC reader instead; see the package documentation. + args = gcm.Seal(args, nonce, data, args[:formatVersionSize]) result := make([]byte, hex.EncodedLen(len(args))) hex.Encode(result, args) @@ -150,7 +153,7 @@ func (s *AesEncoder) decryptAesGCM(dataToExtract []byte) ([]byte, error) { nonce := dataToExtract[formatVersionSize : formatVersionSize+gcmNonceSize] cipherText := dataToExtract[formatVersionSize+gcmNonceSize:] - result, err := gcm.Open(nil, nonce, cipherText, nil) + result, err := gcm.Open(nil, nonce, cipherText, dataToExtract[:formatVersionSize]) if err != nil { return nil, errAuthenticationFailed } diff --git a/pkg/secret/aes_encoder_format_test.go b/pkg/secret/aes_encoder_format_test.go index 86e537c..e6c1c00 100644 --- a/pkg/secret/aes_encoder_format_test.go +++ b/pkg/secret/aes_encoder_format_test.go @@ -3,6 +3,7 @@ package secret import ( "bytes" "crypto/aes" + "encoding/binary" "encoding/hex" "errors" "fmt" @@ -42,7 +43,7 @@ func TestAesEncoderShortPlaintextRoundTrip(t *testing.T) { } if binarySize := hex.DecodedLen(len(encodedData)); binarySize >= legacyCBCMinimumDataBinarySize() { - t.Logf("ciphertext is %d binary bytes, legacy minimum is %d", binarySize, legacyCBCMinimumDataBinarySize()) + t.Errorf("ciphertext is %d binary bytes, at or above the legacy minimum of %d, so this no longer exercises a below-minimum ciphertext", binarySize, legacyCBCMinimumDataBinarySize()) } result, err := s.Decrypt(encodedData) @@ -91,6 +92,40 @@ func TestAesEncoderRejectsEveryBitFlip(t *testing.T) { } } +// Rewriting the version prefix of a version-2 ciphertext to the legacy one routes it to +// the CBC reader, which does not authenticate. The length check rejects that outright +// unless the blob happens to suit CBC, and the hardened unpad rejects almost all of the +// rest. This pins the deterministic half; the residual is documented in doc.go. +func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + for _, plainText := range []string{"a", "ab", "abc", "abcde", "value", "a longer secret value"} { + t.Run(plainText, func(t *testing.T) { + encodedData, err := s.Encrypt([]byte(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) + + if _, err := s.Decrypt(downgraded); err == nil { + t.Error("a version-downgraded ciphertext was accepted") + } + }) + } +} + func TestAesEncoderRejectsWrongKey(t *testing.T) { s, err := NewAesEncoder(AesSecretKey) if err != nil { diff --git a/pkg/secret/doc.go b/pkg/secret/doc.go index c0d65f8..58fbe34 100644 --- a/pkg/secret/doc.go +++ b/pkg/secret/doc.go @@ -37,6 +37,16 @@ // 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 a version 2 value is authenticated, so it cannot be altered +// within that format. It cannot be bound any tighter than that: rewriting the prefix to +// 16 sends the value to the legacy CBC reader, which by definition does not authenticate. +// Such a value is still rejected unless its length happens to suit CBC and the decrypted +// tail happens to form valid padding, and the result is unpredictable garbage rather than +// anything the attacker chooses, since they do not hold the key. Removing this last gap +// means dropping the ability to read legacy values, which is exactly what must not break. +// Re-encrypting a repository with rotate-secret-key does not close it either, because the +// legacy reader has to stay for as long as any legacy value might exist. +// // # YAML scalars // // EncryptYamlData and DecryptYamlData encrypt each scalar leaf of a document in place. From 2e874255dc006a89434224c71e45a7fad9597659 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 21:59:58 +0100 Subject: [PATCH 08/12] test(secret): cover the block-aligned version downgrade The downgrade test only covered plaintext lengths that miss the legacy block layout, where a rewritten prefix is rejected on shape alone. That left the one interesting case untested: a plaintext whose length is 4 modulo 16 produces a container the legacy reader will actually parse. Cover it by asserting the property that has to hold there, since the legacy reader cannot authenticate: an accepted downgrade never yields the protected plaintext. Measured over 50000 attempts it never did, because the attacker holds no key and gets unpredictable garbage. Also record in the package documentation that the rewrite grants no new capability. Replacing the value outright with a self-made legacy blob succeeds at the same rate, measured 0.408% against 0.420%, so the exposure is the readable unauthenticated format itself rather than the rewrite. Signed-off-by: Aleksei Igrychev --- pkg/secret/aes_encoder_format_test.go | 58 +++++++++++++++++++++++++-- pkg/secret/doc.go | 18 ++++++--- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/pkg/secret/aes_encoder_format_test.go b/pkg/secret/aes_encoder_format_test.go index e6c1c00..766f1b1 100644 --- a/pkg/secret/aes_encoder_format_test.go +++ b/pkg/secret/aes_encoder_format_test.go @@ -92,10 +92,62 @@ func TestAesEncoderRejectsEveryBitFlip(t *testing.T) { } } +// A version-2 container whose plaintext length is 4 modulo 16 also satisfies the legacy +// block layout, so a rewritten prefix reaches the CBC reader instead of being rejected on +// shape alone. That reader cannot authenticate, so a small fraction of attempts is +// accepted. What must hold is that such an attempt never yields the protected plaintext: +// the attacker has no key, so anything accepted is unpredictable garbage. +func TestAesEncoderDowngradeNeverRevealsPlaintext(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) + } + + plainText := []byte("s3cr") + if len(plainText)%aes.BlockSize != 4 { + t.Fatalf("this test needs a plaintext length of 4 modulo 16 to reach the legacy reader, got %d", len(plainText)) + } + + const trials = 2000 + accepted := 0 + + for i := 0; i < trials; i++ { + 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) + + result, err := s.Decrypt(downgraded) + if err != nil { + continue + } + + accepted++ + if bytes.Equal(result, plainText) { + t.Fatal("a downgraded ciphertext revealed the protected plaintext") + } + } + + // The expected rate is well under 1%; this only guards against the legacy reader + // turning permissive, not against the inherent gap itself. + if accepted*100 > trials*5 { + t.Errorf("legacy reader accepted %d of %d downgraded ciphertexts, far above the expected rate", accepted, trials) + } +} + // Rewriting the version prefix of a version-2 ciphertext to the legacy one routes it to -// the CBC reader, which does not authenticate. The length check rejects that outright -// unless the blob happens to suit CBC, and the hardened unpad rejects almost all of the -// rest. This pins the deterministic half; the residual is documented in doc.go. +// the CBC reader, which does not authenticate. For every plaintext length that does not +// suit the legacy block layout the rewrite is rejected outright, deterministically. func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { s, err := NewAesEncoder(AesSecretKey) if err != nil { diff --git a/pkg/secret/doc.go b/pkg/secret/doc.go index 58fbe34..8cb7b25 100644 --- a/pkg/secret/doc.go +++ b/pkg/secret/doc.go @@ -40,12 +40,18 @@ // The version prefix of a version 2 value is authenticated, so it cannot be altered // within that format. It cannot be bound any tighter than that: rewriting the prefix to // 16 sends the value to the legacy CBC reader, which by definition does not authenticate. -// Such a value is still rejected unless its length happens to suit CBC and the decrypted -// tail happens to form valid padding, and the result is unpredictable garbage rather than -// anything the attacker chooses, since they do not hold the key. Removing this last gap -// means dropping the ability to read legacy values, which is exactly what must not break. -// Re-encrypting a repository with rotate-secret-key does not close it either, because the -// legacy reader has to stay for as long as any legacy value might exist. +// Such a value is still rejected unless its length happens to suit the legacy block +// layout and the decrypted tail happens to form valid padding, and what comes out is +// unpredictable garbage rather than anything the attacker chooses, since they do not hold +// the key. +// +// This grants an attacker nothing they did not already have. Anyone able to rewrite those +// two bytes can just as easily replace the whole value with a legacy blob of their own, +// which this package must keep reading, and that succeeds at the same small rate. The +// exposure is not the rewrite but the fact that an unauthenticated format stays readable, +// 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 // From 35427310478232cffa67ec896534142e2e331244 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 22:03:41 +0100 Subject: [PATCH 09/12] fix(secret): keep every version-2 container off the legacy block grid A version-2 container whose size happened to match the legacy layout could be handed to the CBC reader by rewriting its version prefix, and that reader cannot authenticate. Roughly one plaintext length in sixteen landed on the grid, and such a rewrite was then accepted about 0.4% of the time, returning unpredictable garbage. Append one byte of filler in exactly those cases, with a trailing byte recording how much filler is present, so no container size can ever match the legacy layout. A rewritten prefix now fails the block-size check for every plaintext length instead of most of them, which turns a probabilistic rejection into a certain one. The filler sits inside the sealed data, so it is authenticated along with everything else. This does not widen or narrow what an attacker can do: replacing the value outright with a self-made legacy blob succeeds at the same rate regardless, because reading the unauthenticated legacy format is a requirement. What it removes is the possibility of a version-2 value being silently accepted without authentication. Signed-off-by: Aleksei Igrychev --- pkg/secret/aes_encoder.go | 60 ++++++++++++++++-- pkg/secret/aes_encoder_format_test.go | 89 +++++++++------------------ 2 files changed, 82 insertions(+), 67 deletions(-) diff --git a/pkg/secret/aes_encoder.go b/pkg/secret/aes_encoder.go index 5942548..3f84198 100644 --- a/pkg/secret/aes_encoder.go +++ b/pkg/secret/aes_encoder.go @@ -19,6 +19,11 @@ const ( 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 ( @@ -75,10 +80,8 @@ func (s *AesEncoder) Encrypt(data []byte) ([]byte, error) { return nil, fmt.Errorf("read random nonce: %w", err) } - // The version prefix is authenticated so that it cannot be altered within this - // format. It cannot stop a rewrite to the legacy version, which routes to the - // unauthenticated CBC reader instead; see the package documentation. - args = gcm.Seal(args, nonce, data, args[:formatVersionSize]) + // 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) @@ -145,7 +148,7 @@ func (s *AesEncoder) decryptAesGCM(dataToExtract []byte) ([]byte, error) { return nil, fmt.Errorf("initialize aes-gcm: %w", err) } - minimumDataBinarySize := formatVersionSize + gcmNonceSize + gcm.Overhead() + minimumDataBinarySize := gcmContainerSize(0, 0, gcm.Overhead()) if len(dataToExtract) < minimumDataBinarySize { return nil, minimumDataLengthError(minimumDataBinarySize) } @@ -158,7 +161,52 @@ func (s *AesEncoder) decryptAesGCM(dataToExtract []byte) ([]byte, error) { return nil, errAuthenticationFailed } - return result, nil + return unfill(result) +} + +// 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) ([]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 + } + + return sealed[:len(sealed)-filler-gcmFillerSizeLen], nil } func legacyCBCMinimumDataBinarySize() int { diff --git a/pkg/secret/aes_encoder_format_test.go b/pkg/secret/aes_encoder_format_test.go index 766f1b1..ec5f8d8 100644 --- a/pkg/secret/aes_encoder_format_test.go +++ b/pkg/secret/aes_encoder_format_test.go @@ -42,8 +42,8 @@ func TestAesEncoderShortPlaintextRoundTrip(t *testing.T) { t.Fatal(err) } - if binarySize := hex.DecodedLen(len(encodedData)); binarySize >= legacyCBCMinimumDataBinarySize() { - t.Errorf("ciphertext is %d binary bytes, at or above the legacy minimum of %d, so this no longer exercises a below-minimum ciphertext", binarySize, legacyCBCMinimumDataBinarySize()) + 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) @@ -92,26 +92,38 @@ func TestAesEncoderRejectsEveryBitFlip(t *testing.T) { } } -// A version-2 container whose plaintext length is 4 modulo 16 also satisfies the legacy -// block layout, so a rewritten prefix reaches the CBC reader instead of being rejected on -// shape alone. That reader cannot authenticate, so a small fraction of attempts is -// accepted. What must hold is that such an attempt never yields the protected plaintext: -// the attacker has no key, so anything accepted is unpredictable garbage. -func TestAesEncoderDowngradeNeverRevealsPlaintext(t *testing.T) { +// No version-2 container may ever sit on the legacy block grid, otherwise rewriting its +// version prefix would hand it to the CBC reader, which cannot authenticate. +func TestAesEncoderContainerNeverMatchesLegacyLayout(t *testing.T) { s, err := NewAesEncoder(AesSecretKey) if err != nil { t.Fatal(err) } - plainText := []byte("s3cr") - if len(plainText)%aes.BlockSize != 4 { - t.Fatalf("this test needs a plaintext length of 4 modulo 16 to reach the legacy reader, got %d", len(plainText)) + for dataSize := 0; dataSize <= 200; dataSize++ { + encodedData, err := s.Encrypt(make([]byte, dataSize)) + if err != nil { + t.Fatal(err) + } + + containerSize := hex.DecodedLen(len(encodedData)) + if matchesLegacyLayout(containerSize) { + t.Errorf("a %d-byte plaintext produced a %d-byte container, which the legacy reader would parse", dataSize, containerSize) + } + } +} + +// 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. +func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { + s, err := NewAesEncoder(AesSecretKey) + if err != nil { + t.Fatal(err) } - const trials = 2000 - accepted := 0 + for dataSize := 0; dataSize <= 200; dataSize++ { + plainText := bytes.Repeat([]byte("s"), dataSize) - for i := 0; i < trials; i++ { encodedData, err := s.Encrypt(plainText) if err != nil { t.Fatal(err) @@ -127,55 +139,10 @@ func TestAesEncoderDowngradeNeverRevealsPlaintext(t *testing.T) { downgraded := make([]byte, hex.EncodedLen(len(raw))) hex.Encode(downgraded, raw) - result, err := s.Decrypt(downgraded) - if err != nil { - continue - } - - accepted++ - if bytes.Equal(result, plainText) { - t.Fatal("a downgraded ciphertext revealed the protected plaintext") + if _, err := s.Decrypt(downgraded); err == nil { + t.Fatalf("a version-downgraded ciphertext of a %d-byte plaintext was accepted", dataSize) } } - - // The expected rate is well under 1%; this only guards against the legacy reader - // turning permissive, not against the inherent gap itself. - if accepted*100 > trials*5 { - t.Errorf("legacy reader accepted %d of %d downgraded ciphertexts, far above the expected rate", accepted, trials) - } -} - -// Rewriting the version prefix of a version-2 ciphertext to the legacy one routes it to -// the CBC reader, which does not authenticate. For every plaintext length that does not -// suit the legacy block layout the rewrite is rejected outright, deterministically. -func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { - s, err := NewAesEncoder(AesSecretKey) - if err != nil { - t.Fatal(err) - } - - for _, plainText := range []string{"a", "ab", "abc", "abcde", "value", "a longer secret value"} { - t.Run(plainText, func(t *testing.T) { - encodedData, err := s.Encrypt([]byte(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) - - if _, err := s.Decrypt(downgraded); err == nil { - t.Error("a version-downgraded ciphertext was accepted") - } - }) - } } func TestAesEncoderRejectsWrongKey(t *testing.T) { From 5fec8a36f77b9c13414c4e21c0a984d78e5aaf8f Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Sun, 2 Aug 2026 22:09:31 +0100 Subject: [PATCH 10/12] docs(secret): correct the format contract after the filler change The package documentation was written before the filler was introduced and never updated with it, so it described neither the sealed layout nor the current behaviour. It now records what version 2 actually seals, the value followed by the filler and the byte holding the filler size, so an implementation written from this file does not emit containers that fail to parse or decrypt real ones with a trailing spurious byte. The authentication section claimed a rewritten version prefix was rejected only when the length did not happen to suit the legacy layout. That stopped being true once no container can share a legacy size: such a rewrite is now rejected for every value length. Only the substitution of a self-made legacy blob remains, so the text names that as the residual instead. Signed-off-by: Aleksei Igrychev --- pkg/secret/doc.go | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/pkg/secret/doc.go b/pkg/secret/doc.go index 8cb7b25..6c5964e 100644 --- a/pkg/secret/doc.go +++ b/pkg/secret/doc.go @@ -9,6 +9,14 @@ // version 16: [version][16-byte IV][AES-CBC ciphertext, PKCS#7 padded] // version 2: [version][12-byte nonce][AES-GCM ciphertext and authentication tag] // +// What version 2 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: @@ -38,20 +46,21 @@ // This protects newly written values only; existing values gain it once re-encrypted. // // The version prefix of a version 2 value is authenticated, so it cannot be altered -// within that format. It cannot be bound any tighter than that: rewriting the prefix to -// 16 sends the value to the legacy CBC reader, which by definition does not authenticate. -// Such a value is still rejected unless its length happens to suit the legacy block -// layout and the decrypted tail happens to form valid padding, and what comes out is -// unpredictable garbage rather than anything the attacker chooses, since they do not hold -// the key. -// -// This grants an attacker nothing they did not already have. Anyone able to rewrite those -// two bytes can just as easily replace the whole value with a legacy blob of their own, -// which this package must keep reading, and that succeeds at the same small rate. The -// exposure is not the rewrite but the fact that an unauthenticated format stays readable, -// 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. +// 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: a version 2 +// 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 // From 2062a596e39daaac38c58efeaa39d4c595676e1d Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Mon, 3 Aug 2026 08:43:43 +0100 Subject: [PATCH 11/12] test(secret): make the downgrade test able to fail The downgrade test asserted only that some error came back, which almost any outcome satisfies. With the filler disabled it caught the regression in 0 of 20 runs: a container that reaches the legacy key stream is rejected only when the decrypted tail happens not to form valid padding, about 995 times in 1000, so the test passed while the property it names was broken. It now requires the rejection to come from the size or block check. Reaching unpad at all means the blob was run through an unauthenticated key stream first, which is the thing being prevented. Same mutation now fails 20 of 20, naming the offending length. Drop TestAesEncoderContainerNeverMatchesLegacyLayout. It asserted the property through matchesLegacyLayout, the same predicate the production code decides with, so replacing that predicate with "return false" broke the filler and the test agreed with the break and passed. The strengthened downgrade test now covers the property from the outside, deterministically, which leaves nothing for a tautology to add. Signed-off-by: Aleksei Igrychev --- pkg/secret/aes_encoder_format_test.go | 33 +++++++++------------------ 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/pkg/secret/aes_encoder_format_test.go b/pkg/secret/aes_encoder_format_test.go index ec5f8d8..cb56280 100644 --- a/pkg/secret/aes_encoder_format_test.go +++ b/pkg/secret/aes_encoder_format_test.go @@ -92,29 +92,13 @@ func TestAesEncoderRejectsEveryBitFlip(t *testing.T) { } } -// No version-2 container may ever sit on the legacy block grid, otherwise rewriting its -// version prefix would hand it to the CBC reader, which cannot authenticate. -func TestAesEncoderContainerNeverMatchesLegacyLayout(t *testing.T) { - s, err := NewAesEncoder(AesSecretKey) - if err != nil { - t.Fatal(err) - } - - for dataSize := 0; dataSize <= 200; dataSize++ { - encodedData, err := s.Encrypt(make([]byte, dataSize)) - if err != nil { - t.Fatal(err) - } - - containerSize := hex.DecodedLen(len(encodedData)) - if matchesLegacyLayout(containerSize) { - t.Errorf("a %d-byte plaintext produced a %d-byte container, which the legacy reader would parse", dataSize, containerSize) - } - } -} - // 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 { @@ -139,9 +123,14 @@ func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { downgraded := make([]byte, hex.EncodedLen(len(raw))) hex.Encode(downgraded, raw) - if _, err := s.Decrypt(downgraded); err == nil { + _, 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) + } } } From 855c26de8c11a1491b918e678c22f421c5745316 Mon Sep 17 00:00:00 2001 From: Aleksei Igrychev Date: Mon, 31 Aug 2026 17:04:22 +0100 Subject: [PATCH 12/12] fix(secret): keep encrypted values usable across secret workflows Separate whole-blob and YAML-scalar AES-GCM payloads so documented secret values continue to decrypt. Preserve comment-only edits and reject noncanonical v2 filler. Signed-off-by: Aleksei Igrychev --- pkg/secret/aes_encoder.go | 33 +++++++++++---- pkg/secret/aes_encoder_format_test.go | 53 +++++++++++++++++++++++- pkg/secret/doc.go | 26 ++++++------ pkg/secret/encoder.go | 5 ++- pkg/secret/yaml_encoder.go | 25 ++++++----- pkg/secret/yaml_encoder_fidelity_test.go | 25 ++++++++--- pkg/secret/yaml_helpers.go | 3 ++ pkg/secret/yaml_helpers_test.go | 25 +++++++++++ 8 files changed, 156 insertions(+), 39 deletions(-) diff --git a/pkg/secret/aes_encoder.go b/pkg/secret/aes_encoder.go index 3f84198..804750d 100644 --- a/pkg/secret/aes_encoder.go +++ b/pkg/secret/aes_encoder.go @@ -14,8 +14,9 @@ import ( ) const ( - formatVersionLegacyCBC uint16 = 16 - formatVersionAesGCM uint16 = 2 + formatVersionLegacyCBC uint16 = 16 + formatVersionAesGCM uint16 = 2 + formatVersionAesGCMYaml uint16 = 3 formatVersionSize = 2 gcmNonceSize = 12 @@ -38,7 +39,7 @@ type AesEncoder struct { CipherBlock cipher.Block } -var _ formatAwareDecrypter = (*AesEncoder)(nil) +var _ formatAwareEncoder = (*AesEncoder)(nil) func GenerateAesSecretKey() ([]byte, error) { randomBytes := make([]byte, 16) @@ -67,13 +68,21 @@ func NewAesEncoder(key []byte) (*AesEncoder, error) { } func (s *AesEncoder) Encrypt(data []byte) ([]byte, error) { + return s.encryptWithFormat(data, formatVersionAesGCM) +} + +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) } args := make([]byte, formatVersionSize+gcmNonceSize) - binary.LittleEndian.PutUint16(args[:formatVersionSize], formatVersionAesGCM) + binary.LittleEndian.PutUint16(args[:formatVersionSize], version) nonce := args[formatVersionSize:] if _, err := io.ReadFull(rand.Reader, nonce); err != nil { @@ -116,7 +125,7 @@ func (s *AesEncoder) decryptWithFormat(data []byte) ([]byte, uint16, error) { case formatVersionLegacyCBC: result, err := s.decryptLegacyCBC(dataToExtract) return result, version, err - case formatVersionAesGCM: + case formatVersionAesGCM, formatVersionAesGCMYaml: result, err := s.decryptAesGCM(dataToExtract) return result, version, err default: @@ -152,6 +161,9 @@ func (s *AesEncoder) decryptAesGCM(dataToExtract []byte) ([]byte, error) { if len(dataToExtract) < minimumDataBinarySize { return nil, minimumDataLengthError(minimumDataBinarySize) } + if matchesLegacyLayout(len(dataToExtract)) { + return nil, errAuthenticationFailed + } nonce := dataToExtract[formatVersionSize : formatVersionSize+gcmNonceSize] cipherText := dataToExtract[formatVersionSize+gcmNonceSize:] @@ -161,7 +173,7 @@ func (s *AesEncoder) decryptAesGCM(dataToExtract []byte) ([]byte, error) { return nil, errAuthenticationFailed } - return unfill(result) + return unfill(result, gcm.Overhead()) } // A container whose size matches the legacy layout could be handed to the CBC reader by @@ -196,7 +208,7 @@ func matchesLegacyLayout(containerSize int) bool { (containerSize-formatVersionSize-aes.BlockSize)%aes.BlockSize == 0 } -func unfill(sealed []byte) ([]byte, error) { +func unfill(sealed []byte, overhead int) ([]byte, error) { if len(sealed) < gcmFillerSizeLen { return nil, errAuthenticationFailed } @@ -206,7 +218,12 @@ func unfill(sealed []byte) ([]byte, error) { return nil, errAuthenticationFailed } - return sealed[:len(sealed)-filler-gcmFillerSizeLen], nil + 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 { diff --git a/pkg/secret/aes_encoder_format_test.go b/pkg/secret/aes_encoder_format_test.go index cb56280..0ae36fe 100644 --- a/pkg/secret/aes_encoder_format_test.go +++ b/pkg/secret/aes_encoder_format_test.go @@ -3,6 +3,7 @@ package secret import ( "bytes" "crypto/aes" + "crypto/cipher" "encoding/binary" "encoding/hex" "errors" @@ -134,6 +135,54 @@ func TestAesEncoderRejectsVersionDowngrade(t *testing.T) { } } +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 { @@ -171,14 +220,14 @@ func TestAesEncoderRejectsUnsupportedFormatVersion(t *testing.T) { t.Fatal(err) } - unsupported := append([]byte("0300"), encodedData[4:]...) + 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(), "3") { + if !strings.Contains(err.Error(), "4") { t.Errorf("expected the rejected version in the message, got: %v", err) } diff --git a/pkg/secret/doc.go b/pkg/secret/doc.go index 6c5964e..f746e3d 100644 --- a/pkg/secret/doc.go +++ b/pkg/secret/doc.go @@ -7,9 +7,10 @@ // little-endian format version: // // version 16: [version][16-byte IV][AES-CBC ciphertext, PKCS#7 padded] -// version 2: [version][12-byte nonce][AES-GCM ciphertext and authentication tag] +// 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 version 2 seals is not the value on its own but: +// 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] // @@ -22,21 +23,22 @@ // 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 Encrypt writes. Any other version is rejected with an -// error rather than being decrypted as CBC. +// 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 versions are read with the same secret key, so upgrading needs no new key and no +// 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 cannot be read by a werf or nelm release that predates version 2 support, +// 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 version 2. Re-encrypting everything at once is what rotate-secret-key does. +// understand AES-GCM formats. Re-encrypting everything at once is what rotate-secret-key does. // // # Authentication // @@ -45,10 +47,10 @@ // 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 a version 2 value is authenticated, so it cannot be altered -// within that format. On its own that would not be enough, because rewriting the prefix +// 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: a version 2 +// 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. @@ -65,11 +67,11 @@ // # YAML scalars // // EncryptYamlData and DecryptYamlData encrypt each scalar leaf of a document in place. -// From version 2 on, the YAML tag and the scalar style are stored inside the encrypted +// 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 2 did not store a tag, so that information does not +// 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 diff --git a/pkg/secret/encoder.go b/pkg/secret/encoder.go index 369f877..9b4b3c8 100644 --- a/pkg/secret/encoder.go +++ b/pkg/secret/encoder.go @@ -5,9 +5,10 @@ type Encoder interface { Decrypt(encodedData []byte) ([]byte, error) } -// formatAwareDecrypter is implemented by encoders whose ciphertext carries a format +// 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 formatAwareDecrypter interface { +type formatAwareEncoder interface { + encryptYamlScalar(data []byte) ([]byte, error) decryptWithFormat(encodedData []byte) ([]byte, uint16, error) } diff --git a/pkg/secret/yaml_encoder.go b/pkg/secret/yaml_encoder.go index b0a41f2..7ddfdb6 100644 --- a/pkg/secret/yaml_encoder.go +++ b/pkg/secret/yaml_encoder.go @@ -12,9 +12,9 @@ import ( type YamlEncoder struct { Encoder Encoder - generateFunc func([]byte) ([]byte, error) - extractFunc func([]byte) ([]byte, error) - formatAware formatAwareDecrypter + generateFunc func([]byte) ([]byte, error) + extractFunc func([]byte) ([]byte, error) + formatAware formatAwareEncoder } func NewYamlEncoder(encoder Encoder) *YamlEncoder { @@ -24,7 +24,7 @@ func NewYamlEncoder(encoder Encoder) *YamlEncoder { yamlEncoder.generateFunc = encoder.Encrypt yamlEncoder.extractFunc = encoder.Decrypt - if formatAware, ok := encoder.(formatAwareDecrypter); ok { + if formatAware, ok := encoder.(formatAwareEncoder); ok { yamlEncoder.formatAware = formatAware } } else { @@ -45,7 +45,12 @@ func (s *YamlEncoder) Encrypt(data []byte) ([]byte, error) { } func (s *YamlEncoder) EncryptYamlData(data []byte) ([]byte, error) { - resultData, err := doYamlDataV2(s.generateFunc, s.formatAware, 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) } @@ -79,7 +84,7 @@ func (s *YamlEncoder) DecryptYamlData(data []byte) ([]byte, error) { return resultData, nil } -func doYamlDataV2(doFunc func([]byte) ([]byte, error), formatAware formatAwareDecrypter, 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 { @@ -138,7 +143,7 @@ func deepCopyNode(node *yaml_v3.Node) *yaml_v3.Node { return copyNode } -func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), formatAware formatAwareDecrypter, 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 { @@ -235,7 +240,7 @@ func doYamlValueSecretV2(doFunc func([]byte) ([]byte, error), formatAware format return node, nil } -func scalarPlainText(formatAware formatAwareDecrypter, node *yaml_v3.Node) ([]byte, error) { +func scalarPlainText(formatAware formatAwareEncoder, node *yaml_v3.Node) ([]byte, error) { if formatAware != nil { return frameScalar(node.ShortTag(), node.Style, node.Value), nil } @@ -248,13 +253,13 @@ func scalarPlainText(formatAware formatAwareDecrypter, node *yaml_v3.Node) ([]by return []byte(fmt.Sprintf("%v", value)), nil } -func decryptScalarWithMetadata(formatAware formatAwareDecrypter, node *yaml_v3.Node, value string) error { +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 != formatVersionAesGCM { + if version != formatVersionAesGCMYaml { return encodeScalarPreservingComments(node, string(plainText)) } diff --git a/pkg/secret/yaml_encoder_fidelity_test.go b/pkg/secret/yaml_encoder_fidelity_test.go index f1dfdc8..a5337b5 100644 --- a/pkg/secret/yaml_encoder_fidelity_test.go +++ b/pkg/secret/yaml_encoder_fidelity_test.go @@ -69,14 +69,14 @@ var _ = Describe("YamlEncoder scalar fidelity", func() { Expect(string(decrypted)).To(Equal(data)) }) - It("emits the ciphertext as an ordinary string scalar so that older readers accept it", func() { + 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("0200")) + Expect(cipherScalar.Value).To(HavePrefix("0300")) }) It("preserves comments attached to a value and to a key", func() { @@ -141,15 +141,30 @@ var _ = Describe("YamlEncoder scalar fidelity", func() { Expect(string(decoded)).NotTo(ContainSubstring(string([]byte{scalarFrameSeparator}))) }) - It("reports an unframed payload found in a YAML value instead of corrupting it", func() { + 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()) - _, err = NewYamlEncoder(aesEncoder).DecryptYamlData([]byte("v: " + string(blob) + "\n")) - Expect(err).To(MatchError(ContainSubstring("malformed encrypted scalar payload"))) + 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() { diff --git a/pkg/secret/yaml_helpers.go b/pkg/secret/yaml_helpers.go index 0f5a928..90c5ae1 100644 --- a/pkg/secret/yaml_helpers.go +++ b/pkg/secret/yaml_helpers.go @@ -106,6 +106,9 @@ func MergeEncodedYamlNode(oldConfig, newConfig, oldEncodedConfig, newEncodedConf 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 d3ca958..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 { @@ -500,4 +501,28 @@ var _ = Describe("MergeEncodedYaml scalar metadata", func() { 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")) + }) })