Re: Updating the “pre-push” Git hook
On Mon, May 25, 2020 at 01:13:16PM -0700, Vagrant Cascadian wrote: > Wait a minute... you're saying this is something that needs to be > configured on each committer's machine(s)? Yes, it was recommended in HACKING and then, after we removed that file, in the Commit Access section of the manual. We try to remind new committers to read these files but maybe we forgot for you. However, the security model does not depend on either it or on a post-receive server hook. It's simply to prevent mistakes. There are commits in the repo that are unsigned when they should have been signed, and even one commit that is signed but that fails the signature check :/ A simple pre-push hook would have caught that. signature.asc Description: PGP signature
Re: Updating the “pre-push” Git hook
Hi,
Ricardo Wurmus skribis:
> Ludovic Courtès writes:
>
>> Could you try:
>>
>> mv ~/.cache/guix/authentication/channels/guix{,.bak}
>> time make authenticate
>> mv ~/.cache/guix/authentication/channels/guix{.bak,}
>>
>> ?
>
> real 0m49.496s
> user 0m43.733s
> sys 0m1.658s
Same timing if you run:
make guix/{git,openpgp}.go
beforehand?
I get 29s for 16865 commits.
> And then running it again:
>
> $ [env] time make authenticate
> Authenticating Git checkout...
> Authenticating d68de95 to fb1675e (0 commits)...
>
> real 0m2.692s
> user 0m2.877s
> sys 0m0.128s
Oh I see that too. Roughly half of the time seems to be spent loading
the keyring from the ‘keyring’ branch, and the other half is traversing
the commit graph:
--8<---cut here---start->8---
$ ./pre-inst-env guile --no-auto-compile -e git-authenticate
./build-aux/git-authenticate.scm d68de958b60426798ed62797ff7c96c327a672ac $(git
rev-parse HEAD)
Authenticating d68de95 to a1a3bd5 (0 commits)...
% cumulative self
time seconds seconds procedure
37.04 0.73 0.73 anon #x4a44d0
12.96 0.25 0.25 anon #x49e788
11.11 0.22 0.22 guix/openpgp.scm:1030:0:crc24
6.48 0.16 0.13 anon #x4a2810
4.63 0.09 0.09 ice-9/popen.scm:145:0:reap-pipes
3.70 0.07 0.07 ice-9/vlist.scm:539:0:vhash-assq
3.70 0.07 0.07 anon #x497578
2.78 0.05 0.05 anon #x4a52c0
2.78 0.05 0.05 anon #x4a27e0
2.78 0.05 0.05 anon #x49d9e0
1.85 0.05 0.04 anon #x4a5494
1.85 0.05 0.04 anon #x497190
0.93 0.62 0.02 guix/openpgp.scm:1056:0:read-radix-64
0.93 0.33 0.02 gcrypt/base64.scm:154:2:base64-decode
0.93 0.04 0.02 anon #x4a2878
0.93 0.02 0.02 anon #x4a2840
0.93 0.02 0.02 anon #x49f928
0.93 0.02 0.02 git/commit.scm:213:0:commit-parents
0.93 0.02 0.02 anon #x4a1ab8
0.93 0.02 0.02 anon #x4975a8
0.93 0.02 0.02 anon #x5f6210
0.00 1.96 0.00
/home/ludo/src/guix/build-aux/git-authenticate.scm:430:3
0.00 1.15 0.00 guix/git.scm:387:0:commit-closure
0.00 1.15 0.00 guix/git.scm:401:0:commit-difference
0.00 1.05 0.00 srfi/srfi-1.scm:530:0:unfold
0.00 0.96 0.00 git/commit.scm:198:4
0.00 0.82 0.00 srfi/srfi-1.scm:452:2:fold
0.00 0.82 0.00
/home/ludo/src/guix/build-aux/git-authenticate.scm:356:0:authenticate-commits
0.00 0.82 0.00 guix/progress.scm:65:0:call-with-progress-reporter
0.00 0.73 0.00 git/commit.scm:197:14
0.00 0.62 0.00
/home/ludo/src/guix/build-aux/git-authenticate.scm:346:10
0.00 0.20 0.00 guix/openpgp.scm:987:0:get-openpgp-keyring
0.00 0.15 0.00 guix/openpgp.scm:610:0:get-signature
0.00 0.11 0.00 ice-9/format.scm:39:0:format
0.00 0.09 0.00 anon #x495690
0.00 0.09 0.00 anon #x497d54
0.00 0.09 0.00 git/types.scm:83:0
0.00 0.07 0.00 guix/openpgp.scm:614:2:get-sig
0.00 0.07 0.00 gcrypt/pk-crypto.scm:103:4
0.00 0.05 0.00 ice-9/rdelim.scm:193:0:read-line
0.00 0.05 0.00 guix/openpgp.scm:845:0:get-public-key
0.00 0.05 0.00 ice-9/format.scm:759:2:format:out-obj-padded
0.00 0.05 0.00 ice-9/format.scm:113:2:format:format-work
0.00 0.04 0.00 guix/sets.scm:84:0:set-insert
0.00 0.02 0.00 git/types.scm:106:0:make-double-pointer
0.00 0.02 0.00 ice-9/ports.scm:545:0:call-with-output-string
---
Sample count: 108
Total time: 1.963880152 seconds (0.382282758 seconds in GC)
--8<---cut here---end--->8---
We can make that a bit faster by not loading the keyring when there’s
nothing to do, and by storing keys in binary format instead of
ASCII-armored, if needed.
Thanks,
Ludo’.
Re: Updating the “pre-push” Git hook
Hi! Vagrant Cascadian skribis: > On 2020-05-24, Ludovic Courtès wrote: >> Efraim Flashner skribis: >>> On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: Hello Guix! I think we should change our pre-push hook as shown below. Thoughts? > ... >>> (ins)efraim@E5400 ~$ type -P make >>> (ins)efraim@E5400 ~$ command -v make >>> >>> I'd need to run 'guix environment --ad-hoc make -- git push' >> >> You’d need to run ‘git push’ from a full Guix development environment. >> Do you think it could be a problem? > > Wait a minute... you're saying this is something that needs to be > configured on each committer's machine(s)? > > Shouldn't it be on the server-side recieve hooks instead, otherwise > someone might accidentally (or intentially) push commits not > appropriately signed to the repository or validated by this check... > > Or is this an optional check for recommended for committers? Have I been > missing something all along that I was supposed to be doing? It should be a server-side check so we don’t shoot ourselves in the foot. However, it’s not done yet (but hey, the code is not even a month old :-)), so in the meantime, this hook will be very strongly recommended. Making this a server-side hook on Savannah will be challenging since “we” don’t have direct access to Savannah. That makes me wonder if we should have a push server say on berlin, and make Savannah mirror it or something. Help welcome! > For my own workflow, I usually do not (yet) sign or push commits from a > machine with guix installed... it's a bit awkward, admittedly, but I > don't yet have any SSH or OpenPGP keys I trust guix with directly > (ironically, "make authenticate" is working towards addressing exactly > that trust issue). Heh. :-) Thanks, Ludo’.
Re: Updating the “pre-push” Git hook
Efraim Flashner skribis: > I'd probably run 'guix environment guix -- git push origin master' and > view it as an additional safe guard to not push to the wrong branch or > something, similar to how I view the password on the key. I bet there's > an option to create a repo-specific alias in .git/config so that 'git > push' will run inside 'guix environment guix'. OK. > I'm not convinced that my case is unique or that it should hold back the > change. How does it work if 'make authenticate' fails? ‘git push’ fails, just like with the current pre-push hook. Ludo’.
Re: Updating the “pre-push” Git hook
On 2020-05-24, Ludovic Courtès wrote: > Efraim Flashner skribis: >> On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: >>> Hello Guix! >>> >>> I think we should change our pre-push hook as shown below. >>> >>> Thoughts? ... >> (ins)efraim@E5400 ~$ type -P make >> (ins)efraim@E5400 ~$ command -v make >> >> I'd need to run 'guix environment --ad-hoc make -- git push' > > You’d need to run ‘git push’ from a full Guix development environment. > Do you think it could be a problem? Wait a minute... you're saying this is something that needs to be configured on each committer's machine(s)? Shouldn't it be on the server-side recieve hooks instead, otherwise someone might accidentally (or intentially) push commits not appropriately signed to the repository or validated by this check... Or is this an optional check for recommended for committers? Have I been missing something all along that I was supposed to be doing? For my own workflow, I usually do not (yet) sign or push commits from a machine with guix installed... it's a bit awkward, admittedly, but I don't yet have any SSH or OpenPGP keys I trust guix with directly (ironically, "make authenticate" is working towards addressing exactly that trust issue). live well, vagrant signature.asc Description: PGP signature
Re: Updating the “pre-push” Git hook
Ludovic Courtès writes:
> Could you try:
>
> mv ~/.cache/guix/authentication/channels/guix{,.bak}
> time make authenticate
> mv ~/.cache/guix/authentication/channels/guix{.bak,}
>
> ?
real0m49.496s
user0m43.733s
sys 0m1.658s
And then running it again:
--8<---cut here---start->8---
$ [env] time make authenticate
Authenticating Git checkout...
Authenticating d68de95 to fb1675e (0 commits)...
real0m2.692s
user0m2.877s
sys 0m0.128s
$ [env] time make authenticate
Authenticating Git checkout...
Authenticating d68de95 to fb1675e (0 commits)...
real0m2.754s
user0m2.939s
sys 0m0.111s
--8<---cut here---end--->8---
--
Ricardo
Re: Updating the “pre-push” Git hook
On Sun, May 24, 2020 at 11:45:34PM +0200, Ludovic Courtès wrote: > Hi, > > Efraim Flashner skribis: > > > On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: > >> Hello Guix! > >> > >> I think we should change our pre-push hook as shown below. > >> > >> Thoughts? > >> > >> Thanks, > >> Ludo’. > >> > > > > (ins)efraim@E5400 ~$ type -P make > > (ins)efraim@E5400 ~$ command -v make > > > > I'd need to run 'guix environment --ad-hoc make -- git push' > > You’d need to run ‘git push’ from a full Guix development environment. > Do you think it could be a problem? I'd probably run 'guix environment guix -- git push origin master' and view it as an additional safe guard to not push to the wrong branch or something, similar to how I view the password on the key. I bet there's an option to create a repo-specific alias in .git/config so that 'git push' will run inside 'guix environment guix'. I'm not convinced that my case is unique or that it should hold back the change. How does it work if 'make authenticate' fails? -- Efraim Flashner אפרים פלשנר GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351 Confidentiality cannot be guaranteed on emails sent or received unencrypted signature.asc Description: PGP signature
Re: Updating the “pre-push” Git hook
Hi, Efraim Flashner skribis: > On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: >> Hello Guix! >> >> I think we should change our pre-push hook as shown below. >> >> Thoughts? >> >> Thanks, >> Ludo’. >> > > (ins)efraim@E5400 ~$ type -P make > (ins)efraim@E5400 ~$ command -v make > > I'd need to run 'guix environment --ad-hoc make -- git push' You’d need to run ‘git push’ from a full Guix development environment. Do you think it could be a problem? Thanks, Ludo’.
Re: Updating the “pre-push” Git hook
Hi,
Ricardo Wurmus skribis:
> Leo Famulari writes:
>
>> On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote:
>>> Hello Guix!
>>>
>>> I think we should change our pre-push hook as shown below.
>>>
>>> Thoughts?
>>
>> Is it fast? :)
>
> This depends on how many commits you have previously authenticated. But
> even the slow case is pretty fast. I just authenticated 16,290 commits
> in less than a minute. Regular contributors won’t have to authenticate
> nearly as many commits.
>
> For 0 commits it takes 4 seconds.
Hmm for me it’s more like 0 to 1 second, and it’s ~20s to authenticate
14K commits:
https://issues.guix.gnu.org/issue/22883#61
Could you try:
mv ~/.cache/guix/authentication/channels/guix{,.bak}
time make authenticate
mv ~/.cache/guix/authentication/channels/guix{.bak,}
?
Anyway, for normal usage, it should be faster than the shell script.
Also, it performs a different job: the shell script would only check
whether a commit is signed at all.
Well, give it a try and lemme know!
Ludo’.
Re: Updating the “pre-push” Git hook
Leo Famulari writes: > On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: >> Hello Guix! >> >> I think we should change our pre-push hook as shown below. >> >> Thoughts? > > Is it fast? :) This depends on how many commits you have previously authenticated. But even the slow case is pretty fast. I just authenticated 16,290 commits in less than a minute. Regular contributors won’t have to authenticate nearly as many commits. For 0 commits it takes 4 seconds. -- Ricardo
Re: Updating the “pre-push” Git hook
On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: > Hello Guix! > > I think we should change our pre-push hook as shown below. > > Thoughts? > > Thanks, > Ludo’. > (ins)efraim@E5400 ~$ type -P make (ins)efraim@E5400 ~$ command -v make I'd need to run 'guix environment --ad-hoc make -- git push' -- Efraim Flashner אפרים פלשנר GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351 Confidentiality cannot be guaranteed on emails sent or received unencrypted signature.asc Description: PGP signature
Re: Updating the “pre-push” Git hook
On Fri, May 22, 2020 at 10:44:48PM +0200, Ludovic Courtès wrote: > Hello Guix! > > I think we should change our pre-push hook as shown below. > > Thoughts? Is it fast? :)
Updating the “pre-push” Git hook
Hello Guix! I think we should change our pre-push hook as shown below. Thoughts? Thanks, Ludo’. diff --git a/etc/git/pre-push b/etc/git/pre-push index 9206a2dfe5..415345fc75 100755 --- a/etc/git/pre-push +++ b/etc/git/pre-push @@ -1,7 +1,8 @@ #!/bin/sh # This hook script prevents the user from pushing to Savannah if any of the new -# commits' OpenPGP signatures cannot be verified. +# commits' OpenPGP signatures cannot be verified, or if a commit is signed +# with an unauthorized key. # Called by "git push" after it has checked the remote status, but before # anything has been pushed. If this script exits with a non-zero status nothing @@ -19,51 +20,13 @@ # # -z40= - # Only use the hook when pushing to Savannah. case "$2" in -*git.sv.gnu.org*) - break +*.gnu.org*) + exec make authenticate check-channel-news + exit 127 ;; -*) +*) exit 0 ;; esac - -while read local_ref local_sha remote_ref remote_sha -do - if [ "$local_sha" = $z40 ] - then - # Handle delete - : - else - if [ "$remote_sha" = $z40 ] - then - # 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. - 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 - -exit 0
