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

src: refine ncrypto more #57300

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 3, 2025

An eventual goal for ncrypto is to completely abstract away
details of working directly with openssl in order to make it
easier to work with multiple different openssl/boringssl versions.
As part of that we want to move away from direct reliance on
specific openssl APIs in the runtime and instead go through
the ncrypto abstractions. Not only does this help other
runtimes trying to be compatible with Node.js, but it helps
Node.js also by reducing the complexity of the crypto code
in Node.js itself.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 3, 2025
@jasnell jasnell requested review from anonrig and tniessen March 3, 2025 18:12
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 40 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (1347d42) to head (d111ff9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_cipher.cc 55.93% 13 Missing and 13 partials ⚠️
src/crypto/crypto_x509.cc 60.00% 0 Missing and 4 partials ⚠️
src/crypto/crypto_aes.cc 70.00% 0 Missing and 3 partials ⚠️
src/crypto/crypto_context.cc 76.92% 0 Missing and 3 partials ⚠️
src/crypto/crypto_rsa.cc 75.00% 0 Missing and 3 partials ⚠️
src/crypto/crypto_hmac.cc 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #57300   +/-   ##
=======================================
  Coverage   90.24%   90.24%           
=======================================
  Files         630      630           
  Lines      184920   184884   -36     
  Branches    36188    36187    -1     
=======================================
- Hits       166876   166848   -28     
+ Misses      11064    11056    -8     
  Partials     6980     6980           
Files with missing lines Coverage Δ
src/crypto/crypto_aes.h 33.33% <ø> (ø)
src/crypto/crypto_cipher.h 60.00% <100.00%> (+0.65%) ⬆️
src/crypto/crypto_hkdf.cc 65.27% <100.00%> (ø)
src/crypto/crypto_hkdf.h 33.33% <ø> (ø)
src/crypto/crypto_hmac.h 20.00% <ø> (ø)
src/crypto/crypto_pbkdf2.cc 68.11% <100.00%> (ø)
src/crypto/crypto_pbkdf2.h 100.00% <ø> (ø)
src/crypto/crypto_rsa.h 52.94% <100.00%> (-2.62%) ⬇️
src/crypto/crypto_sig.cc 70.96% <100.00%> (ø)
src/crypto/crypto_sig.h 63.63% <ø> (-3.04%) ⬇️
... and 7 more

... and 28 files with indirect coverage changes

@jasnell jasnell force-pushed the jasnell/more-ncrypto-more-fun branch from 0c51603 to c5839c9 Compare March 3, 2025 19:39
@nodejs-github-bot

This comment was marked as outdated.

jasnell added 3 commits March 3, 2025 12:20
An eventual goal for ncrypto is to completely abstract away
details of working directly with openssl in order to make it
easier to work with multiple different openssl/boringssl versions.
As part of that we want to move away from direct reliance on
specific openssl APIs in the runtime and instead go through
the ncrypto abstractions. Not only does this help other
runtimes trying to be compatible with Node.js, but it helps
Node.js also by reducing the complexity of the crypto code
in Node.js itself.
@jasnell jasnell force-pushed the jasnell/more-ncrypto-more-fun branch from c5839c9 to ec43618 Compare March 3, 2025 20:20
@jasnell jasnell force-pushed the jasnell/more-ncrypto-more-fun branch from ec43618 to d111ff9 Compare March 3, 2025 20:25
@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member Author

jasnell commented Mar 3, 2025

@nodejs/build ... getting a weird error with the node-test-linter CI job... https://ci.nodejs.org/job/node-test-linter/59226/

12:26:41 added 168 packages, and audited 169 packages in 4s
12:26:41 
12:26:41 40 packages are looking for funding
12:26:41   run `npm fund` for details
12:26:41 
12:26:41 found 0 vulnerabilities
12:26:41 Running JS linter...
12:26:41 
12:26:41 Oops! Something went wrong! :(
12:26:41 
12:26:41 ESLint: 9.21.0
12:26:41 
12:26:41 Error [ERR_REQUIRE_ESM]: require() of ES Module /home/iojs/build/workspace/node-test-linter/tools/eslint/node_modules/@stylistic/eslint-plugin-js/dist/index.js from /home/iojs/build/workspace/node-test-linter/tools/eslint/eslint.config_utils.mjs not supported.
12:26:41 Instead change the require of index.js in /home/iojs/build/workspace/node-test-linter/tools/eslint/eslint.config_utils.mjs to a dynamic import() which is available in all CommonJS modules.
12:26:41     at file:///home/iojs/build/workspace/node-test-linter/eslint.config.mjs?mtime=1741006171417:23:21
12:26:41     at async loadConfigFile (/home/iojs/build/workspace/node-test-linter/tools/eslint/node_modules/eslint/lib/config/config-loader.js:197:21)
12:26:41     at async ConfigLoader.calculateConfigArray (/home/iojs/build/workspace/node-test-linter/tools/eslint/node_modules/eslint/lib/config/config-loader.js:500:32)
12:26:41 make: *** [Makefile:1445: lint-js-ci] Error 2
12:26:41 + cat test-eslint.tap
12:26:41 + grep -v '^ok\|^TAP version 13\|^1\.\.'
12:26:41 + sed '/^\s*$/d'
12:26:41 cat: test-eslint.tap: No such file or directory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants