[PATCH] maint: Suggest ‘guix git authenticate’ for initial authentication.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • pelzflorian (Florian Pelz)
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
normal
L
L
Ludovic Courtès wrote on 7 May 16:13 +0200
[PATCH] maint: Suggest ‘guix git authen ticate’ for initial authentication.
(address . guix-patches@gnu.org)
35c1e4eead584b9e24d3efc3638d6da4fd24cf8d.1715091056.git.ludo@gnu.org
The previous recommendation, running ‘make authenticate’, was insecure
because it led users to run code from the very repository they want to
authenticate:


* 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@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’.


Toggle diff (144 lines)
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@gnu.org>
+# Copyright © 2012-2024 Ludovic Courtès <ludo@gnu.org>
# Copyright © 2013 Andreas Enge <andreas@enge.fr>
# Copyright © 2015, 2017 Alex Kost <alezost@gmail.com>
# Copyright © 2016, 2018 Mathieu Lirzin <mthl@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@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
P
P
pelzflorian (Florian Pelz) wrote on 14 May 12:08 +0200
Re: [bug#70818] [PATCH] maint: Suggest ‘guix git authenticate’ for initial authentication.
(name . Ludovic Courtès)(address . ludo@gnu.org)
87cyposo2d.fsf@pelzflorian.de
Thank you Ludo for this careful implementation. I find One typo:

Ludovic Courtès <ludo@gnu.org> writes:
Toggle quote (16 lines)
> 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
M
M
Maxim Cournoyer wrote on 23 May 02:55 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
877cfl2vpn.fsf@gmail.com
Hi!

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

Toggle quote (20 lines)
> 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@protonmail.com>
> Change-Id: Ia415aa8375013d0dd095e891116f6ce841d93efd

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

(taking into account the typo spotted by Florian).

Thank you for addressing this!

--
Thanks,
Maxim
L
L
Ludovic Courtès wrote on 25 May 16:24 +0200
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87wmniyno9.fsf@gnu.org
Hi,

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

Toggle quote (26 lines)
> Ludovic Courtès <ludo@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@protonmail.com>
>> Change-Id: Ia415aa8375013d0dd095e891116f6ce841d93efd
>
> Reviewed-by: Maxim Cournoyer <maxim.cournoyer@gmail>
>
> (taking into account the typo spotted by Florian).

I fixed the typo and applied it, thanks!

Ludo’.
Closed
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 70818@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 70818
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch