test: enable test_quic_verifyhostnameidentity.cc

This commit adds a suggestion for enabling the cctest test_quic_verifyhostnameidentity.cc. PR-URL: #169 Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Oct 16, 2019 · 46730e8 · 46730e8
1 parent 822f1b9
commit 46730e8
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 82 deletions.
diff --git a/src/node_quic_crypto.cc b/src/node_quic_crypto.cc
@@ -533,84 +533,6 @@ bool CheckCertNames(
   return true;
 }
 
-int VerifyHostnameIdentity(
-    const char* hostname,
-    const std::string& cert_cn,
-    const std::unordered_multimap<std::string, std::string>& altnames) {
-
-  int err = X509_V_ERR_HOSTNAME_MISMATCH;
-
-  // 1. If the hostname is an IP address (v4 or v6), the certificate is valid
-  //    if and only if there is an 'IP Address:' alt name specifying the same
-  //    IP address. The IP address must be canonicalized to ensure a proper
-  //    check. It's possible that the X509_check_ip_asc covers this. If so,
-  //    we can remove this check.
-
-  if (SocketAddress::numeric_host(hostname)) {
-    auto ips = altnames.equal_range("ip");
-    for (auto ip = ips.first; ip != ips.second; ++ip) {
-      if (ip->second.compare(hostname) == 0) {
-        // Success!
-        return 0;
-      }
-    }
-    // No match, and since the hostname is an IP address, skip any
-    // further checks
-    return err;
-  }
-
-  auto dns_names = altnames.equal_range("dns");
-  auto uri_names = altnames.equal_range("uri");
-
-  size_t dns_count = std::distance(dns_names.first, dns_names.second);
-  size_t uri_count = std::distance(uri_names.first, uri_names.second);
-
-  std::vector<std::string> host_parts;
-  SplitHostname(hostname, &host_parts);
-
-  // 2. If there no 'DNS:' or 'URI:' Alt names, if the certificate has a
-  //    Subject, then we need to extract the CN field from the Subject. and
-  //    check that the hostname matches the CN, taking into consideration
-  //    the possibility of a wildcard in the CN. If there is a match, congrats,
-  //    we have a valid certificate. Return and be happy.
-
-  if (dns_count == 0 && uri_count == 0) {
-    if (cert_cn.length() > 0 && CheckCertNames(host_parts, cert_cn))
-        return 0;
-    // No match, and since there are no dns or uri entries, return
-    return err;
-  }
-
-  // 3. If, however, there are 'DNS:' and 'URI:' Alt names, things become more
-  //    complicated. Essentially, we need to iterate through each 'DNS:' and
-  //    'URI:' Alt name to find one that matches. The 'DNS:' Alt names are
-  //    relatively simple but may include wildcards. The 'URI:' Alt names
-  //    require the name to be parsed as a URL, then extract the hostname from
-  //    the URL, which is then checked against the hostname. If you find a
-  //    match, yay! Return and be happy. (Note, it's possible that the 'DNS:'
-  //    check in this step is redundant to the X509_check_host check. If so,
-  //    we can simplify by removing those checks here.)
-
-  // First, let's check dns names
-  for (auto name = dns_names.first; name != dns_names.second; ++name) {
-    if (name->first.length() > 0 &&
-        CheckCertNames(host_parts, name->second)) {
-      return 0;
-    }
-  }
-
-  // Then, check uri names
-  for (auto name = uri_names.first; name != uri_names.second; ++name) {
-    if (name->first.length() > 0 &&
-        CheckCertNames(host_parts, name->second, false)) {
-      return 0;
-    }
-  }
-
-  // 4. Failing all of the previous checks, we assume the certificate is
-  //    invalid for an unspecified reason.
-  return err;
-}
 
 const char* X509ErrorCode(int err) {
   const char* code = "UNSPECIFIED";
@@ -706,6 +628,85 @@ int VerifyHostnameIdentity(SSL* ssl, const char* hostname) {
       GetCertificateAltNames(cert.get()));
 }
 
