Skip to content

store URI: introduce multiple signatures support #12976

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

Merged
merged 2 commits into from
Apr 14, 2025

Conversation

picnoir
Copy link
Member

@picnoir picnoir commented Apr 8, 2025

Add a secretKeyFiles URI parameter in the store URIs receiving a coma-separated list of Nix signing keyfiles.

For instance:

nix copy --to "file:///tmp/store?secret-keys=/tmp/key1,/tmp/key2" \
  "$(nix build --print-out-paths nixpkgs#hello)"

The keys passed through this new store URI parameter are merged with the key specified in the secretKeyFile parameter, if any.

Motivation

We'd like to rotate the signing key for cache.nixos.org. To simplify the transition, we'd like to sign the new paths with two keys: the new one and the current one. With this, the cache can support nix configurations only trusting the new key and legacy configurations only trusting the current key.

See NixOS/rfcs#149 for more informations behind the motivation.

Context


Add 👍 to pull requests you find important.

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

@picnoir picnoir requested a review from Ericson2314 as a code owner April 8, 2025 19:02
@picnoir picnoir marked this pull request as draft April 8, 2025 19:02
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Apr 8, 2025
@picnoir
Copy link
Member Author

picnoir commented Apr 8, 2025

Erf, miss-clicked, I wanted to open this as a draft, sorry for the ping :(

So, it seems to work, but I only tested that in a ad-hoc fashion for now:

#!/usr/bin/env bash
set -euo pipefail

tmpstore=$(mktemp -d)
trap 'rm -rf $tmpstore' EXIT
./nix key generate-secret --key-name cache.alternativebit.fr > key1
./nix key generate-secret --key-name cache.alternativebit.fr > key2
hellopath=$(nix build --print-out-paths nixpkgs#hello)
./nix copy --to "file://${tmpstore}?secret-keys=$PWD/key1,$PWD/key2" "$hellopath"
tree "$tmpstore"
for file in "$tmpstore"/*.narinfo; do
    if [ $(cat "$file" | grep -E  '^Sig: cache.alternativebit.fr' | wc -l) -ne 2 ]; then
        echo "ERROR: Cannot find 2 signatures in ${file}"
        cat "${file}"
        exit 1
    else
        echo "Found 2 signatures in ${file}"
    fi
done
~/code-root/github.com/NixOS/nix/build/src/nix (pic/multisign*) » ./test-multisign.sh
/tmp/tmp.PhBc2e298N
├── log
├── m2047a1xwgblgkrnbxz0yilkaqfrbf2b.narinfo
├── nar
│   ├── 015k427rhdd5zl2fv6ld00nak9x028j94h0hzypry7bgcmsmxzjg.nar.xz
│   ├── 05wlgdfa54n8fgyjscnr0r8bafmmcmc94h4xqwbdxibi9f0sxaj5.nar.xz
│   ├── 0incw0bg5gx0glrkyif4wpsfscanbakpkap3j5bz1vgb1v5q8aa5.nar.xz
│   ├── 1jy4xxwvkv184mqm2awr7d5xihbaggrcjzxvqjiwva6i8axm11gn.nar.xz
│   └── 1yacv71g68dhknaz3q68jjhgxapi2g824wqapf5daig2i5fzyr12.nar.xz
├── nix-cache-info
├── nj19yxkqf0iqjqn4x6dbglsvqk5bgsbs.narinfo
├── realisations
├── rmy663w9p7xb202rcln4jjzmvivznmz8.narinfo
├── y2xxdhhjy2l5mgpm3d0rw2wxmpd61my4.narinfo
└── y4qpcibkj767szhjb58i2sidmz8m24hb.narinfo

4 directories, 11 files
Found 2 signatures in /tmp/tmp.PhBc2e298N/m2047a1xwgblgkrnbxz0yilkaqfrbf2b.narinfo
Found 2 signatures in /tmp/tmp.PhBc2e298N/nj19yxkqf0iqjqn4x6dbglsvqk5bgsbs.narinfo
Found 2 signatures in /tmp/tmp.PhBc2e298N/rmy663w9p7xb202rcln4jjzmvivznmz8.narinfo
Found 2 signatures in /tmp/tmp.PhBc2e298N/y2xxdhhjy2l5mgpm3d0rw2wxmpd61my4.narinfo
Found 2 signatures in /tmp/tmp.PhBc2e298N/y4qpcibkj767szhjb58i2sidmz8m24hb.narinfo

This script should be converted in a proper functional test, but I'm not familiar with those yet. I'll have a look and see how it works in the next few days.

Once we convert this into a proper test, we can undraft this PR.

Reviews welcome in the meantime.

@picnoir picnoir force-pushed the pic/multisign branch 2 times, most recently from 960818f to 876ebcc Compare April 8, 2025 19:36
@Ericson2314
Copy link
Member

#11139 my PR will allow JSON for lists, FYI.

@Mic92
Copy link
Member

Mic92 commented Apr 9, 2025

@picnoir could you extend ./tests/functional/signing.sh with this store path syntax?

@picnoir
Copy link
Member Author

picnoir commented Apr 9, 2025

#11139 my PR will allow JSON for lists, FYI.

Ok. I'm going to add a ad-hoc JSON-style list parser for this1. Unless we already have access to a proper JSON parser? I'll have a look. (Edit: there is)

That way, we don't create a direct dependency between #11139 and this PR, and we won't have to support multiple syntax for this URI param.

Footnotes

  1. Fancy way of saying I'm going to add a [ prefix and ] suffix. :P

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 9, 2025
@picnoir
Copy link
Member Author

picnoir commented Apr 9, 2025

Added the functional test as suggested by mic92.

@Ericson2314 for the JSON syntax part, would you prefer me to adopt the JSON format from the get go in this PR or would you prefer adding that in #11139 ?

Add a `secretKeyFiles` URI parameter in the store URIs receiving a
coma-separated list of Nix signing keyfiles.

For instance:

  nix copy --to "file:///tmp/store?secret-keys=/tmp/key1,/tmp/key2" \
    "$(nix build --print-out-paths nixpkgs#hello)"

The keys passed through this new store URI parameter are merged with
the key specified in the `secretKeyFile` parameter, if any.

We'd like to rotate the signing key for cache.nixos.org. To simplify
the transition, we'd like to sign the new paths with two keys: the new
one and the current one. With this, the cache can support nix
configurations only trusting the new key and legacy configurations
only trusting the current key.

See NixOS/rfcs#149 for more informations
behind the motivation.
@picnoir
Copy link
Member Author

picnoir commented Apr 11, 2025

I tried to implement this JSON configuration here: picnoir@903a2aa

Two issues:

  1. The parser is rejecting the URL before reaching the key parsing. I guess the square brackets should be URI-encoded? Not a fan.
  2. It's weird to use from the CLI. We need to escape the JSON double ticks. See details in the above commit message.

Un-drafting the PR with the comma-separated style for now.

@picnoir picnoir marked this pull request as ready for review April 11, 2025 15:21
@picnoir picnoir requested a review from edolstra as a code owner April 11, 2025 15:21
@Ericson2314
Copy link
Member

Sorry @picnoir, I was not clear. What I meant was that I want to get that one working, land it, and then your change should be an easy to make on top. I didn't mean to ask you to go implement JSON settings values for this yourself.

@picnoir
Copy link
Member Author

picnoir commented Apr 14, 2025

Performance note: we'll be computing the nar fingerprint once for each key:

std::string ValidPathInfo::fingerprint(const Store & store) const
.

We should probably memoize the fingerprint here:

signatures.insert(signer.signDetached(fingerprint()));
and sign it for each key.


Edit: I have a WIP for that, will finish it tonight.

This is a small optimization used when we're signing a narinfo for
multiple keys in one go. Using this sign variant, we only compute the
NAR fingerprint once, then sign it with all the keys.
@picnoir
Copy link
Member Author

picnoir commented Apr 14, 2025

Note for readers: the pointer indirection is necessary because of the Signer abstraction.

We're not really using this abstraction through and it could potentially be removed? It was introduced for the remote signing mechanism, but the rest did not land and seem stalled.

@Mic92
Copy link
Member

Mic92 commented Apr 14, 2025

Note for readers: the pointer indirection is necessary because of the Signers abstraction.

We're not really using this abstraction through and it could potentially be removed? It was introduced for the remote signing mechanism, but the rest did not land and seem stalled.

Since you also proposed to work on the remote signer, maybe we can remove it for a bit longer.

@Mic92 Mic92 merged commit 3f3fd2c into NixOS:master Apr 14, 2025
12 checks passed
@picnoir picnoir deleted the pic/multisign branch April 14, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants