Implement node:crypto sign and verify APIs #3603
Conversation
|
@jasnell it would be great to open docs PR whenever new API are implemented. I don't see #3463 documented at https://developers.cloudflare.com/workers/runtime-apis/nodejs/crypto/ It's probably even more important if only some algos are tested/supported. Also what "does not work" mean in:
Does it throws or could it return incorrect results? (if the later maybe we should throw for all that is not tested?) Thanks |
jasnell
force-pushed
the
jasnell/node-crypto-sign-verify
branch
from
9c88d77 to
857943e
Compare
| // USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
||
| /* TODO: the following is adopted code, enabling linting one day */ | ||
| /* eslint-disable */ |
There was a problem hiding this comment.
| /* eslint-disable */ |
There was a problem hiding this comment.
Going to keep this until I'm sure it's all working correctly.
| return this; | ||
| }; | ||
|
|
||
| function getIntOption(name: string, options: any): number | undefined { |
There was a problem hiding this comment.
| function getIntOption(name: string, options: any): number | undefined { | |
| function getIntOption(name: string, options: Record<string, unknown>): number | undefined { |
There was a problem hiding this comment.
It's not a Record<string, unknown>. It might be a KeyObject, and CryptoKey, etc.
src/node/internal/crypto_sign.ts
Outdated
| return undefined; | ||
| } | ||
|
|
||
| function getDSASignatureEncoding(options: any) { |
There was a problem hiding this comment.
| function getDSASignatureEncoding(options: any) { | |
| function getDSASignatureEncoding(options: unknown): number { |
| return 0; | ||
| } | ||
|
|
||
| function getPrivateKey(options: any): CryptoKey { |
There was a problem hiding this comment.
| function getPrivateKey(options: any): CryptoKey { | |
| function getPrivateKey(options: unknown): CryptoKey { |
|
|
||
| Sign.prototype.sign = function ( | ||
| this: Sign, | ||
| options: any, |
There was a problem hiding this comment.
Can you replace all any with options please?
src/node/internal/crypto_sign.ts
Outdated
| Verify.prototype._write = function _write( | ||
| chunk: string | ArrayBufferView, | ||
| encoding: string | undefined | null, | ||
| callback: (err?: any) => void |
There was a problem hiding this comment.
| callback: (err?: any) => void | |
| callback: (err?: unknown) => void |
| // Derived from DefinitelyTyped https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/stream.d.ts#L1060 | ||
|
|
||
| /* TODO: the following is adopted code, enabling linting one day */ | ||
| /* eslint-disable */ |
There was a problem hiding this comment.
| /* eslint-disable */ |
There was a problem hiding this comment.
Going to keep this until I'm sure things are working correctly
Yep. Haven't gotten to that yet. It hadn't yet been deployed to production until some time last week while I was at TC-39. Opening the PR to add the docs is on my todo list for this week.
Likely throws because I don't think the key types are supported. It should not return incorrect results. It should error noisily. |
|
|
||
| import { EventEmitter } from 'node:events'; | ||
|
|
||
| interface WritableOptions { |
There was a problem hiding this comment.
We don't need to define this here. We can just use types/node
Ref: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/stream.d.ts#L991
There was a problem hiding this comment.
Well, it was needed to make this compile ;-) ... Likely need to rework the streams types definitions separately. For now let's leave this.
|
@jasnell There are conflicts. Can you resolve them? |
jasnell
force-pushed
the
jasnell/node-crypto-sign-verify
branch
2 times, most recently
from
5378117 to
6135f49
Compare
jasnell
force-pushed
the
jasnell/node-crypto-sign-verify
branch
from
6135f49 to
14d2a3e
Compare
|
@irvinebroque @mikenomitch ... just a heads up, I'm going to start labeling workerd PRs that should be highlighted in the changelog using the |
Implements the
node:cryptocreateSign(...)/createVerify(...)/crypto.sign(...)/crypto.verify(...)APIs. RSA and ed25519 are currently test to work, other key types aren't verified yet. DSA likely does not work, for instance. Will be working on expanding tests next but this ought to be able to land.Reviewers: some familiarity with the Node.js implementation is likely necessary to review this for correctness.