GNU bug report logs - #70818
[PATCH] maint: Suggest ‘guix git authenticate’ for initial authentication.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 70818 in the body.
You can then email your comments to 70818 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: guix-patches <at> gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>, guix-security <at> gnu.org,
 Skyler Ferris <skyvine <at> protonmail.com>
Subject: [PATCH] maint: Suggest ‘guix git authenticate’ for initial authentication.
Date: Tue,  7 May 2024 16:13:27 +0200

The previous recommendation, running ‘make authenticate’, was insecure
because it led users to run code from the very repository they want to
authenticate:

  https://lists.gnu.org/archive/html/guix-devel/2024-04/msg00252.html

* Makefile.am (commit_v1_0_0, channel_intro_commit)
(channel_intro_signer, GUIX_GIT_KEYRING, authenticate): Remove.
* Makefile.am (.git/hooks/%): New target, generalization of previous
‘.git/hooks/pre-push’ target.
(nodist_noinst_DATA): Add ‘.git/hooks/post-merge’.
* doc/contributing.texi (Building from Git): Suggest ‘guix git
authenticate’ instead of ‘make authenticate’.
* etc/git/post-merge: New file.
* etc/git/pre-push: Run ‘guix git authenticate’ instead of ‘make
authenticate’.

Reported-by: Skyler Ferris <skyvine <at> protonmail.com>
Change-Id: Ia415aa8375013d0dd095e891116f6ce841d93efd
---
 Makefile.am           | 30 +++++++++---------------------
 doc/contributing.texi | 29 ++++++++++++++++++++++-------
 etc/git/post-merge    |  3 +++
 etc/git/pre-push      |  4 +++-
 4 files changed, 37 insertions(+), 29 deletions(-)
 create mode 100755 etc/git/post-merge

Hello there!

This addresses the security issue Skyler reported regarding
‘make authenticate’, basically removing the makefile target
and adjusting documentation accordingly.  It also adds a
‘post-merge’ hook like ‘guix git authenticate’ now does.

This assumes users have a (very) recent ‘guix git authenticate’
command, but I think that’s acceptable because this targets
an audience of developers.

Thoughts?

Ludo’.


diff --git a/Makefile.am b/Makefile.am
index 77c05ff63b7..d1d953b8923 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,5 +1,5 @@
 # GNU Guix --- Functional package management for GNU
-# Copyright © 2012-2023 Ludovic Courtès <ludo <at> gnu.org>
+# Copyright © 2012-2024 Ludovic Courtès <ludo <at> gnu.org>
 # Copyright © 2013 Andreas Enge <andreas <at> enge.fr>
 # Copyright © 2015, 2017 Alex Kost <alezost <at> gmail.com>
 # Copyright © 2016, 2018 Mathieu Lirzin <mthl <at> gnu.org>
@@ -895,22 +895,6 @@ $(guix_install_go_files): install-nobase_dist_guilemoduleDATA
 install-data-hook:
 	touch "$(DESTDIR)$(guileobjectdir)/guix/config.go"
 
-# Commit corresponding to the 'v1.0.0' tag.
-commit_v1_0_0 = 6298c3ffd9654d3231a6f25390b056483e8f407c
-
-# Introduction of the 'guix' channel.  Keep in sync with (guix channels)!
-channel_intro_commit = 9edb3f66fd807b096b48283debdcddccfea34bad
-channel_intro_signer = BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA
-
-# Authenticate the current Git checkout by checking signatures on every commit.
-GUIX_GIT_KEYRING = origin/keyring
-authenticate:
-	$(AM_V_at)echo "Authenticating Git checkout..." ;	\
-	guix git authenticate					\
-	    --keyring=$(GUIX_GIT_KEYRING)			\
-	    --cache-key=channels/guix --stats			\
-	    "$(channel_intro_commit)" "$(channel_intro_signer)"
-
 # Assuming Guix is already installed and the daemon is up and running, this
 # rule builds from $(srcdir), creating and building derivations.
 as-derivation:
@@ -1227,13 +1211,13 @@ cuirass-jobs: $(GOBJECTS)
 .PHONY: gen-ChangeLog gen-AUTHORS gen-tarball-version
 .PHONY: assert-no-store-file-names assert-binaries-available
 .PHONY: assert-final-inputs-self-contained check-channel-news
-.PHONY: clean-go make-go as-derivation authenticate
+.PHONY: clean-go make-go as-derivation
 .PHONY: update-guix-package update-NEWS cuirass-jobs release
 
 # Git auto-configuration.
-.git/hooks/pre-push: etc/git/pre-push
+.git/hooks/%: etc/git/%
 	$(AM_V_at)if test -d .git; then \
-	cp etc/git/pre-push .git/hooks/pre-push; \
+	cp "$<" "$@"; \
 	fi
 
 .git/config: etc/git/gitconfig
@@ -1256,7 +1240,11 @@ COMMIT_MSG_MAGIC = VGhpcyBpcyB0aGUgY29tbWl0LW1zZyBob29rIG9mIEd1aXg=
 # from a tarball.  Do not add dependencies on these to *_DATA when building
 # from a tarball, as that breaks the build.
 if in_git_p
-nodist_noinst_DATA = .git/hooks/pre-push .git/config .git/hooks/commit-msg
+nodist_noinst_DATA =				\
+  .git/hooks/pre-push				\
+  .git/hooks/post-merge				\
+  .git/config					\
+  .git/hooks/commit-msg
 endif
 
 # Downloading up-to-date PO files.
diff --git a/doc/contributing.texi b/doc/contributing.texi
index 66f4e86d0a9..0005c846dc1 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -276,25 +276,40 @@ Building from Git
 checkout by running:
 
 @example
-make authenticate
+guix git authenticate \
+  9edb3f66fd807b096b48283debdcddccfea34bad \
+  "BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA"
 @end example
 
 The first run takes a couple of minutes, but subsequent runs are faster.
+On subsequent runs, you can run the command without any arguments since
+the @dfn{introduction} (the commit ID and OpenPGP fingerprints above)
+will have been recorded <at> footnote{This requires a recent version of Guix,
+from May 2024 or more recent.}:
 
-Or, when your configuration for your local Git repository doesn't match
+@example
+guix git authenticate
+@end example
+
+When your configuration for your local Git repository doesn't match
 the default one, you can provide the reference for the @code{keyring}
-branch through the variable @code{GUIX_GIT_KEYRING}.  The following
+branch @i{via} the @option{-k} option.  The following
 example assumes that you have a Git remote called @samp{myremote}
 pointing to the official repository:
 
 @example
-make authenticate GUIX_GIT_KEYRING=myremote/keyring
+guix git authenticate \
+  -k myremote/keyring \
+  9edb3f66fd807b096b48283debdcddccfea34bad \
+  "BBB0 2DDF 2CEA F6A8 0D1D  E643 A2A0 6DF2 A33A 54FA"
 @end example
 
+@xref{Invoking guix git authenticate}, for more information on this
+command.
+
 @quotation Note
-You are advised to run @command{make authenticate} after every
-@command{git pull} invocation.  This ensures you keep receiving valid
-changes to the repository.
+By default, hooks installed such that @command{guix git authenticate} is
+invoked anytime you run @command{git pull} or @command{git push}.
 @end quotation
 
 After updating the repository, @command{make} might fail with an error
diff --git a/etc/git/post-merge b/etc/git/post-merge
new file mode 100755
index 00000000000..f2ad37d35c4
--- /dev/null
+++ b/etc/git/post-merge
@@ -0,0 +1,3 @@
+#!/bin/sh
+# Authenticate the repo upon 'git pull' and similar.
+exec guix git authenticate
diff --git a/etc/git/pre-push b/etc/git/pre-push
index 59671b0d583..325b23854bb 100755
--- a/etc/git/pre-push
+++ b/etc/git/pre-push
@@ -32,7 +32,9 @@ do
 		# Only use the hook when pushing to Savannah.
 		case "$2" in
 		    *.gnu.org*)
-			exec make authenticate check-channel-news
+			set -e
+			make check-channel-news
+			exec guix git authenticate
 			exit 127
 			;;
 		    *)

base-commit: 014875b29e68da6357a5323e6dd1eaa74a05b753
-- 
2.41.0

Message #8 received at 70818 <at> debbugs.gnu.org (full text, mbox):

From: "pelzflorian (Florian Pelz)" <pelzflorian <at> pelzflorian.de>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 70818 <at> debbugs.gnu.org, Skyler Ferris <skyvine <at> protonmail.com>,
 guix-security <at> gnu.org
Subject: Re: [bug#70818] [PATCH] maint: Suggest ‘guix
 git authenticate’ for initial authentication.
Date: Tue, 14 May 2024 12:08:42 +0200

Thank you Ludo for this careful implementation.  I find One typo:

Ludovic Courtès <ludo <at> gnu.org> writes:
> diff --git a/doc/contributing.texi b/doc/contributing.texi
> index 66f4e86d0a9..0005c846dc1 100644
> --- a/doc/contributing.texi
> +++ b/doc/contributing.texi
> @@ -276,25 +276,40 @@ Building from Git
> […]
>  @quotation Note
> -You are advised to run @command{make authenticate} after every
> -@command{git pull} invocation.  This ensures you keep receiving valid
> -changes to the repository.
> +By default, hooks installed such that @command{guix git authenticate} is
> +invoked anytime you run @command{git pull} or @command{git push}.
>  @end quotation
>  
>  After updating the repository, @command{make} might fail with an error

By default, hooks *are*

Regards,
Florian

Message #11 received at 70818 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 70818 <at> debbugs.gnu.org, Florian Pelz <pelzflorian <at> pelzflorian.de>,
 Skyler Ferris <skyvine <at> protonmail.com>, guix-security <at> gnu.org
Subject: Re: [bug#70818] [PATCH] maint: Suggest ‘guix
 git authenticate’ for initial authentication.
Date: Wed, 22 May 2024 20:55:16 -0400

Hi!

Ludovic Courtès <ludo <at> gnu.org> writes:

> The previous recommendation, running ‘make authenticate’, was insecure
> because it led users to run code from the very repository they want to
> authenticate:
>
>   https://lists.gnu.org/archive/html/guix-devel/2024-04/msg00252.html
>
> * Makefile.am (commit_v1_0_0, channel_intro_commit)
> (channel_intro_signer, GUIX_GIT_KEYRING, authenticate): Remove.
> * Makefile.am (.git/hooks/%): New target, generalization of previous
> ‘.git/hooks/pre-push’ target.
> (nodist_noinst_DATA): Add ‘.git/hooks/post-merge’.
> * doc/contributing.texi (Building from Git): Suggest ‘guix git
> authenticate’ instead of ‘make authenticate’.
> * etc/git/post-merge: New file.
> * etc/git/pre-push: Run ‘guix git authenticate’ instead of ‘make
> authenticate’.
>
> Reported-by: Skyler Ferris <skyvine <at> protonmail.com>
> Change-Id: Ia415aa8375013d0dd095e891116f6ce841d93efd

Reviewed-by: Maxim Cournoyer <maxim.cournoyer <at> gmail>

(taking into account the typo spotted by Florian).

Thank you for addressing this!

-- 
Thanks,
Maxim

Message #16 received at 70818-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: guix-security <at> gnu.org, Skyler Ferris <skyvine <at> protonmail.com>,
 70818-done <at> debbugs.gnu.org, Florian Pelz <pelzflorian <at> pelzflorian.de>
Subject: Re: [bug#70818] [PATCH] maint: Suggest ‘guix
 git authenticate’ for initial authentication.
Date: Sat, 25 May 2024 16:24:38 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> The previous recommendation, running ‘make authenticate’, was insecure
>> because it led users to run code from the very repository they want to
>> authenticate:
>>
>>   https://lists.gnu.org/archive/html/guix-devel/2024-04/msg00252.html
>>
>> * Makefile.am (commit_v1_0_0, channel_intro_commit)
>> (channel_intro_signer, GUIX_GIT_KEYRING, authenticate): Remove.
>> * Makefile.am (.git/hooks/%): New target, generalization of previous
>> ‘.git/hooks/pre-push’ target.
>> (nodist_noinst_DATA): Add ‘.git/hooks/post-merge’.
>> * doc/contributing.texi (Building from Git): Suggest ‘guix git
>> authenticate’ instead of ‘make authenticate’.
>> * etc/git/post-merge: New file.
>> * etc/git/pre-push: Run ‘guix git authenticate’ instead of ‘make
>> authenticate’.
>>
>> Reported-by: Skyler Ferris <skyvine <at> protonmail.com>
>> Change-Id: Ia415aa8375013d0dd095e891116f6ce841d93efd
>
> Reviewed-by: Maxim Cournoyer <maxim.cournoyer <at> gmail>
>
> (taking into account the typo spotted by Florian).

I fixed the typo and applied it, thanks!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.