[v6] Replace elliptic.js with noble-curves, and load noble-hashes dynamically#1694
Merged
larabr merged 9 commits intoopenpgpjs:v6from Oct 23, 2023
Merged
[v6] Replace elliptic.js with noble-curves, and load noble-hashes dynamically#1694larabr merged 9 commits intoopenpgpjs:v6from
larabr merged 9 commits intoopenpgpjs:v6from
Conversation
larabr
force-pushed
the
v6-drop-elliptic
branch
from
87050b3 to
111bf3d
Compare
twiss
reviewed
Oct 19, 2023
Comment on lines
+114
to
+118
| const { publicKey: V, privateKey: v } = await curve.genKeyPair(); | ||
|
|
||
| // The output includes parity byte | ||
| const rawSharedSecret = nobleCurve.getSharedSecret(v, Q); | ||
| return { publicKey: V, sharedKey: rawSharedSecret.subarray(1) }; |
Member
There was a problem hiding this comment.
This is very nitpicky but it would be clearer as:
Suggested change
| const { publicKey: V, privateKey: v } = await curve.genKeyPair(); | |
| // The output includes parity byte | |
| const rawSharedSecret = nobleCurve.getSharedSecret(v, Q); | |
| return { publicKey: V, sharedKey: rawSharedSecret.subarray(1) }; | |
| const { publicKey, privateKey } = await curve.genKeyPair(); | |
| const rawSharedSecret = nobleCurve.getSharedSecret(privateKey, Q); | |
| const sharedKey = rawSharedSecret.subarray(1); // Remove parity byte | |
| return { publicKey, sharedKey }; |
(we might also want to rename Q, but doesn't need to be done here).
Collaborator
Author
There was a problem hiding this comment.
I'd avoid "privateKey" and "publicKey" because they are confusing in a context where we also have long-term private and public keys (e.g. Q).
The letters are convenient to follow what's going on, e.g. vQ === vdG === dvG === dV, and they are commonly used (e.g. see X25519 RFC).
Since we already have params named d and Q, I think switching to different names here is confusing, as the function is not generic.
If we really want to rename, I'd go with longTermKeyPair vs ephemeralKeyPair. But in a separate commit, so that it's clearer how the values map to the old ones with the lib switch.
Member
There was a problem hiding this comment.
OK, fair enough. Yeah, I'd prefer to have more descriptive names but indeed we can do it separately 👍
Unlike elliptic, noble-curves targets algorithmic constant time, and it relies on the native BigInts when available, resulting in a smaller bundle and improved performance. Also, expand testing of fallback elliptic implementation.
… by old lib bug At some point we used to generate invalid ECDSA sigs with the js (non-native) elliptic lib, if the signature digest had leading zeros: openpgpjs#948 . Brainpool curves are the most likely to have been affected by the bug, since they do not have WebCrypto support (unlike NIST curves). This commit reintroduces support on web to verify such invalid signatures (support for this was previously built-in in the indutny-elliptic library). It also expands the fix to work in Node.
To reflect change of underlying library
Instead of relying on externally provided one (no async loading supported)
larabr
force-pushed
the
v6-drop-elliptic
branch
from
111bf3d to
23f3908
Compare
twiss
approved these changes
Oct 23, 2023
This primarily affects the lightweight build, which will not include these (fairly large) libs in the main bundle file. This allows fetching their code only if required: - Noble-curves is only needed for curves other than curve25519. - Noble-hashes is needed for streamed hashing and e.g. SHA3 on web. - BN.js is used by the above libs, and it's also separately needed for platforms without native BigInt support.
Using a wrapper requires adding some handling code to fix race conditions, but it does not provide advantages until we switch to TS.
larabr
force-pushed
the
v6-drop-elliptic
branch
from
23f3908 to
fdcb199
Compare
…htweight build This is the default setting and it ensures that the main chunk does not include additional exports, which is is important when importing the module as `import *` as shown in the readme. In practice, this change does not affect the chunking with the current code.
larabr
force-pushed
the
v6-drop-elliptic
branch
from
fdcb199 to
ac7d2ca
Compare
andreanadr
approved these changes
Apr 2, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unlike elliptic.js, noble-curves targets algorithmic constant-timeness (close #720).
With these changes, both noble-curves and noble-hashes are loaded dynamically, meaning they are not included in the main bundle of the lightweight build.
This results in considerable bundle size optimizations, since these libs rely on the large BN.js lib when native BigInts are not available.
TODO: