feat(secret): add versioned AES-GCM format and YAML scalar fidelity - #37
Merged
Conversation
) * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> * 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 <aleksei.igrychev@palark.com> --------- Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com> (cherry picked from commit 7f4a363)
Member
Author
Verification
Review focus
Follow-up
|
alexey-igrychev
marked this pull request as ready for review
August 31, 2026 16:25
This was referenced Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Secret encryption writes authenticated, versioned AES-GCM values while retaining read support for existing AES-CBC secrets. This backport targets common-go branch
1: whole blobs and YAML scalar values use distinct authenticated formats, documented--secret-valuesworkflows remain usable, and YAML scalar types, block styles, and comments survive an edit cycle.Related: werf/nelm#677
What
Breaking
Encryption and YAML values
secret values editretains head, line, and foot comment changes even when the secret value, tag, and style are unchanged.Why
Legacy AES-CBC has no integrity check, accepted corrupted ciphertext, and lost scalar metadata by stringifying values before encryption. A single AES-GCM format for both arbitrary whole blobs and framed YAML scalars made their plaintext shapes ambiguous; separate authenticated versions preserve both workflows without sniffing secret content. Legacy read support remains necessary for existing repositories, while new major werf and nelm releases provide the coordinated writer transition.