Refactor pooling logic into a helper method

net/http/http_util.cc

+
 // 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.
 
 // The rules for parsing content-types were borrowed from Firefox:
 // http://lxr.mozilla.org/mozilla/source/netwerk/base/src/nsURLHelper.cpp#834
 
 #include "net/http/http_util.h"
 
 #include <algorithm>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_tokenizer.h"
 #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 {
 
 // Helpers --------------------------------------------------------------------
 
 // Returns the index of the closing quote of the string, if any.  |start| points
 // at the opening quote.
 static size_t FindStringEnd(const std::string& line, size_t start, char delim) {
   DCHECK_LT(start, line.length());
   DCHECK_EQ(line[start], delim);
@@ skipping 702 matching lines @@
 }
 
 // static
 int HttpUtil::MapStatusCodeForHistogram(int code) {
   if (HISTOGRAM_MIN_HTTP_STATUS_CODE <= code &&
       code <= HISTOGRAM_MAX_HTTP_STATUS_CODE)
     return 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 ]
 //   field-name     = token
 //   field-value    = *( field-content | LWS )
 //   field-content  = <the OCTETs making up the field-value
 //                     and consisting of either *TEXT or combinations
 //                     of token, separators, and quoted-string>
 //
 
@@ skipping 145 matching lines @@
       value_is_quoted_ = true;
       // Do not store iterators into this. See declaration of unquoted_value_.
       unquoted_value_ = HttpUtil::Unquote(value_begin_, value_end_);
     }
   }
 
   return true;
 }
 
 }  // namespace net