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

feat: added support for reading certificates from windows system store #44532

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1529,6 +1529,14 @@ environment variables.

See `SSL_CERT_DIR` and `SSL_CERT_FILE`.

### `--node-use-system-ca`

Node.js uses the trusted CA certificates present in the system store along with
the `--use-bundled-ca`, `--use-openssl-ca` options.

Only current user certificates are accessible using this method, not the local
machine store. This option is available to Windows only.
jasnell marked this conversation as resolved.
Show resolved Hide resolved

### `--use-largepages=mode`

<!-- YAML
Expand Down Expand Up @@ -1872,6 +1880,7 @@ Node.js options that are allowed are:
* `--no-global-search-paths`
* `--no-warnings`
* `--node-memory-debug`
* `--node-use-system-ca`
* `--openssl-config`
* `--openssl-legacy-provider`
* `--openssl-shared-config`
Expand Down
109 changes: 101 additions & 8 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
#include <openssl/engine.h>
#endif // !OPENSSL_NO_ENGINE

#ifdef _WIN32
#include <Windows.h>
#include <wincrypt.h>

#include "base64-inl.h"
#endif

namespace node {

using v8::Array;
Expand Down Expand Up @@ -190,18 +197,93 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,

} // namespace

void ReadSystemStoreCertificates(
std::vector<std::string>* system_root_certificates) {
#ifdef _WIN32
const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");
const HCERTSTORE hStore = CertOpenSystemStoreW(nullptr, L"ROOT");

Because the first arg is a pointer, right? How likely is it for this method to fail?

CHECK_NE(hStore, nullptr);

auto cleanup =
OnScopeLeave([hStore]() { CHECK_EQ(CertCloseStore(hStore, 0), TRUE); });
jasnell marked this conversation as resolved.
Show resolved Hide resolved

PCCERT_CONTEXT certificate_context_ptr = nullptr;

std::vector<X509*> system_root_certificates_X509;

while ((certificate_context_ptr = CertEnumCertificatesInStore(
hStore, certificate_context_ptr)) != nullptr) {
Comment on lines +213 to +214
Copy link
Member

Choose a reason for hiding this comment

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

Prefer something like this for readability reasons:

Suggested change
while ((certificate_context_ptr = CertEnumCertificatesInStore(
hStore, certificate_context_ptr)) != nullptr) {
for (;;) {
certificate_context_ptr =
CertEnumCertificatesInStore(hStore, certificate_context_ptr);
if (certificate_context_ptr == nullptr) break;

const DWORD certificate_buffer_size =
CertGetNameStringW(certificate_context_ptr,
CERT_NAME_SIMPLE_DISPLAY_TYPE,
0,
nullptr,
nullptr,
0);

CHECK_GT(certificate_buffer_size, 0);

std::vector<wchar_t> certificate_name(certificate_buffer_size);

CHECK_GT(CertGetNameStringW(certificate_context_ptr,
CERT_NAME_SIMPLE_DISPLAY_TYPE,
0,
nullptr,
certificate_name.data(),
certificate_buffer_size),
0);
const unsigned char* certificate_src_ptr =
reinterpret_cast<const unsigned char*>(
certificate_context_ptr->pbCertEncoded);
const size_t certificate_src_length =
certificate_context_ptr->cbCertEncoded;

X509* cert =
d2i_X509(nullptr, &certificate_src_ptr, certificate_src_length);

system_root_certificates_X509.emplace_back(cert);
}

for (size_t i = 0; i < system_root_certificates_X509.size(); i++) {
int result = 0;

BIOPointer bio(BIO_new(BIO_s_mem()));
CHECK(bio);

BUF_MEM* mem = nullptr;
result = PEM_write_bio_X509(bio.get(), system_root_certificates_X509[i]);

BIO_get_mem_ptr(bio.get(), &mem);
std::string certificate_string_pem(mem->data, mem->length);
system_root_certificates->emplace_back(certificate_string_pem);

bio.reset();
Comment on lines +258 to +259
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bio.reset();

Not necessary to reset manually, the destructor does that automatically when it goes out of scope.

}
#endif
}

X509_STORE* NewRootCertStore() {
static std::vector<X509*> root_certs_vector;
static Mutex root_certs_vector_mutex;
Mutex::ScopedLock lock(root_certs_vector_mutex);

if (root_certs_vector.empty() &&
per_process::cli_options->ssl_openssl_cert_store == false) {
std::vector<std::string> combined_root_certs;

for (size_t i = 0; i < arraysize(root_certs); i++) {
combined_root_certs.emplace_back(root_certs[i]);
}

if (per_process::cli_options->node_use_system_ca) {
ReadSystemStoreCertificates(&combined_root_certs);
}

for (size_t i = 0; i < combined_root_certs.size(); i++) {
X509* x509 =
PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i],
strlen(root_certs[i])).get(),
nullptr, // no re-use of X509 structure
PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].c_str(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].c_str(),
PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].data(),

combined_root_certs[i].length())
.get(),
nullptr, // no re-use of X509 structure
NoPasswordCallback,
nullptr); // no callback data

Expand Down Expand Up @@ -234,19 +316,30 @@ X509_STORE* NewRootCertStore() {

void GetRootCertificates(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Local<Value> result[arraysize(root_certs)];

std::vector<std::string> combined_root_certs;

for (size_t i = 0; i < arraysize(root_certs); i++) {
combined_root_certs.emplace_back(root_certs[i]);
}

if (per_process::cli_options->node_use_system_ca) {
ReadSystemStoreCertificates(&combined_root_certs);
}

std::vector<Local<Value>> result(combined_root_certs.size());

for (size_t i = 0; i < combined_root_certs.size(); i++) {
if (!String::NewFromOneByte(
env->isolate(),
reinterpret_cast<const uint8_t*>(root_certs[i]))
.ToLocal(&result[i])) {
env->isolate(),
reinterpret_cast<const uint8_t*>(combined_root_certs[i].c_str()))
Copy link
Member

Choose a reason for hiding this comment

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

Minor optimization:

Suggested change
reinterpret_cast<const uint8_t*>(combined_root_certs[i].c_str()))
reinterpret_cast<const uint8_t*>(combined_root_certs[i].data()),
v8::NewStringType::kNormal,
combined_root_certs[i].size())

.ToLocal(&result[i])) {
return;
}
}

args.GetReturnValue().Set(
Array::New(env->isolate(), result, arraysize(root_certs)));
Array::New(env->isolate(), result.data(), combined_root_certs.size()));
}

bool SecureContext::HasInstance(Environment* env, const Local<Value>& value) {
Expand Down
4 changes: 4 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,10 @@ PerProcessOptionsParser::PerProcessOptionsParser(
"use an alternative default TLS cipher list",
&PerProcessOptions::tls_cipher_list,
kAllowedInEnvvar);
AddOption("--node-use-system-ca",
"use system's store CA",
&PerProcessOptions::node_use_system_ca,
kAllowedInEnvvar);
AddOption("--use-openssl-ca",
"use OpenSSL's default CA store"
#if defined(NODE_OPENSSL_CERT_STORE)
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ class PerProcessOptions : public Options {
#else
bool ssl_openssl_cert_store = false;
#endif
bool node_use_system_ca = false;
bool use_openssl_ca = false;
bool use_bundled_ca = false;
bool enable_fips_crypto = false;
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-cli-node-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ if (common.hasCrypto) {
expectNoWorker('--use-bundled-ca', 'B\n');
if (!common.hasOpenSSL3)
expectNoWorker('--openssl-config=_ossl_cfg', 'B\n');
if (common.isWindows) {
expectNoWorker('--node-use-system-ca', 'B\n');
}
}

// V8 options
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-cli-node-print-help.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ function validateNodePrintHelp() {
{ compileConstant: HAVE_OPENSSL,
flags: [ '--openssl-config=...', '--tls-cipher-list=...',
'--use-bundled-ca', '--use-openssl-ca',
'--enable-fips', '--force-fips' ] },
'--enable-fips', '--force-fips',
'--node-use-system-ca' ] },
{ compileConstant: NODE_HAVE_I18N_SUPPORT,
flags: [ '--icu-data-dir=...', 'NODE_ICU_DATA' ] },
{ compileConstant: HAVE_INSPECTOR,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const conditionalOpts = [
common.hasOpenSSL3 ? '--openssl-shared-config' : '',
'--tls-cipher-list',
'--use-bundled-ca',
common.isWindows ? '--node-use-system-ca' : '',
'--use-openssl-ca',
'--secure-heap',
'--secure-heap-min',
Expand Down