Add support for signing store paths using ML-DSA#449
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ML-DSA-44/65/87 support alongside Ed25519 via polymorphic SecretKey/PublicKey types, OpenSSL-backed ML-DSA implementations, unique_ptr ownership for keys, CLI key-type selection and PEM conversion, and tests exercising all 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 docstrings
🧪 Generate unit tests (beta)
Comment |
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
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.
| ctx.get(), | ||
| (const unsigned char *) sig.sig.data(), | ||
| sig.sig.size(), | ||
| (const unsigned char *) data.data(), |
There was a problem hiding this comment.
Shouldn't we stream this in? Or do we just sign the narinfo?
There was a problem hiding this comment.
Yes, it's just the narinfo in memory.
|
|
||
| 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
|
|
||
| } catch (Error & e) { | ||
| // Don't show the entire key for security. | ||
| e.addTrace({}, "while decoding key '%s…'", s.substr(0, 32)); |
| }, | ||
| { | ||
| .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.
| } else | ||
| throw Error("public key is not valid"); | ||
| } catch (Error & e) { | ||
| // Don't show the entire key for security. |
There was a problem hiding this comment.
Probably does not apply to the following line
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 Nix knowledge of PKCS#11 vs. Nix knowledge of PEM but I think it's a better trade and makes it easier for nixPKCS. |
|
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. |
4 participants
Motivation
ML-DSA-* is a post-quantum cryptography signature scheme.
To use, just call
nix key generate-secretwith--key-type ml-dsa-{44,65,87}, otherwise it works the same as ed25519 (libsodium) signatures except that it produces much bigger keys/signatures.Context
Summary by CodeRabbit
New Features
nix key generate-secretgains--key-typeto select key formatnix key convert-secret-to-pemandnix key convert-public-to-pemDocumentation
nix key generate-secretdocs; added docs for PEM conversion commandsTests