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: add keyObject.export() JWK format option #37081

Closed
wants to merge 10 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Jan 26, 2021

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

This comment has been minimized.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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

test/fixtures/keys/ec_secp256k1_public.pem Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Outdated Show resolved Hide resolved
@panva
Copy link
Member Author

panva commented Jan 26, 2021

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?

@nodejs-github-bot

This comment has been minimized.

@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2021
doc/api/crypto.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/errors.md Outdated Show resolved Hide resolved
Trott
Trott previously requested changes Jan 31, 2021
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

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

Trott commented Jan 31, 2021

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

panva commented Jan 31, 2021

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 panva requested a review from Trott January 31, 2021 16:33
@nodejs-github-bot

This comment has been minimized.

@panva panva dismissed Trott’s stale review January 31, 2021 16:38

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

@panva panva removed the request for review from Trott January 31, 2021 16:39
doc/api/crypto.md Outdated Show resolved Hide resolved
lib/internal/crypto/keys.js Show resolved Hide resolved
test/parallel/test-crypto-key-objects.js Show resolved Hide resolved
test/parallel/test-crypto-key-objects.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

panva and others added 10 commits February 2, 2021 18:56
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`
Co-authored-by: James M Snell <jasnell@gmail.com>
@panva panva force-pushed the crypto-jwk-keyobject-export branch from 641de23 to a6d1e96 Compare February 2, 2021 17:57
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

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

panva added a commit that referenced this pull request Feb 2, 2021
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
Copy link
Member Author

panva commented Feb 2, 2021

Landed in a8d7de1

@panva panva closed this Feb 2, 2021
@panva 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
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
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
danielleadams added a commit that referenced this pull request Feb 17, 2021
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
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
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
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
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.

8 participants