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

crypto: add keyObject.export() JWK format option by panva · Pull Request #37081 · nodejs/node · GitHub

/ node Public

crypto: add keyObject.export() JWK format option - #37081

Closed
panva wants to merge 10 commits into
nodejs:masterfrom
panva:crypto-jwk-keyobject-export
ConversationCommits (10)ChecksFiles changed
Closed

crypto: add keyObject.export() JWK format option#37081
panva wants to merge 10 commits into
nodejs:masterfrom
panva:crypto-jwk-keyobject-export

Conversation

panva commented Jan 26, 2021

Copy link
Copy Markdown
Member

Adds JWK keyObject.export format option.

  • supported for key types: ec, rsa, ed25519, ed448, x25519, x448, and symmetric keys, resulting in JWK kty (Key Type) values EC, RSA, OKP, and oct.
  • rsa-pss is not supported since the JWK format does not support PSS Parameters.
  • EC JWK curves supported are P-256, secp256k1, P-384, and P-521

cc @nodejs/crypto

panva added 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. labels Jan 26, 2021
panva requested a review from tniessen January 26, 2021 17:27
nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jan 26, 2021
panva changed the title crypto: add keyObject.export() 'jwk' format option crypto: add keyObject.export() JWK format option Jan 26, 2021

This comment has been minimized.

Sign in to view
tniessen reviewed Jan 26, 2021
View reviewed changes

tniessen 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

Code itself LGTM, but I don't really trust my understanding of JWK enough to judge whether this is opening a new can of worms. (And yes, I've read most of the WebCrypto and JWK specs multiple times.)

Comment thread test/fixtures/keys/ec_secp256k1_public.pem Outdated
mscdex reviewed Jan 26, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
mscdex reviewed Jan 26, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated

panva commented Jan 26, 2021

Copy link
Copy Markdown
Member Author

Code itself LGTM, but I don't really trust my understanding of JWK enough to judge whether this is opening a new can of worms. (And yes, I've read most of the WebCrypto and JWK specs multiple times.)

@tniessen Anything I can do to help you disperse this particular trust issue?

This comment has been minimized.

Sign in to view
panva added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 26, 2021
jasnell reviewed Jan 26, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated

nodejs-github-bot commented Jan 27, 2021

Copy link
Copy Markdown
Collaborator

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

Trott reviewed Jan 31, 2021
View reviewed changes
Comment thread doc/api/errors.md Outdated
Trott reviewed Jan 31, 2021
View reviewed changes
Comment thread doc/api/errors.md Outdated
Trott previously requested changes Jan 31, 2021
View reviewed changes

Trott left a comment •
edited
Loading

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

The errors in errors.md are in alphabetical order so before this lands, the names need to be determined and then they need to be moved to the right place in the file. Once that's happened, feel free to clear this request for changes. No need to ping me on it or anything. Just leaving this here to make sure it happens before the code lands.

Trott commented Jan 31, 2021

Copy link
Copy Markdown
Member

I also wonder if we want to avoid creating additional error codes and use something fairly generic like the existing ERR_CRYPTO_UNSUPPORTED_OPERATION? If we want to keep separate error codes for unsupported curves and unsupported key types, maybe make a generic ERR_CRYPTO_UNSUPPORTED_X for each of those and use that? I don't feel strongly about this, though. If ERR_CRYPTO_JWK_UNSUPPORTED_CURVE is the way to go, then OK.

panva commented Jan 31, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I also wonder if we want to avoid creating additional error codes and use something fairly generic like the existing ERR_CRYPTO_UNSUPPORTED_OPERATION? If we want to keep separate error codes for unsupported curves and unsupported key types, maybe make a generic ERR_CRYPTO_UNSUPPORTED_X for each of those and use that? I don't feel strongly about this, though. If ERR_CRYPTO_JWK_UNSUPPORTED_CURVE is the way to go, then OK.

I've kept the specific errors here since i couldn't come up with a generic one. As far as future import feature comes in, these two will be re-used. I will keep the amount of specific error codes to the minimum based on this feedback though.

panva requested a review from Trott January 31, 2021 16:33

This comment has been minimized.

Sign in to view
panva dismissed Trott’s stale review January 31, 2021 16:38

feedback applied, @Trott said "feel free to clear this request for changes"

panva removed the request for review from Trott January 31, 2021 16:39
tniessen reviewed Feb 1, 2021
View reviewed changes
Comment thread doc/api/crypto.md Outdated
Comment thread lib/internal/crypto/keys.js Outdated
Comment thread test/parallel/test-crypto-key-objects.js Outdated
Comment thread test/parallel/test-crypto-key-objects.js Outdated

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
tniessen approved these changes Feb 1, 2021
View reviewed changes

tniessen 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

I still have my doubts about the integration of WebCrypto/JWK into the crypto module, but that might just be because I spent far too much time on core crypto and not enough time on web crypto standards. It's an incredibly complicated topic.

