From adb88fcb4612041c08b69b0923fc1ed2b7a953b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Mon, 17 Jan 2022 14:35:47 +0000
Subject: [PATCH] crypto: support RFC 2818 compatible checkHost

The 'subject' option should not only accept the values 'always' and
'never' because neither is compatible with RFC 2818, i.e., HTTPS. This
change adds a third value 'default', which implies the behavior that
HTTPS mandates.

The new 'default' case matches the default behavior of OpenSSL for both
DNS names and email addresses.

Future Node.js versions should change the default option value from
'always' to 'default'.

Refs: https://github.com/nodejs/node/pull/36804

PR-URL: https://github.com/nodejs/node/pull/41569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 doc/api/crypto.md                   | 41 +++++++++++++++++++++++++++--
 lib/internal/crypto/x509.js         |  4 ++-
 test/parallel/test-x509-escaping.js | 18 +++++++++++++
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/doc/api/crypto.md b/doc/api/crypto.md
index 38ee1e79e9346c..23ba770af30d8d 100644
--- a/doc/api/crypto.md
+++ b/doc/api/crypto.md
@@ -2472,6 +2472,9 @@ added: v15.6.0
 <!-- YAML
 added: v15.6.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/41569
+    description: The subject option can now be set to `'default'`.
   - version: v16.14.1
     pr-url: https://github.com/nodejs/node/pull/41599
     description: The `wildcards`, `partialWildcards`, `multiLabelWildcards`, and
@@ -2481,21 +2484,42 @@ changes:
 
 * `email` {string}
 * `options` {Object}
-  * `subject` {string} `'always'` or `'never'`. **Default:** `'always'`.
+  * `subject` {string} `'default'`, `'always'`, or `'never'`.
+    **Default:** `'always'`.
+  * `wildcards` {boolean} **Default:** `true`.
+  * `partialWildcards` {boolean} **Default:** `true`.
+  * `multiLabelWildcards` {boolean} **Default:** `false`.
+  * `singleLabelSubdomains` {boolean} **Default:** `false`.
 * Returns: {string|undefined} Returns `email` if the certificate matches,
   `undefined` if it does not.
 
 Checks whether the certificate matches the given email address.
 
+If the `'subject'` option is set to `'always'` and if the subject alternative
+name extension either does not exist or does not contain a matching email
+address, the certificate subject is considered.
+
+If the `'subject'` option is set to `'default`', the certificate subject is only
+considered if the subject alternative name extension either does not exist or
+does not contain any email addresses.
+
+If the `'subject'` option is set to `'never'`, the certificate subject is never
+considered, even if the certificate contains no subject alternative names.
+
 ### `x509.checkHost(name[, options])`
 
 <!-- YAML
 added: v15.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/41569
+    description: The subject option can now be set to `'default'`.
 -->
 
 * `name` {string}
 * `options` {Object}
-  * `subject` {string} `'always'` or `'never'`. **Default:** `'always'`.
+  * `subject` {string} `'default'`, `'always'`, or `'never'`.
+    **Default:** `'always'`.
   * `wildcards` {boolean} **Default:** `true`.
   * `partialWildcards` {boolean} **Default:** `true`.
   * `multiLabelWildcards` {boolean} **Default:** `false`.
@@ -2511,6 +2535,18 @@ or it might contain wildcards (e.g., `*.example.com`). Because host name
 comparisons are case-insensitive, the returned subject name might also differ
 from the given `name` in capitalization.
 
+If the `'subject'` option is set to `'always'` and if the subject alternative
+name extension either does not exist or does not contain a matching DNS name,
+the certificate subject is considered.
+
+If the `'subject'` option is set to `'default'`, the certificate subject is only
+considered if the subject alternative name extension either does not exist or
+does not contain any DNS names. This behavior is consistent with [RFC 2818][]
+("HTTP Over TLS").
+
+If the `'subject'` option is set to `'never'`, the certificate subject is never
+considered, even if the certificate contains no subject alternative names.
+
 ### `x509.checkIP(ip)`
 
 <!-- YAML
