[bug#57909] Add link to 'pre-inst-env' from 'installing from git' docs

[bug#57909] Add link to 'pre-inst-env' from 'installing from git' docs

[Emma Turner via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: 0001-doc-link-pre-inst-env-from-building-from-git-docs.patch --]
[-- Type: text/x-patch, Size: 936 bytes --]

From 4849857fcf13f8de572d030cd15defd1f2b84768 Mon Sep 17 00:00:00 2001
From: Emma Turner <em.turner@tutanota.com>
Date: Sun, 18 Sep 2022 12:40:17 +0100
Subject: [PATCH] doc: link pre-inst-env from building from git docs

---
 doc/contributing.texi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 17a54f94cc..d4cd57141d 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -159,6 +159,14 @@ checkout by running:
 make authenticate
 @end example
 
+If you get an error of @code{guix: command not found}, then you can refer
+to the new instance you built above, by running the following
+(see @pxref{Running Guix Before It Is Installed}):
+
+@example
+./pre-inst-env make authenticate
+@end example
+
 The first run takes a couple of minutes, but subsequent runs are faster.
 
 Or, when your configuration for your local Git repository doesn't match
-- 
2.36.1

[bug#57910] [PATCH] Add link to 'pre-inst-env' from 'installing from git' docs

[Emma Turner via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0001-doc-link-pre-inst-env-from-building-from-git-docs.patch --]
[-- Type: text/x-patch, Size: 936 bytes --]

From 4849857fcf13f8de572d030cd15defd1f2b84768 Mon Sep 17 00:00:00 2001
From: Emma Turner <em.turner@tutanota.com>
Date: Sun, 18 Sep 2022 12:40:17 +0100
Subject: [PATCH] doc: link pre-inst-env from building from git docs

---
 doc/contributing.texi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 17a54f94cc..d4cd57141d 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -159,6 +159,14 @@ checkout by running:
 make authenticate
 @end example
 
+If you get an error of @code{guix: command not found}, then you can refer
+to the new instance you built above, by running the following
+(see @pxref{Running Guix Before It Is Installed}):
+
+@example
+./pre-inst-env make authenticate
+@end example
+
 The first run takes a couple of minutes, but subsequent runs are faster.
 
 Or, when your configuration for your local Git repository doesn't match
-- 
2.36.1

bug#57909: Sorry - accidentally opened duplicate issues

[Emma Turner via Guix-patches via]

This is a duplicate of https://issues.guix.gnu.org/57910

[bug#57910] [bug#57909] Add link to 'pre-inst-env' from 'installing from git' docs

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1367 bytes --]

merge 57909 57910
thanks

The given example "make authenticate" is insecure, it has a TOCTTOU 
problem as indicated at <https://issues.guix.gnu.org/22883#59>:

> Moreover, I don't think running 'make authenticate' after 'git pull'
> would really work -- after you pulled, git-authenticate could've been
> modified, so the verify-commit you did earlier doesn't apply anymore.

The solution that was proposed

 > We can solve it by removing ./pre-inst-env from the command in ‘make
 > authenticate’.

would be undone by the proposed patch.  Even then, it remains insecure, 
as an attacker could have modified the "make authenticate", as explained 
in more detail at <https://logs.guix.gnu.org/guix/2022-09-14.log#172610>.

As such, I think we really shouldn't recommend "make authenticate" (and 
even remove "make authenticate".  In fact, I think we should remove 
"make authenticate" and replace the instructions with a direct "guix git 
authenticate ...".

As such, I propose that:

   * you adjust the patch to note that authenticating the checkout is
     impossible if you don't already have Guix installed (instead of
     recommending the insecure "make authenticate")

   * I write a patch removing "make authenticate" and adjusting old uses
     of "make authenticate" to "guix git authenticate ...".

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57909] Add link to 'pre-inst-env' from 'installing from git' docs

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 513 bytes --]



On 18-09-2022 19:26, Maxime Devos wrote:
> [...]
> 
> As such, I propose that:
> 
>    * you adjust the patch to note that authenticating the checkout is
>      impossible if you don't already have Guix installed (instead of
>      recommending the insecure "make authenticate")
> 
>    * I write a patch removing "make authenticate" and adjusting old uses
>      of "make authenticate" to "guix git authenticate ...".


I have attached a patch for the latter.

Greetings,
Maxime.

[-- Attachment #1.1.2: 0001-WIP-Only-use-make-authenticate-for-etc-git-pre-push.patch --]
[-- Type: text/x-patch, Size: 4859 bytes --]

From a00ac3d016131f05c977e727f8ac15ea437aec7e Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sun, 18 Sep 2022 19:52:16 +0200
Subject: [PATCH] WIP Only use "make authenticate" for etc/git/pre-push.

As mentioned in <https://issues.guix.gnu.org/57909>, "make authenticate"
cannot be used for authentication, as it makes use of a Makefile.am
(and configure.ac) that might be provided by the attacker.

As such, only use this is the etc/git/pre-push hook, where it can be
reasonably assumed the current commit is 'safe' and it only needs
to check that the safety is properly conveyed to other people (by
having the commits be signed correctly).

To aid with the transition from "make authenticate" to "guix git
authenticate", print an error message from "make authenticate",
directing the user to use the safe "guix git authenticate" instead.

TODO missing: other uses of "make authenticate" in the documentation.

* Makefile.am (authenticate): Rename to ...
(authenticate-self-check): ... this, and add a new target
(authenticate): that depends on authenticate-self-check and additionally
prints the error message.
* doc/contributing.texi (Commit Access): Adjust for target rename.
* etc/git/pre-push: Adjust for target rename.
---
 Makefile.am           | 20 ++++++++++++++------
 doc/contributing.texi |  2 +-
 etc/git/pre-push      |  2 +-
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 22dcc43f99..bfabf0bf2e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -16,6 +16,7 @@
 # Copyright © 2019 Efraim Flashner <efraim@flashner.co.il>
 # Copyright © 2021 Chris Marusich <cmmarusich@gmail.com>
 # Copyright © 2021 Andrew Tropin <andrew@trop.in>
+# Copyright © 2022 Maxime Devos <maximedevos@telenet.be>
 #
 # This file is part of GNU Guix.
 #
@@ -804,12 +805,19 @@ 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:
+authentication_command = guix git authenticate '--keyring=$(GUIX_GIT_KEYRING)' --cache-key=channels/guix --stats '$(channel_intro_commit)' '$(channel_intro_signer)'
+authenticate-self-check:
 	$(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)"
+	$(authentication_command)
+authenticate: authenticate-self-check
+	$(AM_V_at)echo "\"make authenticate\" is insecure, you need to run"
+	$(AM_V_at)echo "$(authentication_command)"
+	$(AM_V_at)echo "instead.  Do **not** do that inside a ./pre-inst-env,"
+	$(AM_V_at)echo "that would be insecure because of a TOCTTOU problem."
+	$(AM_V_at)echo "Because of the TOCTTOU problem, you likely cannot trust"
+	$(AM_V_at)echo "these instructions unless you have already"
+	$(AM_V_at)echo "authenticated the repository by other means."
+	$(AM_V_at)exit 1
 
 # Assuming Guix is already installed and the daemon is up and running, this
 # rule builds from $(srcdir), creating and building derivations.
@@ -1076,7 +1084,7 @@ 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 authenticate authenticate-self-check
 .PHONY: update-guix-package update-NEWS cuirass-jobs release
 
 # Downloading up-to-date PO files.
diff --git a/doc/contributing.texi b/doc/contributing.texi
index de1d34cc03..353cb71caf 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -1740,7 +1740,7 @@ git config user.signingkey CABBA6EA1DC0FF33
 To check that commits are signed with correct key, use:
 
 @example
-make authenticate
+make authenticate-self-check
 @end example
 
 You can prevent yourself from accidentally pushing unsigned or signed
diff --git a/etc/git/pre-push b/etc/git/pre-push
index 59671b0d58..7fdc533d09 100755
--- a/etc/git/pre-push
+++ b/etc/git/pre-push
@@ -32,7 +32,7 @@ do
 		# Only use the hook when pushing to Savannah.
 		case "$2" in
 		    *.gnu.org*)
-			exec make authenticate check-channel-news
+			exec make authenticate-self-check check-channel-news
 			exit 127
 			;;
 		    *)

base-commit: 31a56967e2869c916b7a5e8ee570e8e10f0210a5
prerequisite-patch-id: 2712efb97bf33985fd0658e4dd8e936dc08be5fe
prerequisite-patch-id: 9d2409b480a8bff0fef029b4b095922d4957e06f
prerequisite-patch-id: 51a32abca3efec1ba67ead59b8694c5ea3129ad3
prerequisite-patch-id: 9092927761a340c07a99f5f3ed314a6add04cdee
prerequisite-patch-id: d0af09fbd5ee0ef60bdee53b87d729e46c1db2ca
prerequisite-patch-id: 4fee177b2d8c9478c6a7b8ce1ca9072942f39863
-- 
2.37.3


[-- Attachment #1.1.3: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57910] [bug#57909] Add link to 'pre-inst-env' from 'installing from git' docs

[Emma Turner via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

Hi Maxime,

Thanks for your replies.  I hadn't thought about the issue with make authenticate, but makes complete sense!  Thanks for explaining.

I've attached an updated patch, which I think does what you asked for (replacing make authenticate with guix git authenticate.

Many thanks,
Emma


Sep 18, 2022, 19:53 by maximedevos@telenet.be:

>
>
> On 18-09-2022 19:26, Maxime Devos wrote:
>
>> [...]
>>
>> As such, I propose that:
>>
>>    * you adjust the patch to note that authenticating the checkout is
>>      impossible if you don't already have Guix installed (instead of
>>      recommending the insecure "make authenticate")
>>
>>    * I write a patch removing "make authenticate" and adjusting old uses
>>      of "make authenticate" to "guix git authenticate ...".
>>
>
>
> I have attached a patch for the latter.
>
> Greetings,
> Maxime.
>


[-- Attachment #2: 0001-doc-contrib-recommend-guix-git-authenticate.patch --]
[-- Type: text/x-patch, Size: 2018 bytes --]

From f30716273f949d5883b7ced5c65ed5d48697479e Mon Sep 17 00:00:00 2001
From: Emma Turner <em.turner@tutanota.com>
Date: Mon, 19 Sep 2022 06:59:25 +0100
Subject: [PATCH] doc: contrib: recommend guix git authenticate

* doc/contributing.texi (Building from git): recommend use of guix git
authenticate on every git pull
---
 doc/contributing.texi | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 17a54f94cc..4fbef71fe7 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -152,29 +152,12 @@ If anything fails, take a look at installation instructions
 (@pxref{Installation}) or send a message to the
 @email{guix-devel@@gnu.org, mailing list}.
 
-From there on, you can authenticate all the commits included in your
-checkout by running:
-
-@example
-make authenticate
-@end example
-
-The first run takes a couple of minutes, but subsequent runs are faster.
-
-Or, 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
-example assumes that you have a Git remote called @samp{myremote}
-pointing to the official repository:
-
-@example
-make authenticate GUIX_GIT_KEYRING=myremote/keyring
-@end example
-
 @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.
+As with the initial clone, you are advised to run @command{guix git authenticate}
+after every @command{git pull} invocation.  This ensures you keep receiving valid
+changes to the repository (@pxref{Invoking guix git authenticate}).
+
+You will not be able to do this unless you already have Guix installed.
 @end quotation
 
 After updating the repository, @command{make} might fail with an error

base-commit: f7565101e49102da6f87ad92e4644ae7996515ab
-- 
2.36.1

[bug#57910] [PATCH] Add link to 'pre-inst-env' from 'installing from git' docs

[Emma Turner via Guix-patches via]

Hi,

Sorry, I'd tried to close 57909 as soon as I realised I'd made duplicate issues, but then it's only just come through, and closed both issues since they were merged.

I'll hopefully remember going forward to give debbugs lots of time!

Emma

[bug#57909] Sorry - accidentally opened duplicate issues

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 289 bytes --]

reopen 57909
thanks

Looks like the closing was accidental, reopening.

See <https://debbugs.gnu.org/server-control.html> if you're interested 
in how to use these debbugs commands (anyone can send those, somehow the 
wide permissions don't cause problems).

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57909] Add link to 'pre-inst-env' from 'installing from git' docs

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 567 bytes --]

On 19-09-2022 08:12, Emma Turner wrote:
> Hi Maxime,
> 
> Thanks for your replies.  I hadn't thought about the issue with make authenticate, but makes complete sense!  Thanks for explaining.
> 
> I've attached an updated patch, which I think does what you asked for (replacing make authenticate with guix git authenticate.


Aside from the copyright line in doc/guix.texi, looks good to me, though 
given the security concerns and the impact on a _large_ number of users, 
it would be best if someone else verified things as well.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57909] bug#57910: [PATCH] Add link to 'pre-inst-env' from 'installing from git' docs

[Ludovic Courtès]

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> As such, I think we really shouldn't recommend "make authenticate"
> (and even remove "make authenticate".  In fact, I think we should
> remove "make authenticate" and replace the instructions with a direct
> "guix git authenticate ...".

“make authenticate” runs ‘guix git authenticate’ with the right
parameters; importantly, it runs the already-installed ‘guix’, not the
one in the build tree, so it’s safe (prepending “./pre-inst-env”
wouldn’t be safe as you wrote).

So I’m not sure we really need changes; WDYT?

Ludo’.

[bug#57909] bug#57910: [PATCH] Add link to 'pre-inst-env' from 'installing from git' docs

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 2240 bytes --]



On 24-09-2022 17:58, Ludovic Courtès wrote:
> Hi,
> 
> Maxime Devos<maximedevos@telenet.be>  skribis:
> 
>> As such, I think we really shouldn't recommend "make authenticate"
>> (and even remove "make authenticate".  In fact, I think we should
>> remove "make authenticate" and replace the instructions with a direct
>> "guix git authenticate ...".
> “make authenticate” runs ‘guix git authenticate’ with the right
> parameters; importantly, it runs the already-installed ‘guix’, not the
> one in the build tree, so it’s safe (prepending “./pre-inst-env”
> wouldn’t be safe as you wrote).
> 
> So I’m not sure we really need changes; WDYT?

While ordinarily, it is true that "make authenticate" runs "guix git 
authenticate" (and not ./pre-inst-env guix git authenticate), an 
attacker could have modified Makefile.am to _not_ call "guix git 
authenticate", as I've explained in the paragraph above the one you quoted:

> The solution that was proposed [...].  __Even then, it remains
> insecure, as an attacker could have modified the "make authenticate",
> as explained in more detail at
> <https://logs.guix.gnu.org/guix/2022-09-14.log#172610>. 

More concretely, I've worked out a method the hypothetical attacker 
could use the fact that "Makefile.am" is used before it is authenticated 
in the message pointed to by the link I quoted:

https://logs.guix.gnu.org/guix/2022-09-14.log#172610 :

<maximed>civodul: Currently, it's like verifying the authenticity of a 
gnupg tarball, by extracting the gnupg tarball, compiling it, and 
running the freshly compiled gnupg tarball.
<antipode>Translated to Guix:
<antipode>(1) You run "git pull" (2) an attacker has intercepted the 
network connection and modified Makefile.am's authenticate target to 
always 'succeed'. Additionally, the attacker inserts some malicious code 
somewhere (e.g. some code in Makefile.am to upload your GnuPG keys to 
evil.com). To add some stealth, the modified Makefile.am automatically 
reverts the malicious commit. (3) You run "make authenticate" as 
recommended by the manual, and now the attacker has your private keys.

Do you see a flaw in this explanation?

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57909] bug#57910: [PATCH] Add link to 'pre-inst-env' from 'installing from git' docs

[Ludovic Courtès]

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> While ordinarily, it is true that "make authenticate" runs "guix git
> authenticate" (and not ./pre-inst-env guix git authenticate), an
> attacker could have modified Makefile.am to _not_ call "guix git
> authenticate", as I've explained in the paragraph above the one you
> quoted:

Oh you’re right; sorry for overlooking this.

So yes, that calls for recommending the full ‘guix git authenticate’
command for the initial checkout.

Thanks,
Ludo’.