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.

Sign up for GitHub

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

[v6.x backport] crypto: add sign/verify support for RSASSA-PSS #14376

Closed
wants to merge 6 commits into from
Closed

[v6.x backport] crypto: add sign/verify support for RSASSA-PSS #14376

wants to merge 6 commits into from

Conversation

tniessen
Copy link
Member

Backport of #11705 to v6.x @nodejs/lts

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. v6.x labels Jul 19, 2017
@tniessen tniessen requested a review from gibfahn July 19, 2017 20:49
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v6.x labels Jul 19, 2017
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 19, 2017
@gibfahn
Copy link
Member

gibfahn commented Jul 20, 2017

@tniessen could you pull in #12405 as well?

@tniessen
Copy link
Member Author

@gibfahn Do you want a separate PR, separate commit, or can I just adopt and force-push that one-character fix?

@tniessen
Copy link
Member Author

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

@sam-github
Copy link
Contributor

same PR, you can amend or cherry-pick, as you wish. I'd probably amend.

@tniessen tniessen force-pushed the backport-11705-to-v6.x branch from ef0394b to da2b6b8 Compare July 20, 2017 22:21
@gibfahn
Copy link
Member

gibfahn commented Jul 21, 2017

Separate commit (cherry-pick it onto your backport-11705-to-v6.x branch). Please don't amend, that makes working out what has been backported harder.

@tniessen tniessen force-pushed the backport-11705-to-v6.x branch from da2b6b8 to e1feaf2 Compare July 21, 2017 07:56
@tniessen
Copy link
Member Author

@gibfahn Done.

gibfahn
gibfahn approved these changes Jul 22, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

This LGTM, although I'd appreciate a quick look from @nodejs/crypto

@sam-github
Copy link
Contributor

LGTM, this basically lands clean, the conflicts are tiny, and just due to sign/verify being given names in master, and being anonymous in v6.x.

@sam-github
Copy link
Contributor

This would have landed clean if #8993 had been landed, I'll comment there.

@sam-github sam-github mentioned this pull request Jul 25, 2017
Closed
2 tasks
rus0000
rus0000 reviewed Jul 25, 2017
View reviewed changes
doc/api/crypto.md Outdated
which can be an RSA public key, a DSA public key, or an X.509 certificate,
or an object with one or more of the following properties:

* `key`: {string} - PEM encoded private key (required)
Copy link
Contributor

Choose a reason for hiding this comment

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

key: {string} - PEM encoded public key (required)

@MylesBorins MylesBorins force-pushed the v6.x-staging branch 5 times, most recently from 22fa484 to f9419c2 Compare August 16, 2017 18:42
@tniessen
Copy link
Member Author

@gibfahn Status on this? Should I rebase?

@gibfahn
Copy link
Member

gibfahn commented Aug 17, 2017

See #11705 (comment), this will have to wait till we review semver-minor backports before 6.12.0. I wouldn't bother rebasing yet.

You can subscribe to nodejs/Release#228 if you want to know when LTS will decide on backporting.

@MylesBorins MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from aaf4e13 to 31f572c Compare September 5, 2017 16:50
@MylesBorins
Copy link
Contributor

We've agreed to land this.
Can you please rebase

@sam-github
Copy link
Contributor

@MylesBorins #8993 will be needed as well to land clean

@sam-github
Copy link
Contributor

@tniessen do you want to take another pass at this now that #8993 (comment) has landed its dependent PR?

If not, ping me, and I will.

targos and others added 6 commits September 20, 2017 15:10
@targos @MylesBorins
path: fix normalize on directories with two dots
bc2115c 
PR-URL: nodejs#14107
Fixes: nodejs#14105
Reviewed-By: Refael Ackermann <refack@gmail.com>
@JacksonTian @MylesBorins
lib: clean up usage of threw
0a608ed 
Use try/catch to instead of threw.

PR-URL: nodejs#10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rsmarples @MylesBorins
build: add NetBSD support to opensslconf.h
60f25d0 
Simplify the BSD list by defining OPENSSL_BSD if using a matching
BSD platform.
Add NetBSD to the list and update documentation.

PR-URL: nodejs#14313
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@maclover7 @MylesBorins
doc: /s/SHASUM256/SHASUMS256
2166503 
As an example, `curl https://nodejs.org/dist/v8.4.0/SHASUM256.txt` will
return a 404 right now.

PR-URL: nodejs#15101
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos @MylesBorins
test: split path tests into multiple files
57beeb8 
Create one file for testing each function of the path module.
Keep general error tests and tests for constant properties in
test-path.js.

PR-URL: nodejs#15093
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@tniessen
crypto: add sign/verify support for RSASSA-PSS
61d1d20 
Adds support for the PSS padding scheme. Until now, the sign/verify
functions used the old EVP_Sign*/EVP_Verify* OpenSSL API, making it
impossible to change the padding scheme. Fixed by first computing the
message digest and then signing/verifying with a custom EVP_PKEY_CTX,
allowing us to specify options such as the padding scheme and the PSS
salt length.

Fixes: nodejs#1127
PR-URL: nodejs#11705
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@tniessen tniessen force-pushed the backport-11705-to-v6.x branch from e1feaf2 to 61d1d20 Compare September 20, 2017 21:53
@tniessen
Copy link
Member Author

Hope I did not miss anything. CI: https://ci.nodejs.org/job/node-test-pull-request/10172/

@MylesBorins MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from b811464 to 2c8fe97 Compare September 26, 2017 03:44
@tniessen tniessen mentioned this pull request Oct 7, 2017
Closed
2 tasks
@MylesBorins
Copy link
Contributor

landed in 1213f38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rus0000 rus0000 rus0000 left review comments

@gibfahn gibfahn gibfahn approved these changes

@jasnell jasnell jasnell approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. 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.