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.

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

src: exclude node_root_certs when use-def-ca-store #11939

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 20, 2017

When configuring node with --openssl-use-def-ca-store the root certs
from OpenSSL should be used and not the ones in src/node_root_certs.h.
I noticed that src/node_root_certs.h is still included even when
using --openssl-use-def-ca-store.

This commit adds check and does not include node_root_certs.h if
--openssl-use-def-ca-store is specified.

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

src

When configuring node with --openssl-use-def-ca-store the root certs
from OpenSSL should be used and not the ones in src/node_root_certs.h.
I noticed that src/node_root_certs.h is still included even when
using --openssl-use-def-ca-store.

This commit adds check and does not include node_root_certs.h if
--openssl-use-def-ca-store is specified.
@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 Mar 20, 2017
@danbev
Copy link
Contributor Author

danbev commented Mar 20, 2017

@joaocgreis
Copy link
Member

First linter job failed because of work in progress in the CI job. Relaunched: https://ci.nodejs.org/job/node-test-linter/7717/

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. and removed crypto Issues and PRs related to the crypto subsystem. labels Mar 20, 2017
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 20, 2017
@jasnell
Copy link
Member

jasnell commented Mar 20, 2017

Marking as semver-major defensively here. @nodejs/crypto thoughts?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but would like @nodejs/crypto folks to sign off

@bnoordhuis
Copy link
Member

Semver-major seems appropriate. Most people would probably consider it a bug fix but since it changes behavior in a way that is not backwards-compatible, semver-major seems right.

jasnell pushed a commit that referenced this pull request Mar 22, 2017
When configuring node with --openssl-use-def-ca-store the root certs
from OpenSSL should be used and not the ones in src/node_root_certs.h.
I noticed that src/node_root_certs.h is still included even when
using --openssl-use-def-ca-store.

This commit adds check and does not include node_root_certs.h if
--openssl-use-def-ca-store is specified.

PR-URL: #11939
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Landed in be98f26

@jasnell jasnell closed this Mar 22, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
@danbev danbev deleted the exclude-node-root-ca branch June 28, 2017 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants