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

Subsystems: documentation, the rest

DORMANTno replies

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Sebastian Götte <hidden>
Date: 2016-06-15 22:56:34

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    |  5 ++++
 builtin/merge.c                    | 33 +++++++++++++++++++++++-
 git-pull.sh                        | 10 ++++++--
 t/t7612-merge-verify-signatures.sh | 52 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 0bcbe0a..31f1067 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -83,6 +83,11 @@ 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 GPG signatures and abort the
+	merge in case they do not.
+
 --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..cb3e9ea 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;
+			char hex[41];
+			struct signature_check signature_check;
+			memset(&signature_check, 0, sizeof(signature_check));
+
+			check_commit_signature(commit, &signature_check);
+
+			strcpy(hex, find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+			switch(signature_check.check_result){
+				case 'G':
+					if (verbosity >= 0)
+						printf(_("Commit %s has a good GPG signature by %s (key fingerprint %s)\n"), hex, signature_check.signer, signature_check.key);
+					break;
+				case 'B':
+					die(_("Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s)."), hex, signature_check.signer, signature_check.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_check.gpg_output);
+			free(signature_check.gpg_status);
+			free(signature_check.signer);
+			free(signature_check.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..6ccfbf3
--- /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 &&
+	test_i18ngrep "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 &&
+	test_i18ngrep "has a bad GPG signature" mergeerror
+'
+
+test_expect_success GPG 'merge signed commit with verification' '
+	git merge --verbose --ff-only --verify-signatures side-signed >mergeoutput &&
+	test_i18ngrep "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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:34

Sebastian Götte [off-list ref] writes:
+	OPT_BOOLEAN(0, "verify-signatures", &verify_signatures,
+		N_("Verify that the named commit has a valid GPG signature")),
Please use OPT_BOOL() in new code.  Verifying existing OPT_BOOLEAN()
can safely converted to OPT_BOOL() and doing so would be a separate
matter and should not be part of this series.
quoted hunk
@@ -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 */
This boolean variable is named clearly enough that you do not need
this comment.
+		for (p = remoteheads; p; p = p->next) {
+			struct commit *commit = p->item;
+			char hex[41];
+			struct signature_check signature_check;
+			memset(&signature_check, 0, sizeof(signature_check));
+
+			check_commit_signature(commit, &signature_check);
+
+			strcpy(hex, find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+			switch(signature_check.check_result){
+				case 'G':
+					if (verbosity >= 0)
+						printf(_("Commit %s has a good GPG signature by %s (key fingerprint %s)\n"), hex, signature_check.signer, signature_check.key);
+					break;
+				case 'B':
+					die(_("Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s)."), hex, signature_check.signer, signature_check.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);
+			}
Style.

        switch (expr) {
	case 'G':
		do_something_for_G();
		break;
		...
	}

Also avoid overlong lines, both in the source, but pay extra
attention to what we show the user.  For example:

    "Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s)."

The first %s will expand to 40 places, the other two are likely to
be around 20-30 places.

    "Commit %s does not have a good GPG signature. In fact, commit %s does not have a GPG signature at all."

Drop everything from the beginning up to "In fact, ", perhaps:

    "Commit '%s' does not have any GPG signature."

is sufficient?  You may also want to consider

	die(_("Commit '%.*s...' does not have any GPG signature."),
	    8, hex);

[PATCH v6 0/5] Verify GPG signatures when merging and extend %G? pretty string

From: Sebastian Götte <hidden>
Date: 2016-06-15 22:56:34

Yuck.  That termination condition is horrible.
Ok, I reverted it to your suggestion. In this case, a much more elegant
termination condition (foo == 1) is not possible without casts because foo is a
pointer. 
quoted
+			switch(signature_check.check_result){
+				case 'G':
+					if (verbosity >= 0)
+						printf(_("Commit %s has a good GPG signature by %s (key fingerprint %s)\n"), hex, signature_check.signer, signature_check.key);
+					break;
+				case 'B':
+					die(_("Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s)."), hex, signature_check.signer, signature_check.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);
+			}
Style.
I moved that verbose printf outside the switch(foo).
Also avoid overlong lines, both in the source, but pay extra
attention to what we show the user.  For example:

    "Commit %s has a bad GPG signature allegedly by %s (key fingerprint %s)."

The first %s will expand to 40 places, the other two are likely to
be around 20-30 places.
The first %s is the output of find_unique_abbrev, so though it will be 40
places worst-case, it will usually be *much* shorter (more like 7-8 places).
    "Commit %s does not have a good GPG signature. In fact, commit %s does not have a GPG signature at all."

Drop everything from the beginning up to "In fact, ", perhaps:

    "Commit '%s' does not have any GPG signature."

is sufficient?  You may also want to consider

	die(_("Commit '%.*s...' does not have any GPG signature."),
	    8, hex);
I shortened these messages and removed the key fingerprint output. Anybody
interested can use git log --show-signature to get these.

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                    |  34 +++++++++++++-
 commit.c                           |  68 +++++++++++++++++++++++++++
 commit.h                           |  10 ++++
 git-pull.sh                        |  10 +++-
 gpg-interface.h                    |  12 +++++
 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, 214 insertions(+), 82 deletions(-)
 create mode 100755 t/t7612-merge-verify-signatures.sh

-- 
1.8.1.5
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help