Skip to content

fix: include decrypted sequence comments in the MAC (fixes #2243)#2245

Merged
felixfontein merged 2 commits into
getsops:mainfrom
cbcoutinho:fix/mac-sequence-comment-decrypt
Jul 11, 2026
Merged

fix: include decrypted sequence comments in the MAC (fixes #2243)#2245
felixfontein merged 2 commits into
getsops:mainfrom
cbcoutinho:fix/mac-sequence-comment-decrypt

Conversation

@cbcoutinho

Copy link
Copy Markdown
Contributor

What

Fixes a regression in 3.13.2 where sops fails to decrypt any YAML/JSON file
containing a comment inside a sequence, reporting MAC mismatch. This
affects files written by ≤3.13.1 and files 3.13.2 encrypts itself.

Fixes #2243.

Root cause

A comment in a sequence is encrypted as an ENC[...,type:comment] element and
restored as a sops.Comment node when the file is loaded again.

#2227 changed the MAC-inclusion test in Tree.Decrypt from inspecting the
decrypted result to inspecting the input node:

-if _, ok := v.(Comment); !ok {
+if !isComment {
     bytes, err := ToBytes(v)
     hash.Write(bytes)
 }

A comment that decrypts successfully becomes a non-Comment value, so the old
check included it in the MAC. Tree.Encrypt still folds the comment in (at
encrypt time it is a non-Comment node), so the encrypt-side and decrypt-side
MACs disagree for any sequence containing a comment.

Fix

Restore the pre-#2227 behaviour (test the decrypted value's type) while keeping
the non-string panic guard #2227 introduced. One-line semantic change.

Reproduction

# repro.yaml
items:
  # - commented@example.com
  - real@example.com
$ sops -v3.13.1 -e repro.yaml > repro.enc.yaml
$ sops -v3.13.1 -d repro.enc.yaml   # OK
$ sops -v3.13.2 -d repro.enc.yaml   # MAC mismatch

A comment on a mapping, or a sequence with no comment, is unaffected.

Test

Adds TestMACWithCommentInSequence (YAML store round-trip: load → encrypt →
emit → reload → decrypt, asserting the MACs match). It fails on main and
passes with this change. go test ./... for the root, stores, decrypt and cmd
packages passes; gofmt/go vet clean.


This PR was generated with the help of AI, and reviewed by a Human

@felixfontein felixfontein left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting this and creating a fix! There are a few issues with the test and the comment for the fix, though.

Comment thread stores/yaml/mac_sequence_comment_test.go Outdated
Comment thread stores/yaml/mac_sequence_comment_test.go Outdated
Comment thread stores/yaml/mac_sequence_comment_test.go Outdated
Comment thread sops.go Outdated
@cbcoutinho

Copy link
Copy Markdown
Contributor Author

Thanks for the review @felixfontein! Pushed 1aa8912 addressing all four points:

  • sops.go comment — replaced with your suggested wording. You're right: the plaintext comment in a sequence is a Comment node, but Encrypt turns it into an ordinary (non-Comment) string element whose metadata marks it a comment; on Decrypt that string decrypts back to a Comment. Hence the MAC decision has to look at the decrypted result's type, not isComment on the input.
  • Test location — moved out of the YAML store and into sops_test.go (package sops).
  • Build tree from SOPS objects — the test now constructs branches directly with a Comment inside a slice, no store involved.
  • No serialize/deserialize — it encrypts and then decrypts the same tree (Encrypt mutates in place), and additionally asserts the sequence comment became a non-Comment string after encryption, so the format is checked explicitly. A small commentCipher in the test models the AES cipher's comment-metadata round-trip without pulling in the aes package (which would be an import cycle in package sops).

Verified the test fails on the pre-fix code and passes with the fix.

@cbcoutinho cbcoutinho force-pushed the fix/mac-sequence-comment-decrypt branch 7 times, most recently from ec3fcb9 to e9782f9 Compare July 8, 2026 20:18
Comment thread sops_test.go Outdated
Comment thread sops_test.go Outdated
Comment thread sops_test.go Outdated
Comment thread sops.go Outdated
@cbcoutinho cbcoutinho force-pushed the fix/mac-sequence-comment-decrypt branch 2 times, most recently from 502d079 to 9bdc594 Compare July 11, 2026 11:36
A comment inside a YAML sequence is encrypted as an ENC[...,type:comment]
element and is restored as a sops.Comment node when the file is loaded
again. Since getsops#2227, Tree.Decrypt performed the MAC-inclusion test on the
input node (isComment) instead of on the decrypted result. A comment that
decrypts successfully becomes a non-Comment value, so testing the input
excluded it from the MAC. Tree.Encrypt still folds the comment in (at
encrypt time it is a non-Comment node), so the encrypt-side and
decrypt-side MACs disagreed and every file containing a comment inside a
sequence failed to decrypt with "MAC mismatch" -- including files that
3.13.2 encrypted itself.

Restore the pre-getsops#2227 behaviour of testing the decrypted value's type,
while keeping the non-string panic guard added in getsops#2227. Add a YAML store
round-trip regression test that fails before this change.

Fixes getsops#2243

Signed-off-by: Chris Coutinho <chris@coutinho.io>

fix: address review feedback on MAC sequence-comment fix

- Correct the explanatory comment in Tree.Decrypt: an encrypted
  non-Comment slice element whose metadata marks it a comment decrypts
  back to a Comment, so the MAC decision must use the decrypted result's
  type, not the input's.
- Move the regression test into sops_test.go, building the tree from
  SOPS objects directly and encrypting/decrypting the same tree instead
  of round-tripping through the YAML store.

Signed-off-by: Chris Coutinho <chris@coutinho.io>
…ment

Address review feedback on the sequence-comment MAC fix:

- sops.go: replace the MAC-inclusion comment with the reviewer's wording
  explaining that non-Comment values can decrypt to Comment values, so
  isComment on the input cannot be used here.
- Move TestMACWithCommentInSequence into a new external test file
  (package sops_test), letting it import and use the real aes cipher. The
  previous stub cipher existed only because an internal (package sops) test
  cannot import aes without an import cycle (aes imports the root sops
  package). Drop the stub commentCipher.
- Use assert.False for the post-encrypt Comment check, and additionally
  assert the decrypted element is restored to a Comment with its content.

Signed-off-by: Chris Coutinho <chris@coutinho.io>
@cbcoutinho cbcoutinho force-pushed the fix/mac-sequence-comment-decrypt branch from 9bdc594 to 7295afe Compare July 11, 2026 11:37
@felixfontein felixfontein merged commit 2bd88a7 into getsops:main Jul 11, 2026
15 checks passed
@felixfontein

Copy link
Copy Markdown
Contributor

@cbcoutinho thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression in 3.13.2: MAC mismatch decrypting files written by ≤3.13.1 that contain a comment on a sequence item

2 participants