Question: How to manage merge conflicts with MAC signature

[chroto]

Hi. Great stuff here. Putting a PoC together for the team using git as a storage and distribution mechanism. We are currently using https://www.passwordstore.org/ and I'm attracted to sops for its support for AWS KMS.

What are your recommendations for settling merges since they (almost?) always result in a conflict on the mac signature with neither signature going to be the correct one.

I'm comfortable with sops --ignore-mac and :wq to re-generate a mac, but this could be a roadblock for adoption since it's additional training to micro-manage the MAC every time there is a merge.

Any recommendations for reducing the scope of this problem? How do you folks handle this issue?

[jvehent]

That's a very valid point, thanks for bringing it up. Conflict resolution has not been an issue yet, because we manage secrets in small files and it's very unlikely two persons will be modifying the same file at the same time. Generally speaking, I think a lot of small files is better than a single big one.

I have not looked into what git provides, but maybe we could invoke a sops command to merge two versions of a file when a conflict occur? That way, sops would verify both MACs, then merge the files and generate a new MAC. It might end up tricky to implement, but worth investigating.

[twolfson]

I took a shot at this tonight. I couldn't find anything straightforward with .gitattributes but following @jvehent's suggestion, I started on a custom merge configuration for SOPS. Here's a proof of concept demonstrating it handling decryption (which includes MAC validation) of source files and opening the default mergetool:

Code available here:

https://gist.github.com/twolfson/962b1eb776ce9947a09d4924d91fd8b2/b18db5fbcea4f3dd4632c9df8d94f14b644c1e52

There is a snag, we need a SOPS command to replace an entire files contents while maintaining the keys. After that is built, the script should be more/less good to go (maybe port to Python for portability).

Additionally, there is nothing to handle a key rotation. Maybe we should have other commands which dump metadata so we can diff/mergetool those as well?

I should also mention that another short term alternative would be to leverage well tested git hooks for merge/rebase like those in victorious-git:

https://github.com/twolfson/victorious-git

Edit: Locked down revision for gist

[jvehent]

Very nice analysis @twolfson ! :+1: I only skimmed through your scripts, but my first impression is that we need a bunch of local git configuration, which hurts user experience a bit. Do you think these could be wrapped under a sops --merge <file> command which handles all the magical git stuff?

[twolfson]

Upon reviewing my code, the .git/config + .gitattributes are more designed for automatic dependency resolution when running git merge (there shouldn't be a mergetool window open, it's all supposed to be automated).

Instead there's git mergetool which is used by users when a conflict is encountered. We should be able to use that without any .git/config or .gitattributes:

git mergetool --tool=./sops-mergetool.sh example.yaml

Unfortunately, I'm not in a good spot to try that out at the moment but will later.

[twolfson]

Outside of that I have a couple more notes:

• For replacing an entire file's contents, we should be able to accept input from STDIN but I don't think it works (tried last night)
  • cat foo.yaml | sops example.yaml
  • sops example.yaml < foo.yaml
• For handling changing meta data like keys, maybe we should pass through meta data to the mergetool as well (maybe via a command like sops --decrypt-with-meta). For example:

# Inside the 1 column of the mergetool
foo: bar
sops:
    mac: ABCDEF
    version: 1.7
    pgp:
    -   fp: 1022470DE3F0BC54BC6AB62DE05550BC07FB1A0A
# ...

Edit: Removed data: key from YAML example

[jvehent]

For replacing an entire file's contents, we should be able to accept input from STDIN but I don't think it works (tried last night)

Two solutions:

• sops -e foo.yaml > example.yaml
• cp foo.yaml example.yaml && sops -e -i example.yaml

That omits --pgp and --kms, but those can be driven through a .sops.yaml file.

For handling changing meta data like keys, maybe we should pass through meta data to the mergetool as well (maybe via a command like sops --decrypt-with-meta).

I think you're looking for sops --show_master_keys example.yaml which is already implemented.

[twolfson]

With respect to STDIN, is foo.yaml raw data (e.g. foo: bar)? If so, how does sops -e know which keys to use when outputting content?

With respect to sops --show_master_keys, is there a way to re-encrypt from that output format? That is convert

foo: bar
sops:
    mac: ABCDEF
    version: 1.7
    pgp:
    -   fp: 1022470DE3F0BC54BC6AB62DE05550BC07FB1A0A

to

foo: ENC[AES256_GCM,data:9l9h,iv:c/QuxSHYuqV4DWLbMMg1MKEKVbsP14sKFYChHSjxJeU=,tag:EDyqP6ckK0aRhE/XcNt6WQ==,type:str]
sops:
    mac: ENC[AES256_GCM,data:eiKgBSVnGKMIMvLKYees7Q74MXCw+XYSjjOXIWuyJfRuHTNU8dFsKjBhgZBL6RHqu4M5LkzS0MAfg0l9GMV1obnnzD0noht8N/Ge3PQ8QXf2aYYsciDsmBkGksQHC2XWHLF7eheLuicHlyZ2S73i+VAs/gOBywwXR+59x58Vntc=,iv:UlCCFAdOs1HuGrya8gCeoWnW8xj4Q2IarmRxbkwPV5I=,tag:z7l7aYnRqWdfL9wnPXDr8w==,type:str]
    version: 1.7
    pgp:
    -   fp: 1022470DE3F0BC54BC6AB62DE05550BC07FB1A0A
# ...

[twolfson]

Ah, I have answered my own question in both respects. -e accepts data that already has a sops key and will use that as its keys

https://github.com/mozilla/sops/blob/f63597f901f50f07ff72452b4bdb485518b85de7/sops/__init__.py#L511-L516

So the answers are:

• No, foo.yaml isn't raw data. It is data + a SOPS branch
• Yes, we can re-encrypt sops --show_master_keys via sops -e

[twolfson]

Alright, I explored git-mergetool. Unfortunately, we would need to have users set up .git/config with a [mergetool "sops-mergetool"] to get it working. That being said, the gist is now focused on git-mergetool and feels like a good experience (after the .git/config):

https://gist.github.com/twolfson/962b1eb776ce9947a09d4924d91fd8b2/1978689a51b7b2f388121fe71a530eae145863fa

[twolfson]

After thinking for a bit, there's no reason why that script has to be used via git mergetool (and consequently configured via .git/config). As I learned during iterations, we can generate all the necessary files from the conflicted state:

https://git-scm.com/book/en/v2/Git-Tools-Advanced-Merging#_manual_remerge

git show :1:example.yaml # Contents of `$BASE`
git show :2:example.yaml # Contents of `$LOCAL`
git show :3:example.yaml # Contents of `$REMOTE`

With respect to keeping the tool agnostic (e.g. agnostic to git/hg/svg), I don't think it's plausible for git as we need to find its mergetool configuration.

[cjbottaro]

I'm using SOPS via Helm Secrets. Is there a way to turn on --ignore-mac via a sops.yaml file or some global config?

Also, I can't believe this isn't a bigger issue. We ran into this almost immediately, and we're not even that big of an engineering team.

[autrilla]

I'm using SOPS via Helm Secrets. Is there a way to turn on --ignore-mac via a sops.yaml file or some global config?

I think this is a bad idea. You should use --ignore-mac reaaaaaally sparingly. It's the only thing that ensures your file hasn't been tampered with by e.g. reordering entries.

Also, I can't believe this isn't a bigger issue. We ran into this almost immediately, and we're not even that big of an engineering team.

This is probably because of your workflow. I've never had a single merge conflict with our SOPS-encrypted files at Mozilla in over two years, and we're a 10-engineer team. We try to isolate secrets into one file per 'application' and environment.

[reidab]

This is probably because of your workflow. I've never had a single merge conflict with our SOPS-encrypted files at Mozilla in over two years, and we're a 10-engineer team. We try to isolate secrets into one file per 'application' and environment.

I agree that global disabling MAC checking is a bad idea, but I'd be curious to know more about your workflow. Like @cjbottaro, I'm using sops + helm-secrets.

We currently store secrets files in the same repos as our application code, one file per environment. The place where we most often hit MAC conflicts is when two feature branches each require adding an additional key to the secrets file, and are both merged down to master. Git cleanly merges the parallel additions, so the only conflict left to resolve at merge time is the MAC.

