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

crypto: reconcile oneshot sign/verify sync and async implementations #37816

Closed
wants to merge 1 commit into from
Closed

crypto: reconcile oneshot sign/verify sync and async implementations #37816

wants to merge 1 commit into from

Conversation

panva
Copy link
Member

@panva panva commented Mar 19, 2021 •
edited
Loading

This reconciles the crypto.sign and crypto.verify C++ implementation when running with or without a callback.

@panva panva added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 19, 2021
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 19, 2021
@panva panva mentioned this pull request Mar 19, 2021
Closed
@panva panva removed the wip Issues and PRs that are still a work in progress. label Mar 19, 2021
@panva panva marked this pull request as ready for review March 19, 2021 21:02
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@panva panva requested a review from jasnell March 19, 2021 21:04
@panva panva removed the needs-ci PRs that need a full CI run. label Mar 19, 2021
@panva
Copy link
Member Author

panva commented Mar 19, 2021

cc @nodejs/crypto

@panva panva requested a review from tniessen March 19, 2021 21:12
@panva panva removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 19, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 20, 2021 •
edited by panva
Loading

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

💚

@mscdex

This comment has been minimized.

Sign in to view
@panva

This comment has been minimized.

Sign in to view
@mscdex

This comment has been minimized.

Sign in to view
@panva panva marked this pull request as draft March 20, 2021 21:37
@panva panva added the wip Issues and PRs that are still a work in progress. label Mar 20, 2021
@panva panva force-pushed the reconsile-oneshot branch 2 times, most recently from 145ce72 to c1b1f4d Compare March 21, 2021 10:04
@panva

This comment has been minimized.

Sign in to view
@panva

This comment has been minimized.

Sign in to view
@panva

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@tniessen
Copy link
Member

@panva @danbev Re thread-safety:

Thinking further...

  • If there really is no way to use EVP_PKEYs from multiple threads, then why do they implement thread-safe reference counters?
  • Assuming there really is no way to use EVP_PKEYs from multiple threads, then we should be able to clone EVP_PKEY instances before using them to prevent mutexes from negatively affecting performance.

@danbev
Copy link
Contributor

danbev commented Mar 24, 2021

I'm aware of these and I saw that evp_pkey_downgrade was recently replaced. But even with it gone I think we might put ourselves in the same situation we have now where we are using objects that are not guaranteed to be thread safe.

I think the suggestions here are good and worth pursuing. What I would like is we get the OpenSSL 3.0 CI job in place before we start this work. It was time consuming to get all the tests to pass against OpenSSL 3.0 and the threading issue was only one of them. At the moment any changes to crypto could break the tests and I have to go chasing them down afterwards.

@panva
Copy link
Member Author

panva commented Mar 24, 2021 •
edited
Loading

@danbev are you tracking the places where parallel performance may have been affected by these openssl3 related changes?

If not we have to look at all crypto async APIs (that's for sure this one here and all of webcrypto) and bench them with the same keyobject vs unique keyobjects, before and after the changes were put in place and figure out a way forward if performance was impacted like this API here.

Ideally this would've been done before the changes landed in a release.

@danbev
Copy link
Contributor

danbev commented Mar 24, 2021

are you tracking the places where parallel performance may have been affected by these openssl3 related changes?

Anywhere an EVP_PKEY is used from multiple threads. There was an assumption that this was not required with versions prior to OpenSSL 3.0, but there was never such a guarantee from OpenSSL about this as far as I understand it.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@panva
Copy link
Member Author

panva commented Apr 7, 2021 •
edited
Loading

@danbev @tniessen I've added a parallel libuv threaded test and the only failure with openssl3 is the one from #38136. Can I assume the lock is therefore not necessary and this can proceed?

@nodejs-github-bot

This comment has been minimized.

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

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

jasnell
jasnell approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Excellent work

@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 8, 2021
mcollina
mcollina approved these changes Apr 8, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Apr 8, 2021

Why is this identified as semver-major?

@panva
Copy link
Member Author

panva commented Apr 8, 2021

Why is this identified as semver-major?

It isn't anymore. At first it looked like there were going to be minor differences.

panva added a commit that referenced this pull request Apr 8, 2021
@panva
crypto: reconcile oneshot sign/verify sync and async implementations
e8cb644 
PR-URL: #37816
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@panva
Copy link
Member Author

panva commented Apr 8, 2021

Landed in e8cb644

@panva panva closed this Apr 8, 2021
@panva panva deleted the reconsile-oneshot branch April 8, 2021 09:14
@BethGriggs BethGriggs mentioned this pull request Apr 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@mcollina mcollina mcollina approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@tniessen tniessen Awaiting requested review from tniessen

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@panva @nodejs-github-bot @mscdex @tniessen @danbev @mcollina @jasnell @targos