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/

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

+ 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


 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-actionsBot 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-actionsBot 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-actionsBot 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:mainFeb 13, 2025

github-actionsBot 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!

Uh oh!

Conversation

joyeecheung commented Feb 10, 2025

Uh oh!

nodejs-github-bot commented Feb 10, 2025

Uh oh!

codecovBot 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!

anonrigFeb 12, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheungFeb 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!

anonrigFeb 12, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheungFeb 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!

nodejs-github-bot commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecovBot commented Feb 10, 2025•
edited
Loading

joyeecheungFeb 12, 2025•
edited
Loading