implement a subset of "Modern Algorithms in the Web Cryptography API"#6403
implement a subset of "Modern Algorithms in the Web Cryptography API"#6403panva wants to merge 1 commit intocloudflare:mainfrom
Conversation
|
/bigbonk review this pull-request. make sure everything aligns with Node.js. roast this pr as much as you can. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Substantial PR implementing a subset of the "Modern Algorithms in the Web Cryptography API" spec. The implementation is well-structured with good use of C++ templates for the ML-KEM/ML-DSA parameter dispatch. Key findings:
- Compat flag gating: These new algorithms and APIs (ML-KEM, ML-DSA, ChaCha20-Poly1305, SHA-3, encapsulate/decapsulate/getPublicKey) are unconditionally available with no compat flag or autogate. Given the project's strong backward compatibility commitment and the fact that these APIs add new methods to
SubtleCryptoplus new key usage strings, this should be gated behind a compat flag. - Dead code allocation in
mldsa.c++:importSpkithat unnecessarily allocates a buffer that is immediately shadowed. raw-privateformat is accepted inimportKeySyncbut never handled by any algorithm — potential developer confusion.DigestStreamgap: SHA-3/TurboSHAKE/cSHAKE are not supported in the streamingDigestStream— onlysubtle.digest(). May be acceptable as a known limitation.outputLengthfor cSHAKE allows 0: The cSHAKE path acceptsoutputLength >= 0(allowing 0) while TurboSHAKE requires> 0. Deliberate or inconsistency?- Minor style nits on variable naming and an unnecessary
_kjcsuffix.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted successfully. Here's a summary of what I flagged: Key findings posted as inline comments:
|
|
@anonrig will update the WPTs separately and then i'll rebase this on top to just get the diff in WPTs related to these changes. |
This comment has been minimized.
This comment has been minimized.
|
/bigbonk roast this pr |
There was a problem hiding this comment.
Review Summary
- ML-KEM/ML-DSA seed not wiped on destroy: Private key seeds stored as plain
kj::Array<kj::byte>instead ofZeroOnFree— secret key material left in memory after key destruction. getPublicKeyspec ordering: Algorithm-support check should precede the key-type check per the spec.- Missing
jsgGetMemoryInfoin ML-KEM/ML-DSA: These key classes tracksizeof(Self)but don't report heap-allocated seed/publicKeyBytes. - XOF
outputLengthhas no upper bound: User-controlledoutputLengthon TurboSHAKE/cSHAKE could allocate unbounded memory. - Nit:
_kjcsuffix on string literal in ChaCha20 code. - TS_OVERRIDE gap: New methods not included in the
JSG_TS_OVERRIDEblock. - Nit: Dead
spki2/algId2/cbb2variable naming in ML-DSAexportSpki.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted successfully. Here's a summary of what I flagged across 8 inline comments:
|
Co-authored-by: GitHub Copilot <copilot@github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6403 +/- ##
==========================================
- Coverage 70.71% 70.00% -0.72%
==========================================
Files 427 431 +4
Lines 117262 119074 +1812
Branches 18903 19193 +290
==========================================
+ Hits 82918 83353 +435
- Misses 23093 24455 +1362
- Partials 11251 11266 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| static constexpr CryptoKeyUsageSet decapsulateBits() { | ||
| return 1 << 11; | ||
| } |
There was a problem hiding this comment.
Nit... to address later... this pattern of how these are defined is a bit brittle.
| @@ -0,0 +1,796 @@ | |||
| #include "impl.h" | |||
There was a problem hiding this comment.
nit: All new files need to include the copyright header. We're not always consistent at remembering this ourselves but we're trying to get better at it ;-)
| namespace { | ||
|
|
||
| // Traits structs for each ML-DSA parameter set. | ||
| struct MlDsa44Params { |
There was a problem hiding this comment.
Nit: for later, it probably makes sense to get these into ncrypto.
jasnell
left a comment
jasnell
left a comment
There was a problem hiding this comment.
LGTM but I'm going to ask someone from our crypto team to take a look as well.
|
@bwesterb ... any chance we can get a crypto/security review on this? |
|
Happy to. This is a lot at once: better to split into separate PRs for each addition. Let's start with ML-DSA & ML-KEM. |
In that case I'll hand it over to you folk. You're better suited to push this across the line for workerd than I am. I don't think this is a lot to take in given it's mostly wiring. And I would hate if by the virtue of splitting this things get missed. |
5 participants
Refs: #3642
From https://wicg.github.io/webcrypto-modern-algos/ this implements:
Tested using existing WPTs as well as using the https://github.com/panva/hpke and https://github.com/panva/jose test suites.
For SHA3, TurboSHAKE, and cSHAKE I added the sha3 crate, everything else goes through BoringSSL.
With the exception of TurboSHAKE (which has a pending PR open) these are available in Node.js too.
Co-authored-by: GitHub Copilot copilot@github.com