Add ML-KEM and ML-DSA WebCrypto support#6872
Conversation
Assisted-by: Codex:gpt-5 Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Keep ML-KEM, ML-DSA, helper APIs, and JWK support. Defer SHA3, ChaCha20-Poly1305, and aws-lc Rust deps. Co-authored-by: Filip Skokan <panva.ip@gmail.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6872 +/- ##
==========================================
- Coverage 67.21% 66.51% -0.70%
==========================================
Files 424 426 +2
Lines 121180 122759 +1579
Branches 20175 20552 +377
==========================================
+ Hits 81447 81653 +206
- Misses 27663 28958 +1295
- Partials 12070 12148 +78 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bwesterb
left a comment
There was a problem hiding this comment.
The ML-DSA & ML-KEM bits look good except for a potential ML-DSA JWT import issue.
Other than that, there are some optional improvements:
- We're wasting memory by keeping both public key and public key bytes in memory. We only need (seed and parsed private key) or parsed public key. Other values can be derived cheaply.
- There is some marshaling/parsing going on, where direct clone or private-to-public-key functions are available.
I did not fully review the non ML-DSA/ML-KEM specific bits, but I did spot a few things:
- The
supports()function doesn't seem to be implemented correctly. Found one problem with checking length for deriveKey in case of ECDH. Do we need to implementsupports()anyway for this PR? - Looks like we're inadvertently disabling some existing test cases.
- We might want to make the compatibility flag ML-KEM/ML-DSA specific, or just enable by default.
Question for someone more versed in V8:
- Sometimes parsed public and private keys are put on the stack. These can be somewhat large—the largest is the parsed ML-DSA-87 private key at 32kB. Would that lead to issues?
| # frameworks when running tests that require the inspector to be functional, and not by end users. | ||
|
|
||
| webCryptoModernAlgorithms @181 :Bool | ||
| $compatEnableFlag("webcrypto_modern_algorithms") |
There was a problem hiding this comment.
Non-blocking: Given it's a subset, maybe that should be reflected in the name webcrypto_pq.
There was a problem hiding this comment.
i thought about it. i feel that having one flag is already plenty. probably a decision for @jasnell
|
|
||
| webCryptoModernAlgorithms @181 :Bool | ||
| $compatEnableFlag("webcrypto_modern_algorithms") | ||
| $experimental; |
There was a problem hiding this comment.
Reminder for me to get back here and discuss whether this should be enabled by default. The question depends on how obvious the API implemented is.
| JSG_REQUIRE( | ||
| keyType == KeyType::PRIVATE, DOMInvalidAccessError, "getPublicKey requires a private key."); | ||
|
|
||
| // Extract the public key by serializing/deserializing through SPKI DER. |
There was a problem hiding this comment.
When moving to a more recent BoringSSL, you can move to this
// EVP_PKEY_copy_public returns a newly-allocated `EVP_PKEY` that contains only
// [the public key]
OPENSSL_EXPORT EVP_PKEY *EVP_PKEY_copy_public(const EVP_PKEY *pkey);that avoids the silly roundtrip.
There was a problem hiding this comment.
probably a future ticket. not sure how this is being tracked by workers team. again, defering to @jasnell here
There was a problem hiding this comment.
By the way, given the scope of the change, I think it'd be fine to enable by default.
There was a problem hiding this comment.
that'd make thing even easier to be fair. let's see what James sugtgest. it might even be behind node_compat
| case SubtleOperation::DERIVE_KEY: | ||
| return false; | ||
|
|
||
| case SubtleOperation::DERIVE_BITS: { |
There was a problem hiding this comment.
Interestingly, the behavior of supports for deriveBits isn't very clear in the draft spec.
There was a problem hiding this comment.
i've added a comment as to why it now returns 528, as well as a test
There was a problem hiding this comment.
issue on the specification repository WICG/webcrypto-modern-algos#74
|
|
||
| // Public key data (present for both public and private keys) | ||
| kj::Maybe<typename P::PublicKey> publicKey; | ||
| kj::Array<kj::byte> publicKeyBytes; |
There was a problem hiding this comment.
No need to fix this now, but want to mention that we're wasting bytes.
Keeping around publicKeyBytes doesn't make a lot of sense. Generating it from a PublicKey or a PrivateKey is cheap. For equality testing we can use MLDSAxx_public_keys_equal.
Similarly, it's cheap to compute a PublicKey from a PrivateKey.
Ideally we keep around seed and PrivateKey for a private key and PublicKey for a public key. In the future BoringSSL might keep the seed around in the private key, but today they don't.
There was a problem hiding this comment.
no actioned in the latest commit. feels like a nit indeed. i wonder what other engines do
|
|
||
| // Private key data (present only for private keys) | ||
| kj::Maybe<kj::Array<kj::byte>> seed; | ||
| kj::Maybe<typename P::PrivateKey> privateKey; |
There was a problem hiding this comment.
Same as for ML-DSA. For private keys we only need seed and PrivateKey. For public keys we only need PublicKey. The others are cheap to derive, and there is a mlkem_public_keys_equal for public key comparisson.
| auto pkBytes = kj::heapArray<kj::byte>(CBB_data(cbb.get()), CBB_len(cbb.get())); | ||
|
|
||
| // If pub field is present, verify it matches | ||
| KJ_IF_SOME(pubField, jwk.pub) { |
There was a problem hiding this comment.
Just want to flag that the spec requires the pub key to be present and matching. That's a mistake of the spec: having a public key in there as well only causes trouble. It doesn't help with performance, as the public key is essentially recomputed anyway.
There was a problem hiding this comment.
do you feel that's something the spec should not mandate? happy to open an issue and follow up
i could imagine some horror stories loading a private key and not actually signing the right thing, and that's one guardrail. but that alsoe means more storage
This adds the first opt-in slice of modern WebCrypto:
getPublicKey()andSubtleCrypto.supports()It also adds
webcrypto_modern_algorithmsflag and gates the new API behind it.Deferred from the original PR (#6403) SHA3/cSHAKE/TurboSHAKE and ChaCha20-Poly1305
Given the code is simply taken out from @panva 's PR, they are still credited as co-author
This first PR should facilitate review and support of other (relevant) algorithms defined in https://wicg.github.io/webcrypto-modern-algos/
I have read the CLA Document and I hereby sign the CLA