My current workaround, like the OP mentions, is re-saving each conflicted file sops --ignore-mac, but this doesn't seem ideal.

[autrilla]

@reidab we don't use helm-secrets, so I'm not particularly sure about the viability of our workflow in your case. We currently store secrets file in a separate repo from the application code, and the infrastructure code is in a different repo as well. We do have one file per environment and per application, so that's a similarity between our workflows. We don't hit the issue you hit because our PRs that add secrets are extremely short-lived. We basically handle secrets completely independently from the code that uses them.

I'm not sure how else this could be handled, and I'm definitely open to ideas. I suppose we could write a sops mergetool command that does the merge for you, checking the MAC of each of the branches, and then opening the (diffed) decrypted files in your editor and reencrypting the result.

[cjbottaro]

@autrilla

I think this is a bad idea. You should use --ignore-mac reaaaaaally sparingly. It's the only thing that ensures your file hasn't been tampered with by e.g. reordering entries.

I understand. Is there a way to put that setting in the .sops.yaml file though?

[autrilla]

I understand. Is there a way to put that setting in the .sops.yaml file though?

Probably not. I don't think we want to make this easy. If you really want it, I'd say you should set up a bash alias or something like that.

[jvehent]

I agree with @autrilla . Allowing the user to disable security every time is a bad idea. I like this approach better:

I suppose we could write a sops mergetool command that does the merge for you, checking the MAC of each of the branches, and then opening the (diffed) decrypted files in your editor and reencrypting the result.

[ChristianWeissCG]

How many conflicts you get, depends on:

• your workflow
• frequency of parallel changes
• the number of leave nodes in your yaml file

Sops was designed which security in mind, not with being compatible with many workflow.

You should always use sops as an interactive editor (sops filename.yaml) or modify value by value using sops --set '["path"]["to"]["key"] "newValue"'. This ensures that checksum (called "mac") is always correct and also ["sops"]["lastmodified"] and ["sops"]["version"] are maintained.

Try to avoid the non-interactive full-file workflow: decryt + change + encrypt, as it introduces in fact a key rotation and puts a lot of noise to a diff, even for not changed values and not changed comments.

This also applies to 3-way-merges. So the 3-way-merge tool should be structure-aware (better then line-based diffs) and it needs to ensure that the "selected" value is applied on top of the encryted version of your "base" file . The 3-way-merge tool should apply a sequence of single-value-patches via sops --set '["path"]["to"]["key"] "newValue"' in the background while you are seeing the plain text version of all 3 file versions. The 3-way-merge tool should display only the plain text version without the sops section. That way you should not be asked to solve conflicts in ["sops"]["mac"], ["sops"]["lastmodified"] and ["sops"]["version"] - this conflict should be solved under the hood and automatically by the above procedure.

I recomment to not use comments in yaml files. Comments are allowed by the spec but treated by that spec as stepmom children, by declaring them as "presentational detail" (similar to indention and other formating stuff). For that reason many tools like yamldiff or yamlpatch ignore comments completly. While yaml was designed as a format for applications and humans, but sops ecosystem turned out to be focused on applications. Humans are now stepmom children of the yaml spec. Not using comments would reduce the noise (encrypted comments) and also would help if diff and patch tools ignore comments.

If i really want to have a comment for '["path"]["to"]["key"] "myValue"' then i tend to add a new leave node above that key: '["path"]["to"]["key_comment"] "MyCommentGoesHere"'. And as i do not want to have my comment being encrypted i usually use '["path"]["to"]["key_comment_unencrypted"] "MyCommentGoesHere"' - this helps when reviewing it at github, gitlab or bitbucket (in the browser) or in my IDE, without forcing me to decrypt all files always. Config options that are not security related also get "_unencrypted" suffix, which also reduces a lot of noise and as they are not part of the mac, changes to them do not trigger a sops-specific conflict.

I hope we will see better tool integration in the future. Hope to see a sops --verify (to verify mac without having a master key; ci server), a sops --diff (to create a structure-aware patch file; not line-based like unix diff) and a sops --applyPatch to easy review and move config data between feature branches.

BTW: 2-way sopsdiff support via git.textconv is useless on 3-way-merges / conflicts.

