Re: pre-push signature hook error reporting

[Ludovic Courtès]

Marius Bakke  skribis:

> Leo Famulari  writes:
>
>> On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
>>> For the pre-push hook, the overhead seems reasonable (perhaps we could
>>> limit the range to commits after the first signed commit to avoid
>>> looping for no reason?) and an improvement.
>>
>> Here is a patch for the hook that I've been using for the past couple weeks.
>>
>> For the common use case of pushing new commits to an existing branch, I
>> don't notice the hook at all, except when it catches my mistakes.
>
> Thanks a lot for this! I haven't tested it, but the code LGTM.

Ditto, thank you!

Ludo’.

Re: pre-push signature hook error reporting

[Marius Bakke]

Leo Famulari  writes:

> On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
>> For the pre-push hook, the overhead seems reasonable (perhaps we could
>> limit the range to commits after the first signed commit to avoid
>> looping for no reason?) and an improvement.
>
> Here is a patch for the hook that I've been using for the past couple weeks.
>
> For the common use case of pushing new commits to an existing branch, I
> don't notice the hook at all, except when it catches my mistakes.

Thanks a lot for this! I haven't tested it, but the code LGTM.


signature.asc
Description: PGP signature

Re: pre-push signature hook error reporting

[Leo Famulari]

On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
> For the pre-push hook, the overhead seems reasonable (perhaps we could
> limit the range to commits after the first signed commit to avoid
> looping for no reason?) and an improvement.

Here is a patch for the hook that I've been using for the past couple weeks.

For the common use case of pushing new commits to an existing branch, I
don't notice the hook at all, except when it catches my mistakes.
From 7d8206949f98a121bb2d50e0eecfcba1d9cce27a Mon Sep 17 00:00:00 2001
From: Leo Famulari 
Date: Mon, 23 Jan 2017 00:57:46 -0500
Subject: [PATCH] etc: The pre-push hook says which commits failed the
 signature check.

* etc/git/pre-push: Check each commit's signature individually so that
we can report which commits fail the check.
---
 etc/git/pre-push | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/etc/git/pre-push b/etc/git/pre-push
index c894c5a9e..9206a2dfe 100755
--- a/etc/git/pre-push
+++ b/etc/git/pre-push
@@ -40,17 +40,29 @@ do
else
if [ "$remote_sha" = $z40 ]
then
-   # New branch, examine all commits
-   range="$local_sha"
+   # We are pushing a new branch. To prevent wasting too
+   # much time for this relatively rare case, we examine
+   # all commits since the first signed commit, rather than
+   # the full history. This check *will* fail, and the user
+   # will need to temporarily disable the hook to push the
+   # new branch.
+   
range="e3d0fcbf7e55e8cbe8d0a1c5a24d73f341d7243b..$local_sha"
else
# Update to existing branch, examine new commits
range="$remote_sha..$local_sha"
fi
 
# Verify the signatures of all commits being pushed.
-   git verify-commit $(git rev-list $range) >/dev/null 2>&1
-
-   exit $?
+   ret=0
+   for commit in $(git rev-list $range)
+   do
+   if ! git verify-commit $commit >/dev/null 2>&1
+   then
+   printf "%s failed signature check\n" $commit
+   ret=1
+   fi
+   done
+   exit $ret
fi
 done
 
-- 
2.11.0



signature.asc
Description: PGP signature

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
> For the pre-push hook, the overhead seems reasonable (perhaps we could
> limit the range to commits after the first signed commit to avoid
> looping for no reason?) and an improvement.

I agree that it's reasonable and an improvement for the common case of
pushing to existing branches; only the new commits' signatures are
verified in this case.

It's a good idea to limit the range when pushing new branches. It will
still fail invariably, but it will fail more quickly.

I believe the first signed commit is e3d0fcbf7e55 (gnu: Default to
GCC 5.).

Due to merges in the history (I think), using `git rev-list` to
enumerate the commits from e3d0fcbf7e55^..HEAD gives a list of commits
begins with aae03c484f21832 (gnu: Add singular.), which is an earlier
commit.

That's a little confusing, but maybe it doesn't matter if we are just
trying to save the user some time before it fails. They'll have to
disable the hook to push a branch anyways.

WDYT?

> Eventually we could rewrite in Scheme using guile-git, which should be
> faster (no need to fork that much).

Yes, that would be good!


signature.asc
Description: PGP signature

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Ludovic Courtès]

Leo Famulari  skribis:

> On Fri, Jan 13, 2017 at 10:24:00AM -0500, Leo Famulari wrote:
>> I bet that you are using the new pre-push hook that verifies commit
>> signatures, and you're trying to push some commits that fail the
>> signature verification check.
>> 
>> Someone should add some error reporting to the hook.
>
> In Git 2.11.0, it seems that `git verify-commit` can't tell the user
> which commits failed verification:
>
> https://git.kernel.org/cgit/git/git.git/tree/builtin/verify-commit.c?h=v2.11.0
>
> With a warm cache and all the public keys on my machine, checking the
> signature of all 17813 commits on the master branch takes ~40 seconds
> with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push
> hook does now.
>
> Checking the commits one at a time takes ~105 seconds, using something
> like this:
>
> for commit in $(git rev-list HEAD); do
>   if ! git verify-commit $commit; then
>   echo $commit
>   fi
> done
>
> We could make the hook do something like that. Thoughts? I think the
> performance regression is worth the convenience of knowing why it
> failed.

For the pre-push hook, the overhead seems reasonable (perhaps we could
limit the range to commits after the first signed commit to avoid
looping for no reason?) and an improvement.

Eventually we could rewrite in Scheme using guile-git, which should be
faster (no need to fork that much).

Thanks!

Ludo’.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Tue, Jan 17, 2017 at 01:56:29PM +0100, Hartmut Goebel wrote:
> Am 17.01.2017 um 12:34 schrieb Danny Milosavljevic:
> > For minimal improvement (I don't even think it's measureable), try
> > `git rev-list HEAD` (backquotes) - it prevents having to spawn a
> > subshell.
> 
> Huh? I doubt this. The bash manual, section "Command Substitution" does
> not distinguish between these both, as far as I understand it.

The POSIX shell command language specification says that:

"The shell shall expand the command substitution by executing command in
a subshell environment (see Shell Execution Environment) and replacing
the command substitution (the text of command plus the enclosing "$()"
or backquotes) with the standard output of the command, removing
sequences of one or more s at the end of the substitution."

http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_06_03

Maybe it's faster, maybe not, but I think my benchmark was
misinterpreted...

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Tue, Jan 17, 2017 at 03:55:42PM +0100, Hartmut Goebel wrote:
> Am 17.01.2017 um 04:14 schrieb Leo Famulari:
> > with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push
> 
> As far as I understand the example hook, it would be possible to do
> something like
> 
> git verify-commit $(git rev-list $remote_sha..$local_sha)
> 
> this would only verify the commits which are going to be pushed.

I'm sorry that I was unclear.

If you read the hook, you'll see that it does what you suggest. I chose
to check the entire history as a benchmark.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Tue, Jan 17, 2017 at 12:34:28PM +0100, Danny Milosavljevic wrote:
> Leo Famulari  wrote:
> > In Git 2.11.0, it seems that `git verify-commit` can't tell the user
> > which commits failed verification:
> 
> We should report that upstream and add the one line that does tell the
> user which commits failed verification upstream (for example print
> argv[i-1] in line 92). 

Well, it does print the output of `gpg`, but parsing that is
error-prone. I'm sure Git would take a patch that did the right thing,
however.

> Uhhh it's already very slow... so even slower doesn't matter anymore
> (HIG guideline maximum duration is 2 seconds, so we are way off
> anyhow).

Do you notice it in practice? Or do you mean that 40 seconds is already
very slow?

Remember that typical usage does not involve checking every commit, but
only the handful of new commits; this is *much* faster. Checking all
commits is just the benchmark I chose before starting because I wanted
any performance difference to be starkly illustrated.

> Do we think that failures are likely?

Yes, we've seen *bad* signatures pushed to Savannah recently, so I think
it's important for everyone to check their commits before pushing.

> Also, git seems to invoke the gpg executable for each and every
> commit. It would be interesting whether gpg-interface.c
> verify_signed_buffer could be adapted to either invoke gpg once or to
> just use a library instead (gpgme ?). 

Indeed, that would be better.

> Long term we could also cache the checking result - I think that's
> something more difficult in the face of keys that expire. It would
> have to store at least the expiration date, the public key and the
> list of commit hashes that were checked and validated successfully.

Agreed. But this hook is only a convenience tool to prevent mistakes.
We need to revamp `guix pull` to handle this properly.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Hartmut Goebel]

Am 17.01.2017 um 12:34 schrieb Danny Milosavljevic:
> For minimal improvement (I don't even think it's measureable), try `git 
> rev-list HEAD` (backquotes) - it prevents having to spawn a subshell.

Huh? I doubt this. The bash manual, section "Command Substitution" does
not distinguish between these both, as far as I understand it.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel  | h.goe...@crazy-compilers.com   |
| www.crazy-compilers.com | compilers which you thought are impossible |



0xBF773B65.asc
Description: application/pgp-keys

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Danny Milosavljevic]

Hi Leo,

On Mon, 16 Jan 2017 22:14:14 -0500
Leo Famulari  wrote:

> In Git 2.11.0, it seems that `git verify-commit` can't tell the user
> which commits failed verification:
> 
> https://git.kernel.org/cgit/git/git.git/tree/builtin/verify-commit.c?h=v2.11.0

We should report that upstream and add the one line that does tell the user 
which commits failed verification upstream (for example print argv[i-1] in line 
92). 

> With a warm cache and all the public keys on my machine, checking the
> signature of all 17813 commits on the master branch takes ~40 seconds
...
> Checking the commits one at a time takes ~105 seconds, using something
> like this:
> 
> for commit in $(git rev-list HEAD); do

For minimal improvement (I don't even think it's measureable), try `git 
rev-list HEAD` (backquotes) - it prevents having to spawn a subshell.

> We could make the hook do something like that. Thoughts? I think the
> performance regression is worth the convenience of knowing why it
> failed.

Uhhh it's already very slow... so even slower doesn't matter anymore (HIG 
guideline maximum duration is 2 seconds, so we are way off anyhow).

So I'd say do it your way for now and report it upstream for the future.

Depending on whether we think it will fail more often than not we could also 
combine it: 
- first check the fast (40 s) path
- if it fails,
  - print "Signature could not be verified to be correct. We are checking which 
failed..." info message
  - check the slow (105 s) path

Do we think that failures are likely?

Also, git seems to invoke the gpg executable for each and every commit. It 
would be interesting whether gpg-interface.c verify_signed_buffer could be 
adapted to either invoke gpg once or to just use a library instead (gpgme ?). 

Long term we could also cache the checking result - I think that's something 
more difficult in the face of keys that expire. It would have to store at least 
the expiration date, the public key and the list of commit hashes that were 
checked and validated successfully.

pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Fri, Jan 13, 2017 at 10:24:00AM -0500, Leo Famulari wrote:
> I bet that you are using the new pre-push hook that verifies commit
> signatures, and you're trying to push some commits that fail the
> signature verification check.
> 
> Someone should add some error reporting to the hook.

In Git 2.11.0, it seems that `git verify-commit` can't tell the user
which commits failed verification:

https://git.kernel.org/cgit/git/git.git/tree/builtin/verify-commit.c?h=v2.11.0

With a warm cache and all the public keys on my machine, checking the
signature of all 17813 commits on the master branch takes ~40 seconds
with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push
hook does now.

Checking the commits one at a time takes ~105 seconds, using something
like this:

for commit in $(git rev-list HEAD); do
if ! git verify-commit $commit; then
echo $commit
fi
done

We could make the hook do something like that. Thoughts? I think the
performance regression is worth the convenience of knowing why it
failed.


signature.asc
Description: PGP signature