[bug#45409,v3,1/3] substitute: Untangle skipping authentication from valid-narinfo?.

Hi,

Christopher Baines <mail@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:
That change would have to be made in the patch that creates (guix
narinfo).

WDYT?

Ludo’.

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

> Hi,
>
> Christopher Baines <mail@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.

Hi,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi,
>>
>> Christopher Baines <mail@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:

My bad, I missed that ‘test-env’ does:

  GUIX_ALLOW_UNAUTHENTICATED_SUBSTITUTES=yes

So what I proposed won’t work.

All in all, let’s just take the patch you proposed.  Sorry for the
confusion!

> 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.

Yeah that’s because (current-output-port) is used to communicate with
the daemon, so if you inadvertently write things there, it breaks.

> 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.

Yeah, I got the logic wrong.

> 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.

Yeah but the patch you proposed is fine.

Thanks and apologies for the back-and-forth!

Ludo’.