| Message ID | 20210926101928.3877-1-attila@lendvai.name |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#50814] guix: git-authenticate: Also authenticate the channel intro commit. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
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.
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.
just a quick update that i'm working on putting together an extensive test for this, and a fix. - attila PGP: 5D5F 45C7 DFCD 0A39
what should happen if a channel-introduction commit does not update the .guix-authorizations file? this means that this single commit will get through the authentication (channel intro commits are always trusted), but any later commits signed with the same key will be rejected. this is the situation that got me confused, and thrust me into attempting to fix this (trying to `guix pull` from a local git checkout of guix containing my patches). i wrote the code for this, but i don't know what should be its UI. how should this be reported to the user? using `(warning ...)` will just print something to stderr. i was hoping to raise a continuable condition of type &warning, that i can even check for in the tests, but i have failed to put that together. the scheme/guile condition system is a bit messy/convoluted. can someone help me out with a hint/outline about how to report this that best fits the rest of guix? note that it's not really an error, because until another commit is added with the new key, this channel is valid. - attila PGP: 5D5F 45C7 DFCD 0A39
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.
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’.
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
> 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.
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’.
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!
> 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.
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’.
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)
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)
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))) (let ((stats (call-with-progress-reporter reporter (lambda (report)