[PATCH] Documentation: clarify signature verification

[PATCH] Documentation: clarify signature verification

[The Fox in the Shell]

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(-)

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

Re: [PATCH] Documentation: clarify signature verification

[Junio C Hamano]

The Fox in the Shell <KellerFuchs@hashbang.sh> writes:

> 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.

> 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.

> 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?

> 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?

Re: [PATCH] Documentation: clarify signature verification

[KellerFuchs]

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].

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.

> > 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.

> > 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

Re: [PATCH] Documentation: clarify signature verification

[Junio C Hamano]

KellerFuchs <KellerFuchs@hashbang.sh> writes:

> 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.

Re: [PATCH] Documentation: clarify signature verification

[KellerFuchs]

On Mon, Apr 11, 2016 at 09:41:22AM -0700, Junio C Hamano wrote:
> 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

>     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

Re: [PATCH] Documentation: clarify signature verification

[Junio C Hamano]

KellerFuchs <KellerFuchs@hashbang.sh> writes:

> 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.

[PATCH] Documentation: clarify signature verification

[Fox in the shell]

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(-)

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