@@ -5896,6 +5932,7 @@ See the [list of SSL OP Flags][] for details.
 [OpenSSL's SPKAC implementation]: https://www.openssl.org/docs/man1.1.0/apps/openssl-spkac.html
 [RFC 1421]: https://www.rfc-editor.org/rfc/rfc1421.txt
 [RFC 2412]: https://www.rfc-editor.org/rfc/rfc2412.txt
+[RFC 2818]: https://www.rfc-editor.org/rfc/rfc2818.txt
 [RFC 3526]: https://www.rfc-editor.org/rfc/rfc3526.txt
 [RFC 3610]: https://www.rfc-editor.org/rfc/rfc3610.txt
 [RFC 4055]: https://www.rfc-editor.org/rfc/rfc4055.txt
diff --git a/lib/internal/crypto/x509.js b/lib/internal/crypto/x509.js
index 738ab194ec62f7..cd20f6868da72c 100644
--- a/lib/internal/crypto/x509.js
+++ b/lib/internal/crypto/x509.js
@@ -65,7 +65,8 @@ function isX509Certificate(value) {
 function getFlags(options = {}) {
   validateObject(options, 'options');
   const {
-    subject = 'always',  // Can be 'always' or 'never'
+    // TODO(tniessen): change the default to 'default'
+    subject = 'always',  // Can be 'default', 'always', or 'never'
     wildcards = true,
     partialWildcards = true,
     multiLabelWildcards = false,
@@ -78,6 +79,7 @@ function getFlags(options = {}) {
   validateBoolean(multiLabelWildcards, 'options.multiLabelWildcards');
   validateBoolean(singleLabelSubdomains, 'options.singleLabelSubdomains');
   switch (subject) {
+    case 'default': /* Matches OpenSSL's default, no flags. */ break;
     case 'always': flags |= X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT; break;
     case 'never': flags |= X509_CHECK_FLAG_NEVER_CHECK_SUBJECT; break;
     default:
diff --git a/test/parallel/test-x509-escaping.js b/test/parallel/test-x509-escaping.js
index ba11dde79b0821..99418e4c0bf21c 100644
--- a/test/parallel/test-x509-escaping.js
+++ b/test/parallel/test-x509-escaping.js
@@ -424,6 +424,15 @@ const { hasOpenSSL3 } = common;
   assert.strictEqual(certX509.subject, `CN=${servername}`);
   assert.strictEqual(certX509.subjectAltName, 'DNS:evil.example.com');
 
+  // The newer X509Certificate API allows customizing this behavior:
+  assert.strictEqual(certX509.checkHost(servername), servername);
+  assert.strictEqual(certX509.checkHost(servername, { subject: 'default' }),
+                     undefined);
+  assert.strictEqual(certX509.checkHost(servername, { subject: 'always' }),
+                     servername);
+  assert.strictEqual(certX509.checkHost(servername, { subject: 'never' }),
+                     undefined);
+
   // Try connecting to a server that uses the self-signed certificate.
   const server = tls.createServer({ key, cert }, common.mustNotCall());
   server.listen(common.mustCall(() => {
@@ -454,6 +463,15 @@ const { hasOpenSSL3 } = common;
   assert.strictEqual(certX509.subject, `CN=${servername}`);
   assert.strictEqual(certX509.subjectAltName, 'IP Address:1.2.3.4');
 
+  // The newer X509Certificate API allows customizing this behavior:
+  assert.strictEqual(certX509.checkHost(servername), servername);
+  assert.strictEqual(certX509.checkHost(servername, { subject: 'default' }),
+                     servername);
+  assert.strictEqual(certX509.checkHost(servername, { subject: 'always' }),
+                     servername);
+  assert.strictEqual(certX509.checkHost(servername, { subject: 'never' }),
+                     undefined);
+
   // Connect to a server that uses the self-signed certificate.
   const server = tls.createServer({ key, cert }, common.mustCall((socket) => {
     socket.destroy();