[PATCH v4 3/5] merge/pull: verify GPG signatures of commits being merged

When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.

Signed-off-by: Sebastian Götte <redacted>
---
 Documentation/merge-options.txt    |  4 +++
 builtin/merge.c                    | 33 +++++++++++++++++++++++-
 git-pull.sh                        | 10 ++++++--
 t/t7612-merge-verify-signatures.sh | 52 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..2f76ab5 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -83,6 +83,10 @@ option can be used to override --squash.
 	Pass merge strategy specific option through to the merge
 	strategy.
 
+--verify-signatures::
+--no-verify-signatures::
+	Verify that the commits being merged have good trusted GPG signatures
+
 --summary::
 --no-summary::
 	Synonyms to --stat and --no-stat; these are deprecated and will be

diff --git a/builtin/merge.c b/builtin/merge.c
index 7c8922c..227040b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -49,7 +49,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, shortlog_len = -1, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int fast_forward_only, option_edit = -1;
-static int allow_trivial = 1, have_message;
+static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
 static struct strategy **use_strategies;

@@ -199,6 +199,8 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
 		N_("abort if fast-forward is not possible")),
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
+	OPT_BOOLEAN(0, "verify-signatures", &verify_signatures,
+		N_("Verify that the named commit has a valid GPG signature")),
 	OPT_CALLBACK('s', "strategy", &use_strategies, N_("strategy"),
 		N_("merge strategy to use"), option_parse_strategy),
 	OPT_CALLBACK('X', "strategy-option", &xopts, N_("option=value"),

@@ -1233,6 +1235,35 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
 
+	if (verify_signatures) {
+		/* Verify the commit signatures */
+		for (p = remoteheads; p; p = p->next) {
+			struct commit *commit = p->item;
+			struct signature signature;
+			signature.check_result = 0;
+
+			check_commit_signature(commit, &signature);
+
+			char hex[41];
+			strcpy(hex, find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+			switch(signature.check_result){
+				case 'G':
+					if (verbosity >= 0)
+						printf(_("Commit %s has a good GPG signature by %s (key fingerprint %s)\n"), hex, signature.signer, signature.key);
+					break;
+				case 'B':
+					die(_("Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s)."), hex, signature.signer, signature.key);
+				default: /* 'N' */
+					die(_("Commit %s does not have a good GPG signature. In fact, commit %s does not have a GPG signature at all."), hex, hex);
+			}
+
+			free(signature.gpg_output);
+			free(signature.gpg_status);
+			free(signature.signer);
+			free(signature.key);
+		}
+	}
+
 	strbuf_addstr(&buf, "merge");
 	for (p = remoteheads; p; p = p->next)
 		strbuf_addf(&buf, " %s", merge_remote_util(p->item)->name);

diff --git a/git-pull.sh b/git-pull.sh
index 266e682..705940d 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -39,7 +39,7 @@ test -z "$(git ls-files -u)" || die_conflict
 test -f "$GIT_DIR/MERGE_HEAD" && die_merge
 
 strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
-log_arg= verbosity= progress= recurse_submodules=
+log_arg= verbosity= progress= recurse_submodules= verify_signatures=
 merge_args= edit=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short="${curr_branch#refs/heads/}"

@@ -125,6 +125,12 @@ do
 	--no-recurse-submodules)
 		recurse_submodules=--no-recurse-submodules
 		;;
+	--verify-signatures)
+		verify_signatures=--verify-signatures
+		;;
+	--no-verify-signatures)
+		verify_signatures=--no-verify-signatures
+		;;
 	--d|--dr|--dry|--dry-|--dry-r|--dry-ru|--dry-run)
 		dry_run=--dry-run
 		;;

@@ -283,7 +289,7 @@ true)
 	eval="$eval --onto $merge_head ${oldremoteref:-$merge_head}"
 	;;
 *)
-	eval="git-merge $diffstat $no_commit $edit $squash $no_ff $ff_only"
+	eval="git-merge $diffstat $no_commit $verify_signatures $edit $squash $no_ff $ff_only"
 	eval="$eval  $log_arg $strategy_args $merge_args $verbosity $progress"
 	eval="$eval \"\$merge_name\" HEAD $merge_head"
 	;;

diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
new file mode 100755
index 0000000..31b67dd
--- /dev/null
+++ b/t/t7612-merge-verify-signatures.sh

@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='merge signature verification tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
+
+test_expect_success GPG 'create signed commits' '
+	echo 1 >file && git add file &&
+	test_tick && git commit -m initial &&
+	git tag initial &&
+
+	git checkout -b side-signed &&
+	echo 3 >elif && git add elif &&
+	test_tick && git commit -S -m "signed on side" &&
+	git checkout initial &&
+
+	git checkout -b side-unsigned &&
+	echo 3 >foo && git add foo &&
+	test_tick && git commit -m "unsigned on side" &&
+	git checkout initial &&
+
+	git checkout -b side-bad &&
+	echo 3 >bar && git add bar &&
+	test_tick && git commit -S -m "bad on side" &&
+	git cat-file commit side-bad >raw &&
+	sed -e "s/bad/forged bad/" raw >forged &&
+	git hash-object -w -t commit forged >forged.commit &&
+	git checkout initial &&
+
+	git checkout master
+'
+
+test_expect_success GPG 'merge unsigned commit with verification' '
+	test_must_fail git merge --ff-only --verify-signatures side-unsigned 2> mergeerror &&
+	grep "does not have a GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge commit with bad signature with verification' '
+	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2> mergeerror &&
+	grep "has a bad GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+	git merge -v --ff-only --verify-signatures side-signed > mergeoutput &&
+	grep "has a good GPG signature" mergeoutput
+'
+
+test_expect_success GPG 'merge commit with bad signature without verification' '
+	git merge $(cat forged.commit)
+'
+
+test_done

-- 
1.8.1.5

Sebastian Götte [off-list ref] writes:

When --verify-signatures is specified on the command-line of git-merge
or git-pull, check whether the commits being merged have good gpg
signatures and abort the merge in case they do not. This allows e.g.
auto-deployment from untrusted repo hosts.

Signed-off-by: Sebastian Götte <redacted>
---
...
+	if (verify_signatures) {
+		/* Verify the commit signatures */
+		for (p = remoteheads; p; p = p->next) {
+			struct commit *commit = p->item;
+			struct signature signature;
+			signature.check_result = 0;

Even though you may happen to know all other fields are output only,
it is a good practice to clear it with

			memset(&signature, 0, sizeof(signature);

By the way, I think this variable and type should be called more
like "signature_check" or something with "check" in its name, not
"signature".  After all it is _not_ a signature itself.

+			check_commit_signature(commit, &signature);
+
+			char hex[41];

builtin/merge.c: In function 'cmd_merge':
builtin/merge.c:1247: error: ISO C90 forbids mixed declarations and code

+
+test_expect_success GPG 'merge unsigned commit with verification' '
+	test_must_fail git merge --ff-only --verify-signatures side-unsigned 2> mergeerror &&

No SP between redirection and the target filename.

+	grep "does not have a GPG signature" mergeerror

Do we plan to make this message localized?  If so I think you would
need to do this with test_i18ngrep.

+'
+
+test_expect_success GPG 'merge commit with bad signature with verification' '
+	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2> mergeerror &&
+	grep "has a bad GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+	git merge -v --ff-only --verify-signatures side-signed > mergeoutput &&
+	grep "has a good GPG signature" mergeoutput
+'

Hmph.

So the caller needs to check both the standard output and the
standard error to get the whole picture?  Is there a reason why we
are not sending everything to the standard output consistently?

+test_expect_success GPG 'merge commit with bad signature without verification' '
+	git merge $(cat forged.commit)
+'

Good to see that negative case where the new feature should _not_
trigger is properly tested.

+
+test_done

I hope I did not introduce more problems than I fixed in this revision ;)

On 03/28/2013 11:33 PM, Junio C Hamano wrote:

It would be much easier to read if it were "unless we are not
looking at the very first byte, the previous byte must be LF", i.e.

	if (found != buf && found[-1] != '\n')

Is that continue correct?  Don't you want to retry from the end of
the line that contains the mistaken hit?

Actually it is not. Sorry.

The "\n" at the beginning anchors the expected string for quicker
multi-line scan done with strstr().  If you really want to lose that
LF and still write this function correctly and clearly, I think you
would need to iterate over the buffer line by line.

The function now does that and calls prefixcmp on each line.

On 03/28/2013 11:33 PM, Junio C Hamano wrote:> Sebastian Götte [off-list ref] writes:

quoted

+	if (verify_signatures) {
+		/* Verify the commit signatures */
+		for (p = remoteheads; p; p = p->next) {
+			struct commit *commit = p->item;
+			struct signature signature;
+			signature.check_result = 0;

[...] 
By the way, I think this variable and type should be called more
like "signature_check" or something with "check" in its name, not
"signature".  After all it is _not_ a signature itself.

I renamed it "signature_check". I put that into the commit moving the code to
commit.c. I also renamed the array/struct containing the GPG status output
strings since that was originally called "signature_check".

quoted

+	grep "does not have a GPG signature" mergeerror

Do we plan to make this message localized?  If so I think you would
need to do this with test_i18ngrep.

Yes, this message should be localized since it is "normal" status output. I
fixed the test case, however I noticed a possible problem with the "git log
 --show-signature" test case (t/t7510-signed-commit.sh): Here, grep is used on
git output, but this git output is actually just passed-through GPG output, and
GPG localizes that output. Are the tests alwasy run with LANG=en_US.utf-8,
LANG=C etc.?

quoted

+test_expect_success GPG 'merge commit with bad signature with verification' '
+	test_must_fail git merge --ff-only --verify-signatures $(cat forged.commit) 2> mergeerror &&
+	grep "has a bad GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+	git merge -v --ff-only --verify-signatures side-signed > mergeoutput &&
+	grep "has a good GPG signature" mergeoutput
+'

Hmph.

So the caller needs to check both the standard output and the
standard error to get the whole picture?  Is there a reason why we
are not sending everything to the standard output consistently?

If --verify-signatures is given, everything but a good signature results in
"die(_("foo")), with _("foo") being printed on stderr. I clarified that point
in merge-options.txt. If additionally --verbose is given on the command line,
git will print _("Commit %s has a good GPG signature by %s (key fingerprint
%s)\n") on stdout for each "good" commit.  I think it is ok that way because
the caller only needs to check the exit code to get a picture. The "good GPG
signature"-message is only meant to convey the signer's name and key
fingerprint in case the caller is interested. If the caller does not want to
abort the merge in case of GPG trouble, git merge may be called without
 --verify-signatures followed by a git log --show-signature on the commits
that have been merged.

Sebastian Götte (5):
  Move commit GPG signature verification to commit.c
  commit.c/GPG signature verification: Also look at the first GPG status
    line
  merge/pull: verify GPG signatures of commits being merged
  merge/pull Check for untrusted good GPG signatures
  pretty printing: extend %G? to include 'N' and 'U'

 Documentation/merge-options.txt    |   5 ++
 Documentation/pretty-formats.txt   |   3 +-
 builtin/merge.c                    |  35 +++++++++++++-
 commit.c                           |  67 ++++++++++++++++++++++++++
 commit.h                           |  10 ++++
 git-pull.sh                        |  10 +++-
 gpg-interface.h                    |   8 ++++
 pretty.c                           |  93 ++++++-------------------------------
 t/lib-gpg/pubring.gpg              | Bin 1164 -> 2359 bytes
 t/lib-gpg/random_seed              | Bin 600 -> 600 bytes
 t/lib-gpg/secring.gpg              | Bin 1237 -> 3734 bytes
 t/lib-gpg/trustdb.gpg              | Bin 1280 -> 1360 bytes
 t/t7612-merge-verify-signatures.sh |  61 ++++++++++++++++++++++++
 13 files changed, 210 insertions(+), 82 deletions(-)
 create mode 100755 t/t7612-merge-verify-signatures.sh

-- 
1.8.1.5