Add script to verify all merge commits are signed #5149

trusted-keys is missing fingerprints for jgarzik/gmaxwell

Good idea, but at least mention this in contrib/README.md

Edit: Looking through the commits, all jgarzik's commits and merges are signed with: Primary key fingerprint: AF8B E07C 7049 F3A2 6B23 9D53 25B3 0832 0178 2B2F

The verify script seems overly complex to me if it's just supposed to be verifying merge commits. I took a stab at it and came up with:

#!/bin/sh
DIR="`git rev-parse --show-toplevel`/contrib/verify-commits"
VERIFIED_ROOT=`< "${DIR}/trusted-git-root"`
TEST_COMMIT="$1"
RET=0
for i in `git rev-list --min-parents=2 --max-parents=2 "${VERIFIED_ROOT}".."${TEST_COMMIT}"`; do
  git -c "gpg.program=${DIR}/gpg.sh" verify-commit $i >/dev/null || \
    ( echo "$i was not signed with a trusted key!"; false ) || RET=`expr $RET + 1`
done
exit $RET

Is there a reason for using recursive bottom-up traversal rather than the simpler top-down from the merges?

@theuni that would not catch non-merge commits that get pushed directly rather than via a merge commit.

git rev-list --first-parent seems to do the thing we want (though it seems @TheBlueMatt also wants to cover the case where a (recursively) signed commit is signed merged into a non-signed branch).

Technically, merge commit parents dont have to be in a specific order, so even though if using the merge shell script in contrib you wouldnt hit it, it is not unreasonable for someone to have been developing on their own branch, merging two pulls or so, and then merging that branch.

@TheBlueMatt Well, sure, but "git merge branch" will have the current HEAD as first parent and branch as second parent (assuming it wasn't a fast forward), so I don't see how anything merged into master should not have its first parent (and its first parents) signed.

@sipa Indeed, though my point was more that its not unreasonable to be on a separate branch and merge in master. While we try to keep our git history clean of that kind of confusion, its not incorrect and not unreasonable for it to happen, so I find it better to support it.

Updated the commit base because @gavinandresen insists on merging without signing his commits and squashed.

I believe this code can lead to exponential time in the number of merges :)

In an unrealistic worst-case, yes...if someone who's key was on the list started signing all of their commits, it is possible that it would go quite far down both sides of a merge, however, in any other case, it wont pass one commit down a merge branch that isnt followed by other merges.

One solution employed in kernel-land is hooks. I wonder how possible it is to hook something like this PR's script with a pre-commit hook that prevents pushes containing unsigned merge commits.

@jgarzik sounds good...is there a way to limit a hook to only apply if you're pushing to github.com/bitcoin/bitcoin?

@jgarzik To answer your question, yes. I updated this pull to (a) include @gmaxwell's pubkey, (b) there were several unsigned merges, so had to update the trusted root, (c) include a contrib/verify-commits/pre-push-hook.sh which, if copied to .git/hooks/pre-push (and you have bash installed), will prevent the pushing of unsigned commits to master on bitcoin/bitcoin (but not apply for any other repos).

of course, people can hit the merge button on github, and then the script will be broken after that, no?

Yes, this is why the script only applies to people with push access...if you hit the merge button, you'll require everyone else to update contrib/verify-commits/trusted-git-root (or simply disable the hook). Its enough that it'll remind everyone to sign their merges, but it doesnt block all unsigned code from getting uploaded, ie its easy to work around.