KeyObjects ignore some JWK properties, e.g., key usages. And that's by design, that's not what KeyObjects are for. But it also means that importing and then exporting a JWK means it loses those additional JWK properties.

ASN.1 remains the most relevant and popular format for public and private keys. I designed the KeyObject class and many other crypto APIs in core, but I cannot fully estimate the impact of the addition of web crypto standards on them. I am fine with these additions as long as they don't get in the way of ASN.1 import and export capabilities, or the mapping of asymmetricKeyType to EVP_PKEY_* in OpenSSL. If JWK key types and EVP_PKEY_* key types ever start diverging, we might be in serious trouble.

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
panva and others added 10 commits February 2, 2021 18:56
crypto: add keyObject.export() 'jwk' format option
9f6e3d1 
Adds [JWK](https://tools.ietf.org/html/rfc7517) keyObject.export format
option.

Supported key types: `ec`, `rsa`, `ed25519`, `ed448`, `x25519`, `x448`,
and symmetric keys, resulting in JWK `kty` (Key Type) values `EC`,
`RSA`, `OKP`, and `oct`.

`rsa-pss` is not supported since the JWK format does not support
PSS Parameters.

`EC` JWK curves supported are `P-256`, `secp256k1`, `P-384`, and `P-521`
add custom doc type
5ba12fb
add doc changes entry
1cfd243
address doc comments
25a6c8c
add makefile for fixtures
955696f
get a curve not supported on openssl111fips
a76db7a
Update doc/api/crypto.md
5f03cdf 
Co-authored-by: James M Snell <jasnell@gmail.com>
apply review feedback
5ca795c
address nits
44d49e0
fail on private export with a passphrase
a6d1e96
panva force-pushed the crypto-jwk-keyobject-export branch from 641de23 to a6d1e96 Compare February 2, 2021 17:57

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Feb 2, 2021

Copy link
Copy Markdown
Collaborator

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

panva added a commit that referenced this pull request Feb 2, 2021
crypto: add keyObject.export() 'jwk' format option
a8d7de1 
Adds [JWK](https://tools.ietf.org/html/rfc7517) keyObject.export format
option.

Supported key types: `ec`, `rsa`, `ed25519`, `ed448`, `x25519`, `x448`,
and symmetric keys, resulting in JWK `kty` (Key Type) values `EC`,
`RSA`, `OKP`, and `oct`.

`rsa-pss` is not supported since the JWK format does not support
PSS Parameters.

`EC` JWK curves supported are `P-256`, `secp256k1`, `P-384`, and `P-521`

PR-URL: #37081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

panva commented Feb 2, 2021

Copy link
Copy Markdown
Member Author

Landed in a8d7de1

panva closed this Feb 2, 2021
panva deleted the crypto-jwk-keyobject-export branch February 2, 2021 22:40
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
crypto: add keyObject.export() 'jwk' format option
011910b 
Adds [JWK](https://tools.ietf.org/html/rfc7517) keyObject.export format
option.

Supported key types: `ec`, `rsa`, `ed25519`, `ed448`, `x25519`, `x448`,
and symmetric keys, resulting in JWK `kty` (Key Type) values `EC`,
`RSA`, `OKP`, and `oct`.

`rsa-pss` is not supported since the JWK format does not support
PSS Parameters.

`EC` JWK curves supported are `P-256`, `secp256k1`, `P-384`, and `P-521`

PR-URL: #37081
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams added a commit that referenced this pull request Feb 16, 2021
2021-02-16, Version 15.9.0 (Current)
5133659 
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
This was referenced Feb 16, 2021
Closed
Merged
danielleadams added a commit that referenced this pull request Feb 17, 2021
2021-02-17, Version 15.9.0 (Current)
889ffde 
Notable Changes:

* crypto:
  * add keyObject.export() 'jwk' format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 17, 2021
2021-02-17, Version 15.9.0 (Current)
877f66e 
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
danielleadams added a commit that referenced this pull request Feb 18, 2021
2021-02-17, Version 15.9.0 (Current)
3bf50a7 
PR-URL: #37406

Notable Changes:

* crypto:
  * add keyObject.export() jwk format option (Filip Skokan) #37081
* deps:
  * upgrade to libuv 1.41.0 (Colin Ihrig) #37360
* doc:
  * add dmabupt to collaborators (Xu Meng) #37377
  * refactor fs docs structure (James M Snell) #37170
* fs:
  * add fsPromises.watch() (James M Snell) #37179
  * use a default callback for fs.close() (James M Snell) #37174
  * add AbortSignal support to watch (Benjamin Gruenbaum) #37190
* perf_hooks:
  * introduce createHistogram (James M Snell) #37155
* stream:
  * improve Readable.from error handling (Benjamin Gruenbaum) #37158
* timers:
  * introduce setInterval async iterator (linkgoron) #37153
* tls:
  * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070
panva mentioned this pull request Aug 21, 2021
Closed
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

jasnell jasnell approved these changes
tniessen tniessen approved these changes
Trott Trott left review comments
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

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.

8 participants

Back | FazBrowse Home | New Git URL