{"thread":{"id":"41976","subject":"[PATCH] Documentation: clarify signature verification","startedAt":"2016-04-09T20:08:39Z","lastAt":"2016-05-13T09:51:01Z","messageCount":7,"participants":["The Fox in the Shell","Junio C Hamano","KellerFuchs","Fox in the shell"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"283046","messageId":"20160409200756.GA22694@hashbang.sh","threadId":"41976","inReplyTo":null,"subject":"[PATCH] Documentation: clarify signature verification","fromName":"The Fox in the Shell","fromEmail":"kellerfuchs@hashbang.sh","sentAt":"2016-04-09T20:08:39Z","receivedAt":"2016-04-09T20:08:39Z","isPatch":true,"sender":{"key":"kellerfuchs@hashbang.sh","avatar":"https://avatars.githubusercontent.com/u/14331571?v=4"},"body":"Hi,\n\nI encountered some issues with the git documentation while modifying\nmy deployment scripts to enforce that the tree being fetched was\nsigned by a trusted key.\n\nIt was unclear which commits needed to be signed (in the case of `git\nmerge`) and what were the criteria for the signature to be considered\nvalid.\n\nHere is a patch proposal.\n\nSigned-off-by: The Fox in the Shell \n---\n Documentation/merge-options.txt | 4 +++-\n Documentation/pretty-formats.txt | 4 ++--\n Documentation/pretty-options.txt | 4 ++--\n 3 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex f08e9b8..edd50bf 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -89,8 +89,10 @@ option can be used to override --squash.\n \n --verify-signatures::\n --no-verify-signatures::\n-\tVerify that the commits being merged have good and trusted GPG signatures\n+\tVerify that the commits being merged have good and valid GPG signatures\n \tand abort the merge in case they do not.\n+\tFor instance, when running `git merge --verify-signature remote/branch`,\n+\tonly the head commit on `remote/branch` needs to be signed.\n \n --summary::\n --no-summary::\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 671cebd..29b19b9 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -143,8 +143,8 @@ ifndef::git-rev-list[]\n - '%N': commit notes\n endif::git-rev-list[]\n - '%GG': raw verification message from GPG for a signed commit\n-- '%G?': show \"G\" for a Good signature, \"B\" for a Bad signature, \"U\" for a good,\n- untrusted signature and \"N\" for no signature\n+- '%G?': show \"G\" for a good (valid) signature, \"B\" for a bad signature,\n+ \"U\" for a good signature with unknown validity and \"N\" for no signature\n - '%GS': show the name of the signer for a signed commit\n - '%GK': show the key used to sign a signed commit\n - '%gD': reflog selector, e.g., `refs/stash@{1}`\ndiff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\nindex 54b88b6..62cbae2 100644\n--- a/Documentation/pretty-options.txt\n+++ b/Documentation/pretty-options.txt\n@@ -78,5 +78,5 @@ being displayed. Examples: \"--notes=foo\" will show only notes from\n endif::git-rev-list[]\n \n --show-signature::\n-\tCheck the validity of a signed commit object by passing the signature\n-\tto `gpg --verify` and show the output.\n+\tCheck the validity of a signed commit object, by passing the signature\n+\tto `gpg --verify`, and show the output.\n-- \n2.1.4\n"},{"id":"283108","messageId":"xmqqa8l1ti8d.fsf@gitster.mtv.corp.google.com","threadId":"41976","inReplyTo":"20160409200756.GA22694@hashbang.sh","subject":"Re: [PATCH] Documentation: clarify signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-10T18:46:10Z","receivedAt":"2016-04-10T18:46:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The Fox in the Shell writes:\n\n> Hi,\n>\n> I encountered some issues with the git documentation while modifying\n> my deployment scripts to enforce that the tree being fetched was\n> signed by a trusted key.\n>\n> It was unclear which commits needed to be signed (in the case of `git\n> merge`) and what were the criteria for the signature to be considered\n> valid.\n>\n> Here is a patch proposal.\n>\n> Signed-off-by: The Fox in the Shell \n> ---\n\nI'll leave commenting on and suggesting updates for the above to\nothers.\n\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index f08e9b8..edd50bf 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -89,8 +89,10 @@ option can be used to override --squash.\n> \n> --verify-signatures::\n> --no-verify-signatures::\n> -\tVerify that the commits being merged have good and trusted GPG signatures\n> +\tVerify that the commits being merged have good and valid GPG signatures\n> \tand abort the merge in case they do not.\n> +\tFor instance, when running `git merge --verify-signature remote/branch`,\n> +\tonly the head commit on `remote/branch` needs to be signed.\n\nThe first part of this change and all other changes are of dubious\nvalue, but the last two lines is truly an improvement--it adds\nmissing information people who use the feature may care about.\n\nI'd suggest doing the addition of the last two lines as a standalone\npatch, and make the remainder a separate patch on top.\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 671cebd..29b19b9 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -143,8 +143,8 @@ ifndef::git-rev-list[]\n> - '%N': commit notes\n> endif::git-rev-list[]\n> - '%GG': raw verification message from GPG for a signed commit\n> -- '%G?': show \"G\" for a Good signature, \"B\" for a Bad signature, \"U\" for a good,\n> - untrusted signature and \"N\" for no signature\n> +- '%G?': show \"G\" for a good (valid) signature, \"B\" for a bad signature,\n> + \"U\" for a good signature with unknown validity and \"N\" for no signature\n\nThe reason I said the other changes are of dubious value is shown\nvery well in this hunk. I am not sure if it is an improvement to\nrephrase \"Good\" to \"good (valid)\" and \"untrusted\" to \"good signature\nwith unknown validity\". They are saying pretty much the same thing,\nno?\n\n> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\n> index 54b88b6..62cbae2 100644\n> --- a/Documentation/pretty-options.txt\n> +++ b/Documentation/pretty-options.txt\n> @@ -78,5 +78,5 @@ being displayed. Examples: \"--notes=foo\" will show only notes from\n> endif::git-rev-list[]\n> \n> --show-signature::\n> -\tCheck the validity of a signed commit object by passing the signature\n> -\tto `gpg --verify` and show the output.\n> +\tCheck the validity of a signed commit object, by passing the signature\n> +\tto `gpg --verify`, and show the output.\n\nThe update one may be gramattically correct, but I personally find\nthe original easier to read. Is there a reason for this change?\n"},{"id":"283120","messageId":"20160411003242.GC9034@hashbang.sh","threadId":"41976","inReplyTo":"xmqqa8l1ti8d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Documentation: clarify signature verification","fromName":"KellerFuchs","fromEmail":"kellerfuchs@hashbang.sh","sentAt":"2016-04-11T00:32:42Z","receivedAt":"2016-04-11T00:32:42Z","isPatch":true,"sender":{"key":"kellerfuchs@hashbang.sh","avatar":"https://avatars.githubusercontent.com/u/14331571?v=4"},"body":"On Sun, Apr 10, 2016 at 11:46:10AM -0700, Junio C Hamano wrote:\n> > --- a/Documentation/merge-options.txt\n> > +++ b/Documentation/merge-options.txt\n> > @@ -89,8 +89,10 @@ option can be used to override --squash.\n> > \n> > --verify-signatures::\n> > --no-verify-signatures::\n> > -\tVerify that the commits being merged have good and trusted GPG signatures\n> > +\tVerify that the commits being merged have good and valid GPG signatures\n> > \tand abort the merge in case they do not.\n> > +\tFor instance, when running `git merge --verify-signature remote/branch`,\n> > +\tonly the head commit on `remote/branch` needs to be signed.\n> \n> The first part of this change and all other changes are of dubious\n> value, but the last two lines is truly an improvement--it adds\n> missing information people who use the feature may care about.\n\nThe reason for the first edit is that “trusted” and “valid” are OpenPGP\n concepts: a key is trusted if the user set a trust level for it,\n and a uid is valid if it has been signed by a trusted key [0].\n\nMost of my confusion came from this, since it sounded like the signature\n would only be accepted if it came from a key with a non-zero ownertrust.\n\n[0] That actually only holds for the default trust model,\n so I'm oversimplifying a bit here.\n\n> I'd suggest doing the addition of the last two lines as a standalone\n> patch, and make the remainder a separate patch on top.\n\nSure, will do when submitting for inclusion.\n\n> > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> > index 671cebd..29b19b9 100644\n> > --- a/Documentation/pretty-formats.txt\n> > +++ b/Documentation/pretty-formats.txt\n> > @@ -143,8 +143,8 @@ ifndef::git-rev-list[]\n> > - '%N': commit notes\n> > endif::git-rev-list[]\n> > - '%GG': raw verification message from GPG for a signed commit\n> > -- '%G?': show \"G\" for a Good signature, \"B\" for a Bad signature, \"U\" for a good,\n> > - untrusted signature and \"N\" for no signature\n> > +- '%G?': show \"G\" for a good (valid) signature, \"B\" for a bad signature,\n> > + \"U\" for a good signature with unknown validity and \"N\" for no signature\n> \n> The reason I said the other changes are of dubious value is shown\n> very well in this hunk. I am not sure if it is an improvement to\n> rephrase \"Good\" to \"good (valid)\" and \"untrusted\" to \"good signature\n> with unknown validity\". They are saying pretty much the same thing,\n> no?\n\nAs said above, it was about consistency with the OpenPGP terminology.\n\nIf git wants to have it's own vocabulary for that (which I would argue\n against), then it would need to be defined somewhere.\n\n> > diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt\n> > index 54b88b6..62cbae2 100644\n> > --- a/Documentation/pretty-options.txt\n> > +++ b/Documentation/pretty-options.txt\n> > @@ -78,5 +78,5 @@ being displayed. Examples: \"--notes=foo\" will show only notes from\n> > endif::git-rev-list[]\n> > \n> > --show-signature::\n> > -\tCheck the validity of a signed commit object by passing the signature\n> > -\tto `gpg --verify` and show the output.\n> > +\tCheck the validity of a signed commit object, by passing the signature\n> > +\tto `gpg --verify`, and show the output.\n> \n> The update one may be gramattically correct, but I personally find\n> the original easier to read. Is there a reason for this change?\n\nThat one is arguably more dubious, and I would happily drop it.\nFor some reason, I kept parsing it as “Check the validity [...] by\n (passing the signature to `gpg --verify` and showing the output)”.\n\n\nBest regards,\n\n kf\n"},{"id":"283142","messageId":"xmqqtwj8rtcd.fsf@gitster.mtv.corp.google.com","threadId":"41976","inReplyTo":"20160411003242.GC9034@hashbang.sh","subject":"Re: [PATCH] Documentation: clarify signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-11T16:41:22Z","receivedAt":"2016-04-11T16:41:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"KellerFuchs writes:\n\n> On Sun, Apr 10, 2016 at 11:46:10AM -0700, Junio C Hamano wrote:\n>> > --- a/Documentation/merge-options.txt\n>> > +++ b/Documentation/merge-options.txt\n>> > @@ -89,8 +89,10 @@ option can be used to override --squash.\n>> > \n>> > --verify-signatures::\n>> > --no-verify-signatures::\n>> > -\tVerify that the commits being merged have good and trusted GPG signatures\n>> > +\tVerify that the commits being merged have good and valid GPG signatures\n>> > \tand abort the merge in case they do not.\n>> > +\tFor instance, when running `git merge --verify-signature remote/branch`,\n>> > +\tonly the head commit on `remote/branch` needs to be signed.\n>> \n>> The first part of this change and all other changes are of dubious\n>> value, but the last two lines is truly an improvement--it adds\n>> missing information people who use the feature may care about.\n>\n> The reason for the first edit is that “trusted” and “valid” are OpenPGP\n> concepts: a key is trusted if the user set a trust level for it,\n> and a uid is valid if it has been signed by a trusted key [0].\n\nOK, so it is wrong to talk about \"trusted\" and/or \"valid\" \"GPG\nsignatures\" like the original one. We should say \"... have GPG\nsignatures that were signed by valid key\" (not \"trusted\" key)?\n\n> Most of my confusion came from this, since it sounded like the signature\n> would only be accepted if it came from a key with a non-zero ownertrust.\n\nThanks for clarification. The distinction between trusted and valid\nshould at least be in the log message and possibly (if we can find a\ngood way to flow it into the description) added to the documentation.\n\nPerhaps like this?\n\n Verify that the tip commit of the side branch being merged is\n signed with a valid key (i.e. a key that is signed by a key that\n the user set the trust level as trusted), and abort the merge if\n it is not.\n"},{"id":"283162","messageId":"20160412010009.GD9034@hashbang.sh","threadId":"41976","inReplyTo":"xmqqtwj8rtcd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Documentation: clarify signature verification","fromName":"KellerFuchs","fromEmail":"kellerfuchs@hashbang.sh","sentAt":"2016-04-12T01:00:09Z","receivedAt":"2016-04-12T01:00:09Z","isPatch":true,"sender":{"key":"kellerfuchs@hashbang.sh","avatar":"https://avatars.githubusercontent.com/u/14331571?v=4"},"body":"On Mon, Apr 11, 2016 at 09:41:22AM -0700, Junio C Hamano wrote:\n> KellerFuchs writes:\n> > The reason for the first edit is that “trusted” and “valid” are OpenPGP\n> > concepts: a key is trusted if the user set a trust level for it,\n> > and a uid is valid if it has been signed by a trusted key [0].\n> \n> OK, so it is wrong to talk about \"trusted\" and/or \"valid\" \"GPG\n> signatures\" like the original one. We should say \"... have GPG\n> signatures that were signed by valid key\" (not \"trusted\" key)?\n\nWell, the GnuPG documentation also talks of valid signatures,\n and it is a convenient short-hand:\n\n https://www.gnupg.org/documentation/manuals/gpgme/Verify.html\n \nOn the other hand, being more explicit here cannot hurt.\n\n> Thanks for clarification. The distinction between trusted and valid\n> should at least be in the log message and possibly (if we can find a\n> good way to flow it into the description) added to the documentation.\n\nOk. I will have a second go at the patch (with the split you requested,\n a more explicit description and an explanation in the commit msg).\n\nWhat is the prefered way to send a second version of a patchset here?\nJust git-email-ing it here In-Reply-To the first mail?\n\n> Verify that the tip commit of the side branch being merged is\n> signed with a valid key (i.e. a key that is signed by a key that\n> the user set the trust level as trusted), and abort the merge if\n> it is not.\n\nI would rather see something like\n\n> Verify that the tip commit of the side branch being merged is\n> signed with a valid key, i.e. a key that has a valid uid: in the\n> default trust model, this means it has been signed by a trusted key.\n> If the tip commit of the side branch is not signed with a valid key,\n> the merge is aborted.\n\nIt's unfortunately more verbose, but I don't want to make promises\n about GnuPG's behaviour that depends on the user's configuration.\n\n\nBest,\n\n kf\n"},{"id":"283192","messageId":"xmqqh9f6su8l.fsf@gitster.mtv.corp.google.com","threadId":"41976","inReplyTo":"20160412010009.GD9034@hashbang.sh","subject":"Re: [PATCH] Documentation: clarify signature verification","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T15:48:58Z","receivedAt":"2016-04-12T15:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"KellerFuchs writes:\n\n> I would rather see something like\n>\n>> Verify that the tip commit of the side branch being merged is\n>> signed with a valid key, i.e. a key that has a valid uid: in the\n>> default trust model, this means it has been signed by a trusted key.\n>> If the tip commit of the side branch is not signed with a valid key,\n>> the merge is aborted.\n>\n> It's unfortunately more verbose, but I don't want to make promises\n> about GnuPG's behaviour that depends on the user's configuration.\n\nGood thinking. Thanks.\n"},{"id":"286440","messageId":"1463133061-6834-1-git-send-email-KellerFuchs@hashbang.sh","threadId":"41976","inReplyTo":"20160409200756.GA22694@hashbang.sh","subject":"[PATCH] Documentation: clarify signature verification","fromName":"Fox in the shell","fromEmail":"kellerfuchs@hashbang.sh","sentAt":"2016-05-13T09:51:01Z","receivedAt":"2016-05-13T09:51:01Z","isPatch":true,"sender":{"key":"kellerfuchs@hashbang.sh","avatar":"https://avatars.githubusercontent.com/u/14331571?v=4"},"body":"From: Keller Fuchs \n\nUniformise the vocabulary used wrt. key/signature validity with OpenPGP:\n- a signature is valid if made by a key with a valid uid;\n- in the default trust-model, a uid is valid if signed by a trusted key;\n- a key is trusted if the (local) user set a trust level for it.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Keller Fuchs \n---\n Documentation/merge-options.txt | 7 +++++--\n Documentation/pretty-formats.txt | 4 ++--\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex f08e9b8..30808a0 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -89,8 +89,11 @@ option can be used to override --squash.\n \n --verify-signatures::\n --no-verify-signatures::\n-\tVerify that the commits being merged have good and trusted GPG signatures\n-\tand abort the merge in case they do not.\n+\tVerify that the tip commit of the side branch being merged is\n+\tsigned with a valid key, i.e. a key that has a valid uid: in the\n+\tdefault trust model, this means the signing key has been signed by\n+\ta trusted key. If the tip commit of the side branch is not signed\n+\twith a valid key, the merge is aborted.\n \n --summary::\n --no-summary::\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 671cebd..29b19b9 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -143,8 +143,8 @@ ifndef::git-rev-list[]\n - '%N': commit notes\n endif::git-rev-list[]\n - '%GG': raw verification message from GPG for a signed commit\n-- '%G?': show \"G\" for a Good signature, \"B\" for a Bad signature, \"U\" for a good,\n- untrusted signature and \"N\" for no signature\n+- '%G?': show \"G\" for a good (valid) signature, \"B\" for a bad signature,\n+ \"U\" for a good signature with unknown validity and \"N\" for no signature\n - '%GS': show the name of the signer for a signed commit\n - '%GK': show the key used to sign a signed commit\n - '%gD': reflog selector, e.g., `refs/stash@{1}`\n-- \n2.1.4\n"}]}