GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — crypto: experimental (Ed/X)25519/(Ed/X)448 support by jasnell · Pull Request #36879 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Jan 11, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 [email protected]

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with hooray emoji bricss and gobengo reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 2 reactions
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Jan 11, 2021
@jasnell jasnell added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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
@devsnek

This comment has been minimized.

Sign in to view
@jasnell

jasnell commented Jan 11, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the webcrypto-raw-keys branch from b20d66d to e174d08 Compare January 11, 2021 18:17
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 11, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 11, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva
panva self-requested a review January 11, 2021 18:31
@panva

panva commented Jan 11, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jan 11, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 12, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 12, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 12, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen

tniessen commented Jan 12, 2021

Copy link
Copy Markdown
Member

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva

panva commented Jan 12, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@jasnell @tniessen this?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva
panva removed their request for review January 13, 2021 14:49
@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell requested review from addaleax, devsnek and panva January 15, 2021 16:33
panva
panva reviewed Jan 15, 2021
View reviewed changes

@panva panva left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell
jasnell force-pushed the webcrypto-raw-keys branch from 97ce2c7 to c9315fb Compare January 15, 2021 17:55
@jasnell
crypto: experimental (Ed/X)25519/(Ed/X)448 support
422f1d0 
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 <[email protected]>
Fixes: nodejs#36076
@jasnell
jasnell force-pushed the webcrypto-raw-keys branch from c9315fb to 422f1d0 Compare January 15, 2021 18:47
@jasnell

jasnell commented Jan 15, 2021

Copy link
Copy Markdown
Member Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 15, 2021
@jasnell
fixup! crypto: experimental (Ed/X)25519/(Ed/X)448 support
5722494
@jasnell

jasnell commented Jan 15, 2021

Copy link
Copy Markdown
Member Author

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
panva reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 15, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 15, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
fixup! fixup! crypto: experimental (Ed/X)25519/(Ed/X)448 support
8eed610
panva
panva approved these changes Jan 16, 2021
View reviewed changes

@panva panva left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@panva panva mentioned this pull request Jan 18, 2021
crypto: add KeyObject.from and keyObject.export JWK format support #36203
Closed
4 tasks
jasnell added a commit that referenced this pull request Jan 18, 2021
@jasnell
crypto: experimental (Ed/X)25519/(Ed/X)448 support
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 <[email protected]>
Fixes: #36076

PR-URL: #36879
Reviewed-By: Filip Skokan <[email protected]>
@jasnell

jasnell commented Jan 18, 2021

Copy link
Copy Markdown
Member Author

Landed in bd899bc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Jan 18, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
@jasnell @ruyadorno
crypto: experimental (Ed/X)25519/(Ed/X)448 support
68ffe5a 
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 <[email protected]>
Fixes: #36076

PR-URL: #36879
Reviewed-By: Filip Skokan <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
v15.7.0 proposal #37020
Merged
ruyadorno added a commit that referenced this pull request Jan 22, 2021
@ruyadorno
2021-01-22, Version 15.7.0 (Current)
78b8cde 
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)
@tniessen

tniessen commented Jan 23, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tniessen tniessen mentioned this pull request Jan 24, 2021
generateKeyPair('ec') should not support NODE-ED* and NODE-X* #37055
Closed
@tniessen

tniessen commented Jan 24, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@panva panva mentioned this pull request Jan 25, 2021
crypto: generateKeyPair('ec') should not support NODE-ED* and NODE-X* #37063
Closed
targos pushed a commit that referenced this pull request Feb 2, 2021
@jasnell @targos
crypto: experimental (Ed/X)25519/(Ed/X)448 support
53a0bdf 
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 <[email protected]>
Fixes: #36076

PR-URL: #36879
Reviewed-By: Filip Skokan <[email protected]>
@targos targos mentioned this pull request Feb 2, 2021
v15.8.0 release proposal #37183
Merged
targos added a commit that referenced this pull request Feb 2, 2021
@targos
2021-02-02, Version 15.8.0 (Current)
5a59dde 
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
targos added a commit that referenced this pull request Feb 2, 2021
@targos
2021-02-02, Version 15.8.0 (Current)
bc1c428 
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
targos added a commit that referenced this pull request Feb 2, 2021
@targos
2021-02-02, Version 15.8.0 (Current)
211574b 
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
@tniessen tniessen mentioned this pull request Feb 5, 2021
crypto: introduce crypto/promises #37218
Closed
8 tasks
@vlovich vlovich mentioned this pull request May 7, 2021
support for Curve25519 in WebCrypto. w3c/webcrypto#233
Closed
@targos targos added dont-land-on-v12.x labels Aug 8, 2021
@twiss twiss mentioned this pull request Feb 19, 2024
Add Curve25519 w3c/webcrypto#362
Merged
8 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panva panva panva approved these changes
@addaleax addaleax Awaiting requested review from addaleax
@devsnek devsnek Awaiting requested review from devsnek

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jasnell @devsnek @nodejs-github-bot @panva @tniessen @targos
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.