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 optional callback to crypto.diffieHellman #57274

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

panva
Copy link
Member

@panva panva commented Mar 2, 2025

This adds optional callback for the stateless crypto.diffieHellman method to allow for the crypto execution to happen in libuv's threadpool.

In the asynchronous case, the OpenSSL code/library/reason error decoration is missing (because it relies on retrieving details from OpenSSL's error queue, which does not work across threads), this is something we will have to refactor in a semver-major with a broader callback methods error handling reimagining (as I've discussed privately with @tniessen), this is likely a larger endeavor now that ncrypto is in the mix.

cc @jasnell

@panva panva added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 2, 2025
@panva panva requested review from jasnell and tniessen March 2, 2025 13:47
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@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 2, 2025
@panva panva force-pushed the diffiehellman-callback branch from 3e30c14 to dcc1f44 Compare March 2, 2025 14:25
@jasnell
Copy link
Member

jasnell commented Mar 2, 2025

Initial read through looks good. Will do a full review pass and sign off once it's not draft.

@panva panva force-pushed the diffiehellman-callback branch 2 times, most recently from 5d0f506 to fc80d87 Compare March 2, 2025 16:02
@panva panva changed the title crypto: add optional callback to crypto.diffiehellman crypto: add optional callback to crypto.diffieHellman Mar 2, 2025
@panva panva marked this pull request as ready for review March 2, 2025 16:02
@panva panva force-pushed the diffiehellman-callback branch from fc80d87 to e6d0554 Compare March 2, 2025 16:07
@nodejs-github-bot

This comment was marked as outdated.

@panva
Copy link
Member Author

panva commented Mar 2, 2025

Before anyone jumps the gun, despite CI possibly passing (soon-ish) - I'd like this to wait for @tniessen's review.

@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 2, 2025
@panva panva force-pushed the diffiehellman-callback branch from e6d0554 to e1f142b Compare March 2, 2025 16:38
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2025
@jasnell jasnell removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2025
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Mar 2, 2025

Codecov Report

Attention: Patch coverage is 85.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (6b0af17) to head (47f7784).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_dh.cc 64.28% 0 Missing and 5 partials ⚠️
lib/internal/crypto/diffiehellman.js 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57274      +/-   ##
==========================================
- Coverage   90.24%   90.24%   -0.01%     
==========================================
  Files         630      630              
  Lines      184908   184915       +7     
  Branches    36181    36197      +16     
==========================================
+ Hits       166874   166876       +2     
+ Misses      11061    11042      -19     
- Partials     6973     6997      +24     
Files with missing lines Coverage Δ
lib/internal/crypto/diffiehellman.js 98.74% <96.15%> (+1.14%) ⬆️
src/crypto/crypto_dh.cc 64.79% <64.28%> (+3.21%) ⬆️

... and 24 files with indirect coverage changes

@panva
Copy link
Member Author

panva commented Mar 2, 2025

Let me see if I can do more for coverage

@panva panva force-pushed the diffiehellman-callback branch from e1f142b to 374b548 Compare March 2, 2025 20:34
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2025
@nodejs-github-bot

This comment was marked as outdated.

@panva panva force-pushed the diffiehellman-callback branch from 374b548 to 71ff746 Compare March 2, 2025 20:57
@nodejs-github-bot

This comment was marked as outdated.

@panva panva force-pushed the diffiehellman-callback branch from 71ff746 to 47f7784 Compare March 2, 2025 21:14
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.

4 participants