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..f47881fe74408811bbf409fc6b2b400967b8f559 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -292,6 +292,18 @@ bool HttpStreamFactoryImpl::Job::CanUseExistingSpdySession() const {
          proxy_info_.proxy_server().is_https() || IsSpdyAlternate();
 }
 
+bool HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin(
+    base::WeakPtr<SpdySession> spdy_session) {
+  DCHECK(spdy_session);
+  if (!using_ssl_) {
+    return true;

[Ryan Hamilton, 2015/04/10 18:37:37]

We only speak SPDY over TLS, right? I think this check is redundant. If not, can
you explain why not?

[Bence, 2015/04/10 19:55:16]

Well, not quite, this member was called for non-ssl Jobs too.  I replaced this
with an if (!IsSpdyAlternate()) return true; statement, which is closer to our
intentions, so you are right, this check is not necessary.

+  }
+  if (origin_url_.host() == spdy_session->host_port_pair().host()) {

[Ryan Hamilton, 2015/04/10 18:37:36]

It seems that this is simply a performance optimization to avoid calling
VerifyDomainAuthentication. Is that right? If so, I wonder if we can push this
logic down to VerifyDomainAuthentication and avoid doing this here, hence being
able to get rid of this method.

[Bence, 2015/04/10 19:55:16]

Not exactly.  I added this test here because otherwise some unittests (for
example, NextProto/HttpNetworkTransactionTest.UseAlternateProtocolForNpnSpdy as
of Patch Set 3, and
NextProto/HttpNetworkTransactionTest.SpdyPreconnectErrorNotConnectedOnWrite as
of Patch Set 2) fail, because they provide a dummy SSLSocketDataProvider with no
actual certificate.  That begin said, I would not mind modifying these tests, or
moving this check from
HttpStreamFactoryImpl::Job::IsAlternativeCertificateValidForOrigin to
SpdySession::VerifyDomainAuthentication.

[Ryan Hamilton, 2015/04/11 02:40:41]

Ah. I'm not a big fan of having "real" code that's only there for tests, so I
think that tweaking those tests seems like the right idea.

[Bence, 2015/04/13 17:52:40]

I totally agree with you.  It turns out that there are much more tests doing
this that what I thought.  Most unittests do not bother to create an actual
cert.  (I think the only way to do it is to read a .pem file from disk, which
would be too much of a performance hit for that many tests.)  Also note that
this hostname check only delays this problem, because there will be a lot more
ALTSVC tests with different hosts.  Maybe we need a bool member |check_cert_| in
SSLInfo or SSLSocketDataProvider that would be set to false for unittests that
do not care, how does that sound?

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

It seems like it should be pretty easy to add a helper method to, say,
spdy_test_util_common.h which loads a well defined cert so we could just say:

  SSLSocketDataProvider ssl(ASYNC, OK);
  ssl.cert = LoadDefaultTestCert();
  session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl);

So it'd basically be just a 1 line add to the tests which do "the wrong thing".
Does that sound plausible?

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

Done.

+    return true;
+  }
+  return spdy_session->VerifyDomainAuthentication(origin_url_.host());
+}
+
 void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
   DCHECK(stream_.get());
   DCHECK(!IsPreconnecting());
@@ -457,7 +469,8 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
     return ERR_IO_PENDING;
   }
 
-  if (IsCertificateError(result)) {
+  if (IsCertificateError(result) &&
+      result != ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN) {

[Ryan Hamilton, 2015/04/10 18:37:37]

Yeah, definitely move this error out of the certificate error section.

[Bence, 2015/04/10 19:55:16]

Done.

     // Retrieve SSL information from the socket.
     GetSSLInfo();
 
@@ -535,6 +548,20 @@ int HttpStreamFactoryImpl::Job::RunLoop(int result) {
       }
       return ERR_IO_PENDING;
 
+    case ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN:

[Ryan Hamilton, 2015/04/10 18:37:36]

This should only happen for a job that IsAlternate(), right? If so, can you
DCHECK() this?

[Bence, 2015/04/10 19:55:16]

This wasn't actually true, but yes, now it can only happen for a Spdy alternate
Job.

+      if (job_status_ != STATUS_BROKEN) {
+        DCHECK_EQ(STATUS_RUNNING, job_status_);
+        job_status_ = STATUS_FAILED;
+        // TODO(bnc): Instead of marking alternative service broken, mark
+        // (origin, alternative service) couple as invalid.

[Ryan Hamilton, 2015/04/10 18:37:37]

Is this a TODO (instead of being implemented) because we don't have the right
method on HttpServerProperties?

[Bence, 2015/04/10 19:55:16]

Correct.

+        MaybeMarkAlternativeServiceBroken();
+      }
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&Job::OnStreamFailedCallback, ptr_factory_.GetWeakPtr(),
+                     ERR_CONNECTION_REFUSED));

