Add secp256k1 ECDSA support via aws-lc-rs#6689
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
| // Overload that adds secp256k1 to the table when the compat flag is set. | ||
| EllipticCurveInfo lookupEllipticCurve(jsg::Lock& js, kj::StringPtr curveName) { | ||
| if (FeatureFlags::get(js).getSecp256k1EcdsaCurve() && | ||
| strcasecmp(curveName.cStr(), "secp256k1") == 0) { |
There was a problem hiding this comment.
(note to self) we need to add equalsIgnoreCase to StringPtr.
kentonv
left a comment
There was a problem hiding this comment.
Hold on here.
This is adding a non-standard crypto algorithm to our WebCrypto implementation, requiring a new dependency on a third-party library.
Sorry but this isn't something we can do on a whim. Once landed we'll have to support this forever. This has long-term maintenance and binary bloat costs for us. We need some sort of argument for why this is worthwihle.
|
@mikea informed me that @ibremseth is a Cloudflare employee, which I would never have guessed from the profile and PR description. |
Discussed with @mikea, seems like there was context elsewhere that was just missing from the PR description.
0733892 to
e36815d
Compare
|
Will need to weigh this carefully relative to #6403 as well. We need to carefully consider the additional dependency on the new rust libraries relative to what boringssl provides and settle the question of which (if any) additional dependencies we want to accept. I understand there's other work depending on this but let's be sure to settle the dependency questions before merging this. |
| EllipticCurveInfo lookupEllipticCurve(jsg::Lock& js, kj::StringPtr curveName) { | ||
| if (FeatureFlags::get(js).getSecp256k1EcdsaCurve() && isSecp256k1(curveName)) { | ||
| return {"secp256k1", 0, 32}; | ||
| } |
There was a problem hiding this comment.
If we add this, it likely should be with an autogate and not a compat flag. Once we're sure it's ready, it should just be there, rather than made conditional on an autogate. That said, we might want to consider using a namespaced name for it just in case the WebCrypto standard adds the algorithm later with the same name but potentially conflicting details.
aws-lc-rs is the chosen second library to use in addition to the existing boringssl |
e36815d to
b3e8124
Compare
7630bce to
60c901d
Compare
|
I'm not aware of any extension to Web Cryptography that defines these (or the mentioned followups). I feel compelled to suggest that it should use vendor prefix for the
As far as this and all followups go, i'd suggest finding a home for this in an existing SDO such that a) you don't need a vendor prefix, b) get feedback from other implementers, and c) stand a chance of other (server) runtimes picking it up as well. |
|
After some discussion we've decided to add secp256k1 as a WASM module directly in a worker! Closing this, appreciate everyones time and thoughts |
Summary
Adds support for
namedCurve: "secp256k1"to the WebCryptoECDSAalgorithm, gated behind the newsecp256k1_ecdsa_curveflag. secp256k1 is the curve used by Bitcoin, Ethereum, and related ecosystems.BoringSSL doesn't include secp256k1, so this PR vendors aws-lc-rs and routes secp256k1 operations through it. aws-lc-rs wraps AWS-LC, a BoringSSL fork that includes secp256k1.
Scope
rawandjwkr || scompact, random noncesFollow-ups