crypto: experimental (Ed/X)25519/(Ed/X)448 support by jasnell · Pull Request #36879 · nodejs/node · GitHub

Implements initial experimental support for Curve25519 and Curve448 support for both ECDH and sign/verify in Web Crypto (with raw public and private key import)

Introduced as a Node.js-specific extension to Web Crypto.

Signed-off-by: James M Snell jasnell@gmail.com

/cc @indutny @panva @devsnek @bnb @nodejs/crypto

@tniessen ... will this need the mutexes on the key uses?

CI: https://ci.nodejs.org/job/node-test-pull-request/35450/

Didn't dive into a detailed review yet, one thing i see tho is this being an extension to the "EC" key type. As in, the docs extend e.g. EcKeyImportParams rather than definining the right thing...

In the JWK registry, the Key Type ('kty') for these keys is not "EC", it's "OKP". Defined in rfc8037, private key components are (in addition to kty) crv, x and d; public only crv, and x. The crv (when in OKP keys being the "sub type", not curve 🤦) values when JWK format is used are Ed25519, Ed448, X25519, X448.

I don't know how to deal with this being a node-specific extension to webcrypto as in - should we allow JWK export/import? I'll play around with the implementation before coming back with more feedback.

Yeah I specifically punted on the JWK support for now for precisely those reasons.

CI: https://ci.nodejs.org/job/node-test-pull-request/35458/

CI: https://ci.nodejs.org/job/node-test-pull-request/35460/

@tniessen ... will this need the mutexes on the key uses?

@jasnell Sorry, could you clarify which code sections you are referring to?

@jasnell @tniessen this?

ed25519/448 private - ['sign']
ed25519/448 public - ['verify']
x25519/448 private - ['deriveBits', 'deriveKey']
x25519/448 public - []

CI: https://ci.nodejs.org/job/node-test-pull-request/35533/ 💚

I ran just a few sanity checks so far:

⬇️ I would expect these to fail on the account of ECDH being forbidden with Ed* in RFC8037, not sure if outside of JOSE its okay.

subtle.generateKey({ name: 'ECDH', namedCurve: 'NODE-ED25519' }, true, ['deriveKey', 'deriveBits'])
subtle.generateKey({ name: 'ECDH', namedCurve: 'NODE-ED448' }, true, ['deriveKey', 'deriveBits'])

⬇️ I would expect these to fail on the account of ECDSA being forbidden with X* in RFC8037, not sure if outside of JOSE its okay.

subtle.generateKey({ name: 'ECDSA', namedCurve: 'NODE-X25519' }, true, ['sign', 'verify'])
subtle.generateKey({ name: 'ECDSA', namedCurve: 'NODE-X448' }, true, ['sign', 'verify'])

Number of oddities follow

⬇️ Is that rejection expected?

> kp
{
  publicKey: CryptoKey {
    type: 'public',
    extractable: true,
    algorithm: { name: 'NODE-ED25519', namedCurve: 'NODE-ED25519' },
    usages: [ 'verify' ]
  },
  privateKey: CryptoKey {
    type: 'private',
    extractable: true,
    algorithm: { name: 'NODE-ED25519', namedCurve: 'NODE-ED25519' },
    usages: [ 'sign' ]
  }
}
> subtle.exportKey('raw',kp.publicKey).then(console.log)
Promise { <pending> }
> ArrayBuffer {
  [Uint8Contents]: <80 b1 40 32 f2 4e eb 54 7e c5 18 10 de 28 64 5b b2 34 b5 8d f7 64 84 43 61 30 ae b9 d8 5e bf fa>,
  byteLength: 32
}
> subtle.exportKey('raw',kp.privateKey).then(console.log)
Promise { <pending> }
> Uncaught:
DOMException [InvalidAccessError]: Unable to export a raw NODE-ED25519 private key
    at __node_internal_ (node:internal/crypto/util:71:10)
    at exportKeyRaw (node:internal/crypto/webcrypto:310:9)
    at SubtleCrypto.exportKey (node:internal/crypto/webcrypto:382:24)

Implements initial experimental support for Curve25519 and Curve448 support for both ECDH and sign/verify in Web Crypto. Introduced as a Node.js-specific extension to Web Crypto. Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: nodejs#36076

@panva:

I would expect these to fail on the account of ECDH being forbidden with Ed* in RFC8037, not sure if outside of JOSE its okay.
...
I would expect these to fail on the account of ECDSA being forbidden with X* in RFC8037, not sure if outside of JOSE its okay.

Good catch.

Is that rejection expected?

Yes, but only because I hadn't completed it. Just added those bits. Note that it's still not possible to export an ECDH private key (so X25519 private keys) per spec.

the public key property should be x, not k. k is only used by oct secret keys.

Doh, I knew that lol.. not sure why I had k stuck in my head. Fixed

CI: https://ci.nodejs.org/job/node-test-pull-request/35564/

Can't speak for the c++ implementation. The API surface LGTM.

Implements initial experimental support for Curve25519 and Curve448 support for both ECDH and sign/verify in Web Crypto. Introduced as a Node.js-specific extension to Web Crypto. Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: #36076 PR-URL: #36879 Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Landed in bd899bc

Implements initial experimental support for Curve25519 and Curve448 support for both ECDH and sign/verify in Web Crypto. Introduced as a Node.js-specific extension to Web Crypto. Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: #36076 PR-URL: #36879 Reviewed-By: Filip Skokan <panva.ip@gmail.com>

PR-URL: #37020 Notable changes: * buffer: * introduce Blob (James M Snell) [#36811](#36811) * add base64url encoding option (Filip Skokan) [#36952](#36952) * crypto: * experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) [#36879](#36879) * doc: * add @iansu to collaborators (Ian Sutherland) [#36951](#36951) * add @RaisinTen to collaborators (Darshan Sen) [#36998](#36998) * add @miladfarca to collaborators (Milad Fa) [#36934](#36934) * fs: * allow position parameter to be a BigInt in read and readSync (raisinten) [#36190](#36190) * http: * attach request as res.req (Ian Storm Taylor) [#36505](#36505) * expose urlToHttpOptions utility (Yongsheng Zhang) [#35960](#35960)

Note that this technically landed without approval of the full PR. (And our rules might technically allow that.)

Hoping to get this issue addressed before this PR lands in a release: #37055.

Implements initial experimental support for Curve25519 and Curve448 support for both ECDH and sign/verify in Web Crypto. Introduced as a Node.js-specific extension to Web Crypto. Signed-off-by: James M Snell <jasnell@gmail.com> Fixes: #36076 PR-URL: #36879 Reviewed-By: Filip Skokan <panva.ip@gmail.com>

Notable changes: crypto: * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997 * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879 deps: * upgrade npm to 7.5.0 (Ruy Adorno) #37117 dgram: * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026 doc: * add Zijian Liu to collaborators (ZiJian Liu) #37075 esm: * deprecate legacy main lookup for modules (Guy Bedford) #36918 readline: * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662 * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676 PR-URL: #37183

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 11, 2021

jasnell added request-ci Add this label to start a Jenkins CI on a PR. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. experimental Issues and PRs related to experimental features. labels Jan 11, 2021

This comment has been minimized.

Sign in to view

jasnell force-pushed the webcrypto-raw-keys branch from b20d66d to e174d08 Compare January 11, 2021 18:17

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2021

panva self-requested a review January 11, 2021 18:31

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021

panva removed their request for review January 13, 2021 14:49

jasnell requested review from addaleax, devsnek and panva January 15, 2021 16:33

panva reviewed Jan 15, 2021

View reviewed changes

jasnell force-pushed the webcrypto-raw-keys branch from 97ce2c7 to c9315fb Compare January 15, 2021 17:55

jasnell force-pushed the webcrypto-raw-keys branch from c9315fb to 422f1d0 Compare January 15, 2021 18:47

jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2021

fixup! crypto: experimental (Ed/X)25519/(Ed/X)448 support

5722494

jasnell removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2021

fixup! fixup! crypto: experimental (Ed/X)25519/(Ed/X)448 support

8eed610

panva approved these changes Jan 16, 2021

View reviewed changes

panva mentioned this pull request Jan 18, 2021

crypto: add KeyObject.from and keyObject.export JWK format support #36203

Closed

4 tasks

jasnell closed this Jan 18, 2021

ruyadorno mentioned this pull request Jan 22, 2021

v15.7.0 proposal #37020

Merged

tniessen mentioned this pull request Jan 24, 2021

generateKeyPair('ec') should not support NODE-ED* and NODE-X* #37055

Closed

panva mentioned this pull request Jan 25, 2021

crypto: generateKeyPair('ec') should not support NODE-ED* and NODE-X* #37063

Closed

targos mentioned this pull request Feb 2, 2021

v15.8.0 release proposal #37183

Merged

tniessen mentioned this pull request Feb 5, 2021

crypto: introduce crypto/promises #37218

Closed

8 tasks

vlovich mentioned this pull request May 7, 2021

support for Curve25519 in WebCrypto. w3c/webcrypto#233

Closed

targos added dont-land-on-v12.x labels Aug 8, 2021

twiss mentioned this pull request Feb 19, 2024

Add Curve25519 w3c/webcrypto#362

Merged

8 tasks

crypto: experimental (Ed/X)25519/(Ed/X)448 support by jasnell · Pull Request #36879 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: experimental (Ed/X)25519/(Ed/X)448 support - #36879

crypto: experimental (Ed/X)25519/(Ed/X)448 support#36879jasnell wants to merge 3 commits intonodejs:masternodejs/node:masterfrom jasnell:webcrypto-raw-keysjasnell/node:webcrypto-raw-keysCopy head branch name to clipboard

Conversation

jasnell commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

jasnell commented Jan 11, 2021

Uh oh!

nodejs-github-bot commented Jan 11, 2021

Uh oh!

panva commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jan 11, 2021

Uh oh!

nodejs-github-bot commented Jan 12, 2021

Uh oh!

nodejs-github-bot commented Jan 12, 2021

Uh oh!

tniessen commented Jan 12, 2021

Uh oh!

panva commented Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 14, 2021 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 15, 2021

Uh oh!

jasnell commented Jan 15, 2021

Uh oh!

nodejs-github-bot commented Jan 15, 2021

Uh oh!

panva left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 18, 2021

Uh oh!

tniessen commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Jan 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

crypto: experimental (Ed/X)25519/(Ed/X)448 support#36879
jasnell wants to merge 3 commits into
nodejs:masterfrom
jasnell:webcrypto-raw-keys

jasnell commented Jan 11, 2021 •
edited
Loading

panva commented Jan 11, 2021 •
edited
Loading

panva commented Jan 12, 2021 •
edited
Loading

nodejs-github-bot commented Jan 14, 2021 •
edited by jasnell
Loading

panva left a comment •
edited
Loading

tniessen commented Jan 23, 2021 •
edited
Loading