Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow empty hash in derivations #3674

Merged
merged 4 commits into from
Jun 12, 2020
Merged

Conversation

matthewbauer
Copy link
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
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
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
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
Member

Woah, I did not know that. Yikes.

@matthewbauer
Copy link
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
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
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
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
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
Member

Thanks!

@nixos-discourse
Copy link

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