implement a subset of "Modern Algorithms in the Web Cryptography API"

[panva]

Refs: #3642

From https://wicg.github.io/webcrypto-modern-algos/ this implements:

• crypto.subtle.getPublicKey()
• SubtleCrypto.supports()
• ML-KEM (768, 1024)
• ML-DSA (44, 65, 87)
• SHA3 (256, 384, 512)
• ChaCha20-Poly1305
• cSHAKE (128, 256) - no functionName or customization, just plain SHAKE.
• TurboSHAKE (128, 256)
• raw-* format aliases for old algorithms

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.

These are all available in Node.js too.

Co-authored-by: GitHub Copilot copilot@github.com

[anonrig]

/bigbonk review this pull-request. make sure everything aligns with Node.js. roast this pr as much as you can.

[ask-bonk]

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 SubtleCrypto plus new key usage strings, this should be gated behind a compat flag.
• Dead code allocation in mldsa.c++:importSpki that unnecessarily allocates a buffer that is immediately shadowed.
• raw-private format is accepted in importKeySync but never handled by any algorithm — potential developer confusion.
• DigestStream gap: SHA-3/TurboSHAKE/cSHAKE are not supported in the streaming DigestStream — only subtle.digest(). May be acceptable as a known limitation.
• outputLength for cSHAKE allows 0: The cSHAKE path accepts outputLength >= 0 (allowing 0) while TurboSHAKE requires > 0. Deliberate or inconsistency?
• Minor style nits on variable naming and an unnecessary _kjc suffix.

This review was generated by an AI assistant and may contain inaccuracies.

[ask-bonk]

Review posted successfully. Here's a summary of what I flagged:

Key findings posted as inline comments:

1. Compat flag gating (crypto.c++ L131-137): All new algorithms and SubtleCrypto methods are unconditionally available. This should be gated behind a compat flag per workerd conventions.

2. Dead code (mldsa.c++ L419-420): publicKeyBytes is allocated then immediately shadowed by rawPkBytes. Suggested removal with a code suggestion.

3. raw-private format (crypto.c++ L704): Accepted in importKeySync but never handled by any algorithm — will cause confusing late errors.

4. cSHAKE outputLength=0 (crypto.c++ L441): cSHAKE allows outputLength >= 0 while TurboSHAKE requires > 0 — potential inconsistency.

5. ChaCha20-Poly1305 format asymmetry (chacha20.c++ L51): Only supports raw-secret, not raw, unlike AES/HMAC.

6. getPublicKey fragile dispatch (crypto.c++ L843-855): Algorithm-name if/else chain for allowed usages should be a virtual method on Impl.

7. ML-KEM/ML-DSA key memory footprint (mlkem.c++ L96-100): PQC key structs stored inline in kj::Maybe are several KB each.

8. JWK pub/priv fields (crypto.h L547): New fields on JsonWebKey affect all algorithms, with no documentation comment.

9. Rust defense-in-depth (lib.rs L57): TurboSHAKE domain separation should have debug assertion on Rust side.

10. Secret key material double-copy (crypto.c++ L791): Shared secret is copied during encapsulateKey — consider zeroing the original.

github run

[panva]

@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.

[anonrig]

@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.

Opened #6405

[anonrig]

/bigbonk roast this pr

[ask-bonk]

Review Summary

• ML-KEM/ML-DSA seed not wiped on destroy: Private key seeds stored as plain kj::Array<kj::byte> instead of ZeroOnFree — secret key material left in memory after key destruction.
• getPublicKey spec ordering: Algorithm-support check should precede the key-type check per the spec.
• Missing jsgGetMemoryInfo in ML-KEM/ML-DSA: These key classes track sizeof(Self) but don't report heap-allocated seed/publicKeyBytes.
• XOF outputLength has no upper bound: User-controlled outputLength on TurboSHAKE/cSHAKE could allocate unbounded memory.
• Nit: _kjc suffix on string literal in ChaCha20 code.
• TS_OVERRIDE gap: New methods not included in the JSG_TS_OVERRIDE block.
• Nit: Dead spki2/algId2/cbb2 variable naming in ML-DSA exportSpki.

This review was generated by an AI assistant and may contain inaccuracies.

[ask-bonk]

Review posted successfully. Here's a summary of what I flagged across 8 inline comments:

1. ML-KEM/ML-DSA seed not wiped on destroy (mlkem.c++:442): Private key seeds stored as plain kj::Array<kj::byte> instead of ZeroOnFree. Every other secret key class uses ZeroOnFree to wipe key material on destruction. This applies to both ML-KEM and ML-DSA.

