tls requires a subject even when altNames are defined

[CriGoT]

When using TLS checkServerIdentity is using only subject to determine if a certificate is empty or not

node/lib/tls.js

Line 172 in a2ae089

} else if (subject) {

RFC 5280 allows for a certificate to have only altNames and an empty subject. The existing code already considers altName a priority and uses any supported altNames, if present, instead of the subject.

Although subject empty certificates are not common Windows uses them in their infrastructure so improving this check will simplify integration with systems using certificates with empty subjects.

[bnoordhuis]

The logic in tls.checkServerIdentity() looks okay to me: it only consults the Common Name when no SANs are present. It does however expect that cert.subject is an object.

I think the real bug is in tls.translatePeerCertificate(), it fails to turn .subject into an object when it is an empty string. Possible fix:

diff --git a/lib/_tls_common.js b/lib/_tls_common.js
index 56baf7b..0e06714 100644
--- a/lib/_tls_common.js
+++ b/lib/_tls_common.js
@@ -148,11 +148,11 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
   if (!c)
     return null;
 
-  if (c.issuer) c.issuer = tls.parseCertString(c.issuer);
-  if (c.issuerCertificate && c.issuerCertificate !== c) {
+  if (c.issuer != null) c.issuer = tls.parseCertString(c.issuer);
+  if (c.issuerCertificate != null && c.issuerCertificate !== c) {
     c.issuerCertificate = translatePeerCertificate(c.issuerCertificate);
   }
-  if (c.subject) c.subject = tls.parseCertString(c.subject);
+  if (c.subject != null) c.subject = tls.parseCertString(c.subject);
   if (c.infoAccess) {
     var info = c.infoAccess;
     c.infoAccess = {};

cc @indutny since you wrote that code.

[akdor1154]

is two months of no activity enough to warrant pinging this?

[indutny]

Absolutely good idea to ping us. Thank you 😉

[amellnik]

Just checking in after a further two months -- is there any plan to meet the RFC or should package owners plan on passing their own authentication function like auth0 did?

[bnoordhuis]

@indutny The question was more if you think #11771 (comment) is a proper fix.

[amellnik]

Looking at this further, this actually affects a significant number of packages, and I would recommend that it be treated as a bug. @bnoordhuis -- want to submit your fix as a pull request?

[bnoordhuis]

I was hoping to get feedback from @indutny but it seems it was not to be. I went ahead and opened #14447.

[jurjendijkstra]

Great! This issue has given me headaches. Since this is committed to version 9, which is not LTS yet, would you consider to merge it to version 8?

[jurjendijkstra]

Nevermind, I see it was already merged, sorry

[jasonmacgowan]

The fix @bnoordhuis supplied didn't seem to change the behavior to adhere to RFC 5280.

#22906 fixes the issue for me.

[jkinkead]

This issue should be re-opened, per the last comment.

[MylesBorins]

I'm going to reopen this until #22906 lands

[addaleax]

I'm going to reopen this until #22906 lands

That has happened now, closing again :)