Skip to content

Allow empty hash in derivations#3674

Merged
edolstra merged 4 commits intoNixOS:masterfrom
matthewbauer:allow-empty-hash2
Jun 12, 2020
Merged

Allow empty hash in derivations#3674
edolstra merged 4 commits intoNixOS:masterfrom
matthewbauer:allow-empty-hash2

Conversation

@matthewbauer
Copy link
Copy Markdown
Member

@matthewbauer matthewbauer commented Jun 9, 2020

follow up of #3544

This allows hash="" so that it can be used for debugging purposes. For
instance, this gives you an error message like:

warning: found empty hash, assuming you wanted 'sha256:0000000000000000000000000000000000000000000000000000'
hash mismatch in fixed-output derivation '/nix/store/asx6qw1r1xk6iak6y6jph4n58h4hdmbm-nix':
wanted: sha256:0000000000000000000000000000000000000000000000000000
got: sha256:0fpfhipl9v1mfzw2ffmxiyyzqwlkvww22bh9wcy4qrfslb4jm429

this only effects "derivations" with outputHash set.

follow up of NixOS#3544

This allows hash="" so that it can be used for debugging purposes. For
instance, this gives you an error message like:

  warning: found empty hash, assuming you wanted 'sha256:0000000000000000000000000000000000000000000000000000'
  hash mismatch in fixed-output derivation '/nix/store/asx6qw1r1xk6iak6y6jph4n58h4hdmbm-nix':
    wanted: sha256:0000000000000000000000000000000000000000000000000000
    got:    sha256:0fpfhipl9v1mfzw2ffmxiyyzqwlkvww22bh9wcy4qrfslb4jm429
@matthewbauer matthewbauer mentioned this pull request Jun 9, 2020
matthewbauer added a commit to matthewbauer/nixpkgs that referenced this pull request Jun 9, 2020
Meant as a companion to NixOS/nix#3674

This just resets outputHash if nothing is passed in.
@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented Jun 9, 2020

Yay, no more copying and modifying a hash from elsewhere when I am making a new fixed output derivation!

fetchTarball, fetchTree, and fetchGit all have *optional* hash attrs.
This means that we need to be careful with what we allow to avoid
accidentally making these defaults. When ‘hash = ""’ we assume the
empty hash is wanted.
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jun 9, 2020

Yay, no more copying and modifying a hash from elsewhere [...]

That's not a good way to obtain hashes, regardless of this PR. (because fetchurl -> curl --insecure)

EDIT: well, perhaps we should now drop the --insecure iff hash is empty 🤔

@matthewbauer
Copy link
Copy Markdown
Member Author

EDIT: well, perhaps we should now drop the --insecure iff hash is empty 🤔

Why is --insecure set in fetchurl builder at all?

@Ericson2314
Copy link
Copy Markdown
Member

Woah, I did not know that. Yikes.

@matthewbauer
Copy link
Copy Markdown
Member Author

Yay, no more copying and modifying a hash from elsewhere [...]

That's not a good way to obtain hashes, regardless of this PR. (because fetchurl -> curl --insecure)

EDIT: well, perhaps we should now drop the --insecure iff hash is empty 🤔

I think this should be addressed in NixOS/nixpkgs@0046802. Note that this is actually a Nixpkgs issue, not a Nix one.

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jun 9, 2020

I thought the main argument is that when you know the output hash, there's not that much to gain from TLS (well, some privacy, though meta-data will probably leak part of that anyway).

EDIT: yes, I knew this aspect's tangential to this Nix PR, but the issue has been long on my mind, as it's too convenient to do things in an insecure way. And this could be way out 📈

@matthewbauer
Copy link
Copy Markdown
Member Author

matthewbauer commented Jun 9, 2020

Yeah - I noticed fetchgit just sets up certs unconditionally already (https://github.com/NixOS/nixpkgs/blob/a1aecffc971202acaca5882e87510d2a15a000f7/pkgs/build-support/fetchgit/default.nix#L64).

Hash h;
if (outputHash->empty()) {
h = Hash(ht);
printError("warning: found empty hash, assuming you wanted '%s'", h.to_string());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> warn().

expectedHash = Hash(htSHA256);
printError("warning: found empty hash, assuming you wanted '%s'", expectedHash.to_string());
} else
expectedHash = Hash(hashStr, htSHA256);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of cut&paste here, maybe it's better to add a function that parses a hash and prints a warning it if's empty?

This replaces the copy&paste with a helper function in hash.hh.
@edolstra edolstra merged commit 00fa7e2 into NixOS:master Jun 12, 2020
@edolstra
Copy link
Copy Markdown
Member

Thanks!

@nixos-discourse
Copy link
Copy Markdown

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

https://discourse.nixos.org/t/why-wouldnt-the-sha256-hash-change-when-fetching-from-a-different-url-when-updating-a-package/11745/8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants