{"thread":{"id":"48995","subject":"[PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","startedAt":"2018-07-31T20:12:50Z","lastAt":"2018-08-13T15:14:53Z","messageCount":19,"participants":["Vojtech Myslivec","brian m. carlson","Santiago Torres","Karel Kočí","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"354117","messageId":"09f9803c-3f4b-a97c-2c59-e9d6b924892f@nic.cz","threadId":"48995","inReplyTo":null,"subject":"[PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Vojtech Myslivec","fromEmail":"vojtech.myslivec@nic.cz","sentAt":"2018-07-31T20:05:22Z","receivedAt":"2018-07-31T20:12:50Z","isPatch":true,"sender":{"key":"vojtech.myslivec@nic.cz","avatar":null},"body":"Hello,\n\nme and my colleague are struggling with automation of verifying git\nrepositories and we have encountered that git verify-commit and\nverify-tag accepts untrusted signatures and exit successfully.\n\nWe have done some investigation of the GPG verification changes in git\nrepository which I includes in this patch message. GPG results\n`TRUST_NEVER` and `TRUST_UNDEFINED` in raw output is treated as\nuntrusted in git (U) and should not be accepted in verify-commit and\nverify-tag command.\n\n\nIn 434060ec6d verify-tag and verify-commit was centralized into\ncheck_signature function and good (G) and untrusted (U) signatures were\nmarked as valid and exited successfully. In this commit it is\nincorrectly stated that this behavior is adopted from older verify-tag\nfunction however original verify-tag behavior was to return exit code\nfrom gpg process itself (removed in a4cc18f29).\n\nAlso rejecting untrusted (U) signature is the pull/merge with\n--verify-signatures behavior (defined in builtin/merge.c cmd_merge\nfunction and presented in eb307ae7bb).\n\nThe behavior of merge/pull --verify-signatures and\nverify-commit/verify-tag should be the same.\n\n\nWith regards,\nVojtech Myslivec and Karel Koci\n\n\n\nFrom c9c7b555da284c4f67fe36dc95d592644089544a Mon Sep 17 00:00:00 2001\nFrom: Vojtech Myslivec <vojtech.myslivec@nic.cz>\nDate: Tue, 31 Jul 2018 20:32:32 +0200\nSubject: [PATCH] gpg-interface: Do not accept untrusted signatures\n\nIn 434060ec6d verify-tag and verify-commit was centralized into\ncheck_signature function and good (G) and untrusted (U) signatures were\nmarked as valid and exited successfully. In this commit it is\nincorrectly stated that this behavior is adopted from older verify-tag\nfunction however original verify-tag behavior was to return exit code\nfrom gpg process itself (removed in a4cc18f29).\n\nAlso rejecting untrusted (U) signature is the pull/merge with\n--verify-signatures behavior (defined in builtin/merge.c cmd_merge\nfunction and presented in eb307ae7bb).\n\nThe behavior of merge/pull --verify-signatures and\nverify-commit/verify-tag should be the same.\n---\n gpg-interface.c          | 2 +-\n t/t7030-verify-tag.sh    | 4 ++--\n t/t7510-signed-commit.sh | 6 +++---\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 09ddfbc26..83adc7d12 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -86,7 +86,7 @@ int check_signature(const char *payload, size_t plen, const char *signature,\n \tstrbuf_release(&gpg_status);\n \tstrbuf_release(&gpg_output);\n \n-\treturn sigc->result != 'G' && sigc->result != 'U';\n+\treturn sigc->result != 'G';\n }\n \n void print_signature_buffer(const struct signature_check *sigc, unsigned flags)\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 291a1e2b0..d6f77c443 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -63,7 +63,7 @@ test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor tag in eighth-signed-alt\n \t\tdo\n-\t\t\tgit verify-tag $tag 2>actual &&\n+\t\t\ttest_must_fail git verify-tag $tag 2>actual &&\n \t\t\tgrep \"Good signature from\" actual &&\n \t\t\t! grep \"BAD signature from\" actual &&\n \t\t\tgrep \"not certified\" actual &&\n@@ -103,7 +103,7 @@ test_expect_success GPG 'verify signatures with --raw' '\n \t(\n \t\tfor tag in eighth-signed-alt\n \t\tdo\n-\t\t\tgit verify-tag --raw $tag 2>actual &&\n+\t\t\ttest_must_fail git verify-tag --raw $tag 2>actual &&\n \t\t\tgrep \"GOODSIG\" actual &&\n \t\t\t! grep \"BADSIG\" actual &&\n \t\t\tgrep \"TRUST_UNDEFINED\" actual &&\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 6e2015ed9..5cb388cb6 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -89,8 +89,8 @@ test_expect_success GPG 'verify and show signatures' '\n \t)\n '\n \n-test_expect_success GPG 'verify-commit exits success on untrusted signature' '\n-\tgit verify-commit eighth-signed-alt 2>actual &&\n+test_expect_success GPG 'verify-commit exits unsuccessfully on untrusted signature' '\n+\ttest_must_fail git verify-commit eighth-signed-alt 2>actual &&\n \tgrep \"Good signature from\" actual &&\n \t! grep \"BAD signature from\" actual &&\n \tgrep \"not certified\" actual\n@@ -118,7 +118,7 @@ test_expect_success GPG 'verify signatures with --raw' '\n \t(\n \t\tfor commit in eighth-signed-alt\n \t\tdo\n-\t\t\tgit verify-commit --raw $commit 2>actual &&\n+\t\t\ttest_must_fail git verify-commit --raw $commit 2>actual &&\n \t\t\tgrep \"GOODSIG\" actual &&\n \t\t\t! grep \"BADSIG\" actual &&\n \t\t\tgrep \"TRUST_UNDEFINED\" actual &&\n-- \n2.18.0\n\n"},{"id":"354139","messageId":"20180801001942.GC45452@genre.crustytoothpaste.net","threadId":"48995","inReplyTo":"09f9803c-3f4b-a97c-2c59-e9d6b924892f@nic.cz","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-01T00:19:42Z","receivedAt":"2018-08-01T00:19:53Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 31, 2018 at 10:05:22PM +0200, Vojtech Myslivec wrote:\n> Hello,\n> \n> me and my colleague are struggling with automation of verifying git\n> repositories and we have encountered that git verify-commit and\n> verify-tag accepts untrusted signatures and exit successfully.\n\nI don't have strong feelings on your change one way or the other, but\nfor automation it may be useful to use the --raw flag, which gives you\nthe raw gpg output and much greater control.  For example, you can\nrequire that a subkey is or is not used or require certain algorithms.\n\nI will say that most signatures are untrusted in my experience, so\nunless people are using TOFU mode or making local signatures, git will\nexit nonzero for most signatures.  I think the current status is to exit\non a good signature, even if it isn't necessarily a valid signature.\n\nI'm interested to hear others' thoughts on this.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"354143","messageId":"20180801002546.du52zkxjupzcw2p6@LykOS.localdomain","threadId":"48995","inReplyTo":"20180801001942.GC45452@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2018-08-01T00:25:47Z","receivedAt":"2018-08-01T01:24:11Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"On Wed, Aug 01, 2018 at 12:19:42AM +0000, brian m. carlson wrote:\n> On Tue, Jul 31, 2018 at 10:05:22PM +0200, Vojtech Myslivec wrote:\n> > Hello,\n> > \n> > me and my colleague are struggling with automation of verifying git\n> > repositories and we have encountered that git verify-commit and\n> > verify-tag accepts untrusted signatures and exit successfully.\n> \n> I don't have strong feelings on your change one way or the other, but\n> for automation it may be useful to use the --raw flag, which gives you\n> the raw gpg output and much greater control.  For example, you can\n> require that a subkey is or is not used or require certain algorithms.\n> \n> I will say that most signatures are untrusted in my experience, so\n> unless people are using TOFU mode or making local signatures, git will\n> exit nonzero for most signatures.  I think the current status is to exit\n> on a good signature, even if it isn't necessarily a valid signature.\n> \n> I'm interested to hear others' thoughts on this.\n\nI'd find it odd that we deviate from the gpg behavior, that returns 0\nwhen verifyng an untrusted signatures. Tooling around gpg is generally\ndifficult for this reason, but using the raw output should be enough to\ndiscard signatures with untrusted keys.\n\nAnother alternative is to use a keyring with trusted keys *only* and\ndisable fetching keys from hkp servers. This way signature verification\nshould fail.\n\nThanks,\n-Santiago.\n\n> -- \n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n\n\n"},{"id":"354388","messageId":"20180803133630.32oxubwttealvhxk@work-pc","threadId":"48995","inReplyTo":"20180801002546.du52zkxjupzcw2p6@LykOS.localdomain","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Karel Kočí","fromEmail":"karel.koci@nic.cz","sentAt":"2018-08-03T13:36:30Z","receivedAt":"2018-08-03T13:47:49Z","isPatch":true,"sender":{"key":"karel.koci@nic.cz","avatar":null},"body":"On Tue, Jul 31, 2018 at 08:25:47PM -0400, Santiago Torres wrote:\n> On Wed, Aug 01, 2018 at 12:19:42AM +0000, brian m. carlson wrote:\n> > On Tue, Jul 31, 2018 at 10:05:22PM +0200, Vojtech Myslivec wrote:\n> > > Hello,\n> > > \n> > > me and my colleague are struggling with automation of verifying git\n> > > repositories and we have encountered that git verify-commit and\n> > > verify-tag accepts untrusted signatures and exit successfully.\n> > \n> > I don't have strong feelings on your change one way or the other, but\n> > for automation it may be useful to use the --raw flag, which gives you\n> > the raw gpg output and much greater control.  For example, you can\n> > require that a subkey is or is not used or require certain algorithms.\n> > \n> > I will say that most signatures are untrusted in my experience, so\n> > unless people are using TOFU mode or making local signatures, git will\n> > exit nonzero for most signatures.  I think the current status is to exit\n> > on a good signature, even if it isn't necessarily a valid signature.\n> > \n> > I'm interested to hear others' thoughts on this.\n> \n> I'd find it odd that we deviate from the gpg behavior, that returns 0\n> when verifyng an untrusted signatures. Tooling around gpg is generally\n> difficult for this reason, but using the raw output should be enough to\n> discard signatures with untrusted keys.\n> \n> Another alternative is to use a keyring with trusted keys *only* and\n> disable fetching keys from hkp servers. This way signature verification\n> should fail.\n> \n> Thanks,\n> -Santiago.\n\nThis is not a deviation. GPG correctly recognizes difference between trusted,\nuntrusted and unknown levels. git on the other hand does not. Well it did until\nthe commit a4cc18f29. That one removed GPG exit code propagation.\n\nI think that core of the problem is that git considers both 'TRUST_NEVER' and\n'TRUST_UNDEFINED' as being the same. In git they both should result in error or at\nlest 'TRUST_NEVER' should (the same way as it does with just GPG).\n\nThere is an illustration of difference of untrusted and unknown key verification\nat the end of this mail. You can test it just by using your own GPG key (just use\nour fingerprint instead).\n\nMy and my colleague's expectations are that 'git verify-commit branch' would\nhandle the commit on the tip of `branch` the  same way as 'git merge\n--verify-signature branch'.\n\nIt is also confusing that untrusted key is ok but expired or missing one results\nin error. GPG's primary usage is for building web of trust. And I think that\nuntrusted keys are more problematic in that sense than expired ones.\n\n\nI also disagree with idea that using --raw should be only way how to check if we\ntrust a key. It is handy in scripts for sure if some additional info about\nsignature is required but I think that it should not be primary way to check for\nsignature validity. Exit code should serve that purpose the same way as in case of\nGPG itself.\n\nSmall example would be that to replace current GPG behavior in portable way\n(shells such as dash or ash) it is required to do at least this:\n\nset -e\nRES=\"$(git verify-tag --raw v1.1 2>&1)\"\n! echo \"$RES\" | grep -q \"^\\[GNUPG:\\] TRUST_NEVER\"\n\nAnd that will for sure get more complicated when I use other trust levels and/or\ntrust database and different trust model but direct.\n\nOverall I thing that deviation from returning GPG exit code was not the best and I\ndon't understand why it was done. Decision if signature validity is reported as\nerror should be on GPG not on git specially when there is almost no configuration\npossibility for GPG in git.\n\nNote that our proposed change is conservative and drawn as a unification of\nbehavior of pull/merge and verify-*. To make it really optimal for automation it\nwould be best to return exit code of GPG command.\n\nWith regards\nKarel Kočí\n\n\nIllustration of difference between unknown and untrusted key:\n\n$ gpg --sign file\n$ mkdir -m 700 gpgt\n$ export GNUPGHOME=$PWD/gpgt\n$ echo 'trust-model:0:\"direct' | gpgconfg --change-options gpg\n$ gpg --recv-keys 2B1F70F95F1B48DA2265A7FAA6BC8B8CEB31659B\n$ gpg --list-key\n/home/cynerd/gpgt/pubring.kbx\n-----------------------------\npub   rsa2048 2016-07-07 [SC]\n      2B1F70F95F1B48DA2265A7FAA6BC8B8CEB31659B\nuid           [ unknown] Karel Kočí (nic.cz) <karel.koci@nic.cz>\nuid           [ unknown] Karel Kočí (cynerd.cz) <cynerd@email.cz>\nuid           [ unknown] Karel Kočí (sh.cvut.cz) <k.koci@sh.cvut.cz>\nuid           [ unknown] Karel Kočí (fel.cvut.cz) <kocikare@fel.cvut.cz>\nsub   rsa2048 2016-07-07 [E]\nsub   rsa4096 2016-09-27 [S]\n$ gpg --verify file.gpg\ngpg: Signature made Fri 03 Aug 2018 02:01:49 PM CEST\ngpg:                using RSA key 46D715A0ED0E0C433CBF5963D83BD732AC2BD828\ngpg:                issuer \"karel.koci@nic.cz\"\ngpg: Good signature from \"Karel Kočí (nic.cz) <karel.koci@nic.cz>\" [marginal]\ngpg:                 aka \"Karel Kočí (cynerd.cz) <cynerd@email.cz>\" [marginal]\ngpg:                 aka \"Karel Kočí (sh.cvut.cz) <k.koci@sh.cvut.cz>\" [marginal]\ngpg:                 aka \"Karel Kočí (fel.cvut.cz) <kocikare@fel.cvut.cz>\" [marginal]\ngpg: WARNING: This key is not certified with sufficiently trusted signatures!\ngpg:          It is not certain that the signature belongs to the owner.\n$ echo $?\n0\n$ echo \"2B1F70F95F1B48DA2265A7FAA6BC8B8CEB31659B:4:\" | gpg --import-ownertrust\n$ gpg --list-key\n/home/cynerd/gpgt/pubring.kbx\n-----------------------------\npub   rsa2048 2016-07-07 [SC]\n      2B1F70F95F1B48DA2265A7FAA6BC8B8CEB31659B\nuid           [  never ] Karel Kočí (nic.cz) <karel.koci@nic.cz>\nuid           [  never ] Karel Kočí (cynerd.cz) <cynerd@email.cz>\nuid           [  never ] Karel Kočí (sh.cvut.cz) <k.koci@sh.cvut.cz>\nuid           [  never ] Karel Kočí (fel.cvut.cz) <kocikare@fel.cvut.cz>\nsub   rsa2048 2016-07-07 [E]\nsub   rsa4096 2016-09-27 [S]\n$ gpg --verify file.gpg\ngpg: WARNING: unsafe permissions on homedir '/home/cynerd/gpgt'\ngpg: Signature made Fri 03 Aug 2018 02:01:49 PM CEST\ngpg:                using RSA key 46D715A0ED0E0C433CBF5963D83BD732AC2BD828\ngpg:                issuer \"karel.koci@nic.cz\"\ngpg: Good signature from \"Karel Kočí (nic.cz) <karel.koci@nic.cz>\" [never]\ngpg:                 aka \"Karel Kočí (cynerd.cz) <cynerd@email.cz>\" [never]\ngpg:                 aka \"Karel Kočí (sh.cvut.cz) <k.koci@sh.cvut.cz>\" [never]\ngpg:                 aka \"Karel Kočí (fel.cvut.cz) <kocikare@fel.cvut.cz>\" [never]\ngpg: WARNING: We do NOT trust this key!\ngpg:          The signature is probably a FORGERY.\n$ echo $?\n1\n"},{"id":"354395","messageId":"20180803154343.z3lgkudleood6lhs@LykOS.localdomain","threadId":"48995","inReplyTo":"20180803133630.32oxubwttealvhxk@work-pc","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Santiago Torres","fromEmail":"santiago@nyu.edu","sentAt":"2018-08-03T15:43:44Z","receivedAt":"2018-08-03T15:43:50Z","isPatch":true,"sender":{"key":"santiago@nyu.edu","avatar":"https://avatars.githubusercontent.com/u/3579933?v=4"},"body":"> > disable fetching keys from hkp servers. This way signature verification\n> > should fail.\n> > \n> > Thanks,\n> > -Santiago.\n> \n> This is not a deviation. GPG correctly recognizes difference between trusted,\n> untrusted and unknown levels. git on the other hand does not. Well it did until\n> the commit a4cc18f29. That one removed GPG exit code propagation.\n\nOh wow. Sorry my assumption parted from looking at the code back in the\nday where this happens. I assumed git was quietly propagating the gpg\nerror code and took it from there. \n\nNow that I think about it though, verify tag can verify more than one\ntag. I assume that this would make it difficult to propagate individual\nerrors in trusting. I honestly don't know what's the best way to modify\nthis behavior then.\n\nThanks,\n-Santiago\n"},{"id":"354400","messageId":"20180803160634.GA19944@sigill.intra.peff.net","threadId":"48995","inReplyTo":"20180803154343.z3lgkudleood6lhs@LykOS.localdomain","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-03T16:06:34Z","receivedAt":"2018-08-03T16:06:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2018 at 11:43:44AM -0400, Santiago Torres wrote:\n\n> > This is not a deviation. GPG correctly recognizes difference between trusted,\n> > untrusted and unknown levels. git on the other hand does not. Well it did until\n> > the commit a4cc18f29. That one removed GPG exit code propagation.\n> \n> Oh wow. Sorry my assumption parted from looking at the code back in the\n> day where this happens. I assumed git was quietly propagating the gpg\n> error code and took it from there. \n> \n> Now that I think about it though, verify tag can verify more than one\n> tag. I assume that this would make it difficult to propagate individual\n> errors in trusting. I honestly don't know what's the best way to modify\n> this behavior then.\n\nI think the only sensible thing is to err on the conservative side, and\nreturn non-zero if we saw _any_ invalid signature.\n\nI will note, though, that just checking the exit code of `verify-tag`\nisn't really that thorough. It shows that there was _a_ signature, but\nwe don't know:\n\n  - if it was an identity the user would expect to be signing tags\n\n  - if it even matches the refname we used to find the tag\n\nSo I'd argue that any real verification needs to either have a human in\nthe loop, or implement a custom policy based on reading the full output.\n\nI know we (and you specifically Santiago) talked about this a while ago,\nand we ended up providing ways to get more information out of\nverify-tag, so that a tool could sit on top of that and implement more\nproject-specific policy. I don't know offhand of any reusable tools that\ndo so, though.\n\n-Peff\n"},{"id":"354413","messageId":"xmqqpnyzxso1.fsf@gitster-ct.c.googlers.com","threadId":"48995","inReplyTo":"20180803154343.z3lgkudleood6lhs@LykOS.localdomain","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T17:32:14Z","receivedAt":"2018-08-03T17:32:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Santiago Torres <santiago@nyu.edu> writes:\n\n> Now that I think about it though, verify tag can verify more than one\n> tag. I assume that this would make it difficult to propagate individual\n> errors in trusting. I honestly don't know what's the best way to modify\n> this behavior then.\n\nI am not sure if changing the behaviour is warranted to begin with.\nEspecially when somebody wants to get more than a single bit.  I\nthink our single bit signals \"does signature compute correctly?\"\nwithout taking \"how much trust do we place in that particular key?\"\ninto account.  As Brian mentioned in an earlier response, those who\nwant to take different factors like the level of trust etc. into\naccount can read from \"--raw\", using the exit code only for \"does\nsignature compute correctly?\" and nothing else.  That would allow\nthem to perform any validation underlying GNUPG let them to do.\n\n\n\n"},{"id":"354485","messageId":"20180804084346.fhte5wusbfb5baem@cynerd-laptop","threadId":"48995","inReplyTo":"20180803160634.GA19944@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Karel Kočí","fromEmail":"karel.koci@nic.cz","sentAt":"2018-08-04T08:43:46Z","receivedAt":"2018-08-04T08:44:27Z","isPatch":true,"sender":{"key":"karel.koci@nic.cz","avatar":null},"body":"On Fri, Aug 03, 2018 at 12:06:34PM -0400, Jeff King wrote:\n> On Fri, Aug 03, 2018 at 11:43:44AM -0400, Santiago Torres wrote:\n> \n> > > This is not a deviation. GPG correctly recognizes difference between trusted,\n> > > untrusted and unknown levels. git on the other hand does not. Well it did until\n> > > the commit a4cc18f29. That one removed GPG exit code propagation.\n> > \n> > Oh wow. Sorry my assumption parted from looking at the code back in the\n> > day where this happens. I assumed git was quietly propagating the gpg\n> > error code and took it from there. \n> > \n> > Now that I think about it though, verify tag can verify more than one\n> > tag. I assume that this would make it difficult to propagate individual\n> > errors in trusting. I honestly don't know what's the best way to modify\n> > this behavior then.\n> \n> I think the only sensible thing is to err on the conservative side, and\n> return non-zero if we saw _any_ invalid signature.\n> \n> I will note, though, that just checking the exit code of `verify-tag`\n> isn't really that thorough. It shows that there was _a_ signature, but\n> we don't know:\n> \n>   - if it was an identity the user would expect to be signing tags\n> \n>   - if it even matches the refname we used to find the tag\n> \n> So I'd argue that any real verification needs to either have a human in\n> the loop, or implement a custom policy based on reading the full output.\n> \n> I know we (and you specifically Santiago) talked about this a while ago,\n> and we ended up providing ways to get more information out of\n> verify-tag, so that a tool could sit on top of that and implement more\n> project-specific policy. I don't know offhand of any reusable tools that\n> do so, though.\n\nI think that it would be even legit to exit on first tag verification failure. If\nsomeone wants to really verify all tags then it can be done with simple for loop.\ngit that way does not have to solve problem of error combination.\n\n>   - if it was an identity the user would expect to be signing tags\nThat can be done just by using trust levels.\n\n>   - if it even matches the refname we used to find the tag\nCan you explain this more? You mean that string (such as v1.1) used to lookup tag\nobject is not verified as part of that object?\n\nOK I thing that it was enough of abstract concepts from me. Let me explain you\nwhat am I trying to achieve. I am implementing feeds (in other words git\nrepositories with packages) and package sources verification for OpenWRT. We\n(project Turris by CZ.NIC) are signing all our commits and all our tags. Now we\nare using small script that is verifying our repositories just before we run\nbuild. That is against keyring maintained on our server. I am trying to extend\nthat to whole OpenWRT tree. That introduces problem of having a lot of keys and a\nlot of packages sharing same allowed keys. Fetching all allowed keys for every\npackage from key servers is just slow because of that I have to share those\nbetween packages. In general there are two options. First one is to have cache of\nalready fetched keys in armor format. Second one is to have one keyring and by\nsetting all keys explicitly as never trusted with package given exception.\nUnfortunately first option can't be used because of one other request that is from\nour team. We don't want to be forced to update list of allowed contributors to our\nprojects every time we have new colleague. Solution we come up with is to have\ncentral PGP key that signs our whole team and then verification is done by\nallowing GPG to fetch additional keys with max-cert-depth 1. That brings me to git\nverify-commit/tag that won't exit with zero code when signature is not trusted.\n\nI have a solution for my problem (calling git verify-* twice and grep). That is\nnot the point of this email nor this contribution. The point is that although\nGPG's behavior of exiting with 0 code when trust level is unknown is unexpected\nbut in the end understandable, git's behavior of exiting with 0 code even if key\nis explicitly untrusted is just counterintuitive. I think that few people are\nstill going to get nasty surprise when I consider that this change was introduced\nmid 2014 just after v2.4.0 and Ubuntu 14.04 lts (running even on part of our\ninfrastructure) still contains version 1.9.1 and in that release it was\nacknowledging GPG exit code.\n\nK.K.\n"},{"id":"354955","messageId":"20180808230456.GA21882@sigill.intra.peff.net","threadId":"48995","inReplyTo":"20180804084346.fhte5wusbfb5baem@cynerd-laptop","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-08T23:04:56Z","receivedAt":"2018-08-08T23:05:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 04, 2018 at 10:43:46AM +0200, Karel Kočí wrote:\n\n> > I think the only sensible thing is to err on the conservative side, and\n> > return non-zero if we saw _any_ invalid signature.\n> > \n> > I will note, though, that just checking the exit code of `verify-tag`\n> > isn't really that thorough. It shows that there was _a_ signature, but\n> > we don't know:\n> > \n> >   - if it was an identity the user would expect to be signing tags\n> > \n> >   - if it even matches the refname we used to find the tag\n> > \n> > So I'd argue that any real verification needs to either have a human in\n> > the loop, or implement a custom policy based on reading the full output.\n> > \n> > I know we (and you specifically Santiago) talked about this a while ago,\n> > and we ended up providing ways to get more information out of\n> > verify-tag, so that a tool could sit on top of that and implement more\n> > project-specific policy. I don't know offhand of any reusable tools that\n> > do so, though.\n> \n> I think that it would be even legit to exit on first tag verification failure. If\n> someone wants to really verify all tags then it can be done with simple for loop.\n> git that way does not have to solve problem of error combination.\n\nYeah, I'd be fine with that.\n\n> >   - if it was an identity the user would expect to be signing tags\n> That can be done just by using trust levels.\n\nI may be showing my PGP cluelessness here, but I thought trust levels\nwere about saying \"to what degree do I think this uid matches this key\".\nOr are you proposing feeding gpg a fixed trust-db pre-seeded with keys\nthat are allowed to sign?\n\nI suppose that would work, but:\n\n  - is it much easier than just verifying the uid from gpg output\n    against a trusted list?\n\n  - it mixes authentication and authorization. I.e., you lose the\n    ability to know about a case of \"yes, this signature is valid by\n    this person, but they are not an authorized tagger\".\n\nDefinitely for some use cases that is fine (and easier still is to just\nnot even have disallowed keys in your keyring). But I don't think it's a\ngeneral solution.\n\n> >   - if it even matches the refname we used to find the tag\n> Can you explain this more? You mean that string (such as v1.1) used to lookup tag\n> object is not verified as part of that object?\n\nYes. The signature is just over the tag object contents itself. That\nobject does contain a \"tag\" field, but it may or may not be the tag\nyou're expecting. Git _could_ confirm that when you looked up a tag via\nrefs/tags/v1.1 that the tag object we found has \"tag v1.1\" in it. But\nthat's not always possible (you might feed Git a resolved name, for\nexample).\n\n> OK I thing that it was enough of abstract concepts from me. Let me explain you\n> what am I trying to achieve. I am implementing feeds (in other words git\n> repositories with packages) and package sources verification for OpenWRT. We\n> (project Turris by CZ.NIC) are signing all our commits and all our tags. Now we\n> are using small script that is verifying our repositories just before we run\n> build. That is against keyring maintained on our server. I am trying to extend\n> that to whole OpenWRT tree. That introduces problem of having a lot of keys and a\n> lot of packages sharing same allowed keys. Fetching all allowed keys for every\n> package from key servers is just slow because of that I have to share those\n> between packages. In general there are two options. First one is to have cache of\n> already fetched keys in armor format. Second one is to have one keyring and by\n> setting all keys explicitly as never trusted with package given exception.\n> Unfortunately first option can't be used because of one other request that is from\n> our team. We don't want to be forced to update list of allowed contributors to our\n> projects every time we have new colleague. Solution we come up with is to have\n> central PGP key that signs our whole team and then verification is done by\n> allowing GPG to fetch additional keys with max-cert-depth 1. That brings me to git\n> verify-commit/tag that won't exit with zero code when signature is not trusted.\n\nOK, that makes some sense (and I guess answers my \"how would you use the\ntrustdb\" question above).\n\n> I have a solution for my problem (calling git verify-* twice and grep). That is\n> not the point of this email nor this contribution. The point is that although\n> GPG's behavior of exiting with 0 code when trust level is unknown is unexpected\n> but in the end understandable, git's behavior of exiting with 0 code even if key\n> is explicitly untrusted is just counterintuitive. I think that few people are\n> still going to get nasty surprise when I consider that this change was introduced\n> mid 2014 just after v2.4.0 and Ubuntu 14.04 lts (running even on part of our\n> infrastructure) still contains version 1.9.1 and in that release it was\n> acknowledging GPG exit code.\n\nFWIW, I'm on board with returning non-zero in any case where gpg would.\n\n-Peff\n"},{"id":"354957","messageId":"20180808231226.GA34639@genre.crustytoothpaste.net","threadId":"48995","inReplyTo":"20180808230456.GA21882@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-08T23:12:27Z","receivedAt":"2018-08-08T23:12:36Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Aug 08, 2018 at 07:04:56PM -0400, Jeff King wrote:\n> On Sat, Aug 04, 2018 at 10:43:46AM +0200, Karel Kočí wrote:\n> > I have a solution for my problem (calling git verify-* twice and grep). That is\n> > not the point of this email nor this contribution. The point is that although\n> > GPG's behavior of exiting with 0 code when trust level is unknown is unexpected\n> > but in the end understandable, git's behavior of exiting with 0 code even if key\n> > is explicitly untrusted is just counterintuitive. I think that few people are\n> > still going to get nasty surprise when I consider that this change was introduced\n> > mid 2014 just after v2.4.0 and Ubuntu 14.04 lts (running even on part of our\n> > infrastructure) still contains version 1.9.1 and in that release it was\n> > acknowledging GPG exit code.\n> \n> FWIW, I'm on board with returning non-zero in any case where gpg would.\n\nI think that's probably the best solution overall.  There's a bug report\nin Debian (https://bugs.debian.org/895048) that requests that behavior\ninstead of the status quo, and also it's the behavior that's documented:\n\n       gpg.program\n           Use this custom program instead of \"gpg\" found on $PATH when\n           making or verifying a PGP signature. The program must support\n           the same command-line interface as GPG, namely, to verify a\n           detached signature, \"gpg --verify $file - <$signature\" is\n           run, and the program is expected to signal a good signature\n           by exiting with code 0, and to generate an ASCII-armored\n           detached signature, the standard input of \"gpg -bsau $key\" is\n           fed with the contents to be signed, and the program is\n           expected to send the result to its standard output.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"354964","messageId":"xmqqzhxwcq2o.fsf@gitster-ct.c.googlers.com","threadId":"48995","inReplyTo":"20180808231226.GA34639@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T00:59:43Z","receivedAt":"2018-08-09T00:59:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n>> FWIW, I'm on board with returning non-zero in any case where gpg would.\n>\n> I think that's probably the best solution overall.\n\nFWIW, I am not married to the current behaviour.  I would not be\nsurprised if it mostly came by accident and not designed.\n\n> There's a bug report\n> in Debian (https://bugs.debian.org/895048) that requests that behavior\n> instead of the status quo, and also it's the behavior that's documented:\n\nThe last bit is a bit questionable; I think you are reading too much\ninto the description.\n\nA substitute for gpg.program MUST signal good (or not good)\nsignature the same way as gpg would with its exit code---that is all\nthe description says.  It does not say anything about how that exit\ncode affects the exit status of \"tag --verify\" and friends that\ncalled gpg.program.\n\n>        gpg.program\n>            Use this custom program instead of \"gpg\" found on $PATH when\n>            making or verifying a PGP signature. The program must support\n>            the same command-line interface as GPG, namely, to verify a\n>            detached signature, \"gpg --verify $file - <$signature\" is\n>            run, and the program is expected to signal a good signature\n>            by exiting with code 0, and to generate an ASCII-armored\n>            detached signature, the standard input of \"gpg -bsau $key\" is\n>            fed with the contents to be signed, and the program is\n>            expected to send the result to its standard output.\n"},{"id":"354965","messageId":"20180809014302.GB34639@genre.crustytoothpaste.net","threadId":"48995","inReplyTo":"xmqqzhxwcq2o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-09T01:43:02Z","receivedAt":"2018-08-09T01:43:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Aug 08, 2018 at 05:59:43PM -0700, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> >> FWIW, I'm on board with returning non-zero in any case where gpg would.\n> >\n> > I think that's probably the best solution overall.\n> \n> FWIW, I am not married to the current behaviour.  I would not be\n> surprised if it mostly came by accident and not designed.\n\nSince apparently I was the author of the commit that changed the\nbehavior originally, let me simply say that I was not aware that gpg\nsignalled the correctness of a signature by its exit status when I wrote\nthat patch.  If I had known that, I would have deferred to gpg in my\nchange.  My goal was consistency between verify-tag and verify-commit,\nand in retrospect I probably made the wrong decision.\n\n> > There's a bug report\n> > in Debian (https://bugs.debian.org/895048) that requests that behavior\n> > instead of the status quo, and also it's the behavior that's documented:\n> \n> The last bit is a bit questionable; I think you are reading too much\n> into the description.\n> \n> A substitute for gpg.program MUST signal good (or not good)\n> signature the same way as gpg would with its exit code---that is all\n> the description says.  It does not say anything about how that exit\n> code affects the exit status of \"tag --verify\" and friends that\n> called gpg.program.\n\nI agree that the description doesn't specifically say that.  In fact, it\ndoesn't explicitly say that it must exit nonzero on a bad signature,\nalthough I agree with your interpretation that that would be logical\n(and, AIUI, the behavior of gpg).\n\nHowever, I would assert that we do want Git's verification to exit\nsuccessfully on a good signature and unsuccessfully on a bad signature,\nand deferring to gpg may be the most robust way of ensuring that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"354993","messageId":"20180809143010.GD1439@sigill.intra.peff.net","threadId":"48995","inReplyTo":"20180809014302.GB34639@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T14:30:10Z","receivedAt":"2018-08-09T14:30:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 01:43:02AM +0000, brian m. carlson wrote:\n\n> On Wed, Aug 08, 2018 at 05:59:43PM -0700, Junio C Hamano wrote:\n> > \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> > \n> > >> FWIW, I'm on board with returning non-zero in any case where gpg would.\n> > >\n> > > I think that's probably the best solution overall.\n> > \n> > FWIW, I am not married to the current behaviour.  I would not be\n> > surprised if it mostly came by accident and not designed.\n> \n> Since apparently I was the author of the commit that changed the\n> behavior originally, let me simply say that I was not aware that gpg\n> signalled the correctness of a signature by its exit status when I wrote\n> that patch.  If I had known that, I would have deferred to gpg in my\n> change.  My goal was consistency between verify-tag and verify-commit,\n> and in retrospect I probably made the wrong decision.\n\nOK, so it seems like we're all in agreement now.\n\nWhat next?\n\nThere was a patch at the start of this thread, but it specifically\nchecks for \"sigc->result == U\".  That's probably OK, since I think it\nrestores the behavior in earlier versions of Git. But I wonder if we\nshould simply be storing the fact that gpg exited non-zero and relaying\nthat. That would fix this problem and truly make the rule \"if gpg\nreported an error, we propagate that\".\n\n-Peff\n"},{"id":"354995","messageId":"xmqqtvo3d0by.fsf@gitster-ct.c.googlers.com","threadId":"48995","inReplyTo":"20180809143010.GD1439@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T15:30:25Z","receivedAt":"2018-08-09T15:30:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There was a patch at the start of this thread, but it specifically\n> checks for \"sigc->result == U\".  That's probably OK, since I think it\n> restores the behavior in earlier versions of Git. But I wonder if we\n> should simply be storing the fact that gpg exited non-zero and relaying\n> that. That would fix this problem and truly make the rule \"if gpg\n> reported an error, we propagate that\".\n\nYeah, I like that.  Something like this, perhaps?  Points to note:\n\n * status gets the return value from verify_signed_buffer(), which\n   essentially is what wait_or_whine() gives us for the \"gpg\n   --verify\" process.\n\n * Even if status says \"failed\", we still need to parse the output\n   to set sigc->result.  We used to use sigc->result as the sole\n   source of our return value, but now we turn 'status' into 'bad'\n   (i.e. non-zero) after parsing and finding it is not mechanically\n   good (which is the same criteria as we have always used before).\n   An already bad status is left as bad.\n\n * And we return 'status'.\n\nIf we choose to blindly trust the exit status of \"gpg --verify\" and\nnot interpret the result ourselves, we can lose the \"smudge status\nto be bad if not G/U\" bit, which I offhand do not think makes much\ndifference either way.  I just left it there because showing what\ncan be removed and saying it can be dropped is easier than showing\nthe result of removal and saying it can be added--simply because I\nneed to describe \"it\" if I go the latter route.\n\n gpg-interface.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 09ddfbc267..2e0885c511 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -67,26 +67,27 @@ static void parse_gpg_output(struct signature_check *sigc)\n int check_signature(const char *payload, size_t plen, const char *signature,\n \tsize_t slen, struct signature_check *sigc)\n {\n \tstruct strbuf gpg_output = STRBUF_INIT;\n \tstruct strbuf gpg_status = STRBUF_INIT;\n \tint status;\n \n \tsigc->result = 'N';\n \n \tstatus = verify_signed_buffer(payload, plen, signature, slen,\n \t\t\t\t      &gpg_output, &gpg_status);\n \tif (status && !gpg_output.len)\n \t\tgoto out;\n \tsigc->payload = xmemdupz(payload, plen);\n \tsigc->gpg_output = strbuf_detach(&gpg_output, NULL);\n \tsigc->gpg_status = strbuf_detach(&gpg_status, NULL);\n \tparse_gpg_output(sigc);\n+\tstatus |= sigc->result != 'G' && sigc->result != 'U';\n \n  out:\n \tstrbuf_release(&gpg_status);\n \tstrbuf_release(&gpg_output);\n \n-\treturn sigc->result != 'G' && sigc->result != 'U';\n+\treturn !!status;\n }\n \n void print_signature_buffer(const struct signature_check *sigc, unsigned flags)\n"},{"id":"355011","messageId":"20180809171219.GF1439@sigill.intra.peff.net","threadId":"48995","inReplyTo":"xmqqtvo3d0by.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T17:12:19Z","receivedAt":"2018-08-09T17:12:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 08:30:25AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > There was a patch at the start of this thread, but it specifically\n> > checks for \"sigc->result == U\".  That's probably OK, since I think it\n> > restores the behavior in earlier versions of Git. But I wonder if we\n> > should simply be storing the fact that gpg exited non-zero and relaying\n> > that. That would fix this problem and truly make the rule \"if gpg\n> > reported an error, we propagate that\".\n> \n> Yeah, I like that.  Something like this, perhaps?  Points to note:\n> \n>  * status gets the return value from verify_signed_buffer(), which\n>    essentially is what wait_or_whine() gives us for the \"gpg\n>    --verify\" process.\n> \n>  * Even if status says \"failed\", we still need to parse the output\n>    to set sigc->result.  We used to use sigc->result as the sole\n>    source of our return value, but now we turn 'status' into 'bad'\n>    (i.e. non-zero) after parsing and finding it is not mechanically\n>    good (which is the same criteria as we have always used before).\n>    An already bad status is left as bad.\n> \n>  * And we return 'status'.\n\nYeah, this is exactly what I had in mind. And the size of the code\nchange is much smaller than I feared. The case that I thought might be\ncomplicated is still reading the output after we've seen the non-zero\nstatus, but the existing \"if (status && !gpg_output.len)\" covers that.\n\n> If we choose to blindly trust the exit status of \"gpg --verify\" and\n> not interpret the result ourselves, we can lose the \"smudge status\n> to be bad if not G/U\" bit, which I offhand do not think makes much\n> difference either way.  I just left it there because showing what\n> can be removed and saying it can be dropped is easier than showing\n> the result of removal and saying it can be added--simply because I\n> need to describe \"it\" if I go the latter route.\n\nI guess leaving it serves as a sort of cross-check if gpg would return a\nzero exit code but indicate in the status result that the signature was\nnot good. Sort of a belt-and-suspenders, I guess (which might not be\nthat implausible if we think about somebody wrapping gpg with a sloppy\nbit of shell code that loses the exit code -- it's their fault, but it\nmight be nice for us to err on the conservative side).\n\nProbably it should go back to just \"result != G\" then, though (thus\nbringing the whole conversation full circle :) ).\n\nI could live with or without it, though.\n\n-Peff\n"},{"id":"355022","messageId":"xmqqy3dfbcys.fsf@gitster-ct.c.googlers.com","threadId":"48995","inReplyTo":"20180809171219.GF1439@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T18:40:27Z","receivedAt":"2018-08-09T18:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I guess leaving it serves as a sort of cross-check if gpg would return a\n> zero exit code but indicate in the status result that the signature was\n> not good. Sort of a belt-and-suspenders, I guess (which might not be\n> that implausible if we think about somebody wrapping gpg with a sloppy\n> bit of shell code that loses the exit code -- it's their fault, but it\n> might be nice for us to err on the conservative side).\n\nOK, this time a real log message.\n\n-- >8 --\nSubject: [PATCH] gpg-interface: propagate exit status from gpg back to the callers\n\nWhen gpg-interface API unified support for signature verification\ncodepaths for signed tags and signed commits in mid 2015 at around\nv2.6.0-rc0~114, we accidentally loosened the GPG signature\nverification.\n\nBefore that change, signed commits were verified by looking for\n\"G\"ood signature from GPG, while ignoring the exit status of \"gpg\n--verify\" process, while signed tags were verified by simply passing\nthe exit status of \"gpg --verify\" through.  The unified code we\ncurrently have ignores the exit status of \"gpg --verify\" and returns\nsuccessful verification when the signature matches an unexpired key\nregardless of the trust placed on the key (i.e. in addition to \"G\"ood\nones, we accept \"U\"ntrusted ones).\n\nMake these commands signal failure with their exit status when\nunderlying \"gpg --verify\" (or the custom command specified by\n\"gpg.program\" configuration variable) does so.  This essentially\nchanges their behaviour in a backward incompatible way to reject\nsignatures that have been made with untrusted keys even if they\ncorrectly verify, as that is how \"gpg --verify\" behaves.\n\nNote that the code still overrides a zero exit status obtained from\n\"gpg\" (or gpg.program) if the output does not say the signature is\ngood or computes correctly but made with untrusted keys, to catch\na poorly written wrapper around \"gpg\" the user may give us.\n\nWe could exclude \"U\"ntrusted support from this fallback code, but\nthat would be making two backward incompatible changes in a single\ncommit, so let's avoid that for now.  A follow-up change could do so\nif desired.\n\nHelped-by: Vojtech Myslivec <vojtech.myslivec@nic.cz>\nHelped-by: brian m. carlson <sandals@crustytoothpaste.net>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n gpg-interface.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0647bd6348..b5e64c55e2 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -81,12 +81,13 @@ int check_signature(const char *payload, size_t plen, const char *signature,\n \tsigc->gpg_output = strbuf_detach(&gpg_output, NULL);\n \tsigc->gpg_status = strbuf_detach(&gpg_status, NULL);\n \tparse_gpg_output(sigc);\n+\tstatus |= sigc->result != 'G' && sigc->result != 'U';\n \n  out:\n \tstrbuf_release(&gpg_status);\n \tstrbuf_release(&gpg_output);\n \n-\treturn sigc->result != 'G' && sigc->result != 'U';\n+\treturn !!status;\n }\n \n void print_signature_buffer(const struct signature_check *sigc, unsigned flags)\n-- \n2.18.0-547-g1d89318c48\n\n"},{"id":"355030","messageId":"20180809195026.GD32376@sigill.intra.peff.net","threadId":"48995","inReplyTo":"xmqqy3dfbcys.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-09T19:50:26Z","receivedAt":"2018-08-09T19:50:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 09, 2018 at 11:40:27AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I guess leaving it serves as a sort of cross-check if gpg would return a\n> > zero exit code but indicate in the status result that the signature was\n> > not good. Sort of a belt-and-suspenders, I guess (which might not be\n> > that implausible if we think about somebody wrapping gpg with a sloppy\n> > bit of shell code that loses the exit code -- it's their fault, but it\n> > might be nice for us to err on the conservative side).\n> \n> OK, this time a real log message.\n> \n> -- >8 --\n> Subject: [PATCH] gpg-interface: propagate exit status from gpg back to the callers\n> [...]\n\nThanks, the explanation and the patch look good to me.\n\nI'm on the fence over whether a follow-up patch to take away the \"U\" is\nworth it. In practice the code should never trigger either way, since\ngpg would already have exited non-zero in such a case.\n\n-Peff\n"},{"id":"355074","messageId":"20180810022749.GA70480@genre.crustytoothpaste.net","threadId":"48995","inReplyTo":"xmqqy3dfbcys.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-10T02:27:50Z","receivedAt":"2018-08-10T02:27:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, Aug 09, 2018 at 11:40:27AM -0700, Junio C Hamano wrote:\n> -- >8 --\n> Subject: [PATCH] gpg-interface: propagate exit status from gpg back to the callers\n> \n> When gpg-interface API unified support for signature verification\n> codepaths for signed tags and signed commits in mid 2015 at around\n> v2.6.0-rc0~114, we accidentally loosened the GPG signature\n> verification.\n> \n> Before that change, signed commits were verified by looking for\n> \"G\"ood signature from GPG, while ignoring the exit status of \"gpg\n> --verify\" process, while signed tags were verified by simply passing\n> the exit status of \"gpg --verify\" through.  The unified code we\n> currently have ignores the exit status of \"gpg --verify\" and returns\n> successful verification when the signature matches an unexpired key\n> regardless of the trust placed on the key (i.e. in addition to \"G\"ood\n> ones, we accept \"U\"ntrusted ones).\n> \n> Make these commands signal failure with their exit status when\n> underlying \"gpg --verify\" (or the custom command specified by\n> \"gpg.program\" configuration variable) does so.  This essentially\n> changes their behaviour in a backward incompatible way to reject\n> signatures that have been made with untrusted keys even if they\n> correctly verify, as that is how \"gpg --verify\" behaves.\n> \n> Note that the code still overrides a zero exit status obtained from\n> \"gpg\" (or gpg.program) if the output does not say the signature is\n> good or computes correctly but made with untrusted keys, to catch\n> a poorly written wrapper around \"gpg\" the user may give us.\n> \n> We could exclude \"U\"ntrusted support from this fallback code, but\n> that would be making two backward incompatible changes in a single\n> commit, so let's avoid that for now.  A follow-up change could do so\n> if desired.\n\nThis looks great to me.  Thanks.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"355362","messageId":"88aca5f1-61e6-c089-f69e-952937c85a5f@nic.cz","threadId":"48995","inReplyTo":"xmqqy3dfbcys.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted","fromName":"Vojtech Myslivec","fromEmail":"vojtech.myslivec@nic.cz","sentAt":"2018-08-13T15:14:50Z","receivedAt":"2018-08-13T15:14:53Z","isPatch":true,"sender":{"key":"vojtech.myslivec@nic.cz","avatar":null},"body":"On 9.8.2018 20:40, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> I guess leaving it serves as a sort of cross-check if gpg would return a\n>> zero exit code but indicate in the status result that the signature was\n>> not good. Sort of a belt-and-suspenders, I guess (which might not be\n>> that implausible if we think about somebody wrapping gpg with a sloppy\n>> bit of shell code that loses the exit code -- it's their fault, but it\n>> might be nice for us to err on the conservative side).\n> OK, this time a real log message.\n\nNow it is possible to achieve git verify-tag/verify-commit exits\nunsuccessfully when signatures are not trusted. I would like to\nintroduce some tests for this behavior to prevent this problem in the\nfuture.\n\nThank you all for discussion and for solving the issue.\n\nVojtech and Karel\n"}]}