[autrilla]

Hope to see a sops --verify (to verify mac without having a master key; ci server)

This is not possible, because the MAC is authenticated with the data key, which is not available unless you have a master key.

[ChristianWeissCG]

ok, thanks for this detail.

Having a plain text mac and a signature sounds like a better option as its purpose is to verify that nothing has changed, but not to hide that data. That way a sops command line tool at my build server could verify that a sops-encrypted file is valid, without being a master key owner (not involved in encryption), just need one public key of a person that signed the checksum (should be possible to be signed by more then one person) and need to define trust to at least one of that persons once.

This is not neccessary a backward compatibility break - we could use separate key in sops section for that (optional; default on newer sops releases).

[autrilla]

If the MAC isn't authenticated someone can just modify the file by reordering, duplicating, or even swapping values and recomputing the MAC, which defeats its purpose.

We'd be happy to take a patch for some way to improve the merge conflict workflow though.

[ChristianWeissCG]

MAC is encrypted with AES_GCM. What i ask for is to not encrypt it, just calculate and then sign that MAC with e.g. GPG (https://www.gnupg.org/gph/en/manual/x135.html):

In addition to authenticating branches of the tree using keys as additional data, sops computes a MAC on all the values to ensure that no value has been added or removed fraudulently.

This sounds like a simple calculation without encryption involved. A (GPG) signature on top of it should be enough. An attacker would need to change the valuse/structure, re-calculate a valid MAC and needs a valid master key from a trusted signee to trick the validation process, by creating a valid signature. But if a master key is no longer private then your encryption is also broken - so it is not worst then current solution.

Can you provide more details on how the MAC is calculated? (i am not sure if i understand "authenticating branches" correctly)

[autrilla]

MAC is encrypted with AES_GCM. What i ask for is to not encrypt it, just calculate and then sign that MAC with e.g. GPG (https://www.gnupg.org/gph/en/manual/x135.html):

What if the file is only encrypted with AWS KMS? Our master keys can be symmetric keys, which totally defeat your purpose. If you only want support for GPG, you could just sign the file yourself.

What if it's encrypted with both AWS KMS and GPG? Should the MAC only be signed by GPG?

This sounds like a simple calculation without encryption involved. A (GPG) signature on top of it should be enough. An attacker would need to change the valuse/structure, re-calculate a valid MAC and needs a valid master key from a trusted signee to trick the validation process, by creating a valid signature. But if a master key is no longer private then your encryption is also broken - so it is not worst then current solution.

Sure, it is a simple calculation with no encryption involved. But we still need a way to sign/authenticate the data that works for all our master keys. I don't have any formal security training, but my understanding is that this is not possible with symmetric keys.

Also, perhaps this should be discussed in a separate issue? It's not really relevant to how managing merge conflicts should work.

Can you provide more details on how the MAC is calculated? (i am not sure if i understand "authenticating branches" correctly)

Sure. The relevant code is here.

[ChristianWeissCG]

Thanks for the link. True. This should go to a separat ticket: #437

[ChristianWeissCG]

"--verify" is not required to implement a "sops auto merge conflict resolver", but it may prevents to commit broken sops files to branches (git hook), which will bring the "sops auto merge conflict resolver" to fail.

[stpierre]

Very surprised to see this still outstanding. :/ We've been using SOPS for about a month and have already run into this issue, in spite of using very small (per-application, per-environment) files. But if the Mozilla team is only 10 people that could explain part of it.

As a workaround, it'd be awfully nice to just have a sops --regenerate-mac command or similar, so that after a 3-way merge is resolved we can regenerate it without having to edit with --ignore-mac, make a change, and save. (At least for me, just sops --ignore-mac and exiting immediately doesn't work; it detects that no change has been made and does nothing.)

[jvehent]

we use many small files. that's why we don't run into it.

[jvehent]

As a workaround, it'd be awfully nice to just have a sops --regenerate-mac command or similar, so that after a 3-way merge is resolved we can regenerate it without having to edit with --ignore-mac, make a change, and save. (At least for me, just sops --ignore-mac and exiting immediately doesn't work; it detects that no change has been made and does nothing.)

That goes along the line of the sops mergetool idea. Someone just needs to implement it.

[stpierre]

in spite of using very small (per-application, per-environment) files.

So do we. You also have a very small team.

An excellent tool that doesn't work for medium-sized teams or medium-sized files is actually not so excellent. :(

[jvehent]

It's a matter of someone being motivated enough to write the fix and solve the issue for everyone. Are you that person?

[ChristianWeissCG]

As there is no significant development in sops and due to the issue above, i tend to recommend all my teams to use https://www.vaultproject.io/ instead of sops, as sops turned out to be not a good tool for our use case - even with adjusting our workflows to a sops-style.

@stpierre i think the same applies to you and your team. Try to use vault instead.

[autrilla]

That there is no significant development is very debatable. We still review and welcome PRs from contributors and have merged a few features lately. Just because a feature on your personal wishlist has not been implemented it doesn't mean that the project is not maintained.

Vault is a completely different beast from sops. If it fits your use case, great! But I don't see how Vault makes this problem any better. In fact, I think it makes it worse. If there's a conflict in Vault, data is just lost, no?

So do we. You also have a very small team.

An excellent tool that doesn't work for medium-sized teams or medium-sized files is actually not so excellent. :(

If you have more than 10 people not only accessing, but also editing the same secrets, I think you have a much bigger problem than SOPS not handling merge conflicts well. But again, your use case might be very different than the use cases I imagine for SOPS. Can you go into why this is the case more? It might be good to have a section in the documentation titled something along the lines of "Is SOPS for you?".

[jvehent]

As the original author of Sops and manager of the team in charge of it, I can say this: Sops is actively maintained and in heavy use in major services. It will continue being maintained for the foreseeable future.

We implement the features that we need for our own use cases at Mozilla. We also provide feedback and guidance to contributors who wish to implement the features they need for their own use cases., while keeping the project and code base sane and maintainable.

Unlike other secrets management tools, we do not have a business model around Sops. As a result, we do not provide free engineering services. If someone wants a fix for their own use case, they need to implement it themselves. This is usually how open source works, and it doesn't make Sops a lesser tool, but if you were hoping for free beer, sorry, we don't have any.

[Morriz]

Is there really no merge conflict problem in the mozilla teams working with sops? I can't believe it. It's inherent to the solution when storing the results in git. Or are you not doing that? Are you working around it in a cumbersome way, breaking git workflows?

[autrilla]

We really were doing that, it's not like we gain anything from lying about it. We worked around it by not having a huge file with all our secrets in it, but by splitting secrets into multiple files, separated per service, usually. That, plus pulling before starting to make a change meant I never encountered a merge conflict in my ~2y with the team.

While it'd be great for sops to support this, ISTM that there's also other ways to work around it -- pull before working on something, small files, and quick review turnaround so that merges are quick as well.

[Perdjesk]

It is possible to create an alias to ease the regeneration of MAC without having to open interactively an editor.

alias sops-regen-mac='EDITOR="vim -es +'"'"'norm Go'"'"' +'"'"':wq'"'"'"  sops --ignore-mac'

However this will regenerate the MAC even if there is no mismatch in the target file. It might be recommended to only to do it for files that do have mismatches.

#!/bin/bash

set -o errexit
set -o pipefail
set -o nounset

find . -name '*.enc.yaml' | while read -r file;
do
    sops -d "$file" >/dev/null 2>&1 && rc=$? || rc=$?
    # In case of MAC mismatch, then MAC is regenerated
    # See https://github.com/mozilla/sops/blob/v3.6.1/cmd/sops/codes/codes.go#L19
    if [ $rc -eq 51  ] ; then
      echo "Regenerating sops MAC for: $file"
      EDITOR="vim -es +'norm Go' +':wq'"  sops --ignore-mac "$file"
    fi
done

In our team, our git branching workflow handles the MAC mismatch git conflict in pull-requests and it is the task of authors to resolve/regenerate a correct MAC within their PRs. It is for sure a thing more to take care of but it helps to ensure file integrity.

[mitar]

In #972 I am proposing that one way to address this might be to have MAC per-value and not per-file. This could help address merging issues described here, too. (But does change security properties of your settings.)

[autrilla]

There already is a MAC per value, the AES AAD

On Sat, 18 Dec 2021 at 18:42, Mitar @.***> wrote:

In #972 https://github.com/mozilla/sops/issues/972 I am proposing that one way to address this might be to have MAC per-value and not per-file.

— Reply to this email directly, view it on GitHub https://github.com/mozilla/sops/issues/52#issuecomment-997236700, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARH4V2EURQKFV2MD7KRO43URTBW3ANCNFSM4B3KNX7A . You are receiving this because you were mentioned.Message ID: @.***>

[mitar]

Yes, I noticed this later. See my updated proposal in #972.

[mitar]

Given that though, I am not sure what exactly is the threat the per-file MAC is protecting against? That somebody manages to modify the file and put some older (but unknown, encrypted) values for some settings? But that file still has to get deployed without anyone noticing. And also, per-file MAC does not help with revertion of the whole file to some older version. So how insecure really is to always use sops --ignore-mac?

[rubber-ant]

up

[jfly]

Sorry for the bump, but I'm quite interested in the answer to @mitar's question: what is the per-file MAC really protecting against? "Reordering entries" is mentioned above, but that's only something that can happen if someone gains write-access to our repository, which is not a threat we're looking to protect against with sops. We really just want a solution better than committing secrets in plaintext.

In my view of the world: at the point that a malicious actor has the ability to tamper with your repos, they can do all sorts of game-over things (like adding backdoors to your code). If you want to ensure that all the code in a repo was written with good intent, then you should have a robust code submission/code review policy and maybe consider using signed commits.

[felixfontein]

MACs authenticate content. Sops encrypted files are not only part of git repositories, but also exist outside of these (for example, some encrypted credentials that are copied from your git repo to a target machine/VM/container). Checking the MAC while decrypting makes sure that these files weren't tampered with after they were copied out of your repo. Signed commits won't help for this.

[jfly]

Interesting. I think that makes sense, but I'm not convinced that's a problem that sops itself should be solving. IIUC, whether this problem exists (and if it's something you care about) really depends on your architecture and how many "steps"/"layers" exist between your git repos that hold sops files and the actual places where the sops binary gets invoked to decrypt those files.

By anology with code: code is also part of git repositories, but also exists outside of them (for example, when building a docker image, .deb file, python package, etc, and uploading that artifact somewhere). Is it absolutely true that we should worry about those artifacts getting tampered with, but that's not something we try to solve by signing the source code: instead, the trusted server/human that reacts to git commits and does the building/copying is responsible for signing the generated artifact.

[felixfontein]

I strongly disagree with that. (I won't comment on this any further in this issue though, since this is getting more off-topic IMO.)

[mitar]

Checking the MAC while decrypting makes sure that these files weren't tampered with after they were copied out of your repo.

Checking MAC is not enough because somebody could still roll-back to an old (possibly compromised) version of the file and MAC checking would still pass. If you worry if files are exactly the version you intend, then simply doing hash comparison of original file and destination file before deploying is good enough and it solves both the attack you are describing as MAC protecting against and roll-back attack.

[felixfontein]

Yes, it won't protect about that one. But it does protect against someone modifying the deployed config by copying the TLS private key into the also encrypted "message included in 404 page" field, and sops simply decrypting that without complaining.

[mitar]

Yes, it won't protect about that one. But it does protect against someone modifying the deployed config by copying the TLS private key into the also encrypted "message included in 404 page" field, and sops simply decrypting that without complaining.

No, because an individual field is still MACed based on the location in the tree structure.

[felixfontein]

Isn't that exactly what I said?

[mitar]

Sorry, my understanding is that you are arguing FOR whole-file MACs and were providing arguments what attacks are possible if whole-file MAC is not present. I am arguing FOR field-only MACs (which are already there, because fields are already encrypted with authenticated encryption which takes tree path as part of its additional data to authenticate). And I am arguing that whole-file MACs bring little to none extra utility, while interfering with the user experience of the tool. While security is always a trade off between security and usability, I am claiming that whole-file MACs bring little to none extra security (maybe even just a feeling of security: rollbacks are still possible of the whole file) so that we should at least have an option to have only field-based MACs (at least as opt-in).