tls: fix reference to certificate object subjectAltName in checkServerIdentity function

[bnielsen1965]

tls: fix reference to certificate object subjectAltName in checkServerIdentity function

The checkServerIdentify() function always fails with ERR_TLS_CERT_ALTNAME_INVALID
error due to a bad reference to the certificate object's subjectAltName
property. Changed the property reference from all lowercase to camelcase.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/net

[mcollina]

Can you please add a unit test?

[bnielsen1965]

Sorry, I'm new to the node source code. Turns out there were tests in place for this case, as well as others, but the tests were also using the lowercase of the certificate field "subjectAltName".

I updated the tests, documentation, and there was a property in env.h that I changed but I don't fully understand the purpose and need somebody who knows to verify.

[jasnell]

I don't think this is the right fix. The code clearly is written for the field to be the lowercase subjectaltname as verified by the tests. Where exactly is the issue? Is this just a documentation problem?

[bnielsen1965]

The issue was in real world use the checkServerIdentity() function fails on valid certificates because the crypto module uses camelcase for the certificate properties...

https://github.com/nodejs/node/blob/master/src/crypto/crypto_x509.cc#L64

[ShogunPanda]

Doing this would be a semver-major change, I guess.
Would it be possible to maintain both property (for backward compatibility) and?

[bnoordhuis]

I'd also argue for adding a subjectaltname -> subjectAltName, i.e.:

diff --git a/lib/internal/crypto/x509.js b/lib/internal/crypto/x509.js
index 386b41f3e4a..555d31c52ac 100644
--- a/lib/internal/crypto/x509.js
+++ b/lib/internal/crypto/x509.js
@@ -163,6 +163,10 @@ class X509Certificate extends JSTransferable {
     return value;
   }
 
+  get subjectaltname() {
+    return this.subjectAltName;
+  }
+
   get subjectAltName() {
     let value = this[kInternalState].get('subjectAltName');
     if (value === undefined) {

[tniessen]

There are many inconsistencies between the legacy certificate object and instances of X509Certificate. Some properties even exist on both objects but have different types. And that's made worse by bugs in X509Certificate.prototype.toLegacyObject() that effectively lead to three different certificate representations (before node 18 / #42124).

Both certificate representations are flawed. The legacy certificate objects allow simpler access to some fields (e.g., subject is an object) but then does not present those values unambiguously. The X509Certificate class uses non-standard string representations in some places, which are somewhat better, but not great either.

I've been trying to make various small improvements to those representations, but it's difficult without breaking things. Eventually, we'll probably want to migrate to X509Certificate everywhere, and find a way to work around its flaws.

[jasnell]

Eventually, we'll probably want to migrate to X509Certificate everywhere, and find a way to work around its flaws.

X509Certificate is still really quite new and unlikely to be extensively used yet. If we need to make breaking changes, let's do them now.

[tniessen]

I'd also argue for adding a subjectaltname -> subjectAltName, i.e.:

I don't think we should. It reinforces unusual naming conventions and, since other properties are already incompatible between legacy certificate objects and X509Certificate, it does not give us much benefit.

Instead of using checkServerIdentity with X509Certificate, consider using the newer X509Certificate.prototype.checkHost. (However, that also does non-spec compliant things without #41569. Hopefully, #41600 somewhat fixes it in node 18.)

[tniessen]

Thanks for your contribution @bnielsen1965. Unfortunately, I don't think this is the right way to fix the problem.

[bnielsen1965]

@tniessen going forward is a warning needed in the docs for the checkServerIdentity() function? The checkHost() method should take care of my needs but others may unknowingly fall into the same path where I was trapped.

In looking for a certificate validation method my search led me to checkServerIdentity() where the cert object argument then landed me on the X509Certificate object and I failed to noticed that I didn't really need the checkServerIdentify(). My issue is handled but just want to help others to avoid the same situation.

[tniessen]

going forward is a warning needed in the docs for the checkServerIdentity() function? The checkHost() method should take care of my needs but others may unknowingly fall into the same path where I was trapped.

@bnielsen1965 That's a good idea! Please take a look at #42495.