tls: add derCertToPemCert()

[kaplanmaxe]

When calling getPeerCertificate(), the cert is returned as a raw DER buffer. I often need to convert these to PEM format. This was modeled after Python's ssl.der_cert_to_pem_cert.

If concept is approved, I will add documentation.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[vsemozhetbyt]

CI: https://ci.nodejs.org/job/node-test-pull-request/14829/

[vsemozhetbyt]

It seems the test fails on Windows due to EOL difference.

[bnoordhuis]

I'm -.5 on this PR. I can see how it's useful but it's also trivial.

The general guideline for Node.js core is "if it can be done outside core, it should be done outside core" and this falls squarely in that category.

[bnoordhuis]

This is a horribly inefficient way of building a string. Try this:

const slices = ['-----BEGIN CERTIFICATE-----'];
for (var i = 0; i < chars.length; i += 64)
  slices.push(chars.slice(i, i + 64));
slices.push('-----END CERTIFICATE-----');
return slices.join('\n');

[mscdex]

You could also do:

return '-----BEGIN CERTIFICATE-----\n' +
       cert.toString('base64').replace(/.{64}/g, '$&\n') +
       (cert.length % 64 ? '\n' : '') +
       '-----END CERTIFICATE-----\n';

[kaplanmaxe]

@bnoordhuis Thanks a ton for reviewing the PR and giving your feedback.

While I have never contributed to node core, I have been a user for several years. Currently, I have a package that contains the same logic in this PR. Generally, when I need to do any DER to PEM conversion, security is critically important to the application. While the node / npm package system is really great, I really try and stay as dependency free as possible when security is such a big concern. Anyone who is using this method will most likely be working on an application where security is highly important as well, and I think many end users will be really appreciative of not having to add yet another node package to their application. In most cases, I do agree with you that if something can be done outside of core, it should, however I think in this context, it would be really nice to have this as part of node core.

I am currently working on getting a windows environment set up so I can fix the failing tests on windows, and will also update the function to be more efficient based on the suggestions if the concept is approved.

[kaplanmaxe]

@vsemozhetbyt Do you mind restarting the pipeline? I got a Windows environment setup and the tests are now passing locally for me. Thanks in advance.

Also, how can I go about getting more reviews on this PR?

[vsemozhetbyt]

CI: https://ci.nodejs.org/job/node-test-pull-request/15108/

[vsemozhetbyt]

Also, how can I go about getting more reviews on this PR?

Let's cc @nodejs/crypto.

[ryzokuken]

I'm with @bnoordhuis on this one. Does it need to be in core? It can be, sure, but if you can make a simple npm package for this instead, then that route should be preferred.

[ryzokuken]

On a quick glance, it seems fairly trivial to implement this without adding dependencies to your application? Why won't someone just write this function out?

[ryzokuken]

On a quick glance, it seems fairly trivial to implement this without adding dependencies to your application? Why won't someone just write this function out?

While I might understand the case of you not wanting to add functions to your projects, trying not to repeat yourself, why won't you just make an npm package for this? You shouldn't be concerned about breakage since if a particular version works for your case, as long as you don't update it explicitly, it will keep working.

[kaplanmaxe]

@ryzokuken Thanks for the review.

While I might understand the case of you not wanting to add functions to your projects, trying not to repeat yourself, why won't you just make an npm package for this?

There is no doubt the logic here is trivial and the function can simply be added to any project. However making just another npm package for this is something I am trying to avoid. I do have an NPM package that currently handles this logic for me. However, I personally do not agree that individuals should be encouraged to simply add packages for every little piece of logic when security is such a high concern. Like I mentioned in my previous comment, I think the npm package ecosystem is really great, however there are some serious security concerns with simply adding packages at will. If this didn't relate to security, I wouldn't be submitting a PR to core here. While I do agree that anything that can be done outside of core should be done outside of core, I'm not sure that's a good standard to follow when dealing with security.

Quoting my previous comment:

While the node / npm package system is really great, I really try and stay as dependency free as possible when security is such a big concern. Anyone who is using this method will most likely be working on an application where security is highly important as well, and I think many end users will be really appreciative of not having to add yet another node package to their application. In most cases, I do agree with you that if something can be done outside of core, it should, however I think in this context, it would be really nice to have this as part of node core.

[bnoordhuis]

"It's about security" is not a strong argument, IMO. It's a simple binary-to-text converter, not some subtle cryptography that only a handful of people have the expertise to vet.

[ryzokuken]

@kaplanmaxe While I appreciate your effort, I'm still not 100% sold on the idea of adding this function to core, as I cannot justify to myself why we would need it in here in the first place. It's a simple utility function, It concerns formats so opinions come into the view, and it makes the slope a tad slippery, allowing more such functions to percolate into core, something which would make maintaining it extremely difficult, so I'd be -1 on this as well.

I'd rather not see this land only to get reverted in a day or two (which won't be pleasant), or have us maintain this function for posterity (something which can be entirely avoided).

[tniessen]

Sorry, I didn't notice this PR before. Some notes about the PR in its current state:

• This is oddly specific. Most APIs in core aim to be extensible and to cover all relevant aspects of a feature. What if someone wanted a PEM to DER conversion soon? We would have to add another API for such a trivial task.
• Almost any package you require can completely destroy your application's security, whether it is TLS-related stuff or not. The argument "implement it in core to have fewer dependencies" would apply to all NPM packages.
• As @bnoordhuis pointed out, this is not a particularly sensitive API, it is easy to implement and incredibly difficult to get wrong to the point where it means a security risk.
• Other PEM/DER conversions are possible depending on the wrapped contents. I think if we are going to implement Exposing OpenSSL RSA KeyGen #15116, we should have a more general API to convert between key / certificate formats.

[ryzokuken]

Closing this because the collaborators seem to have reached consensus that this should probably not be added to core. @kaplanmaxe that said, I'd ask you to not be discouraged by this and continue helping us out. Looking forward to your next PR.