Fix `builtins.storePath` in pure mode #8090

roberth · 2023-03-21T22:56:45Z

Motivation

This allows packaging nix-built proprietary programs without forcing uses to deal with impurities, and unblocks two other use cases.

Context

This is now blocking a customer of mine, and it is also blocking

@edolstra has previously rejected this idea in #5868

It may not be impure, but it does make builds non-reproducible: it prevents rebuilding a flake unless you already have the store path or you have a substituter that can provide it.

but @tomberek:

I'm leaning toward the simpler approach and removing the restriction from the builtin. The loss of reproducibility would be identical compared to declaring any src via git/github/url/mirrors with regards to it being possible to be unable to fetch + reproduce the content if the source has changed/moved/permissions/etc. In some ways this would be more resilient due to substituters being utilized as the mechanism to get their output, they are similar to having mirrors available.

If the new behavior is still not acceptable, would a new option be ok? Such an option allows users opt in to this behavior without opening the impure floodgates.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

roberth · 2023-03-22T00:16:10Z

I would really like to avoid writing a NixOS module that fills in the store path dependencies on activation.
That breaks properties of NixOS, so it can't believe that that would be a better solution, and frankly it's not, because it breaks deployment into firewalled networks.

edolstra · 2023-03-22T10:43:18Z

If the new behavior is still not acceptable, would a new option be ok?

It probably should be a different setting than restrict-eval, since that's intended to mean "only allow access to the paths/URLs in NIX_PATH / allowed-uri". It's best not to overload it (even more).

tomberek · 2023-03-23T05:28:19Z

A change like this should be present in the non-CA variants of builtins.fetchClosure which are very similar to builtins.storePath, just with extra substituter information

edolstra · 2023-03-23T08:26:57Z

Oh right, I forgot about fetchClosure. @roberth Would fetchClosure work for your use case?

roberth · 2023-03-23T12:38:05Z

A change like this should be present in the non-CA variants of builtins.fetchClosure which are very similar to builtins.storePath, just with extra substituter information

Done. I wasn't aware of its non-CA behavior, so thanks @tomberek :)

Oh right, I forgot about fetchClosure. @roberth Would fetchClosure work for your use case?

Content addressing, as is currently required in pure mode, would be too risky.

This is useful for - packaging and deploying non-OSS software with flakes. - bringing the shell and coreutils packages used by the test suite into the test VM (NixOS#6735) The test VM actually uses a proper sandbox, so we wouldn't be able to get away with reading those files impurely in the builder anymore. Note that neither use would be feasible with `builtins.fetchClosure`, because ca-derivations is too experimental and in principle not guaranteed to work for all builds that do support input addressing.

roberth · 2023-05-01T12:22:11Z

Will reimplement this on fetchClosure as agreed in the meeting today.
New fetchClosure syntax:

  builtins.fetchClosure {
     # specify one or two of:
     inputAddressedPath = 
     contentAddressedPath = 
  }

fricklerhandwerk · 2023-05-02T09:10:52Z

Discussed in the Nix team meeting 2023-05-01:

Closing this PR. @roberth will change builtins.fetchClosure to require an explicit distinction of input-addressed and content-addressed paths.

Complete discussion

@tomberek: Options:
1. do nothing
2. builtins.fetchClosure with input-address is pure
3. all builtins.storePath and builtins.fetchClosure is pure
@tomberek: what is the definition of purity?
- @edolstra: no undeclared input, network, etc. On success it produces the expected result.
- @edolstra: already possible in impure mode
@tomberek: fetchClosure would be better than storePath for reproducibility (point 2)
- @edolstra: the principle for pure mechanisms is that they shouldn't require configuration or ambient authority
  @tomberek: an actual definition of purity would be helpful. Ideas
- safe caching is implied by purity
- a degree of reproducibility (e.g. input addressed)
- security: you can pull things from places that an expression shouldn't access
- restriction of scope
  @tomberek: (2) does better than (3) in restriction of scope and reproducibility
- @tomberek: equational reasoning
  - @roberth: relates to safe caching
  - @tomberek: e.g. network status can not be observed within an expression
- @roberth: independent of configuration
  - @tomberek: consider no access to the internet.
  - @tomberek: a store path has less reliance on the language and evaluation of Nixpkgs etc etc, but instead only relies on the store layer. In some sense more reliable. Less chance of something failing.
@tomberek: leaning towards (2)
- @edolstra: (2) ok, except one problem. With something like fetchUrl, you have to provide a hash, with fetchClosure there's no content hash. For purity we'd need some closure hash.
- @tomberek: the store objects are already inside the Nix model. Does that mean we need the full closure hash, or can we inherit the purity semantics of store objects.
- @tomberek: we don't provide bitwise repro. We only provide that with CA.
@edolstra: what if fetchClosure is input to a CA derivation?
- @tomberek: is it an input source or input derivation?
- @edolstra: source
- @edolstra: if you use input addressed input in CA you're asking for trouble
  - @tomberek: should it be an error?
  - @edolstra: it's up to the user to make their build output reproducible
- @edolstra: maybe we don't need the closure hash. We already assume that store paths are produced in a good way.
- @tomberek: we are inheriting whatever the properties are of the upstream object. It is already inside the Nix model/ Nix world. It's not weakening it.
@tomberek: it's an experimental feature. We can iterate on it. Another reason to prefer 2 over 3.
- @edolstra: is this a matter of removing a check? Maybe add an attribute contentAddressed = false to lift the restriction.
@tomberek: isn't fromPath/toPath sufficient?
- @edolstra: toPath has to be CA if you're in pure mode
- @tomberek: what if we only allow input-addressed fromPath by itself:
  - @tomberek: remove the restriction at fetchClosure.cc:109?
  - @edolstra: want to be explicit about it

drafting on a scratch pad:

builtins.fetchClosure {
  fromPath = /nix/store/abc;
  toPath = ...; # implies contentAddressed = true
  contentAddressed = false;
}
builtins.fetchClosure {
  toPath = "/asdfas/asdfasdf"   # always means CA
}
builtins.fetchClosure {
  fromPath = "/asdfas/asdfasdf"  # always means input-addressed
}
builtins.fetchClosure {
   inputAddressedPath = 
   contentAddressedPath = 
}

@edolstra: { toPath } vs { fromPath } is too implicit
- @tomberek: we can do more checks, or have more explicit names
- @edolstra: inputAddressedPath and contentAddressedPath are nice
  - @roberth: more explicit
  - @edolstra: except for the direction; not immediately clear in which direction the rewriting happens
@roberth: will reimplement using the new syntax in fetchClosure [option 2]
@roberth closes PR.

nixos-discourse · 2023-05-02T09:11:59Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-01-nix-team-meeting-minutes-51/27803/1

nixos-discourse · 2023-05-16T21:34:44Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/flox-nix-community-update-2023-04/28233/1

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

@edolstra

When explicitly requested by the caller, as suggested in the meeting (NixOS#8090 (comment)) > @edolstra: { toPath } vs { fromPath } is too implicit I've opted for the `inputAddressed = true` requirement, because it we did not agree on renaming the path attributes. > @roberth: more explicit > @edolstra: except for the direction; not immediately clear in which direction the rewriting happens This is in fact the most explicit syntax and a bit redundant, which is good, because that redundancy lets us deliver an error message that reminds expression authors that CA provides a better experience to their users.

roberth added language The Nix expression language; parser, interpreter, primops, evaluation, etc fetching Networking with the outside (non-Nix) world, input locking labels Mar 21, 2023

roberth requested a review from edolstra as a code owner March 21, 2023 22:56

roberth changed the title ~~Fix builtins store path in pure mode~~ Fix builtins.storePath in pure mode Mar 22, 2023

roberth force-pushed the fix-builtins-storePath-in-pure-mode branch from 0d52b1f to 4d3b009 Compare March 24, 2023 14:14

roberth added 2 commits March 24, 2023 18:41

Add enable-store-path-references option

ce8bc8b

roberth force-pushed the fix-builtins-storePath-in-pure-mode branch from 4d3b009 to ce8bc8b Compare March 24, 2023 17:41

roberth closed this May 1, 2023

roberth mentioned this pull request May 19, 2023

fetchClosure: input addressed and pure #8370

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `builtins.storePath` in pure mode #8090

Fix `builtins.storePath` in pure mode #8090

roberth commented Mar 21, 2023

roberth commented Mar 22, 2023

edolstra commented Mar 22, 2023

tomberek commented Mar 23, 2023 •

edited

Loading

edolstra commented Mar 23, 2023

roberth commented Mar 23, 2023

roberth commented May 1, 2023

fricklerhandwerk commented May 2, 2023

nixos-discourse commented May 2, 2023

nixos-discourse commented May 16, 2023

Fix builtins.storePath in pure mode #8090

Fix builtins.storePath in pure mode #8090

Conversation

roberth commented Mar 21, 2023

Motivation

Context

Checklist for maintainers

Priorities

roberth commented Mar 22, 2023

edolstra commented Mar 22, 2023

tomberek commented Mar 23, 2023 • edited Loading

edolstra commented Mar 23, 2023

roberth commented Mar 23, 2023

roberth commented May 1, 2023

fricklerhandwerk commented May 2, 2023

nixos-discourse commented May 2, 2023

nixos-discourse commented May 16, 2023

Fix `builtins.storePath` in pure mode #8090

Fix `builtins.storePath` in pure mode #8090

tomberek commented Mar 23, 2023 •

edited

Loading