X Tutup
The Wayback Machine - https://web.archive.org/web/20210318141410/https://github.com/nodejs/node/pull/37254
Skip to content

/ node

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from
+246 −56

Conversation

@panva
Copy link
Member

@panva panva commented Feb 6, 2021 •
edited

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 panva requested review from jasnell and tniessen Feb 6, 2021
@nodejs-github-bot nodejs-github-bot added crypto errors labels Feb 6, 2021
@panva
Copy link
Member Author

@panva panva commented Feb 6, 2021

I think this will conflict with #37240, but opening here to get early feedback.
@panva panva added the semver-minor label Feb 6, 2021
@panva
Copy link
Member Author

@panva panva commented Feb 8, 2021

cc @nodejs/crypto
@panva panva marked this pull request as ready for review Feb 8, 2021
@panva panva force-pushed the panva:crypto-jwk-keyobject-import branch from 3f77664 to 42b091a Feb 8, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@tniessen
Copy link
Member

@tniessen tniessen commented Feb 9, 2021

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 panva changed the title crypto: support JWK objects in create*Key crypto: support JWK objects in create(Public|Private)Key Feb 9, 2021
@panva
Copy link
Member Author

@panva panva commented Feb 9, 2021 •
edited

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

@tniessen tniessen commented Feb 18, 2021

@panva Is this a WIP or ready to be reviewed?
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@panva
Copy link
Member Author

@panva panva commented Feb 18, 2021

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

To be reviewed please.
@tniessen
tniessen reviewed Feb 18, 2021
View changes
doc/api/crypto.md Outdated Show resolved Hide resolved
lib/internal/crypto/keys.js Outdated Show resolved Hide resolved
lib/internal/crypto/keys.js
validateString(key.qi, 'key.qi');
}

const handle = new KeyObjectHandle();

This comment has been minimized.

Sign in to view
@tniessen

tniessen Feb 18, 2021
Member

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

This comment has been minimized.

Sign in to view
@panva

panva Feb 18, 2021
Author Member

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

This comment has been minimized.

Sign in to view
@tniessen

tniessen Mar 10, 2021
Member

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.
lib/internal/errors.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@panva panva force-pushed the panva:crypto-jwk-keyobject-import branch from 7c9fedf to b41c5d3 Feb 21, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@panva panva requested a review from tniessen Feb 24, 2021
@panva panva force-pushed the panva:crypto-jwk-keyobject-import branch from b41c5d3 to d54e775 Feb 24, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@panva panva requested review from jasnell and removed request for jasnell Feb 24, 2021
@panva
Copy link
Member Author

@panva panva commented Feb 26, 2021

cc @nodejs/crypto
@panva panva added the review wanted label Mar 8, 2021
@jasnell
jasnell reviewed Mar 8, 2021
View changes
lib/internal/crypto/keys.js Outdated Show resolved Hide resolved
@jasnell
jasnell approved these changes Mar 8, 2021
View changes
Copy link
Member

@jasnell jasnell left a comment

LGTM with exception to the use of delete as commented.
@panva panva force-pushed the panva:crypto-jwk-keyobject-import branch from d54e775 to bb9a212 Mar 8, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@panva panva added author ready and removed review wanted labels Mar 8, 2021
@panva
crypto: support JWK objects in create*Key
Loading status checks…
4485936
@panva panva force-pushed the panva:crypto-jwk-keyobject-import branch from bb9a212 to 4485936 Mar 8, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Mar 9, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36558/
@mcollina
mcollina approved these changes Mar 10, 2021
View changes
Copy link
Member

@mcollina mcollina left a comment

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

@panva panva commented Mar 10, 2021

Landed in 117e293
@panva panva closed this Mar 10, 2021
@panva panva deleted the panva:crypto-jwk-keyobject-import branch Mar 10, 2021
danielleadams added a commit that referenced this pull request Mar 16, 2021
@panva @danielleadams
crypto: support JWK objects in create*Key
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
55e522c 
PR-URL: #37254
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams added a commit that referenced this pull request Mar 16, 2021
@danielleadams
2021-03-16, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
6144aa7 
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 danielleadams mentioned this pull request Mar 16, 2021
Merged
danielleadams added a commit that referenced this pull request Mar 16, 2021
@danielleadams
2021-03-16, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
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
@danielleadams
2021-03-16, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
cfc87a2 
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
@danielleadams
2021-03-16, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
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
@danielleadams
2021-03-16, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
aba22e7 
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
@danielleadams
2021-03-17, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
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
@danielleadams
2021-03-17, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
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
@danielleadams
2021-03-17, Version 15.12.0 (Current)
Verified
This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
GPG key ID: D3A89613643B6201 Learn about signing commits
Loading status checks…
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina

@jasnell jasnell

@tniessen tniessen

Assignees
No one assigned
Labels
author ready crypto errors semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@panva @nodejs-github-bot @tniessen @mcollina @jasnell
X Tutup