From edc4d47d54cb1c0ec1550aa50bba86b23720a72f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 6 Nov 2018 02:50:17 -0500 Subject: merge: extract verify_merge_signature() helper The logic to implement "merge --verify-signatures" is inline in cmd_merge(), but this site misses some cases. Let's extract the logic into a function so we can call it from more places. We'll move it to commit.[ch], since one of the callers (git-pull) is outside our source file. This function isn't all that general (after all, its main function is to exit the program) but it's not worth trying to fix that. The heavy lifting is done by check_commit_signature(), and our purpose here is just sharing the die() logic. We'll mark it with a comment to make that clear. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/merge.c | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) (limited to 'builtin') diff --git a/builtin/merge.c b/builtin/merge.c index 8f4a5065c2..e1bc5a484b 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1355,31 +1355,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (verify_signatures) { for (p = remoteheads; p; p = p->next) { - struct commit *commit = p->item; - char hex[GIT_MAX_HEXSZ + 1]; - struct signature_check signature_check; - memset(&signature_check, 0, sizeof(signature_check)); - - check_commit_signature(commit, &signature_check); - - find_unique_abbrev_r(hex, &commit->object.oid, DEFAULT_ABBREV); - switch (signature_check.result) { - case 'G': - break; - case 'U': - die(_("Commit %s has an untrusted GPG signature, " - "allegedly by %s."), hex, signature_check.signer); - case 'B': - die(_("Commit %s has a bad GPG signature " - "allegedly by %s."), hex, signature_check.signer); - default: /* 'N' */ - die(_("Commit %s does not have a GPG signature."), hex); - } - if (verbosity >= 0 && signature_check.result == 'G') - printf(_("Commit %s has a good GPG signature by %s\n"), - hex, signature_check.signer); - - signature_check_clear(&signature_check); + verify_merge_signature(p->item, verbosity); } } -- cgit v1.2.3 From 7488ba3eeaaaeb8c0e06cca12384f9f7f76082b5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 6 Nov 2018 02:51:15 -0500 Subject: merge: handle --verify-signatures for unborn branch When git-merge sees that we are on an unborn branch (i.e., there is no HEAD), it follows a totally separate code path than the usual merge logic. This code path does not know about verify_signatures, and so we fail to notice bad or missing signatures. This has been broken since --verify-signatures was added in efed002249 (merge/pull: verify GPG signatures of commits being merged, 2013-03-31). In an ideal world, we'd unify the flow for this case with the regular merge logic, which would fix this bug and avoid introducing similar ones. But because the unborn case is so different, it would be a burden on the rest of the function to continually handle the missing HEAD. So let's just port the verification check to this special case. Reported-by: Felix Eckhofer Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/merge.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'builtin') diff --git a/builtin/merge.c b/builtin/merge.c index e1bc5a484b..05ea0bbe01 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1334,6 +1334,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) die(_("%s - not something we can merge"), argv[0]); if (remoteheads->next) die(_("Can merge only exactly one commit into empty head")); + + if (verify_signatures) + verify_merge_signature(remoteheads->item, verbosity); + remote_head_oid = &remoteheads->item->object.oid; read_empty(remote_head_oid, 0); update_ref("initial pull", "HEAD", remote_head_oid, NULL, 0, -- cgit v1.2.3 From 01a31f3bcaae8b62e5e11ee12d7b1606700f0721 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 6 Nov 2018 02:52:13 -0500 Subject: pull: handle --verify-signatures for unborn branch We usually just forward the --verify-signatures option along to git-merge, and trust it to do the right thing. However, when we are on an unborn branch (i.e., there is no HEAD yet), we handle this case ourselves without even calling git-merge. And in this code path, we do not respect the verification option at all. It may be more maintainable in the long run to call git-merge for the unborn case. That would fix this bug, as well as prevent similar ones in the future. But unfortunately it's not easy to do. As t5520.3 demonstrates, there are some special cases that git-merge does not handle, like "git pull .. master:master" (by the time git-merge is invoked, we've overwritten the unborn HEAD). So for now let's just teach git-pull to handle this feature. Reported-by: Felix Eckhofer Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/pull.c | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'builtin') diff --git a/builtin/pull.c b/builtin/pull.c index 681c127a07..96c795f581 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -556,6 +556,17 @@ static int run_fetch(const char *repo, const char **refspecs) static int pull_into_void(const struct object_id *merge_head, const struct object_id *curr_head) { + if (opt_verify_signatures) { + struct commit *commit; + + commit = lookup_commit(the_repository, merge_head); + if (!commit) + die(_("unable to access commit %s"), + oid_to_hex(merge_head)); + + verify_merge_signature(commit, opt_verbosity); + } + /* * Two-way merge: we treat the index as based on an empty tree, * and try to fast-forward to HEAD. This ensures we will not lose -- cgit v1.2.3