From patchwork Tue Nov 6 07:50:17 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10669853 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 5C6E714E2 for ; Tue, 6 Nov 2018 07:50:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4D24029B2F for ; Tue, 6 Nov 2018 07:50:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 41AB229B5E; Tue, 6 Nov 2018 07:50:21 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CB2E329B2F for ; Tue, 6 Nov 2018 07:50:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729488AbeKFROP (ORCPT ); Tue, 6 Nov 2018 12:14:15 -0500 Received: from cloud.peff.net ([104.130.231.41]:41824 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1729160AbeKFROP (ORCPT ); Tue, 6 Nov 2018 12:14:15 -0500 Received: (qmail 17702 invoked by uid 109); 6 Nov 2018 07:50:19 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 06 Nov 2018 07:50:19 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29085 invoked by uid 111); 6 Nov 2018 07:49:37 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 06 Nov 2018 02:49:37 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 06 Nov 2018 02:50:17 -0500 Date: Tue, 6 Nov 2018 02:50:17 -0500 From: Jeff King To: git@vger.kernel.org Cc: Felix Eckhofer , Junio C Hamano Subject: [PATCH 1/3] merge: extract verify_merge_signature() helper Message-ID: <20181106075017.GA22021@sigill.intra.peff.net> References: <20181106074910.GA31978@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181106074910.GA31978@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- builtin/merge.c | 26 +------------------------- commit.c | 26 ++++++++++++++++++++++++++ commit.h | 7 +++++++ 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 4aa6071598..c677f9375e 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1357,31 +1357,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); } } diff --git a/commit.c b/commit.c index d566d7e45c..e1428aed6d 100644 --- a/commit.c +++ b/commit.c @@ -1100,7 +1100,33 @@ int check_commit_signature(const struct commit *commit, struct signature_check * return ret; } +void verify_merge_signature(struct commit *commit, int verbosity) +{ + 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); +} void append_merge_tag_headers(struct commit_list *parents, struct commit_extra_header ***tail) diff --git a/commit.h b/commit.h index 6c4428c593..a98cca635a 100644 --- a/commit.h +++ b/commit.h @@ -331,6 +331,13 @@ extern int remove_signature(struct strbuf *buf); */ extern int check_commit_signature(const struct commit *commit, struct signature_check *sigc); +/* + * Verify a single commit with check_commit_signature() and die() if it is not + * a good signature. This isn't really suitable for general use, but is a + * helper to implement consistent logic for pull/merge --verify-signatures. + */ +void verify_merge_signature(struct commit *commit, int verbose); + int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused); int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused); From patchwork Tue Nov 6 07:51:15 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10669855 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 09D811751 for ; Tue, 6 Nov 2018 07:51:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EE41629B2D for ; Tue, 6 Nov 2018 07:51:19 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E027429D7D; Tue, 6 Nov 2018 07:51:19 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AFBBC29B2D for ; Tue, 6 Nov 2018 07:51:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729628AbeKFRPO (ORCPT ); Tue, 6 Nov 2018 12:15:14 -0500 Received: from cloud.peff.net ([104.130.231.41]:41834 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1729160AbeKFRPN (ORCPT ); Tue, 6 Nov 2018 12:15:13 -0500 Received: (qmail 17760 invoked by uid 109); 6 Nov 2018 07:51:18 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 06 Nov 2018 07:51:18 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29103 invoked by uid 111); 6 Nov 2018 07:50:35 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 06 Nov 2018 02:50:35 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 06 Nov 2018 02:51:15 -0500 Date: Tue, 6 Nov 2018 02:51:15 -0500 From: Jeff King To: git@vger.kernel.org Cc: Felix Eckhofer , Junio C Hamano Subject: [PATCH 2/3] merge: handle --verify-signatures for unborn branch Message-ID: <20181106075115.GB22021@sigill.intra.peff.net> References: <20181106074910.GA31978@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181106074910.GA31978@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- builtin/merge.c | 4 ++++ t/t7612-merge-verify-signatures.sh | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/builtin/merge.c b/builtin/merge.c index c677f9375e..be156b122d 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1336,6 +1336,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, diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh index e2b1df817a..d99218a725 100755 --- a/t/t7612-merge-verify-signatures.sh +++ b/t/t7612-merge-verify-signatures.sh @@ -103,4 +103,11 @@ test_expect_success GPG 'merge commit with bad signature with merge.verifySignat git merge --no-verify-signatures $(cat forged.commit) ' +test_expect_success GPG 'merge unsigned commit into unborn branch' ' + test_when_finished "git checkout initial" && + git checkout --orphan unborn && + test_must_fail git merge --verify-signatures side-unsigned 2>mergeerror && + test_i18ngrep "does not have a GPG signature" mergeerror +' + test_done From patchwork Tue Nov 6 07:52:13 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10669857 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 03CBD14E2 for ; Tue, 6 Nov 2018 07:52:17 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E48C229B2F for ; Tue, 6 Nov 2018 07:52:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D240229E22; Tue, 6 Nov 2018 07:52:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 612D029B2F for ; Tue, 6 Nov 2018 07:52:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729326AbeKFRQL (ORCPT ); Tue, 6 Nov 2018 12:16:11 -0500 Received: from cloud.peff.net ([104.130.231.41]:41842 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1729160AbeKFRQL (ORCPT ); Tue, 6 Nov 2018 12:16:11 -0500 Received: (qmail 17831 invoked by uid 109); 6 Nov 2018 07:52:15 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 06 Nov 2018 07:52:15 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 29124 invoked by uid 111); 6 Nov 2018 07:51:33 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Tue, 06 Nov 2018 02:51:33 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Tue, 06 Nov 2018 02:52:13 -0500 Date: Tue, 6 Nov 2018 02:52:13 -0500 From: Jeff King To: git@vger.kernel.org Cc: Felix Eckhofer , Junio C Hamano Subject: [PATCH 3/3] pull: handle --verify-signatures for unborn branch Message-ID: <20181106075212.GC22021@sigill.intra.peff.net> References: <20181106074910.GA31978@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20181106074910.GA31978@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 --- builtin/pull.c | 11 +++++++++++ t/t5573-pull-verify-signatures.sh | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/builtin/pull.c b/builtin/pull.c index 798ecf7faf..91616cf7d6 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -557,6 +557,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 diff --git a/t/t5573-pull-verify-signatures.sh b/t/t5573-pull-verify-signatures.sh index 747775c147..3e9876e197 100755 --- a/t/t5573-pull-verify-signatures.sh +++ b/t/t5573-pull-verify-signatures.sh @@ -78,4 +78,11 @@ test_expect_success GPG 'pull commit with bad signature with --no-verify-signatu git pull --ff-only --no-verify-signatures bad 2>pullerror ' +test_expect_success GPG 'pull unsigned commit into unborn branch' ' + git init empty-repo && + test_must_fail \ + git -C empty-repo pull --verify-signatures .. 2>pullerror && + test_i18ngrep "does not have a GPG signature" pullerror +' + test_done