fetchgit: inherit allowedRequisites in mkDerivation

[ghost]

For an example use case, see #171223

Motivation

When maintainers override stages of fetchgit (e.g. postPatch) it is very easy for them to accidentally leak the outpath-hash of their current stdenv into fetchgit's output, and therefore into the value they paste into sha256.

This is a problem, because the resulting expression will break whenever any change is made to stdenv or when anybody attempts to build the expression on a different platform than the one used by the original maintainer.

It is sometimes difficult to communicate to package maintainers why their expression is problematic. Having allowedRequisites passed through makes it easier to do this: "look, when I set allowedRequisites=[] your package breaks; are you sure you meant to hardcode the hash of today's x86_64-linux.stdenv into your expression?" Without this PR I have to change fetchgit in order to show them the problem, which makes it more likely that they will blow me off with "yeah, well, you screwed around with fetchgit's internals; of course that broke things!"

Description of changes

Let's offer maintainers the option to check that they aren't making this mistake, by passing through allowedRequisites.

The default value is null: no checking, all/any requisites are allowed.

Broader issues

Almost as much of a problem is the fact that CI does not catch these problems. The fetchgit is run only once, then its output goes into cachix, and all future builds (hydra, CI, ofborg) pull from cachix.

This is part of a larger problem with nixpkgs infra: there are parts of cachix which cannot be reproduced easily if they are lost. Once something goes into cachix, we never ever again reverify the procedure by which it was placed into cachix. As a nixpkgser who regularly builds with --option trusted-public-keys "" --option substituters "" and stdenv overlays I seem to be one of the few who is aware of the scale of the problem. Or maybe I'm just a crank. 😉 Probably a bit of both.

Follow-up PRs

It might be worth a PR in staging which changes the default to [] (i.e. no requisites are allowed unless explicitly overridden) plus a backward-compatibility treewide to locally reverts i to allowedRequisites=null in any packages which currently need it. This would ensure that more cases of this problem don't wind up in nixpkgs unless maintainers deliberately override the check (which will attract the attention of reviewers and hopefully result in a comment justifying the exception).

I volunteer run the nix-build --option substituters "" release.nix from an empty /nix/store in order to find all packages needing an exception in the treewide.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Fits CONTRIBUTING.md.

[ghost]

Ping...

[roberth]

Going for staging doesn't really help because of the output caching. Instead, you can test this with a script like

set -ex
srcs=( $(
  grep -l NIX_GIT_SSL_CAINFO $(
    nix-store --store /tmp/store -qR $(
      nix-instantiate --store /tmp/store nixos/release.nix -A iso_gnome) \
    | grep '\.drv' \
    | sed -e 's_/nix/store_/tmp/store/nix/store_') \
  | sed -e 's_/tmp/store__'
) )
nix-build --store /tmp/store -A bash -A git -A gitMinimal -A git-lfs -A stdenv -A stdenvNoCC -A cacert -A nukeReferences
nix-build --store /tmp/store ${srcs[0]}
nix-build --store /tmp/store --option substitute false ${srcs[@]} 

All sources for iso_gnome, iso_plasma5 and amazonImage are ok, so I think we can just go for it and set the strict default.

[ghost]

Instead, you can test this with a script like

  grep -l NIX_GIT_SSL_CAINFO $(

That is a really clever trick for finding fetcher-based FODs! But I think it will only notice fetchgit-based fetchers. I think you need SSL_CERT_FILE as well to get fetchurl. And there are still some corner cases for the exotic fetchers (fetchipfs, etc).

I think we can just go for it

Thanks.

and set the strict default.

I would prefer to do that in a separate PR after this one. That way if it causes breakage for a bunch of people the torches-and-pitchforks mob will only need to revert the change-of-default, rather than also reverting the inherit allowedRequisites added by this PR. The main case I'm worried about here is PRs that have passed CI but not merged yet. This PR merges, then those other PRs merge after it and break eval. I think OfBorg only re-verifies eval when you push new commits to a PR.

Going for staging doesn't really help because of the output caching.

You're right. If this PR merges I will submit a second PR, also to master, to change the default from null to []. I'll also include a "don't hesitate to revert this" in the commit message for that PR.

[roberth]

separate PR

👍

I think you need SSL_CERT_FILE as well to get fetchurl.

Yeah, something like that, but fetchurl is not affected by this particular PR. The other fetchers should be done at some point, but let's start with only fetchgit.