Skip to content

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

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

Closed
twitharshil wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
716aecf
crypto: added support for reading system store certificates in windows
twitharshil Oct 6, 2022
9d628a7
crypto: added documentation for the newer node option
twitharshil Oct 6, 2022
00f666a
crypto: addressed PR comments
twitharshil Nov 3, 2022
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
Next Next commit
crypto: added support for reading system store certificates in windows
  • Loading branch information
@twitharshil
twitharshil committed Nov 2, 2022
commit 716aecf7d76fa84c7cbb58217de7d8dee204ea9b
104 changes: 96 additions & 8 deletions src/crypto/crypto_context.cc
View file
Open in desktop
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,88 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,

} // namespace

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

@bnoordhuis bnoordhuis Nov 19, 2022

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?

Copy link
Copy Markdown
Contributor

@timja timja Jan 29, 2025

Choose a reason for hiding this comment

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

Worth taking a look at this change to openjdk and thinking about is the system store enough:
openjdk/jdk@5e5500c

https://bugs.openjdk.org/browse/JDK-6782021

There's also the Jetbrains implementation that accesses a number of stores and aggregates them:
https://github.com/JetBrains/jvm-native-trusted-roots/blob/trunk/src/main/java/org/jetbrains/nativecerts/win32/Crypt32ExtUtil.java#L30-L36

I've had confirmation that the jetbrains one doesn't work with intermediate certificates, awaiting confirmation with the OpenJDK one.

Copy link
Copy Markdown
Member

@joyeecheung joyeecheung Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To make matters a bit more complex - Windows has this feature of revoking certificates through the "disallowed" store, that Chromium implements, for example https://github.com/chromium/chromium/blob/a5cf86ae718b86764946713e7abae12f1fa42d08/net/cert/internal/trust_store_win.cc#L334

Though I feel that for an initial implementation, not supporting that is fine because apparently many runtimes do not support it either (also we do not respect that when using the bundled certificates, anyway)

CHECK_NE(hStore, NULLPTR);

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

PCCERT_CONTEXT pCtx = nullptr;

while ((pCtx = CertEnumCertificatesInStore(hStore, pCtx)) != nullptr) {
const DWORD cbSize = CertGetNameStringW(
pCtx, CERT_NAME_SIMPLE_DISPLAY_TYPE, 0, nullptr, nullptr, 0);

CHECK_GT(cbSize, 0);

std::vector<wchar_t> pszName(cbSize);
Comment thread
jasnell marked this conversation as resolved.
Outdated

CHECK_GT(CertGetNameStringW(pCtx,
CERT_NAME_SIMPLE_DISPLAY_TYPE,
0,
nullptr,
pszName.data(),
cbSize),
0);

const char* certificate_src_ptr =
reinterpret_cast<const char*>(pCtx->pbCertEncoded);
const size_t slen = pCtx->cbCertEncoded;
const size_t dlen = base64_encoded_size(slen);

char* certificate_dst_ptr = UncheckedMalloc(dlen);
Comment thread
jasnell marked this conversation as resolved.
Outdated

CHECK_NOT_NULL(certificate_dst_ptr);

auto cleanup =
OnScopeLeave([certificate_dst_ptr]() { free(certificate_dst_ptr); });

const size_t written =
base64_encode(certificate_src_ptr, slen, certificate_dst_ptr, dlen);
CHECK_EQ(written, dlen);

std::string base64_string_output(certificate_dst_ptr, dlen);

constexpr size_t distance = 72;
size_t pos = distance;

while (pos < base64_string_output.size()) {
base64_string_output.insert(pos, "\n");
pos += distance + 1;
}

base64_string_output = "-----BEGIN CERTIFICATE-----\n" +
base64_string_output + "\n-----END CERTIFICATE-----";
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Oct 10, 2022

Choose a reason for hiding this comment

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

This manual formatting of a certificate is kind of questionable. If you have the certificate in DER format, you can pass it to openssl directly:

X509Pointer cert(d2i_X509(nullptr, buf, len));
if (cert) {
  // ...
}

openssl has utility functions like X509_print_ex() for formatting it as PEM, which I guess you're doing so they show up in tls.rootCertificates?


system_root_certificates->emplace_back(std::move(base64_string_output));
}
}

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
Copy Markdown
Member

@bnoordhuis bnoordhuis Nov 19, 2022

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 +311,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
Copy Markdown
Member

@bnoordhuis bnoordhuis Nov 19, 2022

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
View file
Open in desktop
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
View file
Open in desktop
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