{"thread":{"id":"52909","subject":"Signed commit regression?","startedAt":"2020-02-28T16:45:18Z","lastAt":"2020-03-04T11:47:30Z","messageCount":6,"participants":["Linus Torvalds","Junio C Hamano","brian m. carlson","Hans Jerry Illikainen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"392671","messageId":"CAHk-=wgg8ctNmHbKjy-yYnz07L3m8=1et_q2FJHKM9mZKXzGAA@mail.gmail.com","threadId":"52909","inReplyTo":null,"subject":"Signed commit regression?","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2020-02-28T16:44:56Z","receivedAt":"2020-02-28T16:45:18Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I suspect this may be due to gpg updates, not git, but I'm about to\nleave for the airport in an hour and don't have time to look into it\nmore closely right now.\n\nBecause of the imminent travel, I did a \"git pull\" on my laptop, and\nit doesn't have all the pgp keys I have on my desktop. It was a signed\ntag, but the pull results in:\n\n commit bfeb4f9977348daaaf7283ff365d81f7ee95940a\n merged tag 'zonefs-5.6-rc4'\n No signature\n Merge: 45d0b75b98bf 0dda2ddb7ded\n Author: Linus Torvalds \n Date: 5 minutes ago\n\n Merge tag 'zonefs-5.6-rc4' of\ngit://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/zonefs\n\n Pull zonefs fixes from Damien Le Moal:\n \"Two fixes in here:\n ....\n\nand notice the \"No signature\". It's entirely wrong. There _is_ a\nsignature, it's just that we don't have the key.\n\nAnd the \"No signature\" thing is particularly unhelpful, because it now\ndoesn't show us what key is missing, like it used to.\n\n\"git verify-tag\" still works correctly:\n\n [torvalds@xps13 linux]$ git verify-tag FETCH_HEAD\n gpg: Signature made Fri 28 Feb 2020 12:03:36 AM PST\n gpg: using EDDSA key 913EFF2D612BE1C00CC97738DDA1CDD2C5DA1876\n gpg: Can't check signature: No public key\n\nand shows the key ID, and properly says \"Can't check signature\" (which\nis very very different from \"No signature\").\n\nThis is a big regression. The \"No signature\" message really is\ncompletely incorrect, and is very very wrong indeed.\n\nI suspect it's due to this commit:\n\n 72b006f4bf (\"gpg-interface: prefer check_signature() for GPG verification\")\n\nbut as mentioned I don't have the ability to really dig deeper right now.\n\n Linus\n"},{"id":"392672","messageId":"CAHk-=whg3uip_N1EjLEzaZNMvS8v+5u2GGueE9Wm2xnY87D+-A@mail.gmail.com","threadId":"52909","inReplyTo":"CAHk-=wgg8ctNmHbKjy-yYnz07L3m8=1et_q2FJHKM9mZKXzGAA@mail.gmail.com","subject":"Re: Signed commit regression?","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2020-02-28T16:47:23Z","receivedAt":"2020-02-28T16:47:45Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Feb 28, 2020 at 8:44 AM Linus Torvalds\n wrote:\n>\n> I suspect it's due to this commit:\n>\n> 72b006f4bf (\"gpg-interface: prefer check_signature() for GPG verification\")\n>\n> but as mentioned I don't have the ability to really dig deeper right now.\n\nNever mind - I did a mindless \"just revert that and test\", and it\nindeed is that commit.\n\nPlease revert it in upstream git. The \"No signature\" message really is\nhorribly wrong. It's both technically entirely wrong, but it's wrong\nfrom a UI standpoint too since you really need to show what the\nmissing key was:\n\nWith the revert, I get the proper output:\n\n commit bfeb4f9977348daaaf7283ff365d81f7ee95940a\n merged tag 'zonefs-5.6-rc4'\n gpg: Signature made Fri 28 Feb 2020 12:03:36 AM PST\n gpg: using EDDSA key 913EFF2D612BE1C00CC97738DDA1CDD2C5DA1876\n gpg: Can't check signature: No public key\n Merge: 45d0b75b98bf 0dda2ddb7ded\n Author: Linus Torvalds \n Date: 10 minutes ago\n\n Merge tag 'zonefs-5.6-rc4' of\ngit://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/zonefs\n\n Pull zonefs fixes from Damien Le Moal:\n \"Two fixes in here:\n ....\n\nwhich is useful.\n\n Linus\n"},{"id":"392674","messageId":"xmqqpndyeim1.fsf@gitster-ct.c.googlers.com","threadId":"52909","inReplyTo":"CAHk-=whg3uip_N1EjLEzaZNMvS8v+5u2GGueE9Wm2xnY87D+-A@mail.gmail.com","subject":"Re: Signed commit regression?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-28T17:17:58Z","receivedAt":"2020-02-28T17:18:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, Feb 28, 2020 at 8:44 AM Linus Torvalds\n> wrote:\n>>\n>> I suspect it's due to this commit:\n>>\n>> 72b006f4bf (\"gpg-interface: prefer check_signature() for GPG verification\")\n>>\n>> but as mentioned I don't have the ability to really dig deeper right now.\n>\n> Never mind - I did a mindless \"just revert that and test\", and it\n> indeed is that commit.\n>\n> Please revert it in upstream git. The \"No signature\" message really is\n> horribly wrong. It's both technically entirely wrong, but it's wrong\n> from a UI standpoint too since you really need to show what the\n> missing key was.\n\nTrue---the messages that told you the missing piece of information\nwith the original code came directly from gnupg, and the problematic\nchange stopped showing that and replaced it with the generic (and\nwrong) \"We tried to verify signature and it failed---it must be that\nthe input did not have signature\" message.\n\nIt is in v2.25 already, so we'd need to revert it out of 'maint'; it\nseems to have a minimum fallout on a topic in flight, but it looks\nmanageable.\n\nThanks.\n"},{"id":"392679","messageId":"xmqqlfomefj2.fsf@gitster-ct.c.googlers.com","threadId":"52909","inReplyTo":"xmqqpndyeim1.fsf@gitster-ct.c.googlers.com","subject":"Re: Signed commit regression?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-28T18:24:33Z","receivedAt":"2020-02-28T18:24:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n>> On Fri, Feb 28, 2020 at 8:44 AM Linus Torvalds\n>> wrote:\n>>>\n>>> I suspect it's due to this commit:\n>>>\n>>> 72b006f4bf (\"gpg-interface: prefer check_signature() for GPG verification\")\n>>>\n>>> but as mentioned I don't have the ability to really dig deeper right now.\n>>\n>> Never mind - I did a mindless \"just revert that and test\", and it\n>> indeed is that commit.\n>>\n>> Please revert it in upstream git. The \"No signature\" message really is\n>> horribly wrong. It's both technically entirely wrong, but it's wrong\n>> from a UI standpoint too since you really need to show what the\n>> missing key was.\n>\n> True---the messages that told you the missing piece of information\n> with the original code came directly from gnupg, and the problematic\n> change stopped showing that and replaced it with the generic (and\n> wrong) \"We tried to verify signature and it failed---it must be that\n> the input did not have signature\" message.\n>\n> It is in v2.25 already, so we'd need to revert it out of 'maint'; it\n> seems to have a minimum fallout on a topic in flight, but it looks\n> manageable.\n\nI've prepared a topic to revert that commit and it is now in the\nmiddle of 'pu'; it will get merged down to 'next', 'master' and\n'maint' in due course as other topics.\n\nBrian's SHA-256 (1/4) topic had a couple of changes that depended on\nthe GPG interface API from 72b006f4 (\"gpg-interface: prefer\ncheck_signature() for GPG verification\", 2019-11-27), so I ejected\nthem out of the topic for now:\n\n - tag: store SHA-256 signatures in a header\n - gpg-interface: improve interface for parsing tags\n\nIn the longer term, however, we do want an updated GPG interface\nlayer that lets us achieve 72b006f4 wanted to, namely\n\n - have a single entry point into GPG interface API, so that the\n changes in the future can be centralized;\n\n - not to depend _too_ heavily on the GnuPG's behaviour. The pieces\n of information that was lost from our output and made Linus upset\n was given to the end-user by us parrotting what gpg said without\n the code really understanding what is being said, and we should\n instead make our code aware of _why_ verify_signed_buffer() or\n check_signature() have failed, and make sure we report that to\n the callers.\n\nI'd expect that there may be another round of attempt to update the\nGPG interface. Let's make sure we won't lose info given to the\nend-users while doing so.\n\nThanks.\n"},{"id":"392695","messageId":"20200228222711.GA9290@camp.crustytoothpaste.net","threadId":"52909","inReplyTo":"xmqqlfomefj2.fsf@gitster-ct.c.googlers.com","subject":"Re: Signed commit regression?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-28T22:27:11Z","receivedAt":"2020-02-28T22:27:19Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-28 at 18:24:33, Junio C Hamano wrote:\n> I've prepared a topic to revert that commit and it is now in the\n> middle of 'pu'; it will get merged down to 'next', 'master' and\n> 'maint' in due course as other topics.\n> \n> Brian's SHA-256 (1/4) topic had a couple of changes that depended on\n> the GPG interface API from 72b006f4 (\"gpg-interface: prefer\n> check_signature() for GPG verification\", 2019-11-27), so I ejected\n> them out of the topic for now:\n> \n> - tag: store SHA-256 signatures in a header\n> - gpg-interface: improve interface for parsing tags\n\nThat's fine. I'll rebase my series on top of that patch. Thanks for\nletting me know.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392849","messageId":"87zhcwxskc.hji@dyntopia.com","threadId":"52909","inReplyTo":"xmqqlfomefj2.fsf@gitster-ct.c.googlers.com","subject":"Re: Signed commit regression?","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2020-03-04T11:33:55Z","receivedAt":"2020-03-04T11:47:30Z","isPatch":false,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"On Fri, Feb 28 2020, Junio C Hamano wrote:\n> I'd expect that there may be another round of attempt to update the\n> GPG interface. Let's make sure we won't lose info given to the\n> end-users while doing so.\n\nThe regression was introduced by this botched chunk in 72b006f4bf:\n\n@@ -521,18 +522,19 @@ static int show_one_mergetag(struct commit *commit,\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\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\nThere are two ridiculous bugs in my original patch:\n\n1. The output from GPG is only added to `verify_message' if a signature\n verifies successfully.\n\n2. On verification failure, the \"No signature\" message is always added\n to `verify_message'. This is because, again, no output from GPG is\n added to `verify_message' on failure, so its length will always equal\n `gpg_message_offset' (see the initial assignment) when\n `check_signature()' returns non-zero, sigh...\n\nNot sure of the proper way of fixing a reverted commit, but I'll send v1\nbased on pu that includes regression tests.\n\nI'm sorry for my fuckup and the headache it caused!\n\n-- \nhji\n"}]}