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

[Ludovic Courtès]

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