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.

Sign up for GitHub

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

Jump to bottom

stdenv.mkDerivation: Reject MD5 in outputHash #255150

Merged
merged 2 commits into from
Oct 26, 2023
Merged

stdenv.mkDerivation: Reject MD5 in outputHash #255150

merged 2 commits into from
Oct 26, 2023

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Sep 14, 2023 •
edited
Loading

Description of changes

  • Add a check for hash type, in make-derivation.nix.
  • Add a test checking MD5 hashes are rejected.

This is necessary, even though mkDerivation has no md5 parameter, as the nix interpreter accepts it in outputHashAlgo or in SRI-like hashes such as:

fetchurl {
  url  = "https://www.perdu.com";
  hash = "md5-rrdBU2a35b2PM2ZO+n/zGw==";
}

Note: md5 is not valid in SRI hashes, and I reported the underlying bug: NixOS/nix#8982.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@nbraud nbraud requested a review from a user September 14, 2023 17:16
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Sep 14, 2023
@nbraud nbraud changed the title stdenv.mkDerivation: Reject fixed-output hashes that use MD5 stdenv.mkDerivation: Reject MD5 in outputHash Sep 14, 2023
@infinisil
Copy link
Member

Nice! But considering that md5 is practically not used anywhere, we should make sure this doesn't noticeable affect performance. Currently these are the stats, with a 0.22% increase in memory usage. I think this can be improved, I'll make some suggestions.

@nbraud nbraud force-pushed the throw-md5-into-the-Sun branch 3 times, most recently from 37642e1 to b3f762b Compare October 17, 2023 17:08
@nbraud nbraud requested a review from infinisil October 17, 2023 20:20
@nbraud
Copy link
Contributor Author

nbraud commented Oct 19, 2023 •
edited
Loading

@infinisil If the proposed version gets your approval, I can go and squash the last 2 commits.

infinisil
infinisil requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Other than one concern this looks good to me :)

pkgs/test/stdenv/default.nix Outdated Show resolved Hide resolved
@nbraud
Copy link
Contributor Author

nbraud commented Oct 24, 2023

@infinisil Done, and thanks for the feedback.

@nbraud
Copy link
Contributor Author

nbraud commented Oct 25, 2023

P.S. : since this is technically a breaking change to mkDerivation (albeit one which doesn't affect anything within nixpkgs), I guess this should either be merged before freeze on the 30th, or go into the next release?

@infinisil
Copy link
Member

That date is mostly meant for breaking changes to builds, but I can merge it before then :)

Though indeed since this is technically a breaking change, I guess it wouldn't hurt to write a short note on it in the backwards incompatibilies section of the release notes. Can you do that?

@nbraud
stdenv.mkDerivation: Reject MD5 hashes
87c2210 
While there is no fetcher or builder (in nixpkgs) that takes an `md5` parameter,
for some inscrutable reason the nix interpreter accepts the following:
```nix
fetchurl {
  url = "https://www.perdu.com";
  hash = "md5-rrdBU2a35b2PM2ZO+n/zGw==";
}
```

Note that neither MD5 nor SHA1 are allowed by the syntax of SRI hashes.
@nbraud
tests/stdenv: Check derivations with an MD5 outputHash fail to eval…
1cabb1c 
…uate
@nbraud
Copy link
Contributor Author

nbraud commented Oct 25, 2023

Done!

infinisil
infinisil approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Nice! I'll merge when CI passed :)

@infinisil infinisil merged commit d8bb0bd into NixOS:master Oct 26, 2023
20 checks passed
@nbraud nbraud deleted the throw-md5-into-the-Sun branch October 26, 2023 15:25
@nbraud nbraud mentioned this pull request Dec 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: stdenv Standard environment 8.has: changelog 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nbraud @infinisil