Revert "Merge pull request #9902 from NixOS/require-fixed-output-fetchurl" #9911

puckipedia · 2024-02-02T14:42:56Z

This reverts commit 081dc5d, reversing changes made to ef6d055.

Motivation

This is not actually a bug on Nix (2.18, at least), and breaks code I've written (with no other way to write that code.)

nix-repl> :b builtins.derivation { name = "nix-cache-info"; system = "x86_64-linux"; builder = "builtin:fetchurl"; url = https://cache.nixos.org/nix-cache-info; outputHashMode = "flat"; }
warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 309 ms
warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 607 ms
warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 1002 ms
warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 2554 ms
error: builder for '/nix/store/z89j6fdfm6k9k1qir99449wyvldpm2ma-nix-cache-info.drv' failed with exit code 1;
       last 4 log lines:
       > error:
       >        … writing file '/nix/store/l9ax62fdagg4cvw9wd4h84cf4pxqcmlg-nix-cache-info'
       >
       >        error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6)
       For full logs, run 'nix-store -l /nix/store/z89j6fdfm6k9k1qir99449wyvldpm2ma-nix-cache-info.drv'.

Context

Reverts #9902.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

edolstra · 2024-02-02T14:49:40Z

Being able to download files without specifying a hash is 100% a very serious bug, so we cannot revert this bug fix, as annoying as that may be to users who were relying on this bug. (It's also worth noting that builtin:fetchurl is not documented. The wrapper <nix/fetchurl.nix> does require an outputHash.)

puckipedia · 2024-02-02T14:55:07Z

What do you mean? Have you *tried* running this on a properly sandboxed Nix install? I literally ran your example that showed the bug in action and it can't access the internet. I also never said that I used it to download files, anyways.

As mentioned in my talk on Zilch, I use builtin:fetchurl as an easy way to unpack NAR files that were created by other derivations. Unless you can provide me another way to create arbitrary directory structures that depend on the outputs of other arbitrary derivations, while depending solely on what the Nix sandbox provides, and in a way that is backwards compatible, I'd be happy to use that instead. But I believe that's not something that will happen.

And something being undocumented may make it "unstable" from your PoV, but you're not the only user of Nix. Did 9999years go through this much effort keeping an undocumented assumption (that nix-instantiate --eval output is stable) only for you to be the unilateral decider on what is or is not stable?

thufschmitt · 2024-02-02T14:58:11Z

I didn't notice that

this doesn't actually run builtin:fetchurl inside a FOD, which means that the example given only succeeds if run with sandboxing disabled

This is actually fairly sensible (and you already do that same thing with a non-FOD derivation that calls curl).

That being said, I'm not sure that we want that in Nix itself, since it can be emulated (and honestly, these fetchers are mostly hacks so that Nix can do some basic operation without depending on Nixpkgs). @puckipedia, what's the reason for using builtins:fetchurl instead of a custom derivation? Just the closure size?

…-fetchurl" This is not actually a bug on Nix (version 2.18, at least), and breaks code I've written (with no other way to write that code.) nix-repl> :b builtins.derivation { name = "nix-cache-info"; system = "x86_64-linux"; builder = "builtin:fetchurl"; url = https://cache.nixos.org/nix-cache-info; outputHashMode = "flat"; } warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 309 ms warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 607 ms warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 1002 ms warning: error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6); retrying in 2554 ms error: builder for '/nix/store/z89j6fdfm6k9k1qir99449wyvldpm2ma-nix-cache-info.drv' failed with exit code 1; last 4 log lines: > error: > … writing file '/nix/store/l9ax62fdagg4cvw9wd4h84cf4pxqcmlg-nix-cache-info' > > error: unable to download 'https://cache.nixos.org/nix-cache-info': Couldn't resolve host name (6) For full logs, run 'nix-store -l /nix/store/z89j6fdfm6k9k1qir99449wyvldpm2ma-nix-cache-info.drv'. This reverts commit 081dc5d, reversing changes made to ef6d055.

Ericson2314 · 2024-02-02T17:12:01Z

With all do respect @puckipedia, I do get a small bit of https://xkcd.com/1172/ vibes from this. If I understand correctly, you are creating an input-addressed derivation with builtins:fetchurl that relies on sandboxing being disabled to succeed?

There is hopefully another way Nix can provide what you need.

puckipedia · 2024-02-02T17:12:16Z

That being said, I'm not sure that we want that in Nix itself, since it can be emulated (and honestly, these fetchers are mostly hacks so that Nix can do some basic operation without depending on Nixpkgs).

If you do not want this in Nix, open an RFC to remove it properly. I'm will not accept someone seemingly accidentally merging a PR (without even checking the reasoning is correct) removing a feature as the new status quo.

@puckipedia, what's the reason for using builtins:fetchurl instead of a custom derivation? Just the closure size?

As mentioned in my talk on Zilch, I use builtin:fetchurl as an easy way to unpack NAR files that were created by other derivations. This code is part of the bootstrap of Zilch's Nix implementation. I cannot depend on any compiler for any programming language being around, as any of them would depend on the output of this builtin:fetchurl call. All builtin builders (fetchurl, unpack-channel, buildenv) will also run with any system setting, which means that the derivation (and thus its output path) is not dependent on the system being built on.

Does it matter what I use this for, though? I was under the impression the policy of the Nix team was to avoid code breakage as much as possible. And this sure smells like breakage of previously valid code with perfectly valid semantics to me. And if this truly was such a critical security issue, I'd have expected it to be backported to versions of Nix considered stable by downstream users, like Nixpkgs.

puckipedia · 2024-02-02T17:18:02Z

With all do respect @puckipedia, I do get a small bit of https://xkcd.com/1172/ vibes from this. If I understand correctly, you are creating an input-addressed derivation with builtins:fetchurl that relies on sandboxing being disabled to succeed?

As mentioned in the opening message of this PR, builtin derivation builders are not exempt from sandboxing.

Ericson2314 · 2024-02-02T17:18:21Z

Does it matter what I use this for, though?

The point of https://xkcd.com/1172/ is that, very strictly speaking, almost every change can be construed as a breaking change, and there is always some subjectivity as to where the line is drawn. I think everyone agrees that e.g. small timing variations are are not supposed to be stable. I am also suspicious of things that rely on sandboxing being disabled. At the same time, I recognize a strict declaration of sandboxing being disabled not mattering would invalidate macOS support for most of its history.

So I think the best resolution will be to think about what exactly you are using this for, and finding you something else you can use that is acceptable. And of course, we better go and document whatever that thing is (if it needs it) so you can be confident this unfortunate "rug pulled out from under you" will not happen again with the replacement.

puckipedia · 2024-02-02T17:33:52Z

Does it matter what I use this for, though?

The point of https://xkcd.com/1172/ is that, very strictly speaking, almost every change can be construed as a breaking change, and there is always some subjectivity as to where the line is drawn. I think everyone agrees that e.g. small timing variations are are not supposed to be stable. I am also suspicious of things that rely on sandboxing being disabled. At the same time, I recognize a strict declaration of sandboxing being disabled not mattering would invalidate macOS support for most of its history.

If I *wanted* to fetch a remote URL without declaring a derivation as fixed-output on an unsandboxed system, I'd just use curl. Making builtin:fetchurl check whether it is a fixed-output derivation doesn't materially improve anything, or fix a bug (like the example you gave of xkcd 1172). And if this is indeed something that should be changed, I'd want it to be a conscious decision, rather than something that was decided on a whim by one person and based on inaccurate information.

I also believe that having seemingly one person be able to just decide "This feature is unstable" is bad actually, and not having a stability policy in place is a pretty massive footgun. As long as there is no policy, I (and many others) will assume that anything that's been the same for quite a while (e.g. builtin:fetchurl, which has behaved like this since 2015) is likely to be stable, and thus can be relied upon, especially because this is a derivation we're talking about. And this guideline seems to be used by some other Nix team members as well.

So I think the best resolution will be to think about what exactly you are using this for, and finding you something else you can use that is acceptable. And of course, we better go and document whatever that thing is (if it needs it) so you can be confident this unfortunate "rug pulled out from under you" will not happen again with the replacement.

As mentioned in my reply to @edolstra, I use builtin:fetchurl to implement a reasonably backwards-compatible way to create a file tree that contains (usually) directories and symlinks, which can depend on paths contained within derivation outputs. There's no alternative I can think of, and there's no way to do feature detection for this, as it depends on the version of Nix installed on the builder, rather than the version of the evaluator.

Ericson2314 · 2024-02-02T17:40:12Z

which can depend on paths contained within derivation outputs.

Ah! I think this is it? You need to add data with arbitrary references, but all our existing addToStore variations don't support that (except for the one that takes an arbitrary ValidPathInfo, which is very free-form intentionally not really exposed).

Would it work if you could insert arbitrary content-addressed data with arbitrary references? I would support extending nix store add (a follow up from #9815) to do that. It should support self-references too.

puckipedia · 2024-02-02T17:41:15Z

nix store add can't be called from the evaluator. Which, if you looked at the previous messages and/or my talk, you would probably be able to say is where the code runs. I do not care about anything outside the evaluator, as I could just run something better there.

Ericson2314 · 2024-02-02T17:44:55Z

I don't mind having a builtin for it too. Indeed we made the hash conversion builtin and nix hash convert intentionally match.

puckipedia · 2024-02-02T17:48:29Z

How will that builtin interact with derivation outputs? Will it force IFD? will it output a derivation of its own? Will that derivation be buildable on older Nix daemon versions?

I think there's already enough questions for such a builtin that I'd want this "fix" to a non-existent issue be reverted before we can even discuss what color the currently non-existent bikeshed will be.

puckipedia · 2024-02-02T17:50:54Z

..wait, you mean a builtin builder. Yeah, you cannot feature detect those. This means I'd either have to be incompatible with newer versions of the Nix daemon, or older version fo the Nix daemon. There'd be no way to be backward compatible.

Ericson2314 · 2024-02-02T17:52:28Z

I mean an evaluation built-in, but I guess that is basically the same as the builtin fetchers which fetch at eval time and don't require a NAR hash. If you want it deferred and you don't want the hash known in advanced, there are "impure derivations". We could also make builtins:fetchurl support that, as part of that experimental feature (and subject to change).

puckipedia · 2024-02-02T18:02:23Z

Builtin builders and evaluator builtins are very different, and I'd suggest reading up on them before suggesting they are "basically the same."

But, to answer your message: Impure derivations are unstable, and require having code that can be run inside the derivation at that point. derivation { builder = "builtin:fetchurl"; } is not unstable. I'm not replacing a feature that has been unchanging for almost ten years with one that has been marked unstable for less than two, and requires a lot more infrastructure. But even then, there's still no reason to limit builtin:fetchurl to FODs only.

And, repeating myself once more: I'm not going to stand for people being needlessly forced to consider certain features "stable" when someone else can come by and just say "oh this was never meant to be stable" and change it. If the Nix team are unwilling to come up with a stability policy, they get to deal with issues like this.

Ericson2314 · 2024-02-02T18:06:27Z

I know they are different. I said the new evaluator builtin I was thinking of is actually the same as an existing evaluator builtin. I was correcting myself, not correcting you.

@puckipedia you tone makes it very hard to have this conversation. I am very keen on properly documenting the store layer in isolation precisely so this stuff is pinned down, but until that process is complete situations where "bug or feature" is a matter of pinion like this will arise.

I did in fact flip through your slides, but I am not going to watch the entire talk because you don't want to explain what you are trying to do from first principles so we can collaboratively find a new solution that everyone finds agreeable.

I am going to go eat a meal now.

puckipedia · 2024-02-02T18:17:02Z

I want to run a binary in my bootstrap. I use <nix/fetchurl.nix> with the flag executable = true; for this. This binary, for a multitude of reasons, refuses to run directly from the Nix store, as it is a multi-applet binary. To solve this, the binary should be located at $out/toybox. This is impossible to do in pure Nixlang.

To solve this impossibility, I generate a .nar file on the fly, import it into the store without any references, then reimport it using an input-addressed builtin:fetchurl with unpack = true; and with injected references, which allows (most of) this process to be done system-independently. (As an aside, this code originally depended on fixed-output derivations allowing references, but since that was unceremoniously broken a while ago, it now depends on input-addressed builtin:fetchurl.)

I generalised this mechanism to allow creating arbitrary trees of directories and symlinks, as it is a useful primitive to not have to depend as strongly on large derivation closures just to create a single symlink, and thus it should work with both input-addressed and content-addressed store paths.

There is no way to easily replace this with any other existing mechanism: Impure derivations require code to build the tree (or unpack the .nar), and there's no way to create directories or symlinks in Nix (or with the /bin/sh present inside the Nix sandbox). builtins.fetchTarball doesn't allow references. And builtins.fetchurl can't unpack files.

Nonetheless, any new mechanism would still require at least a few versions of Nix in which builtin:fetchurl accepts input-addressed mechanisms, as the Nix daemon and evaluator may not be the same version.

Ericson2314 · 2024-02-02T18:17:29Z

Thanks, also see #9912.

9999years · 2024-02-02T19:01:05Z

Being able to download files without specifying a hash is 100% a very serious bug

What happened to the existing behavior being covered by the stability policy even if it's buggy? PRs get rejected with this rationale all the time. The lack of a written stability policy means that, in practice, only contributors with political cachet can make breaking (or "breaking") changes.

This rationale in particular is pretty goofy because downloading files without specifying a hash is a long-standing documented use-case for functions like builtins.fetchTarball.

EDIT: To clarify, I know that builtins:fetchurl is different than builtins.fetchurl, but if we're changing the behavior of builtins:fetchurl because "being able to download files without specifying a hash is 100% a very serious bug", then we should be consistent and also fix the builtins that allow this behavior.

9999years · 2024-02-02T19:03:21Z

The point of https://xkcd.com/1172/ is that, very strictly speaking, almost every change can be construed as a breaking change, and there is always some subjectivity as to where the line is drawn. I think everyone agrees that e.g. small timing variations are are not supposed to be stable.

I sure thought we could agree that the nix-instantiate output was meant to be human readable as well, but the Nix team didn't like that! Either we need to be serious about "all observable behavior is stable and won't change" or write an actual stability policy and follow it.

thufschmitt · 2024-02-02T23:13:46Z

I don't think this should have been possible in the first place, but since it indeed isn't a correctness issue, we might as well not break user space and keep allowing it (besides, the hack is rather nice).

With that said, let's cool down a little here.
Regardless of whether the change was good or not, there's just no reason for the aggressivity in this thread. It's at best just hiding the (interesting and useful) side of the discussion.

@puckipedia Now you got me curious. You mention that using an FOD doesn't work because of the references, but what are the references you need to have if all you're doing it putting a static binary under $out/bin?

nixos-discourse · 2024-02-03T05:11:10Z

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

https://discourse.nixos.org/t/whats-nixs-stable-api-is-code-in-nix-instantiate-part-of-it/37986/6

puckipedia · 2024-02-03T06:35:48Z

I don't think this should have been possible in the first place, but since it indeed isn't a correctness issue, we might as well not break user space and keep allowing it (besides, the hack is rather nice).

With that said, let's cool down a little here. Regardless of whether the change was good or not, there's just no reason for the aggressivity in this thread. It's at best just hiding the (interesting and useful) side of the discussion.

I don't believe I've been aggressive in this thread. I have been frustrated, and that leaked into my messages, sure, but I think I have enough reason to be at least a little annoyed at the others in this PR:

@edolstra responds to my PR with a point that, had he actually *read* the PR, he would've known didn't apply
Your response also asks a question I think was reasonably answered in my response to Eelco ("while depending solely on what the Nix sandbox provides,"). But do you really think I wouldn't've considered any possibly simpler solution either? :p
@Ericson2314 also shows he did not read my message by assuming that I depend on builtin:fetchurl being unsandboxed, which again, was already shown not to be the case in my PR message.

Together with the recent negative experiences of friends (this isn't just me being handled this way!), I was more than reasonable responding as I did.

@puckipedia Now you got me curious. You mention that using an FOD doesn't work because of the references, but what are the references you need to have if all you're doing it putting a static binary under $out/bin?

As mentioned in my previous message, the Nix sandbox does not provide any way to copy files, nor create directories, nor create symlinks, and I (for various reasons) do not want to maintain a properly shaped NAR file myself. (otherwise i would've just patched the issue that i wrote this entire thing for out!)

Thus, seemingly my best option is to synthesize a NAR file containing a single symlink into the Nix store, and use fetchurl to unpack it. I could write something more specialized, but that is either less portable, and misses the point of this being a general "symlink tree" builder one could use from any point, with no restrictions on what paths it can depend on (as the dichotomy of being unable to use e.g. toFile with derivation outputs is useless, imo, but that's irrelevant to this PR)

edolstra · 2024-02-03T08:05:38Z

I don't think this should have been possible in the first place, but since it indeed isn't a correctness issue, we might as well not break user space and keep allowing it (besides, the hack is rather nice).

Completely disagree. This is like not fixing a buffer overflow bug because somebody might be relying on it to gain access to the system (and indeed it gives https://xkcd.com/1172/ vibes). It should be completely uncontroversial that Nix does not permit hash-less downloads from the network (at build time, not evaluation time) since that has always been the case (expect for this bug in an undocumented interface). If we're going to compromise on this, then why even have sandboxing?

Remember, this bug would have allowed Nixpkgs to download the latest version of a source tarball, which is not very reproducible. It should be clear that we cannot allow that in a supposedly reproducible build system.

puckipedia · 2024-02-03T08:09:12Z

@edolstra I will ask, and I cannot believe I have to do this, but, have you read my PR message? Please clarify why, when running the example you gave in the initial PR as the reason to remove this functionality, I get the expected result of not the builtin builder not being able to communicate to the internet, as it should behave, rather than the result you seem to be getting, which is it bypassing the Nix sandbox.

edolstra · 2024-02-03T08:19:44Z

That's because there appears to be a separate bug in builtin:fetchurl related to DNS resolution (probably preloadNSS() is broken again). But it's definitely supposed to work with sandboxing enabled.

puckipedia · 2024-02-03T08:20:56Z

DNS resolution isn't just broken here, don't worry.

nix-repl> :b builtins.derivation { name = "nix-cache-info"; system = "x86_64-linux"; builder = "builtin:fetchurl"; url = http://49.12.206.3/; outputHashMode = "flat"; }
warning: error: unable to download 'http://49.12.206.3/': Couldn't connect to server (7); retrying in 281 ms
warning: error: unable to download 'http://49.12.206.3/': Couldn't connect to server (7); retrying in 508 ms
warning: error: unable to download 'http://49.12.206.3/': Couldn't connect to server (7); retrying in 1000 ms

And if that was actually the case, why repeat the exact same point you made before without even trying to explain why the behavior I was seeing didn't match that?

edolstra · 2024-02-03T08:24:41Z

You're right. However, whether sandboxing is enabled shouldn't affect whether an outputHash is needed.

puckipedia · 2024-02-03T08:32:21Z

Okay, so we're where we should've been about one reply into the thread.

I think that breaking a feature that worked since 2015, with documented uses, for no good reason, is bad. And no real alternative exists for me to use as a replacement of fetchurl with a file:// url. As @thufschmitt said, "we might as well not break userspace". And, just like what @9999years asked earlier, what happened to the policy of existing behavior being considered stable?

puckipedia · 2024-02-03T08:34:30Z

whether sandboxing is enabled shouldn't affect whether an outputHash is needed.

To actually respond to this point: As mentioned before, if I wanted to fetch a file in an unsandboxed input-addressed derivation, I'd just call curl. There's no reason to artificially limit the builtin builder. Or do you not trust the Nix sandbox enough?

7c6f434c · 2024-02-03T08:47:15Z

would have allowed Nixpkgs

False, Nixpkgs maintains avoidance of some of the Nix features.

Nix does not permit hash-less downloads from the network

Well, file:// is not from the network (even more so if the path is within the store…), so the restriction introduced is more restrictive than the motivation justifies, and breaks userspace for all the wrong reasons.

(By the way, Nix does project this impression of «we won't tell what's stable and we will conspiciously break whatever we do not personally use» from time to time, so I can understand the frustration in messages by @puckipedia )

yorickvP · 2024-02-03T09:31:12Z

Drive by summarization:

The builtins:fetchurl fetcher was never meant to run as a regular sandboxed derivation, just as a FOD. However, you could still make it run as a regular derivation where it would be sandboxed, which isn't very useful for an URL fetcher.

Then, @puckipedia ¹ found an unexpected usecase where you could pass it file:// urls and its unpacking behavior would be useful even when sandboxed. She uses this to unpack .nar files during her custom bootstrapping (I think).

Personal Opinion:

Personally I dislike this behavior and I think we should have something less weird, maybe a builtins:unpack fetcher that just unpacks arbitrary files inside the sandbox. However, I see little² reason to disable this misfeature before we have that, since the sandbox still functions properly for it.

undocumented behaviors @puckipedia, who lives in a cave and relies on 20 undocumented behaviors per day ↩
One might argue that this leads to unexpected behavior for users who have the sandbox disabled, but nixpkgs.fetchurl already enforces the hash requirement, and people who use builtins:fetchurl know what they're doing ↩

tomberek · 2024-02-07T01:10:39Z

The unpack and non-FOD behavior of builtin:fetchurl allows one to create arbitrary directory trees via NAR manipulation. This is similar to the requested feature here: #6697. Having access to more built-in builder capability has come up a few times (I've used "builtin:buildenv" to do similar tricks before when I did not want to depend on all of Nixpkgs for utilities like mkdir). So the desired use-case is valid and valuable.

The code below demonstrates the requested feature as it is currently being used: @puckipedia is this a close minimal example to what you are doing? The examples provided above using https://cache.nixos.org/nix-cache-info, and http://49.12.206.3, are distractions. Only the "file://" behavior is being asked for?

The non-FOD behavior of fetchurl was intended to be restricted as builtin:fetchurl is normally expected to be a FOD. This normal expectation means it can have different behavior when connecting to various remote resources and there is a very real possibility of the proposed change to cause a security problem, interact with sandboxing, or to connect to unexpected URLs. This specific behavior does not seem to have been relied upon until now. Therefore it is valid to consider if supporting it will lead to ossification and continued usage.

Some possibilities that are not mutually exclusive or prioritised:

prevent any non-FOD usage of builtin:fetchurl from now on
support "file://" URLs for builtin:fetchurl from now on
support alternative means to create directory trees with a builtin
support the non-FOD "file://" usage until an alternative is ready

let
  some-derivation = derivation {
    name = "hello-world";
    system = "x86_64-linux";
    builder = "/bin/sh";
    args = ["-c" "echo hello world > $out"];
  };
  a = derivation {
  name = "something.nar";
  builder = "/bin/sh";
  system = "x86_64-linux";
  args = ["-c" ''
    str(){
      len=''${#1}
      padlen=$(( (8 - (len % 8)) % 8  ))
      int $len
      printf "%s" "$1"
      pad $padlen
    }
    int(){
      x="$(printf "%016x" "$1")"
      rev=
      while read -n 2 a; do
        rev="$(printf '%s ' "$a" "$rev")"
      done <<EOF
      $(printf "$x")
    EOF
      rev="$(printf "\x%s " $rev)"
      printf "%b" $rev
    }
    pad(){
      x="$(printf %-''${1}s "")"
      printf "''${x// /$(printf "%s" "\x0")}"
    }

    {
    str nix-archive-1
    str "("
      str "type"
      str "directory"

      str "entry"
      str "("
        str "name"
        str "some-binary"
        str "node"
        str "("
          str "type"
          str "symlink"
          str "target"
          str "${some-derivation}"
        str ")"
      str ")"

      str "entry"
      str "("
        str "name"
        str "some-binary-2"
        str "node"
        str "("
          str "type"
          str "regular"
          str "executable" ; str ""
          str "contents"
          str "some-binary-contents"
        str ")"

      str ")"
    str ")"
    } >> $out
  ''];
};
in
derivation {
  name = "unpacker";
  builder = "builtin:fetchurl";
  outputHashMode = "flat";
  system = "no-system";
  unpack = true;
  url = "file://${a}";
}

puckipedia · 2024-02-07T01:28:54Z

The unpack and non-FOD behavior of builtin:fetchurl allows one to create arbitrary directory trees via NAR manipulation. This is similar to the requested feature here: #6697. Having access to more built-in builder capability has come up a few times (I've used "builtin:buildenv" to do similar tricks before when I did not want to depend on all of Nixpkgs for utilities like mkdir). So the desired use-case is valid and valuable.

The code below demonstrates the requested feature as it is currently being used: @puckipedia is this a close minimal example to what you are doing?

Yeah; that's pretty close to what I wrote.

The examples provided above using https://cache.nixos.org/nix-cache-info, and http://49.12.206.3, are distractions.

They are not. The examples explicitly used to prove that @edolstra's point, and the reason he created + merged his own PR, was invalid, and trying to add more reason to revert this change before discussing alternative fixes.

Only the "file://" behavior is being asked for?

None of the other URI schemes that curl may support are useful when no networking is available.

The non-FOD behavior of fetchurl was intended to be restricted as builtin:fetchurl is normally expected to be a FOD.

I mean, without it being a FOD it is inherently useless, other than the file:/// URIs, afaik.

This normal expectation means it can have different behavior when connecting to various remote resources

This hasn't been an issue for any non-builtin builder that is intended to run as a fixed-output derivation? It seems to me that you are trying to explain away the reason why I see different behavior than Eelco in the most generic possible manner, without actually addressing the points I made. (In fact, noone has actually addressed the points I made against his PR in the first place.)

and there is a very real possibility of the proposed change to cause a security problem, interact with sandboxing,

First of all, "proposed change" implies that this change would be (allegedly) introducing a security problem that wasn't there before. This PR is an exact revert of a patch that has not even made it into a Nix release.
Second of all: all derivations, including builtin derivation builders, are run under the sandbox. It's not any different than running curl inside a normal input-addressed derivation.

or to connect to unexpected URLs.

Idem on the explaining away bit. If builtin:fetchurl somehow connected to an unexpected URL, even if it was a FOD, I'd be incredibly worried.

This specific behavior does not seem to have been relied upon until now.

"now"? This code has been around for quite a few years; it's just not been very public. Does Nix code need to be 100% public to not be accidentally broken by upstream Nix? What does that say about proprietary/in-company usage of Nix?

Therefore it is valid to consider if supporting it will lead to ossification and continued usage.

Why do *I* need to fight to keep this feature stable and working as it does, even after proving that @edolstra's reasoning for merging the PR was invalid, while e.g. nix-instantiate --eval output is kept perfectly identical (after which @edolstra explicitly stated it was not intended to be stable)?

Some possibilities that are not mutually exclusive or prioritised:

prevent any non-FOD usage of builtin:fetchurl from now on

support "file://" URLs for builtin:fetchurl from now on

support alternative means to create directory trees with a builtin

support the non-FOD "file://" usage until an alternative is ready

A bunch of these ("prevent non-FOD usage", "support file:// urls for builtin:fetchurl", and "support non-FOD file:// usage") read quite mutually exclusive to me.

Note, also, that FOD file:// usage here is incompatible, as it disallows creating references.

lf- · 2024-02-07T01:43:15Z

In short, the change being reverted here was made due to ostensible security impact. The suggested security impact does not exist since the code is being run inside a sandbox and aside from the executable being included with Nix, is a normal build. It's possible that it was tested with the sandbox disabled, leading to the incorrect conclusions stated in #9902, but that is sort of irrelevant, since /usr/bin/curl can do the same thing with the sandbox disabled on macOS.

The change regresses behaviour which exists and has been talked about publicly several months ago, in a lecture hall Eelco was personally in, though that is not a bar for functionality to be supported, of course. The stated reasons to make the change have been shown to be incorrect, so #9902 does not need a replacement either.

Perhaps a discussion should be had about replacements for this rather emergent feature that are less cursed, but at the moment, the fact that removing it regresses existing codebases that are doing something reasonable with no alternative seems like a very normal reason to simply revert a PR, regardless of its contents, to think it through some more later.

It seems very strange that reverting this unintended removal of functionality in the builder, which has no alternate functionality today that could be migrated to, nor any way to feature-detect, is being fought over. At the same time, tvix using the nix-instantiate CLI "creatively" in their test suite (that pins nix) in a way that eminently has alternatives like --xml that have existed over a decade, was determined to be stable and not something to be fixed in client code.

tomberek · 2024-02-07T04:51:39Z

Up till now, I have only been trying make sure I have a clear understanding of the situation.

The ability to unpack NARs was specifically added for a similar use case, but intended to be used via "<nix/fetchurl.nix>" and to refer to a known hash, even for "file://" URLs, this is what was tested. (dae5dc7)

Another related thread: #3493 suggesting that builtin:fetchurl was not for external use, but that it would be okay to document it and presumably make it user-facing in addition to builtins.fetchurl.

Generally, I would like for there to be something to solve the original problem to create a file tree without relying on Nixpkgs; specifically thinking about future RFC92 work. And while crafting a NAR via shell printf does solve the problem, I'm doubtful that it should be the primary mechanism. I am mostly interested in what that could be. @Ericson2314 : this was a big missing piece when trying to put a nice user interface on top of dyn-drvs.

Would it make sense for the new behavior of builtin:fetchurl to be:

https, et. al. are restricted to FOD.
"file://" allows for non-FOD usage, specifically to allow for NAR unpacking from the Nix Store

We need to check to ensure that the requested item is only a valid store path. This would be needed to address the problem posed by asking for netrc. builtin:fetchurl is special because this file is copied in for its use, but not for other derivations, it is not a normal build.:

nix build --expr 'builtins.derivation { name = "nix-cache-info"; system = "x86_64-linux"; builder = "builtin:fetchurl"; url = "file:///build/netrc"; outputHashMode = "flat"; }' -L

puckipedia · 2024-02-08T14:57:53Z

Up till now, I have only been trying make sure I have a clear understanding of the situation.

This doesn't really require vague overgeneralising speculation, though, such as "This normal expectation means it can have different behavior when connecting to various remote resources"? Bring me real possibilities for security issues, rather than just these vague guesses at possibilities of things going wrong.

The ability to unpack NARs was specifically added for a similar use case, but intended to be used via "<nix/fetchurl.nix>" and to refer to a known hash, even for "file://" URLs, this is what was tested. (dae5dc7)

Another related thread: #3493 suggesting that builtin:fetchurl was not for external use, but that it would be okay to document it and presumably make it user-facing in addition to builtins.fetchurl.

Generally, I would like for there to be something to solve the original problem to create a file tree without relying on Nixpkgs; specifically thinking about future RFC92 work. And while crafting a NAR via shell printf does solve the problem, I'm doubtful that it should be the primary mechanism. I am mostly interested in what that could be. @Ericson2314 : this was a big missing piece when trying to put a nice user interface on top of dyn-drvs.

I agree with this. However: This should not come at the cost of existing code that's already out there.

Would it make sense for the new behavior of builtin:fetchurl to be:
* https, et. al. are restricted to FOD.

* "file://" allows for non-FOD usage, specifically to allow for NAR unpacking from the Nix Store

This isn't necessary; FODs should run in a sandbox anyways. And if they don't, then any network access is allowed anyways.

We need to check to ensure that the requested item is only a valid store path. This would be needed to address the problem posed by asking for netrc. builtin:fetchurl is special because this file is copied in for its use, but not for other derivations, it is not a normal build.:

I'd be okay with explicitly skipping copying the netrc in the case of non-FODs. Interestingly enough, the preview you showed, showed fetching a file:// url from outside the sandbox :)

nix build --expr 'builtins.derivation { name = "nix-cache-info"; system = "x86_64-linux"; builder = "builtin:fetchurl"; url = "file:///build/netrc"; outputHashMode = "flat"; }' -L

(I'd totally missed this; I don't use netrc and don't know any others that do, as only builtin:fetchurl uses them, and it'd have to be in root's netrc file anyways, and noone puts stuff there. see e.g. #6942)

puckipedia · 2024-02-08T21:29:19Z

Related, but I just noticed #9902 in fact breaks calling builtin:fetchurl as an impure derivation, which I feel is an actual bug.

thufschmitt · 2024-03-04T14:09:41Z

@puckipedia is that still an issue for you?

nixos-discourse · 2024-09-19T04:28:48Z

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

https://discourse.nixos.org/t/nar-unpacking-vulnerability-post-mortem/52301/31

puckipedia requested review from thufschmitt and edolstra as code owners February 2, 2024 14:42

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Feb 2, 2024

puckipedia force-pushed the revert-9902 branch from 4ee60b9 to ba7a34d Compare February 2, 2024 15:08

Ericson2314 mentioned this pull request Feb 2, 2024

Manually adding/fetching content-addressed data with references #9912

Open

puckipedia mentioned this pull request Jun 15, 2024

fetchTarball accepts ISO images (and likely many unexpected archive formats) #10917

Closed

edolstra requested a review from Ericson2314 as a code owner November 12, 2024 19:47

Revert "Merge pull request #9902 from NixOS/require-fixed-output-fetchurl" #9911

Are you sure you want to change the base?

Revert "Merge pull request #9902 from NixOS/require-fixed-output-fetchurl" #9911

Conversation

puckipedia commented Feb 2, 2024

Motivation

Context

Priorities and Process

edolstra commented Feb 2, 2024

puckipedia commented Feb 2, 2024 • edited Loading

thufschmitt commented Feb 2, 2024

Ericson2314 commented Feb 2, 2024 • edited Loading

puckipedia commented Feb 2, 2024

puckipedia commented Feb 2, 2024

Ericson2314 commented Feb 2, 2024 • edited Loading

puckipedia commented Feb 2, 2024 • edited Loading

Ericson2314 commented Feb 2, 2024

puckipedia commented Feb 2, 2024 • edited Loading

Ericson2314 commented Feb 2, 2024

puckipedia commented Feb 2, 2024

puckipedia commented Feb 2, 2024

Ericson2314 commented Feb 2, 2024 • edited Loading

puckipedia commented Feb 2, 2024

Ericson2314 commented Feb 2, 2024 • edited Loading

puckipedia commented Feb 2, 2024 • edited Loading

Ericson2314 commented Feb 2, 2024

9999years commented Feb 2, 2024 • edited Loading

9999years commented Feb 2, 2024 • edited Loading

thufschmitt commented Feb 2, 2024

nixos-discourse commented Feb 3, 2024

puckipedia commented Feb 3, 2024

edolstra commented Feb 3, 2024 • edited Loading

puckipedia commented Feb 3, 2024 • edited Loading

edolstra commented Feb 3, 2024

puckipedia commented Feb 3, 2024 • edited Loading

edolstra commented Feb 3, 2024

puckipedia commented Feb 3, 2024 • edited Loading

puckipedia commented Feb 3, 2024 • edited Loading

7c6f434c commented Feb 3, 2024

yorickvP commented Feb 3, 2024

Drive by summarization:

Personal Opinion:

Footnotes

tomberek commented Feb 7, 2024

puckipedia commented Feb 7, 2024 • edited Loading

lf- commented Feb 7, 2024 • edited Loading

tomberek commented Feb 7, 2024

puckipedia commented Feb 8, 2024 • edited Loading

puckipedia commented Feb 8, 2024

thufschmitt commented Mar 4, 2024

nixos-discourse commented Sep 19, 2024

puckipedia commented Feb 2, 2024 •

edited

Loading

Ericson2314 commented Feb 2, 2024 •

edited

Loading

Ericson2314 commented Feb 2, 2024 •

edited

Loading

puckipedia commented Feb 2, 2024 •

edited

Loading

puckipedia commented Feb 2, 2024 •

edited

Loading

Ericson2314 commented Feb 2, 2024 •

edited

Loading

Ericson2314 commented Feb 2, 2024 •

edited

Loading

puckipedia commented Feb 2, 2024 •

edited

Loading

9999years commented Feb 2, 2024 •

edited

Loading

9999years commented Feb 2, 2024 •

edited

Loading

edolstra commented Feb 3, 2024 •

edited

Loading

puckipedia commented Feb 3, 2024 •

edited

Loading

puckipedia commented Feb 3, 2024 •

edited

Loading

puckipedia commented Feb 3, 2024 •

edited

Loading

puckipedia commented Feb 3, 2024 •

edited

Loading

puckipedia commented Feb 7, 2024 •

edited

Loading

lf- commented Feb 7, 2024 •

edited

Loading

puckipedia commented Feb 8, 2024 •

edited

Loading