search

cloudflare / workerd

The JavaScript / Wasm runtime that powers Cloudflare Workers
https://blog.cloudflare.com/workerd-open-source-workers-runtime/
Apache License 2.0
6.11k stars 290 forks source link

Consolidate and impl node:crypto key handling internals (part 1) #2345

Closed jasnell closed 2 months ago

jasnell commented 3 months ago

This is incomplete but for the sake of trying to make for smaller PRs I'll finish this up in a separate follow on PR

When reviewing this it is important to understand one of the key differences between Node.js' KeyObject and the Web Crypto CryptoKey... when importing a CryptoKey, the algorithm and some bits of metadata (such as digest name that is optional in some operations and required in others) must be specified up front. The imported key data is validated using the specific metadata. In Node.js API, the key type is detected as part of the import and some properties (like the digest) are not specified at all. This makes for a challenge since it means we can't really reuse the existing web crypto key import implementation and have to work things a slightly different way.

In Node.js, the KeyObject existed first, so the CryptoKey in that runtime is just a wrapper around that existing KeyObject, which works well given that KeyObject is more generic than CryptoKey. In our implementation, CryptoKey existed first and the first take at implementing KeyObject wraps CryptoKey::Impl under the covers. This makes things a bit more complicated and convoluted for my tastes. I'll likely rework this a bit more to be closer to the model that Node.js uses as it will make the code a lot cleaner and easier to understand/maintain, but that's going to be a big job that will require a number of iterations.

jasnell commented 2 months ago

Putting this back in draft as there's likely to be a more involved change here.

jasnell commented 2 months ago

Going to close this and take another pass at consolidating code in a more incremental way...