GNU bug report logs - #45409
[PATCH 0/3] Move some (guix scripts substitute) code to two new modules

Previous Next

Full log

Message #59 received at 45409 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 45409 <at> debbugs.gnu.org
Subject: Re: [bug#45409] [PATCH v3 1/3] substitute: Untangle skipping
 authentication from valid-narinfo?.
Date: Tue, 05 Jan 2021 22:58:17 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> Rather than having valid-narinfo? evaluate to #t if
>> %allow-unauthenticated-substitutes? is set to #t, just use (const #t) for
>> valid-narinfo? when %allow-unauthenticated-substitutes? is set to #t.  This
>> will allow moving valid-narinfo? in to a (guix substitutes) module.
>>
>> * guix/scripts/substitute.scm (process-query, process-substitution): Change
>> the authorized? argument to lookup-narinfo and lookup-narinfos/diverse based
>> on %allow-unauthenticated-substitutes?.
>> (valid-narinfo?): Remove use of %allow-unauthenticated-substitutes?.
>
> Bummer that there are two call sites.
>
> What about doing away with ‘%allow-unauthenticated-substitutes?’ and
> instead changing its only user, ‘tests/substitute.scm’, like so:
>
> diff --git a/tests/substitute.scm b/tests/substitute.scm
> index 542aaf603f..1827ffe8d4 100644
> --- a/tests/substitute.scm
> +++ b/tests/substitute.scm
> @@ -178,10 +178,10 @@ a file for NARINFO."
>          (call-with-output-file
>              (string-append narinfo-directory "/example.nar")
>            (cute write-file
> -                (string-append narinfo-directory "/example.out") <>))
> -
> -        (%allow-unauthenticated-substitutes? #f))
> -      thunk
> +                (string-append narinfo-directory "/example.out") <>)))
> +      (lambda ()
> +        (mock ((guix narinfo) valid-narinfo?) (const #t)
> +              (thunk)))
>        (lambda ()
>          (when (file-exists? cache-directory)
>            (delete-file-recursively cache-directory))))))
>
> That change would have to be made in the patch that creates (guix
> narinfo).
>
> WDYT?

I don't know what's up with these tests in particular, adding peek in
places makes tests fail... not using Guile debugging helpers and
outputting to (current-error-port) seems to not change the result
though.

I didn't really understand this code, but looking at it more, I'm
thinking now that what it actually does is affects all the tests, and
for some tests in the (tests substitute) module, the
%allow-unauthenticated-substitutes? behaviour is turned off.

Commenting out the relevant code in the script seems to support this,
the substitute tests still pass, but tests in the store, derivation and
guix-daemon modules fail. The substitute tests are actually fine, and
break if you disable substitute authentication. The mock approach is
probably feasible, but it would need to be done in those
modules/tests. I haven't looked at the details, but I'd be a little
concerned that it might require mocking in each of the individual 15
failing tests, maybe that's good for being explicit though?

Back to the use of %allow-unauthenticated-substitutes? in the code,
there are two call sites, for the two separate code paths, but it would
be pretty easy to move to one call site. Both process-query and
process-substitution take an acl, but they could instead take some
(valid? obj) procedure. That would either call (valid-narinfo? obj acl)
or just evaluate to #t in the allow unauthorized case. This effectively
moves the logic and call site to the command.

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.