| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
This is not a bug fix. Prehashing, external_mu, or ph variants of ed25519 are simply not supported and they would require a different approach than the one in the PR which already conflicts with ml-dsa, ed25519/448 or defaulting to sha256 for ecdsa/rsa.
Sorry, something went wrong.
|
The only actual pre-hashing usecase in the wild is 'ec', { namedCurve: 'secp256k1' } likely Perhaps this could be special-cased to allow null digest there, while not affecting all other curves/algos? @panva wdyt? |
Sorry, something went wrong.
|
@ChALkeR generally speaking if there was a sound extension proposal for the existing single-shot APIs that would deal with ml-dsa external_mu, ed25519ph, and prehashed ecdsa/rsa in a non-breaking manner - sure. |
Sorry, something went wrong.
|
@panva I'm thinking more of a change that would only allow prehashed secp256k1 That would way easier to implement/support, and will likely close the reason why people want this I.e. it could be added on curve-by-curve basis, not blocking on a supporting everything at once (and just keep throwing on unsupported) That will also protect against regressions when new algos are introduced |
Sorry, something went wrong.
|
@ChALkeR that approach makes sense for targeted support. The main use case I’m addressing comes from porting Ethereum protocols to node:crypto, which has three specific requirements:
Adding prehashed secp256k1 only for these cases would satisfy this use case, while avoiding regressions and keeping other algorithms unchanged. Adding "raw" as a keyword for the algorithm parameter could handle this cleanly without breaking existing behavior. It would allow signing prehashed or fixed-length data directly, covering the use case while keeping all other algorithms unchanged and consistent. |
Sorry, something went wrong.
|
@codermapuche Do you want to propose that as a PR? It also should check input and reject everything that is not 32-byte (for secp256k1), as truncation on those is unobvious to consumers / might be a source of bugs |
Sorry, something went wrong.
|
@codermapuche Also note that you won't be to plug this in as libsecp256k1 replacement directly without more steps/checks. There are more things differing this from libsecp256k1
|
Sorry, something went wrong.
|
I'm also unsure if signing would be even faster than https://www.npmjs.com/package/@noble/curves for your usecase. |
Sorry, something went wrong.
|
This would be my rough expectation. crypto.sign(algorithm, message, {
key,
messageType: crypto.constants.MESSAGE_TYPE_RAW // RAW is current behaviour & default, other options crypto.constants.MESSAGE_TYPE_DIGEST (covers RSA, ECDSA, Ed25519ph, Ed448ph), crypto.constants.MESSAGE_TYPE_EXTERNAL_MU
})crypto.verify(algorithm, message, {
key,
messageType: crypto.constants.MESSAGE_TYPE_RAW // RAW is current behaviour & default, other options crypto.constants.MESSAGE_TYPE_DIGEST (covers RSA, ECDSA, Ed25519ph, Ed448ph), crypto.constants.MESSAGE_TYPE_EXTERNAL_MU
}, signature)I'm not supportive of singling out secp256k1 ECDSA. |
Sorry, something went wrong.
|
A proposal that covers all prehashed use cases: #62345 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Use case: