| Message ID | 20200520213802.2170-1-ludo@gnu.org |
|---|---|
| Headers | show |
Hi Ludo, On Wed, 20 May 2020 at 23:39, Ludovic Courtès <ludo@gnu.org> wrote: > By default ‘guix pull’ would now error out if the target commit of a > channel is not a descendant of the currently-used commit, according to > the commit graph. There’s an option to bypass that. ‘guix > time-machine’ behavior is unchanged though: it never complains. What is the extra time cost of such check? Well, it depends on the "distance" between the 2 commits and maybe the complexity of the graph -- it it not clear what happen for complex merge -- but say pulling once a month. It is not easy -- nor impossible -- to evaluate such cost at the level of "guix pull". And I failed to evaluate it using 'commit-relation' with "guix repl" -- Segmentation fault with commit c81457a5883ea43950eb2ecdcbb58a5b144bcd11 and 4bdf4182fe080c3409f6ef9b410146b67cfa2595; probably because I did used correctly the API. Well, what will be the timing impact of checking the "fast-fowardness"? All the best, simon
Hi Simon, zimoun <zimon.toutoune@gmail.com> skribis: > On Wed, 20 May 2020 at 23:39, Ludovic Courtès <ludo@gnu.org> wrote: > >> By default ‘guix pull’ would now error out if the target commit of a >> channel is not a descendant of the currently-used commit, according to >> the commit graph. There’s an option to bypass that. ‘guix >> time-machine’ behavior is unchanged though: it never complains. > > What is the extra time cost of such check? The problem is not the cost. ‘guix pull’ compares the target commit(s) against the commit(s) of the currently-used ‘guix’; it can clearly see if it’s a forward pull or not. However, in the case of ‘guix time-machine’, there’s nothing to compare against (it’s a bit like a fresh ‘git clone’ as opposed to a ‘git pull’, if you see what I mean.) Additionally, the purpose of ‘guix time-machine’ is to travel in time, usually in the past, so it would be inconvenient to get warnings or errors every time. > It is not easy -- nor impossible -- to evaluate such cost at the level > of "guix pull". And I failed to evaluate it using 'commit-relation' > with "guix repl" -- Segmentation fault with commit > c81457a5883ea43950eb2ecdcbb58a5b144bcd11 and > 4bdf4182fe080c3409f6ef9b410146b67cfa2595; probably because I did used > correctly the API. How can I reproduce the issue? > Well, what will be the timing impact of checking the "fast-fowardness"? I haven’t measured it, but it’s small compared to the cost of fetching the new revisions and performing the checkout. It’s roughly what ‘git pull’ does, although ‘git pull’ is probably faster because it’s in C and has been well optimized over the years. Thanks for your feedback! Ludo’.
Pushed! 9744cc7b46 pull: Protect against downgrade attacks. 872898f768 channels: 'latest-channel-instances' guards against non-forward updates. 8d1d56578a git: 'update-cached-checkout' returns the commit relation. 9b049de84e channels: 'latest-channel-instances' doesn't leak internal state. c098c11be8 git: Add 'commit-relation'. One step closer to addressing <https://issues.guix.gnu.org/22883>… Ludo’.
On Fri, 22 May 2020 at 15:56, Ludovic Courtès <ludo@gnu.org> wrote: > > It is not easy -- nor impossible -- to evaluate such cost at the level > > of "guix pull". And I failed to evaluate it using 'commit-relation' > > with "guix repl" -- Segmentation fault with commit > > c81457a5883ea43950eb2ecdcbb58a5b144bcd11 and > > 4bdf4182fe080c3409f6ef9b410146b67cfa2595; probably because I did used > > correctly the API. Obviously, one had to read "probably I did *not* used correctly the API". :-) > How can I reproduce the issue? --8<---------------cut here---------------start------------->8--- (use-modules (guix git) (guix channels) (guix tests git) (git)) (define url-cache-directory (@@ (guix git) url-cache-directory)) (define dir (url-cache-directory (channel-url (car %default-channels)))) (define merge (with-repository dir repo (find-commit repo "Merge"))) merge ;; $1 = #<git-commit 4bdf4182fe080c3409f6ef9b410146b67cfa2595> (define left (car (commit-parents merge))) left ;; $2 = #<git-commit c81457a5883ea43950eb2ecdcbb58a5b144bcd11> (commit-relation left merge) Segmentation fault --8<---------------cut here---------------end--------------->8--- Because of 'commit-closure'. I do not know if it is the correct use of the API; and because I do not know how to get easily a commit, I use 'find-commit' which is not nice. > > Well, what will be the timing impact of checking the "fast-fowardness"? > > I haven’t measured it, but it’s small compared to the cost of fetching > the new revisions and performing the checkout. It’s roughly what ‘git > pull’ does, although ‘git pull’ is probably faster because it’s in C and > has been well optimized over the years. My "worry" is about the complexity of the graph because 'commit-relation' walks somehow the graph of commits. Cheers, simon
Hi, zimoun <zimon.toutoune@gmail.com> skribis: > (use-modules (guix git) (guix channels) (guix tests git) (git)) > (define url-cache-directory (@@ (guix git) url-cache-directory)) > (define dir (url-cache-directory (channel-url (car %default-channels)))) > (define merge (with-repository dir repo (find-commit repo "Merge"))) > merge > ;; $1 = #<git-commit 4bdf4182fe080c3409f6ef9b410146b67cfa2595> > (define left (car (commit-parents merge))) > left > ;; $2 = #<git-commit c81457a5883ea43950eb2ecdcbb58a5b144bcd11> > (commit-relation left merge) > Segmentation fault It took me a while to notice, but the problem with the code above is that ‘repo’ is closed when you call ‘commit-relation’, and thus the commit objects are invalid. It works if you keep ‘repo’ alive: --8<---------------cut here---------------start------------->8--- $ guix describe Generacio 145 May 25 2020 00:37:58 (nuna) guix 9744cc7 repository URL: https://git.savannah.gnu.org/git/guix.git branch: master commit: 9744cc7b4636fafb772c94adb8f05961b5b39f16 $ guix repl GNU Guile 3.0.2 Copyright (C) 1995-2020 Free Software Foundation, Inc. Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'. This program is free software, and you are welcome to redistribute it under certain conditions; type `,show c' for details. Enter `,help' for help. scheme@(guix-user)> (use-modules (guix git) (guix channels) (guix tests git) (git)) (define url-cache-directory (@@ (guix git) url-cache-directory)) (define dir (url-cache-directory (channel-url (car %default-channels)))) ;;; <stdin>:2:0: warning: possibly unused local top-level variable `url-cache-directory' ;;; <stdin>:3:0: warning: possibly unused local top-level variable `dir' scheme@(guix-user)> (define repo (repository-open dir)) ;;; <stdin>:4:0: warning: possibly unused local top-level variable `repo' scheme@(guix-user)> (define merge (find-commit repo "Merge")) ;;; <stdin>:5:0: warning: possibly unused local top-level variable `merge' scheme@(guix-user)> merge $1 = #<git-commit b4440de133401abc6ce8be6c1c2e720efd9b2ba3> scheme@(guix-user)> (define left (car (commit-parents merge))) left ;;; <stdin>:7:0: warning: possibly unused local top-level variable `left' $2 = #<git-commit 141262f266ab702c856f634889d4130ae661e79f> scheme@(guix-user)> (commit-relation left merge) $3 = ancestor scheme@(guix-user)> (gc) scheme@(guix-user)> (commit-relation left merge) $4 = ancestor --8<---------------cut here---------------end--------------->8--- The solution in such cases is to synchronize the object lifetimes. In this case, commits would keep a reference to the repository object to prevent it from being GC’d, as is done with ‘%submodule-owners’ in (git submodule). Could you make an issue over at <https://gitlab.com/guile-git/guile-git>? Thanks, Ludo’.
Hello! This patch series aims to protect against “downgrade attacks”, whereby a “guix pull” command would in fact deploy an older or an unrelated revision of Guix, potentially leading you to install vulnerable or malicious software. By default ‘guix pull’ would now error out if the target commit of a channel is not a descendant of the currently-used commit, according to the commit graph. There’s an option to bypass that. ‘guix time-machine’ behavior is unchanged though: it never complains. This is generally useful and it’s a requirement for authenticated checkouts as discussed in <https://issues.guix.gnu.org/22883>, otherwise one could easily escape the intended authentication scheme by branching and providing a different ‘.guix-authorizations’ file. Feedback welcome! Ludo’. Ludovic Courtès (5): git: Add 'commit-relation'. channels: 'latest-channel-instances' doesn't leak internal state. git: 'update-cached-checkout' returns the commit relation. channels: 'latest-channel-instances' guards against non-forward updates. pull: Protect against downgrade attacks. doc/guix.texi | 15 ++++ guix/channels.scm | 156 ++++++++++++++++++++++++++++++------------ guix/git.scm | 37 ++++++++-- guix/import/opam.scm | 2 +- guix/scripts/pull.scm | 35 +++++++++- tests/channels.scm | 47 +++++++++++-- tests/git.scm | 42 +++++++++++- 7 files changed, 276 insertions(+), 58 deletions(-)