{"thread":{"id":"52361","subject":"[PATCH 0/1] gpg-interface: prefer check_signature() for GPG verification","startedAt":"2019-11-27T17:48:26Z","lastAt":"2020-03-15T16:40:55Z","messageCount":10,"participants":["Hans Jerry Illikainen","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"387188","messageId":"20191127174821.5830-1-hji@dyntopia.com","threadId":"52361","inReplyTo":null,"subject":"[PATCH 0/1] gpg-interface: prefer check_signature() for GPG verification","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-27T17:48:20Z","receivedAt":"2019-11-27T17:48:26Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This patch refactors the use of verify_signed_buffer() for GPG\nverification to use check_signature() instead.\n\nPreviously, both check_signature() and verify_signed_buffer() were used\nto verify signatures in various parts of Git.  However,\nverify_signed_buffer() does not parse the GPG status message.  Instead,\nit relies entirely on the exit code from GPG coupled with the existence\nof a GOODSIG string in the output buffer.  Unfortunately, the mere\nprescience of GOODSIG does not necessarily imply a valid signature, as\nshown by Michał Górny [1].\n\nverify_signed_buffer() should be reserved for internal use by\ncheck_signature() since check_signature() parses and verifies the status\nmessage.  This is accomplished in this patch.\n\nNote that the patch is prepared for the next branch.  I'm not sure if\nthat's appropriate -- but it seemed sensible since I've already touched\ncode in gpg-interface.c that's been merged in next.\n\n[1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html\n\nHans Jerry Illikainen (1):\n  gpg-interface: prefer check_signature() for GPG verification\n\n builtin/fmt-merge-msg.c | 11 +++--\n gpg-interface.c         | 97 +++++++++++++++++++++--------------------\n gpg-interface.h         |  9 ----\n log-tree.c              | 30 +++++++------\n 4 files changed, 72 insertions(+), 75 deletions(-)\n\n--\n2.24.GIT\n"},{"id":"387189","messageId":"20191127174821.5830-2-hji@dyntopia.com","threadId":"52361","inReplyTo":"20191127174821.5830-1-hji@dyntopia.com","subject":"[PATCH 1/1] gpg-interface: prefer check_signature() for GPG verification","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-27T17:48:21Z","receivedAt":"2019-11-27T17:48:30Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This commit refactors the use of verify_signed_buffer() outside of\ngpg-interface.c to use check_signature() instead.  It also turns\nverify_signed_buffer() into a file-local function since it's now only\ninvoked internally by check_signature().\n\nThere were previously two globally scoped functions used in different\nparts of Git to perform GPG signature verification:\nverify_signed_buffer() and check_signature().  Now only\ncheck_signature() is used.\n\nThe verify_signed_buffer() function doesn't guard against duplicate\nsignatures as described by Michał Górny [1].  Instead it only ensures a\nnon-erroneous exit code from GPG and the presence of at least one\nGOODSIG status field.  This stands in contrast with check_signature()\nthat returns an error if more than one signature is encountered.\n\nThe lower degree of verification makes the use of verify_signed_buffer()\nproblematic if callers don't parse and validate the various parts of the\nGPG status message themselves.  And processing these messages seems like\na task that should be reserved to gpg-interface.c with the function\ncheck_signature().\n\nFurthermore, the use of verify_signed_buffer() makes it difficult to\nintroduce new functionality that relies on the content of the GPG status\nlines.\n\nNow all operations that does signature verification share a single entry\npoint to gpg-interface.c.  This makes it easier to propagate changed or\nadditional functionality in GPG signature verification to all parts of\nGit, without having odd edge-cases that don't perform the same degree of\nverification.\n\n[1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n builtin/fmt-merge-msg.c | 11 +++--\n gpg-interface.c         | 97 +++++++++++++++++++++--------------------\n gpg-interface.h         |  9 ----\n log-tree.c              | 30 +++++++------\n 4 files changed, 72 insertions(+), 75 deletions(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex a4615587fd..f7ed102d8b 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -495,6 +495,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\tenum object_type type;\n \t\tunsigned long size, len;\n \t\tchar *buf = read_object_file(oid, &type, &size);\n+\t\tstruct signature_check sigc = { 0 };\n \t\tstruct strbuf sig = STRBUF_INIT;\n \n \t\tif (!buf || type != OBJ_TAG)\n@@ -503,10 +504,12 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \n \t\tif (size == len)\n \t\t\t; /* merely annotated */\n-\t\telse if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) {\n-\t\t\tif (!sig.len)\n-\t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n-\t\t}\n+\t\telse if (!check_signature(buf, len, buf + len, size - len,\n+\t\t\t\t\t  &sigc)) {\n+\t\t\tstrbuf_addstr(&sig, sigc.gpg_output);\n+\t\t\tsignature_check_clear(&sigc);\n+\t\t} else\n+\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n \n \t\tif (!tag_number++) {\n \t\t\tfmt_tag_signature(&tagbuf, &sig, buf, len);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 131e7d529e..5134ce2780 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -207,6 +207,55 @@ static void parse_gpg_output(struct signature_check *sigc)\n \tFREE_AND_NULL(sigc->key);\n }\n \n+static int verify_signed_buffer(const char *payload, size_t payload_size,\n+\t\t\t\tconst char *signature, size_t signature_size,\n+\t\t\t\tstruct strbuf *gpg_output,\n+\t\t\t\tstruct strbuf *gpg_status)\n+{\n+\tstruct child_process gpg = CHILD_PROCESS_INIT;\n+\tstruct gpg_format *fmt;\n+\tstruct tempfile *temp;\n+\tint ret;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n+\tif (!temp)\n+\t\treturn error_errno(_(\"could not create temporary file\"));\n+\tif (write_in_full(temp->fd, signature, signature_size) < 0 ||\n+\t    close_tempfile_gently(temp) < 0) {\n+\t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n+\t\t\t    temp->filename.buf);\n+\t\tdelete_tempfile(&temp);\n+\t\treturn -1;\n+\t}\n+\n+\tfmt = get_format_by_sig(signature);\n+\tif (!fmt)\n+\t\tBUG(\"bad signature '%s'\", signature);\n+\n+\targv_array_push(&gpg.args, fmt->program);\n+\targv_array_pushv(&gpg.args, fmt->verify_args);\n+\targv_array_pushl(&gpg.args,\n+\t\t\t \"--status-fd=1\",\n+\t\t\t \"--verify\", temp->filename.buf, \"-\",\n+\t\t\t NULL);\n+\n+\tif (!gpg_status)\n+\t\tgpg_status = &buf;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\tret = pipe_command(&gpg, payload, payload_size,\n+\t\t\t   gpg_status, 0, gpg_output, 0);\n+\tsigchain_pop(SIGPIPE);\n+\n+\tdelete_tempfile(&temp);\n+\n+\tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n+\tstrbuf_release(&buf); /* no matter it was used or not */\n+\n+\treturn ret;\n+}\n+\n int check_signature(const char *payload, size_t plen, const char *signature,\n \tsize_t slen, struct signature_check *sigc)\n {\n@@ -351,51 +400,3 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \n \treturn 0;\n }\n-\n-int verify_signed_buffer(const char *payload, size_t payload_size,\n-\t\t\t const char *signature, size_t signature_size,\n-\t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n-{\n-\tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tstruct gpg_format *fmt;\n-\tstruct tempfile *temp;\n-\tint ret;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n-\tif (!temp)\n-\t\treturn error_errno(_(\"could not create temporary file\"));\n-\tif (write_in_full(temp->fd, signature, signature_size) < 0 ||\n-\t    close_tempfile_gently(temp) < 0) {\n-\t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n-\t\t\t    temp->filename.buf);\n-\t\tdelete_tempfile(&temp);\n-\t\treturn -1;\n-\t}\n-\n-\tfmt = get_format_by_sig(signature);\n-\tif (!fmt)\n-\t\tBUG(\"bad signature '%s'\", signature);\n-\n-\targv_array_push(&gpg.args, fmt->program);\n-\targv_array_pushv(&gpg.args, fmt->verify_args);\n-\targv_array_pushl(&gpg.args,\n-\t\t\t \"--status-fd=1\",\n-\t\t\t \"--verify\", temp->filename.buf, \"-\",\n-\t\t\t NULL);\n-\n-\tif (!gpg_status)\n-\t\tgpg_status = &buf;\n-\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\tret = pipe_command(&gpg, payload, payload_size,\n-\t\t\t   gpg_status, 0, gpg_output, 0);\n-\tsigchain_pop(SIGPIPE);\n-\n-\tdelete_tempfile(&temp);\n-\n-\tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n-\tstrbuf_release(&buf); /* no matter it was used or not */\n-\n-\treturn ret;\n-}\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 3e624ec289..93cc3aff5c 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -46,15 +46,6 @@ size_t parse_signature(const char *buf, size_t size);\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n \t\tconst char *signing_key);\n \n-/*\n- * Run \"gpg\" to see if the payload matches the detached signature.\n- * gpg_output, when set, receives the diagnostic output from GPG.\n- * gpg_status, when set, receives the status output from GPG.\n- */\n-int verify_signed_buffer(const char *payload, size_t payload_size,\n-\t\t\t const char *signature, size_t signature_size,\n-\t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status);\n-\n int git_gpg_config(const char *, const char *, void *);\n void set_signing_key(const char *);\n const char *get_signing_key(void);\ndiff --git a/log-tree.c b/log-tree.c\nindex 151e12f415..4e32638de8 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -449,22 +449,22 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n {\n \tstruct strbuf payload = STRBUF_INIT;\n \tstruct strbuf signature = STRBUF_INIT;\n-\tstruct strbuf gpg_output = STRBUF_INIT;\n+\tstruct signature_check sigc = { 0 };\n \tint status;\n \n \tif (parse_signed_commit(commit, &payload, &signature) <= 0)\n \t\tgoto out;\n \n-\tstatus = verify_signed_buffer(payload.buf, payload.len,\n-\t\t\t\t      signature.buf, signature.len,\n-\t\t\t\t      &gpg_output, NULL);\n-\tif (status && !gpg_output.len)\n-\t\tstrbuf_addstr(&gpg_output, \"No signature\\n\");\n-\n-\tshow_sig_lines(opt, status, gpg_output.buf);\n+\tstatus = check_signature(payload.buf, payload.len, signature.buf,\n+\t\t\t\t signature.len, &sigc);\n+\tif (status && sigc.result == 'N')\n+\t\tshow_sig_lines(opt, status, \"No signature\\n\");\n+\telse {\n+\t\tshow_sig_lines(opt, status, sigc.gpg_output);\n+\t\tsignature_check_clear(&sigc);\n+\t}\n \n  out:\n-\tstrbuf_release(&gpg_output);\n \tstrbuf_release(&payload);\n \tstrbuf_release(&signature);\n }\n@@ -497,6 +497,7 @@ static int show_one_mergetag(struct commit *commit,\n \tstruct object_id oid;\n \tstruct tag *tag;\n \tstruct strbuf verify_message;\n+\tstruct signature_check sigc = { 0 };\n \tint status, nth;\n \tsize_t payload_size, gpg_message_offset;\n \n@@ -525,12 +526,13 @@ static int show_one_mergetag(struct commit *commit,\n \tstatus = -1;\n \tif (extra->len > payload_size) {\n \t\t/* could have a good signature */\n-\t\tif (!verify_signed_buffer(extra->value, payload_size,\n-\t\t\t\t\t  extra->value + payload_size,\n-\t\t\t\t\t  extra->len - payload_size,\n-\t\t\t\t\t  &verify_message, NULL))\n+\t\tif (!check_signature(extra->value, payload_size,\n+\t\t\t\t     extra->value + payload_size,\n+\t\t\t\t     extra->len - payload_size, &sigc)) {\n+\t\t\tstrbuf_addstr(&verify_message, sigc.gpg_output);\n+\t\t\tsignature_check_clear(&sigc);\n \t\t\tstatus = 0; /* good */\n-\t\telse if (verify_message.len <= gpg_message_offset)\n+\t\t} else if (verify_message.len <= gpg_message_offset)\n \t\t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n \t\t/* otherwise we couldn't verify, which is shown as bad */\n \t}\n-- \n2.24.GIT\n\n"},{"id":"387352","messageId":"xmqqo8wt10yl.fsf@gitster-ct.c.googlers.com","threadId":"52361","inReplyTo":"20191127174821.5830-2-hji@dyntopia.com","subject":"Re: [PATCH 1/1] gpg-interface: prefer check_signature() for GPG verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T21:57:54Z","receivedAt":"2019-11-30T21:58:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> This commit refactors the use of verify_signed_buffer() outside of\n> gpg-interface.c to use check_signature() instead.  It also turns\n> verify_signed_buffer() into a file-local function since it's now only\n> invoked internally by check_signature().\n>\n> There were previously two globally scoped functions used in different\n> parts of Git to perform GPG signature verification:\n> verify_signed_buffer() and check_signature().  Now only\n> check_signature() is used.\n\nOK.  I wondered why the former does not get removed, but if the\nlatter uses it as its implementation detail, then it is expected\nthat it has to remain.  So check_signature() allows it to make a\nminimum sanity check, but it does its own verification based on the\nstatus output, and both have to pass for a signature to be valid?\nWhich makes sense to me.\n\n> Now all operations that does signature verification share a single entry\n> point to gpg-interface.c.  This makes it easier to propagate changed or\n> additional functionality in GPG signature verification to all parts of\n> Git, without having odd edge-cases that don't perform the same degree of\n> verification.\n\nMakes sense.\n"},{"id":"392850","messageId":"20200304114804.19108-1-hji@dyntopia.com","threadId":"52361","inReplyTo":"20191127174821.5830-1-hji@dyntopia.com","subject":"[PATCH v1 0/2] gpg-interface: prefer check_signature() for GPG verification","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2020-03-04T11:48:02Z","receivedAt":"2020-03-04T11:48:11Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This patch refactors the use of verify_signed_buffer() for GPG\nverification to use check_signature() instead.\n\nPreviously, both check_signature() and verify_signed_buffer() were used\nto verify signatures in various parts of Git.  However,\nverify_signed_buffer() does not parse the GPG status message.  Instead,\nit relies entirely on the exit code from GPG coupled with the existence\nof a GOODSIG string in the output buffer.  Unfortunately, the mere\nprescience of GOODSIG does not necessarily imply a valid signature, as\nshown by Michał Górny [1].\n\nverify_signed_buffer() should be reserved for internal use by\ncheck_signature() since check_signature() parses and verifies the status\nmessage.  This is accomplished in this patch.\n\nChanges since v0:\n* Added regression tests for log-tree and fmt-merge-msg.\n* Fixed a bug in log-tree.c that caused \"No signature\" to be shown\n  erroneously.\n* Fixed a similar bug in fmt-merge-msg.c.\n* Always invoke signature_check_clear() after check_signature().  The\n  check function may touch the signature_check structure on failure.\n\n[1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html\n\nHans Jerry Illikainen (2):\n  t: increase test coverage of signature verification output\n  gpg-interface: prefer check_signature() for GPG verification\n\n builtin/fmt-merge-msg.c  |  11 ++--\n gpg-interface.c          |  97 +++++++++++++++++------------------\n gpg-interface.h          |   9 ----\n log-tree.c               |  34 ++++++-------\n t/t4202-log.sh           | 106 +++++++++++++++++++++++++++++++++++++++\n t/t6200-fmt-merge-msg.sh |  23 +++++++++\n 6 files changed, 202 insertions(+), 78 deletions(-)\n\n-- \n2.25.1.709.g558d21736a\n\n"},{"id":"392851","messageId":"20200304114804.19108-2-hji@dyntopia.com","threadId":"52361","inReplyTo":"20200304114804.19108-1-hji@dyntopia.com","subject":"[PATCH v1 1/2] t: increase test coverage of signature verification output","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2020-03-04T11:48:03Z","receivedAt":"2020-03-04T11:48:16Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"There weren't any tests for unsuccessful signature verification of\nsigned merge tags shown in 'git log'.  There also weren't any tests for\nthe GPG output from 'git fmt-merge-msg'.  This was noticed while\ninvestigating a buggy refactor that slipped through the test suite; see\ncommit 72b006f4bfd30b7c5037c163efaf279ab65bea9c.\n\nThis commit adds signature verification tests to the 'log' and\n'fmt-merge-msg' builtins.\n\nThanks to Linus Torvalds for reporting and finding the (now reverted)\ncommit that introduced the regression.\n\nNote that the \"log --show-signature for merged tag with GPG failure\"\ntest case is really hacky.  It relies on an implementation detail of\nverify_signed_buffer() -- namely, it assumes that the signature is\nwritten to a temporary file whose path is under TMPDIR.\n\nThe rationale for that test case is to check whether the code path that\nyields the \"No signature\" message is reachable on failure.  The\nfunctionality in log-tree.c that may show this message does some\npre-parsing of a possible signature that prevents the GPG interface from\nbeing invoked if a signature is actually missing.  And I haven't been\nable to construct a signature that both 1. satisfies that\npre-processing, and 2. causes GPG to fail without any sort of output on\nstderr along the lines of \"this is a bogus/corrupt/... signature\" (the\n\"No signature\" message should only be shown if GPG produce no output).\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n t/t4202-log.sh           | 106 +++++++++++++++++++++++++++++++++++++++\n t/t6200-fmt-merge-msg.sh |  23 +++++++++\n 2 files changed, 129 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 0f766ba65f..1783c6f7d9 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1607,6 +1607,67 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success GPG 'log --graph --show-signature for merged tag with missing key' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b plain-nokey master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-nokey master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_nokey &&\n+\tgit checkout plain-nokey &&\n+\tgit merge --no-ff -m msg signed_tag_nokey &&\n+\tGNUPGHOME=. git log --graph --show-signature -n1 plain-nokey >actual &&\n+\tgrep \"^|\\\\\\  merged tag\" actual &&\n+\tgrep \"^| | gpg: Signature made\" actual &&\n+\tgrep \"^| | gpg: Can'\"'\"'t check signature: \\(public key not found\\|No public key\\)\" actual\n+'\n+\n+test_expect_success GPG 'log --graph --show-signature for merged tag with bad signature' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b plain-bad master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-bad master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_bad &&\n+\tgit cat-file tag signed_tag_bad >raw &&\n+\tsed -e \"s/signed_tag_msg/forged/\" raw >forged &&\n+\tgit hash-object -w -t tag forged >forged.tag &&\n+\tgit checkout plain-bad &&\n+\tgit merge --no-ff -m msg \"$(cat forged.tag)\" &&\n+\tgit log --graph --show-signature -n1 plain-bad >actual &&\n+\tgrep \"^|\\\\\\  merged tag\" actual &&\n+\tgrep \"^| | gpg: Signature made\" actual &&\n+\tgrep \"^| | gpg: BAD signature from\" actual\n+'\n+\n+test_expect_success GPG 'log --show-signature for merged tag with GPG failure' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b plain-fail master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-fail master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_fail &&\n+\tgit checkout plain-fail &&\n+\tgit merge --no-ff -m msg signed_tag_fail &&\n+\tTMPDIR=\"$(pwd)/bogus\" git log --show-signature -n1 plain-fail >actual &&\n+\tgrep \"^merged tag\" actual &&\n+\tgrep \"^No signature\" actual &&\n+\t! grep \"^gpg: Signature made\" actual\n+'\n+\n+\n test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n \tgit checkout -b plain-shallow master &&\n@@ -1648,6 +1709,51 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '\n \tgrep \"^| | gpgsm: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 missing key' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\ttest_config gpg.format x509 &&\n+\ttest_config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit checkout -b plain-x509-nokey master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-x509-nokey master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_x509_nokey &&\n+\tgit checkout plain-x509-nokey &&\n+\tgit merge --no-ff -m msg signed_tag_x509_nokey &&\n+\tGNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual &&\n+\tgrep \"^|\\\\\\  merged tag\" actual &&\n+\tgrep \"^| | gpgsm: certificate not found\" actual\n+'\n+\n+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 bad signature' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\ttest_config gpg.format x509 &&\n+\ttest_config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit checkout -b plain-x509-bad master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-x509-bad master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_x509_bad &&\n+\tgit cat-file tag signed_tag_x509_bad >raw &&\n+\tsed -e \"s/signed_tag_msg/forged/\" raw >forged &&\n+\tgit hash-object -w -t tag forged >forged.tag &&\n+\tgit checkout plain-x509-bad &&\n+\tgit merge --no-ff -m msg \"$(cat forged.tag)\" &&\n+\tgit log --graph --show-signature -n1 plain-x509-bad >actual &&\n+\tgrep \"^|\\\\\\  merged tag\" actual &&\n+\tgrep \"^| | gpgsm: Signature made\" actual &&\n+\tgrep \"^| | gpgsm: invalid signature\" actual\n+'\n+\n+\n test_expect_success GPG '--no-show-signature overrides --show-signature' '\n \tgit log -1 --show-signature --no-show-signature signed >actual &&\n \t! grep \"^gpg:\" actual\ndiff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\nindex 8a72b4c43a..1922c1c42e 100755\n--- a/t/t6200-fmt-merge-msg.sh\n+++ b/t/t6200-fmt-merge-msg.sh\n@@ -6,6 +6,7 @@\n test_description='fmt-merge-msg test'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-gpg.sh\"\n \n test_expect_success setup '\n \techo one >one &&\n@@ -73,6 +74,10 @@ test_expect_success setup '\n \tapos=\"'\\''\"\n '\n \n+test_expect_success GPG '\n+\tgit tag -s -m signed-tag-msg signed-good-tag left\n+'\n+\n test_expect_success 'message for merging local branch' '\n \techo \"Merge branch ${apos}left${apos}\" >expected &&\n \n@@ -83,6 +88,24 @@ test_expect_success 'message for merging local branch' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success GPG 'message for merging local tag signed by good key' '\n+\tgit checkout master &&\n+\tgit fetch . signed-good-tag &&\n+\tgit fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n+\tgrep \"^Merge tag ${apos}signed-good-tag${apos}\" actual &&\n+\tgrep \"^# gpg: Signature made\" actual &&\n+\tgrep \"^# gpg: Good signature from\" actual\n+'\n+\n+test_expect_success GPG 'message for merging local tag signed by unknown key' '\n+\tgit checkout master &&\n+\tgit fetch . signed-good-tag &&\n+\tGNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n+\tgrep \"^Merge tag ${apos}signed-good-tag${apos}\" actual &&\n+\tgrep \"^# gpg: Signature made\" actual &&\n+\tgrep \"^# gpg: Can${apos}t check signature: \\(public key not found\\|No public key\\)\" actual\n+'\n+\n test_expect_success 'message for merging external branch' '\n \techo \"Merge branch ${apos}left${apos} of $(pwd)\" >expected &&\n \n-- \n2.25.1.709.g558d21736a\n\n"},{"id":"392852","messageId":"20200304114804.19108-3-hji@dyntopia.com","threadId":"52361","inReplyTo":"20200304114804.19108-1-hji@dyntopia.com","subject":"[PATCH v1 2/2] gpg-interface: prefer check_signature() for GPG verification","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2020-03-04T11:48:04Z","receivedAt":"2020-03-04T11:48:18Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This commit refactors the use of verify_signed_buffer() outside of\ngpg-interface.c to use check_signature() instead.  It also turns\nverify_signed_buffer() into a file-local function since it's now only\ninvoked internally by check_signature().\n\nThere were previously two globally scoped functions used in different\nparts of Git to perform GPG signature verification:\nverify_signed_buffer() and check_signature().  Now only\ncheck_signature() is used.\n\nThe verify_signed_buffer() function doesn't guard against duplicate\nsignatures as described by Michał Górny [1].  Instead it only ensures a\nnon-erroneous exit code from GPG and the presence of at least one\nGOODSIG status field.  This stands in contrast with check_signature()\nthat returns an error if more than one signature is encountered.\n\nThe lower degree of verification makes the use of verify_signed_buffer()\nproblematic if callers don't parse and validate the various parts of the\nGPG status message themselves.  And processing these messages seems like\na task that should be reserved to gpg-interface.c with the function\ncheck_signature().\n\nFurthermore, the use of verify_signed_buffer() makes it difficult to\nintroduce new functionality that relies on the content of the GPG status\nlines.\n\nNow all operations that does signature verification share a single entry\npoint to gpg-interface.c.  This makes it easier to propagate changed or\nadditional functionality in GPG signature verification to all parts of\nGit, without having odd edge-cases that don't perform the same degree of\nverification.\n\n[1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n builtin/fmt-merge-msg.c | 11 +++--\n gpg-interface.c         | 97 +++++++++++++++++++++--------------------\n gpg-interface.h         |  9 ----\n log-tree.c              | 34 +++++++--------\n 4 files changed, 73 insertions(+), 78 deletions(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 736f666f64..172dfbd852 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -494,6 +494,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\tenum object_type type;\n \t\tunsigned long size, len;\n \t\tchar *buf = read_object_file(oid, &type, &size);\n+\t\tstruct signature_check sigc = { 0 };\n \t\tstruct strbuf sig = STRBUF_INIT;\n \n \t\tif (!buf || type != OBJ_TAG)\n@@ -502,10 +503,12 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \n \t\tif (size == len)\n \t\t\t; /* merely annotated */\n-\t\telse if (verify_signed_buffer(buf, len, buf + len, size - len, &sig, NULL)) {\n-\t\t\tif (!sig.len)\n-\t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n-\t\t}\n+\t\telse if (check_signature(buf, len, buf + len, size - len, &sigc) &&\n+\t\t\t!sigc.gpg_output)\n+\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n+\t\telse\n+\t\t\tstrbuf_addstr(&sig, sigc.gpg_output);\n+\t\tsignature_check_clear(&sigc);\n \n \t\tif (!tag_number++) {\n \t\t\tfmt_tag_signature(&tagbuf, &sig, buf, len);\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 165274d74a..2d538bcd6e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -256,6 +256,55 @@ static void parse_gpg_output(struct signature_check *sigc)\n \tFREE_AND_NULL(sigc->key);\n }\n \n+static int verify_signed_buffer(const char *payload, size_t payload_size,\n+\t\t\t\tconst char *signature, size_t signature_size,\n+\t\t\t\tstruct strbuf *gpg_output,\n+\t\t\t\tstruct strbuf *gpg_status)\n+{\n+\tstruct child_process gpg = CHILD_PROCESS_INIT;\n+\tstruct gpg_format *fmt;\n+\tstruct tempfile *temp;\n+\tint ret;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n+\tif (!temp)\n+\t\treturn error_errno(_(\"could not create temporary file\"));\n+\tif (write_in_full(temp->fd, signature, signature_size) < 0 ||\n+\t    close_tempfile_gently(temp) < 0) {\n+\t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n+\t\t\t    temp->filename.buf);\n+\t\tdelete_tempfile(&temp);\n+\t\treturn -1;\n+\t}\n+\n+\tfmt = get_format_by_sig(signature);\n+\tif (!fmt)\n+\t\tBUG(\"bad signature '%s'\", signature);\n+\n+\targv_array_push(&gpg.args, fmt->program);\n+\targv_array_pushv(&gpg.args, fmt->verify_args);\n+\targv_array_pushl(&gpg.args,\n+\t\t\t \"--status-fd=1\",\n+\t\t\t \"--verify\", temp->filename.buf, \"-\",\n+\t\t\t NULL);\n+\n+\tif (!gpg_status)\n+\t\tgpg_status = &buf;\n+\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\tret = pipe_command(&gpg, payload, payload_size,\n+\t\t\t   gpg_status, 0, gpg_output, 0);\n+\tsigchain_pop(SIGPIPE);\n+\n+\tdelete_tempfile(&temp);\n+\n+\tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n+\tstrbuf_release(&buf); /* no matter it was used or not */\n+\n+\treturn ret;\n+}\n+\n int check_signature(const char *payload, size_t plen, const char *signature,\n \tsize_t slen, struct signature_check *sigc)\n {\n@@ -418,51 +467,3 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \n \treturn 0;\n }\n-\n-int verify_signed_buffer(const char *payload, size_t payload_size,\n-\t\t\t const char *signature, size_t signature_size,\n-\t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n-{\n-\tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tstruct gpg_format *fmt;\n-\tstruct tempfile *temp;\n-\tint ret;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n-\tif (!temp)\n-\t\treturn error_errno(_(\"could not create temporary file\"));\n-\tif (write_in_full(temp->fd, signature, signature_size) < 0 ||\n-\t    close_tempfile_gently(temp) < 0) {\n-\t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n-\t\t\t    temp->filename.buf);\n-\t\tdelete_tempfile(&temp);\n-\t\treturn -1;\n-\t}\n-\n-\tfmt = get_format_by_sig(signature);\n-\tif (!fmt)\n-\t\tBUG(\"bad signature '%s'\", signature);\n-\n-\targv_array_push(&gpg.args, fmt->program);\n-\targv_array_pushv(&gpg.args, fmt->verify_args);\n-\targv_array_pushl(&gpg.args,\n-\t\t\t \"--status-fd=1\",\n-\t\t\t \"--verify\", temp->filename.buf, \"-\",\n-\t\t\t NULL);\n-\n-\tif (!gpg_status)\n-\t\tgpg_status = &buf;\n-\n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\tret = pipe_command(&gpg, payload, payload_size,\n-\t\t\t   gpg_status, 0, gpg_output, 0);\n-\tsigchain_pop(SIGPIPE);\n-\n-\tdelete_tempfile(&temp);\n-\n-\tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n-\tstrbuf_release(&buf); /* no matter it was used or not */\n-\n-\treturn ret;\n-}\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex 796571e9e9..f4e9b4f371 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -54,15 +54,6 @@ size_t parse_signature(const char *buf, size_t size);\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n \t\tconst char *signing_key);\n \n-/*\n- * Run \"gpg\" to see if the payload matches the detached signature.\n- * gpg_output, when set, receives the diagnostic output from GPG.\n- * gpg_status, when set, receives the status output from GPG.\n- */\n-int verify_signed_buffer(const char *payload, size_t payload_size,\n-\t\t\t const char *signature, size_t signature_size,\n-\t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status);\n-\n int git_gpg_config(const char *, const char *, void *);\n void set_signing_key(const char *);\n const char *get_signing_key(void);\ndiff --git a/log-tree.c b/log-tree.c\nindex 52127427ff..897a90233e 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -449,22 +449,21 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n {\n \tstruct strbuf payload = STRBUF_INIT;\n \tstruct strbuf signature = STRBUF_INIT;\n-\tstruct strbuf gpg_output = STRBUF_INIT;\n+\tstruct signature_check sigc = { 0 };\n \tint status;\n \n \tif (parse_signed_commit(commit, &payload, &signature) <= 0)\n \t\tgoto out;\n \n-\tstatus = verify_signed_buffer(payload.buf, payload.len,\n-\t\t\t\t      signature.buf, signature.len,\n-\t\t\t\t      &gpg_output, NULL);\n-\tif (status && !gpg_output.len)\n-\t\tstrbuf_addstr(&gpg_output, \"No signature\\n\");\n-\n-\tshow_sig_lines(opt, status, gpg_output.buf);\n+\tstatus = check_signature(payload.buf, payload.len, signature.buf,\n+\t\t\t\t signature.len, &sigc);\n+\tif (status && !sigc.gpg_output)\n+\t\tshow_sig_lines(opt, status, \"No signature\\n\");\n+\telse\n+\t\tshow_sig_lines(opt, status, sigc.gpg_output);\n+\tsignature_check_clear(&sigc);\n \n  out:\n-\tstrbuf_release(&gpg_output);\n \tstrbuf_release(&payload);\n \tstrbuf_release(&signature);\n }\n@@ -497,8 +496,9 @@ static int show_one_mergetag(struct commit *commit,\n \tstruct object_id oid;\n \tstruct tag *tag;\n \tstruct strbuf verify_message;\n+\tstruct signature_check sigc = { 0 };\n \tint status, nth;\n-\tsize_t payload_size, gpg_message_offset;\n+\tsize_t payload_size;\n \n \thash_object_file(the_hash_algo, extra->value, extra->len,\n \t\t\t type_name(OBJ_TAG), &oid);\n@@ -520,19 +520,19 @@ static int show_one_mergetag(struct commit *commit,\n \telse\n \t\tstrbuf_addf(&verify_message,\n \t\t\t    \"parent #%d, tagged '%s'\\n\", nth + 1, tag->tag);\n-\tgpg_message_offset = verify_message.len;\n \n \tpayload_size = parse_signature(extra->value, extra->len);\n \tstatus = -1;\n \tif (extra->len > payload_size) {\n \t\t/* could have a good signature */\n-\t\tif (!verify_signed_buffer(extra->value, payload_size,\n-\t\t\t\t\t  extra->value + payload_size,\n-\t\t\t\t\t  extra->len - payload_size,\n-\t\t\t\t\t  &verify_message, NULL))\n-\t\t\tstatus = 0; /* good */\n-\t\telse if (verify_message.len <= gpg_message_offset)\n+\t\tstatus = check_signature(extra->value, payload_size,\n+\t\t\t\t\t extra->value + payload_size,\n+\t\t\t\t\t extra->len - payload_size, &sigc);\n+\t\tif (sigc.gpg_output)\n+\t\t\tstrbuf_addstr(&verify_message, sigc.gpg_output);\n+\t\telse\n \t\t\tstrbuf_addstr(&verify_message, \"No signature\\n\");\n+\t\tsignature_check_clear(&sigc);\n \t\t/* otherwise we couldn't verify, which is shown as bad */\n \t}\n \n-- \n2.25.1.709.g558d21736a\n\n"},{"id":"392886","messageId":"xmqqftem4p6y.fsf@gitster-ct.c.googlers.com","threadId":"52361","inReplyTo":"20200304114804.19108-1-hji@dyntopia.com","subject":"Re: [PATCH v1 0/2] gpg-interface: prefer check_signature() for GPG verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-05T18:44:05Z","receivedAt":"2020-03-05T18:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> This patch refactors the use of verify_signed_buffer() for GPG\n> verification to use check_signature() instead.\n>\n> Previously, both check_signature() and verify_signed_buffer() were used\n> to verify signatures in various parts of Git.  However,\n> verify_signed_buffer() does not parse the GPG status message.  Instead,\n> it relies entirely on the exit code from GPG coupled with the existence\n> of a GOODSIG string in the output buffer.  Unfortunately, the mere\n> prescience of GOODSIG does not necessarily imply a valid signature, as\n> shown by Michał Górny [1].\n>\n> verify_signed_buffer() should be reserved for internal use by\n> check_signature() since check_signature() parses and verifies the status\n> message.  This is accomplished in this patch.\n>\n> Changes since v0:\n> * Added regression tests for log-tree and fmt-merge-msg.\n> * Fixed a bug in log-tree.c that caused \"No signature\" to be shown\n>   erroneously.\n> * Fixed a similar bug in fmt-merge-msg.c.\n> * Always invoke signature_check_clear() after check_signature().  The\n>   check function may touch the signature_check structure on failure.\n\nThanks.  Will queue.  Let's cook it slower and aim for the next\ncycle.\n\n> [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html\n>\n> Hans Jerry Illikainen (2):\n>   t: increase test coverage of signature verification output\n>   gpg-interface: prefer check_signature() for GPG verification\n>\n>  builtin/fmt-merge-msg.c  |  11 ++--\n>  gpg-interface.c          |  97 +++++++++++++++++------------------\n>  gpg-interface.h          |   9 ----\n>  log-tree.c               |  34 ++++++-------\n>  t/t4202-log.sh           | 106 +++++++++++++++++++++++++++++++++++++++\n>  t/t6200-fmt-merge-msg.sh |  23 +++++++++\n>  6 files changed, 202 insertions(+), 78 deletions(-)\n"},{"id":"392897","messageId":"nycvar.QRO.7.76.6.2003061257470.46@tvgsbejvaqbjf.bet","threadId":"52361","inReplyTo":"xmqqftem4p6y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1 0/2] gpg-interface: prefer check_signature() for GPG verification","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-06T11:59:38Z","receivedAt":"2020-03-06T11:59:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 5 Mar 2020, Junio C Hamano wrote:\n\n> Hans Jerry Illikainen <hji@dyntopia.com> writes:\n>\n> > This patch refactors the use of verify_signed_buffer() for GPG\n> > verification to use check_signature() instead.\n> >\n> > Previously, both check_signature() and verify_signed_buffer() were used\n> > to verify signatures in various parts of Git.  However,\n> > verify_signed_buffer() does not parse the GPG status message.  Instead,\n> > it relies entirely on the exit code from GPG coupled with the existence\n> > of a GOODSIG string in the output buffer.  Unfortunately, the mere\n> > prescience of GOODSIG does not necessarily imply a valid signature, as\n> > shown by Michał Górny [1].\n> >\n> > verify_signed_buffer() should be reserved for internal use by\n> > check_signature() since check_signature() parses and verifies the status\n> > message.  This is accomplished in this patch.\n> >\n> > Changes since v0:\n> > * Added regression tests for log-tree and fmt-merge-msg.\n> > * Fixed a bug in log-tree.c that caused \"No signature\" to be shown\n> >   erroneously.\n> > * Fixed a similar bug in fmt-merge-msg.c.\n> > * Always invoke signature_check_clear() after check_signature().  The\n> >   check function may touch the signature_check structure on failure.\n>\n> Thanks.  Will queue.  Let's cook it slower and aim for the next\n> cycle.\n\nGood call about cooking this slower: it fails both on Windows and on macOS\n(see\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=32672&view=ms.vss-test-web.build-test-results-tab&runId=101636&resultId=101463&paneView=debug\nfor details):\n\n\texpecting success of 6200.2 'GPG':\n\t\tgit tag -s -m signed-tag-msg signed-good-tag left\n\n\t++ git tag -s -m signed-tag-msg signed-good-tag left\n\terror: gpg failed to sign the data\n\terror: unable to sign the tag\n\terror: last command exited with $?=128\n\n\nNot very helpful log, I must say.\n\nCiao,\nDscho\n\n>\n> > [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html\n> >\n> > Hans Jerry Illikainen (2):\n> >   t: increase test coverage of signature verification output\n> >   gpg-interface: prefer check_signature() for GPG verification\n> >\n> >  builtin/fmt-merge-msg.c  |  11 ++--\n> >  gpg-interface.c          |  97 +++++++++++++++++------------------\n> >  gpg-interface.h          |   9 ----\n> >  log-tree.c               |  34 ++++++-------\n> >  t/t4202-log.sh           | 106 +++++++++++++++++++++++++++++++++++++++\n> >  t/t6200-fmt-merge-msg.sh |  23 +++++++++\n> >  6 files changed, 202 insertions(+), 78 deletions(-)\n>\n>\n"},{"id":"393249","messageId":"nycvar.QRO.7.76.6.2003150020010.46@tvgsbejvaqbjf.bet","threadId":"52361","inReplyTo":"20200304114804.19108-2-hji@dyntopia.com","subject":"Re: [PATCH v1 1/2] t: increase test coverage of signature verification output","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-14T23:25:40Z","receivedAt":"2020-03-15T02:06:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hans,\n\nI was wondering why your patches made the CI/PR builds fail on macOS and\nWindows. This was a pretty hard thing to figure out, see below:\n\nOn Wed, 4 Mar 2020, Hans Jerry Illikainen wrote:\n\n> diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\n> index 8a72b4c43a..1922c1c42e 100755\n> --- a/t/t6200-fmt-merge-msg.sh\n> +++ b/t/t6200-fmt-merge-msg.sh\n> @@ -6,6 +6,7 @@\n>  test_description='fmt-merge-msg test'\n>\n>  . ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-gpg.sh\"\n>\n>  test_expect_success setup '\n>  \techo one >one &&\n> @@ -73,6 +74,10 @@ test_expect_success setup '\n>  \tapos=\"'\\''\"\n>  '\n>\n> +test_expect_success GPG '\n\nFor developers who are very familiar with Git's test suite, it is really\nhard to spot what is wrong with this line, and I was fooled for quite a\nfew days, too.\n\nThe thing is that this `GPG` looks like an innocent prereq and it is\ncorrect: this test case really depends on GPG being present and working.\n\nBut it is not a prereq.\n\nThis is used as the _title_ of the test case. And on the macOS/Windows\nagents, the GPG prereq is not met.\n\nThe reason is that this `test_expect_success` call only receives two\narguments, so it does not interpret the first one as a prereq. But I think\nthat this `GPG` was actually intended as a prereq, so the test case's\ntitle is missing.\n\nCould you kindly change this patch so that it adds a title, e.g. `set up\nsigned tag`?\n\nThat should let the CI build pass again.\n\nThank you,\nDscho\n\n> +\tgit tag -s -m signed-tag-msg signed-good-tag left\n> +'\n> +\n>  test_expect_success 'message for merging local branch' '\n>  \techo \"Merge branch ${apos}left${apos}\" >expected &&\n>\n> @@ -83,6 +88,24 @@ test_expect_success 'message for merging local branch' '\n>  \ttest_cmp expected actual\n>  '\n>\n> +test_expect_success GPG 'message for merging local tag signed by good key' '\n> +\tgit checkout master &&\n> +\tgit fetch . signed-good-tag &&\n> +\tgit fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n> +\tgrep \"^Merge tag ${apos}signed-good-tag${apos}\" actual &&\n> +\tgrep \"^# gpg: Signature made\" actual &&\n> +\tgrep \"^# gpg: Good signature from\" actual\n> +'\n> +\n> +test_expect_success GPG 'message for merging local tag signed by unknown key' '\n> +\tgit checkout master &&\n> +\tgit fetch . signed-good-tag &&\n> +\tGNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n> +\tgrep \"^Merge tag ${apos}signed-good-tag${apos}\" actual &&\n> +\tgrep \"^# gpg: Signature made\" actual &&\n> +\tgrep \"^# gpg: Can${apos}t check signature: \\(public key not found\\|No public key\\)\" actual\n> +'\n> +\n>  test_expect_success 'message for merging external branch' '\n>  \techo \"Merge branch ${apos}left${apos} of $(pwd)\" >expected &&\n>\n> --\n> 2.25.1.709.g558d21736a\n>\n>\n>\n"},{"id":"393273","messageId":"xmqqzhcha7w0.fsf@gitster.c.googlers.com","threadId":"52361","inReplyTo":"nycvar.QRO.7.76.6.2003150020010.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 1/2] t: increase test coverage of signature verification output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-15T16:40:47Z","receivedAt":"2020-03-15T16:40:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Hans,\n>\n> I was wondering why your patches made the CI/PR builds fail on macOS and\n> Windows. This was a pretty hard thing to figure out, see below:\n>\n> On Wed, 4 Mar 2020, Hans Jerry Illikainen wrote:\n>\n>> diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\n>> index 8a72b4c43a..1922c1c42e 100755\n>> --- a/t/t6200-fmt-merge-msg.sh\n>> +++ b/t/t6200-fmt-merge-msg.sh\n>> @@ -6,6 +6,7 @@\n>>  test_description='fmt-merge-msg test'\n>>\n>>  . ./test-lib.sh\n>> +. \"$TEST_DIRECTORY/lib-gpg.sh\"\n>>\n>>  test_expect_success setup '\n>>  \techo one >one &&\n>> @@ -73,6 +74,10 @@ test_expect_success setup '\n>>  \tapos=\"'\\''\"\n>>  '\n>>\n>> +test_expect_success GPG '\n\nAhh, thanks for a pair of sharp eyes.\n\n"}]}