-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(node/crypto): Elliptic Curve Diffie-Hellman (ECDH) support #18832
Conversation
"p256", | ||
"p384", | ||
"p256 0.11.1", | ||
"p384 0.11.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to avoid these duplicate dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not duplicates - each is a pure rust implementation of an elliptic curve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind I see what you mean
export const ellipticCurves: Array<EllipticCurve> = [ | ||
{ | ||
name: "secp256k1", | ||
ephemeral: false, | ||
privateKeySize: 32, | ||
publicKeySize: 65, | ||
sharedSecretSize: 32, | ||
}, // Weierstrass-class EC used by Bitcoin | ||
{ | ||
name: "prime256v1", | ||
ephemeral: true, | ||
privateKeySize: 32, | ||
publicKeySize: 65, | ||
sharedSecretSize: 32, | ||
}, // NIST P-256 EC | ||
{ | ||
name: "secp256r1", | ||
ephemeral: true, | ||
privateKeySize: 32, | ||
publicKeySize: 65, | ||
sharedSecretSize: 32, | ||
}, // NIST P-256 EC (same as above) | ||
{ | ||
name: "secp384r1", | ||
ephemeral: true, | ||
privateKeySize: 48, | ||
publicKeySize: 97, | ||
sharedSecretSize: 48, | ||
}, // NIST P-384 EC | ||
{ | ||
name: "secp224r1", | ||
ephemeral: true, | ||
privateKeySize: 28, | ||
publicKeySize: 57, | ||
sharedSecretSize: 28, | ||
}, // NIST P-224 EC | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps as const satisfies Array<EllipticCurve>
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the type becomes clearer. Please see this TS Playground.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use Resources to represent curve structures because there is no API to clean up memory.
Please use the exisiting infra for storing key material in JS and passing that to ops.
See https://www.notion.so/denolandinc/node-crypto-design-99fc33f568d24e47a5e4b36002c5325d
Example PR: 9496dfc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you avoid the duplicate deps? Btw we already use p[256,384,512] in ext/crypto so extract them out to the workspace Cargo.toml so its the same version.
export function getCurves(): readonly string[] { | ||
notImplemented("crypto.getCurves"); | ||
return ellipticCurves.map((x) => x.name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const curveNames = ellipticCurves.map((x) => x.name);
export function getCurves(): readonly string[] {
return curveNames;
}
i ran out of tea
third_party
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops
Looks good! Please add / enable Node compat tests for this |
Nice work Lev! |
WIP.
Curves:
TODO:
encoding
parameter in ECDH classformatting
parameter in ECDH classMy gist for testing some of this:
https://gist.github.com/levex/6a22f20eea40e0eb337a02ba906f79d8
(will be updated as the PR progresses)