comment_encryption: decouple comment encryption from value encryption - #2282
Closed
AnuchitO wants to merge 1 commit into
Closed
comment_encryption: decouple comment encryption from value encryption#2282AnuchitO wants to merge 1 commit into
AnuchitO wants to merge 1 commit into
Conversation
opt-in creation_rule/CLI setting: comment_encryption: plaintext|encrypted. before: comment encryption always tied to whatever selector governed values, no way to keep comments readable while encrypting the value under them. - shouldBeEncrypted: one guard, comment nodes only, when set. value logic + unset case untouched - compatible w/ 4 value selectors; rejected w/ encrypted/unencrypted_comment_regex (both already control comments). 6-selector exclusion untouched - wired: config.go (.sops.yaml), stores.go (persisted, for correct decrypt/rotate), cmd/sops (--comment-encryption flag, warn+ignore on binary) - shared validation in sops.ValidateCommentEncryption, no triplication - tests: all 4 value/comment combos, backward compat, config+store round-trip/validation, full 6-selector compat matrix - verified e2e w/ real age keys: dotenv/yaml/ini/json/binary Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: AnuchitO <3372362+AnuchitO@users.noreply.github.com>
Author
|
Closing to review before this goes to maintainers — will reopen or resubmit once I've gone through it myself. |
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
2 participants
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.
What
opt-in
comment_encryption: plaintext|encrypted. controls comment encryption independent of value encryption. today comment encryption is always tied to whichever of the 6 existing selectors is active — no way to keep comments (e.g. inline docs like# required | default: db) readable while the value under them is encrypted, or vice versa.How
Metadata.CommentEncryption+ one guard inshouldBeEncrypted, comment nodes only, when set. value-node logic and the unset/default case untouched (backward compat)encrypted_regex/unencrypted_regex/encrypted_suffix/unencrypted_suffix); rejected w/encrypted_comment_regex/unencrypted_comment_regex(both already decide comment encryption — combining is ambiguous). existing 6-selector mutual exclusion untouchedconfig.go(.sops.yaml),stores.go(persisted in the file's ownsopsmetadata, for correct decrypt/rotate),cmd/sops(--comment-encryptionflag, ignored w/ warning on single-value stores like binary)sops.ValidateCommentEncryption, used by all 3 call sites instead of triplicating itTests
agekeys across dotenv/YAML/INI/JSON/binaryNote
found an unrelated pre-existing bug while testing (MAC mismatch decrypting a YAML/JSON sequence when
encrypted_comment_regexactually encrypts a sequence comment — reproduces onmainw/ zero involvement of this PR). filed separately as #2281, not touched here — flagging for visibility since it's in the same area ofsops.go.comment_encryptionnaming/values open to feedback.