Refactor pooling logic into a helper method

net/http/http_util.cc

Index: net/http/http_util.cc
diff --git a/net/http/http_util.cc b/net/http/http_util.cc
index f4f994af6052f294bb52e1714cf1ca625af1cadc..54b66ffe61c9f07198b084b6543950c4acfa46b6 100644
--- a/net/http/http_util.cc
+++ b/net/http/http_util.cc
@@ -1,3 +1,4 @@
+
 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
@@ -17,7 +18,11 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
-
+#include "net/cert/cert_verify_result.h"
+#include "net/cert/x509_certificate.h"
+#include "net/http/transport_security_state.h"
+#include "net/ssl/channel_id_service.h"
+#include "net/ssl/ssl_info.h"
 
 namespace net {
 
@@ -740,6 +745,42 @@ int HttpUtil::MapStatusCodeForHistogram(int code) {
   return 0;
 }
 
+// static
+bool HttpUtil::CanPool(TransportSecurityState* transport_security_state,
+                       const SSLInfo& ssl_info,
+                       const std::string& old_hostname,
+                       const std::string& new_hostname) {
+  // Pooling is prohibited if the server cert is not valid for the new domain,
+  // and for connections on which client certs were sent. It is also prohibited
+  // when channel ID was sent if the hosts are from different eTLDs+1.
+  bool unused = false;
+  if (!ssl_info.cert->VerifyNameMatch(new_hostname, &unused))
+    return false;

[Ryan Sleevi, 2014/08/07 18:49:29]

You should check this on 771

[Ryan Hamilton, 2014/08/08 19:27:43]

Done. (But how come?)

[Ryan Sleevi, 2014/08/11 19:03:43]

Forgot to answer this - verify the cert is trusted/trustworthy before acting
upon/processing the parsed data. Avoid hijinks from hostile certs :)

[Ryan Hamilton, 2014/08/12 14:39:06]

Oh! Good point.

+
+  if (IsCertStatusError(ssl_info.cert_status))
+    return false;
+
+  if (ssl_info.client_cert_sent)
+    return false;
+
+  if (ssl_info.channel_id_sent &&
+      ChannelIDService::GetDomainForHost(new_hostname) !=
+      ChannelIDService::GetDomainForHost(old_hostname)) {
+    return false;
+  }
+
+  if (!transport_security_state->VerifyPinning(
+      ssl_info.public_key_hashes,
+      ssl_info.is_issued_by_known_root,
+      /* sni_available= */ true,
+      new_hostname,
+      /* pinning_failure_log= */ NULL)) {

[Ryan Sleevi, 2014/08/07 18:49:29]

1) git-cl-format this (you should be four *additional* spaces indented)
2) It seems like the style from
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
should apply here , which matches a good bit of Chromium

That is
true, /* sni_available */
NULL, /* pinning_failure_log */

[Ryan Hamilton, 2014/08/08 19:27:43]

Done.
I don't think that part of the style guide applies here, because that is simple
commenting out the name of a parameter in the location it would otherwise
appear. I think the part of the style guide which is more on point is here,
right?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem...
I'm ok switching to this (or perhaps to "// SNI is available") but I don't love
it and I'm curious what you think. The trouble comes with:

  false, /* is_server */

When I read that, I'm always confused if that is saying that is_server is false,
or if it is saying that false means that this is a server. (This is particularly
trick if the term is something like "success"). That's why I prefer the style
with the =.

That being said, when in Rome...

+    return false;
+  }
+
+  return true;
+}
+
 // BNF from section 4.2 of RFC 2616:
 //
 //   message-header = field-name ":" [ field-value ]