Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging encrypted files with conflicts produces encrypted content in the "merged" file #69

Closed
jmurty opened this issue Nov 1, 2019 · 4 comments

Comments

@jmurty
Copy link
Collaborator

jmurty commented Nov 1, 2019

When you merge branches containing transcrypt-encrypted files, if the files have conflicts the resulting "merged" file produced by Git for manual resolution ends up as a real mess, mostly or entirely full of the encrypted text which cannot be sensibly merged.

The root problem seems to be that git does not run the smudge/textconv filter on all the BASE, LOCAL, REMOTE conflicting versions of files before attempting a three-way merge, and instead attempts the merge using at least one – and maybe all – of the encrypted versions of these files. E.g. the encrypted content as available from git show :1:FILENAME and git show :2:FILENAME etc mid-merge.

I have not been able to find a workaround just within Git, and have instead added a custom "crypt" merge driver to handle this situation.

Here is a PR against our fork of Transcrypt as an example: ixc#1

This change isn't yet fully tested, and is written against a fairly old version of transcrypt.

Let me know if I should bring it up to date and submit a PR here directly to add the "crypt" merge driver to the latest version of transcrypt?

@jmurty
Copy link
Collaborator Author

jmurty commented Dec 6, 2019

In case it helps someone else, here is our latest merge driver script that fixes merging of transcrypt-encrypted files whether or not the merged changes result in conflicts.

You can see the transcrypt (1.1.0) changes necessary to fully support this on the branch https://github.com/ixc/transcrypt/commits/fix-merge-with-conflicts

You enable the driver by:

  • adding this block to .git/config:

    [merge "crypt"]
        driver = \"$(git rev-parse --git-common-dir)\"/crypt/merge %O %A %B %L %P
        name = Merge transcrypt secret files
    
  • for each transcrypt-ed file entry in .gitattributes add merge=crypt like so:

    *.secret* filter=crypt diff=crypt merge=crypt
    
  • save the following merge driver script to .git/crypt/merge

#!/usr/bin/env bash
# Decrypt BASE, LOCAL, and REMOTE versions of file being merged
echo "$(cat $1 | ./.git/crypt/smudge)" > $1
echo "$(cat $2 | ./.git/crypt/smudge)" > $2
echo "$(cat $3 | ./.git/crypt/smudge)" > $3

# Merge the decrypted files to the working copy named by $5
# We must redirect stdout to $5 here instead of letting merge-file write to
# $2 as it would by default, because we need $5 to contain the final result
# content so a later crypt `clean` generates the correct hash salt value
git merge-file --stdout --marker-size=$4 -L local -L base -L remote $2 $1 $3 > $5

if [[ "$?" == "0" ]]; then
  # If the merge was successful (no conflicts) re-encrypt the merged working
  # copy file to the incoming "local" temp file $2 which git will then
  # update in the index during the "Auto-merging" step.
  # Git needs the cleaned copy to avoid triggering the error:
  #     error: add_cacheinfo failed to refresh for path 'FILE'; merge aborting.
  echo "$(cat $5 | ./.git/crypt/clean $5)" > $2
  exit 0
else
  # If the merge was not successful, copy the merged working copy file to the
  # "local" temp file $2 which git will then re-copy back to the working copy
  # so the user can fix it manually
  cp $5 $2
  exit 1
fi

@jmurty
Copy link
Collaborator Author

jmurty commented Apr 7, 2020

A fix for this issue is now on the https://github.com/elasticdog/transcrypt/tree/legacy-v1 branch pending possible inclusion in a future update to the legacy version 1 stream, and definite inclusion in the version 2 stream.

@jmurty jmurty self-assigned this Apr 7, 2020
@jmurty
Copy link
Collaborator Author

jmurty commented Apr 7, 2020

Tickets that are probably related to this one: #8 #23 #67

jmurty added a commit that referenced this issue Apr 12, 2020
Fix merged and adapted from version on legacy version 1 branch.

  Clarify grouping & source of crypt tests
  Apply latest test improvements from master branch
  Add merge=crypt to setup instructions and detailed change notes
  Update tests to account for merge fixes
  Add test showing successful and unsuccessful merges
  Add bats-core tests of core functionality and Travis config
  Fix handling of successfully merged encrypted files
  Fix merge of encrypted files with conflicts

# Conflicts:
#	README.md
#	tests/test_crypt.bats
#	tests/test_init.bats
#	transcrypt
jmurty added a commit that referenced this issue Apr 12, 2020
Show actual names of the target and incoming branch/ref in conflict
markers for merges, instead of the generic "local" and "remote" we
used for these markers previously.

This required much more digging through Git entrails than I had
expected...
jmurty added a commit that referenced this issue Apr 12, 2020
Show actual names of the target and incoming branch/ref in conflict
markers for merges, instead of the generic "local" and "remote" we
used for these markers previously.

This required much more digging through Git entrails than I had
expected...
jmurty added a commit that referenced this issue Apr 14, 2020
* legacy-v1:
  #69 Improve target and source names used for merge conflict markers
  Clarify grouping & source of crypt tests
  Apply latest test improvements from master branch
  Add merge=crypt to setup instructions and detailed change notes
  Update tests to account for merge fixes
  Add test showing successful and unsuccessful merges
  Add bats-core tests of core functionality and Travis config
  Fix handling of successfully merged encrypted files
  Fix merge of encrypted files with conflicts
jmurty added a commit that referenced this issue Apr 14, 2020
…t-hook-safety-check

* add-pre-commit-hook-safety-check:
  Don't rely on transcrypt being available to pre-commit hook script
  Improve installation and clean-up of pre-commit hook
  Improve and test warning when pre-commit hook cannot be installed
  Mention pre-commit hook addition in README
  Add tests of pre-commit hook
  #69 Improve target and source names used for merge conflict markers
  Clarify grouping & source of crypt tests
  Apply latest test improvements from master branch
  Add merge=crypt to setup instructions and detailed change notes
  Update tests to account for merge fixes
  Add test showing successful and unsuccessful merges
  Add bats-core tests of core functionality and Travis config
  Fix handling of successfully merged encrypted files
  Allow commit of empty encrypted files
  Add Git pre-commit hook to abort commit with unencrypted files
  Fix merge of encrypted files with conflicts

# Conflicts:
#	README.md
#	tests/test_crypt.bats
#	tests/test_init.bats
#	transcrypt
jmurty added a commit that referenced this issue Apr 27, 2020
…s-with-conflicts

Fix `transcrypt`'s handling of merges where encrypted files have conflicting changes, a situation which would lead to Git producing "merged" files with conflict markers around partially- or fully-encrypted content that cannot be sensibly merged by a person. See issue #69 and a bunch of related issues.

The root problem is that git does not run the `smudge`/`textconv` filter on all BASE, LOCAL, REMOTE conflicting version files before attempting a three-way merge.

This change adds:
- a merge driver script to pre-decrypt conflicting BASE, LOCAL, and REMOTE file  versions then run git's internal `merge-file`  command to merge the decrypted versions
- git repo settings to configure the merge driver
- recommendation to add the extra `merge=crypt`  setting to *.gitattribute* definitions
- tests of merge functionality to prove that non-conflicting and conflicting merges work.

Also included are minor listing and formatting fixes from applying the recommended tools to do this clean-up, and documentation for how to run these tools in *README.md*

The bulk of this work is originally from https://github.com/ixc/transcrypt/commits/fix-merge-with-conflicts
@jmurty
Copy link
Collaborator Author

jmurty commented Apr 27, 2020

Fix merged to master branch, to be included in the next release

@jmurty jmurty closed this as completed Apr 27, 2020
@jmurty jmurty removed their assignment Apr 29, 2020
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

No branches or pull requests

1 participant