[Ryan Hamilton, 2015/04/10 18:37:37]

Why ERR_CONNECTION_REFUSED? Actually, this looks a lot like the default: case
below. Is it different?

[Bence, 2015/04/10 19:55:16]

In fact, you are right, this is a lot like the default case.  Note, however,
that if I return |result| here, then the error codes surfacing to the unittest
in the new connection and pooling case are different.

[Ryan Hamilton, 2015/04/11 02:40:41]

Is there a difference between the two cases? (default: vs
ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN:)

[Bence, 2015/04/13 17:52:40]

Only the DCHECK (and the TODO), so I merged them for now.

+      return ERR_IO_PENDING;
+
     default:
       if (job_status_ != STATUS_BROKEN) {
         DCHECK_EQ(STATUS_RUNNING, job_status_);
@@ -804,6 +831,9 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
       session_->spdy_session_pool()->FindAvailableSession(
           spdy_session_key, net_log_);
   if (spdy_session && CanUseExistingSpdySession()) {
+    if (!IsAlternativeCertificateValidForOrigin(spdy_session)) {

[Ryan Hamilton, 2015/04/10 18:37:37]

We don't need to do this call, unless this job is a SPDY Alt-Svc job to a
different hostname, right?

[Bence, 2015/04/10 19:55:16]

That is true.  I think it might be easier to check that in
IsAlternativeCertificateValidForOrigin().  Maybe we need to find a better name
for this method.

+      return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN;
+    }
     // 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())
@@ -812,14 +842,14 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
     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;
@@ -989,6 +1019,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;
   }
@@ -1131,6 +1164,9 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
       spdy_pool->FindAvailableSession(spdy_session_key, net_log_);
 
   if (spdy_session) {
+    if (!IsAlternativeCertificateValidForOrigin(spdy_session)) {
+      return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN;
+    }
     return SetSpdyHttpStream(spdy_session, direct);
   }
 
@@ -1146,6 +1182,10 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     return ERR_SPDY_INADEQUATE_TRANSPORT_SECURITY;
   }
 
+  if (!IsAlternativeCertificateValidForOrigin(spdy_session)) {
+    return ERR_ALTERNATIVE_CERT_NOT_VALID_FOR_ORIGIN;

[Ryan Hamilton, 2015/04/10 18:37:37]

Hm. It seems like there are a lot of different places that we need to perform
this check. That feels a bit error prone to me. I'm not sure what the solution
is, but it feels like there's room to clean this up.

[Bence, 2015/04/10 19:55:16]

I do agree with you.  Please let me know if you have any suggestions.

[Ryan Hamilton, 2015/04/11 02:40:41]

So it looks like each time we get a SPDY session from the session pool, we need
to verify that it's usable. So that means the calls to FindAvailableSession and
CreateAvailableSessionFromSocket. One option would be to create a wrapper class
(in an anon name space at the top of the .cc file?) which is constructed with
the origin and the real spdy session pool. Then it exposes just the two methods
you need, something like

int FindAvailableSession(SpdySessionKey, NetLog, SpdySession*)

Then we would use only this wrapped pool, not the real pool. What do you think?

[Bence, 2015/04/13 17:52:40]

I've done it, but it doesn't quite feel right.  There is too much dependency,
like the ValidSpdySessionPool instance has three members that mirror Job
members, so it can only be instantiated once there members are calculated, and
it is a latent invariant that they should not diverge between Job and
ValidSpdySessionPool.  Maybe FindAvailableSession,
CreateAvailableSessionFromSocket, and IsAlternativeCertificateValidForOrigin
should just be private methods of Job, what do you think?

+  }
+
   new_spdy_session_ = spdy_session;
   spdy_session_direct_ = direct;
   const HostPortPair& host_port_pair = spdy_session_key.host_port_pair();