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: support JWK objects in create(Public|Private)Key #37254

Closed
wants to merge 1 commit into from

Conversation

panva
Copy link
Member

@panva panva commented Feb 6, 2021

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

@nodejs-github-bot 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
Copy link
Member Author

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 PRs that contain new features and should be released in the next minor version. label Feb 6, 2021
@panva
Copy link
Member Author

panva commented Feb 8, 2021

cc @nodejs/crypto

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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@tniessen
Copy link
Member

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 commented Feb 9, 2021

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

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

@nodejs-github-bot

This comment has been minimized.

@panva
Copy link
Member Author

panva commented Feb 18, 2021

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

To be reviewed please.

doc/api/crypto.md Outdated Show resolved Hide resolved
lib/internal/crypto/keys.js Outdated Show resolved Hide resolved
validateString(key.qi, 'key.qi');
}

const handle = new KeyObjectHandle();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@panva panva force-pushed the crypto-jwk-keyobject-import branch from 7c9fedf to b41c5d3 Compare February 21, 2021 20:33
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@panva
Copy link
Member Author

panva commented Feb 26, 2021

cc @nodejs/crypto

@panva panva added the review wanted PRs that need reviews. label Mar 8, 2021
lib/internal/crypto/keys.js Outdated Show resolved Hide resolved
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with exception to the use of delete as commented.

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

This comment has been minimized.

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels Mar 8, 2021
@panva panva force-pushed the crypto-jwk-keyobject-import branch from bb9a212 to 4485936 Compare March 8, 2021 23:06
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

panva added a commit that referenced this pull request Mar 10, 2021
PR-URL: #37254
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@panva
Copy link
Member Author

panva commented Mar 10, 2021

Landed in 117e293

@panva panva closed this Mar 10, 2021
@panva 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
PR-URL: #37254
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
danielleadams added a commit that referenced this pull request Mar 16, 2021
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
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
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
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
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
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
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 [email protected] (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
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 [email protected] (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
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
Development

Successfully merging this pull request may close these issues.

6 participants