Support using comments to select parts to encrypt #1392
Conversation
|
@mitar I think the two comments I added address the issues from my review in #974 (review). Can you take a look at the commits? |
| if ok && tree.Metadata.UnencryptedCommentRegex != "" { | ||
| // If an encrypted comment matches tree.Metadata.UnencryptedCommentRegex, decryption will fail | ||
| // as the MAC does not match, and the commented value will not be decrypted. | ||
| matched, _ := regexp.Match(tree.Metadata.UnencryptedCommentRegex, []byte(in.(string))) |
There was a problem hiding this comment.
Why not use here MatchString?
Didn't we say we will also try to match against EncryptedCommentRegex? So both of them should not match the resulting encrypted string?
There was a problem hiding this comment.
I originally implemented this, and then when trying to test it I removed the code again. As opposed to UnencryptedCommentRegex, encrypted comments matching EncryptedCommentRegex aren't a problem since comments are only encrypted/decrypted if they are part of an encrypted subtree that's encrypted due to a comment higher up.
For example, you can encrypt
---
#ENC[AES256_GCM,data:MI3A,iv:c9lHpZCbz3W9ooGuGbmQTcWcXAH7JEL/5JyJdC41dec=,tag:LrQLBEgQayguId5RrNiOCA==,type:comment]
foo:
bar: barbar #ENC[AES256_GCM,data:MI3A,iv:c9lHpZCbz3W9ooGuGbmQTcWcXAH7JEL/5JyJdC41dec=,tag:LrQLBEgQayguId5RrNiOCA==,type:comment]
# something else
baz:
bam: bambam #ENC[AES256_GCM,data:MI3A,iv:c9lHpZCbz3W9ooGuGbmQTcWcXAH7JEL/5JyJdC41dec=,tag:LrQLBEgQayguId5RrNiOCA==,type:comment]with --encrypted-comment-regex ENC and then decrypt it again without any problems.
We could still implement the check to be extra cautious, but it isn't needed (or I'm missing something :) ).
There was a problem hiding this comment.
OK. I think it is then fine and we can revisit this if anyone finds a realistic case where it is breaking some user flow.
|
Thanks. I think this looks good as it is already. Thank you for all the work. I made a small comment, but I think it is not critical to do it. |
felixfontein
force-pushed
the
pr-974-continue
branch
from
de3e3ac
to
46c488a
Compare
|
Now that #1389 has been merged, this is ready for review! 🎉 |
| if vIsComment { | ||
| // If v is a comment, we add it to the slice of active comments. | ||
| // This allows us to also encrypt comments themselves by enabling encryption in a prior comment. | ||
| commentsStack[len(commentsStack)-1] = append(commentsStack[len(commentsStack)-1], c.Value) |
There was a problem hiding this comment.
Is there (n)ever a theoretical chance of len(commentsStack) == 0?
There was a problem hiding this comment.
That can't happen since in the first line of the function, one element is appended to commentsStack, and during the execution of this function no value is removed from commentsStack.
Signed-off-by: Mitar <[email protected]>
…ments. Signed-off-by: Felix Fontein <[email protected]>
Signed-off-by: Felix Fontein <[email protected]>
felixfontein
force-pushed
the
pr-974-continue
branch
from
46c488a
to
e55463f
Compare
|
This looks good, any idea when this could be merged ? |
|
It's currently waiting for further reviews / approval by maintainer(s). |
|
Is there anything to do to push this further? |
This PR adds support to annotate comments with a string (e.g.,
sops:enc) which can then be matched with a regex. If it matches, the corresponding value (the one which follows the comment) is encrypted while other values are not. (There is also the opposite regex available, to select those values which should not be encrypted.)This enables the YAML file to have the same structure encrypted and decrypted, without having to add suffixes or manage complex regexes to match keys. See #543 for more discussion.
(This PR currently contains #1389 and will be rebased once that's merged. It is a lot more likely that #1389 will be merged before this PR or any of its variants, so I decided to use that one as a basis.)This PR continues the work of #974 by rebasing #974 upon #1389 and adding some final touches (see #974 (comment)).
Closes #974.