tls requires a subject even when altNames are defined

[jasonmacgowan]

Behavior described in #11771 is still true even though the issue is closed. This PR is to allow DNS and URI names, even when there is not a subject.

Checklist

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

[jasonmacgowan]

@bnoordhuis thoughts?

[addaleax]

@nodejs/crypto ?

[ryzokuken]

/cc @bnoordhuis is it expected to behave like this after #14473?

[addaleax]

@jasnell Since you approved this … is this ready to be landed? Is it semver-major?

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

[jasnell]

I'd prefer a look from @nodejs/crypto given the context. It looks fine for me but this particular bit of stuff always can stand more eyes.

[bnoordhuis]

Line 215 is going to throw a TypeError if it tries to read subject.CN when subject == null and altNames does not contain DNS:, IP: or URI: fields.

The logic should be something like } else if (subject || haveAltNames) { where haveAltNames is this:

consthaveAltNames=dnsNames.length>0||ips.length>0||uriNames.length>0;

I'd swap the branches of the if statement on line 214 so you can write if (haveAltNames) { - negations are to be avoided when possible.

[sam-github]

I would have done the opposite: defined const noAltNames because switching the branches textually from their previous location makes the diff larger, and obscures what is actually being changed, which is much smaller.

[jasonmacgowan]

@bnoordhuis please review. Thanks!

[bnoordhuis]

It would be better to move these back inside an if statement.

unfqdn() and splitHost() are somewhat expensive operations that the user shouldn't have to pay for when the other branches are taken.

[sam-github]

ping @jasonmacgowan

[jasonmacgowan]

@bnoordhuis take 3; please review. Thanks!

cc: @sam-github

[sam-github]

It surprised me that the check against a subjectAlternativeName didn't fall back on failure to the CN attribute of the subject, but I found that is required, at least it is here: https://tools.ietf.org/html/rfc6125#section-6.4.4

However, the domainComponent (if used, its likely uncommon) of the subject does look like it can be checked as a backup to the alt name: https://tools.ietf.org/html/rfc5280#section-4.2.1.6 But... we don't support that. Unrelated to this PR.

While I agree in general with Ben's point about negatives in conditionals, in this case I think it causes excessive churn, but I'll leave that to him to comment on.

So, just one request: can you please ensure that tests exist with a subject CN, proving that it is disregarded for matching the hostname when when an alt name exists, but only if the altname contains one of the 3 "domainish" attributes? I think that'd be at least two tests.

[adamrobertbacon]

Is it cheeky for me to bump this issue. I have been dabbling my feet into nodejs for the first time and I've just fallen foul of the issue myself. My internal CA provider has decided to start using certs with an empty subject line. Could find no way around it so I had to allow unauthorised connections for an LDAPS lookup. Not the end of the world but far from ideal. Keep up the good work!

[jasonmacgowan]

@adamrobertbacon check out auth0/ad-ldap-connector@1f4dd2b for a decent work around that doesn't require allowing all certs blindly.

[rajha-korithrien]

If it was cheeky for @adamrobertbacon to ping this issue back in April, I suppose it is even more cheeky for me to ping it in late May??

Thanks @jasonmacgowan for this code and a pointer to ad-ldap-connector. It will fit my needs of getting ldapjs to be a client of an Active Directory ldaps server with a default Microsoft self signed certifitate. Said certificate falls exactly into this boat, empty subject, DNS host name as a Subject Alternative Name.

Is there a thought as to what is still needed to get this merged? It is unexpected that an admittedly strange but (to my knowledge) standards conformant cert is not supported by the node.js TLS system.

Thanks!

[Monual]

Can we get this approved and merged? This bug is affecting our production environment.

[addaleax]

@sam-github You seem knowledgeable here, do you think you could write the tests you requested and push them here?

[nodejs-github-bot]

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

[sam-github]

I'd like to, but I just don't have the time. Consider my comment unblocking.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[addaleax]

Squashed and rebased to avoid a merge conflict.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[addaleax]

Landed in ff48009 🎉