[PATCH] guix: git-authenticate: Also authenticate the channel intro commit.

message to point to the relevant part of the manual.

(authenticate-repository): Explicitly authenticate the channel introduction

commit, so that it's also rejected unless it is signed by an authorized

key. Otherwise only the second commit would yield an error, which

is confusing.

---

here's how i tested this:

i set up pulling from a local checkout of guix.

in that branch i created a signed dummy commit, and added it as a channel

introduction, replacing guix in my /etc/guix/channels.scm. then tried to

guix pull, which worked.

then i added another dummy commit, which resulted in an error when pulling.

then i reset the branch back to only contain the first commit, and added

this code that then resulted in an error even with a single commit.

i have encountered it while i was trying to set up my local checkout to

test my patches on my live guix, and i was utterly confused why my commit

was rejected as unauthenticated (i misunderstood how git-authenticate

works).

guix/git-authenticate.scm | 11 ++++++++---

1 file changed, 8 insertions(+), 3 deletions(-)

severity 50814 grave

On Sun, Sep 26, 2021 at 12:19:29PM +0200, Attila Lendvai wrote:

Thanks for your report.

I've marked the severity as "grave", which in Debbugs parlance means

"makes the package in question unusable or mostly so, or causes data

loss, or introduces a security hole allowing access to the accounts of

users who use the package."

I'm not sure if that's justified or not but this patch should be

prioritized.

Attila Lendvai schreef op zo 26-09-2021 om 12:19 [+0200]:

Could you add a test to tests/git-authenticate.scm, verifying the right comit

is reported? (Maybe use unauthorized-commit-error?, guard and

authenticate-repository.)

I'm not sure explicitely validating the start commit is sufficient. What happens

in the following scenario:

(Order of commits)

0. start commit

1. valid (already authenticated?) commit

2. invalid commit

3. invalid commit

Is commit 2 reported, or commit 3 reported? I think commit 2 should be reported,

but from your messages on IRC, I think you saw commit 3 being reported?

Greetings,

Maxime.

just a quick update that i'm working on putting together an extensive test for this, and a fix.

- attila

PGP: 5D5F 45C7 DFCD 0A39

what should happen if a channel-introduction commit does not update the .guix-authorizations file?

this means that this single commit will get through the authentication (channel intro commits are always trusted), but any later commits signed with the same key will be rejected.

this is the situation that got me confused, and thrust me into attempting to fix this (trying to `guix pull` from a local git checkout of guix containing my patches).

i wrote the code for this, but i don't know what should be its UI. how should this be reported to the user?

using `(warning ...)` will just print something to stderr.

i was hoping to raise a continuable condition of type &warning, that i can even check for in the tests, but i have failed to put that together. the scheme/guile condition system is a bit messy/convoluted.

can someone help me out with a hint/outline about how to report this that best fits the rest of guix?

note that it's not really an error, because until another commit is added with the new key, this channel is valid.

- attila

PGP: 5D5F 45C7 DFCD 0A39

a nested way under a with-temporary-git-repository.

(populate-git-repository): Extend the DSL with (add "some-noise"), (reset

"[commit hash]"), (checkout "branch" orphan).

---

Ready for merging modulo possible feedback, and one TODO

regarding the UI of giving feedback to the user.

guix/tests/git.scm | 23 +++++++++++++++++++++--

guix/tests/gnupg.scm | 8 ++++++--

2 files changed, 27 insertions(+), 4 deletions(-)

The third key will be used in an upcoming commit.

Rename public keys to .pub.

(%ed25519-3-secret-key-file): New variable.

(%ed25519-2-public-key-file): Renamed from %ed25519bis-public-key-file.

(%ed25519-2-secret-key-file): Renamed from %ed25519bis-secret-key-file.

---

Makefile.am | 20 +++++-----

build-aux/test-env.in | 6 +--

guix/tests/gnupg.scm | 22 ++++++----

tests/channels.scm | 18 ++++-----

tests/git-authenticate.scm | 23 +++++------

tests/guix-authenticate.sh | 4 +-

tests/{civodul.key => keys/civodul.pub} | 0

tests/{dsa.key => keys/dsa.pub} | 0

tests/{ed25519bis.key => keys/ed25519-2.pub} | 0

tests/{ed25519bis.sec => keys/ed25519-2.sec} | 0

tests/keys/ed25519-3.pub | 9 +++++

tests/keys/ed25519-3.sec | 10 +++++

tests/{ed25519.key => keys/ed25519.pub} | 0

tests/{ => keys}/ed25519.sec | 0

tests/{rsa.key => keys/rsa.pub} | 0

tests/{ => keys}/signing-key.pub | 0

tests/{ => keys}/signing-key.sec | 0

tests/openpgp.scm | 42 +++++++++++---------

18 files changed, 93 insertions(+), 61 deletions(-)

rename tests/{civodul.key => keys/civodul.pub} (100%)

rename tests/{dsa.key => keys/dsa.pub} (100%)

rename tests/{ed25519bis.key => keys/ed25519-2.pub} (100%)

rename tests/{ed25519bis.sec => keys/ed25519-2.sec} (100%)

create mode 100644 tests/keys/ed25519-3.pub

create mode 100644 tests/keys/ed25519-3.sec

rename tests/{ed25519.key => keys/ed25519.pub} (100%)

rename tests/{ => keys}/ed25519.sec (100%)

rename tests/{rsa.key => keys/rsa.pub} (100%)

rename tests/{ => keys}/signing-key.pub (100%)

rename tests/{ => keys}/signing-key.sec (100%)

Will be fixed in a subsequent commit.

channel-introduction".

---

tests/git-authenticate.scm | 111 +++++++++++++++++++++++++++++++++++++

1 file changed, 111 insertions(+)

Always verify the channel introduction commit, so that no commit can slip

through that was signed with a different key.

Always update the cache, because it affects the behavior of later calls.

Warn when a channel introduction commit doesn't also update the

'.guix-authentications' file.

message to point to the relevant part of the manual.

(authenticate-repository): Eliminate optimizations to make the code path less

dependent on the input. Always trust the intro-commit itself. Always call

verify-introductory-commit.

(verify-introductory-commit): Check if the commit contains the key that was

used to sign it, and issue a warning otherwise. This is to avoid the confusion

caused by only the *second* commit yielding an error, because intro-commits

are always trusted.

(authenticate-commit): Clarify error message.

(authorized-keys-at-commit): Factored out to the toplevel from

commit-authorized-keys.

---

guix/channels.scm | 4 +-

guix/git-authenticate.scm | 153 ++++++++++++++++++++++----------------

2 files changed, 91 insertions(+), 66 deletions(-)

Attila Lendvai schreef op ma 27-09-2021 om 18:45 [+0000]:

[...]

Technically, Scheme supports continuable exceptions with 'raise-continuable'

and with-exception-hander. E.g., the following Racket example:

(use-modules (rnrs exceptions) (rnrs conditions))

(with-exception-handler

(lambda (con)

(cond

((not (warning? con))

(raise con))

((message-condition? con)

(display (condition-message con)))

(else

(display "a warning has been issued")))

42)

(lambda ()

(+ (raise-continuable

(condition

(make-warning)

(make-message-condition

"should be a number")))

23)))

prints: should be a number

⇒ 65

