cloudflare / workerd

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

Feature Request — Runtime APIs - Incorrect reporting of available crypto modules #3009

Open BjornTheProgrammer opened 1 month ago

BjornTheProgrammer commented 1 month ago

The following methods: generateKey, generateKeySync, generateKeyPair, and generateKeyPairSync are all methods that are not implemented. Despite that, cloudflare's docs suggest that all of these methods are supported.

Upon looking at the code base, this is what all of the functions state

export function generateKey(
  _type: SecretKeyType,
  _options: GenerateKeyOptions,
  callback: GenerateKeyCallback
) {
  // We intentionally have not implemented key generation up to this point.
  // The reason is that generation of cryptographically safe keys is a CPU
  // intensive operation that can often exceed limits on the amount of CPU
  // time a worker is allowed.
  callback(new ERR_METHOD_NOT_IMPLEMENTED('crypto.generateKeySync'));
}

Should the docs be updated to reflect this reality?

vicb commented 4 weeks ago

@jasnell Could those calls be routed to subtle crypto or are those fundamentally different?

vicb commented 4 weeks ago

I guess probably not after seeing https://github.com/cloudflare/workerd/commit/e7e8f3fb238a697696142cf1db4469b977f7599a

Let me know if I should update crypto.ts and the doc to mark those as throwing unimplemented.

jasnell commented 3 weeks ago

They are different. I have been in the process of extracting Node.js' implementation of key generation into a dependency library that we can pull into workerd to ensure that we are compatible with their approach. It is a very slow process given Node.js' requirements around code review, etc, and just the sheer size of the task. tl;dr is this is a work in progress. Eventually we will have a full implementation of the node:crypto module in workerd that does not rely on any polyfills.

jasnell commented 3 weeks ago

Changing this to a feature request.

jasnell commented 3 weeks ago

Let me know if I should update crypto.ts and the doc to mark those as throwing unimplemented.

Please do.

vicb commented 3 weeks ago

@jasnell Thanks for the feedback, I have sent PRs to workerd and docs to clarify this.

@BjornTheProgrammer I hope this helps, you should look at the Web crypto APIs to generate crypto keys for now.

Thanks!

BjornTheProgrammer commented 3 weeks ago

@vicb Thank you for the response!

I was actually concerned about this because I was attempting to implement the createSign function and Sign class, which is a part of node:crypto, but it sounds like that might be part of the node library that @jasnell is working on. Is that true?

If so I might be interested in potentially contributing.

vicb commented 3 weeks ago

@BjornTheProgrammer James is indeed working on bringing in the whole node:crypto module:

Eventually we will have a full implementation of the node:crypto module in workerd that does not rely on any polyfills.

This is a long because it requires refactoring Node.js sources:

It is a very slow process given Node.js' requirements around code review, etc, and just the sheer size of the task

James will be able to tell you more about if/how you can help with that