Skip to content

Commit

Permalink
feat!: use OpenSSL instead of node-forge for DER-to-PEM conversion
Browse files Browse the repository at this point in the history
This commit removes node-forge as a dependency and the conversion
logic for X.509 certificates from DER to PEM format that we used it for,
and replaces that logic with direct calls to OpenSSL functions
(which are always available in Node.js).

The main downside of this approach is that it removes the ABI stability
guarantees we get from Node-API and instead makes us dependent on the
OpenSSL ABI as well. Currently, the functions used in this code are
compatible with OpenSSL 1.1.x and 3.x, which should be good enough
in the present and for at least a good number of years into the future.

The main upside of this approach is that it is significantly faster,
and should enable applications that use this package to get the
system certificate list to do so by default, rather than as an opt-in
feature.
  • Loading branch information
addaleax committed Jun 21, 2024
1 parent 7a9b0e1 commit 578f970
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 17 deletions.
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,17 @@ Valid options are:
to try. Default: `['CERT_SYSTEM_STORE_LOCAL_MACHINE', 'CERT_SYSTEM_STORE_CURRENT_USER']`.

This functions returns an array of PEM-formatted certificates.
(Note that this can be a fairly slow operation.)

## Testing

You need to import `testkeys\certificate.pfx` manually into your local
CA store in order for the tests to pass. Make sure to import that certificate
with the "exportable private key" option. The password for the file is `pass`.

## Compatibility

Current versions of this package use OpenSSL to perform a format conversion
from DER to PEM. This means that, unlike other Node-API addons, this addon
has a stronger dependency on the ABI exposed by Node.js, and in particular
may need to be updated once Node.js starts using OpenSSL 4.x (which is not
planned at the time of writing).
41 changes: 40 additions & 1 deletion binding.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
#define NOMINMAX // otherwise windows.h #defines min and max as macros
#include <limits>
#include <napi.h>
#include <windows.h>
#include <wincrypt.h>
#include <openssl/err.h>
#include <openssl/x509.h>
#include <openssl/pem.h>

using namespace Napi;

Expand Down Expand Up @@ -54,6 +59,13 @@ void ThrowWindowsError(Env env, const char* call) {
throw Error::New(env, buf);
}

void ThrowOpenSSLError(Env env, const char* call) {
std::string error = call;
error += " failed with: ";
error += ERR_error_string(ERR_get_error(), nullptr);
throw Error::New(env, error);
}

// Create a temporary certificate store, add 'cert' to it, and then
// export it (using 'password' for encryption).
Buffer<BYTE> CertToBuffer(Env env, PCCERT_CONTEXT cert, LPCWSTR password, DWORD export_flags) {
Expand All @@ -79,6 +91,33 @@ Buffer<BYTE> CertToBuffer(Env env, PCCERT_CONTEXT cert, LPCWSTR password, DWORD
return outbuf;
}

// Export a X.509 certificate (which does not include a key) as a PEM string.
String CertToPEMBuffer(Env env, PCCERT_CONTEXT cert) {
const unsigned char* pbCertEncoded = reinterpret_cast<const unsigned char*>(cert->pbCertEncoded);
X509* x509cert = d2i_X509(nullptr, &pbCertEncoded, cert->cbCertEncoded);
if (!x509cert) {
ThrowOpenSSLError(env, "d2i_X509()");
}
Cleanup cleanup_cert([&]() { X509_free(x509cert); });

BIO* bio = BIO_new(BIO_s_mem());
if (!bio) {
ThrowOpenSSLError(env, "BIO_new()");
}
Cleanup cleanup_bio([&]() { BIO_free(bio); });

if (!PEM_write_bio_X509(bio, x509cert)) {
ThrowOpenSSLError(env, "PEM_write_bio_X509()");
}

char* string_data = nullptr;
long string_length = BIO_get_mem_data(bio, &string_data);
if (!string_data || static_cast<uintmax_t>(string_length) > std::numeric_limits<size_t>::max()) {
ThrowOpenSSLError(env, "BIO_get_mem_data()");
}
return String::New(env, string_data, string_length);
}

class CertStoreHandle {
public:
CertStoreHandle(HCERTSTORE store) : store_(store) {}
Expand Down Expand Up @@ -136,7 +175,7 @@ Value ExportAllCertificates(const CallbackInfo& args) {
Array result = Array::New(args.Env());
size_t index = 0;
while (cert = sys_cs.next()) {
Buffer<BYTE> buf = CertToBuffer(args.Env(), cert, L"", 0);
String buf = CertToPEMBuffer(args.Env(), cert);
result[index++] = buf;
}
return result;
Expand Down
16 changes: 3 additions & 13 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
'use strict';
const {
exportCertificateAndKey,
exportAllCertificates,
storeTypes
} = require('bindings')('win_export_cert');
const { randomBytes } = require('crypto');
const forge = require('node-forge');
const util = require('util');

const DEFAULT_STORE_TYPE_LIST = ['CERT_SYSTEM_STORE_LOCAL_MACHINE', 'CERT_SYSTEM_STORE_CURRENT_USER'];
Expand All @@ -28,18 +28,8 @@ function exportSystemCertificates(opts = {}) {

const result = new Set();
for (const storeType of storeTypeList) {
const certs = exportAllCertificates(store || 'ROOT', storeType);
for (const cert of certs) {
// Convert PKCS#12 (aka .pfx) to PEM
const asn1 = forge.asn1.fromDer(cert.toString('latin1'));
const pkcs12 = forge.pkcs12.pkcs12FromAsn1(asn1, '');
const certBags = pkcs12.getBags({ bagType: forge.pki.oids.certBag })[forge.pki.oids.certBag];
for (const bag of certBags) {
if (bag.cert) {
const pem = forge.pki.certificateToPem(bag.cert);
result.add(pem);
}
}
for (const cert of exportAllCertificates(store || 'ROOT', storeType)) {
result.add(cert);
}
}

Expand Down
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
"gypfile": true,
"dependencies": {
"bindings": "^1.5.0",
"node-addon-api": "^3.1.0",
"node-forge": "^1.2.1"
"node-addon-api": "^3.1.0"
},
"license": "Apache-2.0",
"exports": {
Expand Down
1 change: 1 addition & 0 deletions test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
'use strict';
const tls = require('tls');
const fs = require('fs');
const assert = require('assert');
Expand Down

0 comments on commit 578f970

Please sign in to comment.