Verify alternative server certificate validity for origin.

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 78f4eedb1a19300346cca7428e756a73c41d2279..bdbc0ecf4af09090de6b1a00775f4ec581797637 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -271,12 +271,11 @@ SpdySessionKey HttpStreamFactoryImpl::Job::GetSpdySessionKey() const {
   // In the case that we're using an HTTPS proxy for an HTTP url,
   // we look for a SPDY session *to* the proxy, instead of to the
   // origin server.
-  PrivacyMode privacy_mode = request_info_.privacy_mode;
   if (IsHttpsProxyAndHttpUrl()) {
     return SpdySessionKey(proxy_info_.proxy_server().host_port_pair(),
-                          ProxyServer::Direct(),
-                          privacy_mode);
+                          ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
   }
+  PrivacyMode privacy_mode = request_info_.privacy_mode;
   return SpdySessionKey(server_, proxy_info_.proxy_server(), privacy_mode);

[Ryan Hamilton, 2015/04/13 18:29:56]

I recommend doing this cleanup in a different CL since it's orthogonal.

[Bence, 2015/04/15 21:07:46]

Done: https://crrev.com/1063873003.

 }
 
@@ -536,9 +535,14 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
       return ERR_IO_PENDING;
 
     default:
+      DCHECK((result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN) ||

[Ryan Hamilton, 2015/04/13 18:29:56]

No need for the 2nd set of ()s. Feel free to put IsSpdyAlternate() first if that
helps).  Or do:

if (!SpdyAlternate())
  DCHECK_NE(ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN, result);

[Bence, 2015/04/15 21:07:46]

Done.

+             IsSpdyAlternate());
       if (job_status_ != STATUS_BROKEN) {
         DCHECK_EQ(STATUS_RUNNING, job_status_);
         job_status_ = STATUS_FAILED;
+        // TODO(bnc): If (result == ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN),
+        // then instead of marking alternative service broken, mark (origin,
+        // alternative service) couple as invalid.
         MaybeMarkAlternativeServiceBroken();
       }
       base::MessageLoop::current()->PostTask(
@@ -623,6 +627,8 @@ int HttpStreamFactoryImpl::Job::DoStart() {
   }
   origin_url_ =
       stream_factory_->ApplyHostMappingRules(request_info_.url, &server_);
+  valid_spdy_session_pool_.reset(
+      new ValidSpdySessionPool(session_, origin_url_, IsSpdyAlternate()));
 
   net_log_.BeginEvent(
       NetLog::TYPE_HTTP_STREAM_JOB,
@@ -797,29 +803,36 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
     return rv;
   }
 
+  SpdySessionKey spdy_session_key = GetSpdySessionKey();
+
   // Check first if we have a spdy session for this group.  If so, then go
   // straight to using that.
-  SpdySessionKey spdy_session_key = GetSpdySessionKey();
-  base::WeakPtr<SpdySession> spdy_session =
-      session_->spdy_session_pool()->FindAvailableSession(
-          spdy_session_key, net_log_);
-  if (spdy_session && CanUseExistingSpdySession()) {
-    // If we're preconnecting, but we already have a SpdySession, we don't
-    // actually need to preconnect any sockets, so we're done.
-    if (IsPreconnecting())
+  if (CanUseExistingSpdySession()) {
+    bool is_valid;
+    base::WeakPtr<SpdySession> spdy_session =
+        valid_spdy_session_pool_->FindAvailableSession(spdy_session_key,
+                                                       net_log_, &is_valid);
+    if (!is_valid) {
+      return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN;
+    }
+    if (spdy_session) {
+      // If we're preconnecting, but we already have a SpdySession, we don't
+      // actually need to preconnect any sockets, so we're done.
+      if (IsPreconnecting())
+        return OK;
+      using_spdy_ = true;
+      next_state_ = STATE_CREATE_STREAM;
+      existing_spdy_session_ = spdy_session;
       return OK;
-    using_spdy_ = true;
-    next_state_ = STATE_CREATE_STREAM;
-    existing_spdy_session_ = spdy_session;
-    return OK;
-  } else if (request_ && !request_->HasSpdySessionKey() && using_ssl_) {
+    }
+  }
+  if (request_ && !request_->HasSpdySessionKey() && using_ssl_) {
     // Update the spdy session key for the request that launched this job.
     request_->SetSpdySessionKey(spdy_session_key);
   }
 
   // OK, there's no available SPDY session. Let |waiting_job_| resume if it's
   // paused.
-
   if (waiting_job_) {
     waiting_job_->Resume(this);
     waiting_job_ = NULL;
@@ -864,10 +877,11 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
 
   // If we can't use a SPDY session, don't bother checking for one after
   // the hostname is resolved.
-  OnHostResolutionCallback resolution_callback = CanUseExistingSpdySession() ?
-      base::Bind(&Job::OnHostResolution, session_->spdy_session_pool(),
-                 GetSpdySessionKey()) :
-      OnHostResolutionCallback();
+  OnHostResolutionCallback resolution_callback =
+      CanUseExistingSpdySession()
+          ? base::Bind(&Job::OnHostResolution, session_->spdy_session_pool(),
+                       spdy_session_key)
+          : OnHostResolutionCallback();
   if (stream_factory_->for_websockets_) {
     // TODO(ricea): Re-enable NPN when WebSockets over SPDY is supported.
     SSLConfig websocket_server_ssl_config = server_ssl_config_;
@@ -989,6 +1003,9 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
 
   if (!ssl_started && result < 0 && IsAlternate()) {
     job_status_ = STATUS_BROKEN;
+    // TODO(bnc): if (result == ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN), then
+    // instead of marking alternative service broken, mark (origin, alternative
+    // service) couple as invalid.
     MaybeMarkAlternativeServiceBroken();
     return result;
   }
@@ -1099,12 +1116,7 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
 
   CHECK(!stream_.get());
 
-  bool direct = true;
-  const ProxyServer& proxy_server = proxy_info_.proxy_server();
-  PrivacyMode privacy_mode = request_info_.privacy_mode;
-  if (IsHttpsProxyAndHttpUrl())
-    direct = false;
-
+  bool direct = !IsHttpsProxyAndHttpUrl();
   if (existing_spdy_session_.get()) {
     // We picked up an existing session, so we don't need our socket.
     if (connection_->socket())
@@ -1116,30 +1128,25 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     return set_result;
   }
 
-  SpdySessionKey spdy_session_key(server_, proxy_server, privacy_mode);
-  if (IsHttpsProxyAndHttpUrl()) {
-    // If we don't have a direct SPDY session, and we're using an HTTPS
-    // proxy, then we might have a SPDY session to the proxy.
-    // We never use privacy mode for connection to proxy server.
-    spdy_session_key = SpdySessionKey(proxy_server.host_port_pair(),
-                                      ProxyServer::Direct(),
-                                      PRIVACY_MODE_DISABLED);
-  }
-
-  SpdySessionPool* spdy_pool = session_->spdy_session_pool();
+  SpdySessionKey spdy_session_key = GetSpdySessionKey();
+  bool is_valid;
   base::WeakPtr<SpdySession> spdy_session =
-      spdy_pool->FindAvailableSession(spdy_session_key, net_log_);
-
+      valid_spdy_session_pool_->FindAvailableSession(spdy_session_key, net_log_,
+                                                     &is_valid);
+  if (!is_valid) {

[Ryan Hamilton, 2015/04/13 18:29:56]

Hm. I'm not in love with this API. Instead, I would make the ValidPool methods
return int and make the session an out param. That way the logic of your new
NAME_NOT_VALID error would go in the pool

[Bence, 2015/04/15 21:07:46]

Done.

+    return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN;
+  }
   if (spdy_session) {
     return SetSpdyHttpStream(spdy_session, direct);
   }
 
-  spdy_session =
-      spdy_pool->CreateAvailableSessionFromSocket(spdy_session_key,
-                                                  connection_.Pass(),
-                                                  net_log_,
-                                                  spdy_certificate_error_,
-                                                  using_ssl_);
+  spdy_session = valid_spdy_session_pool_->CreateAvailableSessionFromSocket(
+      spdy_session_key, connection_.Pass(), net_log_, spdy_certificate_error_,
+      using_ssl_, &is_valid);
+  if (!is_valid) {
+    return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN;
+  }
+
   if (!spdy_session->HasAcceptableTransportSecurity()) {
     spdy_session->CloseSessionOnError(
         ERR_SPDY_INADEQUATE_TRANSPORT_SECURITY, "");
@@ -1475,6 +1482,48 @@ void HttpStreamFactoryImpl::Job::MaybeMarkAlternativeServiceBroken() {
   }
 }
 
+HttpStreamFactoryImpl::Job::ValidSpdySessionPool::ValidSpdySessionPool(
+    HttpNetworkSession* session,

[Ryan Hamilton, 2015/04/13 18:29:56]

Looks like you can just take the Pool instead of session_, so you don't have to
do session_->spdy_session_pool()

[Bence, 2015/04/15 21:07:46]

Done.

+    GURL& origin_url,
+    bool is_spdy_alternate)
+    : session_(session),
+      origin_url_(origin_url),
+      is_spdy_alternate_(is_spdy_alternate) {
+}
+
+base::WeakPtr<SpdySession>
+HttpStreamFactoryImpl::Job::ValidSpdySessionPool::FindAvailableSession(
+    const SpdySessionKey& key,
+    const BoundNetLog& net_log,
+    bool* is_valid) {
+  base::WeakPtr<SpdySession> spdy_session =
+      session_->spdy_session_pool()->FindAvailableSession(key, net_log);
+  *is_valid = IsAlternativeCertificateValidForOrigin(spdy_session);
+  return spdy_session;
+}
+
+base::WeakPtr<SpdySession> HttpStreamFactoryImpl::Job::ValidSpdySessionPool::
+    CreateAvailableSessionFromSocket(const SpdySessionKey& key,
+                                     scoped_ptr<ClientSocketHandle> connection,
+                                     const BoundNetLog& net_log,
+                                     int certificate_error_code,
+                                     bool is_secure,
+                                     bool* is_valid) {
+  base::WeakPtr<SpdySession> spdy_session =
+      session_->spdy_session_pool()->CreateAvailableSessionFromSocket(
+          key, connection.Pass(), net_log, certificate_error_code, is_secure);
+  *is_valid = IsAlternativeCertificateValidForOrigin(spdy_session);
+  return spdy_session;
+}
+
+bool HttpStreamFactoryImpl::Job::ValidSpdySessionPool::
+    IsAlternativeCertificateValidForOrigin(
+        base::WeakPtr<SpdySession> spdy_session) {
+  return !is_spdy_alternate_ || !spdy_session ||

[Ryan Hamilton, 2015/04/13 18:29:56]

It seems like is_spdy_alternate_ is not adding any value here since it seems
like it's basically the same thing (effectively) as checking the hostname?

[Bence, 2015/04/15 21:07:46]

Per your other comment, I removed hostname checking.  Hostname checking was more
restrictive, since almost every unittest uses the same hostname (as changing
hostnames is not implemented yet, this very CL will enable it).

[Ryan Hamilton, 2015/04/15 21:24:34]

Can you add a comment about why you don't need to call
VerifyDomainAuthentication here when it's not a SPDY alt-svc job? (It's obvious
if you're familiar with the code, but not otherwise).

[Bence, 2015/04/23 17:17:07]

Done.

+         (origin_url_.host() == spdy_session->host_port_pair().host()) ||
+         spdy_session->VerifyDomainAuthentication(origin_url_.host());
+}
+
 ClientSocketPoolManager::SocketGroupType
 HttpStreamFactoryImpl::Job::GetSocketGroup() const {
   std::string scheme = origin_url_.scheme();