[czawistowski-nvidia]

Motivation

BinaryCacheStore::uploadData() hashes every NAR it uploads, but never compares the result with the ValidPathInfo it publishes:

• In addToStore() the check has been commented out since Get rid of Hash::dummy from BinaryCacheStore NixOS/nix#3935 (2020), with FIXME reinstate these, once we can correctly do hash modulo sink as needed. We need to throw here in case we uploaded a corrupted store path.
• addMultipleToStore(), which nix copy --to uses, never had a check.

So nix copy --to <cache> of a store path whose contents changed after registration, or nix nario import --store <cache> of a nario with wrong path info, succeeds and publishes a .narinfo that doesn't match its NAR. Every client then fails to substitute that path with hash mismatch importing path, later and on another machine, instead of the upload failing.

Change

uploadData() now throws hash mismatch uploading path '…' to '…' or size mismatch uploading path '…' to '…' when the NAR hash or size it computed differs from the ValidPathInfo that mkInfo returns. It throws before writing the NAR, the listing, the debuginfo links or the .narinfo. The hash and size were already being computed, so the check costs nothing.

The check is unconditional, because no legitimate caller passes info that doesn't match its NAR:

• The Hash::dummy callers that motivated the FIXME were moved to addToStoreFromDump() in the same PR, which builds its info from the computed hash.
• copyPaths()/copyStorePath() stream narFromPath() unchanged; makeContentAddressed() computes narHash/narSize from the rewritten NAR.
• LocalStore::addToStore() already rejects mismatches regardless of repair and checkSigs, so a mismatched .narinfo was never usable by any client.

Testing

New functional test binary-cache-upload-verify.sh:

• corrupts one registered path keeping its size (bad NarHash) and grows another (bad NarSize); nix copy --to file:// must fail for both;
• edits a NAR inside a nario; nix nario import --store file:// must fail;
• in each case no NAR or .narinfo may be written, and good paths must still upload.

Without the libstore change the test fails (nix copy succeeds). With it, on this branch: binary-cache-upload-verify, binary-cache, binary-cache-compression, nix-copy-ssh, nix-copy-ssh-ng, export, repair, fetchClosure, make-content-addressed, signing and nix-store-tests pass.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: dab212a4-ef8f-4697-ae46-de5b0094592b

Commits

Reviewing files that changed from the base of the PR and between 7fa39d9 and 8e4169c.

Files selected for processing: 4

• src/libstore/binary-cache-store.cc
• src/libstore/include/nix/store/binary-cache-store.hh
• tests/functional/binary-cache-upload-verify.sh
• tests/functional/meson.build

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts