* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
@ 2021-09-26 18:02 ` Leo Famulari
2021-10-09 13:44 ` Ludovic Courtès
2021-09-26 18:14 ` Maxime Devos
` (6 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Leo Famulari @ 2021-09-26 18:02 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814, guix-security
[-- Attachment #1: Type: text/plain, Size: 1598 bytes --]
On Sun, Sep 26, 2021 at 12:19:29PM +0200, Attila Lendvai wrote:
> * guix/git-authenticate.scm (authenticate-commit): Reword and extend the error
> 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).
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."
https://debbugs.gnu.org/Developer.html#severities
I'm not sure if that's justified or not but this patch should be
prioritized.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 18:02 ` Leo Famulari
@ 2021-10-09 13:44 ` Ludovic Courtès
2021-10-12 15:17 ` Leo Famulari
0 siblings, 1 reply; 38+ messages in thread
From: Ludovic Courtès @ 2021-10-09 13:44 UTC (permalink / raw)
To: Leo Famulari; +Cc: 50814, Attila Lendvai, guix-security
Hi!
Leo Famulari <leo@famulari.name> skribis:
> 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."
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’.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-09 13:44 ` Ludovic Courtès
@ 2021-10-12 15:17 ` Leo Famulari
0 siblings, 0 replies; 38+ messages in thread
From: Leo Famulari @ 2021-10-12 15:17 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 50814, Attila Lendvai, guix-security
On Sat, Oct 09, 2021 at 03:44:40PM +0200, Ludovic Courtès wrote:
> 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. :-)
Noted!
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
2021-09-26 18:02 ` Leo Famulari
@ 2021-09-26 18:14 ` Maxime Devos
2021-09-27 18:01 ` Attila Lendvai
2021-09-27 18:45 ` Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 1/4] tests: Smarten up git repository testing framework Attila Lendvai
` (5 subsequent siblings)
7 siblings, 2 replies; 38+ messages in thread
From: Maxime Devos @ 2021-09-26 18:14 UTC (permalink / raw)
To: Attila Lendvai, 50814
[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]
Attila Lendvai schreef op zo 26-09-2021 om 12:19 [+0200]:
> * guix/git-authenticate.scm (authenticate-commit): Reword and extend the error
> 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(-)
>
> diff --git a/guix/git-authenticate.scm b/guix/git-authenticate.scm
> index ab3fcd8b2f..7d66bf0754 100644
> --- a/guix/git-authenticate.scm
> +++ b/guix/git-authenticate.scm
> @@ -236,8 +236,8 @@ not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
> (condition
> (&unauthorized-commit-error (commit id)
> (signing-key signing-key)))
> - (formatted-message (G_ "commit ~a not signed by an authorized \
> -key: ~a")
> + (formatted-message (G_ "commit ~a is signed by an unauthorized \
> +key: ~a\nSee info guix \"Specifying Channel Authorizations\".")
> (oid->string id)
> (openpgp-format-fingerprint
> (openpgp-public-key-fingerprint
> @@ -424,7 +424,12 @@ denoting the authorized keys for commits whose parent lack the
> ;; If it's our first time, verify START-COMMIT's signature.
> (when (null? authenticated-commits)
> (verify-introductory-commit repository keyring
> - start-commit signer))
> + start-commit signer)
> + ;; Explicitly authenticate the channel introduction commit, so that
> + ;; it's also rejected unless it's signed by an authorized
> + ;; key. Otherwise only the second commit would yield an error, which
> + ;; is confusing.
> + (authenticate-commits repository (list start-commit)))
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.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 18:14 ` Maxime Devos
@ 2021-09-27 18:01 ` Attila Lendvai
2021-09-27 18:45 ` Attila Lendvai
1 sibling, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-27 18:01 UTC (permalink / raw)
To: Maxime Devos; +Cc: 50814
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 18:14 ` Maxime Devos
2021-09-27 18:01 ` Attila Lendvai
@ 2021-09-27 18:45 ` Attila Lendvai
2021-09-28 10:02 ` Maxime Devos
1 sibling, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-09-27 18:45 UTC (permalink / raw)
To: Maxime Devos; +Cc: 50814
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-27 18:45 ` Attila Lendvai
@ 2021-09-28 10:02 ` Maxime Devos
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Devos @ 2021-09-28 10:02 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]
Attila Lendvai schreef op ma 27-09-2021 om 18:45 [+0000]:
>
[...]
> 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.
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.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 1/4] tests: Smarten up git repository testing framework.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
2021-09-26 18:02 ` Leo Famulari
2021-09-26 18:14 ` Maxime Devos
@ 2021-09-28 1:05 ` Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 2/4] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
` (2 more replies)
2021-09-28 16:24 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
` (4 subsequent siblings)
7 siblings, 3 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 1:05 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
* guix/tests/git.scm (with-git-repository): New macro that can be used in
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/gnupg.scm (key-fingerprint-vector): New function.
---
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(-)
diff --git a/guix/tests/git.scm b/guix/tests/git.scm
index 69960284d9..76f5a8b937 100644
--- a/guix/tests/git.scm
+++ b/guix/tests/git.scm
@@ -26,6 +26,7 @@
#:use-module (ice-9 control)
#:export (git-command
with-temporary-git-repository
+ with-git-repository
find-commit))
(define git-command
@@ -59,8 +60,9 @@ Return DIRECTORY on success."
(apply invoke (git-command) "-C" directory
command args)))))
- (mkdir-p directory)
- (git "init")
+ (unless (directory-exists? (string-append directory "/.git"))
+ (mkdir-p directory)
+ (git "init"))
(let loop ((directives directives))
(match directives
@@ -78,6 +80,9 @@ Return DIRECTORY on success."
port)))
(git "add" file)
(loop rest)))
+ ((('add file-name-and-content) rest ...)
+ (loop (cons `(add ,file-name-and-content ,file-name-and-content)
+ rest)))
((('remove file) rest ...)
(git "rm" "-f" file)
(loop rest))
@@ -99,12 +104,18 @@ Return DIRECTORY on success."
((('checkout branch) rest ...)
(git "checkout" branch)
(loop rest))
+ ((('checkout branch 'orphan) rest ...)
+ (git "checkout" "--orphan" branch)
+ (loop rest))
((('merge branch message) rest ...)
(git "merge" branch "-m" message)
(loop rest))
((('merge branch message ('signer fingerprint)) rest ...)
(git "merge" branch "-m" message
(string-append "--gpg-sign=" fingerprint))
+ (loop rest))
+ ((('reset to) rest ...)
+ (git "reset" "--hard" to)
(loop rest)))))
(define (call-with-temporary-git-repository directives proc)
@@ -121,6 +132,14 @@ per DIRECTIVES."
(lambda (directory)
exp ...)))
+(define-syntax-rule (with-git-repository directory
+ directives exp ...)
+ "Evaluate EXP in a context where DIRECTORY is (further) populated as
+per DIRECTIVES."
+ (begin
+ (populate-git-repository directory directives)
+ exp ...))
+
(define (find-commit repository message)
"Return the commit in REPOSITORY whose message includes MESSAGE, a string."
(let/ec return
diff --git a/guix/tests/gnupg.scm b/guix/tests/gnupg.scm
index eb8ff63a43..c7630db912 100644
--- a/guix/tests/gnupg.scm
+++ b/guix/tests/gnupg.scm
@@ -33,6 +33,7 @@
read-openpgp-packet
key-fingerprint
+ key-fingerprint-vector
key-id))
(define gpg-command
@@ -76,7 +77,10 @@ process is terminated afterwards."
(open-bytevector-input-port
(call-with-input-file file read-radix-64))))
+(define key-fingerprint-vector
+ (compose openpgp-public-key-fingerprint
+ read-openpgp-packet))
+
(define key-fingerprint
(compose openpgp-format-fingerprint
- openpgp-public-key-fingerprint
- read-openpgp-packet))
+ key-fingerprint-vector))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 2/4] tests: Move keys into ./tests/keys/ and add a third ed25519 key.
2021-09-28 1:05 ` [bug#50814] [PATCH 1/4] tests: Smarten up git repository testing framework Attila Lendvai
@ 2021-09-28 1:05 ` Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 3/4] tests: Add failing test for .guix-authorizations and channel intro Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 4/4] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
2 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 1:05 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
The third key will be used in an upcoming commit.
Rename public keys to .pub.
* guix/tests/gnupg.scm (%ed25519-3-public-key-file): New variable.
(%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.
* tests/keys/ed25519-3.key: New file.
* tests/keys/ed25519-3.sec: New 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%)
diff --git a/Makefile.am b/Makefile.am
index 042cf28464..c0a5b14f02 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -640,16 +640,18 @@ EXTRA_DIST += \
build-aux/update-guix-package.scm \
build-aux/update-NEWS.scm \
tests/test.drv \
- tests/signing-key.pub \
- tests/signing-key.sec \
tests/cve-sample.json \
- tests/civodul.key \
- tests/rsa.key \
- tests/dsa.key \
- tests/ed25519.key \
- tests/ed25519.sec \
- tests/ed25519bis.key \
- tests/ed25519bis.sec \
+ tests/keys/signing-key.pub \
+ tests/keys/signing-key.sec \
+ tests/keys/civodul.pub \
+ tests/keys/rsa.pub \
+ tests/keys/dsa.pub \
+ tests/keys/ed25519.pub \
+ tests/keys/ed25519.sec \
+ tests/keys/ed25519-2.pub \
+ tests/keys/ed25519-2.sec \
+ tests/keys/ed25519-3.pub \
+ tests/keys/ed25519-3.sec \
build-aux/config.rpath \
bootstrap \
doc/build.scm \
diff --git a/build-aux/test-env.in b/build-aux/test-env.in
index 7efc43206c..ca786437e9 100644
--- a/build-aux/test-env.in
+++ b/build-aux/test-env.in
@@ -73,9 +73,9 @@ then
# Copy the keys so that the secret key has the right permissions (the
# daemon errors out when this is not the case.)
mkdir -p "$GUIX_CONFIGURATION_DIRECTORY"
- cp "@abs_top_srcdir@/tests/signing-key.sec" \
- "@abs_top_srcdir@/tests/signing-key.pub" \
- "$GUIX_CONFIGURATION_DIRECTORY"
+ cp "@abs_top_srcdir@/tests/keys/signing-key.sec" \
+ "@abs_top_srcdir@/tests/keys/signing-key.pub" \
+ "$GUIX_CONFIGURATION_DIRECTORY"
chmod 400 "$GUIX_CONFIGURATION_DIRECTORY/signing-key.sec"
fi
diff --git a/guix/tests/gnupg.scm b/guix/tests/gnupg.scm
index c7630db912..09f02a2b67 100644
--- a/guix/tests/gnupg.scm
+++ b/guix/tests/gnupg.scm
@@ -28,8 +28,10 @@
%ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file
+ %ed25519-3-public-key-file
+ %ed25519-3-secret-key-file
read-openpgp-packet
key-fingerprint
@@ -64,13 +66,17 @@ process is terminated afterwards."
(call-with-fresh-gnupg-setup imported (lambda () exp ...)))
(define %ed25519-public-key-file
- (search-path %load-path "tests/ed25519.key"))
+ (search-path %load-path "tests/keys/ed25519.pub"))
(define %ed25519-secret-key-file
- (search-path %load-path "tests/ed25519.sec"))
-(define %ed25519bis-public-key-file
- (search-path %load-path "tests/ed25519bis.key"))
-(define %ed25519bis-secret-key-file
- (search-path %load-path "tests/ed25519bis.sec"))
+ (search-path %load-path "tests/keys/ed25519.sec"))
+(define %ed25519-2-public-key-file
+ (search-path %load-path "tests/keys/ed25519-2.pub"))
+(define %ed25519-2-secret-key-file
+ (search-path %load-path "tests/keys/ed25519-2.sec"))
+(define %ed25519-3-public-key-file
+ (search-path %load-path "tests/keys/ed25519-3.pub"))
+(define %ed25519-3-secret-key-file
+ (search-path %load-path "tests/keys/ed25519-3.sec"))
(define (read-openpgp-packet file)
(get-openpgp-packet
diff --git a/tests/channels.scm b/tests/channels.scm
index 3e82315b0c..d45c450241 100644
--- a/tests/channels.scm
+++ b/tests/channels.scm
@@ -480,8 +480,8 @@
#t
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add ".guix-channel"
,(object->string
@@ -507,7 +507,7 @@
(commit-id-string commit1)
(openpgp-public-key-fingerprint
(read-openpgp-packet
- %ed25519bis-public-key-file)))) ;different key
+ %ed25519-2-public-key-file)))) ;different key
(channel (channel (name 'example)
(url (string-append "file://" directory))
(introduction intro))))
@@ -519,7 +519,7 @@
(oid->string (commit-id commit1))
(key-fingerprint %ed25519-public-key-file)
(key-fingerprint
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(authenticate-channel channel directory
(commit-id-string commit2)
#:keyring-reference-prefix "")
@@ -530,8 +530,8 @@
#t
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add ".guix-channel"
,(object->string
@@ -552,12 +552,12 @@
(signer ,(key-fingerprint %ed25519-public-key-file)))
(add "c.txt" "C")
(commit "third commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(branch "channel-keyring")
(checkout "channel-keyring")
(add "signer.key" ,(call-with-input-file %ed25519-public-key-file
get-string-all))
- (add "other.key" ,(call-with-input-file %ed25519bis-public-key-file
+ (add "other.key" ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(commit "keyring commit")
(checkout "master"))
@@ -588,7 +588,7 @@
(unauthorized-commit-error-signing-key c))
(openpgp-public-key-fingerprint
(read-openpgp-packet
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(authenticate-channel channel directory
(commit-id-string commit3)
#:keyring-reference-prefix "")
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index d87eacc659..f66ef191b0 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -161,14 +161,14 @@
(test-assert "signed commits, .guix-authorizations, unauthorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add "signer1.key"
,(call-with-input-file %ed25519-public-key-file
get-string-all))
(add "signer2.key"
- ,(call-with-input-file %ed25519bis-public-key-file
+ ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(add ".guix-authorizations"
,(object->string
@@ -184,7 +184,7 @@
(checkout "devel")
(add "devel/1.txt" "1")
(commit "first devel commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(checkout "master")
(add "b.txt" "B")
(commit "second commit"
@@ -203,7 +203,7 @@
(openpgp-public-key-fingerprint
(unauthorized-commit-error-signing-key c))
(openpgp-public-key-fingerprint
- (read-openpgp-packet %ed25519bis-public-key-file)))))
+ (read-openpgp-packet %ed25519-2-public-key-file)))))
(and (authenticate-commits repository (list master1 master2)
#:keyring-reference "master")
@@ -230,14 +230,14 @@
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add "signer1.key"
,(call-with-input-file %ed25519-public-key-file
get-string-all))
(add "signer2.key"
- ,(call-with-input-file %ed25519bis-public-key-file
+ ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(add ".guix-authorizations"
,(object->string
@@ -258,12 +258,12 @@
%ed25519-public-key-file)
(name "Alice"))
(,(key-fingerprint
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(commit "first devel commit"
(signer ,(key-fingerprint %ed25519-public-key-file)))
(add "devel/2.txt" "2")
(commit "second devel commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(checkout "master")
(add "b.txt" "B")
(commit "second commit"
@@ -273,7 +273,7 @@
;; After the merge, the second signer is authorized.
(add "c.txt" "C")
(commit "third commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file))))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file))))
(with-repository directory repository
(let ((master1 (find-commit repository "first commit"))
(master2 (find-commit repository "second commit"))
@@ -328,4 +328,3 @@
'failed)))))))
(test-end "git-authenticate")
-
diff --git a/tests/guix-authenticate.sh b/tests/guix-authenticate.sh
index 3a05b232c1..0de6da1878 100644
--- a/tests/guix-authenticate.sh
+++ b/tests/guix-authenticate.sh
@@ -28,7 +28,7 @@ rm -f "$sig" "$hash"
trap 'rm -f "$sig" "$hash"' EXIT
-key="$abs_top_srcdir/tests/signing-key.sec"
+key="$abs_top_srcdir/tests/keys/signing-key.sec"
key_len="`echo -n $key | wc -c`"
# A hexadecimal string as long as a sha256 hash.
@@ -67,7 +67,7 @@ test "$code" -ne 0
# encoded independently of the current locale: <https://bugs.gnu.org/43421>.
hash="636166e9636166e9636166e9636166e9636166e9636166e9636166e9636166e9"
latin1_cafe="caf$(printf '\351')"
-echo "sign 21:tests/signing-key.sec 64:$hash" | guix authenticate \
+echo "sign 26:tests/keys/signing-key.sec 64:$hash" | guix authenticate \
| LC_ALL=C grep "hash sha256 \"$latin1_cafe"
# Test for <http://bugs.gnu.org/17312>: make sure 'guix authenticate' produces
diff --git a/tests/civodul.key b/tests/keys/civodul.pub
similarity index 100%
rename from tests/civodul.key
rename to tests/keys/civodul.pub
diff --git a/tests/dsa.key b/tests/keys/dsa.pub
similarity index 100%
rename from tests/dsa.key
rename to tests/keys/dsa.pub
diff --git a/tests/ed25519bis.key b/tests/keys/ed25519-2.pub
similarity index 100%
rename from tests/ed25519bis.key
rename to tests/keys/ed25519-2.pub
diff --git a/tests/ed25519bis.sec b/tests/keys/ed25519-2.sec
similarity index 100%
rename from tests/ed25519bis.sec
rename to tests/keys/ed25519-2.sec
diff --git a/tests/keys/ed25519-3.pub b/tests/keys/ed25519-3.pub
new file mode 100644
index 0000000000..72f311984c
--- /dev/null
+++ b/tests/keys/ed25519-3.pub
@@ -0,0 +1,9 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+mDMEYVH/7xYJKwYBBAHaRw8BAQdALMLeUhjEG2/UPCJj2j/debFwwAK5gT3G0l5d
+ILfFldm0FTxleGFtcGxlQGV4YW1wbGUuY29tPoiWBBMWCAA+FiEEjO6M85jMSK68
+7tINGBzA7NyoagkFAmFR/+8CGwMFCQPCZwAFCwkIBwIGFQoJCAsCBBYCAwECHgEC
+F4AACgkQGBzA7Nyoagl3lgEAw6yqIlX11lTqwxBGhZk/Oy34O13cbJSZCGv+m0ja
++hcA/3DCNOmT+oXjgO/w6enQZUQ1m/d6dUjCc2wOLlLz+ZoG
+=+r3i
+-----END PGP PUBLIC KEY BLOCK-----
diff --git a/tests/keys/ed25519-3.sec b/tests/keys/ed25519-3.sec
new file mode 100644
index 0000000000..04128a4131
--- /dev/null
+++ b/tests/keys/ed25519-3.sec
@@ -0,0 +1,10 @@
+-----BEGIN PGP PRIVATE KEY BLOCK-----
+
+lFgEYVH/7xYJKwYBBAHaRw8BAQdALMLeUhjEG2/UPCJj2j/debFwwAK5gT3G0l5d
+ILfFldkAAP92goSbbzQ0ttElr9lr5Cm6rmQtqUZ2Cu/Jk9fvfZROwxI0tBU8ZXhh
+bXBsZUBleGFtcGxlLmNvbT6IlgQTFggAPhYhBIzujPOYzEiuvO7SDRgcwOzcqGoJ
+BQJhUf/vAhsDBQkDwmcABQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEBgcwOzc
+qGoJd5YBAMOsqiJV9dZU6sMQRoWZPzst+Dtd3GyUmQhr/ptI2voXAP9wwjTpk/qF
+44Dv8Onp0GVENZv3enVIwnNsDi5S8/maBg==
+=EmOt
+-----END PGP PRIVATE KEY BLOCK-----
diff --git a/tests/ed25519.key b/tests/keys/ed25519.pub
similarity index 100%
rename from tests/ed25519.key
rename to tests/keys/ed25519.pub
diff --git a/tests/ed25519.sec b/tests/keys/ed25519.sec
similarity index 100%
rename from tests/ed25519.sec
rename to tests/keys/ed25519.sec
diff --git a/tests/rsa.key b/tests/keys/rsa.pub
similarity index 100%
rename from tests/rsa.key
rename to tests/keys/rsa.pub
diff --git a/tests/signing-key.pub b/tests/keys/signing-key.pub
similarity index 100%
rename from tests/signing-key.pub
rename to tests/keys/signing-key.pub
diff --git a/tests/signing-key.sec b/tests/keys/signing-key.sec
similarity index 100%
rename from tests/signing-key.sec
rename to tests/keys/signing-key.sec
diff --git a/tests/openpgp.scm b/tests/openpgp.scm
index c2be26fa49..1f20466772 100644
--- a/tests/openpgp.scm
+++ b/tests/openpgp.scm
@@ -59,18 +59,22 @@ vBSFjNSiVHsuAA==
(define %civodul-fingerprint
"3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5")
-(define %civodul-key-id #x090B11993D9AEBB5) ;civodul.key
-
-;; Test keys. They were generated in a container along these lines:
-;; guix environment -CP --ad-hoc gnupg pinentry
-;; then, within the container:
-;; mkdir ~/.gnupg
-;; echo pinentry-program ~/.guix-profile/bin/pinentry-tty > ~/.gnupg/gpg-agent.conf
-;; gpg --quick-gen-key '<ludo+test-rsa@chbouib.org>' rsa
-;; or similar.
-(define %rsa-key-id #xAE25DA2A70DEED59) ;rsa.key
-(define %dsa-key-id #x587918047BE8BD2C) ;dsa.key
-(define %ed25519-key-id #x771F49CBFAAE072D) ;ed25519.key
+(define %civodul-key-id #x090B11993D9AEBB5) ;civodul.pub
+
+#|
+Test keys in ./tests/keys. They were generated in a container along these lines:
+ guix environment -CP --ad-hoc gnupg pinentry coreutils
+then, within the container:
+ mkdir ~/.gnupg && chmod -R og-rwx ~/.gnupg
+ gpg --batch --passphrase '' --quick-gen-key '<example@example.com>' ed25519
+ gpg --armor --export example@example.com
+ gpg --armor --export-secret-key example@example.com
+ # echo pinentry-program ~/.guix-profile/bin/pinentry-curses > ~/.gnupg/gpg-agent.conf
+or similar.
+|#
+(define %rsa-key-id #xAE25DA2A70DEED59) ;rsa.pub
+(define %dsa-key-id #x587918047BE8BD2C) ;dsa.pub
+(define %ed25519-key-id #x771F49CBFAAE072D) ;ed25519.pub
(define %rsa-key-fingerprint
(base16-string->bytevector
@@ -168,7 +172,7 @@ Pz7oopeN72xgggYUNT37ezqN3MeCqw0=
(not (port-ascii-armored? (open-bytevector-input-port %binary-sample))))
(test-assert "get-openpgp-keyring"
- (let* ((key (search-path %load-path "tests/civodul.key"))
+ (let* ((key (search-path %load-path "tests/keys/civodul.pub"))
(keyring (get-openpgp-keyring
(open-bytevector-input-port
(call-with-input-file key read-radix-64)))))
@@ -228,8 +232,10 @@ Pz7oopeN72xgggYUNT37ezqN3MeCqw0=
(verify-openpgp-signature signature keyring
(open-input-string "Hello!\n"))))
(list status (openpgp-public-key-id key)))))
- (list "tests/rsa.key" "tests/dsa.key"
- "tests/ed25519.key" "tests/ed25519.key" "tests/ed25519.key")
+ (list "tests/keys/rsa.pub" "tests/keys/dsa.pub"
+ "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub")
(list %hello-signature/rsa %hello-signature/dsa
%hello-signature/ed25519/sha256
%hello-signature/ed25519/sha512
@@ -248,9 +254,9 @@ Pz7oopeN72xgggYUNT37ezqN3MeCqw0=
(call-with-input-file key read-radix-64))
keyring)))
%empty-keyring
- '("tests/rsa.key" "tests/dsa.key"
- "tests/ed25519.key" "tests/ed25519.key"
- "tests/ed25519.key"))))
+ '("tests/keys/rsa.pub" "tests/keys/dsa.pub"
+ "tests/keys/ed25519.pub" "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub"))))
(map (lambda (signature)
(let ((signature (string->openpgp-packet signature)))
(let-values (((status key)
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 3/4] tests: Add failing test for .guix-authorizations and channel intro.
2021-09-28 1:05 ` [bug#50814] [PATCH 1/4] tests: Smarten up git repository testing framework Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 2/4] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
@ 2021-09-28 1:05 ` Attila Lendvai
2021-09-29 13:58 ` Maxime Devos
2021-09-28 1:05 ` [bug#50814] [PATCH 4/4] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
2 siblings, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 1:05 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
Will be fixed in a subsequent commit.
* tests/git-authenticate.scm: New test "signed commits, .guix-authorizations,
channel-introduction".
---
tests/git-authenticate.scm | 111 +++++++++++++++++++++++++++++++++++++
1 file changed, 111 insertions(+)
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index f66ef191b0..672aff2177 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -226,6 +226,117 @@
#:keyring-reference "master")
#f)))))))
+(unless (gpg+git-available?) (test-skip 1))
+(test-assert "signed commits, .guix-authorizations, channel-introduction"
+ (let* ((result #true)
+ (key1 %ed25519-public-key-file)
+ (key2 %ed25519-2-public-key-file)
+ (key3 %ed25519-3-public-key-file))
+ (with-fresh-gnupg-setup (list key1 %ed25519-secret-key-file
+ key2 %ed25519-2-secret-key-file
+ key3 %ed25519-3-secret-key-file)
+ (with-temporary-git-repository dir
+ `((checkout "keyring" orphan)
+ (add "signer1.key" ,(call-with-input-file key1 get-string-all))
+ (add "signer2.key" ,(call-with-input-file key2 get-string-all))
+ (add "signer3.key" ,(call-with-input-file key3 get-string-all))
+ (commit "keyring commit")
+
+ (checkout "main" orphan)
+ (add "noise0")
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))))))
+ (commit "commit 0" (signer ,(key-fingerprint key3)))
+ (add "noise1")
+ (commit "commit 1" (signer ,(key-fingerprint key1)))
+ (add "noise2")
+ (commit "commit 2" (signer ,(key-fingerprint key1))))
+ (with-repository dir repo
+ (let* ((commit-0 (find-commit repo "commit 0"))
+ (check-from
+ (lambda* (commit #:key (should-fail? #false) (key key1)
+ (historical-authorizations
+ ;; key3 is trusted to authorize commit 0
+ (list (key-fingerprint-vector key3))))
+ (guard (c ((unauthorized-commit-error? c)
+ (if should-fail?
+ c
+ (let ((port (current-output-port)))
+ (format port "FAILURE: Unexpected exception at commit '~s':~%"
+ commit)
+ (print-exception port (stack-ref (make-stack #t) 1)
+ c (exception-args c))
+ (set! result #false)
+ '()))))
+ (format #true "~%~%Checking ~s, should-fail? ~s, repo commits:~%"
+ commit should-fail?)
+ ;; to be able to inspect in the logs
+ (invoke "git" "-C" dir "log" "--reverse" "--pretty=oneline" "main")
+ (set! commit (find-commit repo commit))
+ (authenticate-repository
+ repo
+ (commit-id commit)
+ (key-fingerprint-vector key)
+ #:historical-authorizations historical-authorizations)
+ (when should-fail?
+ (format #t "FAILURE: Authenticating commit '~s' should have failed.~%" commit)
+ (set! result #false))
+ '()))))
+ (check-from "commit 0" #:key key3)
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (with-git-repository dir
+ `((add "noise 3")
+ ;; a commit with key2
+ (commit "commit 3" (signer ,(key-fingerprint key2))))
+ ;; Should fail because it is signed with key2, not key1
+ (check-from "commit 3" #:should-fail? #true)
+ ;; Specify commit 3 as a channel-introduction signed with
+ ;; key2. This is valid, but it should warn the user, because
+ ;; .guix-authorizations is not updated to include key2, which
+ ;; means that any subsequent commits with the same key will be
+ ;; rejected.
+ ;;
+ ;; TODO we should check somehow that a warning is issued
+ (check-from "commit 3" #:key key2))
+ (with-git-repository dir
+ `((reset ,(oid->string (commit-id (find-commit repo "commit 2"))))
+ (add "noise 4")
+ ;; set it up properly
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ (,(key-fingerprint key2) (name "Bob"))))))
+ (commit "commit 4" (signer ,(key-fingerprint key2))))
+ ;; This should fail because even though commit 4 adds key2 to
+ ;; .guix-authorizations, the commit itself is not authorized.
+ (check-from "commit 1" #:should-fail? #true)
+ ;; This should pass, because it's a valid channel intro at commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 5")
+ (commit "commit 5" (signer ,(key-fingerprint key2))))
+ ;; This is not very intuitive: because commit 4 has once been
+ ;; used as a channel intro, it got marked as trusted in the
+ ;; ~/.cache/, and because commit 1 is one of its parent, it is
+ ;; also trusted.
+ (check-from "commit 1")
+ (check-from "commit 2")
+ ;; Should still be fine, but only when starting from commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 6")
+ (commit "commit 6" (signer ,(key-fingerprint key1))))
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (check-from "commit 4" #:key key2))))))
+ result))
+
(unless (gpg+git-available?) (test-skip 1))
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 4/4] guix: git-authenticate: Fix authenticate-repository.
2021-09-28 1:05 ` [bug#50814] [PATCH 1/4] tests: Smarten up git repository testing framework Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 2/4] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
2021-09-28 1:05 ` [bug#50814] [PATCH 3/4] tests: Add failing test for .guix-authorizations and channel intro Attila Lendvai
@ 2021-09-28 1:05 ` Attila Lendvai
2 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 1:05 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
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.
* guix/git-authenticate.scm (authenticate-commit): Reword and extend the error
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(-)
diff --git a/guix/channels.scm b/guix/channels.scm
index e4e0428eb5..b84064537f 100644
--- a/guix/channels.scm
+++ b/guix/channels.scm
@@ -347,8 +347,8 @@ commits)...~%")
(progress-reporter/bar (length commits)))
(define authentic-commits
- ;; Consider the currently-used commit of CHANNEL as authentic so
- ;; authentication can skip it and all its closure.
+ ;; Optimization: consider the currently-used commit of CHANNEL as
+ ;; authentic, so that authentication can skip it and all its closure.
(match (find (lambda (candidate)
(eq? (channel-name candidate) (channel-name channel)))
(current-channels))
diff --git a/guix/git-authenticate.scm b/guix/git-authenticate.scm
index ab3fcd8b2f..713642d2ea 100644
--- a/guix/git-authenticate.scm
+++ b/guix/git-authenticate.scm
@@ -30,6 +30,7 @@
#:select (cache-directory with-atomic-file-output))
#:use-module ((guix build utils)
#:select (mkdir-p))
+ #:use-module (guix diagnostics)
#:use-module (guix progress)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-11)
@@ -38,6 +39,7 @@
#:use-module (srfi srfi-35)
#:use-module (rnrs bytevectors)
#:use-module (rnrs io ports)
+ #:use-module (ice-9 exceptions)
#:use-module (ice-9 match)
#:autoload (ice-9 pretty-print) (pretty-print)
#:export (read-authorizations
@@ -159,11 +161,10 @@ return a list of authorized fingerprints."
(string-downcase (string-filter char-set:graphic fingerprint))))
fingerprints))))
-(define* (commit-authorized-keys repository commit
- #:optional (default-authorizations '()))
- "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
-authorizations listed in its parent commits. If one of the parent commits
-does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
+(define (authorized-keys-at-commit repository commit default-authorizations)
+ "Return the list of authorized key fingerprints from the '.guix-authorizations'
+file at the given commit."
+
(define (parents-have-authorizations-file? commit)
;; Return true if at least one of the parents of COMMIT has the
;; '.guix-authorizations' file.
@@ -185,28 +186,35 @@ does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
to remove '.guix-authorizations' file")
(oid->string (commit-id commit)))))))
- (define (commit-authorizations commit)
- (catch 'git-error
- (lambda ()
- (let* ((tree (commit-tree commit))
- (entry (tree-entry-bypath tree ".guix-authorizations"))
- (blob (blob-lookup repository (tree-entry-id entry))))
- (read-authorizations
- (open-bytevector-input-port (blob-content blob)))))
- (lambda (key error)
- (if (= (git-error-code error) GIT_ENOTFOUND)
- (begin
- ;; Prevent removal of '.guix-authorizations' since it would make
- ;; it trivial to force a fallback to DEFAULT-AUTHORIZATIONS.
- (assert-parents-lack-authorizations commit)
- default-authorizations)
- (throw key error)))))
+ (catch 'git-error
+ (lambda ()
+ (let* ((tree (commit-tree commit))
+ (entry (tree-entry-bypath tree ".guix-authorizations"))
+ (blob (blob-lookup repository (tree-entry-id entry))))
+ (read-authorizations
+ (open-bytevector-input-port (blob-content blob)))))
+ (lambda (key error)
+ (if (= (git-error-code error) GIT_ENOTFOUND)
+ (begin
+ ;; Prevent removal of '.guix-authorizations' since it would make
+ ;; it trivial to force a fallback to DEFAULT-AUTHORIZATIONS.
+ (assert-parents-lack-authorizations commit)
+ default-authorizations)
+ (throw key error)))))
+(define* (commit-authorized-keys repository commit
+ #:optional (default-authorizations '()))
+ "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
+authorizations listed in its parent commits. If one of the parent commits
+does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
(match (commit-parents commit)
(() default-authorizations)
(parents
(apply lset-intersection bytevector=?
- (map commit-authorizations parents)))))
+ (map (lambda (commit)
+ (authorized-keys-at-commit repository commit
+ default-authorizations))
+ parents)))))
(define* (authenticate-commit repository commit keyring
#:key (default-authorizations '()))
@@ -236,8 +244,8 @@ not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
(condition
(&unauthorized-commit-error (commit id)
(signing-key signing-key)))
- (formatted-message (G_ "commit ~a not signed by an authorized \
-key: ~a")
+ (formatted-message (G_ "commit ~a is signed by an unauthorized \
+key: ~a\nSee info guix \"Specifying Channel Authorizations\".")
(oid->string id)
(openpgp-format-fingerprint
(openpgp-public-key-fingerprint
@@ -356,7 +364,8 @@ authenticated (only COMMIT-ID is written to cache, though)."
(base64-encode
(sha256 (string->utf8 (repository-directory repository))))))
-(define (verify-introductory-commit repository keyring commit expected-signer)
+(define (verify-introductory-commit repository commit expected-signer keyring
+ authorizations)
"Look up COMMIT in REPOSITORY, and raise an exception if it is not signed by
EXPECTED-SIGNER."
(define actual-signer
@@ -364,13 +373,25 @@ EXPECTED-SIGNER."
(commit-signing-key repository (commit-id commit) keyring)))
(unless (bytevector=? expected-signer actual-signer)
- (raise (formatted-message (G_ "initial commit ~a is signed by '~a' \
+ (raise (make-compound-condition
+ (condition (&unauthorized-commit-error (commit (commit-id commit))
+ (signing-key actual-signer)))
+ (formatted-message (G_ "initial commit ~a is signed by '~a' \
instead of '~a'")
- (oid->string (commit-id commit))
- (openpgp-format-fingerprint actual-signer)
- (openpgp-format-fingerprint expected-signer)))))
-
-(define* (authenticate-repository repository start signer
+ (oid->string (commit-id commit))
+ (openpgp-format-fingerprint actual-signer)
+ (openpgp-format-fingerprint expected-signer)))))
+ (unless (member actual-signer
+ (authorized-keys-at-commit repository commit authorizations)
+ bytevector=?)
+ ;; FIXME Is this the right way to tell the user about this situation? It
+ ;; would also be nice if the tests could assert for this warning.
+ (warning (G_ "initial commit ~a does not add \
+the key it is signed with (~a) to the '.guix-authorizations' file.")
+ (oid->string (commit-id commit))
+ (openpgp-format-fingerprint actual-signer))))
+
+(define* (authenticate-repository repository intro-commit-hash intro-signer
#:key
(keyring-reference "keyring")
(cache-key (repository-cache-key repository))
@@ -380,11 +401,12 @@ instead of '~a'")
(historical-authorizations '())
(make-reporter
(const progress-reporter/silent)))
- "Authenticate REPOSITORY up to commit END, an OID. Authentication starts
-with commit START, an OID, which must be signed by SIGNER; an exception is
-raised if that is not the case. Commits listed in AUTHENTIC-COMMITS and their
-closure are considered authentic. Return an alist mapping OpenPGP public keys
-to the number of commits signed by that key that have been traversed.
+ "Authenticate REPOSITORY up to commit END, an OID. Authentication starts with
+commit INTRO-COMMIT-HASH, an OID, which must be signed by INTRO-SIGNER; an
+exception is raised if that is not the case. Commits listed in
+AUTHENTIC-COMMITS and their closure are considered authentic. Return an
+alist mapping OpenPGP public keys to the number of commits signed by that
+key that have been traversed.
The OpenPGP keyring is loaded from KEYRING-REFERENCE in REPOSITORY, where
KEYRING-REFERENCE is the name of a branch. The list of authenticated commits
@@ -393,8 +415,10 @@ is cached in the authentication cache under CACHE-KEY.
HISTORICAL-AUTHORIZATIONS must be a list of OpenPGP fingerprints (bytevectors)
denoting the authorized keys for commits whose parent lack the
'.guix-authorizations' file."
- (define start-commit
- (commit-lookup repository start))
+
+ (define intro-commit
+ (commit-lookup repository intro-commit-hash))
+
(define end-commit
(commit-lookup repository end))
@@ -404,36 +428,37 @@ denoting the authorized keys for commits whose parent lack the
(define authenticated-commits
;; Previously-authenticated commits that don't need to be checked again.
(filter-map (lambda (id)
+ ;; We need to tolerate when cached commits disappear due to
+ ;; --allow-downgrades.
(false-if-git-not-found
(commit-lookup repository (string->oid id))))
(append (previously-authenticated-commits cache-key)
- authentic-commits)))
+ authentic-commits
+ ;; The intro commit is unconditionally trusted.
+ (list (oid->string intro-commit-hash)))))
(define commits
;; Commits to authenticate, excluding the closure of
;; AUTHENTICATED-COMMITS.
- (commit-difference end-commit start-commit
- authenticated-commits))
-
- ;; 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))
-
- (let ((stats (call-with-progress-reporter reporter
- (lambda (report)
- (authenticate-commits repository commits
- #:keyring keyring
- #:default-authorizations
- historical-authorizations
- #:report-progress report)))))
- (cache-authenticated-commit cache-key
- (oid->string (commit-id end-commit)))
-
- stats))))
+ (commit-difference end-commit intro-commit
+ authenticated-commits))
+
+ (verify-introductory-commit repository intro-commit
+ intro-signer keyring
+ historical-authorizations)
+
+ (let* ((reporter (make-reporter intro-commit end-commit commits))
+ (stats (call-with-progress-reporter reporter
+ (lambda (report)
+ (authenticate-commits repository commits
+ #:keyring keyring
+ #:default-authorizations
+ historical-authorizations
+ #:report-progress report)))))
+ ;; Note that this will make the then current end commit of any channel,
+ ;; that has been used/trusted in the past with a channel introduction,
+ ;; remain trusted until the cache is cleared.
+ (cache-authenticated-commit cache-key
+ (oid->string (commit-id end-commit)))
+
+ stats))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
` (2 preceding siblings ...)
2021-09-28 1:05 ` [bug#50814] [PATCH 1/4] tests: Smarten up git repository testing framework Attila Lendvai
@ 2021-09-28 16:24 ` Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
` (3 more replies)
2021-10-09 13:53 ` [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Ludovic Courtès
` (3 subsequent siblings)
7 siblings, 4 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 16:24 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
* guix/tests/git.scm (with-git-repository): New macro that can be used in
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/gnupg.scm (key-fingerprint-vector): New function.
---
guix/tests/git.scm | 23 +++++++++++++++++++++--
guix/tests/gnupg.scm | 8 ++++++--
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/guix/tests/git.scm b/guix/tests/git.scm
index 69960284d9..76f5a8b937 100644
--- a/guix/tests/git.scm
+++ b/guix/tests/git.scm
@@ -26,6 +26,7 @@
#:use-module (ice-9 control)
#:export (git-command
with-temporary-git-repository
+ with-git-repository
find-commit))
(define git-command
@@ -59,8 +60,9 @@ Return DIRECTORY on success."
(apply invoke (git-command) "-C" directory
command args)))))
- (mkdir-p directory)
- (git "init")
+ (unless (directory-exists? (string-append directory "/.git"))
+ (mkdir-p directory)
+ (git "init"))
(let loop ((directives directives))
(match directives
@@ -78,6 +80,9 @@ Return DIRECTORY on success."
port)))
(git "add" file)
(loop rest)))
+ ((('add file-name-and-content) rest ...)
+ (loop (cons `(add ,file-name-and-content ,file-name-and-content)
+ rest)))
((('remove file) rest ...)
(git "rm" "-f" file)
(loop rest))
@@ -99,12 +104,18 @@ Return DIRECTORY on success."
((('checkout branch) rest ...)
(git "checkout" branch)
(loop rest))
+ ((('checkout branch 'orphan) rest ...)
+ (git "checkout" "--orphan" branch)
+ (loop rest))
((('merge branch message) rest ...)
(git "merge" branch "-m" message)
(loop rest))
((('merge branch message ('signer fingerprint)) rest ...)
(git "merge" branch "-m" message
(string-append "--gpg-sign=" fingerprint))
+ (loop rest))
+ ((('reset to) rest ...)
+ (git "reset" "--hard" to)
(loop rest)))))
(define (call-with-temporary-git-repository directives proc)
@@ -121,6 +132,14 @@ per DIRECTIVES."
(lambda (directory)
exp ...)))
+(define-syntax-rule (with-git-repository directory
+ directives exp ...)
+ "Evaluate EXP in a context where DIRECTORY is (further) populated as
+per DIRECTIVES."
+ (begin
+ (populate-git-repository directory directives)
+ exp ...))
+
(define (find-commit repository message)
"Return the commit in REPOSITORY whose message includes MESSAGE, a string."
(let/ec return
diff --git a/guix/tests/gnupg.scm b/guix/tests/gnupg.scm
index eb8ff63a43..c7630db912 100644
--- a/guix/tests/gnupg.scm
+++ b/guix/tests/gnupg.scm
@@ -33,6 +33,7 @@
read-openpgp-packet
key-fingerprint
+ key-fingerprint-vector
key-id))
(define gpg-command
@@ -76,7 +77,10 @@ process is terminated afterwards."
(open-bytevector-input-port
(call-with-input-file file read-radix-64))))
+(define key-fingerprint-vector
+ (compose openpgp-public-key-fingerprint
+ read-openpgp-packet))
+
(define key-fingerprint
(compose openpgp-format-fingerprint
- openpgp-public-key-fingerprint
- read-openpgp-packet))
+ key-fingerprint-vector))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key.
2021-09-28 16:24 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
@ 2021-09-28 16:24 ` Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 3/5] tests: Add failing test for .guix-authorizations and channel intro Attila Lendvai
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 16:24 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
The third key will be used in an upcoming commit.
Rename public keys to .pub.
* guix/tests/gnupg.scm (%ed25519-3-public-key-file): New variable.
(%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.
* tests/keys/ed25519-3.key: New file.
* tests/keys/ed25519-3.sec: New 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%)
diff --git a/Makefile.am b/Makefile.am
index b66789fa0b..00604f2f93 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -643,16 +643,18 @@ EXTRA_DIST += \
build-aux/update-guix-package.scm \
build-aux/update-NEWS.scm \
tests/test.drv \
- tests/signing-key.pub \
- tests/signing-key.sec \
tests/cve-sample.json \
- tests/civodul.key \
- tests/rsa.key \
- tests/dsa.key \
- tests/ed25519.key \
- tests/ed25519.sec \
- tests/ed25519bis.key \
- tests/ed25519bis.sec \
+ tests/keys/signing-key.pub \
+ tests/keys/signing-key.sec \
+ tests/keys/civodul.pub \
+ tests/keys/rsa.pub \
+ tests/keys/dsa.pub \
+ tests/keys/ed25519.pub \
+ tests/keys/ed25519.sec \
+ tests/keys/ed25519-2.pub \
+ tests/keys/ed25519-2.sec \
+ tests/keys/ed25519-3.pub \
+ tests/keys/ed25519-3.sec \
build-aux/config.rpath \
bootstrap \
doc/build.scm \
diff --git a/build-aux/test-env.in b/build-aux/test-env.in
index 7efc43206c..ca786437e9 100644
--- a/build-aux/test-env.in
+++ b/build-aux/test-env.in
@@ -73,9 +73,9 @@ then
# Copy the keys so that the secret key has the right permissions (the
# daemon errors out when this is not the case.)
mkdir -p "$GUIX_CONFIGURATION_DIRECTORY"
- cp "@abs_top_srcdir@/tests/signing-key.sec" \
- "@abs_top_srcdir@/tests/signing-key.pub" \
- "$GUIX_CONFIGURATION_DIRECTORY"
+ cp "@abs_top_srcdir@/tests/keys/signing-key.sec" \
+ "@abs_top_srcdir@/tests/keys/signing-key.pub" \
+ "$GUIX_CONFIGURATION_DIRECTORY"
chmod 400 "$GUIX_CONFIGURATION_DIRECTORY/signing-key.sec"
fi
diff --git a/guix/tests/gnupg.scm b/guix/tests/gnupg.scm
index c7630db912..09f02a2b67 100644
--- a/guix/tests/gnupg.scm
+++ b/guix/tests/gnupg.scm
@@ -28,8 +28,10 @@
%ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file
+ %ed25519-3-public-key-file
+ %ed25519-3-secret-key-file
read-openpgp-packet
key-fingerprint
@@ -64,13 +66,17 @@ process is terminated afterwards."
(call-with-fresh-gnupg-setup imported (lambda () exp ...)))
(define %ed25519-public-key-file
- (search-path %load-path "tests/ed25519.key"))
+ (search-path %load-path "tests/keys/ed25519.pub"))
(define %ed25519-secret-key-file
- (search-path %load-path "tests/ed25519.sec"))
-(define %ed25519bis-public-key-file
- (search-path %load-path "tests/ed25519bis.key"))
-(define %ed25519bis-secret-key-file
- (search-path %load-path "tests/ed25519bis.sec"))
+ (search-path %load-path "tests/keys/ed25519.sec"))
+(define %ed25519-2-public-key-file
+ (search-path %load-path "tests/keys/ed25519-2.pub"))
+(define %ed25519-2-secret-key-file
+ (search-path %load-path "tests/keys/ed25519-2.sec"))
+(define %ed25519-3-public-key-file
+ (search-path %load-path "tests/keys/ed25519-3.pub"))
+(define %ed25519-3-secret-key-file
+ (search-path %load-path "tests/keys/ed25519-3.sec"))
(define (read-openpgp-packet file)
(get-openpgp-packet
diff --git a/tests/channels.scm b/tests/channels.scm
index 3e82315b0c..d45c450241 100644
--- a/tests/channels.scm
+++ b/tests/channels.scm
@@ -480,8 +480,8 @@
#t
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add ".guix-channel"
,(object->string
@@ -507,7 +507,7 @@
(commit-id-string commit1)
(openpgp-public-key-fingerprint
(read-openpgp-packet
- %ed25519bis-public-key-file)))) ;different key
+ %ed25519-2-public-key-file)))) ;different key
(channel (channel (name 'example)
(url (string-append "file://" directory))
(introduction intro))))
@@ -519,7 +519,7 @@
(oid->string (commit-id commit1))
(key-fingerprint %ed25519-public-key-file)
(key-fingerprint
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(authenticate-channel channel directory
(commit-id-string commit2)
#:keyring-reference-prefix "")
@@ -530,8 +530,8 @@
#t
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add ".guix-channel"
,(object->string
@@ -552,12 +552,12 @@
(signer ,(key-fingerprint %ed25519-public-key-file)))
(add "c.txt" "C")
(commit "third commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(branch "channel-keyring")
(checkout "channel-keyring")
(add "signer.key" ,(call-with-input-file %ed25519-public-key-file
get-string-all))
- (add "other.key" ,(call-with-input-file %ed25519bis-public-key-file
+ (add "other.key" ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(commit "keyring commit")
(checkout "master"))
@@ -588,7 +588,7 @@
(unauthorized-commit-error-signing-key c))
(openpgp-public-key-fingerprint
(read-openpgp-packet
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(authenticate-channel channel directory
(commit-id-string commit3)
#:keyring-reference-prefix "")
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index d87eacc659..f66ef191b0 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -161,14 +161,14 @@
(test-assert "signed commits, .guix-authorizations, unauthorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add "signer1.key"
,(call-with-input-file %ed25519-public-key-file
get-string-all))
(add "signer2.key"
- ,(call-with-input-file %ed25519bis-public-key-file
+ ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(add ".guix-authorizations"
,(object->string
@@ -184,7 +184,7 @@
(checkout "devel")
(add "devel/1.txt" "1")
(commit "first devel commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(checkout "master")
(add "b.txt" "B")
(commit "second commit"
@@ -203,7 +203,7 @@
(openpgp-public-key-fingerprint
(unauthorized-commit-error-signing-key c))
(openpgp-public-key-fingerprint
- (read-openpgp-packet %ed25519bis-public-key-file)))))
+ (read-openpgp-packet %ed25519-2-public-key-file)))))
(and (authenticate-commits repository (list master1 master2)
#:keyring-reference "master")
@@ -230,14 +230,14 @@
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add "signer1.key"
,(call-with-input-file %ed25519-public-key-file
get-string-all))
(add "signer2.key"
- ,(call-with-input-file %ed25519bis-public-key-file
+ ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(add ".guix-authorizations"
,(object->string
@@ -258,12 +258,12 @@
%ed25519-public-key-file)
(name "Alice"))
(,(key-fingerprint
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(commit "first devel commit"
(signer ,(key-fingerprint %ed25519-public-key-file)))
(add "devel/2.txt" "2")
(commit "second devel commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(checkout "master")
(add "b.txt" "B")
(commit "second commit"
@@ -273,7 +273,7 @@
;; After the merge, the second signer is authorized.
(add "c.txt" "C")
(commit "third commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file))))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file))))
(with-repository directory repository
(let ((master1 (find-commit repository "first commit"))
(master2 (find-commit repository "second commit"))
@@ -328,4 +328,3 @@
'failed)))))))
(test-end "git-authenticate")
-
diff --git a/tests/guix-authenticate.sh b/tests/guix-authenticate.sh
index 3a05b232c1..0de6da1878 100644
--- a/tests/guix-authenticate.sh
+++ b/tests/guix-authenticate.sh
@@ -28,7 +28,7 @@ rm -f "$sig" "$hash"
trap 'rm -f "$sig" "$hash"' EXIT
-key="$abs_top_srcdir/tests/signing-key.sec"
+key="$abs_top_srcdir/tests/keys/signing-key.sec"
key_len="`echo -n $key | wc -c`"
# A hexadecimal string as long as a sha256 hash.
@@ -67,7 +67,7 @@ test "$code" -ne 0
# encoded independently of the current locale: <https://bugs.gnu.org/43421>.
hash="636166e9636166e9636166e9636166e9636166e9636166e9636166e9636166e9"
latin1_cafe="caf$(printf '\351')"
-echo "sign 21:tests/signing-key.sec 64:$hash" | guix authenticate \
+echo "sign 26:tests/keys/signing-key.sec 64:$hash" | guix authenticate \
| LC_ALL=C grep "hash sha256 \"$latin1_cafe"
# Test for <http://bugs.gnu.org/17312>: make sure 'guix authenticate' produces
diff --git a/tests/civodul.key b/tests/keys/civodul.pub
similarity index 100%
rename from tests/civodul.key
rename to tests/keys/civodul.pub
diff --git a/tests/dsa.key b/tests/keys/dsa.pub
similarity index 100%
rename from tests/dsa.key
rename to tests/keys/dsa.pub
diff --git a/tests/ed25519bis.key b/tests/keys/ed25519-2.pub
similarity index 100%
rename from tests/ed25519bis.key
rename to tests/keys/ed25519-2.pub
diff --git a/tests/ed25519bis.sec b/tests/keys/ed25519-2.sec
similarity index 100%
rename from tests/ed25519bis.sec
rename to tests/keys/ed25519-2.sec
diff --git a/tests/keys/ed25519-3.pub b/tests/keys/ed25519-3.pub
new file mode 100644
index 0000000000..72f311984c
--- /dev/null
+++ b/tests/keys/ed25519-3.pub
@@ -0,0 +1,9 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+mDMEYVH/7xYJKwYBBAHaRw8BAQdALMLeUhjEG2/UPCJj2j/debFwwAK5gT3G0l5d
+ILfFldm0FTxleGFtcGxlQGV4YW1wbGUuY29tPoiWBBMWCAA+FiEEjO6M85jMSK68
+7tINGBzA7NyoagkFAmFR/+8CGwMFCQPCZwAFCwkIBwIGFQoJCAsCBBYCAwECHgEC
+F4AACgkQGBzA7Nyoagl3lgEAw6yqIlX11lTqwxBGhZk/Oy34O13cbJSZCGv+m0ja
++hcA/3DCNOmT+oXjgO/w6enQZUQ1m/d6dUjCc2wOLlLz+ZoG
+=+r3i
+-----END PGP PUBLIC KEY BLOCK-----
diff --git a/tests/keys/ed25519-3.sec b/tests/keys/ed25519-3.sec
new file mode 100644
index 0000000000..04128a4131
--- /dev/null
+++ b/tests/keys/ed25519-3.sec
@@ -0,0 +1,10 @@
+-----BEGIN PGP PRIVATE KEY BLOCK-----
+
+lFgEYVH/7xYJKwYBBAHaRw8BAQdALMLeUhjEG2/UPCJj2j/debFwwAK5gT3G0l5d
+ILfFldkAAP92goSbbzQ0ttElr9lr5Cm6rmQtqUZ2Cu/Jk9fvfZROwxI0tBU8ZXhh
+bXBsZUBleGFtcGxlLmNvbT6IlgQTFggAPhYhBIzujPOYzEiuvO7SDRgcwOzcqGoJ
+BQJhUf/vAhsDBQkDwmcABQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEBgcwOzc
+qGoJd5YBAMOsqiJV9dZU6sMQRoWZPzst+Dtd3GyUmQhr/ptI2voXAP9wwjTpk/qF
+44Dv8Onp0GVENZv3enVIwnNsDi5S8/maBg==
+=EmOt
+-----END PGP PRIVATE KEY BLOCK-----
diff --git a/tests/ed25519.key b/tests/keys/ed25519.pub
similarity index 100%
rename from tests/ed25519.key
rename to tests/keys/ed25519.pub
diff --git a/tests/ed25519.sec b/tests/keys/ed25519.sec
similarity index 100%
rename from tests/ed25519.sec
rename to tests/keys/ed25519.sec
diff --git a/tests/rsa.key b/tests/keys/rsa.pub
similarity index 100%
rename from tests/rsa.key
rename to tests/keys/rsa.pub
diff --git a/tests/signing-key.pub b/tests/keys/signing-key.pub
similarity index 100%
rename from tests/signing-key.pub
rename to tests/keys/signing-key.pub
diff --git a/tests/signing-key.sec b/tests/keys/signing-key.sec
similarity index 100%
rename from tests/signing-key.sec
rename to tests/keys/signing-key.sec
diff --git a/tests/openpgp.scm b/tests/openpgp.scm
index c2be26fa49..1f20466772 100644
--- a/tests/openpgp.scm
+++ b/tests/openpgp.scm
@@ -59,18 +59,22 @@ vBSFjNSiVHsuAA==
(define %civodul-fingerprint
"3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5")
-(define %civodul-key-id #x090B11993D9AEBB5) ;civodul.key
-
-;; Test keys. They were generated in a container along these lines:
-;; guix environment -CP --ad-hoc gnupg pinentry
-;; then, within the container:
-;; mkdir ~/.gnupg
-;; echo pinentry-program ~/.guix-profile/bin/pinentry-tty > ~/.gnupg/gpg-agent.conf
-;; gpg --quick-gen-key '<ludo+test-rsa@chbouib.org>' rsa
-;; or similar.
-(define %rsa-key-id #xAE25DA2A70DEED59) ;rsa.key
-(define %dsa-key-id #x587918047BE8BD2C) ;dsa.key
-(define %ed25519-key-id #x771F49CBFAAE072D) ;ed25519.key
+(define %civodul-key-id #x090B11993D9AEBB5) ;civodul.pub
+
+#|
+Test keys in ./tests/keys. They were generated in a container along these lines:
+ guix environment -CP --ad-hoc gnupg pinentry coreutils
+then, within the container:
+ mkdir ~/.gnupg && chmod -R og-rwx ~/.gnupg
+ gpg --batch --passphrase '' --quick-gen-key '<example@example.com>' ed25519
+ gpg --armor --export example@example.com
+ gpg --armor --export-secret-key example@example.com
+ # echo pinentry-program ~/.guix-profile/bin/pinentry-curses > ~/.gnupg/gpg-agent.conf
+or similar.
+|#
+(define %rsa-key-id #xAE25DA2A70DEED59) ;rsa.pub
+(define %dsa-key-id #x587918047BE8BD2C) ;dsa.pub
+(define %ed25519-key-id #x771F49CBFAAE072D) ;ed25519.pub
(define %rsa-key-fingerprint
(base16-string->bytevector
@@ -168,7 +172,7 @@ Pz7oopeN72xgggYUNT37ezqN3MeCqw0=
(not (port-ascii-armored? (open-bytevector-input-port %binary-sample))))
(test-assert "get-openpgp-keyring"
- (let* ((key (search-path %load-path "tests/civodul.key"))
+ (let* ((key (search-path %load-path "tests/keys/civodul.pub"))
(keyring (get-openpgp-keyring
(open-bytevector-input-port
(call-with-input-file key read-radix-64)))))
@@ -228,8 +232,10 @@ Pz7oopeN72xgggYUNT37ezqN3MeCqw0=
(verify-openpgp-signature signature keyring
(open-input-string "Hello!\n"))))
(list status (openpgp-public-key-id key)))))
- (list "tests/rsa.key" "tests/dsa.key"
- "tests/ed25519.key" "tests/ed25519.key" "tests/ed25519.key")
+ (list "tests/keys/rsa.pub" "tests/keys/dsa.pub"
+ "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub")
(list %hello-signature/rsa %hello-signature/dsa
%hello-signature/ed25519/sha256
%hello-signature/ed25519/sha512
@@ -248,9 +254,9 @@ Pz7oopeN72xgggYUNT37ezqN3MeCqw0=
(call-with-input-file key read-radix-64))
keyring)))
%empty-keyring
- '("tests/rsa.key" "tests/dsa.key"
- "tests/ed25519.key" "tests/ed25519.key"
- "tests/ed25519.key"))))
+ '("tests/keys/rsa.pub" "tests/keys/dsa.pub"
+ "tests/keys/ed25519.pub" "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub"))))
(map (lambda (signature)
(let ((signature (string->openpgp-packet signature)))
(let-values (((status key)
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 3/5] tests: Add failing test for .guix-authorizations and channel intro.
2021-09-28 16:24 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
@ 2021-09-28 16:24 ` Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 5/5] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
3 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 16:24 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
Will be fixed in a subsequent commit.
* tests/git-authenticate.scm: New test "signed commits, .guix-authorizations,
channel-introduction".
---
tests/git-authenticate.scm | 132 +++++++++++++++++++++++++++++++++++++
1 file changed, 132 insertions(+)
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index f66ef191b0..745a6d6dbe 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -18,6 +18,7 @@
(define-module (test-git-authenticate)
#:use-module (git)
+ #:use-module (guix diagnostics)
#:use-module (guix git)
#:use-module (guix git-authenticate)
#:use-module (guix openpgp)
@@ -28,6 +29,10 @@
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-64)
#:use-module (rnrs bytevectors)
+ #:use-module ((rnrs conditions)
+ #:select (warning?))
+ #:use-module ((rnrs exceptions)
+ #:select (with-exception-handler))
#:use-module (rnrs io ports))
;; Test the (guix git-authenticate) tools.
@@ -226,6 +231,133 @@
#:keyring-reference "master")
#f)))))))
+(unless (gpg+git-available?) (test-skip 1))
+(test-assert "signed commits, .guix-authorizations, channel-introduction"
+ (let* ((result #true)
+ (key1 %ed25519-public-key-file)
+ (key2 %ed25519-2-public-key-file)
+ (key3 %ed25519-3-public-key-file))
+ (with-fresh-gnupg-setup (list key1 %ed25519-secret-key-file
+ key2 %ed25519-2-secret-key-file
+ key3 %ed25519-3-secret-key-file)
+ (with-temporary-git-repository dir
+ `((checkout "keyring" orphan)
+ (add "signer1.key" ,(call-with-input-file key1 get-string-all))
+ (add "signer2.key" ,(call-with-input-file key2 get-string-all))
+ (add "signer3.key" ,(call-with-input-file key3 get-string-all))
+ (commit "keyring commit")
+
+ (checkout "main" orphan)
+ (add "noise0")
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ (,(key-fingerprint key3) (name "Charlie"))))))
+ (commit "commit 0" (signer ,(key-fingerprint key3)))
+ (add "noise1")
+ (commit "commit 1" (signer ,(key-fingerprint key1)))
+ (add "noise2")
+ (commit "commit 2" (signer ,(key-fingerprint key1))))
+ (with-repository dir repo
+ (let* ((commit-0 (find-commit repo "commit 0"))
+ (check-from
+ (lambda* (commit #:key (should-fail? #false) (key key1)
+ (historical-authorizations
+ ;; key3 is trusted to authorize commit 0
+ (list (key-fingerprint-vector key3))))
+ (guard (c ((unauthorized-commit-error? c)
+ (if should-fail?
+ c
+ (let ((port (current-output-port)))
+ (format port "FAILURE: Unexpected exception at commit '~s':~%"
+ commit)
+ (print-exception port (stack-ref (make-stack #t) 1)
+ c (exception-args c))
+ (set! result #false)
+ '()))))
+ (format #true "~%~%Checking ~s, should-fail? ~s, repo commits:~%"
+ commit should-fail?)
+ ;; to be able to inspect in the logs
+ (invoke "git" "-C" dir "log" "--reverse" "--pretty=oneline" "main")
+ (set! commit (find-commit repo commit))
+ (authenticate-repository
+ repo
+ (commit-id commit)
+ (key-fingerprint-vector key)
+ #:historical-authorizations historical-authorizations)
+ (when should-fail?
+ (format #t "FAILURE: Authenticating commit '~s' should have failed.~%" commit)
+ (set! result #false))
+ '()))))
+ (check-from "commit 0" #:key key3)
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (with-git-repository dir
+ `((add "noise 3")
+ ;; a commit with key2
+ (commit "commit 3" (signer ,(key-fingerprint key2))))
+ ;; Should fail because it is signed with key2, not key1
+ (check-from "commit 3" #:should-fail? #true)
+ ;; Specify commit 3 as a channel-introduction signed with
+ ;; key2. This is valid, but it should warn the user, because
+ ;; .guix-authorizations is not updated to include key2, which
+ ;; means that any subsequent commits with the same key will be
+ ;; rejected.
+ (set! result
+ (and result
+ (let ((signalled? #false))
+ (with-exception-handler
+ (lambda (c)
+ (cond
+ ((not (warning? c))
+ (raise c))
+ ((formatted-message? c)
+ (format #true "warning (expected): ~a~%"
+ (apply format #false
+ (formatted-message-string c)
+ (formatted-message-arguments c)))
+ (set! signalled? #true)))
+ '())
+ (lambda ()
+ (check-from "commit 3" #:key key2)
+ signalled?))))))
+ (with-git-repository dir
+ `((reset ,(oid->string (commit-id (find-commit repo "commit 2"))))
+ (add "noise 4")
+ ;; set it up properly
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ (,(key-fingerprint key2) (name "Bob"))))))
+ (commit "commit 4" (signer ,(key-fingerprint key2))))
+ ;; This should fail because even though commit 4 adds key2 to
+ ;; .guix-authorizations, the commit itself is not authorized.
+ (check-from "commit 1" #:should-fail? #true)
+ ;; This should pass, because it's a valid channel intro at commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 5")
+ (commit "commit 5" (signer ,(key-fingerprint key2))))
+ ;; This is not very intuitive: because commit 4 has once been
+ ;; used as a channel intro, it got marked as trusted in the
+ ;; ~/.cache/, and because commit 1 is one of its parent, it is
+ ;; also trusted.
+ (check-from "commit 1")
+ (check-from "commit 2")
+ ;; Should still be fine, but only when starting from commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 6")
+ (commit "commit 6" (signer ,(key-fingerprint key1))))
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (check-from "commit 4" #:key key2))))))
+ result))
+
(unless (gpg+git-available?) (test-skip 1))
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions.
2021-09-28 16:24 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
2021-09-28 16:24 ` [bug#50814] [PATCH 3/5] tests: Add failing test for .guix-authorizations and channel intro Attila Lendvai
@ 2021-09-28 16:24 ` Attila Lendvai
2021-09-29 14:13 ` Maxime Devos
2021-09-28 16:24 ` [bug#50814] [PATCH 5/5] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
3 siblings, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 16:24 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
* guix/store.scm (call-with-store): Use dynamic-wind so that continuable
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.
* guix/ui.scm (call-with-error-handling): Handle &warning type exceptions by
printing them to the user, and then continuing at the place they were
signalled at.
* guix/diagnostics.scm (emit-formatted-warning): New exported
function.
---
guix/diagnostics.scm | 4 ++++
guix/store.scm | 16 ++++++++++------
guix/ui.scm | 11 ++++++++++-
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/guix/diagnostics.scm b/guix/diagnostics.scm
index 6a792febd4..343213fb45 100644
--- a/guix/diagnostics.scm
+++ b/guix/diagnostics.scm
@@ -48,6 +48,7 @@
formatted-message?
formatted-message-string
formatted-message-arguments
+ emit-formatted-warning
&fix-hint
fix-hint?
@@ -161,6 +162,9 @@ messages."
(report-error args ...)
(exit 1)))
+(define* (emit-formatted-warning fmt . args)
+ (emit-diagnostic fmt args #:prefix (G_ "warning: ") #:colors %warning-color))
+
(define* (emit-diagnostic fmt args
#:key location (colors (color)) (prefix ""))
"Report diagnostic message FMT with the given ARGS and the specified
diff --git a/guix/store.scm b/guix/store.scm
index 89a719bcfc..33d4039037 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -45,6 +45,8 @@
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
#:use-module (srfi srfi-39)
+ #:use-module ((rnrs conditions)
+ #:select (warning?))
#:use-module (ice-9 match)
#:use-module (ice-9 vlist)
#:use-module (ice-9 popen)
@@ -651,19 +653,21 @@ connection. Use with care."
(define (call-with-store proc)
"Call PROC with an open store connection."
- (let ((store (open-connection)))
+ (let ((store '()))
(define (thunk)
(parameterize ((current-store-protocol-version
(store-connection-version store)))
(call-with-values (lambda () (proc store))
(lambda results
- (close-connection store)
(apply values results)))))
- (with-exception-handler (lambda (exception)
- (close-connection store)
- (raise-exception exception))
- thunk)))
+ (dynamic-wind
+ (lambda ()
+ (set! store (open-connection)))
+ thunk
+ (lambda ()
+ (close-connection store)
+ (set! store '())))))
(define-syntax-rule (with-store store exp ...)
"Bind STORE to an open connection to the store and evaluate EXPs;
diff --git a/guix/ui.scm b/guix/ui.scm
index 1428c254b3..88940f99ef 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -69,6 +69,8 @@
#:use-module (srfi srfi-31)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
+ #:use-module ((rnrs conditions)
+ #:select (warning?))
#:autoload (ice-9 ftw) (scandir)
#:use-module (ice-9 match)
#:use-module (ice-9 format)
@@ -689,7 +691,14 @@ evaluating the tests and bodies of CLAUSES."
(and (not (port-closed? port))
(port-filename port)))
- (guard* (c ((package-input-error? c)
+ (guard* (c ((warning? c)
+ (if (formatted-message? c)
+ (apply emit-formatted-warning
+ (formatted-message-string c)
+ (formatted-message-arguments c))
+ (emit-formatted-warning "~a" c))
+ '())
+ ((package-input-error? c)
(let* ((package (package-error-package c))
(input (package-error-invalid-input c))
(location (package-location package))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions.
2021-09-28 16:24 ` [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions Attila Lendvai
@ 2021-09-29 14:13 ` Maxime Devos
2021-09-29 14:50 ` Attila Lendvai
0 siblings, 1 reply; 38+ messages in thread
From: Maxime Devos @ 2021-09-29 14:13 UTC (permalink / raw)
To: Attila Lendvai, 50814
[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]
Attila Lendvai schreef op di 28-09-2021 om 18:24 [+0200]:
> (define (call-with-store proc)
> "Call PROC with an open store connection."
> - (let ((store (open-connection)))
> + (let ((store '()))
> (define (thunk)
> (parameterize ((current-store-protocol-version
> (store-connection-version store)))
> (call-with-values (lambda () (proc store))
> (lambda results
> - (close-connection store)
> (apply values results)))))
>
> - (with-exception-handler (lambda (exception)
> - (close-connection store)
> - (raise-exception exception))
> - thunk)))
> + (dynamic-wind
> + (lambda ()
> + (set! store (open-connection)))
> + thunk
> + (lambda ()
> + (close-connection store)
> + (set! store '())))))
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
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions.
2021-09-29 14:13 ` Maxime Devos
@ 2021-09-29 14:50 ` Attila Lendvai
2021-09-29 20:36 ` Maxime Devos
0 siblings, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-09-29 14:50 UTC (permalink / raw)
To: Maxime Devos; +Cc: 50814
> 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.
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions.
2021-09-29 14:50 ` Attila Lendvai
@ 2021-09-29 20:36 ` Maxime Devos
2021-09-29 21:22 ` Attila Lendvai
0 siblings, 1 reply; 38+ messages in thread
From: Maxime Devos @ 2021-09-29 20:36 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]
Attila Lendvai schreef op wo 29-09-2021 om 14:50 [+0000]:
> > 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.
>
> 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?
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.
> if 2) then i'll try to smarten up the handler to use raise-continuable
> if the exception is of type &warning.
That should work. Or simpler: always use raise-continuable.
> or any better ideas?
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
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions.
2021-09-29 20:36 ` Maxime Devos
@ 2021-09-29 21:22 ` Attila Lendvai
2021-09-29 22:03 ` Maxime Devos
0 siblings, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-09-29 21:22 UTC (permalink / raw)
To: Maxime Devos; +Cc: 50814
> 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.
ok, so this is a no-go. thanks for the clarification!
> 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?
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions.
2021-09-29 21:22 ` Attila Lendvai
@ 2021-09-29 22:03 ` Maxime Devos
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Devos @ 2021-09-29 22:03 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
[-- Attachment #1: Type: text/plain, Size: 974 bytes --]
Attila Lendvai schreef op wo 29-09-2021 om 21:22 [+0000]:
> >
[...]
>
> > 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?
>
> 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?
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.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH 5/5] guix: git-authenticate: Fix authenticate-repository.
2021-09-28 16:24 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
` (2 preceding siblings ...)
2021-09-28 16:24 ` [bug#50814] [PATCH 4/5] guix: Prepare the UI for continuable &warning exceptions Attila Lendvai
@ 2021-09-28 16:24 ` Attila Lendvai
2021-09-29 23:14 ` Maxime Devos
3 siblings, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-09-28 16:24 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
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.
* guix/git-authenticate.scm (authenticate-commit): Reword and extend the error
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(-)
diff --git a/guix/channels.scm b/guix/channels.scm
index e4e0428eb5..b84064537f 100644
--- a/guix/channels.scm
+++ b/guix/channels.scm
@@ -347,8 +347,8 @@ commits)...~%")
(progress-reporter/bar (length commits)))
(define authentic-commits
- ;; Consider the currently-used commit of CHANNEL as authentic so
- ;; authentication can skip it and all its closure.
+ ;; Optimization: consider the currently-used commit of CHANNEL as
+ ;; authentic, so that authentication can skip it and all its closure.
(match (find (lambda (candidate)
(eq? (channel-name candidate) (channel-name channel)))
(current-channels))
diff --git a/guix/git-authenticate.scm b/guix/git-authenticate.scm
index ab3fcd8b2f..b2821a45ad 100644
--- a/guix/git-authenticate.scm
+++ b/guix/git-authenticate.scm
@@ -30,6 +30,7 @@
#:select (cache-directory with-atomic-file-output))
#:use-module ((guix build utils)
#:select (mkdir-p))
+ #:use-module (guix diagnostics)
#:use-module (guix progress)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-11)
@@ -37,7 +38,10 @@
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
#:use-module (rnrs bytevectors)
+ #:use-module ((rnrs exceptions)
+ #:select (raise-continuable))
#:use-module (rnrs io ports)
+ #:use-module (ice-9 exceptions)
#:use-module (ice-9 match)
#:autoload (ice-9 pretty-print) (pretty-print)
#:export (read-authorizations
@@ -159,11 +163,10 @@ return a list of authorized fingerprints."
(string-downcase (string-filter char-set:graphic fingerprint))))
fingerprints))))
-(define* (commit-authorized-keys repository commit
- #:optional (default-authorizations '()))
- "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
-authorizations listed in its parent commits. If one of the parent commits
-does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
+(define (authorized-keys-at-commit repository commit default-authorizations)
+ "Return the list of authorized key fingerprints from the '.guix-authorizations'
+file at the given commit."
+
(define (parents-have-authorizations-file? commit)
;; Return true if at least one of the parents of COMMIT has the
;; '.guix-authorizations' file.
@@ -185,28 +188,35 @@ does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
to remove '.guix-authorizations' file")
(oid->string (commit-id commit)))))))
- (define (commit-authorizations commit)
- (catch 'git-error
- (lambda ()
- (let* ((tree (commit-tree commit))
- (entry (tree-entry-bypath tree ".guix-authorizations"))
- (blob (blob-lookup repository (tree-entry-id entry))))
- (read-authorizations
- (open-bytevector-input-port (blob-content blob)))))
- (lambda (key error)
- (if (= (git-error-code error) GIT_ENOTFOUND)
- (begin
- ;; Prevent removal of '.guix-authorizations' since it would make
- ;; it trivial to force a fallback to DEFAULT-AUTHORIZATIONS.
- (assert-parents-lack-authorizations commit)
- default-authorizations)
- (throw key error)))))
+ (catch 'git-error
+ (lambda ()
+ (let* ((tree (commit-tree commit))
+ (entry (tree-entry-bypath tree ".guix-authorizations"))
+ (blob (blob-lookup repository (tree-entry-id entry))))
+ (read-authorizations
+ (open-bytevector-input-port (blob-content blob)))))
+ (lambda (key error)
+ (if (= (git-error-code error) GIT_ENOTFOUND)
+ (begin
+ ;; Prevent removal of '.guix-authorizations' since it would make
+ ;; it trivial to force a fallback to DEFAULT-AUTHORIZATIONS.
+ (assert-parents-lack-authorizations commit)
+ default-authorizations)
+ (throw key error)))))
+(define* (commit-authorized-keys repository commit
+ #:optional (default-authorizations '()))
+ "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
+authorizations listed in its parent commits. If one of the parent commits
+does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
(match (commit-parents commit)
(() default-authorizations)
(parents
(apply lset-intersection bytevector=?
- (map commit-authorizations parents)))))
+ (map (lambda (commit)
+ (authorized-keys-at-commit repository commit
+ default-authorizations))
+ parents)))))
(define* (authenticate-commit repository commit keyring
#:key (default-authorizations '()))
@@ -236,8 +246,8 @@ not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
(condition
(&unauthorized-commit-error (commit id)
(signing-key signing-key)))
- (formatted-message (G_ "commit ~a not signed by an authorized \
-key: ~a")
+ (formatted-message (G_ "commit ~a is signed by an unauthorized \
+key: ~a\nSee info guix \"Specifying Channel Authorizations\".")
(oid->string id)
(openpgp-format-fingerprint
(openpgp-public-key-fingerprint
@@ -356,7 +366,8 @@ authenticated (only COMMIT-ID is written to cache, though)."
(base64-encode
(sha256 (string->utf8 (repository-directory repository))))))
-(define (verify-introductory-commit repository keyring commit expected-signer)
+(define (verify-introductory-commit repository commit expected-signer keyring
+ authorizations)
"Look up COMMIT in REPOSITORY, and raise an exception if it is not signed by
EXPECTED-SIGNER."
(define actual-signer
@@ -364,13 +375,26 @@ EXPECTED-SIGNER."
(commit-signing-key repository (commit-id commit) keyring)))
(unless (bytevector=? expected-signer actual-signer)
- (raise (formatted-message (G_ "initial commit ~a is signed by '~a' \
+ (raise (make-compound-condition
+ (condition (&unauthorized-commit-error (commit (commit-id commit))
+ (signing-key actual-signer)))
+ (formatted-message (G_ "initial commit ~a is signed by '~a' \
instead of '~a'")
- (oid->string (commit-id commit))
- (openpgp-format-fingerprint actual-signer)
- (openpgp-format-fingerprint expected-signer)))))
-
-(define* (authenticate-repository repository start signer
+ (oid->string (commit-id commit))
+ (openpgp-format-fingerprint actual-signer)
+ (openpgp-format-fingerprint expected-signer)))))
+ (unless (member actual-signer
+ (authorized-keys-at-commit repository commit authorizations)
+ bytevector=?)
+ (raise-continuable
+ (make-compound-condition
+ (condition (&warning))
+ (formatted-message (G_ "initial commit ~a does not add \
+the key it is signed with (~a) to the '.guix-authorizations' file.")
+ (oid->string (commit-id commit))
+ (openpgp-format-fingerprint actual-signer))))))
+
+(define* (authenticate-repository repository intro-commit-hash intro-signer
#:key
(keyring-reference "keyring")
(cache-key (repository-cache-key repository))
@@ -380,11 +404,12 @@ instead of '~a'")
(historical-authorizations '())
(make-reporter
(const progress-reporter/silent)))
- "Authenticate REPOSITORY up to commit END, an OID. Authentication starts
-with commit START, an OID, which must be signed by SIGNER; an exception is
-raised if that is not the case. Commits listed in AUTHENTIC-COMMITS and their
-closure are considered authentic. Return an alist mapping OpenPGP public keys
-to the number of commits signed by that key that have been traversed.
+ "Authenticate REPOSITORY up to commit END, an OID. Authentication starts with
+commit INTRO-COMMIT-HASH, an OID, which must be signed by INTRO-SIGNER; an
+exception is raised if that is not the case. Commits listed in
+AUTHENTIC-COMMITS and their closure are considered authentic. Return an
+alist mapping OpenPGP public keys to the number of commits signed by that
+key that have been traversed.
The OpenPGP keyring is loaded from KEYRING-REFERENCE in REPOSITORY, where
KEYRING-REFERENCE is the name of a branch. The list of authenticated commits
@@ -393,8 +418,10 @@ is cached in the authentication cache under CACHE-KEY.
HISTORICAL-AUTHORIZATIONS must be a list of OpenPGP fingerprints (bytevectors)
denoting the authorized keys for commits whose parent lack the
'.guix-authorizations' file."
- (define start-commit
- (commit-lookup repository start))
+
+ (define intro-commit
+ (commit-lookup repository intro-commit-hash))
+
(define end-commit
(commit-lookup repository end))
@@ -404,36 +431,37 @@ denoting the authorized keys for commits whose parent lack the
(define authenticated-commits
;; Previously-authenticated commits that don't need to be checked again.
(filter-map (lambda (id)
+ ;; We need to tolerate when cached commits disappear due to
+ ;; --allow-downgrades.
(false-if-git-not-found
(commit-lookup repository (string->oid id))))
(append (previously-authenticated-commits cache-key)
- authentic-commits)))
+ authentic-commits
+ ;; The intro commit is unconditionally trusted.
+ (list (oid->string intro-commit-hash)))))
(define commits
;; Commits to authenticate, excluding the closure of
;; AUTHENTICATED-COMMITS.
- (commit-difference end-commit start-commit
- authenticated-commits))
-
- ;; 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))
-
- (let ((stats (call-with-progress-reporter reporter
- (lambda (report)
- (authenticate-commits repository commits
- #:keyring keyring
- #:default-authorizations
- historical-authorizations
- #:report-progress report)))))
- (cache-authenticated-commit cache-key
- (oid->string (commit-id end-commit)))
-
- stats))))
+ (commit-difference end-commit intro-commit
+ authenticated-commits))
+
+ (verify-introductory-commit repository intro-commit
+ intro-signer keyring
+ historical-authorizations)
+
+ (let* ((reporter (make-reporter intro-commit end-commit commits))
+ (stats (call-with-progress-reporter reporter
+ (lambda (report)
+ (authenticate-commits repository commits
+ #:keyring keyring
+ #:default-authorizations
+ historical-authorizations
+ #:report-progress report)))))
+ ;; Note that this will make the then current end commit of any channel,
+ ;; that has been used/trusted in the past with a channel introduction,
+ ;; remain trusted until the cache is cleared.
+ (cache-authenticated-commit cache-key
+ (oid->string (commit-id end-commit)))
+
+ stats))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 5/5] guix: git-authenticate: Fix authenticate-repository.
2021-09-28 16:24 ` [bug#50814] [PATCH 5/5] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
@ 2021-09-29 23:14 ` Maxime Devos
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Devos @ 2021-09-29 23:14 UTC (permalink / raw)
To: Attila Lendvai, 50814
[-- Attachment #1: Type: text/plain, Size: 969 bytes --]
Attila Lendvai schreef op di 28-09-2021 om 18:24 [+0200]:
> [...]
> -(define* (commit-authorized-keys repository commit
> - #:optional (default-authorizations '()))
> - "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
> -authorizations listed in its parent commits. If one of the parent commits
> -does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
> +(define (authorized-keys-at-commit repository commit default-authorizations)
> + "Return the list of authorized key fingerprints from the '.guix-authorizations'
> +file at the given commit."
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
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
` (3 preceding siblings ...)
2021-09-28 16:24 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
@ 2021-10-09 13:53 ` Ludovic Courtès
2021-10-09 15:31 ` Attila Lendvai
2021-10-10 14:15 ` [bug#50814] [PATCH] tests: Add test for .guix-authorizations and channel intro Attila Lendvai
` (2 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Ludovic Courtès @ 2021-10-09 13:53 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
Hi Attila,
Attila Lendvai <attila@lendvai.name> skribis:
> * guix/git-authenticate.scm (authenticate-commit): Reword and extend the error
> 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.
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
^ permalink raw reply [flat|nested] 38+ messages in thread* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-09 13:53 ` [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Ludovic Courtès
@ 2021-10-09 15:31 ` Attila Lendvai
2021-10-12 9:39 ` Ludovic Courtès
2021-10-17 10:09 ` Attila Lendvai
0 siblings, 2 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-09 15:31 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 50814
> Does that make sense?
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-09 15:31 ` Attila Lendvai
@ 2021-10-12 9:39 ` Ludovic Courtès
2021-10-17 10:09 ` Attila Lendvai
1 sibling, 0 replies; 38+ messages in thread
From: Ludovic Courtès @ 2021-10-12 9:39 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
Hi,
Attila Lendvai <attila@lendvai.name> skribis:
> 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
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. :-)
> 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.
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
> 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.
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’.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-09 15:31 ` Attila Lendvai
2021-10-12 9:39 ` Ludovic Courtès
@ 2021-10-17 10:09 ` Attila Lendvai
2021-10-18 9:10 ` Ludovic Courtès
1 sibling, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-10-17 10:09 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 50814
> 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.
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.
> 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.
understood. the problem is that it all started out as adding a
warning, and the rest were just side-quests... :)
> 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.
> :-)
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.
> 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?
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-17 10:09 ` Attila Lendvai
@ 2021-10-18 9:10 ` Ludovic Courtès
2021-10-18 15:27 ` Attila Lendvai
0 siblings, 1 reply; 38+ messages in thread
From: Ludovic Courtès @ 2021-10-18 9:10 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
Hi Attila,
Attila Lendvai <attila@lendvai.name> skribis:
>> 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.
>
> i ran the test without my fix, and indeed it fails at two points:
Sorry, which test is failing? Is that part of the patches you sent?
I need more context. :-)
[...]
>> 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.
>> :-)
>
>
> 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.
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
>> 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?
>
>
> 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.
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’.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-18 9:10 ` Ludovic Courtès
@ 2021-10-18 15:27 ` Attila Lendvai
0 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-18 15:27 UTC (permalink / raw)
To: Ludovic Courtès; +Cc: 50814
hi Ludo,
> > i ran the test without my fix, and indeed it fails at two points:
>
> Sorry, which test is failing? Is that part of the patches you sent?
>
> I need more context. :-)
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.
> > > 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.
> > >
> > > :-)
> >
> > 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.
>
> Yes, in general it’s best to have the test and the fix in the same
> commit.
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.
> 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
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.
> 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!
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)
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] tests: Add test for .guix-authorizations and channel intro.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
` (4 preceding siblings ...)
2021-10-09 13:53 ` [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Ludovic Courtès
@ 2021-10-10 14:15 ` Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
2022-04-04 6:47 ` Attila Lendvai
7 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-10 14:15 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
This test used to fail before a recent fix to authenticate-repository.
* tests/git-authenticate.scm: New test "signed commits, .guix-authorizations,
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'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.
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(+)
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index f66ef191b0..7989f46924 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -18,6 +18,7 @@
(define-module (test-git-authenticate)
#:use-module (git)
+ #:use-module (guix diagnostics)
#:use-module (guix git)
#:use-module (guix git-authenticate)
#:use-module (guix openpgp)
@@ -28,6 +29,10 @@
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-64)
#:use-module (rnrs bytevectors)
+ #:use-module ((rnrs conditions)
+ #:select (warning?))
+ #:use-module ((rnrs exceptions)
+ #:select (with-exception-handler))
#:use-module (rnrs io ports))
;; Test the (guix git-authenticate) tools.
@@ -226,6 +231,140 @@
#:keyring-reference "master")
#f)))))))
+(unless (gpg+git-available?) (test-skip 1))
+(test-assert "signed commits, .guix-authorizations, channel-introduction"
+ (let* ((result #true)
+ (key1 %ed25519-public-key-file)
+ (key2 %ed25519-2-public-key-file)
+ (key3 %ed25519-3-public-key-file))
+ (with-fresh-gnupg-setup (list key1 %ed25519-secret-key-file
+ key2 %ed25519-2-secret-key-file
+ key3 %ed25519-3-secret-key-file)
+ (with-temporary-git-repository dir
+ `((checkout "keyring" orphan)
+ (add "signer1.key" ,(call-with-input-file key1 get-string-all))
+ (add "signer2.key" ,(call-with-input-file key2 get-string-all))
+ (add "signer3.key" ,(call-with-input-file key3 get-string-all))
+ (commit "keyring commit")
+
+ (checkout "main" orphan)
+ (add "noise0")
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ (,(key-fingerprint key3) (name "Charlie"))))))
+ (commit "commit 0" (signer ,(key-fingerprint key3)))
+ (add "noise1")
+ (commit "commit 1" (signer ,(key-fingerprint key1)))
+ (add "noise2")
+ (commit "commit 2" (signer ,(key-fingerprint key1))))
+ (with-repository dir repo
+ (let* ((commit-0 (find-commit repo "commit 0"))
+ (check-from
+ (lambda* (commit #:key (should-fail? #false) (key key1)
+ (historical-authorizations
+ ;; key3 is trusted to authorize commit 0
+ (list (key-fingerprint-vector key3))))
+ (guard (c ((unauthorized-commit-error? c)
+ (if should-fail?
+ c
+ (let ((port (current-output-port)))
+ (format port "FAILURE: Unexpected exception at commit '~s':~%"
+ commit)
+ (print-exception port (stack-ref (make-stack #t) 1)
+ c (exception-args c))
+ (set! result #false)
+ '()))))
+ (format #true "~%~%Checking ~s, should-fail? ~s, repo commits:~%"
+ commit should-fail?)
+ ;; to be able to inspect in the logs
+ (invoke "git" "-C" dir "log" "--reverse" "--pretty=oneline" "main")
+ (set! commit (find-commit repo commit))
+ (authenticate-repository
+ repo
+ (commit-id commit)
+ (key-fingerprint-vector key)
+ #:historical-authorizations historical-authorizations)
+ (when should-fail?
+ (format #t "FAILURE: Authenticating commit '~s' should have failed.~%" commit)
+ (set! result #false))
+ '()))))
+ (check-from "commit 0" #:key key3)
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (with-git-repository dir
+ `((add "noise 3")
+ ;; a commit with key2
+ (commit "commit 3" (signer ,(key-fingerprint key2))))
+ ;; Should fail because it is signed with key2, not key1
+ (check-from "commit 3" #:should-fail? #true)
+ ;; Specify commit 3 as a channel-introduction signed with
+ ;; key2. This is valid, but it should warn the user, because
+ ;; .guix-authorizations is not updated to include key2, which
+ ;; means that any subsequent commits with the same key will be
+ ;; rejected.
+ (set! result
+ (and (let ((signalled? #false))
+ (with-exception-handler
+ (lambda (c)
+ (cond
+ ((not (warning? c))
+ (raise c))
+ ((formatted-message? c)
+ (format #true "warning (expected): ~a~%"
+ (apply format #false
+ (formatted-message-string c)
+ (formatted-message-arguments c)))
+ (set! signalled? #true)))
+ '())
+ (lambda ()
+ (check-from "commit 3" #:key key2)
+ (unless signalled?
+ (format #t "FAILURE: No warning signalled for commit 3~%"))
+ signalled?)))
+ result)))
+ (with-git-repository dir
+ `((reset ,(oid->string (commit-id (find-commit repo "commit 2"))))
+ (add "noise 4")
+ ;; set it up properly
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ (,(key-fingerprint key2) (name "Bob"))))))
+ (commit "commit 4" (signer ,(key-fingerprint key2))))
+ ;; This should fail because even though commit 4 adds key2 to
+ ;; .guix-authorizations, the commit itself is not authorized.
+ (check-from "commit 1" #:should-fail? #true)
+ ;; This should pass, because it's a valid channel intro at commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 5")
+ (commit "commit 5" (signer ,(key-fingerprint key2))))
+ ;; 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")
+ ;; Should still be fine, but only when starting from commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 6")
+ (commit "commit 6" (signer ,(key-fingerprint key1))))
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (check-from "commit 4" #:key key2))))))
+ result))
+
(unless (gpg+git-available?) (test-skip 1))
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
` (5 preceding siblings ...)
2021-10-10 14:15 ` [bug#50814] [PATCH] tests: Add test for .guix-authorizations and channel intro Attila Lendvai
@ 2021-10-18 15:57 ` Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
` (3 more replies)
2022-04-04 6:47 ` Attila Lendvai
7 siblings, 4 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-18 15:57 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
* guix/tests/git.scm (with-git-repository): New macro that can be used in
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/gnupg.scm (key-fingerprint-vector): New function.
---
guix/tests/git.scm | 23 +++++++++++++++++++++--
guix/tests/gnupg.scm | 8 ++++++--
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/guix/tests/git.scm b/guix/tests/git.scm
index 69960284d9..76f5a8b937 100644
--- a/guix/tests/git.scm
+++ b/guix/tests/git.scm
@@ -26,6 +26,7 @@ (define-module (guix tests git)
#:use-module (ice-9 control)
#:export (git-command
with-temporary-git-repository
+ with-git-repository
find-commit))
(define git-command
@@ -59,8 +60,9 @@ (define (git command . args)
(apply invoke (git-command) "-C" directory
command args)))))
- (mkdir-p directory)
- (git "init")
+ (unless (directory-exists? (string-append directory "/.git"))
+ (mkdir-p directory)
+ (git "init"))
(let loop ((directives directives))
(match directives
@@ -78,6 +80,9 @@ (define (git command . args)
port)))
(git "add" file)
(loop rest)))
+ ((('add file-name-and-content) rest ...)
+ (loop (cons `(add ,file-name-and-content ,file-name-and-content)
+ rest)))
((('remove file) rest ...)
(git "rm" "-f" file)
(loop rest))
@@ -99,12 +104,18 @@ (define (git command . args)
((('checkout branch) rest ...)
(git "checkout" branch)
(loop rest))
+ ((('checkout branch 'orphan) rest ...)
+ (git "checkout" "--orphan" branch)
+ (loop rest))
((('merge branch message) rest ...)
(git "merge" branch "-m" message)
(loop rest))
((('merge branch message ('signer fingerprint)) rest ...)
(git "merge" branch "-m" message
(string-append "--gpg-sign=" fingerprint))
+ (loop rest))
+ ((('reset to) rest ...)
+ (git "reset" "--hard" to)
(loop rest)))))
(define (call-with-temporary-git-repository directives proc)
@@ -121,6 +132,14 @@ (define-syntax-rule (with-temporary-git-repository directory
(lambda (directory)
exp ...)))
+(define-syntax-rule (with-git-repository directory
+ directives exp ...)
+ "Evaluate EXP in a context where DIRECTORY is (further) populated as
+per DIRECTIVES."
+ (begin
+ (populate-git-repository directory directives)
+ exp ...))
+
(define (find-commit repository message)
"Return the commit in REPOSITORY whose message includes MESSAGE, a string."
(let/ec return
diff --git a/guix/tests/gnupg.scm b/guix/tests/gnupg.scm
index eb8ff63a43..c7630db912 100644
--- a/guix/tests/gnupg.scm
+++ b/guix/tests/gnupg.scm
@@ -33,6 +33,7 @@ (define-module (guix tests gnupg)
read-openpgp-packet
key-fingerprint
+ key-fingerprint-vector
key-id))
(define gpg-command
@@ -76,7 +77,10 @@ (define (read-openpgp-packet file)
(open-bytevector-input-port
(call-with-input-file file read-radix-64))))
+(define key-fingerprint-vector
+ (compose openpgp-public-key-fingerprint
+ read-openpgp-packet))
+
(define key-fingerprint
(compose openpgp-format-fingerprint
- openpgp-public-key-fingerprint
- read-openpgp-packet))
+ key-fingerprint-vector))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key.
2021-10-18 15:57 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
@ 2021-10-18 15:57 ` Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 3/5] guix: Prepare the UI for continuable &warning exceptions Attila Lendvai
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-18 15:57 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
The third key will be used in an upcoming commit.
Rename public keys to .pub.
* guix/tests/gnupg.scm (%ed25519-3-public-key-file): New variable.
(%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.
* tests/keys/ed25519-3.key: New file.
* tests/keys/ed25519-3.sec: New 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%)
diff --git a/Makefile.am b/Makefile.am
index 635147efc1..95c6597c17 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -645,16 +645,18 @@ EXTRA_DIST += \
build-aux/update-guix-package.scm \
build-aux/update-NEWS.scm \
tests/test.drv \
- tests/signing-key.pub \
- tests/signing-key.sec \
tests/cve-sample.json \
- tests/civodul.key \
- tests/rsa.key \
- tests/dsa.key \
- tests/ed25519.key \
- tests/ed25519.sec \
- tests/ed25519bis.key \
- tests/ed25519bis.sec \
+ tests/keys/signing-key.pub \
+ tests/keys/signing-key.sec \
+ tests/keys/civodul.pub \
+ tests/keys/rsa.pub \
+ tests/keys/dsa.pub \
+ tests/keys/ed25519.pub \
+ tests/keys/ed25519.sec \
+ tests/keys/ed25519-2.pub \
+ tests/keys/ed25519-2.sec \
+ tests/keys/ed25519-3.pub \
+ tests/keys/ed25519-3.sec \
build-aux/config.rpath \
bootstrap \
doc/build.scm \
diff --git a/build-aux/test-env.in b/build-aux/test-env.in
index 7efc43206c..ca786437e9 100644
--- a/build-aux/test-env.in
+++ b/build-aux/test-env.in
@@ -73,9 +73,9 @@ then
# Copy the keys so that the secret key has the right permissions (the
# daemon errors out when this is not the case.)
mkdir -p "$GUIX_CONFIGURATION_DIRECTORY"
- cp "@abs_top_srcdir@/tests/signing-key.sec" \
- "@abs_top_srcdir@/tests/signing-key.pub" \
- "$GUIX_CONFIGURATION_DIRECTORY"
+ cp "@abs_top_srcdir@/tests/keys/signing-key.sec" \
+ "@abs_top_srcdir@/tests/keys/signing-key.pub" \
+ "$GUIX_CONFIGURATION_DIRECTORY"
chmod 400 "$GUIX_CONFIGURATION_DIRECTORY/signing-key.sec"
fi
diff --git a/guix/tests/gnupg.scm b/guix/tests/gnupg.scm
index c7630db912..09f02a2b67 100644
--- a/guix/tests/gnupg.scm
+++ b/guix/tests/gnupg.scm
@@ -28,8 +28,10 @@ (define-module (guix tests gnupg)
%ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file
+ %ed25519-3-public-key-file
+ %ed25519-3-secret-key-file
read-openpgp-packet
key-fingerprint
@@ -64,13 +66,17 @@ (define-syntax-rule (with-fresh-gnupg-setup imported exp ...)
(call-with-fresh-gnupg-setup imported (lambda () exp ...)))
(define %ed25519-public-key-file
- (search-path %load-path "tests/ed25519.key"))
+ (search-path %load-path "tests/keys/ed25519.pub"))
(define %ed25519-secret-key-file
- (search-path %load-path "tests/ed25519.sec"))
-(define %ed25519bis-public-key-file
- (search-path %load-path "tests/ed25519bis.key"))
-(define %ed25519bis-secret-key-file
- (search-path %load-path "tests/ed25519bis.sec"))
+ (search-path %load-path "tests/keys/ed25519.sec"))
+(define %ed25519-2-public-key-file
+ (search-path %load-path "tests/keys/ed25519-2.pub"))
+(define %ed25519-2-secret-key-file
+ (search-path %load-path "tests/keys/ed25519-2.sec"))
+(define %ed25519-3-public-key-file
+ (search-path %load-path "tests/keys/ed25519-3.pub"))
+(define %ed25519-3-secret-key-file
+ (search-path %load-path "tests/keys/ed25519-3.sec"))
(define (read-openpgp-packet file)
(get-openpgp-packet
diff --git a/tests/channels.scm b/tests/channels.scm
index 3e82315b0c..d45c450241 100644
--- a/tests/channels.scm
+++ b/tests/channels.scm
@@ -480,8 +480,8 @@ (define (find-commit* message)
#t
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add ".guix-channel"
,(object->string
@@ -507,7 +507,7 @@ (define (find-commit* message)
(commit-id-string commit1)
(openpgp-public-key-fingerprint
(read-openpgp-packet
- %ed25519bis-public-key-file)))) ;different key
+ %ed25519-2-public-key-file)))) ;different key
(channel (channel (name 'example)
(url (string-append "file://" directory))
(introduction intro))))
@@ -519,7 +519,7 @@ (define (find-commit* message)
(oid->string (commit-id commit1))
(key-fingerprint %ed25519-public-key-file)
(key-fingerprint
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(authenticate-channel channel directory
(commit-id-string commit2)
#:keyring-reference-prefix "")
@@ -530,8 +530,8 @@ (define (find-commit* message)
#t
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add ".guix-channel"
,(object->string
@@ -552,12 +552,12 @@ (define (find-commit* message)
(signer ,(key-fingerprint %ed25519-public-key-file)))
(add "c.txt" "C")
(commit "third commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(branch "channel-keyring")
(checkout "channel-keyring")
(add "signer.key" ,(call-with-input-file %ed25519-public-key-file
get-string-all))
- (add "other.key" ,(call-with-input-file %ed25519bis-public-key-file
+ (add "other.key" ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(commit "keyring commit")
(checkout "master"))
@@ -588,7 +588,7 @@ (define (find-commit* message)
(unauthorized-commit-error-signing-key c))
(openpgp-public-key-fingerprint
(read-openpgp-packet
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(authenticate-channel channel directory
(commit-id-string commit3)
#:keyring-reference-prefix "")
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index d87eacc659..f66ef191b0 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -161,14 +161,14 @@ (define (gpg+git-available?)
(test-assert "signed commits, .guix-authorizations, unauthorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add "signer1.key"
,(call-with-input-file %ed25519-public-key-file
get-string-all))
(add "signer2.key"
- ,(call-with-input-file %ed25519bis-public-key-file
+ ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(add ".guix-authorizations"
,(object->string
@@ -184,7 +184,7 @@ (define (gpg+git-available?)
(checkout "devel")
(add "devel/1.txt" "1")
(commit "first devel commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(checkout "master")
(add "b.txt" "B")
(commit "second commit"
@@ -203,7 +203,7 @@ (define (correct? c commit)
(openpgp-public-key-fingerprint
(unauthorized-commit-error-signing-key c))
(openpgp-public-key-fingerprint
- (read-openpgp-packet %ed25519bis-public-key-file)))))
+ (read-openpgp-packet %ed25519-2-public-key-file)))))
(and (authenticate-commits repository (list master1 master2)
#:keyring-reference "master")
@@ -230,14 +230,14 @@ (define (correct? c commit)
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
%ed25519-secret-key-file
- %ed25519bis-public-key-file
- %ed25519bis-secret-key-file)
+ %ed25519-2-public-key-file
+ %ed25519-2-secret-key-file)
(with-temporary-git-repository directory
`((add "signer1.key"
,(call-with-input-file %ed25519-public-key-file
get-string-all))
(add "signer2.key"
- ,(call-with-input-file %ed25519bis-public-key-file
+ ,(call-with-input-file %ed25519-2-public-key-file
get-string-all))
(add ".guix-authorizations"
,(object->string
@@ -258,12 +258,12 @@ (define (correct? c commit)
%ed25519-public-key-file)
(name "Alice"))
(,(key-fingerprint
- %ed25519bis-public-key-file))))))
+ %ed25519-2-public-key-file))))))
(commit "first devel commit"
(signer ,(key-fingerprint %ed25519-public-key-file)))
(add "devel/2.txt" "2")
(commit "second devel commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file)))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file)))
(checkout "master")
(add "b.txt" "B")
(commit "second commit"
@@ -273,7 +273,7 @@ (define (correct? c commit)
;; After the merge, the second signer is authorized.
(add "c.txt" "C")
(commit "third commit"
- (signer ,(key-fingerprint %ed25519bis-public-key-file))))
+ (signer ,(key-fingerprint %ed25519-2-public-key-file))))
(with-repository directory repository
(let ((master1 (find-commit repository "first commit"))
(master2 (find-commit repository "second commit"))
@@ -328,4 +328,3 @@ (define (correct? c commit)
'failed)))))))
(test-end "git-authenticate")
-
diff --git a/tests/guix-authenticate.sh b/tests/guix-authenticate.sh
index 3a05b232c1..0de6da1878 100644
--- a/tests/guix-authenticate.sh
+++ b/tests/guix-authenticate.sh
@@ -28,7 +28,7 @@ rm -f "$sig" "$hash"
trap 'rm -f "$sig" "$hash"' EXIT
-key="$abs_top_srcdir/tests/signing-key.sec"
+key="$abs_top_srcdir/tests/keys/signing-key.sec"
key_len="`echo -n $key | wc -c`"
# A hexadecimal string as long as a sha256 hash.
@@ -67,7 +67,7 @@ test "$code" -ne 0
# encoded independently of the current locale: <https://bugs.gnu.org/43421>.
hash="636166e9636166e9636166e9636166e9636166e9636166e9636166e9636166e9"
latin1_cafe="caf$(printf '\351')"
-echo "sign 21:tests/signing-key.sec 64:$hash" | guix authenticate \
+echo "sign 26:tests/keys/signing-key.sec 64:$hash" | guix authenticate \
| LC_ALL=C grep "hash sha256 \"$latin1_cafe"
# Test for <http://bugs.gnu.org/17312>: make sure 'guix authenticate' produces
diff --git a/tests/civodul.key b/tests/keys/civodul.pub
similarity index 100%
rename from tests/civodul.key
rename to tests/keys/civodul.pub
diff --git a/tests/dsa.key b/tests/keys/dsa.pub
similarity index 100%
rename from tests/dsa.key
rename to tests/keys/dsa.pub
diff --git a/tests/ed25519bis.key b/tests/keys/ed25519-2.pub
similarity index 100%
rename from tests/ed25519bis.key
rename to tests/keys/ed25519-2.pub
diff --git a/tests/ed25519bis.sec b/tests/keys/ed25519-2.sec
similarity index 100%
rename from tests/ed25519bis.sec
rename to tests/keys/ed25519-2.sec
diff --git a/tests/keys/ed25519-3.pub b/tests/keys/ed25519-3.pub
new file mode 100644
index 0000000000..72f311984c
--- /dev/null
+++ b/tests/keys/ed25519-3.pub
@@ -0,0 +1,9 @@
+-----BEGIN PGP PUBLIC KEY BLOCK-----
+
+mDMEYVH/7xYJKwYBBAHaRw8BAQdALMLeUhjEG2/UPCJj2j/debFwwAK5gT3G0l5d
+ILfFldm0FTxleGFtcGxlQGV4YW1wbGUuY29tPoiWBBMWCAA+FiEEjO6M85jMSK68
+7tINGBzA7NyoagkFAmFR/+8CGwMFCQPCZwAFCwkIBwIGFQoJCAsCBBYCAwECHgEC
+F4AACgkQGBzA7Nyoagl3lgEAw6yqIlX11lTqwxBGhZk/Oy34O13cbJSZCGv+m0ja
++hcA/3DCNOmT+oXjgO/w6enQZUQ1m/d6dUjCc2wOLlLz+ZoG
+=+r3i
+-----END PGP PUBLIC KEY BLOCK-----
diff --git a/tests/keys/ed25519-3.sec b/tests/keys/ed25519-3.sec
new file mode 100644
index 0000000000..04128a4131
--- /dev/null
+++ b/tests/keys/ed25519-3.sec
@@ -0,0 +1,10 @@
+-----BEGIN PGP PRIVATE KEY BLOCK-----
+
+lFgEYVH/7xYJKwYBBAHaRw8BAQdALMLeUhjEG2/UPCJj2j/debFwwAK5gT3G0l5d
+ILfFldkAAP92goSbbzQ0ttElr9lr5Cm6rmQtqUZ2Cu/Jk9fvfZROwxI0tBU8ZXhh
+bXBsZUBleGFtcGxlLmNvbT6IlgQTFggAPhYhBIzujPOYzEiuvO7SDRgcwOzcqGoJ
+BQJhUf/vAhsDBQkDwmcABQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEBgcwOzc
+qGoJd5YBAMOsqiJV9dZU6sMQRoWZPzst+Dtd3GyUmQhr/ptI2voXAP9wwjTpk/qF
+44Dv8Onp0GVENZv3enVIwnNsDi5S8/maBg==
+=EmOt
+-----END PGP PRIVATE KEY BLOCK-----
diff --git a/tests/ed25519.key b/tests/keys/ed25519.pub
similarity index 100%
rename from tests/ed25519.key
rename to tests/keys/ed25519.pub
diff --git a/tests/ed25519.sec b/tests/keys/ed25519.sec
similarity index 100%
rename from tests/ed25519.sec
rename to tests/keys/ed25519.sec
diff --git a/tests/rsa.key b/tests/keys/rsa.pub
similarity index 100%
rename from tests/rsa.key
rename to tests/keys/rsa.pub
diff --git a/tests/signing-key.pub b/tests/keys/signing-key.pub
similarity index 100%
rename from tests/signing-key.pub
rename to tests/keys/signing-key.pub
diff --git a/tests/signing-key.sec b/tests/keys/signing-key.sec
similarity index 100%
rename from tests/signing-key.sec
rename to tests/keys/signing-key.sec
diff --git a/tests/openpgp.scm b/tests/openpgp.scm
index c2be26fa49..1f20466772 100644
--- a/tests/openpgp.scm
+++ b/tests/openpgp.scm
@@ -59,18 +59,22 @@ (define %binary-sample
(define %civodul-fingerprint
"3CE4 6455 8A84 FDC6 9DB4 0CFB 090B 1199 3D9A EBB5")
-(define %civodul-key-id #x090B11993D9AEBB5) ;civodul.key
-
-;; Test keys. They were generated in a container along these lines:
-;; guix environment -CP --ad-hoc gnupg pinentry
-;; then, within the container:
-;; mkdir ~/.gnupg
-;; echo pinentry-program ~/.guix-profile/bin/pinentry-tty > ~/.gnupg/gpg-agent.conf
-;; gpg --quick-gen-key '<ludo+test-rsa@chbouib.org>' rsa
-;; or similar.
-(define %rsa-key-id #xAE25DA2A70DEED59) ;rsa.key
-(define %dsa-key-id #x587918047BE8BD2C) ;dsa.key
-(define %ed25519-key-id #x771F49CBFAAE072D) ;ed25519.key
+(define %civodul-key-id #x090B11993D9AEBB5) ;civodul.pub
+
+#|
+Test keys in ./tests/keys. They were generated in a container along these lines:
+ guix environment -CP --ad-hoc gnupg pinentry coreutils
+then, within the container:
+ mkdir ~/.gnupg && chmod -R og-rwx ~/.gnupg
+ gpg --batch --passphrase '' --quick-gen-key '<example@example.com>' ed25519
+ gpg --armor --export example@example.com
+ gpg --armor --export-secret-key example@example.com
+ # echo pinentry-program ~/.guix-profile/bin/pinentry-curses > ~/.gnupg/gpg-agent.conf
+or similar.
+|#
+(define %rsa-key-id #xAE25DA2A70DEED59) ;rsa.pub
+(define %dsa-key-id #x587918047BE8BD2C) ;dsa.pub
+(define %ed25519-key-id #x771F49CBFAAE072D) ;ed25519.pub
(define %rsa-key-fingerprint
(base16-string->bytevector
@@ -168,7 +172,7 @@ (define %hello-signature/ed25519/sha1 ;digest-algo: sha1
(not (port-ascii-armored? (open-bytevector-input-port %binary-sample))))
(test-assert "get-openpgp-keyring"
- (let* ((key (search-path %load-path "tests/civodul.key"))
+ (let* ((key (search-path %load-path "tests/keys/civodul.pub"))
(keyring (get-openpgp-keyring
(open-bytevector-input-port
(call-with-input-file key read-radix-64)))))
@@ -228,8 +232,10 @@ (define %hello-signature/ed25519/sha1 ;digest-algo: sha1
(verify-openpgp-signature signature keyring
(open-input-string "Hello!\n"))))
(list status (openpgp-public-key-id key)))))
- (list "tests/rsa.key" "tests/dsa.key"
- "tests/ed25519.key" "tests/ed25519.key" "tests/ed25519.key")
+ (list "tests/keys/rsa.pub" "tests/keys/dsa.pub"
+ "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub")
(list %hello-signature/rsa %hello-signature/dsa
%hello-signature/ed25519/sha256
%hello-signature/ed25519/sha512
@@ -248,9 +254,9 @@ (define %hello-signature/ed25519/sha1 ;digest-algo: sha1
(call-with-input-file key read-radix-64))
keyring)))
%empty-keyring
- '("tests/rsa.key" "tests/dsa.key"
- "tests/ed25519.key" "tests/ed25519.key"
- "tests/ed25519.key"))))
+ '("tests/keys/rsa.pub" "tests/keys/dsa.pub"
+ "tests/keys/ed25519.pub" "tests/keys/ed25519.pub"
+ "tests/keys/ed25519.pub"))))
(map (lambda (signature)
(let ((signature (string->openpgp-packet signature)))
(let-values (((status key)
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 3/5] guix: Prepare the UI for continuable &warning exceptions.
2021-10-18 15:57 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
@ 2021-10-18 15:57 ` Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 4/5] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 5/5] tests: Add test for .guix-authorizations and channel intro Attila Lendvai
3 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-18 15:57 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
* guix/store.scm (call-with-store): Use dynamic-wind so that continuable
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.
* guix/ui.scm (call-with-error-handling): Handle &warning type exceptions by
printing them to the user, and then continuing at the place they were
signalled at.
* guix/diagnostics.scm (emit-formatted-warning): New exported
function.
---
guix/diagnostics.scm | 4 ++++
guix/store.scm | 7 +++++--
guix/ui.scm | 11 ++++++++++-
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/guix/diagnostics.scm b/guix/diagnostics.scm
index 6a792febd4..343213fb45 100644
--- a/guix/diagnostics.scm
+++ b/guix/diagnostics.scm
@@ -48,6 +48,7 @@ (define-module (guix diagnostics)
formatted-message?
formatted-message-string
formatted-message-arguments
+ emit-formatted-warning
&fix-hint
fix-hint?
@@ -161,6 +162,9 @@ (define-syntax-rule (leave args ...)
(report-error args ...)
(exit 1)))
+(define* (emit-formatted-warning fmt . args)
+ (emit-diagnostic fmt args #:prefix (G_ "warning: ") #:colors %warning-color))
+
(define* (emit-diagnostic fmt args
#:key location (colors (color)) (prefix ""))
"Report diagnostic message FMT with the given ARGS and the specified
diff --git a/guix/store.scm b/guix/store.scm
index 89a719bcfc..1b177cc952 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -34,6 +34,8 @@ (define-module (guix store)
#:use-module (guix profiling)
#:autoload (guix build syscalls) (terminal-columns)
#:use-module (rnrs bytevectors)
+ #:use-module ((rnrs conditions) #:select (warning?))
+ #:use-module ((rnrs exceptions) #:select (raise-continuable))
#:use-module (ice-9 binary-ports)
#:use-module ((ice-9 control) #:select (let/ec))
#:use-module (ice-9 atomic)
@@ -661,8 +663,9 @@ (define (thunk)
(apply values results)))))
(with-exception-handler (lambda (exception)
- (close-connection store)
- (raise-exception exception))
+ (unless (warning? exception)
+ (close-connection store))
+ (raise-continuable exception))
thunk)))
(define-syntax-rule (with-store store exp ...)
diff --git a/guix/ui.scm b/guix/ui.scm
index 1428c254b3..88940f99ef 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -69,6 +69,8 @@ (define-module (guix ui)
#:use-module (srfi srfi-31)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
+ #:use-module ((rnrs conditions)
+ #:select (warning?))
#:autoload (ice-9 ftw) (scandir)
#:use-module (ice-9 match)
#:use-module (ice-9 format)
@@ -689,7 +691,14 @@ (define (port-filename* port)
(and (not (port-closed? port))
(port-filename port)))
- (guard* (c ((package-input-error? c)
+ (guard* (c ((warning? c)
+ (if (formatted-message? c)
+ (apply emit-formatted-warning
+ (formatted-message-string c)
+ (formatted-message-arguments c))
+ (emit-formatted-warning "~a" c))
+ '())
+ ((package-input-error? c)
(let* ((package (package-error-package c))
(input (package-error-invalid-input c))
(location (package-location package))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 4/5] guix: git-authenticate: Fix authenticate-repository.
2021-10-18 15:57 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 2/5] tests: Move keys into ./tests/keys/ and add a third ed25519 key Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 3/5] guix: Prepare the UI for continuable &warning exceptions Attila Lendvai
@ 2021-10-18 15:57 ` Attila Lendvai
2021-10-18 15:57 ` [bug#50814] [PATCH 5/5] tests: Add test for .guix-authorizations and channel intro Attila Lendvai
3 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2021-10-18 15:57 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
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.
* guix/git-authenticate.scm (authenticate-commit): Reword and extend the error
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(-)
diff --git a/guix/channels.scm b/guix/channels.scm
index e4e0428eb5..b84064537f 100644
--- a/guix/channels.scm
+++ b/guix/channels.scm
@@ -347,8 +347,8 @@ (define (make-reporter start-commit end-commit commits)
(progress-reporter/bar (length commits)))
(define authentic-commits
- ;; Consider the currently-used commit of CHANNEL as authentic so
- ;; authentication can skip it and all its closure.
+ ;; Optimization: consider the currently-used commit of CHANNEL as
+ ;; authentic, so that authentication can skip it and all its closure.
(match (find (lambda (candidate)
(eq? (channel-name candidate) (channel-name channel)))
(current-channels))
diff --git a/guix/git-authenticate.scm b/guix/git-authenticate.scm
index ab3fcd8b2f..a667863d65 100644
--- a/guix/git-authenticate.scm
+++ b/guix/git-authenticate.scm
@@ -30,6 +30,7 @@ (define-module (guix git-authenticate)
#:select (cache-directory with-atomic-file-output))
#:use-module ((guix build utils)
#:select (mkdir-p))
+ #:use-module (guix diagnostics)
#:use-module (guix progress)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-11)
@@ -37,7 +38,10 @@ (define-module (guix git-authenticate)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
#:use-module (rnrs bytevectors)
+ #:use-module ((rnrs exceptions)
+ #:select (raise-continuable))
#:use-module (rnrs io ports)
+ #:use-module (ice-9 exceptions)
#:use-module (ice-9 match)
#:autoload (ice-9 pretty-print) (pretty-print)
#:export (read-authorizations
@@ -159,11 +163,12 @@ (define (read-authorizations port)
(string-downcase (string-filter char-set:graphic fingerprint))))
fingerprints))))
-(define* (commit-authorized-keys repository commit
- #:optional (default-authorizations '()))
- "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
-authorizations listed in its parent commits. If one of the parent commits
-does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
+(define (authorized-keys-at-commit repository commit default-value)
+ "Return the list of authorized key fingerprints in REPOSITORY as encoded in
+the '.guix-authorizations' file at the point denoted by COMMIT. If the file is
+not present, then assert that it has never been there (i.e. do not allow
+its removal), and return DEFAULT-VALUE."
+
(define (parents-have-authorizations-file? commit)
;; Return true if at least one of the parents of COMMIT has the
;; '.guix-authorizations' file.
@@ -185,28 +190,35 @@ (define (assert-parents-lack-authorizations commit)
to remove '.guix-authorizations' file")
(oid->string (commit-id commit)))))))
- (define (commit-authorizations commit)
- (catch 'git-error
- (lambda ()
- (let* ((tree (commit-tree commit))
- (entry (tree-entry-bypath tree ".guix-authorizations"))
- (blob (blob-lookup repository (tree-entry-id entry))))
- (read-authorizations
- (open-bytevector-input-port (blob-content blob)))))
- (lambda (key error)
- (if (= (git-error-code error) GIT_ENOTFOUND)
- (begin
- ;; Prevent removal of '.guix-authorizations' since it would make
- ;; it trivial to force a fallback to DEFAULT-AUTHORIZATIONS.
- (assert-parents-lack-authorizations commit)
- default-authorizations)
- (throw key error)))))
+ (catch 'git-error
+ (lambda ()
+ (let* ((tree (commit-tree commit))
+ (entry (tree-entry-bypath tree ".guix-authorizations"))
+ (blob (blob-lookup repository (tree-entry-id entry))))
+ (read-authorizations
+ (open-bytevector-input-port (blob-content blob)))))
+ (lambda (key error)
+ (if (= (git-error-code error) GIT_ENOTFOUND)
+ (begin
+ ;; Prevent removal of '.guix-authorizations' since it would make
+ ;; it trivial to force a fallback to DEFAULT-VALUE.
+ (assert-parents-lack-authorizations commit)
+ default-value)
+ (throw key error)))))
+(define* (commit-authorized-keys repository commit
+ #:optional (default-authorizations '()))
+ "Return the list of OpenPGP fingerprints authorized to sign COMMIT, based on
+authorizations listed in its parent commits. If one of the parent commits
+does not specify anything, fall back to DEFAULT-AUTHORIZATIONS."
(match (commit-parents commit)
(() default-authorizations)
(parents
(apply lset-intersection bytevector=?
- (map commit-authorizations parents)))))
+ (map (lambda (commit)
+ (authorized-keys-at-commit repository commit
+ default-authorizations))
+ parents)))))
(define* (authenticate-commit repository commit keyring
#:key (default-authorizations '()))
@@ -236,8 +248,8 @@ (define signing-key
(condition
(&unauthorized-commit-error (commit id)
(signing-key signing-key)))
- (formatted-message (G_ "commit ~a not signed by an authorized \
-key: ~a")
+ (formatted-message (G_ "commit ~a is signed by an unauthorized \
+key: ~a\nSee info guix \"Specifying Channel Authorizations\".")
(oid->string id)
(openpgp-format-fingerprint
(openpgp-public-key-fingerprint
@@ -356,7 +368,8 @@ (define (repository-cache-key repository)
(base64-encode
(sha256 (string->utf8 (repository-directory repository))))))
-(define (verify-introductory-commit repository keyring commit expected-signer)
+(define (verify-introductory-commit repository commit expected-signer keyring
+ authorizations)
"Look up COMMIT in REPOSITORY, and raise an exception if it is not signed by
EXPECTED-SIGNER."
(define actual-signer
@@ -364,13 +377,26 @@ (define actual-signer
(commit-signing-key repository (commit-id commit) keyring)))
(unless (bytevector=? expected-signer actual-signer)
- (raise (formatted-message (G_ "initial commit ~a is signed by '~a' \
+ (raise (make-compound-condition
+ (condition (&unauthorized-commit-error (commit (commit-id commit))
+ (signing-key actual-signer)))
+ (formatted-message (G_ "initial commit ~a is signed by '~a' \
instead of '~a'")
- (oid->string (commit-id commit))
- (openpgp-format-fingerprint actual-signer)
- (openpgp-format-fingerprint expected-signer)))))
-
-(define* (authenticate-repository repository start signer
+ (oid->string (commit-id commit))
+ (openpgp-format-fingerprint actual-signer)
+ (openpgp-format-fingerprint expected-signer)))))
+ (unless (member actual-signer
+ (authorized-keys-at-commit repository commit authorizations)
+ bytevector=?)
+ (raise-continuable
+ (make-compound-condition
+ (condition (&warning))
+ (formatted-message (G_ "initial commit ~a does not add \
+the key it is signed with (~a) to the '.guix-authorizations' file.")
+ (oid->string (commit-id commit))
+ (openpgp-format-fingerprint actual-signer))))))
+
+(define* (authenticate-repository repository intro-commit-hash intro-signer
#:key
(keyring-reference "keyring")
(cache-key (repository-cache-key repository))
@@ -380,11 +406,12 @@ (define* (authenticate-repository repository start signer
(historical-authorizations '())
(make-reporter
(const progress-reporter/silent)))
- "Authenticate REPOSITORY up to commit END, an OID. Authentication starts
-with commit START, an OID, which must be signed by SIGNER; an exception is
-raised if that is not the case. Commits listed in AUTHENTIC-COMMITS and their
-closure are considered authentic. Return an alist mapping OpenPGP public keys
-to the number of commits signed by that key that have been traversed.
+ "Authenticate REPOSITORY up to commit END, an OID. Authentication starts with
+commit INTRO-COMMIT-HASH, an OID, which must be signed by INTRO-SIGNER; an
+exception is raised if that is not the case. Commits listed in
+AUTHENTIC-COMMITS and their closure are considered authentic. Return an
+alist mapping OpenPGP public keys to the number of commits signed by that
+key that have been traversed.
The OpenPGP keyring is loaded from KEYRING-REFERENCE in REPOSITORY, where
KEYRING-REFERENCE is the name of a branch. The list of authenticated commits
@@ -393,8 +420,10 @@ (define* (authenticate-repository repository start signer
HISTORICAL-AUTHORIZATIONS must be a list of OpenPGP fingerprints (bytevectors)
denoting the authorized keys for commits whose parent lack the
'.guix-authorizations' file."
- (define start-commit
- (commit-lookup repository start))
+
+ (define intro-commit
+ (commit-lookup repository intro-commit-hash))
+
(define end-commit
(commit-lookup repository end))
@@ -404,36 +433,37 @@ (define keyring
(define authenticated-commits
;; Previously-authenticated commits that don't need to be checked again.
(filter-map (lambda (id)
+ ;; We need to tolerate when cached commits disappear due to
+ ;; --allow-downgrades.
(false-if-git-not-found
(commit-lookup repository (string->oid id))))
(append (previously-authenticated-commits cache-key)
- authentic-commits)))
+ authentic-commits
+ ;; The intro commit is unconditionally trusted.
+ (list (oid->string intro-commit-hash)))))
(define commits
;; Commits to authenticate, excluding the closure of
;; AUTHENTICATED-COMMITS.
- (commit-difference end-commit start-commit
- authenticated-commits))
-
- ;; 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))
-
- (let ((stats (call-with-progress-reporter reporter
- (lambda (report)
- (authenticate-commits repository commits
- #:keyring keyring
- #:default-authorizations
- historical-authorizations
- #:report-progress report)))))
- (cache-authenticated-commit cache-key
- (oid->string (commit-id end-commit)))
-
- stats))))
+ (commit-difference end-commit intro-commit
+ authenticated-commits))
+
+ (verify-introductory-commit repository intro-commit
+ intro-signer keyring
+ historical-authorizations)
+
+ (let* ((reporter (make-reporter intro-commit end-commit commits))
+ (stats (call-with-progress-reporter reporter
+ (lambda (report)
+ (authenticate-commits repository commits
+ #:keyring keyring
+ #:default-authorizations
+ historical-authorizations
+ #:report-progress report)))))
+ ;; Note that this will make the then current end commit of any channel,
+ ;; that has been used/trusted in the past with a channel introduction,
+ ;; remain trusted until the cache is cleared.
+ (cache-authenticated-commit cache-key
+ (oid->string (commit-id end-commit)))
+
+ stats))
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH 5/5] tests: Add test for .guix-authorizations and channel intro.
2021-10-18 15:57 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
` (2 preceding siblings ...)
2021-10-18 15:57 ` [bug#50814] [PATCH 4/5] guix: git-authenticate: Fix authenticate-repository Attila Lendvai
@ 2021-10-18 15:57 ` Attila Lendvai
2022-01-10 14:53 ` [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Ludovic Courtès
3 siblings, 1 reply; 38+ messages in thread
From: Attila Lendvai @ 2021-10-18 15:57 UTC (permalink / raw)
To: 50814; +Cc: Attila Lendvai
This test used to fail before a recent fix to authenticate-repository.
* tests/git-authenticate.scm: New test "signed commits, .guix-authorizations,
channel-introduction".
---
tests/git-authenticate.scm | 150 +++++++++++++++++++++++++++++++++++++
1 file changed, 150 insertions(+)
diff --git a/tests/git-authenticate.scm b/tests/git-authenticate.scm
index f66ef191b0..25b4962ea4 100644
--- a/tests/git-authenticate.scm
+++ b/tests/git-authenticate.scm
@@ -18,6 +18,7 @@
(define-module (test-git-authenticate)
#:use-module (git)
+ #:use-module (guix diagnostics)
#:use-module (guix git)
#:use-module (guix git-authenticate)
#:use-module (guix openpgp)
@@ -28,6 +29,10 @@ (define-module (test-git-authenticate)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-64)
#:use-module (rnrs bytevectors)
+ #:use-module ((rnrs conditions)
+ #:select (warning?))
+ #:use-module ((rnrs exceptions)
+ #:select (with-exception-handler))
#:use-module (rnrs io ports))
;; Test the (guix git-authenticate) tools.
@@ -226,6 +231,151 @@ (define (correct? c commit)
#:keyring-reference "master")
#f)))))))
+(unless (gpg+git-available?) (test-skip 1))
+(test-assert "signed commits, .guix-authorizations, channel-introduction"
+ (let* ((result #true)
+ (key1 %ed25519-public-key-file)
+ (key2 %ed25519-2-public-key-file)
+ (key3 %ed25519-3-public-key-file))
+ (with-fresh-gnupg-setup (list key1 %ed25519-secret-key-file
+ key2 %ed25519-2-secret-key-file
+ key3 %ed25519-3-secret-key-file)
+ (with-temporary-git-repository dir
+ `((checkout "keyring" orphan)
+ (add "signer1.key" ,(call-with-input-file key1 get-string-all))
+ (add "signer2.key" ,(call-with-input-file key2 get-string-all))
+ (add "signer3.key" ,(call-with-input-file key3 get-string-all))
+ (commit "keyring commit")
+
+ (checkout "main" orphan)
+ (add "noise0")
+ (add ".guix-authorizations"
+ ,(object->string
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ ;; Notice that key2 is not authorized at this point.
+ (,(key-fingerprint key3) (name "Charlie"))))))
+ (commit "commit 0" (signer ,(key-fingerprint key3)))
+ (add "noise1")
+ (commit "commit 1" (signer ,(key-fingerprint key1)))
+ (add "noise2")
+ (commit "commit 2" (signer ,(key-fingerprint key1))))
+ (with-repository dir repo
+ (let* ((commit-0 (find-commit repo "commit 0"))
+ (check-from
+ (lambda* (commit #:key (should-fail? #false) (key key1)
+ (historical-authorizations
+ ;; Let's mark key3 to be trusted
+ ;; unconditionally, so that it authorizes
+ ;; commit 0.
+ (list (key-fingerprint-vector key3))))
+ (guard (c ((unauthorized-commit-error? c)
+ (if should-fail?
+ c
+ (let ((port (current-output-port)))
+ (format port "FAILURE: Unexpected exception at commit '~s':~%"
+ commit)
+ (print-exception port (stack-ref (make-stack #t) 1)
+ c (exception-args c))
+ (set! result #false)
+ '()))))
+ (format #true "~%~%Checking ~s, should-fail? ~s, repo commits:~%"
+ commit should-fail?)
+ ;; To be able to inspect git's state in the logs.
+ (invoke "git" "-C" dir "log" "--reverse" "--pretty=oneline" "main")
+ (set! commit (find-commit repo commit))
+ (authenticate-repository repo
+ (commit-id commit)
+ (key-fingerprint-vector key)
+ #:historical-authorizations
+ historical-authorizations)
+ (when should-fail?
+ (format #t "FAILURE: Authenticating commit '~s' should have failed.~%" commit)
+ (set! result #false))
+ '()))))
+ (check-from "commit 0" #:key key3)
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (with-git-repository dir
+ `((add "noise 3")
+ (commit "commit 3" (signer ,(key-fingerprint key2))))
+ ;; This should fail because it is signed by key2, i.e. an
+ ;; unauthorized key.
+ (check-from "commit 3" #:should-fail? #true)
+ ;; Specify commit 3 as a channel-introduction signed with
+ ;; key2. This is valid, but it should warn the user, because
+ ;; .guix-authorizations is not updated to include key2, which
+ ;; means that any subsequent commits with the same key will be
+ ;; rejected.
+ (set! result
+ (and (let ((signalled? #false))
+ (with-exception-handler
+ (lambda (c)
+ (cond
+ ((not (warning? c))
+ (raise c))
+ ((formatted-message? c)
+ (format #true "warning (expected): ~a~%"
+ (apply format #false
+ (formatted-message-string c)
+ (formatted-message-arguments c)))
+ (set! signalled? #true)))
+ '())
+ (lambda ()
+ (check-from "commit 3" #:key key2)
+ (unless signalled?
+ (format #t "FAILURE: No warning signalled for commit 3~%"))
+ signalled?)))
+ result)))
+ (with-git-repository dir
+ ;; Drop the faulty commit 3
+ `((reset ,(oid->string (commit-id (find-commit repo "commit 2"))))
+ (add "noise 4")
+ (add ".guix-authorizations"
+ ,(object->string
+ ;; Remove key3, add key2.
+ `(authorizations
+ (version 0)
+ ((,(key-fingerprint key1) (name "Alice"))
+ (,(key-fingerprint key2) (name "Bob"))))))
+ (commit "commit 4" (signer ,(key-fingerprint key2))))
+ ;; This should fail because even though commit 4 adds key2 to
+ ;; .guix-authorizations, but commit 1 was created prior to that,
+ ;; therefore it is not authorized.
+ (check-from "commit 1" #:should-fail? #true)
+ ;; This should pass, because it's a valid channel intro at commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 5")
+ (commit "commit 5" (signer ,(key-fingerprint key2))))
+ ;; 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")
+ ;; Should still be fine, but only when starting from commit 4
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 6")
+ (commit "commit 6" (signer ,(key-fingerprint key1))))
+ (check-from "commit 1")
+ (check-from "commit 2")
+ (check-from "commit 4" #:key key2))
+ (with-git-repository dir
+ `((add "noise 7")
+ (commit "commit 7" (signer ,(key-fingerprint key3))))
+ ;; This should fail because key3 is not among the authorized
+ ;; keys anymore, and commit 7 is signed by it.
+ (check-from "commit 6" #:should-fail? #true))))))
+ result))
+
(unless (gpg+git-available?) (test-skip 1))
(test-assert "signed commits, .guix-authorizations, authorized merge"
(with-fresh-gnupg-setup (list %ed25519-public-key-file
--
2.33.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-10-18 15:57 ` [bug#50814] [PATCH 5/5] tests: Add test for .guix-authorizations and channel intro Attila Lendvai
@ 2022-01-10 14:53 ` Ludovic Courtès
0 siblings, 0 replies; 38+ messages in thread
From: Ludovic Courtès @ 2022-01-10 14:53 UTC (permalink / raw)
To: Attila Lendvai; +Cc: 50814
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:
> This test used to fail before a recent fix to authenticate-repository.
>
> * tests/git-authenticate.scm: New test "signed commits, .guix-authorizations,
> channel-introduction".
I won’t comment on the whole test for now, because it’s too complex, but
here’s what I noticed:
[...]
> + (checkout "main" orphan)
> + (add "noise0")
> + (add ".guix-authorizations"
> + ,(object->string
> + `(authorizations
> + (version 0)
> + ((,(key-fingerprint key1) (name "Alice"))
> + ;; Notice that key2 is not authorized at this point.
> + (,(key-fingerprint key3) (name "Charlie"))))))
> + (commit "commit 0" (signer ,(key-fingerprint key3)))
> + (add "noise1")
> + (commit "commit 1" (signer ,(key-fingerprint key1)))
> + (add "noise2")
> + (commit "commit 2" (signer ,(key-fingerprint key1))))
> + (with-repository dir repo
> + (let* ((commit-0 (find-commit repo "commit 0"))
> + (check-from
> + (lambda* (commit #:key (should-fail? #false) (key key1)
> + (historical-authorizations
> + ;; Let's mark key3 to be trusted
> + ;; unconditionally, so that it authorizes
> + ;; commit 0.
> + (list (key-fingerprint-vector key3))))
This is exercising support for “historical authorizations”, which works
slightly differently than the regular ‘.guix-authorizations’-process
used by ‘guix pull’ & co.
> + (set! commit (find-commit repo commit))
Mutation is making it harder for me to understand what the test does.
> + (with-git-repository dir
> + ;; Drop the faulty commit 3
> + `((reset ,(oid->string (commit-id (find-commit repo "commit 2"))))
> + (add "noise 4")
> + (add ".guix-authorizations"
> + ,(object->string
> + ;; Remove key3, add key2.
> + `(authorizations
> + (version 0)
> + ((,(key-fingerprint key1) (name "Alice"))
> + (,(key-fingerprint key2) (name "Bob"))))))
> + (commit "commit 4" (signer ,(key-fingerprint key2))))
Due to ‘reset’ here, the intro commit that ‘check-from’ passes to
‘authenticate-repository’ is no longer the right one (IIUC).
> + ;; This should fail because even though commit 4 adds key2 to
> + ;; .guix-authorizations, but commit 1 was created prior to that,
> + ;; therefore it is not authorized.
> + (check-from "commit 1" #:should-fail? #true)
> + ;; This should pass, because it's a valid channel intro at commit 4
> + (check-from "commit 4" #:key key2))
> + (with-git-repository dir
> + `((add "noise 5")
> + (commit "commit 5" (signer ,(key-fingerprint key2))))
> + ;; 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).
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’.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit.
2021-09-26 10:19 [bug#50814] [PATCH] guix: git-authenticate: Also authenticate the channel intro commit Attila Lendvai
` (6 preceding siblings ...)
2021-10-18 15:57 ` [bug#50814] [PATCH 1/5] tests: Smarten up git repository testing framework Attila Lendvai
@ 2022-04-04 6:47 ` Attila Lendvai
7 siblings, 0 replies; 38+ messages in thread
From: Attila Lendvai @ 2022-04-04 6:47 UTC (permalink / raw)
To: 50814@debbugs.gnu.org
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)
^ permalink raw reply [flat|nested] 38+ messages in thread