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
Prev Previous commit
crypto: addressed PR comments
  • Loading branch information
@twitharshil
twitharshil committed Nov 3, 2022
commit 00f666a7ae9213253e315b3bdacccfdf9278f1d7
5 changes: 3 additions & 2 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1534,8 +1534,8 @@ See `SSL_CERT_DIR` and `SSL_CERT_FILE`.
Node.js uses the trusted CA certificates present in the system store along with
the `--use-bundled-ca`, `--use-openssl-ca` options.

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

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

Expand Down Expand Up @@ -1880,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
73 changes: 39 additions & 34 deletions src/crypto/crypto_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,61 +199,66 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,

void ReadSystemStoreCertificates(
std::vector<std::string>* system_root_certificates) {
#ifdef _WIN32
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);
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;
PCCERT_CONTEXT certificate_context_ptr = nullptr;

std::vector<X509*> system_root_certificates_X509;

while ((pCtx = CertEnumCertificatesInStore(hStore, pCtx)) != nullptr) {
const DWORD cbSize = CertGetNameStringW(
pCtx, CERT_NAME_SIMPLE_DISPLAY_TYPE, 0, nullptr, nullptr, 0);
while ((certificate_context_ptr = CertEnumCertificatesInStore(
hStore, certificate_context_ptr)) != nullptr) {
Comment on lines +213 to +214
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.

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(cbSize, 0);
CHECK_GT(certificate_buffer_size, 0);

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

CHECK_GT(CertGetNameStringW(pCtx,
CHECK_GT(CertGetNameStringW(certificate_context_ptr,
CERT_NAME_SIMPLE_DISPLAY_TYPE,
0,
nullptr,
pszName.data(),
cbSize),
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;

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);

CHECK_NOT_NULL(certificate_dst_ptr);

auto cleanup =
OnScopeLeave([certificate_dst_ptr]() { free(certificate_dst_ptr); });
X509* cert =
d2i_X509(nullptr, &certificate_src_ptr, certificate_src_length);

const size_t written =
base64_encode(certificate_src_ptr, slen, certificate_dst_ptr, dlen);
CHECK_EQ(written, dlen);
system_root_certificates_X509.emplace_back(cert);
}

std::string base64_string_output(certificate_dst_ptr, dlen);
for (size_t i = 0; i < system_root_certificates_X509.size(); i++) {
int result = 0;

constexpr size_t distance = 72;
size_t pos = distance;
BIOPointer bio(BIO_new(BIO_s_mem()));
CHECK(bio);

while (pos < base64_string_output.size()) {
base64_string_output.insert(pos, "\n");
pos += distance + 1;
}
BUF_MEM* mem = nullptr;
result = PEM_write_bio_X509(bio.get(), system_root_certificates_X509[i]);

base64_string_output = "-----BEGIN CERTIFICATE-----\n" +
base64_string_output + "\n-----END CERTIFICATE-----";
BIO_get_mem_ptr(bio.get(), &mem);
std::string certificate_string_pem(mem->data, mem->length);
system_root_certificates->emplace_back(certificate_string_pem);

system_root_certificates->emplace_back(std::move(base64_string_output));
bio.reset();
Comment on lines +258 to +259
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
bio.reset();

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

}
#endif
}

X509_STORE* NewRootCertStore() {
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-cli-node-options.js
View file
Open in desktop
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
View file
Open in desktop
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
1 change: 1 addition & 0 deletions test/parallel/test-process-env-allowed-flags-are-documented.js
View file
Open in desktop
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