SERVER-37370 Improve CN/SAN mismatch error message

6 files changed, 146 insertions, 9 deletions

diff --git a/jstests/ssl/libs/localhost-cn-with-san.pem b/jstests/ssl/libs/localhost-cn-with-san.pem
new file mode 100644
index 00000000000..0c91b96d0c4
--- /dev/null
+++ b/jstests/ssl/libs/localhost-cn-with-san.pem
@@ -0,0 +1,48 @@
+-----BEGIN CERTIFICATE-----
+MIIDizCCAnOgAwIBAgIUI+TiE0RShikzjCJa7vR4f1xAciMwDQYJKoZIhvcNAQEL
+BQAwdDEXMBUGA1UEAxMOS2VybmVsIFRlc3QgQ0ExDzANBgNVBAsTBktlcm5lbDEQ
+MA4GA1UEChMHTW9uZ29EQjEWMBQGA1UEBxMNTmV3IFlvcmsgQ2l0eTERMA8GA1UE
+CBMITmV3IFlvcmsxCzAJBgNVBAYTAlVTMB4XDTE5MDQxNjE3MTIwNloXDTI5MDQx
+MzE3MTIwNlowbzELMAkGA1UEBhMCVVMxETAPBgNVBAgMCE5ldyBZb3JrMRYwFAYD
+VQQHDA1OZXcgWW9yayBDaXR5MRAwDgYDVQQKDAdNb25nb0RCMQ8wDQYDVQQLDAZL
+ZXJuZWwxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcNAQEBBQADggEP
+ADCCAQoCggEBANRCyWerY91orawrcKBsT2CW3aVoCsqyT95bB7gAwrY1Z1++/C6P
+MaLR7DS7XUR/lFt/lfXfvQqHNaJp3P9OFyL8I0fKMo+EfmAUDyQSjPoFuNL0Tugh
+1MX72eOaAwHChcoK4V+zPdLV+w0Z8fQFLOuxTJts+hTMpXliFYz3+uVynWRwscMl
+SmJ3DfJbUj3aGc/r6FNRfzgpMqykeRzd6fo83McW38Ini9qH73hiHMGoIAsLvI84
+5Ob29sqe/mnsReWsFY4irGHRQjrb8Of5KN497pWFzXELDrnr29h7jSG2KZX1LqxQ
+W1ME7aj4LnKljo7YBDK1wH3q653ZwyBCkDECAwEAAaMaMBgwFgYDVR0RBA8wDYIL
+ZXhhbXBsZS5jb20wDQYJKoZIhvcNAQELBQADggEBADUAs//njtVr4T0ALnkbQnfb
+CvKkkmB57Ahqh4MnNdxbt6O68XyNHD71wSB0LzDT8rSzRDQ3zyVsOsHEPh49+ZaD
+AfXd3rCLmkUKqAGf0FCuQfqdmffAI+kDNavM3qlUhXTq/6j6J+ExqQvYob0UNIcb
+ZivCV0XJc8IMnEEUco2pdVvt49frp/jmSvXyNVsd+rYBc9ouffCa+Di3iYuAVbGF
+3Q+i0zJKjkaoklVsYQkxgCxiNzmWREKukPyq5CqAYkLehChTlXNY0+mmihqFaX7l
+x62xNDItbOPCrOAk57yCqgEkT8oGPqcp0mARZvqTZVVVHtQ/UFAKwX6Rhonfok8=
+-----END CERTIFICATE-----
+-----BEGIN RSA PRIVATE KEY-----
+MIIEpAIBAAKCAQEA1ELJZ6tj3WitrCtwoGxPYJbdpWgKyrJP3lsHuADCtjVnX778
+Lo8xotHsNLtdRH+UW3+V9d+9Coc1omnc/04XIvwjR8oyj4R+YBQPJBKM+gW40vRO
+6CHUxfvZ45oDAcKFygrhX7M90tX7DRnx9AUs67FMm2z6FMyleWIVjPf65XKdZHCx
+wyVKYncN8ltSPdoZz+voU1F/OCkyrKR5HN3p+jzcxxbfwieL2ofveGIcwaggCwu8
+jzjk5vb2yp7+aexF5awVjiKsYdFCOtvw5/ko3j3ulYXNcQsOuevb2HuNIbYplfUu
+rFBbUwTtqPgucqWOjtgEMrXAferrndnDIEKQMQIDAQABAoIBABAlOF6rWcBIqxUy
+hLGfK3aEYz2mqVN7trmKIQ8s96KWc1OEGniMKdFhuF7WdaS1M5q2HLkEGMQJPtoo
+xUeiL/5YQXK/fZMPF/HE9NXgl1qZPtLFBI+Bq0aX/p3iZCIpIrYYl8YIw0YbQqpf
+c9kE38LZzKLx6h/U5S1TeaIsZqg3KSrUbHJtqpPpakSFHrRTek4h/fpME5JKlhBP
+9ZA9wWicyO4f3B45nWVg6P0gJiN+YUDz5KrhjDZjcGzl1FFCiQVVqJkh7tsPJG1c
+ajJROcYxQe9nnqdsVeAmVR8OWRT4MyW/pgSjCaNwwguHLcNF/dINirqgYZURwQT1
+qOum43ECgYEA7pKrSN1Q8xOfdqZpXcCZ0Y+0+JIM6PW3UnskkTV/KgxNZhSLEDVx
+xmo/yEd2iaNJeHcX06+IK4Ln6e/FmLk7L3hic3omKJGgsH60KF7OVrTOpVYPSBlt
+8zSo34CIp1TE8c0NVw6SMzJeuMpXhEuGsB65jp9OUhXyNMXqByPOkEUCgYEA48QU
+ooZOGekB5cTdMkvxHPq3n8glhkUBIqaV8XICfAU9fnThdPTr0K97pHEbEElt3k3l
+9fsNDdH5cJExxDp7daTegxIpZiGI34/YT1qeaguy9teLkrEs/HpQBAbWLwPga0up
+3rA6NspNfj1qa3pGgmjBuZx88Bp2A3p9ER17zP0CgYEA1MclU0hokVO4DqefJAnK
+ZriIDIaeUwwL49mGpKgU8ZilU10RJnS0M+XBW0w5ccqblYCKVLeieEQ5uVCraGoU
+WmNBU4LnE5nGuoNXBQaBe52pDJcz6/caNCaMU0KdHrihKpPayQFq7ZSIHMICyjBe
+QSg2EPkObnG6Ysz9zD5oVg0CgYBcitrnfjJVjLrcpLRLL1f0MlpRbXzxpS/k6FKc
+G3qu8WIe81Z3stvtmp9eHSal42/klzoMS8juCJaVCilyVWC8WBSgpMx1VsLKMyPs
+eiFLbR119fYb0Ck7HGdOe4ii6axRbBjFBk1g3anG1SVcLf5gFjhANjj9RYMVvrr6
+LUST8QKBgQCAWmYkVGOREQHFaeSjITnrnASSf+vQLc+3Xl1TMAp2tie7l4SFX2GE
+46WVmKBIJnOh0yqxXdymBOKQ7gHsj9BIQC5U6/tCaTl8dNQYhKZqKXtL3u3fpJ/G
+lwMJqsLp1E8KHvm8WB5OLBQ93+TIKSMMbTdPzxXmsQ+D3Q3uioNT8A==
+-----END RSA PRIVATE KEY-----
diff --git a/jstests/ssl/libs/localhost-cn-with-san.pem.sh b/jstests/ssl/libs/localhost-cn-with-san.pem.sh
new file mode 100755
index 00000000000..da3789a506b
--- /dev/null
+++ b/jstests/ssl/libs/localhost-cn-with-san.pem.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+# Create a certificate with `CN=localhost` to satisfy CN matching,
+# but override it with a SAN field which will not match.
+set -ev
+
+RDN="/C=US/ST=New York/L=New York City/O=MongoDB/OU=Kernel/CN=localhost"
+OPENSSL="/opt/mongodbtoolchain/v3/bin/openssl"
+FILE="jstests/ssl/libs/localhost-cn-with-san"
+
+$OPENSSL req -new -subj "${RDN}" \
+             -keyout "${FILE}.key" -out "${FILE}.csr" \
+             -nodes -batch -sha256 -newkey rsa:2048
+$OPENSSL rsa -in "${FILE}.key" -out "${FILE}.rsa"
+$OPENSSL x509 -in "${FILE}.csr" -out "${FILE}.pem" -req -CA "jstests/libs/ca.pem" \
+              -days 3650 -CAcreateserial \
+              -extfile <(printf "subjectAltName=DNS:example.com")
+
+# Create final bundle and cleanup.
+cat "${FILE}.rsa" >> "${FILE}.pem"
+
+rm jstests/libs/ca.srl
+rm "${FILE}.key" "${FILE}.rsa" "${FILE}.csr"
diff --git a/jstests/ssl/ssl_cn_with_san.js b/jstests/ssl/ssl_cn_with_san.js
new file mode 100644
index 00000000000..41e039c0a48
--- /dev/null
+++ b/jstests/ssl/ssl_cn_with_san.js
@@ -0,0 +1,46 @@
+// Test that a certificate with a valid CN, but invalid SAN
+// does not permit connection, but provides a useful error.
+
+(function() {
+    'use strict';
+    load('jstests/ssl/libs/ssl_helpers.js');
+
+    // server-intermediate-ca was signed by ca.pem, not trusted-ca.pem
+    const CA = 'jstests/libs/ca.pem';
+    const SERVER = 'jstests/ssl/libs/localhost-cn-with-san.pem';
+
+    const mongod = MongoRunner.runMongod({
+        sslMode: 'requireSSL',
+        sslPEMKeyFile: SERVER,
+        sslCAFile: CA,
+    });
+    assert(mongod);
+
+    // Try with `tlsAllowInvalidHostnames` to look for the warning.
+    clearRawMongoProgramOutput();
+    const mongo = runMongoProgram('mongo',
+                                  '--tls',
+                                  '--tlsCAFile',
+                                  CA,
+                                  'localhost:' + mongod.port,
+                                  '--eval',
+                                  ';',
+                                  '--tlsAllowInvalidHostnames');
+    assert.neq(mongo, 0, "Shell connected when it should have failed");
+    assert(rawMongoProgramOutput().includes(' would have matched, but was overridden by SAN'),
+           'Expected detail warning not seen');
+
+    // On OpenSSL only, start without `tlsAllowInvalidHostnames`
+    // Windowds/Mac will bail out too early to show this message.
+    if (determineSSLProvider() === 'openssl') {
+        clearRawMongoProgramOutput();
+        const mongo = runMongoProgram(
+            'mongo', '--tls', '--tlsCAFile', CA, 'localhost:' + mongod.port, '--eval', ';');
+        assert.neq(mongo, 0, "Shell connected when it should have failed");
+        assert(rawMongoProgramOutput().includes(
+                   'CN: localhost would have matched, but was overridden by SAN'),
+               'Expected detail warning not seen');
+    }
+
+    MongoRunner.stopMongod(mongod);
+})();
diff --git a/src/mongo/util/net/ssl_manager_apple.cpp b/src/mongo/util/net/ssl_manager_apple.cpp
index fd90601440d..c8ad459d88a 100644
--- a/src/mongo/util/net/ssl_manager_apple.cpp
+++ b/src/mongo/util/net/ssl_manager_apple.cpp
@@ -1598,8 +1598,9 @@ StatusWith<boost::optional<SSLPeerInfo>> SSLManagerApple::parseAndValidatePeerCe
             }
             certErr << san << " ";
         }
+    }
 
-    } else {
+    if (!sanMatch) {
         auto swCN = peerSubjectName.getOID(kOID_CommonName);
         if (swCN.isOK()) {
             auto commonName = std::move(swCN.getValue());
@@ -1611,8 +1612,13 @@ StatusWith<boost::optional<SSLPeerInfo>> SSLManagerApple::parseAndValidatePeerCe
             } else if (hostNameMatchForX509Certificates(remoteHost, commonName)) {
                 cnMatch = true;
             }
-            certErr << "CN: " << commonName;
-        } else {
+
+            if (cnMatch && !sans.empty()) {
+                // SANs override CN for matching purposes.
+                cnMatch = false;
+                certErr << "CN: " << commonName << " would have matched, but was overridden by SAN";
+            }
+        } else if (sans.empty()) {
             certErr << "No Common Name (CN) or Subject Alternate Names (SAN) found";
         }
     }
diff --git a/src/mongo/util/net/ssl_manager_openssl.cpp b/src/mongo/util/net/ssl_manager_openssl.cpp
index 8ebdf5158ef..a73c0180cc6 100644
--- a/src/mongo/util/net/ssl_manager_openssl.cpp
+++ b/src/mongo/util/net/ssl_manager_openssl.cpp
@@ -1601,17 +1601,24 @@ StatusWith<boost::optional<SSLPeerInfo>> SSLManagerOpenSSL::parseAndValidatePeer
             }
         }
         sk_GENERAL_NAME_pop_free(sanNames, GENERAL_NAME_free);
-    } else {
-        // If Subject Alternate Name (SAN) doesn't exist and Common Name (CN) does,
-        // check Common Name.
+    }
+
+    if (!sanMatch) {
+        // If SAN doesn't match, check to see if CN does.
+        // If it does and no SAN was provided, that's a match.
+        // Anything else is a varying degree of failure.
         auto swCN = peerSubject.getOID(kOID_CommonName);
         if (swCN.isOK()) {
             auto commonName = std::move(swCN.getValue());
+            certificateNames << "CN: " << commonName;
             if (hostNameMatchForX509Certificates(remoteHost, commonName)) {
-                cnMatch = true;
+                if (sanNames) {
+                    certificateNames << " would have matched, but was overridden by SAN";
+                } else {
+                    cnMatch = true;
+                }
             }
-            certificateNames << "CN: " << commonName;
-        } else {
+        } else if (!sanNames) {
             certificateNames << "No Common Name (CN) or Subject Alternate Names (SAN) found";
         }
     }
diff --git a/src/mongo/util/net/ssl_manager_windows.cpp b/src/mongo/util/net/ssl_manager_windows.cpp
index 95b5bda7bd8..c5b8771f945 100644
--- a/src/mongo/util/net/ssl_manager_windows.cpp
+++ b/src/mongo/util/net/ssl_manager_windows.cpp
@@ -1675,7 +1675,9 @@ Status validatePeerCertificate(const std::string& remoteHost,
 
             // Give the user a hint why the certificate validation failed.
             StringBuilder certificateNames;
+            bool hasSAN = false;
             if (swAltNames.isOK() && !swAltNames.getValue().empty()) {
+                hasSAN = true;
                 for (auto& name : swAltNames.getValue()) {
                     certificateNames << name << " ";
                 }
@@ -1683,6 +1685,12 @@ Status validatePeerCertificate(const std::string& remoteHost,
 
             certificateNames << ", Subject Name: " << *peerSubjectName;
 
+            auto swCN = peerSubjectName->getOID(kOID_CommonName);
+            if (hasSAN && swCN.isOK() &&
+                hostNameMatchForX509Certificates(remoteHost, swCN.getValue())) {
+                certificateNames << " would have matched, but was overridden by SAN";
+            }
+
             str::stream msg;
             msg << "The server certificate does not match the host name. Hostname: " << remoteHost
                 << " does not match " << certificateNames.str();