SSL: remove NOTREACHED from GetNextProto error path.

SSL: don't ask for next proto if we got an error.

GetNextProto only returns an error if the NSPR socket passed in is not a valid
SSL socket. However, this happens in tests (see the bug).

It appears that in some code paths the socket is invalid by the time that we
call HttpNetworkTransaction::DoSSLConnectComplete.

This patch also changes the logic such that we will only use SPDY if the server actually advertises support for it. Previously we would also use it in the case that the server advertised a disjoint set of protocols.

BUG=31611

[wtc, 2010-01-05 21:58:14]

We should keep this NOTREACHED() assertion because it
detected a bug.

The bug is in HttpNetworkTransaction::DoSSLConnectComplete.
It should call ssl_socket->GetNextProto(&proto) only if
'result' is 'OK'.

[wtc, 2010-01-05 22:01:12]

HttpNetworkTransaction::DoSSLConnectComplete gets called
when SSL connect has completed, and it may have completed
with a failure ('result' < 0).  This is why the socket
(when cast to ssl_socket) may be invalid when we call
HttpNetworkTransaction::DoSSLConnectComplete.

[agl, 2010-01-06 02:08:20]

PTAL

[wtc, 2010-01-06 02:11:37]

Is Patch Set 2 complete?  It removes our only
GetNextProto call.

[agl, 2010-01-06 02:24:32]

I didn't notice another hunk in the same function. (And my compile failed
shortly after too.)

[wtc, 2010-01-06 02:37:09]

LGTM.

Do we not have to ignore certificate errors for SPDY now?
If we still have to do that, we can call GetNextProto if
result == OK || IsCertificateError(result).

[agl, 2010-01-06 02:54:25]

On 2010/01/06 02:37:09, wtc wrote:
> Do we not have to ignore certificate errors for SPDY now?

We shouldn't have to, although I'm going to check with willchan first in case
there's a testing setup that I'm going to break.

> If we still have to do that, we can call GetNextProto if
> result == OK || IsCertificateError(result)

I don't believe that we can call GetNextProto in the case that result != OK and
IsCertificateError(result). That was the case that was crashing. We would have
to handle it in the usual manner, in the callback.

[willchan no longer on Chromium, 2010-01-07 22:17:18]

On Tue, Jan 5, 2010 at 6:54 PM,  <agl@chromium.org> wrote:
> On 2010/01/06 02:37:09, wtc wrote:
>>
>> Do we not have to ignore certificate errors for SPDY now?
>
> We shouldn't have to, although I'm going to check with willchan first in
> case
> there's a testing setup that I'm going to break.

I think this will break if you run a local gfe since we don't generate
certificates for them.

>
>> If we still have to do that, we can call GetNextProto if
>> result == OK || IsCertificateError(result)
>
> I don't believe that we can call GetNextProto in the case that result != OK
> and
> IsCertificateError(result). That was the case that was crashing. We would
> have
> to handle it in the usual manner, in the callback.
>
>
>
> http://codereview.chromium.org/519048
>

[agl, 2010-01-07 23:45:15]

On Thu, Jan 7, 2010 at 2:17 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> I think this will break if you run a local gfe since we don't generate
> certificates for them.

How about if I add a --ignore-certs flag for this use case? Would that
be enough?

(I've never run a local GFE I'm afraid.)


AGL

[willchan no longer on Chromium, 2010-01-07 23:48:49]

On Thu, Jan 7, 2010 at 3:44 PM, Adam Langley <agl@chromium.org> wrote:
> On Thu, Jan 7, 2010 at 2:17 PM, William Chan (陈智昌)
> <willchan@chromium.org> wrote:
>> I think this will break if you run a local gfe since we don't generate
>> certificates for them.
>
> How about if I add a --ignore-certs flag for this use case? Would that
> be enough?

Idea seems fine to me.

>
> (I've never run a local GFE I'm afraid.)
>
>
> AGL
>

[agl, 2010-01-08 01:41:42]

PTAL

[willchan no longer on Chromium, 2010-01-08 02:51:39]

http://codereview.chromium.org/519048/diff/6002/6003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/519048/diff/6002/6003#newcode841
chrome/browser/browser_init.cc:841: if
(command_line.HasSwitch(switches::kIgnoreCertErrors)) {
I think we usually drop the braces for one liners like this.

http://codereview.chromium.org/519048/diff/6002/6006
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/519048/diff/6002/6006#newcode801
net/http/http_network_transaction.cc:801: result =
HandleCertificateError(result);
I'm not sure if this code change is valid.  Won't result != OK if we talk to a
gfe without a valid cert?  It won't invoke the delegate which I think leads to
the UI showing the bad cert, but we won't enter the if (result == OK) condition
below, which means we will fail to connect.  Does that sound right?  Would you
like me to bring up a gfe for you to test against?

[agl, 2010-01-08 03:17:09]

http://codereview.chromium.org/519048/diff/6002/6006
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/519048/diff/6002/6006#newcode801
net/http/http_network_transaction.cc:801: result =
HandleCertificateError(result);
On 2010/01/08 02:51:39, willchan wrote:
> I'm not sure if this code change is valid.  Won't result != OK if we talk to a
> gfe without a valid cert?  It won't invoke the delegate which I think leads to
> the UI showing the bad cert, but we won't enter the if (result == OK)
condition
> below, which means we will fail to connect.  Does that sound right?  Would you
> like me to bring up a gfe for you to test against?

Hmm, you might be right. In which case we should fix the result == OK branch: we
want to be able to click through a cert warning on a SPDY site and continue
talking SPDY, no?

I'll take a look tomorrow. I think I can simulate a GFE well enough with a
patched openssl. If not, I'll bug you for instructions.

Cheers


AGL

[wtc, 2010-01-08 03:51:15]

Adam: I propose a different solution.  Please see my
second comment below.

http://codereview.chromium.org/519048/diff/6002/6003
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/519048/diff/6002/6003#newcode842
chrome/browser/browser_init.cc:842: URLRequestHttpJob::IgnoreCertErrors(true);
URLRequestHttpJob is a derived class of URLRequestJob, and I
think it's considered an implementation detail of the network
stack.  URLRequestHttpJob should not be exposed outside the
net directory like this.

http://codereview.chromium.org/519048/diff/6002/6006
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/519048/diff/6002/6006#newcode801
net/http/http_network_transaction.cc:801: result =
HandleCertificateError(result);
For a fair comparison of the performances of SPDY and HTTP,
we should ignore certificate errors for SPDY here.

I think the key to the fix for this bug is in whether
we can call ssl_socket->GetNextProto when result != OK &&
IsCertificateError(result).  You believe we can't because
this is why those SSL UI tests hit the NOTREACHED() assertion
failures.  But based on my understanding of the code,
it should be safe to call ssl_socket->GetNextProto when
we get a certificate error.  I am wondering if 'result'
is actually a non-certificate error in those SSL UI tests,
because at the end of the test, the test also connects to
the test server and expects a connection failure.  Is it
possible that the following events happened?

  1. The UI test opened a TCP connection to the test server
  2. The test server shut down, closing the TCP connection
  3. The UI test tried to do SSL connect and the
    transport_->GetPeerName() call in SSLClientSocketNSS
    failed.  Would getpeername fail with ENOTCONN
    under this condition?  Note that we would not have
    called SSL_ImportFD to convert nss_fd_ to an NSS SSL
    socket.
  4. We called HttpNetworkTransaction::DoSSLConnectComplete
    with result=ERR_UNEXPECTED.

We can try changing the following DLOG(ERROR) in
SSLClientSocketNSS::InitializeSSLOptions to NOTREACHED()
and see if the SSL UI tests hit that NOTREACHED() instead:

  int err = transport_->GetPeerName((struct sockaddr *)&peername, &len);
  if (err) {
    DLOG(ERROR) << "GetPeerName failed";
    // TODO(wtc): Change GetPeerName to return a network error code.
    return ERR_UNEXPECTED;
  }

In any case, unless we call SSL_ImportFD, nss_fd_ is
not an NSS SSL socket, so we can change the GetNextProto
method to return immediately if it detects that SSL_ImportFD
hasn't been called successfully.
also

[wtc, 2010-01-08 18:46:58]

On 2010/01/06 02:54:25, agl wrote:
>
> I don't believe that we can call GetNextProto in the case that result != OK
and
> IsCertificateError(result). That was the case that was crashing. We would have
> to handle it in the usual manner, in the callback.

Continuing my previous comment: if we get a certificate error,
the SSL_ImportFD call in SSLClientSocketNSS must have succeeded,
so nss_fd_ has been converted into an NSS SSL socket and we should
be able to call SSL_GetNextProto.

[wtc, 2010-01-08 18:53:24]

Let's try something like this for now to ignore
cert errors for SPDY.  Eventually we should use
Patch Set 3.

In HttpNetworkTransaction::DoSSLConnectComplete, change

  SSLClientSocket::NextProtoStatus status = ssl_socket->GetNextProto(&proto);

to

  SSLClientSocket::NextProtoStatus status =
SSLClientSocket::kNextProtoUnsupported;
  if (result == OK || IsCertificateError(result))
    status = ssl_socket->GetNextProto(&proto);

[agl, 2010-01-11 23:26:53]

Ok. I don't like that SPDY is currently ignoring certs for https URLs, but we
can deal with that later.

[wtc, 2010-01-11 23:45:54]

LGTM.

[wtc, 2010-01-11 23:55:49]

http://codereview.chromium.org/519048/diff/12001/12002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/519048/diff/12001/12002#newcode817
net/http/http_network_transaction.cc:817: // We currently ignore certificate
errors for spdy.
Could you add a TODO comment here so we remember to remove
this at the appropriate time?  Please reference
http://crbug.com/32020.