★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/39927Nouvelle recherche | Portail wanayoo
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

Jump to bottom

crypto: add rsa-pss keygen parameters #39927

Closed
wants to merge 6 commits into from
Closed

crypto: add rsa-pss keygen parameters #39927

wants to merge 6 commits into from

Conversation

@panva
Copy link
Member

@panva panva commented Aug 29, 2021 •
edited

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 panva mentioned this pull request Aug 29, 2021
Closed
@panva panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch 3 times, most recently from 623d387 to 2916114 Aug 29, 2021
@panva panva marked this pull request as ready for review Aug 29, 2021
@panva
Copy link
Member Author

@panva panva commented Aug 29, 2021

cc @nodejs/crypto

@panva panva requested review from tniessen and jasnell Aug 29, 2021
@tniessen
Copy link
Member

@tniessen tniessen commented Aug 31, 2021

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

@panva panva commented Aug 31, 2021 •
edited

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

@tniessen tniessen commented Aug 31, 2021

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

@panva panva commented Aug 31, 2021

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

@tniessen
Copy link
Member

@tniessen tniessen commented Aug 31, 2021

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

@panva panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch from 2916114 to a56bd8a Aug 31, 2021
@panva panva changed the title crypto: rsa-pss keygen params aligned with asymmetricKeyDetails crypto: add rsa-pss keygen parameters Aug 31, 2021
@panva panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch 2 times, most recently from 3b3a62f to 6cbaea2 Aug 31, 2021
@panva
Copy link
Member Author

@panva panva commented Aug 31, 2021

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

@panva panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch from 6cbaea2 to a1e3fd9 Aug 31, 2021
@panva
crypto: add rsa-pss keygen parameters
18c2c49
@panva panva force-pushed the crypto-align-expose-rsa-pss-keygen-params branch from a1e3fd9 to 18c2c49 Aug 31, 2021
@tniessen tniessen mentioned this pull request Aug 31, 2021
Closed
@tniessen
Copy link
Member

@tniessen tniessen commented Aug 31, 2021 •
edited

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.

@panva
fixup! crypto: add rsa-pss keygen parameters
8db702a
@panva panva removed the needs-ci label Sep 1, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@panva
fixup! crypto: add rsa-pss keygen parameters
2e6ea20
@nodejs-github-bot

This comment has been hidden.

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

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

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

jasnell
jasnell approved these changes Sep 3, 2021
View changes
tniessen
tniessen reviewed Sep 4, 2021
View changes
test/parallel/test-crypto-keygen.js Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

@tniessen tniessen commented Sep 4, 2021

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

@panva
fixup! crypto: add rsa-pss keygen parameters
aa53f49
@panva
Copy link
Member Author

@panva panva commented Sep 4, 2021 •
edited

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

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

@nodejs-github-bot nodejs-github-bot commented Sep 4, 2021

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Sep 5, 2021

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

@panva
Copy link
Member Author

@panva panva commented Sep 5, 2021

Landed in c6b0ae8

@panva panva closed this Sep 5, 2021
panva added a commit that referenced this issue Sep 5, 2021
@panva
crypto: add rsa-pss keygen parameters
c6b0ae8 
PR-URL: #39927
Reviewed-By: James M Snell <jasnell@gmail.com>
@tniessen tniessen mentioned this pull request Sep 5, 2021
Closed
@panva panva deleted the crypto-align-expose-rsa-pss-keygen-params branch Sep 5, 2021
targos added a commit that referenced this issue Sep 7, 2021
@panva @targos
crypto: add rsa-pss keygen parameters
4f1ec94 
PR-URL: #39927
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau added a commit that referenced this issue Sep 10, 2021
@panva @richardlau
crypto: add rsa-pss keygen parameters
fb226ff 
PR-URL: #39927
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs added a commit that referenced this issue Sep 21, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
45a042b 
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
BethGriggs added a commit that referenced this issue Sep 21, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
10b4fe1 
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
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
BethGriggs added a commit that referenced this issue Sep 22, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
aff2a0a 
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
BethGriggs added a commit that referenced this issue Sep 22, 2021
@BethGriggs
2021-09-22, Version 16.10.0 (Current)
8d5675e 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen

@jasnell jasnell

Assignees
No one assigned
Labels
author ready c++ crypto deprecations notable-change semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@panva @tniessen @nodejs-github-bot @jasnell