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: use system CAs instead of bundled ones #8334

Closed
wants to merge 3 commits into from
Closed

crypto: use system CAs instead of bundled ones #8334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0aa8ac7
crypto: do not use pointers to std::vector
AdamMajer Dec 21, 2016
d1e0228
crypto: Use system CAs instead of using bundled ones
AdamMajer Dec 21, 2016
1172288
crypto: ability to select cert store at runtime
AdamMajer Dec 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ parser.add_option('--openssl-fips',
dest='openssl_fips',
help='Build OpenSSL using FIPS canister .o file in supplied folder')

parser.add_option('--openssl-use-def-ca-store',
action='store_true',
dest='use_openssl_ca_store',
help='Use OpenSSL supplied CA store instead of compiled-in Mozilla CA copy.')

shared_optgroup.add_option('--shared-http-parser',
action='store_true',
dest='shared_http_parser',
Expand Down Expand Up @@ -927,6 +932,8 @@ def configure_openssl(o):
o['variables']['node_use_openssl'] = b(not options.without_ssl)
o['variables']['node_shared_openssl'] = b(options.shared_openssl)
o['variables']['openssl_no_asm'] = 1 if options.openssl_no_asm else 0
if options.use_openssl_ca_store:
o['defines'] += ['NODE_OPENSSL_CERT_STORE']
if options.openssl_fips:
o['variables']['openssl_fips'] = options.openssl_fips
fips_dir = os.path.join(root_dir, 'deps', 'openssl', 'fips')
Expand Down
28 changes: 28 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,24 @@ Load an OpenSSL configuration file on startup. Among other uses, this can be
used to enable FIPS-compliant crypto if Node.js is built with
`./configure --openssl-fips`.

### `--use-openssl-ca`, `--use-bundled-ca`
<!-- YAML
added: REPLACEME
-->

Use OpenSSL's default CA store or use bundled Mozilla CA store as supplied by
current NodeJS version. The default store is selectable at build-time.

Using OpenSSL store allows for external modifications of the store. For most
Linux and BSD distributions, this store is maintained by the distribution
maintainers and system administrators. OpenSSL CA store location is dependent on
configuration of the OpenSSL library but this can be altered at runtime using
environmental variables.

The bundled CA store, as supplied by NodeJS, is a snapshot of Mozilla CA store
that is fixed at release time. It is identical on all supported platforms.

See `SSL_CERT_DIR` and `SSL_CERT_FILE`.

### `--icu-data-dir=file`
<!-- YAML
Expand Down Expand Up @@ -340,6 +358,16 @@ misformatted, but any errors are otherwise ignored.
Note that neither the well known nor extra certificates are used when the `ca`
options property is explicitly specified for a TLS or HTTPS client or server.

### `SSL_CERT_DIR=dir`

If `--use-openssl-ca` is enabled, this overrides and sets OpenSSL's directory
containing trusted certificates.

### `SSL_CERT_FILE=file`
Copy link
Member

Choose a reason for hiding this comment

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

One concern that I have with the use of the environment variables for this is that it is easily possible for a rogue module to overwrite the value for child processes. Obviously rogue modules running untrusted code is a completely different problem in and of itself but I'm not convinced that allowing the trust root to be modified in this way is something we should allow. I'm good with the command-line arguments because those are much more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell this is out of our control, the env vars come as part of the OpenSSl trust store feature. This env var is implemented by OpenSSL in https://github.com/nodejs/node/blob/master/deps/openssl/openssl/crypto/x509/by_file.c#L100, which also implies its an env var supported by pretty much every program that uses OpenSSL, unless they have their own built-in trust store (Node is unusual in this respect).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell basically what @sam-github says, this is just documenting OpenSSL's environment usage.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but this env var currently has no effect in Node.js, correct? This PR makes it so that this env var can be used? At the very least, we should document the potential risk of using it and point out that changing the value in process.env would have an impact on child processes.

Copy link
Contributor

@sam-github sam-github Jan 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(EDIT: sorry, I mistyped that). What is the potential risk of allowing the user to decide for themselves what CAs they trust? Should we document the potential risk of having the certs compiled in... where its impossible for them to be changed if your sec policy requires it, such as on CA compromise?

To emphasize that the behaviour may be inherited, perhaps a note such as below would be OK?

Note: Be aware that unless the child environment is explicitly set, this evironment variable will be inherited by any child processes, and if they use OpenSSL, it may cause them to trust the same CAs as node.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell would my proposed text in https://github.com/nodejs/node/pull/8334/files#r97408670 address your concerns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand these concerns at all. These env. variables already apply for non-node programs and will continue to apply. This change does nothing for that. And warning "unauthorized code can affect child processes"??

Copy link
Contributor

Choose a reason for hiding this comment

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

I know these vars are from OpenSSL, not just node, so do you, but our average user won't, so lets warn. Its easier to modify the PR to make everyone happy than to stall it longer. My suggested text doesn't include the phrase you quote.

Copy link
Member

Choose a reason for hiding this comment

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

This is better and it's good enough for me to land.

@AdamMajer ... as @sam-github mentions, the issue is about making sure the average user knows what the impact of the options are, even if they are not already familiar with OpenSSL.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AdamMajer Do you want to add the text, so I can land right away? I'm willing to add the text while landing, too, but since the commit will have your name on it I'd like an OK from you before adding anything.


If `--use-openssl-ca` is enabled, this overrides and sets OpenSSL's file
containing trusted certificates.

[emit_warning]: process.html#process_process_emitwarning_warning_name_ctor
[Buffer]: buffer.html#buffer_buffer
[debugger]: debugger.html
Expand Down
25 changes: 25 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,22 @@ Load an OpenSSL configuration file on startup. Among other uses, this can be
used to enable FIPS-compliant crypto if Node.js is built with
\fB./configure \-\-openssl\-fips\fR.

.TP
.BR \-\-use\-openssl\-ca,\-\-use\-bundled\-ca
Use OpenSSL's default CA store or use bundled Mozilla CA store as supplied by
current NodeJS version. The default store is selectable at build-time.

Using OpenSSL store allows for external modifications of the store. For most
Linux and BSD distributions, this store is maintained by the distribution
maintainers and system administrators. OpenSSL CA store location is dependent on
configuration of the OpenSSL library but this can be altered at runtime using
environmental variables.

The bundled CA store, as supplied by NodeJS, is a snapshot of Mozilla CA store
that is fixed at release time. It is identical on all supported platforms.

See \fBSSL_CERT_DIR\fR and \fBSSL_CERT_FILE\fR.

.TP
.BR \-\-icu\-data\-dir =\fIfile\fR
Specify ICU data load path. (overrides \fBNODE_ICU_DATA\fR)
Expand Down Expand Up @@ -207,6 +223,15 @@ when outputting to a TTY on platforms which support async stdio.
Setting this will void any guarantee that stdio will not be interleaved or
dropped at program exit. \fBAvoid use.\fR

.TP
.BR SSL_CERT_DIR = \fIdir\fR
If \fB\-\-use\-openssl\-ca\fR is enabled, this overrides and sets OpenSSL's directory
containing trusted certificates.

.TP
.BR SSL_CERT_FILE = \fIfile\fR
If \fB\-\-use\-openssl\-ca\fR is enabled, this overrides and sets OpenSSL's
file containing trusted certificates.

.SH BUGS
Bugs are tracked in GitHub Issues:
Expand Down
22 changes: 22 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,14 @@ static const char* icu_data_dir = nullptr;
bool no_deprecation = false;

#if HAVE_OPENSSL
// use OpenSSL's cert store instead of bundled certs
bool ssl_openssl_cert_store =
#if defined(NODE_OPENSSL_CERT_STORE)
true;
#else
false;
#endif

# if NODE_FIPS_MODE
// used by crypto module
bool enable_fips_crypto = false;
Expand Down Expand Up @@ -3502,6 +3510,16 @@ static void PrintHelp() {
#if HAVE_OPENSSL
" --tls-cipher-list=val use an alternative default TLS cipher "
"list\n"
" --use-bundled-ca use bundled CA store"
#if !defined(NODE_OPENSSL_CERT_STORE)
" (default)"
#endif
"\n"
" --use-openssl-ca use OpenSSL's default CA store"
#if defined(NODE_OPENSSL_CERT_STORE)
" (default)"
#endif
"\n"
#if NODE_FIPS_MODE
" --enable-fips enable FIPS crypto at startup\n"
" --force-fips force FIPS crypto (cannot be disabled)\n"
Expand Down Expand Up @@ -3672,6 +3690,10 @@ static void ParseArgs(int* argc,
#if HAVE_OPENSSL
} else if (strncmp(arg, "--tls-cipher-list=", 18) == 0) {
default_cipher_list = arg + 18;
} else if (strncmp(arg, "--use-openssl-ca", 16) == 0) {
ssl_openssl_cert_store = true;
} else if (strncmp(arg, "--use-bundled-ca", 16) == 0) {
ssl_openssl_cert_store = false;
#if NODE_FIPS_MODE
} else if (strcmp(arg, "--enable-fips") == 0) {
enable_fips_crypto = true;
Expand Down
5 changes: 4 additions & 1 deletion src/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,12 @@ typedef intptr_t ssize_t;
namespace node {

NODE_EXTERN extern bool no_deprecation;
#if HAVE_OPENSSL && NODE_FIPS_MODE
#if HAVE_OPENSSL
NODE_EXTERN extern bool ssl_openssl_cert_store;
# if NODE_FIPS_MODE
NODE_EXTERN extern bool enable_fips_crypto;
NODE_EXTERN extern bool force_fips_crypto;
# endif
#endif

NODE_EXTERN int Start(int argc, char *argv[]);
Expand Down
18 changes: 10 additions & 8 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ const char* const root_certs[] = {
std::string extra_root_certs_file; // NOLINT(runtime/string)

X509_STORE* root_cert_store;
std::vector<X509*>* root_certs_vector;
std::vector<X509*> root_certs_vector;

// Just to generate static methods
template class SSLWrap<TLSWrap>;
Expand Down Expand Up @@ -693,9 +693,7 @@ static int X509_up_ref(X509* cert) {


static X509_STORE* NewRootCertStore() {
if (!root_certs_vector) {
root_certs_vector = new std::vector<X509*>;

if (root_certs_vector.empty()) {
for (size_t i = 0; i < arraysize(root_certs); i++) {
BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
Expand All @@ -707,14 +705,18 @@ static X509_STORE* NewRootCertStore() {
return nullptr;
}

root_certs_vector->push_back(x509);
root_certs_vector.push_back(x509);
}
}

X509_STORE* store = X509_STORE_new();
for (auto& cert : *root_certs_vector) {
X509_up_ref(cert);
X509_STORE_add_cert(store, cert);
if (ssl_openssl_cert_store) {
X509_STORE_set_default_paths(store);
} else {
for (X509 *cert : root_certs_vector) {
X509_up_ref(cert);
X509_STORE_add_cert(store, cert);
}
}

return store;
Expand Down