FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

src: mark/pop OpenSSL errors in NewRootCertStore - #35514

Closed
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:crypto_system_cert_path
ConversationCommits (3)ChecksFiles changed
Closed

src: mark/pop OpenSSL errors in NewRootCertStore#35514
danbev wants to merge 3 commits into
nodejs:masterfrom
danbev:crypto_system_cert_path

Conversation

danbev commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

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 commented Oct 6, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

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

nodejs-github-bot commented Oct 6, 2020

Copy link
Copy Markdown
Collaborator

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

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

nodejs-github-bot commented Oct 6, 2020

Copy link
Copy Markdown
Collaborator

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

Trott commented Oct 6, 2020

Copy link
Copy Markdown
Member

@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 commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

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 commented Oct 6, 2020

Copy link
Copy Markdown
Contributor Author

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 commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

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 commented Oct 7, 2020

Copy link
Copy Markdown
Collaborator

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

danbev commented Oct 7, 2020

Copy link
Copy Markdown
Contributor Author

Does it need to remain static?

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

src: mark/pop OpenSSL errors in NewRootCertStore
0ab510b 
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
danbev force-pushed the crypto_system_cert_path branch from d806af4 to 0ab510b Compare October 13, 2020 12:20

jasnell commented Oct 15, 2020

Copy link
Copy Markdown
Member

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

danbev commented Oct 17, 2020

Copy link
Copy Markdown
Contributor Author

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

nodejs-github-bot commented Oct 18, 2020

Copy link
Copy Markdown
Collaborator

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

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

codecov-io commented Oct 18, 2020

Copy link
Copy Markdown

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 commented Oct 18, 2020

Copy link
Copy Markdown
Collaborator

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

danbev commented Oct 19, 2020

Copy link
Copy Markdown
Contributor Author

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

nodejs-github-bot commented Oct 19, 2020

Copy link
Copy Markdown
Collaborator

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

danbev added a commit that referenced this pull request Oct 21, 2020
src: mark/pop OpenSSL errors in NewRootCertStore
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: #35514
Fixes: #35456
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

danbev commented Oct 21, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 610c68c.

danbev closed this Oct 21, 2020
danbev deleted the crypto_system_cert_path branch October 21, 2020 12:36
BethGriggs pushed a commit that referenced this pull request Oct 21, 2020
src: mark/pop OpenSSL errors in NewRootCertStore
6a7a61b 
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: #35514
Fixes: #35456
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs mentioned this pull request Oct 21, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unexpected openssl error with Node 12.18.0 custom build

8 participants

Back | FazBrowse Home | New Git URL