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.

Sign up for GitHub

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

Jump to bottom

crypto: remove root_cert_store from node_crypto.h #13194

Closed
wants to merge 2 commits into from
Closed

crypto: remove root_cert_store from node_crypto.h #13194

wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 24, 2017

root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@danbev
crypto: remove root_cert_store from node_crypto.h
dae7163 
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels May 24, 2017
@danbev
Copy link
Contributor Author

danbev commented May 24, 2017

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

bnoordhuis
bnoordhuis approved these changes May 24, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Maybe make root_certs and extra_root_certs_file static while you are here.

@danbev
Copy link
Contributor Author

danbev commented May 24, 2017

Maybe make root_certs and extra_root_certs_file static while you are here.

Sounds good, I'll do that.

@danbev
Copy link
Contributor Author

danbev commented May 24, 2017

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

mhdawson
mhdawson approved these changes May 24, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

danbev added a commit to danbev/node that referenced this pull request May 26, 2017
@danbev
crypto: remove root_cert_store from node_crypto.h
49e91e2 
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

PR-URL: nodejs#13194
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented May 26, 2017

Landed in 49e91e2

@danbev danbev closed this May 26, 2017
@danbev danbev deleted the remove-extern-root_cert_store branch May 26, 2017 04:07
jasnell pushed a commit that referenced this pull request May 28, 2017
@danbev @jasnell
crypto: remove root_cert_store from node_crypto.h
7b97f07 
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

PR-URL: #13194
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@sam-github sam-github mentioned this pull request Jul 21, 2017
Closed
2 tasks
@sam-github
Copy link
Contributor

It will land clean if #12788 is landed, I think they both should be, they are minor low-risk cleanups, and landing them will reduce future conflicts.

sam-github pushed a commit that referenced this pull request Jul 21, 2017
@danbev @sam-github
crypto: remove root_cert_store from node_crypto.h
7b8fcf8 
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

PR-URL: #13194
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@sam-github
Copy link
Contributor

landed on v6.x-staging, @nodejs/backporting

MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
@danbev @MylesBorins
crypto: remove root_cert_store from node_crypto.h
f4691bb 
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

PR-URL: #13194
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 12, 2017
@danbev @MylesBorins
crypto: remove root_cert_store from node_crypto.h
5900ebe 
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

PR-URL: #13194
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
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.
8 participants
@danbev @MylesBorins @sam-github @bnoordhuis @jasnell @cjihrig @mhdawson @nodejs-github-bot