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

[RFC 0131] Enforce the usage of SRI hashes in Nixpkgs #131

Closed
wants to merge 3 commits into from
Closed

[RFC 0131] Enforce the usage of SRI hashes in Nixpkgs #131

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d032262
RFC 0131: Enforce the usage of SRI hashes in Nixpkgs
winterqt Jul 26, 2022
c7e411f
fetchgit supports SRI hashes
winterqt Jul 26, 2022
5dc3fb0
Note unresolved question of the hash argument name
winterqt Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions rfcs/0131-sri-hashes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
---
feature: sri-hashes
start-date: 2022-07-25
author: Winter
co-authors: (find a buddy later to help out with the RFC)
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

Use SRI hashes and the `hash` parameter for fetchers.
Copy link
Member

Choose a reason for hiding this comment

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

"SRI hash" needs a reference link with a definition and globally this RFC needs some more context to be explained.

FWIW, since I became aware that this kind of hash was preferred, I have tried to use them more systematically, but I'm annoyed by nix-prefetch-url not generating this type of hash (at least all the way to Nix 2.12) and not knowing how to convert a hash generated by nix-prefetch-url into an SRI hash.

Copy link
Member

@vcunat vcunat Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FYI, for conversion I'm using

nix hash to-sri --type sha256 theHash

EDIT: or you could use

nix hash file path/to/file

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! But another thing that is becoming annoying to me lately is to be forced to activate the nix-command experimental feature for more and more basic things that I cannot avoid.

Another solution that I've found meanwhile was openssl dgst -sha256 -binary /path/to/file | openssl base64 -A, but it's not as convenient.

Copy link

@ShamrockLee ShamrockLee Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But another thing that is becoming annoying to me lately is to be forced to activate the nix-command experimental feature for more and more basic things that I cannot avoid.

I would rather view it as a missing feature of nix-hash that needs to be implemented to get this RFC merged. In other words, we should have

nix-hash --sri --type sha256 path/to/file

and

nix-hash --to-sri --type sha256 theHash

Update: I just created NixOS/nix#7690. Please take a look.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! And thanks to you, I also learned that, in the meantime, I can use nix --extra-experimental-features nix-command file to-sri --type sha256 .... I didn't realize that I don't need to activate the experimental feature globally, and this is already better than nothing.


# Motivation
[motivation]: #motivation

Currently in Nixpkgs, the hash formats that are used in parameters to fetchers range from SRI and base-32 (the most common ones) to hex. In addition to this, packages use the algorithm-specific arguments (`sha256`, `sha512`, etc.) or the generic `hash` argument at random. This creates inconsistencies that we'd rather not have.
Copy link

Choose a reason for hiding this comment

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

What is SRI in this context?

Copy link
Member Author

@winterqt winterqt Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Subresource Integrity, whose hashes look like sha256-2mE26ES+fYSWdfMr8uTsX2VVGTNMDQ9MXEk5E/L95UI=. Maybe I should add at least a definition of SRI in the RFC?

Copy link
Member

Choose a reason for hiding this comment

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

This creates inconsistencies that we'd rather not have.

Why? This is not a motivation, just a claim.

Using the different hash encodings genuinely has value, for example I frequently just copy over the base16 hash from a SHA256SUMS file provided by upstream, requiring less button presses, round trips and most importantly allows others to quickly and easily match the hashes textually when reviewing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a link to what SRI is in the RFC would be good to have. I also didn't know what SRI was.

Copy link
Member Author

@winterqt winterqt Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Using the different hash encodings genuinely has value...

I agree with this point, but at the same time most people use either the SRI hashes given by Nix's hash mismatch errors (which I now realize is only in 2.4), or convert those to base-32 (the default in 2.3 and below).

Copy link
Member

@sternenseemann sternenseemann Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@vojta001 That's why these interfaces tend to have an assert or two behind the scenes…

nix hash to-sri

Well that's actually a very good point: There is no stable interface for converting hashes into the SRI format, because nix-hash(1) only supports SRI as an input, not an output format.

Copy link

@milahu milahu Sep 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Using the different hash encodings genuinely has value, for example I frequently just copy over the base16 hash from a SHA256SUMS file provided by upstream, requiring less button presses, round trips and most importantly allows others to quickly and easily match the hashes textually when reviewing.

no need to convert the hash to base64

SRI hashes only require a prefix:

{
  sha256 = "1cc9110705165dc09dd09974dd7c0b6709c9351d6b6b1cef5a711055f891dd0f";
  sha256 = "sha256-1cc9110705165dc09dd09974dd7c0b6709c9351d6b6b1cef5a711055f891dd0f";
  hash = "sha256-1cc9110705165dc09dd09974dd7c0b6709c9351d6b6b1cef5a711055f891dd0f";
}

base16 hashes are found with
grep -r -E 'sha256 = "[0-9a-f]{64}";'

new and better hash technologies appear everywhere.

sha256 is de-facto standard in nixpkgs,
because its good enough for our use case = global IDs

Copy link
Member

@AndersonTorres AndersonTorres Sep 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

sha256 is de-facto standard in nixpkgs,

MD5 was de facto standard no more than a decade ago.

Also, this was not about "good enough algorithms", but about a better format for the expression - namely, hash = "hashAlgorithm-..." instead of hashAlgorithm = "...".

Copy link
Contributor

Choose a reason for hiding this comment

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

This creates inconsistencies that we'd rather not have.

@winterqt could you add something more specific here, considering this is the whole motivation for the RFC?

The discussion above has brought up some more points:

  • semantic vs syntactical mutual exclusion
  • unique attribute name rather than multiple, algo-specific ones
  • ...

Copy link
Member

Choose a reason for hiding this comment

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

@milahu: conversion would be needed because SRI is currently only accepting base64. However, it would be relatively easy to change: NixOS/nixpkgs#199660


# Detailed design
[design]: #detailed-design

1. Require all new packages to use SRI hashes + `hash` when possible
2. Do a treewide migration for the majority of cases (single fetcher in `src`, using a fetcher that supports `hash` and SRI hashes (`fetchFromGitHub`, `fetchurl`, etc.)) to `hash` + SRI hashes

# Drawbacks
[drawbacks]: #drawbacks

- Some fetchers still don't support SRI hashes (i.e. [`fetchgit`](https://github.com/NixOS/nixpkgs/pull/79987), which is blocked due to [this discussion](https://github.com/NixOS/nixpkgs/pull/79987#discussion_r378735698) not being resolved)
winterqt marked this conversation as resolved.
Show resolved Hide resolved

# Alternatives
[alternatives]: #alternatives

- Do nothing: further inconsistencies will continue to be introduced
Copy link
Member

Choose a reason for hiding this comment

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

I think there are a number of alternatives.

Different rules can be considered. For each of the following formats it could be decided to allow/disallow/preferred:

  • hash = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
  • sha256 = "0000000000000000000000000000000000000000000000000000000000000000";
  • sha256 = "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";
  • sha256 = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";

I've gathered there are a number of ways these rules can be 'enforced':

  • Throw evaluation errors upon not conforming to rule.
  • Have a it be a potential future linter rule.
  • Have PR reviewers nudge contributors to follow the rule
  • Do nothing: keep things like the are right now. Reviewers will suggest their own opinion in PRs if they'd like.
  • Allow any format: do not allow PR reviewers to suggest changing the format in PRs to their preferred format. If the format is allowed, just let it be. This avoids PRs being always potentially invalid due to reviewers having different opinions.

Copy link
Member

Choose a reason for hiding this comment

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

Allow anything: Do not allow PR reviewers to suggest changing the format in PRs. If the format is allowed, just let it be. Avoid conflicting reviewer opinions.

I'm mentioning this option, as at the moment PRs are being reviewed where the reviewers suggest their own 'opinion' on the matter; even though there is no real rule. One outcome of this RFC could be to just let it be.


# Unresolved questions
[unresolved]: #unresolved-questions

- How can we do the treewide migration efficently?
- Text replacement for the majority of cases, with an AST-based solution for others?
- Do we want to eventually deprecate the use of `sha256`, `sha512`, etc. in fetchers that support SRI hashes, leading to their eventual removal?
Copy link
Member

Choose a reason for hiding this comment

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

We can deprecate them without removing them.

- This would require updating every script that uses these arguments, and would cause breakage for any usage of them out-of-tree; we'd need to weigh the benefits of this

# Future work
[future]: #future-work

Currently none.