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

crypto: Use system CAs instead of using bundled ones #11794

Closed
wants to merge 1 commit into from

Conversation

sgallagher
Copy link
Contributor

This is a backport of a patch included in 7.5.0

NodeJS can already use an external, shared OpenSSL library. This
library knows where to look for OS managed certificates. Allow
a compile-time option to use this CA store by default instead of
using bundled certificates.

In case when using bundled OpenSSL, the paths are also valid for
majority of Linux systems without additional intervention. If
this is not set, we can use SSL_CERT_DIR to point it to correct
location.

Fixes: #3159
PR-URL: #8334
Reviewed-By: Sam Roberts vieuxtech@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Fedor Indutny fedor.indutny@gmail.com

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

NodeJS can already use an external, shared OpenSSL library. This
library knows where to look for OS managed certificates. Allow
a compile-time option to use this CA store by default instead of
using bundled certificates.

In case when using bundled OpenSSL, the paths are also valid for
majority of Linux systems without additional intervention. If
this is not set, we can use SSL_CERT_DIR to point it to correct
location.

Fixes: nodejs#3159
PR-URL: nodejs#8334
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@nodejs-github-bot 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. v6.x labels Mar 10, 2017
@sam-github
Copy link
Contributor

@nodejs/lts backport of #8334, consider for 6.x please

@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 13, 2017
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

MylesBorins pushed a commit that referenced this pull request May 16, 2017
The pointer to std::vector is unnecessary, so replace it with standard
instance. Also, make the for() loop more readable by using actual type
instead of inferred - there is no readability benefit here from
obfuscating the type.

PR-URL: #8334
Backport-PR-URL: #11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2017
NodeJS can already use an external, shared OpenSSL library. This
library knows where to look for OS managed certificates. Allow
a compile-time option to use this CA store by default instead of
using bundled certificates.

In case when using bundled OpenSSL, the paths are also valid for
majority of Linux systems without additional intervention. If
this is not set, we can use SSL_CERT_DIR to point it to correct
location.

Fixes: #3159
PR-URL: #8334
Backport-PR-URL: #11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2017
PR-URL: #8334
Backport-PR-URL: #11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@MylesBorins
Copy link
Contributor

This backport only included one of the 3 commits from #8334

The commit that it did backport cherry-picked cleanly. It ended up being 33012e9 that required a bit of massaging to make it work.

@sam-github can you please review

Landed in 4a623fe, e3ef382, 708b978

@sam-github
Copy link
Contributor

@MylesBorins LGTM

MylesBorins pushed a commit that referenced this pull request May 18, 2017
The pointer to std::vector is unnecessary, so replace it with standard
instance. Also, make the for() loop more readable by using actual type
instead of inferred - there is no readability benefit here from
obfuscating the type.

PR-URL: #8334
Backport-PR-URL: #11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
NodeJS can already use an external, shared OpenSSL library. This
library knows where to look for OS managed certificates. Allow
a compile-time option to use this CA store by default instead of
using bundled certificates.

In case when using bundled OpenSSL, the paths are also valid for
majority of Linux systems without additional intervention. If
this is not set, we can use SSL_CERT_DIR to point it to correct
location.

Fixes: #3159
PR-URL: #8334
Backport-PR-URL: #11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
PR-URL: #8334
Backport-PR-URL: #11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
The pointer to std::vector is unnecessary, so replace it with standard
instance. Also, make the for() loop more readable by using actual type
instead of inferred - there is no readability benefit here from
obfuscating the type.

PR-URL: nodejs/node#8334
Backport-PR-URL: nodejs/node#11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
NodeJS can already use an external, shared OpenSSL library. This
library knows where to look for OS managed certificates. Allow
a compile-time option to use this CA store by default instead of
using bundled certificates.

In case when using bundled OpenSSL, the paths are also valid for
majority of Linux systems without additional intervention. If
this is not set, we can use SSL_CERT_DIR to point it to correct
location.

Fixes: nodejs/node#3159
PR-URL: nodejs/node#8334
Backport-PR-URL: nodejs/node#11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
PR-URL: nodejs/node#8334
Backport-PR-URL: nodejs/node#11794
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants