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