Add support for signing store paths using CNSA algorithms#449
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds KeyType and polymorphic SecretKey/PublicKey implementations (Ed25519 and OpenSSL ML-DSA/ECDSA), moves key ownership to unique_ptr, adds CLI key-type selection and PEM conversion commands, updates call sites to use parse/generate factories, and expands tests and docs to exercise multiple key types. ChangesML-DSA and polymorphic key system
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
edolstra
force-pushed
the
eelcodolstra/nix-373
branch
from
ecebf02 to
a6c429b
Compare
|
|
||
| using AutoEVP_PKEY = std::unique_ptr<EVP_PKEY, Deleter<EVP_PKEY_free>>; | ||
| using AutoEVP_PKEY_CTX = std::unique_ptr<EVP_PKEY_CTX, Deleter<EVP_PKEY_CTX_free>>; | ||
| using AutoEVP_MD_CTX = std::unique_ptr<EVP_MD_CTX, Deleter<EVP_MD_CTX_free>>; |
There was a problem hiding this comment.
Seems like these would be clearer to work with if there were a class with proper constructor and destructor
There was a problem hiding this comment.
That's basically what std::unique_ptr with Deleter is. It ensures the C pointer is always destroyed with the right function when it goes out of scope.
There was a problem hiding this comment.
I mean putting the initialization logic into the class constructor too. As is you initialize an AutoEVP_PKEY with any arbitrary pointer to an EVP_PKEY.
|
|
||
| if (EVP_DigestSignInit(ctx.get(), nullptr, nullptr, nullptr, pkey.get()) <= 0) | ||
| throw Error("EVP_DigestSignInit failed"); | ||
| /* Generate a deterministic signature (i.e. only depending on the key and the data) since Ed25519 is also |
There was a problem hiding this comment.
I'm not following why this implies we should be deterministic here.
There was a problem hiding this comment.
It doesn't have to be, but it does provide the nice property that repeated signing with the same key is idempotent (and currently the tests assume that's the case, though we could obviously change that).
edolstra
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libutil/signature/signer.cc`:
- Around line 8-11: The LocalSigner constructor currently dereferences
privateKey in the initializer list when calling privateKey->toPublicKey(), which
will crash if the passed std::unique_ptr<SecretKey> is null; modify the
constructor to validate the incoming _privateKey before calling toPublicKey
(e.g., check _privateKey != nullptr and throw std::invalid_argument or handle
the null case), assign privateKey = std::move(_privateKey) and then initialize
publicKey by calling privateKey->toPublicKey() inside the constructor body after
the null check; refer to the LocalSigner constructor, the privateKey member,
publicKey member and SecretKey::toPublicKey() when making the change.
In `@src/nix/sigs.cc`:
- Around line 164-166: Update the CLI help for the "key-type" option so it lists
all supported ML-DSA variants instead of only `ml-dsa-65`; modify the
.description associated with .longName = "key-type" in src/nix/sigs.cc to
mention `ed25519` and all three ML-DSA variants (`ml-dsa-44`, `ml-dsa-65`,
`ml-dsa-87`) and ensure the wording matches other flag descriptions (keep
`.labels = {"type"}` unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36af1028-43af-4706-b61c-1527b39ea5d7
📒 Files selected for processing (15)
src/libexpr/primops/wasm.ccsrc/libstore/binary-cache-store.ccsrc/libstore/keys.ccsrc/libstore/store-api.ccsrc/libutil/include/nix/util/signature/local-keys.hhsrc/libutil/include/nix/util/signature/signer.hhsrc/libutil/signature/local-keys.ccsrc/libutil/signature/signer.ccsrc/nix/key-convert-public-to-pem.mdsrc/nix/key-convert-secret-to-pem.mdsrc/nix/key-generate-secret.mdsrc/nix/nix-store/nix-store.ccsrc/nix/sigs.ccsrc/perl/lib/Nix/Store.xstests/functional/signing.sh
| }, | ||
| { | ||
| .tag = Xp::MLDSA, | ||
| .name = "ml-dsa", |
There was a problem hiding this comment.
If we were to add RSA and ECDSA support too, would those be separate features? I may implement that next and am wondering, since some people may simply want the new quantum resistant algorithms without P-{256,384,521} or RSA, so should we have a prefix indicating that we have a family of options for signature algorithms here? Maybe sig-ml-dsa so we can also have a sig-ecdsa and sig-rsa?
FWIW I'd also like to add an OpenSSL implementation of Ed25519 for related reasons, there are cases where OpenSSL may be more suitable than libsodium but where the keys and signatures would be compatible.
There was a problem hiding this comment.
I think it can use the same experimental feature flag.
I wouldn't mind switching to OpenSSL for Ed25519 if it generates compatible signatures, since it would allow us to drop the libsodium dependency.
There was a problem hiding this comment.
Maybe a more generic feature flag for enabled OpenSSL algorithms, then?
There was a problem hiding this comment.
FYI, any algorithm OpenSSL supports can also be pkcs11-ified using pkcs11-provider if you have a URL to the key. The wisdom I'd share there is that you just can't assume you can export the key as a pkcs8 if it's backed by a hardware keystore, but it's mostly the same underlying OpenSSL objects as of OpenSSL 3.
There was a problem hiding this comment.
I've renamed the feature flag to cnsa.
cole-h
dismissed
their stale review
didn't see numinit's comments; makes sense to look at them before this merges
|
no worries. The next step I'm going to take after this merges is doing a PR to add a couple other algorithms and PKCS#11 support with https://github.com/numinit/nixpkcs, because I would very much like to use Nix with the hardware keystores I have access to. cc @mschwaig |
|
A refactor like this PR was blocking me and will be a massive help, so thank you a ton @edolstra |
|
@numinit Yeah, getting HSM support would be great. |
|
@edolstra FYI I was simply going to add The trade is going to be (bare minimum, literally just the Edit: The PEM equivalent would be something like |
|
Overall, this is mostly how I would have done it from a format standpoint (but was mostly unsure about how to implement because of unfamiliarity with how to change the classes around) so looks good generally for me. SPKI is certainly the correct format for public keys with special cases for sodium keys like you have. |
ML-DSA-65 is a post-quantum cryptography signaturew scheme/ To use, just call `nix key generate-secret` with `--key-type ml-dsa-65`, otherwise it works the same as ed25519 (libsodium) signatures except that it produces much bigger keys/signatures
edolstra
force-pushed
the
eelcodolstra/nix-373
branch
from
bf1a60c to
9eced63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libutil/experimental-features.cc`:
- Around line 308-317: Update the .description text for the Xp::CNSA
experimental feature entry (the initializer with .tag = Xp::CNSA) so it
explicitly names the supported ML-DSA variants; replace the generic "ML-DSA"
with "ML-DSA-44, ML-DSA-65, and ML-DSA-87" (or "ML-DSA-44/65/87") alongside the
existing "ECDSA P-384" wording to make the supported algorithms explicit.
In `@tests/functional/signing.sh`:
- Around line 36-40: The current check only ensures the openssl binary exists;
change the probe so it actually verifies openssl pkey can parse the test keys
(TEST_ROOT/sk1.pem and TEST_ROOT/pk1.pem) before emitting text output.
Specifically, after detecting openssl (type -p openssl), run a non-verbose parse
check using openssl pkey against both "$TEST_ROOT"/sk1.pem and
"$TEST_ROOT"/pk1.pem and only call openssl pkey -text -noout for the keys if
those parse checks succeed (i.e., gate the ml-dsa-* and ecdsa-p384 text output
on successful parse of both keys). Ensure error branches skip the text output
when parsing fails so builders without PKCS#8/ML-DSA support do not run the
failing flows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c69dcb42-69e3-424a-ac8d-599799f91f36
📒 Files selected for processing (17)
src/libexpr/primops/wasm.ccsrc/libstore/binary-cache-store.ccsrc/libstore/keys.ccsrc/libstore/store-api.ccsrc/libutil/experimental-features.ccsrc/libutil/include/nix/util/experimental-features.hhsrc/libutil/include/nix/util/signature/local-keys.hhsrc/libutil/include/nix/util/signature/signer.hhsrc/libutil/signature/local-keys.ccsrc/libutil/signature/signer.ccsrc/nix/key-convert-public-to-pem.mdsrc/nix/key-convert-secret-to-pem.mdsrc/nix/key-generate-secret.mdsrc/nix/nix-store/nix-store.ccsrc/nix/sigs.ccsrc/perl/lib/Nix/Store.xstests/functional/signing.sh
✅ Files skipped from review due to trivial changes (3)
- src/nix/key-convert-secret-to-pem.md
- src/libexpr/primops/wasm.cc
- src/nix/key-convert-public-to-pem.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/libstore/binary-cache-store.cc
- src/perl/lib/Nix/Store.xs
- src/nix/key-generate-secret.md
- src/libutil/include/nix/util/signature/signer.hh
- src/libutil/include/nix/util/signature/local-keys.hh
- src/nix/sigs.cc
- src/libstore/keys.cc
- src/libutil/signature/signer.cc
- src/libutil/signature/local-keys.cc
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libutil-tests/local-keys.cc (1)
11-11: ⚡ Quick winAvoid leaking experimental feature state between tests.
Line 11, Line 71, and Line 115 mutate a global feature set but never restore it. That can make test behavior order-dependent. Please snapshot/restore in each test (or use a fixture with setup/teardown).
Also applies to: 71-71, 115-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libutil-tests/local-keys.cc` at line 11, The test mutates the global experimental features via experimentalFeatureSettings.experimentalFeatures.get().insert(Xp::CNSA) (also at the other two sites) and never restores state; change each test to snapshot the current feature set before mutating and restore it after (or wrap the test in a fixture with setup/teardown) so the global experimentalFeatureSettings.experimentalFeatures is returned to its prior value when the test finishes. Use the existing getter experimentalFeatureSettings.experimentalFeatures.get() to capture the current set, perform the insert for the test, and restore the captured set in the test teardown (or fixture destructor) to avoid order-dependent failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/libutil-tests/local-keys.cc`:
- Line 11: The test mutates the global experimental features via
experimentalFeatureSettings.experimentalFeatures.get().insert(Xp::CNSA) (also at
the other two sites) and never restores state; change each test to snapshot the
current feature set before mutating and restore it after (or wrap the test in a
fixture with setup/teardown) so the global
experimentalFeatureSettings.experimentalFeatures is returned to its prior value
when the test finishes. Use the existing getter
experimentalFeatureSettings.experimentalFeatures.get() to capture the current
set, perform the insert for the test, and restore the captured set in the test
teardown (or fixture destructor) to avoid order-dependent failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 660b097f-53cc-445d-b54f-4b6d1df0e952
📒 Files selected for processing (2)
src/libutil-tests/local-keys.ccsrc/libutil-tests/meson.build
This uses test vectors from the NIST ACVP ML-DSA-sigGen test set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
edolstra
force-pushed
the
eelcodolstra/nix-373
branch
from
a728235 to
c8a2a35
Compare
edolstra
changed the title
edolstra
force-pushed
the
eelcodolstra/nix-373
branch
from
24cb66e to
99587ed
Compare
|
Thanks for this. I feel like CNSA is the correct framing and P-384 is an appropriate option that doesn't rule out compatibility with (e.g.) TPMs. |
4 participants
Motivation
This adds support for some CNSA 1.0 and 2.0 algorithms: ecdsa-p384, ml-dsa-44, ml-dsa-65 and ml-dsa-87. ML-DSA is a post-quantum cryptography signature scheme.
To use, just call
nix key generate-secretwith--key-type ecdsa-p384|ml-dsa-{44,65,87}, otherwise it works the same as ed25519 (libsodium) signatures except that it produces larger keys/signatures (especially ML-DSA).It also adds commands
nix keys convert-{public,secret}-to-pemwhich may be useful if you want to use the keys with theopensslCLI.Context
Summary by CodeRabbit
New Features
nix key generate-secretgains--key-typeto select algorithmnix key convert-secret-to-pemandnix key convert-public-to-pemDocumentation
nix key generate-secretdocs; added docs for PEM conversion commandsTests