(from https://docs.racket-lang.org/r6rs/r6rs-lib-std/r6rs-lib-Z-H-8.html#node_idx_378)

works in Guile

You might need to modify 'call-with-error-handling' in (guix ui) to recognise

&warning though, such that the &warning exception will be properly handled.

Alternatively, you can use the procedure 'warning' from (guix diagnostics).

To detect the error in this case, you can use parameterise, guix-warning-port

and procedures like call-with-output-string. You may need to reset the locale

first (with dynamic-wind?).

Greetings,

Maxime.

a nested way under a with-temporary-git-repository.

(populate-git-repository): Extend the DSL with (add "some-noise"), (reset

"[commit hash]"), (checkout "branch" orphan).

---

guix/tests/git.scm | 23 +++++++++++++++++++++--

guix/tests/gnupg.scm | 8 ++++++--

2 files changed, 27 insertions(+), 4 deletions(-)

Will be fixed in a subsequent commit.

channel-introduction".

---

tests/git-authenticate.scm | 132 +++++++++++++++++++++++++++++++++++++

1 file changed, 132 insertions(+)

The third key will be used in an upcoming commit.

Rename public keys to .pub.

(%ed25519-3-secret-key-file): New variable.

(%ed25519-2-public-key-file): Renamed from %ed25519bis-public-key-file.

(%ed25519-2-secret-key-file): Renamed from %ed25519bis-secret-key-file.

---

Makefile.am | 20 +++++-----

build-aux/test-env.in | 6 +--

guix/tests/gnupg.scm | 22 ++++++----

tests/channels.scm | 18 ++++-----

tests/git-authenticate.scm | 23 +++++------

tests/guix-authenticate.sh | 4 +-

tests/{civodul.key => keys/civodul.pub} | 0

tests/{dsa.key => keys/dsa.pub} | 0

tests/{ed25519bis.key => keys/ed25519-2.pub} | 0

tests/{ed25519bis.sec => keys/ed25519-2.sec} | 0

tests/keys/ed25519-3.pub | 9 +++++

tests/keys/ed25519-3.sec | 10 +++++

tests/{ed25519.key => keys/ed25519.pub} | 0

tests/{ => keys}/ed25519.sec | 0

tests/{rsa.key => keys/rsa.pub} | 0

tests/{ => keys}/signing-key.pub | 0

tests/{ => keys}/signing-key.sec | 0

tests/openpgp.scm | 42 +++++++++++---------

18 files changed, 93 insertions(+), 61 deletions(-)

rename tests/{civodul.key => keys/civodul.pub} (100%)

rename tests/{dsa.key => keys/dsa.pub} (100%)

rename tests/{ed25519bis.key => keys/ed25519-2.pub} (100%)

rename tests/{ed25519bis.sec => keys/ed25519-2.sec} (100%)

create mode 100644 tests/keys/ed25519-3.pub

create mode 100644 tests/keys/ed25519-3.sec

rename tests/{ed25519.key => keys/ed25519.pub} (100%)

rename tests/{ => keys}/ed25519.sec (100%)

rename tests/{rsa.key => keys/rsa.pub} (100%)

rename tests/{ => keys}/signing-key.pub (100%)

rename tests/{ => keys}/signing-key.sec (100%)

exceptions are not broken by being re-raised as non-continuable. This is

needed for a later commit that uses continuable exceptions from within

git-authenticate to signal warnings to the user. The reason for this is that

this way tests can explicitly check that a warning was signalled in certain

situations.

printing them to the user, and then continuing at the place they were

signalled at.

function.

---

guix/diagnostics.scm | 4 ++++

guix/store.scm | 16 ++++++++++------

guix/ui.scm | 11 ++++++++++-

3 files changed, 24 insertions(+), 7 deletions(-)

Always verify the channel introduction commit, so that no commit can slip

through that was signed with a different key.

Always update the cache, because it affects the behavior of later calls.

Signal a continuable compound-condition (with type &warning included) when a

channel introduction commit doesn't also update the '.guix-authentications'

file.

message to point to the relevant part of the manual.

(authenticate-repository): Eliminate optimizations to make the code path less

dependent on the input. Always trust the intro-commit itself. Always call

verify-introductory-commit.

(verify-introductory-commit): Check if the commit contains the key that was

used to sign it, and issue a warning otherwise. This is to avoid the confusion

caused by only the *second* commit yielding an error, because intro-commits

are always trusted.

(authenticate-commit): Clarify error message.

(authorized-keys-at-commit): Factored out to the toplevel from

commit-authorized-keys.

---

An example output with this patch:

$ ./pre-inst-env guix pull --allow-downgrades

Updating channel 'guix' from Git repository at '/path/guix'...

guix pull: warning: moving channel 'guix' from 26a979105a58e99c6e0fbb51cb1500dfa2bc2cec to unrelated commit 17fc5e35699d2219e6fae1f0583bb8c2ec3deb25

guix pull: warning: initial commit 17fc5e35699d2219e6fae1f0583bb8c2ec3deb25 does not add the key it is signed with (2E4F C7F5 07AB F022 36D3 D51F 31EE D3BE 74EC 3A1F) to the '.guix-authorizations' file.

Authenticating channel 'guix', commits 17fc5e3 to 17fc5e3 (0 new commits)...

[...]

guix/channels.scm | 4 +-

guix/git-authenticate.scm | 156 ++++++++++++++++++++++----------------

2 files changed, 94 insertions(+), 66 deletions(-)

Attila Lendvai schreef op di 28-09-2021 om 03:05 [+0200]:

I recommend placing the patch fixing the error before the test,

to aid bisection.

Greetings,

Maxime.

Attila Lendvai schreef op di 28-09-2021 om 18:24 [+0200]:

Do we really need to close and open the connection again every time

a continuation is made and resumed? This seems inefficient if a threading

mechanism implemented by continuations is used (such as guile-fibers),

and there are two threads (‘fibers’) communicating and waiting with/for

each other in a loop, causing many ‘context switches’ (i.e., many captured

and resumed continuations).

Also note that a connection has some state: to the guix-daemon, it acts as

a GC root for everything built with the connection, and everything added to

the store (with add-to-store & friends) with that connection ... Simply

reconnecting isn't sufficient.

Greetings,

Maxime

pardon my ignorance wrt dynamic-wind and call/cc, but does that^ mean

that 1) i should simply leave the wind part of the dynamic-wind empty

and move back the open-connection call into the let... or that 2) the

entire idea of replacing the exception handler with an unwind-protect

is flawed?

if 2) then i'll try to smarten up the handler to use raise-continuable

if the exception is of type &warning.

or any better ideas?

- attila

PGP: 5D5F 45C7 DFCD 0A39

Attila Lendvai schreef op wo 29-09-2021 om 14:50 [+0000]:

About 1): which 'wind part' of dynamic-wind are you referring to?

The in-guard or the out-guard?

If the out-guard is empty, then the reference to the old connection will

be overwritten when the fiber is paused and resumed, so the old connection

will eventually be GC'ed, thus the daemon forgets some GC roots, leading

to a rare GC bug.

If the in-guard is empty, then the after pausing the fiber and resuming it,

the connection will be closed while the fiber might still need it.

That should work. Or simpler: always use raise-continuable.

Conventionally, to emit warnings, the procedure 'warning' from

(guix diagnostics) is used. See e.g. (guix ci), (guix deprecation), (guix gexp),

(guix import ...), various modules under (guix scripts ...), (guix upstream) ...

Is there any reason not to use this pre-existing procedure?

Greetings,

Maxime

ok, so this is a no-go. thanks for the clarification!

in a more advanced UI it might be a different story, but in the

current setup the only reason is to be able to assert for the warning

in the tests.

is that worth it? shall i just user WARNING and forget about the test?

- attila

PGP: 5D5F 45C7 DFCD 0A39

Attila Lendvai schreef op wo 29-09-2021 om 21:22 [+0000]:

[...]

Testing a warning is emitted seems nice. You could parameterise guix-warning-port

and use call-with-output-sting to capture the warning, and use (not (string-null? ...))

to verify a warning has been emitted. From a quick grep, it appears

tests/transformations.scm and tests/substitute.scm are doing things like this.

Greetings,

Maxime.

Attila Lendvai schreef op di 28-09-2021 om 18:24 [+0200]:

Could 'default-authorizations' still be documented?

Anyway, I don't see any problems with this patch (ignoring the warning and the

docstrings), but I'm completely unfamiliar with the internals of channel

authentication, so I don't know what to look for. You'll need to find someone

else to review this.

Greetings,

Maxime

severity 50814 important

done

Hi!

Leo Famulari <leo@famulari.name> skribis:

This had the unfortunate effect that this patch would not show up in

Emacs debbugs.el, which, for some reason, doesn’t know about “grave”.

I’ve changed to “important” and I’d suggest not going beyond “serious”,

which is already grave enough. :-)

Ludo’.

Hi Attila,

Attila Lendvai <attila@lendvai.name> skribis:

This behavior is intentional and documented (info "(guix) Specifying

Channel Authorizations"):

Channel introductions answer these questions by describing the first

commit of a channel that should be authenticated. The first time a

channel is fetched with ‘guix pull’ or ‘guix time-machine’, the command

looks up the introductory commit and verifies that it is signed by the

specified OpenPGP key. From then on, it authenticates commits according

to the rule above.

[…]

The channel introduction, as we saw above, is the commit/key

pair—i.e., the commit that introduced ‘.guix-authorizations’, and

the fingerprint of the OpenPGP used to sign it.

By definition, parent commits of the introduction do not (not

necessarily) provide ‘.guix-authorizations’. So there’s nothing to be

done here, other than checking that the introductory commit is indeed

signed by the key specified in the introduction.

Does that make sense?

(Other patches you posted in this thread might be useful though, but we

can discuss them independently.)

Thanks,

Ludo’.

PS: If you haven’t already, you can take a look at the following pages

for more on the design rationale:

https://guix.gnu.org/en/blog/2020/securing-updates/

https://issues.guix.gnu.org/22883#69

there are three main topics of this patchset:

1) adding a (hopefully helpful) warning. the primary goal.

2) general cleanups

3) IIRC, fixing some actual bugs in the process

as for 1):

what i did was fork guix master, and now i'm pulling my own

authenticated branch from my own local git checkout, where every once

in a while i merge my various topic branches into my branch, and guix

pull it.

when i added my second commit i have spent a disproportionate amount

of time trying to figure out what was happening: the first commit was

accepted, and i thought it's set up all fine. then who knows how much

later, when i added my second commit, i was staring at the screen

without a clue why pulling doesn't work anymore.

then i ventured into quickly adding warning, so that others won't

waste their time on this, and went down the rabbit hole, which

resulted in fixing actual bugs, i believe. IIRC, they are exposed by

the test that i have added when run on the current codebase.

as for 3), any actual bugs:

i'll investigate again later by running the test without the fix, and write

up my results here, or better yet, in a better commit message.

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

It should be a grammatical if not legal offense to ascribe thoughts, opinions and decisions to "we" without a signed power of attorney.

This test used to fail before a recent fix to authenticate-repository.

channel-introduction".

---

reseding the patch that adds the test (i have extended the comments where the

test fails, and also fixed the check for the warning).

i ran the test without my fix commit, and indeed it fails at two points:

1)

;; Should fail because it is signed with key2, not key1

(check-from "commit 3" #:should-fail? #true)

2)

;; It is not very intuitive why commit 1 and 2 should be trusted

;; at this point: commit 4 has previously been used as a channel

;; intro, thus it got marked as trusted in the ~/.cache/.

;; Because commit 1 and 2 are among its parents, it should also

;; be trusted at this point because of the cache. Note that

;; it's debatable whether this semantics is a good idea, but

;; this is how git-authenticate is and has been implemented for

;; a while (modulo failing to update the cache in the past when

;; taking certain code paths).

(check-from "commit 1")

please take a look at the test, and let me know if any of the

assumptions encoded into the test is wrong, or if anything

else needs clarification.

- attila

tests/git-authenticate.scm | 139 +++++++++++++++++++++++++++++++++++++

1 file changed, 139 insertions(+)

Hi,

Attila Lendvai <attila@lendvai.name> skribis:

Alright. Please next time open one issue per topic: that’s a good way

to maximize the chances that review happens in a timely fashion. :-)

I understand the behavior was surprising to you, but I’d like to see if

we can pinpoint why. Can you think of anything that could be added to

the documentation?

https://guix.gnu.org/manual/en/html_node/Specifying-Channel-Authorizations.html

Yes please. In general, please start by reporting the bug: what you

get, what you expected, and how to reproduce. That makes it easier to

understand and evaluate proposed fixes.

Thanks!

Ludo’.

On Sat, Oct 09, 2021 at 03:44:40PM +0200, Ludovic Courtès wrote:

Noted!

i ran the test without my fix, and indeed it fails at two points:

1)

;; Should fail because it is signed with key2, not key1

(check-from "commit 3" #:should-fail? #true)

2)

;; It is not very intuitive why commit 1 and 2 should be trusted

;; at this point: commit 4 has previously been used as a channel

;; intro, thus it got marked as trusted in the ~/.cache/.

;; Because commit 1 and 2 are among its parents, it should also

;; be trusted at this point because of the cache. Note that

;; it's debatable whether this semantics is a good idea, but

;; this is how git-authenticate is and has been implemented for

;; a while (modulo failing to update the cache in the past when

;; taking certain code paths).

(check-from "commit 1")

(check-from "commit 2")

note that i have extended the above comments compared to what's in the

commits that i have sent previously (and i also fixed the check for

the warning). i suspect there are still things to discuss, so i'll

wait for any feedback before i resend the patches. i did not touch the

test code itself, so you can easily find these points in it.

understood. the problem is that it all started out as adding a

warning, and the rest were just side-quests... :)

can i mark dependencies between issues/patchsets?

because all that i could do here is split this into two sets of

commits (because of the dependencies between the commits):

1) the 3 test commits, and

2) the 2 guix commits.

i thought that separating the test that is exhibiting the bug, from

the fix that fixes it, would only hinder the process.

if we assume that everyone reads and internalizes every page of the

documentation of every software that they use, then i guess nothing

needs to be added.

but if our goal is to maximize the effectiveness of the users, then no

amount of static, free-flowing text can compete with a warning that is

signalled in close context to the issue.

i think the right question to ask here is how often would this warning

be superfluous. my assumption is that very rarely, if ever, but i may

not be aware of some use-cases.

looking forward to any feedback on how to improve this.

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

If the source of fear is the unknown, and fear is the only way to be controlled, then knowledge is the only way to be free.

Hi Attila,

Attila Lendvai <attila@lendvai.name> skribis:

Sorry, which test is failing? Is that part of the patches you sent?

I need more context. :-)

[...]

Yes, in general it’s best to have the test and the fix in the same

commit.

However, at this point, I’m not sure which “bug” we’re talking about.

What you described in your initial message is not a bug in my view:

https://issues.guix.gnu.org/50814#28

Sure, I agree.

However, you’re clearly a power user at this point :-), and we’re

talking about one of the most sensitive pieces of code in Guix. I think

it’s important to make sure we’re on the same level of understanding of

the design and current implementation; I also think it’s not

unreasonable to expect channel writers to pay attention to documentation

on these matters.

I’m not saying that we should not change anything, but rather that it’s

not like a simple usability/UX issue.

I hope this makes sense!

Ludo’.

hi Ludo,

i have sent 5 patches. three of them are prefixed with 'test:', and

two of those are test idempotent test infrastructure changes. the

third of them adds a new test that tests git-authenticate. this is the

test that i'm talking about.

if you apply only these 3 test related commits, and run the new test

on the unpatched codebase, then you'll see the two failures that i'm

talking about in my previous mail.

search the test log for 'FAILURE' (the test runs fully, but fails in

case any of the tests fail).

one of the two failures is a more serious issue because a channel

intro commit is accepted while it shouldn't be.

i cut the fix and the test in separate commits (but sent them in the

same patchset/issue), so that it's possible to partially apply only

the test commits, and study its behavior on the current codebase.

the bug is described, formally, by the test that i have added (unless

the test itself is wrong, that is). IIRC, i started putting together

this new test to expose the bugs that i have suspected while reviewing

the implementation of git-authenticate, and then to support my effort

to fix them afterwards.

i think the best next-action is for someone qualified to take a look

at the test that i have added, and see if any of the assumptions

encoded in it is wrong. i think i understand this part of the codebase

pretty well now, but i may have erred.

if the test seems to be valid, then proceed to review the rest of the

commits.

yes, it does! actually, i welcome the reluctance to haphazardly apply

patches to this part of the codebase. i was kinda expecting this, and

that's why i have prepared the commits so that the test can be applied

and tried separately.

hope this clarifies the situation,

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“Everything can be taken from a man but one thing: the last of the human freedoms—to choose one’s attitude in any given set of circumstances, to choose one’s own way.”

— Viktor E. Frankl (1905–1997), 'Man's Search for Meaning' (1946)

a nested way under a with-temporary-git-repository.

(populate-git-repository): Extend the DSL with (add "some-noise"), (reset

"[commit hash]"), (checkout "branch" orphan).

---

guix/tests/git.scm | 23 +++++++++++++++++++++--

guix/tests/gnupg.scm | 8 ++++++--

2 files changed, 27 insertions(+), 4 deletions(-)

The third key will be used in an upcoming commit.

Rename public keys to .pub.

(%ed25519-3-secret-key-file): New variable.

(%ed25519-2-public-key-file): Renamed from %ed25519bis-public-key-file.

(%ed25519-2-secret-key-file): Renamed from %ed25519bis-secret-key-file.

---

Makefile.am | 20 +++++-----

build-aux/test-env.in | 6 +--

guix/tests/gnupg.scm | 22 ++++++----

tests/channels.scm | 18 ++++-----

tests/git-authenticate.scm | 23 +++++------

tests/guix-authenticate.sh | 4 +-

tests/{civodul.key => keys/civodul.pub} | 0

tests/{dsa.key => keys/dsa.pub} | 0

tests/{ed25519bis.key => keys/ed25519-2.pub} | 0

tests/{ed25519bis.sec => keys/ed25519-2.sec} | 0

tests/keys/ed25519-3.pub | 9 +++++

tests/keys/ed25519-3.sec | 10 +++++

tests/{ed25519.key => keys/ed25519.pub} | 0

tests/{ => keys}/ed25519.sec | 0

tests/{rsa.key => keys/rsa.pub} | 0

tests/{ => keys}/signing-key.pub | 0

tests/{ => keys}/signing-key.sec | 0

tests/openpgp.scm | 42 +++++++++++---------

18 files changed, 93 insertions(+), 61 deletions(-)

rename tests/{civodul.key => keys/civodul.pub} (100%)

rename tests/{dsa.key => keys/dsa.pub} (100%)

rename tests/{ed25519bis.key => keys/ed25519-2.pub} (100%)

rename tests/{ed25519bis.sec => keys/ed25519-2.sec} (100%)

create mode 100644 tests/keys/ed25519-3.pub

create mode 100644 tests/keys/ed25519-3.sec

rename tests/{ed25519.key => keys/ed25519.pub} (100%)

rename tests/{ => keys}/ed25519.sec (100%)

rename tests/{rsa.key => keys/rsa.pub} (100%)

rename tests/{ => keys}/signing-key.pub (100%)

rename tests/{ => keys}/signing-key.sec (100%)

Always verify the channel introduction commit, so that no commit can slip

through that was signed with a different key.

Always update the cache, because it affects the behavior of later calls.

Signal a continuable compound-condition (with type &warning included) when a

channel introduction commit doesn't also update the '.guix-authentications'

file.

message to point to the relevant part of the manual.

(authenticate-repository): Eliminate optimizations to make the code path less

dependent on the input. Always trust the intro-commit itself. Always call

verify-introductory-commit.

(verify-introductory-commit): Check if the commit contains the key that was

used to sign it, and issue a warning otherwise. This is to avoid the confusion

caused by only the *second* commit yielding an error, because intro-commits

are always trusted.

(authenticate-commit): Clarify error message.

(authorized-keys-at-commit): Factored out to the toplevel from

commit-authorized-keys.

---

guix/channels.scm | 4 +-

guix/git-authenticate.scm | 158 +++++++++++++++++++++++---------------

2 files changed, 96 insertions(+), 66 deletions(-)

exceptions are not broken by being re-raised as non-continuable. This is

needed for a later commit that uses continuable exceptions from within

git-authenticate to signal warnings to the user. The reason for this is that

this way tests can explicitly check that a warning was signalled in certain

situations.

printing them to the user, and then continuing at the place they were

signalled at.

function.

---

guix/diagnostics.scm | 4 ++++

guix/store.scm | 7 +++++--

guix/ui.scm | 11 ++++++++++-

3 files changed, 19 insertions(+), 3 deletions(-)

This test used to fail before a recent fix to authenticate-repository.

channel-introduction".

---

tests/git-authenticate.scm | 150 +++++++++++++++++++++++++++++++++++++

1 file changed, 150 insertions(+)

Hi Attila,

Sorry for dropping the ball. On the private guix-security mailing list,

I just sent an analysis showing why the bug reported there was, AFAICS,

not a bug; however, the test here is different and seems to be poised to

expose a different problem.

Attila Lendvai <attila@lendvai.name> skribis:

I won’t comment on the whole test for now, because it’s too complex, but

here’s what I noticed:

[...]

This is exercising support for “historical authorizations”, which works

slightly differently than the regular ‘.guix-authorizations’-process

used by ‘guix pull’ & co.

Mutation is making it harder for me to understand what the test does.

Due to ‘reset’ here, the intro commit that ‘check-from’ passes to

‘authenticate-repository’ is no longer the right one (IIUC).

Any commit in the closure of one of those listed in

~/.cache/guix/authentication is considered authentic.

So, if you arrange to populate that file with IDs of commits that were

not authenticated, then yes, you’ll find that ‘authenticate-repository’

reports surprising things. But that’s because fundamentally, we cannot

protect the user from self-inflicted attacks.

To summarize, I do not see a bug here, but I’m also not sure what the

test was trying to demonstrate.

Could you boil it down? (Keep in mind that it takes a lot of time to

merely review and under the test and claim.)

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

On guix-security (in your Dec. 22, 2021, message), you provided a

different test case, showing that the introductory commit’s signature is

not verified when ‘authenticate-repository’ is asked to authenticate an

empty commit range (introductory commit = tip of the branch). This is

due to the (null? commit) condition in ‘authenticate-repository’:

;; When COMMITS is empty, it's because END-COMMIT is in the closure of

;; START-COMMIT and/or AUTHENTICATED-COMMITS, in which case it's known to

;; be authentic already.

(if (null? commits)

'()

(let ((reporter (make-reporter start-commit end-commit commits)))

;; If it's our first time, verify START-COMMIT's signature.

(when (null? authenticated-commits)

(verify-introductory-commit repository keyring

start-commit signer))

…))

When START = END, the (null? commits) condition is true, and thus

‘verify-introductory-commit’ is not called. This is the “bug”, although

START = END is a situation where there’s not much to do anyway.

As I wrote, we could move the ‘verify-introductory-commit’ call before

the ‘if’, specifically for this test case, but that doesn’t strike me as

useful; it may also have a visible performance impact on real cases.

Thanks,

Ludo’.

FTR, apparently, i have abandoned the pursuit of this issue.

to sum up my perspective:

guix pull's behavior has greatly confused me once by accepting a commit that it (much) later rejected (when it wasn't the tip of the history anymore). i have wasted quite some time debugging/understanding this, which included reading the code. due to my annoyance i got into the presented work/patch while in the process.

if this is not considered an issue, then i don't have a dog in this fight anymore. i have learned this quirk by now, so it won't bite me again, and the rest is up to the maintainers to decide.

feel free to take from this patch whatever you find useful, or close it if you find no more value in it.

happy hacking,

--

• attila lendvai

• PGP: 963F 5D5F 45C7 DFCD 0A39

--

“Nothing is so permanent as a temporary government program.”

— Milton Friedman (1912–2006)