Skip to content
Open
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
52 changes: 46 additions & 6 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "node.h"
#include "node_buffer.h"
#include "node_options.h"
#include "node_process-inl.h"
#include "util.h"
#include "v8.h"

Expand Down Expand Up @@ -1084,35 +1085,74 @@ template <typename It>
MaybeLocal<Array> X509sToArrayOfStrings(Environment* env,
It first,
It last,
size_t size) {
size_t size,
bool skip_on_error = false) {
ClearErrorOnReturn clear_error_on_return;
EscapableHandleScope scope(env->isolate());

LocalVector<Value> result(env->isolate(), size);
size_t i = 0;
for (It cur = first; cur != last; ++cur, ++i) {
LocalVector<Value> result(env->isolate());
result.reserve(size);
size_t skipped = 0;
for (It cur = first; cur != last; ++cur) {
X509View view(*cur);
auto pem_bio = view.toPEM();
if (!pem_bio) {
if (skip_on_error) {
auto subject_bio = view.getSubject();
char* subject_data = nullptr;
std::string subject_str = "<unknown>";
if (subject_bio) {
auto subject_size =
BIO_get_mem_data(subject_bio.get(), &subject_data);
if (subject_size > 0 && subject_data) {
subject_str = std::string(subject_data, subject_size);
Comment on lines +1103 to +1108
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
std::string subject_str = "<unknown>";
if (subject_bio) {
auto subject_size =
BIO_get_mem_data(subject_bio.get(), &subject_data);
if (subject_size > 0 && subject_data) {
subject_str = std::string(subject_data, subject_size);
std::string_view subject_str = "<unknown>";
if (subject_bio) {
auto subject_size =
BIO_get_mem_data(subject_bio.get(), &subject_data);
if (subject_size > 0 && subject_data) {
subject_str = std::string_view(subject_data, subject_size);

No good reason to do an extra heap allocation here

}
}
per_process::Debug(DebugCategory::CRYPTO,
"Skipping system certificate with subject "
"'%s' because X509 to PEM conversion failed\n",
subject_str.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
subject_str.c_str());
subject_str);

skipped++;
continue;
}
ThrowCryptoError(env, ERR_get_error(), "X509 to PEM conversion");
return MaybeLocal<Array>();
}

char* pem_data = nullptr;
auto pem_size = BIO_get_mem_data(pem_bio.get(), &pem_data);
if (pem_size <= 0 || !pem_data) {
if (skip_on_error) {
per_process::Debug(DebugCategory::CRYPTO,
"Skipping a system certificate "
"because reading PEM data failed\n");
skipped++;
continue;
}
ThrowCryptoError(env, ERR_get_error(), "Reading PEM data");
return MaybeLocal<Array>();
}

Local<Value> str;
// PEM is base64-encoded, so it must be one-byte.
if (!String::NewFromOneByte(env->isolate(),
reinterpret_cast<uint8_t*>(pem_data),
v8::NewStringType::kNormal,
pem_size)
.ToLocal(&result[i])) {
.ToLocal(&str)) {
return MaybeLocal<Array>();
}
result.push_back(str);
}

if (skipped > 0) {
ProcessEmitWarning(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should make this an option to the API instead of unconditionally emitting a warning? So if the user uses tls.getCACertifcate, they can choose warn, ignore, throw (or even get a list of errors on the side) when there's an error in parsing these certificates. For NODE_USE_SYSTEM_CA/--use-system-ca, warn is a sensible behavior.

env,
"Skipped %zu system certificate(s) that could not be converted "
"to PEM format. Use NODE_DEBUG=crypto for details.",
skipped);
}

return scope.Escape(Array::New(env->isolate(), result.data(), result.size()));
}

Expand Down Expand Up @@ -1196,7 +1236,7 @@ void GetSystemCACertificates(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Local<Array> results;
std::vector<X509*>& certs = GetSystemStoreCACertificates();
if (X509sToArrayOfStrings(env, certs.begin(), certs.end(), certs.size())
if (X509sToArrayOfStrings(env, certs.begin(), certs.end(), certs.size(), true)
.ToLocal(&results)) {
args.GetReturnValue().Set(results);
}
Expand Down
Loading