Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
BjornTheProgrammer opened this issue Oct 26, 2024 · 9 comments
Labels
feature request Request for Workers team to add a feature nodejs compat

Comments

@BjornTheProgrammer
Copy link

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
Copy link
Contributor

vicb commented Oct 31, 2024

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

@vicb
Copy link
Contributor

vicb commented Oct 31, 2024

I guess probably not after seeing e7e8f3f

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

@jasnell
Copy link
Member

jasnell commented Nov 3, 2024

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 jasnell added feature request Request for Workers team to add a feature nodejs compat labels Nov 3, 2024
@jasnell jasnell changed the title 🐛 Bug Report — Runtime APIs - Incorrect reporting of available crypto modules Feature Request — Runtime APIs - Incorrect reporting of available crypto modules Nov 3, 2024
@jasnell
Copy link
Member

jasnell commented Nov 3, 2024

Changing this to a feature request.

@jasnell
Copy link
Member

jasnell commented Nov 3, 2024

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

Please do.

@vicb
Copy link
Contributor

vicb commented Nov 4, 2024

@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
Copy link
Author

BjornTheProgrammer commented Nov 4, 2024

@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
Copy link
Contributor

vicb commented Nov 4, 2024

@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

@LennoxSears
Copy link

hope this can be done soon. Need to implement api encryption on worker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature nodejs compat
Projects
None yet
Development

No branches or pull requests

4 participants