crypto: add rsa-pss keygen parameters #39927

panva · 2021-08-29T08:13:58Z

This PR deprecates (documentation-only deprecation with --pending-deprecation) undocumented API for RSA-PSS key generation parameters that specify the RSASSA-PSS-params sequence and replaces them with options aligned with asymmetricKeyDetails property names.

The update is so that the option names and their values match the ones exposed via asymmetricKeyDetails from #39851. See #39851 (comment) and #39851 (comment)

~~It is semver-major because despite it being undocumented before it might be in use in by users.~~

Edit: We will runtime deprecate the undocumented ones via a followup PR, this is now a semver-minor as it is not breaking potential existing use of the undocumented API.

panva · 2021-08-29T18:28:04Z

cc @nodejs/crypto

tniessen · 2021-08-31T16:10:58Z

I think it makes sense to do this in two steps: add the new options in a semver-minor PR and runtime-deprecate the old options in a semver-major PR. It's more work, but if we don't do that, we can't use these options in any Node.js version < 17.

panva · 2021-08-31T16:19:01Z

I think it makes sense to do this in two steps: add the new options in a semver-minor PR and runtime-deprecate the old options in a semver-major PR. It's more work, but if we don't do that, we can't use these options in any Node.js version < 17.

Sounds good.

Do you recommend to apply the undocumented to be deprecated properties like so?

let { hash, hashAlgorithm } = options
if (hash) deprecationNotice()
hashAlgorithm ??= hash

// proceed with hashAlgorithm

tniessen · 2021-08-31T16:37:12Z

@panva I am not sure. It generally looks good to me but I have a slight preference for throwing if both are set and not equal.

panva · 2021-08-31T16:44:36Z

Sounds good, I'll prepare the PR(s) to supersede this one.

tniessen · 2021-08-31T16:45:18Z

@panva I think you can keep this one for either part and open one more for the other part :) Thank you!

panva · 2021-08-31T18:16:48Z

@tniessen never did a deprecation before, can you please check?

tniessen · 2021-08-31T19:57:32Z

Runtime deprecations must be semver-major:

add the new options in a semver-minor PR and runtime-deprecate the old options in a semver-major PR

We can do a documentation-only deprecation as semver-minor (or a documentation-only deprecation with --pending-deprecation), but not a runtime deprecation. Sorry, I should have been more specific before.

nodejs-github-bot · 2021-09-01T15:43:14Z

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

test/parallel/test-crypto-keygen.js

tniessen · 2021-09-04T09:48:21Z

I'd love to see a test that ensures that asymmetricKeyDetails → generateKeyPair → asymmetricKeyDetails is deepStrictEqual.

panva · 2021-09-04T14:06:20Z

I'd love to see a test that ensures that asymmetricKeyDetails → generateKeyPair → asymmetricKeyDetails is deepStrictEqual.

Can't do that because publicExponent in generate is a number but in key details is bigint. But other than publicExponent we already have tests that expect as much wrt. the RSASSA-PSS-params sequence params

nodejs-github-bot · 2021-09-04T17:05:38Z

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

nodejs-github-bot · 2021-09-05T06:41:14Z

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

panva · 2021-09-05T08:15:44Z

Landed in c6b0ae8

PR-URL: #39927 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: TODO

Notable changes: crypto: * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927 doc: * add Ayase-252 to collaborators (Qingyu Deng) #40078 fs: * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013 http: * (SEMVER-MINOR) limit requests per connection (Artur K) #40082 src: * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754 * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754 * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926 stream: * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067 PR-URL: #40175

panva added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 29, 2021

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 29, 2021

panva mentioned this pull request Aug 29, 2021

crypto: add RSA-PSS params to asymmetricKeyDetails #39851

Closed

panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch 3 times, most recently from 623d387 to 2916114 Compare August 29, 2021 18:27

panva marked this pull request as ready for review August 29, 2021 18:27

panva requested review from tniessen and jasnell August 29, 2021 18:28

panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch from 2916114 to a56bd8a Compare August 31, 2021 18:09

panva changed the title ~~crypto: rsa-pss keygen params aligned with asymmetricKeyDetails~~ crypto: add rsa-pss keygen parameters Aug 31, 2021

panva added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 31, 2021

panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch 2 times, most recently from 3b3a62f to 6cbaea2 Compare August 31, 2021 18:15

panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch from 6cbaea2 to a1e3fd9 Compare August 31, 2021 18:18

crypto: add rsa-pss keygen parameters

18c2c49

panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch from a1e3fd9 to 18c2c49 Compare August 31, 2021 18:21

panva added the deprecations Issues and PRs related to deprecations. label Aug 31, 2021

tniessen mentioned this pull request Aug 31, 2021

doc: add missing changes to generateKeyPair(Sync) #39963

Closed

panva removed the needs-ci PRs that need a full CI run. label Sep 1, 2021

This comment has been minimized.

Sign in to view

fixup! crypto: add rsa-pss keygen parameters

2e6ea20

This comment has been minimized.

Sign in to view

jasnell approved these changes Sep 3, 2021

View reviewed changes

jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 3, 2021

tniessen reviewed Sep 4, 2021

View reviewed changes

test/parallel/test-crypto-keygen.js Outdated Show resolved Hide resolved

fixup! crypto: add rsa-pss keygen parameters

aa53f49

This comment has been minimized.

Sign in to view

panva closed this Sep 5, 2021

panva added a commit that referenced this pull request Sep 5, 2021

crypto: add rsa-pss keygen parameters

c6b0ae8

PR-URL: #39927 Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen mentioned this pull request Sep 5, 2021

crypto: fix RSA-PSS default saltLength #39999

Closed

panva deleted the crypto-align-expose-rsa-pss-keygen-params branch September 5, 2021 18:58

targos pushed a commit that referenced this pull request Sep 7, 2021

crypto: add rsa-pss keygen parameters

4f1ec94

PR-URL: #39927 Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit that referenced this pull request Sep 10, 2021

crypto: add rsa-pss keygen parameters

fb226ff

PR-URL: #39927 Reviewed-By: James M Snell <jasnell@gmail.com>

BethGriggs mentioned this pull request Sep 21, 2021

v16.10.0 proposal #40175

Merged

1 task

panva mentioned this pull request Nov 28, 2022

crypto: runtime deprecate replaced rsa-pss keygen parameters #45653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: add rsa-pss keygen parameters #39927

crypto: add rsa-pss keygen parameters #39927

panva commented Aug 29, 2021 •

edited

Loading

panva commented Aug 29, 2021

tniessen commented Aug 31, 2021

panva commented Aug 31, 2021 •

edited

Loading

tniessen commented Aug 31, 2021

panva commented Aug 31, 2021

tniessen commented Aug 31, 2021

panva commented Aug 31, 2021

tniessen commented Aug 31, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Sep 1, 2021 •

edited by jasnell

Loading

tniessen commented Sep 4, 2021

panva commented Sep 4, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Sep 4, 2021

nodejs-github-bot commented Sep 5, 2021

panva commented Sep 5, 2021

crypto: add rsa-pss keygen parameters #39927

crypto: add rsa-pss keygen parameters #39927

Conversation

panva commented Aug 29, 2021 • edited Loading

panva commented Aug 29, 2021

tniessen commented Aug 31, 2021

panva commented Aug 31, 2021 • edited Loading

tniessen commented Aug 31, 2021

panva commented Aug 31, 2021

tniessen commented Aug 31, 2021

panva commented Aug 31, 2021

tniessen commented Aug 31, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Sep 1, 2021 • edited by jasnell Loading

tniessen commented Sep 4, 2021

panva commented Sep 4, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Sep 4, 2021

nodejs-github-bot commented Sep 5, 2021

panva commented Sep 5, 2021

panva commented Aug 29, 2021 •

edited

Loading

panva commented Aug 31, 2021 •

edited

Loading

tniessen commented Aug 31, 2021 •

edited

Loading

nodejs-github-bot commented Sep 1, 2021 •

edited by jasnell

Loading

panva commented Sep 4, 2021 •

edited

Loading