src: mark/pop OpenSSL errors in NewRootCertStore by danbev · Pull Request #35514 · nodejs/node

danbev · 2020-10-06T11:45:44Z

This commit sets the OpenSSL error mark before calling
X509_STORE_load_locations and pops the error mark afterwards.

The motivation for this is that it is possible that
X509_STORE_load_locations can produce errors if the configuration
option --openssl-system-ca-path file does not exist. Later if a
different function is called which calls an OpenSSL function it could
fail because these errors might still be on the OpenSSL error queue.

Currently, all functions that call NewRootCertStore clear the
OpenSSL error queue upon returning, but this was not the case for
example in v12.18.0.

Fixes: #35456

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2020-10-06T11:45:48Z

Review requested:

@nodejs/crypto

nodejs-github-bot · 2020-10-06T12:09:47Z

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

nodejs-github-bot · 2020-10-06T12:26:15Z

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

Trott · 2020-10-06T13:59:14Z

@nodejs/platform-smartos This isn't compiling successfully on SmartOS (and there's now a commit to add code to skip SmartOS). Thoughts? You can see the compilation failure in either of the first two CI runs above.

cjihrig · 2020-10-06T14:09:30Z

I haven't tried to debug this locally, but would #include'ing the .h file instead of the .cc file in the test make any difference?

danbev · 2020-10-06T16:45:18Z

I haven't tried to debug this locally, but would #include'ing the .h file instead of the .cc file in the test make any difference?

The function node::crypto::NewRootCertStore is static so including the header would not provide access to it unfortunately.

cjihrig · 2020-10-06T16:50:52Z

The function node::crypto::NewRootCertStore is static so including the header would not provide access to it unfortunately.

Does it need to remain static?

nodejs-github-bot · 2020-10-07T02:58:40Z

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

danbev · 2020-10-07T02:59:16Z

Does it need to remain static?

Good question, I'm not really sure if it does or not.

This commit sets the OpenSSL error mark before calling X509_STORE_load_locations and pops the error mark afterwards. The motivation for this is that it is possible that X509_STORE_load_locations can produce errors if the configuration option --openssl-system-ca-path file does not exist. Later if a different function is called which calls an OpenSSL function it could fail because these errors might still be on the OpenSSL error stack. Currently, all functions that call NewRootCertStore clear the OpenSSL error queue upon returning, but this was not the case for example in v12.18.0. Fixes: nodejs#35456

jasnell · 2020-10-15T04:18:32Z

I think it would be fine to move the declaration into the header file

danbev · 2020-10-17T08:40:06Z

@cjihrig @jasnell Sounds good to me, I'll move it into the header. Thanks

nodejs-github-bot · 2020-10-18T10:19:18Z

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

codecov-io · 2020-10-18T11:46:11Z

Codecov Report

Merging #35514 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

@@ Coverage Diff @@## master #35514 +/- ##
==========================================
- Coverage 96.87% 96.40% -0.48% 
==========================================
Files 212 220 +8 Lines 69641 73681 +4040 ==========================================
+ Hits 67466 71031 +3565 - Misses 2175 2650 +475

Impacted Files	Coverage Δ
lib/internal/crypto/scrypt.js	`28.33% <0.00%> (-71.67%)`	⬇️
lib/internal/crypto/diffiehellman.js	`92.47% <0.00%> (-6.50%)`	⬇️
lib/internal/crypto/keys.js	`94.99% <0.00%> (-4.22%)`	⬇️
lib/internal/crypto/util.js	`96.79% <0.00%> (-3.21%)`	⬇️
lib/internal/crypto/sig.js	`97.54% <0.00%> (-2.46%)`	⬇️
lib/internal/source_map/prepare_stack_trace.js	`94.95% <0.00%> (-1.69%)`	⬇️
lib/internal/crypto/keygen.js	`99.20% <0.00%> (-0.49%)`	⬇️
lib/internal/modules/cjs/loader.js	`94.39% <0.00%> (-0.28%)`	⬇️
lib/_http_server.js	`98.34% <0.00%> (-0.19%)`	⬇️
lib/fs.js	`94.13% <0.00%> (-0.13%)`	⬇️
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcf708d...9117d4f. Read the comment docs.

nodejs-github-bot · 2020-10-18T13:31:06Z

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

danbev · 2020-10-19T09:01:41Z

Re-run of failing node-test-commit-linux-containered/ ✔️

nodejs-github-bot · 2020-10-19T09:17:12Z

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

This commit sets the OpenSSL error mark before calling X509_STORE_load_locations and pops the error mark afterwards. The motivation for this is that it is possible that X509_STORE_load_locations can produce errors if the configuration option --openssl-system-ca-path file does not exist. Later if a different function is called which calls an OpenSSL function it could fail because these errors might still be on the OpenSSL error stack. Currently, all functions that call NewRootCertStore clear the OpenSSL error queue upon returning, but this was not the case for example in v12.18.0. PR-URL: #35514Fixes: #35456 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danbev · 2020-10-21T12:35:41Z

Landed in 610c68c.

This commit sets the OpenSSL error mark before calling X509_STORE_load_locations and pops the error mark afterwards. The motivation for this is that it is possible that X509_STORE_load_locations can produce errors if the configuration option --openssl-system-ca-path file does not exist. Later if a different function is called which calls an OpenSSL function it could fail because these errors might still be on the OpenSSL error stack. Currently, all functions that call NewRootCertStore clear the OpenSSL error queue upon returning, but this was not the case for example in v12.18.0. PR-URL: #35514Fixes: #35456 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danbev added the backport-requested-v12.x label Oct 6, 2020

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 6, 2020

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020

Trott approved these changes Oct 6, 2020
View reviewed changes

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 6, 2020

addaleax approved these changes Oct 6, 2020
View reviewed changes

cjihrig approved these changes Oct 6, 2020
View reviewed changes

danbev force-pushed the crypto_system_cert_path branch from d806af4 to 0ab510bCompare October 13, 2020 12:20

danbev added 2 commits October 18, 2020 12:40

squash: move NewRootCertStore to crypto_context.h
f7fd2ec

squash: remove smartos ignore
9117d4f

danbev force-pushed the crypto_system_cert_path branch from 503f592 to 9117d4fCompare October 18, 2020 10:41

danbev closed this Oct 21, 2020

danbev deleted the crypto_system_cert_path branch October 21, 2020 12:36

BethGriggs mentioned this pull request Oct 21, 2020
v15.0.1 proposal #35736
Merged

targos added the dont-land-on-v14.x label May 16, 2021

Uh oh!

Uh oh!

Conversation

danbev commented Oct 6, 2020

Checklist

Uh oh!

nodejs-github-bot commented Oct 6, 2020

Uh oh!

nodejs-github-bot commented Oct 6, 2020

Uh oh!

nodejs-github-bot commented Oct 6, 2020

Uh oh!

Trott commented Oct 6, 2020

Uh oh!

cjihrig commented Oct 6, 2020

Uh oh!

danbev commented Oct 6, 2020

Uh oh!

cjihrig commented Oct 6, 2020

Uh oh!

nodejs-github-bot commented Oct 7, 2020

Uh oh!

danbev commented Oct 7, 2020

Uh oh!

jasnell commented Oct 15, 2020

Uh oh!

danbev commented Oct 17, 2020

Uh oh!

nodejs-github-bot commented Oct 18, 2020

Uh oh!

codecov-io commented Oct 18, 2020

Codecov Report

Uh oh!

nodejs-github-bot commented Oct 18, 2020

Uh oh!

danbev commented Oct 19, 2020

Uh oh!

nodejs-github-bot commented Oct 19, 2020

Uh oh!

danbev commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants