threads / patch / 41976

patchDocumentation: clarify signature verification

Subject: [PATCH] Documentation: clarify signature verification

## tl;dr

7 messages between Apr 9, 2016 and May 13, 2016. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

The Fox in the Shell· Apr 9, 2016, 20:08 UTC · lore
Hi,

I encountered some issues with the git documentation while modifying my deployment scripts to enforce that the tree being fetched was signed by a trusted key.

It was unclear which commits needed to be signed (in the case of `git merge`) and what were the criteria for the signature to be considered valid.

Here is a patch proposal.
Signed-off-by: The Fox in the Shell <KellerFuchs@hashbang.sh>
---
 Documentation/merge-options.txt  | 4 +++-
 Documentation/pretty-formats.txt | 4 ++--
 Documentation/pretty-options.txt | 4 ++--
 3 files changed, 7 insertions(+), 5 deletions(-)
Show changes to 3 files +7 −5

Documentation/merge-options.txt, Documentation/pretty-formats.txt, Documentation/pretty-options.txt

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index f08e9b8..edd50bf 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -89,8 +89,10 @@ option can be used to override --squash.
 
 --verify-signatures::
 --no-verify-signatures::
-	Verify that the commits being merged have good and trusted GPG signatures
+	Verify that the commits being merged have good and valid GPG signatures
 	and abort the merge in case they do not.
+	For instance, when running `git merge --verify-signature remote/branch`,
+	only the head commit on `remote/branch` needs to be signed.
 
 --summary::
 --no-summary::
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 671cebd..29b19b9 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,8 @@ ifndef::git-rev-list[]
 - '%N': commit notes
 endif::git-rev-list[]
 - '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a Good signature, "B" for a Bad signature, "U" for a good,
-  untrusted signature and "N" for no signature
+- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
+  "U" for a good signature with unknown validity and "N" for no signature
 - '%GS': show the name of the signer for a signed commit
 - '%GK': show the key used to sign a signed commit
 - '%gD': reflog selector, e.g., `refs/stash@{1}`
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 54b88b6..62cbae2 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -78,5 +78,5 @@ being displayed. Examples: "--notes=foo" will show only notes from
 endif::git-rev-list[]
 
 --show-signature::
-	Check the validity of a signed commit object by passing the signature
-	to `gpg --verify` and show the output.
+	Check the validity of a signed commit object, by passing the signature
+	to `gpg --verify`, and show the output.

-- 
2.1.4
Junio C Hamano· Apr 10, 2016, 18:46 UTC · re: The Fox in the Shell · lore

Re: [PATCH] Documentation: clarify signature verification

The Fox in the Shell <KellerFuchs@hashbang.sh> writes:
Show 14 quoted lines
> Hi,
>
> I encountered some issues with the git documentation while modifying
> my deployment scripts to enforce that the tree being fetched was
> signed by a trusted key.
>
> It was unclear which commits needed to be signed (in the case of `git
> merge`) and what were the criteria for the signature to be considered
> valid.
>
> Here is a patch proposal.
>
> Signed-off-by: The Fox in the Shell <KellerFuchs@hashbang.sh>
> ---

I'll leave commenting on and suggesting updates for the above to others.

Show 13 quoted lines
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index f08e9b8..edd50bf 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -89,8 +89,10 @@ option can be used to override --squash.
>  
>  --verify-signatures::
>  --no-verify-signatures::
> -	Verify that the commits being merged have good and trusted GPG signatures
> +	Verify that the commits being merged have good and valid GPG signatures
>  	and abort the merge in case they do not.
> +	For instance, when running `git merge --verify-signature remote/branch`,
> +	only the head commit on `remote/branch` needs to be signed.

The first part of this change and all other changes are of dubious value, but the last two lines is truly an improvement--it adds missing information people who use the feature may care about.

I'd suggest doing the addition of the last two lines as a standalone patch, and make the remainder a separate patch on top.

Show 12 quoted lines
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 671cebd..29b19b9 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -143,8 +143,8 @@ ifndef::git-rev-list[]
>  - '%N': commit notes
>  endif::git-rev-list[]
>  - '%GG': raw verification message from GPG for a signed commit
> -- '%G?': show "G" for a Good signature, "B" for a Bad signature, "U" for a good,
> -  untrusted signature and "N" for no signature
> +- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
> +  "U" for a good signature with unknown validity and "N" for no signature

The reason I said the other changes are of dubious value is shown very well in this hunk. I am not sure if it is an improvement to rephrase "Good" to "good (valid)" and "untrusted" to "good signature with unknown validity". They are saying pretty much the same thing, no?

Show 12 quoted lines
> diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
> index 54b88b6..62cbae2 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -78,5 +78,5 @@ being displayed. Examples: "--notes=foo" will show only notes from
>  endif::git-rev-list[]
>  
>  --show-signature::
> -	Check the validity of a signed commit object by passing the signature
> -	to `gpg --verify` and show the output.
> +	Check the validity of a signed commit object, by passing the signature
> +	to `gpg --verify`, and show the output.

The update one may be gramattically correct, but I personally find the original easier to read. Is there a reason for this change?

KellerFuchs· Apr 11, 2016, 00:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] Documentation: clarify signature verification

On Sun, Apr 10, 2016 at 11:46:10AM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> > --- a/Documentation/merge-options.txt
> > +++ b/Documentation/merge-options.txt
> > @@ -89,8 +89,10 @@ option can be used to override --squash.
> >  
> >  --verify-signatures::
> >  --no-verify-signatures::
> > -	Verify that the commits being merged have good and trusted GPG signatures
> > +	Verify that the commits being merged have good and valid GPG signatures
> >  	and abort the merge in case they do not.
> > +	For instance, when running `git merge --verify-signature remote/branch`,
> > +	only the head commit on `remote/branch` needs to be signed.
> 
> The first part of this change and all other changes are of dubious
> value, but the last two lines is truly an improvement--it adds
> missing information people who use the feature may care about.
The reason for the first edit is that “trusted” and “valid” are OpenPGP
  concepts: a key is trusted if the user set a trust level for it,
  and a uid is valid if it has been signed by a trusted key [0].
Most of my confusion came from this, since it sounded like the signature
  would only be accepted if it came from a key with a non-zero ownertrust.
[0] That actually only holds for the default trust model,
    so I'm oversimplifying a bit here.
> I'd suggest doing the addition of the last two lines as a standalone
> patch, and make the remainder a separate patch on top.
Sure, will do when submitting for inclusion.
Show 18 quoted lines
> > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> > index 671cebd..29b19b9 100644
> > --- a/Documentation/pretty-formats.txt
> > +++ b/Documentation/pretty-formats.txt
> > @@ -143,8 +143,8 @@ ifndef::git-rev-list[]
> >  - '%N': commit notes
> >  endif::git-rev-list[]
> >  - '%GG': raw verification message from GPG for a signed commit
> > -- '%G?': show "G" for a Good signature, "B" for a Bad signature, "U" for a good,
> > -  untrusted signature and "N" for no signature
> > +- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
> > +  "U" for a good signature with unknown validity and "N" for no signature
> 
> The reason I said the other changes are of dubious value is shown
> very well in this hunk.  I am not sure if it is an improvement to
> rephrase "Good" to "good (valid)" and "untrusted" to "good signature
> with unknown validity".  They are saying pretty much the same thing,
> no?
As said above, it was about consistency with the OpenPGP terminology.
If git wants to have it's own vocabulary for that (which I would argue
  against), then it would need to be defined somewhere.
Show 15 quoted lines
> > diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
> > index 54b88b6..62cbae2 100644
> > --- a/Documentation/pretty-options.txt
> > +++ b/Documentation/pretty-options.txt
> > @@ -78,5 +78,5 @@ being displayed. Examples: "--notes=foo" will show only notes from
> >  endif::git-rev-list[]
> >  
> >  --show-signature::
> > -	Check the validity of a signed commit object by passing the signature
> > -	to `gpg --verify` and show the output.
> > +	Check the validity of a signed commit object, by passing the signature
> > +	to `gpg --verify`, and show the output.
> 
> The update one may be gramattically correct, but I personally find
> the original easier to read.  Is there a reason for this change?
That one is arguably more dubious, and I would happily drop it.
For some reason, I kept parsing it as “Check the validity [...] by
  (passing the signature to `gpg --verify` and showing the output)”.
Best regards,
  kf
Junio C Hamano· Apr 11, 2016, 16:41 UTC · re: KellerFuchs · lore

Re: [PATCH] Documentation: clarify signature verification

KellerFuchs <KellerFuchs@hashbang.sh> writes:
Show 20 quoted lines
> On Sun, Apr 10, 2016 at 11:46:10AM -0700, Junio C Hamano wrote:
>> > --- a/Documentation/merge-options.txt
>> > +++ b/Documentation/merge-options.txt
>> > @@ -89,8 +89,10 @@ option can be used to override --squash.
>> >  
>> >  --verify-signatures::
>> >  --no-verify-signatures::
>> > -	Verify that the commits being merged have good and trusted GPG signatures
>> > +	Verify that the commits being merged have good and valid GPG signatures
>> >  	and abort the merge in case they do not.
>> > +	For instance, when running `git merge --verify-signature remote/branch`,
>> > +	only the head commit on `remote/branch` needs to be signed.
>> 
>> The first part of this change and all other changes are of dubious
>> value, but the last two lines is truly an improvement--it adds
>> missing information people who use the feature may care about.
>
> The reason for the first edit is that “trusted” and “valid” are OpenPGP
>   concepts: a key is trusted if the user set a trust level for it,
>   and a uid is valid if it has been signed by a trusted key [0].

OK, so it is wrong to talk about "trusted" and/or "valid" "GPG signatures" like the original one. We should say "... have GPG signatures that were signed by valid key" (not "trusted" key)?

> Most of my confusion came from this, since it sounded like the signature
>   would only be accepted if it came from a key with a non-zero ownertrust.

Thanks for clarification. The distinction between trusted and valid should at least be in the log message and possibly (if we can find a good way to flow it into the description) added to the documentation.

