[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#45409] [PATCH v3 1/3] substitute: Untangle skipping authentication
From: |
Ludovic Courtès |
Subject: |
[bug#45409] [PATCH v3 1/3] substitute: Untangle skipping authentication from valid-narinfo?. |
Date: |
Wed, 06 Jan 2021 09:37:16 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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’.
[bug#45409] [PATCH v3 2/3] guix: Move narinfo code from substitute script to module., Christopher Baines, 2021/01/04
[bug#45409] [PATCH v3 1/3] substitute: Untangle skipping authentication from valid-narinfo?., Ludovic Courtès, 2021/01/05