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

Ignore references when importing signatures #10101

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raboof
Copy link
Member

@raboof raboof commented Feb 27, 2024

Motivation

Allow sharing signatures without holding on to the reference information

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Feb 27, 2024
@raboof raboof force-pushed the ignore-references-when-importing-signatures branch from 4cdda68 to cef8b0b Compare February 28, 2024 08:24
@thufschmitt
Copy link
Member

Uh, I don't think we should do that, the references are part of the identity of the path (and of the signature). What's the context that makes you want that?

@raboof raboof force-pushed the ignore-references-when-importing-signatures branch from cef8b0b to 923e79d Compare February 28, 2024 08:49
This way systems sharing signatures no longer need to exchange (or
hold on to) the size or references.

They should not matter: the signature is based on the fingerprint
which contains the narSize and references. Mathematically speaking
there could be collisions, but that would essentially mean
sha256/ed25519 is broken, in which case we're in trouble anyway?
@raboof raboof force-pushed the ignore-references-when-importing-signatures branch from 923e79d to c62e785 Compare February 28, 2024 08:56
@raboof
Copy link
Member Author

raboof commented Feb 28, 2024

Uh, I don't think we should do that, the references are part of the identity of the path (and of the signature). What's the context that makes you want that?

Thanks for the early feedback - I was going to think about this more and add more context before un-"Draft"-ing the PR :)

The context is that we're prototyping some hash collection infrastructure (https://github.com/JulienMalka/nix-hash-collection). We'd want those attestations to be signed, and it seems neat to sign them in the same way the Nix store signs them. A logical next step might be allow importing such shared trusted signatures into your nix store. Currently, however, that would mean also recording the narSize and references as part of the hash collection as well, which seems wasteful.

Indeed the narSize and references are already part of the fingerprint that is being signed, along with the narHash. I guess they are used as a kind of 'salt' in this case, to prevent a possible 'rainbow table'-style attack? I'm having a hard time understanding what this protects against, as the size and references are not sensitive, so any attacker that could forge the signature could 'just' add those fields as well.

@thufschmitt
Copy link
Member

Oh, I see, thanks for the explanation.

These checks don't add anything in terms of security (they run client-side any way, it's easy to bypass them), I think they are more a matter of user friendliness than anything. Fail early if you do something stupid.
But they probably don't add much – checking the narHash is probably more than enough already for that purpose, so I wouldn't mind too much removing them (maybe only when a flag is passed? No strong opinion here).

And just to clarify:

the size and references are not sensitive, so any attacker that could forge the signature could 'just' add those fields as well.

(it doesn't matter in this context, but) references are sensitive since they affect the semantics of the path. If they weren't checked, an attacker in control of the binary cache could just send you a system closure with all the references to glibc removed, causing your machine to crash because all the software would be missing a dependency. Or it could remove a reference to an optional dependency (a plugin that's only dlopen-ed if the .so find exists) and cause your software to behave differently than what you expect.

@thufschmitt
Copy link
Member

Discussed during the Nix maintainers meeting on 2024-03-04.
We don't want to just remove the check as it's a useful one for normal users.
Three ways from there:

  • Store the needed information in your db (indeed somewhat wasteful, but I also see that pretty useful for debugging the source of descrepancies)
  • Work around the check by first getting the narSize and references from the local store (with nix path-info --json or equivalent)
  • Add a flag to disable these checks. But a bit ugly.
  • This sanity check is useful to have
  • References can be different because of different builders configuration (__discardReferences not supported, or possibly some bug/improvement)
  • Can also get these infos from nix path-info
    • This is probably called from a script any way, so acceptable

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-03-04-nix-team-meeting-minute-130/40830/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants