Refactor pooling logic into a helper method

net/spdy/spdy_session.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.
 
 #include "net/spdy/spdy_session.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/basictypes.h"
@@ skipping 11 matching lines @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "crypto/ec_private_key.h"
 #include "crypto/ec_signature_creator.h"
 #include "net/base/connection_type_histograms.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
 #include "net/cert/asn1_util.h"
+#include "net/cert/cert_verify_result.h"
 #include "net/http/http_log_util.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_server_properties.h"
 #include "net/http/http_util.h"
+#include "net/http/transport_security_state.h"
 #include "net/spdy/spdy_buffer_producer.h"
 #include "net/spdy/spdy_frame_builder.h"
 #include "net/spdy/spdy_http_utils.h"
 #include "net/spdy/spdy_protocol.h"
 #include "net/spdy/spdy_session_pool.h"
 #include "net/spdy/spdy_stream.h"
 #include "net/ssl/channel_id_service.h"
 #include "net/ssl/ssl_cipher_suite_names.h"
 #include "net/ssl/ssl_connection_status_flags.h"
 
@@ skipping 476 matching lines @@
 SpdySession::PushedStreamInfo::PushedStreamInfo() : stream_id(0) {}
 
 SpdySession::PushedStreamInfo::PushedStreamInfo(
     SpdyStreamId stream_id,
     base::TimeTicks creation_time)
     : stream_id(stream_id),
       creation_time(creation_time) {}
 
 SpdySession::PushedStreamInfo::~PushedStreamInfo() {}
 
+// static
+bool SpdySession::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.
+  if (IsCertStatusError(ssl_info.cert_status))

[Ryan Sleevi, 2014/08/11 18:45:17]

Should this be

if (IsCertStatusError(ssl_info.cert_status) &&
!IsCertStatusMinorError(ssl_info.cert_status)) ?


Why or why not?

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

No idea :> I'm happy to defer to your judgement because I'm unfamiliar with the
nuances here. We obviously don't want to pool onto a connection that the use
would have to override an interstitial for. If minor errors don't trigger such
user-visible issues, then your suggestion is clearly right. I've made that
change, but I would like confirmation  :>

[Ryan Sleevi, 2014/08/12 14:52:03]

If the answer is no idea, let's defer to closed.

Right now the minor errors do trigger UI, but don't affect things like treating
it as mixed content or the like.

Right now the minor errors are just OCSP fail open statuses. We're still working
out the policy as to whether SHA-1 certs will be treated as a minor error, same
with certs that are valid too long. Neither will cause an interstitial, but they
WILL be surfaced to the user (omnibox) and MAY affect processing (e.g. mixed
content)

I'm happy with requiring strongest/best to get pooling, and seem to recall
something in HTTP/2 to that effect?

[Ryan Hamilton, 2014/08/12 15:37:10]

SGTM. Done.

+    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;
+  }
+
+  bool unused = false;
+  if (!ssl_info.cert->VerifyNameMatch(new_hostname, &unused))
+    return false;
+
+  std::string pinning_failure_log;
+  if (!transport_security_state->CheckPublicKeyPins(
+          new_hostname,
+          true, /* sni_available */
+          ssl_info.is_issued_by_known_root,
+          ssl_info.public_key_hashes,
+          &pinning_failure_log)) {
+    return false;
+  }
+
+  return true;
+}
+
 SpdySession::SpdySession(
     const SpdySessionKey& spdy_session_key,
     const base::WeakPtr<HttpServerProperties>& http_server_properties,
+    TransportSecurityState* transport_security_state,
     bool verify_domain_authentication,
     bool enable_sending_initial_data,
     bool enable_compression,
     bool enable_ping_based_connection_checking,
     NextProto default_protocol,
     size_t stream_initial_recv_window_size,
     size_t initial_max_concurrent_streams,
     size_t max_concurrent_streams_limit,
     TimeFunc time_func,
     const HostPortPair& trusted_spdy_proxy,
     NetLog* net_log)
     : in_io_loop_(false),
       spdy_session_key_(spdy_session_key),
       pool_(NULL),
       http_server_properties_(http_server_properties),
+      transport_security_state_(transport_security_state),
       read_buffer_(new IOBuffer(kReadBufferSize)),
       stream_hi_water_mark_(kFirstStreamId),
       num_pushed_streams_(0u),
       num_active_pushed_streams_(0u),
       in_flight_write_frame_type_(DATA),
       in_flight_write_frame_size_(0),
       is_secure_(false),
       certificate_error_code_(OK),
       availability_state_(STATE_AVAILABLE),
       read_state_(READ_STATE_DO_READ),
@@ skipping 147 matching lines @@
 
   if (availability_state_ == STATE_DRAINING)
     return false;
 
   SSLInfo ssl_info;
   bool was_npn_negotiated;
   NextProto protocol_negotiated = kProtoUnknown;
   if (!GetSSLInfo(&ssl_info, &was_npn_negotiated, &protocol_negotiated))
     return true;   // This is not a secure session, so all domains are okay.
 
-  // Disable pooling for secure sessions.
-  // TODO(rch): re-enable this.
-  return false;
-#if 0
-  bool unused = false;
-  return
-      !ssl_info.client_cert_sent &&
-      (!ssl_info.channel_id_sent ||
-       (ChannelIDService::GetDomainForHost(domain) ==
-        ChannelIDService::GetDomainForHost(host_port_pair().host()))) &&
-      ssl_info.cert->VerifyNameMatch(domain, &unused);
-#endif
+  return CanPool(transport_security_state_, ssl_info,
+                 host_port_pair().host(), domain);
 }
 
 int SpdySession::GetPushStream(
     const GURL& url,
     base::WeakPtr<SpdyStream>* stream,
     const BoundNetLog& stream_net_log) {
   CHECK(!in_io_loop_);
 
   stream->reset();
 
@@ skipping 2425 matching lines @@
     if (!queue->empty()) {
       SpdyStreamId stream_id = queue->front();
       queue->pop_front();
       return stream_id;
     }
   }
   return 0;
 }
 
 }  // namespace net