Perhaps like this?
    Verify that the tip commit of the side branch being merged is
    signed with a valid key (i.e. a key that is signed by a key that
    the user set the trust level as trusted), and abort the merge if
    it is not.
KellerFuchs· Apr 12, 2016, 01:00 UTC · re: Junio C Hamano · lore

Re: [PATCH] Documentation: clarify signature verification

On Mon, Apr 11, 2016 at 09:41:22AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> KellerFuchs <KellerFuchs@hashbang.sh> writes:
> > The reason for the first edit is that “trusted” and “valid” are OpenPGP
> >   concepts: a key is trusted if the user set a trust level for it,
> >   and a uid is valid if it has been signed by a trusted key [0].
> 
> OK, so it is wrong to talk about "trusted" and/or "valid" "GPG
> signatures" like the original one.  We should say "... have GPG
> signatures that were signed by valid key" (not "trusted" key)?
Well, the GnuPG documentation also talks of valid signatures,
  and it is a convenient short-hand:
  https://www.gnupg.org/documentation/manuals/gpgme/Verify.html
  
On the other hand, being more explicit here cannot hurt.
> Thanks for clarification.  The distinction between trusted and valid
> should at least be in the log message and possibly (if we can find a
> good way to flow it into the description) added to the documentation.
Ok.  I will have a second go at the patch (with the split you requested,
  a more explicit description and an explanation in the commit msg).

What is the prefered way to send a second version of a patchset here? Just git-email-ing it here In-Reply-To the first mail?

>     Verify that the tip commit of the side branch being merged is
>     signed with a valid key (i.e. a key that is signed by a key that
>     the user set the trust level as trusted), and abort the merge if
>     it is not.
I would rather see something like
Show 5 quoted lines
>     Verify that the tip commit of the side branch being merged is
>     signed with a valid key, i.e. a key that has a valid uid: in the
>     default trust model, this means it has been signed by a trusted key.
>     If the tip commit of the side branch is not signed with a valid key,
>     the merge is aborted.
It's unfortunately more verbose, but I don't want to make promises
  about GnuPG's behaviour that depends on the user's configuration.
Best,
  kf
Junio C Hamano· Apr 12, 2016, 15:48 UTC · re: KellerFuchs · lore

Re: [PATCH] Documentation: clarify signature verification

KellerFuchs <KellerFuchs@hashbang.sh> writes:
Show 10 quoted lines
> I would rather see something like
>
>>     Verify that the tip commit of the side branch being merged is
>>     signed with a valid key, i.e. a key that has a valid uid: in the
>>     default trust model, this means it has been signed by a trusted key.
>>     If the tip commit of the side branch is not signed with a valid key,
>>     the merge is aborted.
>
> It's unfortunately more verbose, but I don't want to make promises
> about GnuPG's behaviour that depends on the user's configuration.
Good thinking.  Thanks.
Fox in the shell· May 13, 2016, 09:51 UTC · re: The Fox in the Shell · lore
From: Keller Fuchs <kellerfuchs@hashbang.sh>
Uniformise the vocabulary used wrt. key/signature validity with OpenPGP:
- a signature is valid if made by a key with a valid uid;
- in the default trust-model, a uid is valid if signed by a trusted key;
- a key is trusted if the (local) user set a trust level for it.
Helped-by:     Junio C Hamano <gitster@pobox.com>
Signed-off-by: Keller Fuchs   <KellerFuchs@hashbang.sh>
---
 Documentation/merge-options.txt  | 7 +++++--
 Documentation/pretty-formats.txt | 4 ++--
 2 files changed, 7 insertions(+), 4 deletions(-)
Show changes to 2 files +7 −4

Documentation/merge-options.txt, Documentation/pretty-formats.txt

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index f08e9b8..30808a0 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -89,8 +89,11 @@ option can be used to override --squash.
 
 --verify-signatures::
 --no-verify-signatures::
-	Verify that the commits being merged have good and trusted GPG signatures
-	and abort the merge in case they do not.
+	Verify that the tip commit of the side branch being merged is
+	signed with a valid key, i.e. a key that has a valid uid: in the
+	default trust model, this means the signing key has been signed by
+	a trusted key.  If the tip commit of the side branch is not signed
+	with a valid key, the merge is aborted.
 
 --summary::
 --no-summary::
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 671cebd..29b19b9 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -143,8 +143,8 @@ ifndef::git-rev-list[]
 - '%N': commit notes
 endif::git-rev-list[]
 - '%GG': raw verification message from GPG for a signed commit
-- '%G?': show "G" for a Good signature, "B" for a Bad signature, "U" for a good,
-  untrusted signature and "N" for no signature
+- '%G?': show "G" for a good (valid) signature, "B" for a bad signature,
+  "U" for a good signature with unknown validity and "N" for no signature
 - '%GS': show the name of the signer for a signed commit
 - '%GK': show the key used to sign a signed commit
 - '%gD': reflog selector, e.g., `refs/stash@{1}`

-- 
2.1.4

← back to recent threads