2. getPublicKey spec ordering (crypto.c++:838): Per spec §3.2.5, the algorithm-support check should come before the key-type check. Current code checks type first, which produces wrong error types for some edge cases.

3. Missing jsgGetMemoryInfo (mlkem.c++:209): ML-KEM and ML-DSA key classes don't track heap-allocated publicKeyBytes and seed in memory info, so V8 heap snapshots underreport memory.

4. _kjc suffix (chacha20.c++:139): Non-standard string literal suffix, inconsistent with other key classes. Suggested removal.

5. Dead copy-paste variable names (mldsa.c++:601): cbb2/spki2/algId2 — remnants from copy-paste, ML-KEM version uses clean names.

6. TS_OVERRIDE gap (crypto.h:716): New methods (encapsulateKey, getPublicKey, etc.) not included in the explicit TypeScript override block, which may produce suboptimal auto-generated types.

7. ML-DSA getContext semantics (mldsa.c++:156): Suggested adding a comment clarifying the nullptr/empty-context equivalence.

8. XOF output length unbounded (crypto.c++:415): User-controlled outputLength for TurboSHAKE/cSHAKE has no upper bound, allowing allocation of hundreds of MB.

github run

[codecov-commenter]

Codecov Report

❌ Patch coverage is 1.89643% with 1345 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.00%. Comparing base (9039529) to head (6a5af36).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
src/workerd/api/crypto/mldsa.c++	0.00%	517 Missing ⚠️
src/workerd/api/crypto/mlkem.c++	0.00%	398 Missing ⚠️
src/workerd/api/crypto/crypto.c++	8.18%	147 Missing and 10 partials ⚠️
src/workerd/api/crypto/chacha20.c++	0.00%	156 Missing ⚠️
src/rust/sha3/lib.rs	0.00%	57 Missing ⚠️
src/workerd/api/crypto/keys.c++	0.00%	22 Missing ⚠️
src/workerd/api/crypto/rsa.c++	0.00%	12 Missing ⚠️
src/workerd/api/crypto/ec.c++	0.00%	7 Missing and 2 partials ⚠️
src/workerd/api/crypto/impl.h	0.00%	9 Missing ⚠️
src/workerd/api/crypto/aes.c++	0.00%	2 Missing and 2 partials ⚠️
... and 3 more

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasnell]

LGTM but I'm going to ask someone from our crypto team to take a look as well.

[jasnell]

@bwesterb ... any chance we can get a crypto/security review on this?

[panva]

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.

[panva]

ping @jasnell

[jasnell]

@panva ... I'll follow up with the crypto team to see if they've had a chance to review.

[jasnell]

I might take a pass through to tweak a few minor things before landing... specifically, there are a few uses of memcpy where using the kj::ArrayPtr copyFrom would be a bit safer, etc. All just basic cleanup stuff, nothing critical that I can see.

[panva]

rebased to resolve conflicts

[jasnell]

Before I can land this I'll need to get an internal PR opened that adds the new rust dependency in that environment. Otherwise it's ready to go

[jasnell]

@panva ... any chance you can grant me permissions to modify this branch? there are a few smaller edits and I think we might go with a different rust sha3 impl but I need to experiment a bit with it first.

[jasnell]

Alternatively I can pull these commits into a new local branch and work from there... whichever you prefer

[panva]

Edits by maintainers are/were allowed. Go at it! I'm hands off from this point on.

[jasnell]

Just fyi... planning on getting this merged but we're going to need to replace the dependency used. On my list of things to get to in the next few weeks but I've had a few higher priority items I've needed to focus on .

[panva]

Just fyi... planning on getting this merged but we're going to need to replace the dependency used. On my list of things to get to in the next few weeks but I've had a few higher priority items I've needed to focus on .

👍 in the meantime i'm also adding JWK support for ML-KEM (also landed in node) and the SubtleCrypto.supports() method (also in node). Pushing soon-ish.

Updated WPTs from upstream would be nice too but that would have to come from your end to rebase upon, can also be dealt with after merging.

[panva]

@jasnell which dependency you want to change to? I can see if it's an easy switch

[jasnell]

aws-lc

[panva]

aws-lc

Farewell TurboSHAKE and the c from cSHAKE :)

[jasnell]

Yeah, there are some limitations there :-/

[panva]

Yeah, there are some limitations there :-/

aws-lc-sys is needed for the SHAKE XOF, i assume that's ok.