From 76f3e8f63bb93a22e16f641859cbc10e9da15b7b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Wed, 19 Jan 2022 19:05:53 +0000
Subject: [PATCH] crypto: change default check(Host|Email) behavior

This changes the default behavior of the X509Certificate functions
checkHost and checkEmail to match the default behavior of OpenSSL's
X509_check_host and X509_check_email functions, respectively, which
is also what RFC 2818 mandates for HTTPS.

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

PR-URL: https://github.com/nodejs/node/pull/41600
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
---
 doc/api/crypto.md                   | 28 +++++++++++++++++-----------
 lib/internal/crypto/x509.js         |  3 +--
 test/parallel/test-x509-escaping.js |  2 +-
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/doc/api/crypto.md b/doc/api/crypto.md
index c4d1860ccb6633..4b1a54326295c7 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/41600
+    description: The subject option now defaults to `'default'`.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/41599
     description: The `wildcards`, `partialWildcards`, `multiLabelWildcards`, and
@@ -2485,20 +2488,20 @@ changes:
 * `email` {string}
 * `options` {Object}
   * `subject` {string} `'default'`, `'always'`, or `'never'`.
-    **Default:** `'always'`.
+    **Default:** `'default'`.
 * 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 undefined or 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 `'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.
 
@@ -2507,6 +2510,9 @@ considered, even if the certificate contains no subject alternative names.
 <!-- YAML
 added: v15.6.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/41600
+    description: The subject option now defaults to `'default'`.
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/41569
     description: The subject option can now be set to `'default'`.
@@ -2515,7 +2521,7 @@ changes:
 * `name` {string}
 * `options` {Object}
   * `subject` {string} `'default'`, `'always'`, or `'never'`.
-    **Default:** `'always'`.
+    **Default:** `'default'`.
   * `wildcards` {boolean} **Default:** `true`.
   * `partialWildcards` {boolean} **Default:** `true`.
   * `multiLabelWildcards` {boolean} **Default:** `false`.
@@ -2531,15 +2537,15 @@ 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 undefined or 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 `'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.
 
diff --git a/lib/internal/crypto/x509.js b/lib/internal/crypto/x509.js
index cd20f6868da72c..e7098d17da6aac 100644
--- a/lib/internal/crypto/x509.js
+++ b/lib/internal/crypto/x509.js
@@ -65,8 +65,7 @@ function isX509Certificate(value) {
 function getFlags(options = {}) {
   validateObject(options, 'options');
   const {
-    // TODO(tniessen): change the default to 'default'
-    subject = 'always',  // Can be 'default', 'always', or 'never'
+    subject = 'default',  // Can be 'default', 'always', or 'never'
     wildcards = true,
     partialWildcards = true,
     multiLabelWildcards = false,
diff --git a/test/parallel/test-x509-escaping.js b/test/parallel/test-x509-escaping.js
index 58524e06a6e73f..3f534cfa168c72 100644
--- a/test/parallel/test-x509-escaping.js
+++ b/test/parallel/test-x509-escaping.js
@@ -425,7 +425,7 @@ const { hasOpenSSL3 } = common;
   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), undefined);
   assert.strictEqual(certX509.checkHost(servername, { subject: 'default' }),
                      undefined);
   assert.strictEqual(certX509.checkHost(servername, { subject: 'always' }),