FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: support JWK objects in create(Public|Private)Key by panva · Pull Request #37254 · nodejs/node · GitHub

/ node Public

crypto: support JWK objects in create(Public|Private)Key - #37254

Closed
panva wants to merge 1 commit into
nodejs:masterfrom
panva:crypto-jwk-keyobject-import
ConversationCommits (1)ChecksFiles changed
Closed

crypto: support JWK objects in create(Public|Private)Key#37254
panva wants to merge 1 commit into
nodejs:masterfrom
panva:crypto-jwk-keyobject-import

Conversation

panva commented Feb 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This enables create(Public|Private)Key JWK inputs.

crypto.createPublicKey({ key: jwk, format: 'jwk' })

  • Allows both private and public JWKs, always creates a PublicKeyObject
  • Enabled key types RSA, EC (P-256, secp256k1, P-384, P-521), OKP (Ed25519, Ed448, X25519, X448)

crypto.createPrivateKey({ key: jwk, format: 'jwk' })

  • Allows private JWKs, always creates a PrivateKeyObject
  • Enabled key types RSA, EC (P-256, secp256k1, P-384, P-521), OKP (Ed25519, Ed448, X25519, X448)

crypto.createSecretKey(jwk)

  • Allows "oct" JWKs, always creates a SecretKeyObject
  • Enabled key types oct

panva requested review from jasnell and tniessen February 6, 2021 21:49
nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Feb 6, 2021

panva commented Feb 6, 2021

Copy link
Copy Markdown
Member Author

I think this will conflict with #37240, but opening here to get early feedback.

panva added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 6, 2021

panva commented Feb 8, 2021

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

panva marked this pull request as ready for review February 8, 2021 12:29
panva force-pushed the crypto-jwk-keyobject-import branch from 3f77664 to 42b091a Compare February 8, 2021 12:30

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

tniessen commented Feb 9, 2021

Copy link
Copy Markdown
Member

We can also make it work without the ({ key: jwk, format: 'jwk' }) layer, just (jwk) since a JWK will always be an object with a kty property

I don't think we should. What if another "plain object" key type emerges one day and the user passes an object that has both a kty and a somePropertyDefinedByAnotherArbitrarySpec property? The main reason why we support (pem) is for historical reasons and backward compatibility, plus, PEM is still the standard format for HTTPS keys and certificates.

panva changed the title crypto: support JWK objects in create*Key crypto: support JWK objects in create(Public|Private)Key Feb 9, 2021

panva commented Feb 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I've pulled the createSecretKey change. To be done separately if requested. It's as simple to do as createSecretKey(jwk.k, 'base64') anyway.

tniessen commented Feb 18, 2021

Copy link
Copy Markdown
Member

@panva Is this a WIP or ready to be reviewed?

This comment has been minimized.

Sign in to view

panva commented Feb 18, 2021

Copy link
Copy Markdown
Member Author

@panva Is this a WIP or ready to be reviewed?

To be reviewed please.

tniessen reviewed Feb 18, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
Comment thread lib/internal/crypto/keys.js Outdated
Comment thread lib/internal/crypto/keys.js Outdated

tniessen Feb 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not sure how I feel about creating KeyObjectHandles in more places.

panva Feb 18, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Do you have a concrete suggestion? I'm only using what's available.

tniessen Mar 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not right now, sorry. We can probably improve that later. I don't think this internal inconsistency has any visible effect to the user, but I'm not entirely sure.

Comment thread lib/internal/errors.js Outdated

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
panva force-pushed the crypto-jwk-keyobject-import branch from 7c9fedf to b41c5d3 Compare February 21, 2021 20:33

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
panva requested a review from tniessen February 24, 2021 00:09
panva force-pushed the crypto-jwk-keyobject-import branch from b41c5d3 to d54e775 Compare February 24, 2021 10:06

This comment has been minimized.

Sign in to view

panva commented Feb 26, 2021

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

panva added the review wanted PRs that need reviews. label Mar 8, 2021
jasnell reviewed Mar 8, 2021
View reviewed changes
Comment thread lib/internal/crypto/keys.js Outdated
jasnell approved these changes Mar 8, 2021
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with exception to the use of delete as commented.

panva force-pushed the crypto-jwk-keyobject-import branch from d54e775 to bb9a212 Compare March 8, 2021 22:52

This comment has been minimized.

Sign in to view
panva added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed review wanted PRs that need reviews. labels Mar 8, 2021
panva force-pushed the crypto-jwk-keyobject-import branch from bb9a212 to 4485936 Compare March 8, 2021 23:06

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Mar 9, 2021

Copy link
Copy Markdown
Collaborator

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

mcollina approved these changes Mar 10, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

panva added a commit that referenced this pull request Mar 10, 2021
crypto: support JWK objects in create*Key
117e293 
PR-URL: #37254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

panva commented Mar 10, 2021

Copy link
Copy Markdown
Member Author

Landed in 117e293

panva closed this Mar 10, 2021
panva deleted the crypto-jwk-keyobject-import branch March 10, 2021 17:50
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
crypto: support JWK objects in create*Key
55e522c 
PR-URL: #37254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams mentioned this pull request Mar 16, 2021
Merged
danielleadams added a commit that referenced this pull request Mar 16, 2021
2021-03-16, Version 15.12.0 (Current)
e761da8 
PR-URL: #37766

Notable changes:

* crypto:
  * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500
  * support JWK objects in create\*Key (Filip Skokan) #37254
* deps:
  * update archs files for OpenSSL-1.1.1+quic (James M Snell) #37601
  * switch openssl to quictls/openssl (James M Snell) #37601
* doc:
  * update maintaining-openssl guide (James M Snell) #37601
* fs:
  * improve fsPromises writeFile performance (Nitzan Uziely) #37610
  * add promisified readFile benchmark (Nitzan Uziely) #37608
  * improve fsPromises readFile performance (Nitzan Uziely) #37608
* lib:
  * implement AbortSignal.abort() (James M Snell) #37693
* node-api:
  * define version 8 (Gabriel Schulhof) #37652
* test:
  * update dom/abort tests (James M Snell) #37693
  * fixup test to account for quic openssl version (James M Snell) #37601
* worker:
  * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
danielleadams added a commit that referenced this pull request Mar 16, 2021
2021-03-16, Version 15.12.0 (Current)
71b37d8 
PR-URL: #37766

Notable changes:

* crypto:
  * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500
  * support JWK objects in create\*Key (Filip Skokan) #37254
* deps:
  * switch openssl to quictls/openssl (James M Snell) #37601
* fs:
  * improve fsPromises writeFile performance (Nitzan Uziely) #37610
  * improve fsPromises readFile performance (Nitzan Uziely) #37608
* lib:
  * implement AbortSignal.abort() (James M Snell) #37693
* node-api:
  * define version 8 (Gabriel Schulhof) #37652
* worker:
  * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
danielleadams added a commit that referenced this pull request Mar 17, 2021
2021-03-17, Version 15.12.0 (Current)
703aec8 
PR-URL: #37766

Notable changes:

* crypto:
  * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500
  * support JWK objects in create\*Key (Filip Skokan) #37254
* deps:
  * switch openssl to quictls/openssl (James M Snell) #37601
* fs:
  * improve fsPromises writeFile performance (Nitzan Uziely) #37610
  * improve fsPromises readFile performance (Nitzan Uziely) #37608
* lib:
  * implement AbortSignal.abort() (James M Snell) #37693
* node-api:
  * define version 8 (Gabriel Schulhof) #37652
* worker:
  * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
danielleadams added a commit that referenced this pull request Mar 17, 2021
2021-03-17, Version 15.12.0 (Current)
5011009 
PR-URL: #37766

Notable changes:

* crypto:
  * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500
  * support JWK objects in create\*Key (Filip Skokan) #37254
* deps:
  * update to cjs-module-lexer@1.1.0 (Guy Bedford) #37712
  * switch openssl to quictls/openssl (James M Snell) #37601
* fs:
  * improve fsPromises writeFile performance (Nitzan Uziely) #37610
  * improve fsPromises readFile performance (Nitzan Uziely) #37608
* lib:
  * implement AbortSignal.abort() (James M Snell) #37693
* node-api:
  * define version 8 (Gabriel Schulhof) #37652
* worker:
  * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
danielleadams added a commit that referenced this pull request Mar 17, 2021
2021-03-17, Version 15.12.0 (Current)
3b3b02f 
PR-URL: #37766

Notable changes:

* crypto:
  * add optional callback to crypto.sign and crypto.verify (Filip Skokan) #37500
  * support JWK objects in create\*Key (Filip Skokan) #37254
* deps:
  * update to cjs-module-lexer@1.1.0 (Guy Bedford) #37712
  * switch openssl to quictls/openssl (James M Snell) #37601
* fs:
  * improve fsPromises writeFile performance (Nitzan Uziely) #37610
  * improve fsPromises readFile performance (Nitzan Uziely) #37608
* lib:
  * implement AbortSignal.abort() (James M Snell) #37693
* node-api:
  * define version 8 (Gabriel Schulhof) #37652
* worker:
  * add setEnvironmentData/getEnvironmentData (James M Snell) #37486
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
tniessen Awaiting requested review from tniessen

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. 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.

6 participants

Back | FazBrowse Home | New Git URL