Skip to content
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

[v6] Replace elliptic.js with noble-curves, and load noble-hashes dynamically #1694

Merged
merged 9 commits into from
Oct 23, 2023

Conversation

larabr
Copy link
Collaborator

@larabr larabr commented Oct 18, 2023

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:

  • merge after Ed448 PR.

src/biginteger.js Outdated Show resolved Hide resolved
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) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair enough. Yeah, I'd prefer to have more descriptive names but indeed we can do it separately 👍

src/crypto/public_key/elliptic/ecdh.js Outdated Show resolved Hide resolved
src/crypto/public_key/elliptic/ecdsa.js Outdated Show resolved Hide resolved
src/crypto/public_key/elliptic/ecdsa.js Outdated Show resolved Hide resolved
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)
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.
…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 larabr merged commit 3f14488 into openpgpjs:v6 Oct 23, 2023
12 checks passed
@larabr larabr mentioned this pull request Oct 23, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants