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

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

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 11, 2021

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

@nodejs-github-bot 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 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
@devsnek

This comment has been minimized.

@jasnell
Copy link
Member Author

jasnell commented Jan 11, 2021

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2021
@nodejs-github-bot
Copy link
Collaborator

@panva panva self-requested a review January 11, 2021 18:31
@panva
Copy link
Member

panva commented Jan 11, 2021

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.

@jasnell
Copy link
Member Author

jasnell commented Jan 11, 2021

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

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

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

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

@panva
Copy link
Member

panva commented Jan 12, 2021

@jasnell @tniessen this?

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

@panva panva removed their request for review January 13, 2021 14:49
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 14, 2021

Copy link
Member

@panva panva left a 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)

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
Copy link
Member Author

jasnell commented Jan 15, 2021

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

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

jasnell commented Jan 15, 2021

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

@jasnell jasnell removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 15, 2021
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@panva panva left a 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.

jasnell added a commit that referenced this pull request Jan 18, 2021
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
Copy link
Member Author

jasnell commented Jan 18, 2021

Landed in bd899bc

@jasnell jasnell closed this Jan 18, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
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 added a commit that referenced this pull request Jan 22, 2021
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)
* 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)
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
ruyadorno added a commit that referenced this pull request Jan 22, 2021
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)
* 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)
ruyadorno added a commit that referenced this pull request Jan 22, 2021
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
Copy link
Member

tniessen commented Jan 23, 2021

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

@tniessen
Copy link
Member

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

targos pushed a commit that referenced this pull request Feb 2, 2021
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 added a commit that referenced this pull request Feb 2, 2021
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: TODO
@targos targos mentioned this pull request Feb 2, 2021
targos added a commit that referenced this pull request Feb 2, 2021
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
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
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
8 tasks
@twiss twiss mentioned this pull request Feb 19, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants