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

feat(NODE-5875): provide native crypto hooks for OpenSSL 3 #25

Merged
merged 20 commits into from
Jul 30, 2024

Conversation

addaleax
Copy link
Collaborator

@addaleax addaleax commented Jun 19, 2024

Description

$ ~/.nvm/versions/node/v20.14.0/bin/node test/benchmarks/bench.mjs && ~/.nvm/versions/node/v16.20.2/bin/node test/benchmarks/bench.mjs 

- cpu: AMD Ryzen 7 3700X 8-Core Processor
- node: v20.14.0
- cores: 16
- arch: x64
- os: linux (6.5.0-41-generic)
- ram: 31.254920959472656GB

BenchmarkRunner is using libmongocryptVersion=1.10.0, cryptoHooks=native_openssl, warmupSecs=2, testInSecs=57
samples taken:  2
samples taken:  57
Decrypting 1500 fields median ops/sec : 183
{
  info: { test_name: 'javascript_decrypt_1500' },
  created_at: 2024-06-19T14:36:01.591Z,
  completed_at: 2024-06-19T14:37:00.773Z,
  artifacts: [],
  metrics: [ { name: 'medianOpsPerSec', type: 'THROUGHPUT', value: 183 } ],
  sub_tests: []
}

- cpu: AMD Ryzen 7 3700X 8-Core Processor
- node: v16.20.2
- cores: 16
- arch: x64
- os: linux (6.5.0-41-generic)
- ram: 31.254920959472656GB

BenchmarkRunner is using libmongocryptVersion=1.10.0, cryptoHooks=js, warmupSecs=2, testInSecs=57
samples taken:  2
samples taken:  57
Decrypting 1500 fields median ops/sec : 35
{
  info: { test_name: 'javascript_decrypt_1500' },
  created_at: 2024-06-19T14:37:01.096Z,
  completed_at: 2024-06-19T14:38:00.979Z,
  artifacts: [],
  metrics: [ { name: 'medianOpsPerSec', type: 'THROUGHPUT', value: 35 } ],
  sub_tests: []
}

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Need for speed.

Release Highlight

Create native cryptoCallbacks 🔐

Node.js bundles OpenSSL, which means we can access the crypto APIs from C++ directly avoiding the need to define them in javascript and call back into the JS engine to perform encryption. Now, when running the bindings in a version of Node.js that bundles OpenSSL 3 (should correspond to Node.js 18+), the cryptoCallbacks option will be ignored and C++ defined callbacks will be used instead. This improves the performance of encryption dramatically, as much as 5x faster. 🚀

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken changed the title fix(NODE-XXXX): provide native crypto hooks for OpenSSL 3 fix(NODE-5875): provide native crypto hooks for OpenSSL 3 Jun 20, 2024
@nbbeeken nbbeeken force-pushed the 5455-openssl-native-hooks branch from 60a0a10 to 5875c3f Compare June 25, 2024 22:03
@nbbeeken nbbeeken force-pushed the 5455-openssl-native-hooks branch from 2e63577 to 41c17de Compare July 1, 2024 15:25
@nbbeeken nbbeeken changed the title fix(NODE-5875): provide native crypto hooks for OpenSSL 3 feat(NODE-5875): provide native crypto hooks for OpenSSL 3 Jul 1, 2024
@nbbeeken nbbeeken marked this pull request as ready for review July 1, 2024 16:58
@W-A-James W-A-James self-assigned this Jul 2, 2024
@W-A-James W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 2, 2024
src/index.ts Outdated Show resolved Hide resolved
@addaleax
Copy link
Collaborator Author

addaleax commented Jul 11, 2024

fyi, I've pushed 3886c37 because we just learned (the hard way 🙂) that Electron doesn't expose its native SSL bindings, absolutely makes sense for them not to do that but it's not something we had thought about before (plus a GYP variable so there's an easy way to skip this optimization if we need that at some point)

@addaleax addaleax force-pushed the 5455-openssl-native-hooks branch from 81e70d7 to f53a88f Compare July 11, 2024 11:29
@addaleax addaleax force-pushed the 5455-openssl-native-hooks branch from f53a88f to 3886c37 Compare July 11, 2024 11:32
Copy link
Collaborator

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Changes LGTM, why does it make sense that they wouldn't expose those? would it further add to the version matrix of ssls?

@addaleax
Copy link
Collaborator Author

@nbbeeken I think the biggest reason is that they're using BoringSSL because of Chromium instead of OpenSSL, and BoringSSL's README helpfully states:

Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don't recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.

@alcaeus alcaeus requested a review from baileympearson July 19, 2024 14:44
addon/openssl-crypto.cc Show resolved Hide resolved
addon/mongocrypt.cc Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
addon/openssl-crypto.cc Show resolved Hide resolved
addon/openssl-crypto.cc Show resolved Hide resolved
addon/openssl-crypto.cc Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 30, 2024
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

Fire

@nbbeeken nbbeeken merged commit 38f1be6 into main Jul 30, 2024
30 checks passed
@nbbeeken nbbeeken deleted the 5455-openssl-native-hooks branch July 30, 2024 18:14
baileympearson pushed a commit that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants