Message ID | 871rezt5cd.fsf@gnu.org |
---|---|
State | Superseded |
Headers | show |
Series | [bug#45409,v3,1/3] substitute: Untangle skipping authentication from valid-narinfo?. | expand |
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’.
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))))))