+int VerifyHostnameIdentity(
+    const char* hostname,
+    const std::string& cert_cn,
+    const std::unordered_multimap<std::string, std::string>& altnames) {
+
+  int err = X509_V_ERR_HOSTNAME_MISMATCH;
+
+  // 1. If the hostname is an IP address (v4 or v6), the certificate is valid
+  //    if and only if there is an 'IP Address:' alt name specifying the same
+  //    IP address. The IP address must be canonicalized to ensure a proper
+  //    check. It's possible that the X509_check_ip_asc covers this. If so,
+  //    we can remove this check.
+
+  if (SocketAddress::numeric_host(hostname)) {
+    auto ips = altnames.equal_range("ip");
+    for (auto ip = ips.first; ip != ips.second; ++ip) {
+      if (ip->second.compare(hostname) == 0) {
+        // Success!
+        return 0;
+      }
+    }
+    // No match, and since the hostname is an IP address, skip any
+    // further checks
+    return err;
+  }
+
+  auto dns_names = altnames.equal_range("dns");
+  auto uri_names = altnames.equal_range("uri");
+
+  size_t dns_count = std::distance(dns_names.first, dns_names.second);
+  size_t uri_count = std::distance(uri_names.first, uri_names.second);
+
+  std::vector<std::string> host_parts;
+  SplitHostname(hostname, &host_parts);
+
+  // 2. If there no 'DNS:' or 'URI:' Alt names, if the certificate has a
+  //    Subject, then we need to extract the CN field from the Subject. and
+  //    check that the hostname matches the CN, taking into consideration
+  //    the possibility of a wildcard in the CN. If there is a match, congrats,
+  //    we have a valid certificate. Return and be happy.
+
+  if (dns_count == 0 && uri_count == 0) {
+    if (cert_cn.length() > 0 && CheckCertNames(host_parts, cert_cn))
+        return 0;
+    // No match, and since there are no dns or uri entries, return
+    return err;
+  }
+
+  // 3. If, however, there are 'DNS:' and 'URI:' Alt names, things become more
+  //    complicated. Essentially, we need to iterate through each 'DNS:' and
+  //    'URI:' Alt name to find one that matches. The 'DNS:' Alt names are
+  //    relatively simple but may include wildcards. The 'URI:' Alt names
+  //    require the name to be parsed as a URL, then extract the hostname from
+  //    the URL, which is then checked against the hostname. If you find a
+  //    match, yay! Return and be happy. (Note, it's possible that the 'DNS:'
+  //    check in this step is redundant to the X509_check_host check. If so,
+  //    we can simplify by removing those checks here.)
+
+  // First, let's check dns names
+  for (auto name = dns_names.first; name != dns_names.second; ++name) {
+    if (name->first.length() > 0 &&
+        CheckCertNames(host_parts, name->second)) {
+      return 0;
+    }
+  }
+
+  // Then, check uri names
+  for (auto name = uri_names.first; name != uri_names.second; ++name) {
+    if (name->first.length() > 0 &&
+        CheckCertNames(host_parts, name->second, false)) {
+      return 0;
+    }
+  }
+
+  // 4. Failing all of the previous checks, we assume the certificate is
+  //    invalid for an unspecified reason.
+  return err;
+}
+
 const char* GetServerName(QuicSession* session) {
   QuicCryptoContext* ctx = session->CryptoContext();
   return SSL_get_servername(**ctx, TLSEXT_NAMETYPE_host_name);

diff --git a/src/node_quic_crypto.h b/src/node_quic_crypto.h
@@ -94,6 +94,10 @@ bool InvalidRetryToken(
 int VerifyPeerCertificate(SSL* ssl);
 
 int VerifyHostnameIdentity(SSL* ssl, const char* hostname);
+int VerifyHostnameIdentity(
+    const char* hostname,
+    const std::string& cert_cn,
+    const std::unordered_multimap<std::string, std::string>& altnames);
 
 v8::Local<v8::Value> GetValidationErrorReason(Environment* env, int err);
 v8::Local<v8::Value> GetValidationErrorCode(Environment* env, int err);

diff --git a/test/cctest/test_quic_verifyhostnameidentity.cc b/test/cctest/test_quic_verifyhostnameidentity.cc
@@ -10,9 +10,6 @@
 #include <unordered_map>
 #include <vector>
 
-// This test has recently been broken.
-#if 0
-
 using node::quic::VerifyHostnameIdentity;
 
 enum altname_type {
@@ -348,4 +345,3 @@ TEST(QuicCrypto, BasicCN_41_Fail) {
           std::string("xn--*.example.com"), altnames),
       X509_V_ERR_HOSTNAME_MISMATCH);
 }
-#endif