crypto: cleanup root certificates and skip PEM deserialization by joyeecheung · Pull Request #56999 · nodejs/node

joyeecheung · 2025-02-10T19:08:47Z

We do not actually need them in PEM format, so just pass them around as X509 direcrtly.
The cached global X509 structures were previously never cleaned up. Clean them up at process teardown.
Use function-local static to ensure thread-safety in initialization.
Add more comments about how the various options differ.

- We do not actually need them in PEM format, so just pass them around as X509 direcrtly. - The cached global X509 structures were previously never cleaned up. Clean them up at process teardown. - Use function-local static to ensure thread-safety in initialization. - Add more comments about how the various options differ.

nodejs-github-bot · 2025-02-10T19:08:53Z

Review requested:

@nodejs/crypto
@nodejs/startup

codecov · 2025-02-10T20:33:41Z

Codecov Report

Attention: Patch coverage is 63.63636% with 24 lines in your changes missing coverage. Please review.

Project coverage is 89.10%. Comparing base (6ba36b4) to head (a27b3cf).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_context.cc	63.07%	15 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56999      +/-   ##
==========================================
- Coverage   89.14%   89.10%   -0.04%     
==========================================
  Files         665      665              
  Lines      193174   193203      +29     
  Branches    37189    37226      +37     
==========================================
- Hits       172202   172151      -51     
- Misses      13735    13764      +29     
- Partials     7237     7288      +51

Files with missing lines	Coverage Δ
src/crypto/crypto_util.h	`76.13% <ø> (ø)`
src/node.cc	`73.57% <100.00%> (+0.03%)`	⬆️
src/crypto/crypto_context.cc	`68.02% <63.07%> (+0.52%)`	⬆️

... and 41 files with indirect coverage changes

joyeecheung · 2025-02-11T22:58:18Z

Fixed a typo and linter/formatter errors.

nodejs-github-bot · 2025-02-12T13:34:43Z

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

…ization

joyeecheung · 2025-02-12T20:15:58Z

Fixed without-ssl builds

nodejs-github-bot · 2025-02-12T20:28:18Z

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

src/crypto/crypto_context.cc

anonrig · 2025-02-12T21:53:29Z

src/crypto/crypto_context.cc

+  size_t bundled_root_cert_count = arraysize(root_certs);
+  for (size_t i = 0; i < bundled_root_cert_count; i++) {
+    X509* x509 = PEM_read_bio_X509(
+        NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])).get(),


Can we avoid strlen() from this function somehow?

I don't think so, not without changing the perl script that generates the root certificates. Note that this code is not new, it just reverts the copying added by #56599 (which still leads to something like strlen in the string constructor). It has been using strlen since Node.js v0.x days or probably ever since Node.js started implementing TLS..(so > 10 years)

anonrig · 2025-02-12T21:55:06Z

src/crypto/crypto_context.cc


 static std::string extra_root_certs_file;  // NOLINT(runtime/string)

+static std::atomic<bool> has_cached_bundled_root_certs{false};


Why do we need thread safety here? Wouldn't the bundled root certs be only cached once on the initial boot, or am I missing something here?

They are initialized when the root store is first used e.g. when a TLS connection is first made. Not when e.g. the program is started to read a file and does not need to use TLS at all. If multiple workers are starting TLS connection at the same time then there can be a race in initialization.

…eserialization

nodejs-github-bot · 2025-02-12T22:35:26Z

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

nodejs-github-bot · 2025-02-13T11:03:15Z

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

nodejs-github-bot · 2025-02-13T14:32:48Z

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

joyeecheung · 2025-02-13T15:33:14Z

CI is green. PTAL again, thanks @addaleax @anonrig

nodejs-github-bot · 2025-02-13T15:58:24Z

Landed in 79f96b6

- We do not actually need them in PEM format, so just pass them around as X509 direcrtly. - The cached global X509 structures were previously never cleaned up. Clean them up at process teardown. - Use function-local static to ensure thread-safety in initialization. - Add more comments about how the various options differ. PR-URL: #56999 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

- We do not actually need them in PEM format, so just pass them around as X509 direcrtly. - The cached global X509 structures were previously never cleaned up. Clean them up at process teardown. - Use function-local static to ensure thread-safety in initialization. - Add more comments about how the various options differ. PR-URL: nodejs#56999 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

- We do not actually need them in PEM format, so just pass them around as X509 direcrtly. - The cached global X509 structures were previously never cleaned up. Clean them up at process teardown. - Use function-local static to ensure thread-safety in initialization. - Add more comments about how the various options differ. PR-URL: #56999 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

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. needs-ci PRs that need a full CI run. labels Feb 10, 2025

addaleax approved these changes Feb 11, 2025

View reviewed changes

fixup! crypto: cleanup root certificates and skip PEM deserialization

eb5e214

This was referenced Feb 12, 2025

crypto: support --use-system-ca on non-Windows and non-macOS #57009

Merged

2025-02-13, Version 23.8.0 (Current) #57005

Merged

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025

fixup! fixup! crypto: cleanup root certificates and skip PEM deserial…

e6e3091

…ization

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025

anonrig reviewed Feb 12, 2025

View reviewed changes

fixup! fixup! fixup! crypto: cleanup root certificates and skip PEM d…

a27b3cf

…eserialization

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 12, 2025

anonrig approved these changes Feb 13, 2025

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 13, 2025

joyeecheung added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Feb 13, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 13, 2025

nodejs-github-bot merged commit 79f96b6 into nodejs:main Feb 13, 2025
65 checks passed

github-actions bot mentioned this pull request Feb 25, 2025

2025-02-26, Version 23.9.0 (Current) #57207

Merged

aduh95 mentioned this pull request Apr 11, 2025

2025-04-23, Version 22.15.0 'Jod' (LTS), @UlisesGascon prepared by @RafaelGSS #57840

Merged


		static std::string extra_root_certs_file; // NOLINT(runtime/string)

		static std::atomic<bool> has_cached_bundled_root_certs{false};

Uh oh!

Conversation

joyeecheung commented Feb 10, 2025

Uh oh!

nodejs-github-bot commented Feb 10, 2025

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joyeecheung commented Feb 11, 2025

Uh oh!

nodejs-github-bot commented Feb 12, 2025

Uh oh!

joyeecheung commented Feb 12, 2025

Uh oh!

nodejs-github-bot commented Feb 12, 2025

Uh oh!

Uh oh!

anonrig Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 12, 2025

Uh oh!

nodejs-github-bot commented Feb 13, 2025

Uh oh!

nodejs-github-bot commented Feb 13, 2025

Uh oh!

joyeecheung commented Feb 13, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 10, 2025 •

edited

Loading

joyeecheung Feb 12, 2025 •

edited

Loading