Change the bad-certificate handler for SSL (using NSS) to return an...

Change the bad-certificate handler for SSL (using NSS) to return an
error.

This requires a few additional changes in the rest of the code. In
particular, we now have to teach HttpNetworkTransaction about how to
restart connections with bad certificates. This was originally
intended to be done by ReconnectIgnoringLastError(), but that API
turns out be very difficult to implement in the SSLClientSocket. So,
instead, we just create a completely new SSLClientSocket.

We also have to be careful to store a copy of the certificate from
within the bad-certificate handler, as it won't be available by the
time GetSSLInfo() is called.

And we fix a bug that would cause us to erroneously talk SSL on
reconnected TCP sockets, even though we were still supposed to
negotiate a proxy tunnel first.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12809

[Markus (顧孟勤), 2009-03-12 01:28:49]

Based on the feedback that I got from you guys for
http://codereview.chromium.org/39284 I have come up with a new change list.

This change list is more elaborate, but it does allow us to return an error from
OwnBadCertHandler(). I believe, that's the more correct solution, as it prevents
the bad certificate or its associated SSL session id from ever getting cached.

Again, I would appreciate a careful code review, as I am still quite new to this
part of the code.

[wtc, 2009-03-12 18:19:03]

Markus, thanks for writing this CL.  I will review it this afternoon.

Re: failing unit tests: there are one or two SSL unit tests that
"overspecify" the expected behavior -- they require that the SSL
certificate error handling on Mac and Linux match that on Windows.
But it is actually OK for some of the "minor details" to differ
between these platforms as long as the Start() method returns the
expected certificate error code and you can call
ReconnectIgnoringLastError to set up a connection successfully.

[wtc, 2009-03-13 01:37:21]

Markus,

Sorry about the delay in reviewing this CL.  Good CL.
Unfortunately it forced me to think about some hard problems
in our Restart/Reconnect API, so I don't have answers to
those questions.  Also my comments below are a little
unorganized.  Please bear with me.

We can discuss the issues in detail tomorrow.

http://codereview.chromium.org/43115/diff/1/6
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/43115/diff/1/6#newcode154
Line 154: if (transport_->IsConnected()) {
Use IsConnectedAndIdle instead.  The reason is that if the
server closes the SSL connection gracefully, it will send
a close_notify alert message to us, which looks like data
received (hence not "idle") at the transport layer.

http://codereview.chromium.org/43115/diff/1/6#newcode160
Line 160: result = ERR_FAILED;
Please pick an appropriate error code from
net/base/net_error_list.h.  ERR_FAILED is our default error
code; it doesn't help us narrown down the possible causes of
the bug.

But it seems that the right thing to do in this case is
actually to restart the SSL connection setup by setting
next_state_ to the appropriate state (STATE_CONNECT?) and
set result to OK.

Have you tested the current code?  HttpNetworkTransaction
only calls this method when the socket connection is
disconnected, so it seems that this method will just
return ERR_FAILED.

http://codereview.chromium.org/43115/diff/1/6#newcode266
Line 266: ssl_info->cert = server_bad_cert_;
GetSSLInfo needs to set ssl_info->cert whether the server
cert is good or bad, so this statement needs to be moved
outside the if (server_cert_error_ != net::OK) block.

http://codereview.chromium.org/43115/diff/1/6#newcode433
Line 433: // Remember the bad certificate as it will no longer be accessible
after
To make GetSSLInfo work whether the server cert is good or
bad, I think we need to install our certificate
authentication callback using SSL_AuthCertificateHook:
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html#1088805

Our cert auth callback should simply call
SSL_PeerCertificate to save the cert in server_cert_,
and then call the default cert auth callback
SSL_AuthCertificate:
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslfnc.html#1088888

http://codereview.chromium.org/43115/diff/1/5
File net/base/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/43115/diff/1/5#newcode17
Line 17: #include "net/base/x509_certificate.h"
I wonder if we can get by with just a forward declaration of
class X509Certificate.  It works on Windows.

http://codereview.chromium.org/43115/diff/1/5#newcode85
Line 85: // Set when handshake fails.
Nit: add "because of a bad server cert".

http://codereview.chromium.org/43115/diff/1/5#newcode86
Line 86: scoped_refptr<X509Certificate> server_bad_cert_;
Nit: how about naming it server_cert_?  This way if we ever
need to store a good server cert, we won't need to rename
this member.

http://codereview.chromium.org/43115/diff/1/4
File net/http/http_cache.cc (right):

http://codereview.chromium.org/43115/diff/1/4#newcode946
Line 946: DCHECK(network_trans_ != NULL);
These two DCHECKs aren't necessary because the code will
crash immediately if either of these DCHECKs fails.

http://codereview.chromium.org/43115/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/43115/diff/1/2#newcode81
Line 81: ? STATE_WRITE_HEADERS : STATE_RECONNECT;
Nit: our local convention is to put an operator at the end
of a line when wrapping a long expression.  So the ? operator
should be at the end of the previous line.

This is the meat of your CL.  The more I think about it, the
more I think our design of the ReconnectIgnoringLastError
in the ClientSocket interface is wrong.  This is especially
true when an SSL tunnel is used and the tunnel has been
disconnected.  In this case, the tunnel cannot be
reconnected by connection_.socket().  The tunnel can only
be set up by this class (HttpNetworkTransaction) using
the HTTP CONNECT method.  You can test my theory by using
a proxy.

Why don't you make the other changes I suggested while I
think about this issue?

[wtc, 2009-03-17 23:32:16]

Markus,

Sorry about the delay.  Got two meetings today, which
interrupted my code review.

This looks much better.  I think one more pass and it'll be
ready.  I am most worried about the changes to
http_network_transaction.cc (we don't know how to reconnect
a tunnel and ignore last error) as that code is also used by
Windows.  We just released 2.0 Beta for Windows, so we can't
introduce a regression now.

http://codereview.chromium.org/43115/diff/3002/3010
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/43115/diff/3002/3010#newcode467
Line 467: error->ContinueRequest();
Could you explain why we need to call ContinueRequest
after AllowCertForHost?

Let's also reorder the CancelRequest and DenyCertForHost
calls above for consistency.

http://codereview.chromium.org/43115/diff/3002/3006
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/43115/diff/3002/3006#newcode111
Line 111: server_cert_(NULL),
Nit: the default scoped_refptr constructor will initialize
server_cert_ to NULL, so we don't need to have
server_cert(NULL) in the initializer list.

http://codereview.chromium.org/43115/diff/3002/3006#newcode169
Line 169: ignored_bad_server_cert_.clear();
It seems that this shouldn't be necessary.  Is it necessary
to clear ignored_bad_server_cert_ here, or is this defensive
programming?

http://codereview.chromium.org/43115/diff/3002/3006#newcode191
Line 191: ignored_bad_server_cert_.insert(server_cert_);
If we always clear ignored_bad_server_cert_ before inserting
a cert into it, ignored_bad_server_cert_ doesn't need to be
a std::set.  It can be just a scoped_refptr.

http://codereview.chromium.org/43115/diff/3002/3006#newcode208
Line 208: transport_send_busy_ = false;
FYI: in the net module we don't usually align the equal signs
in related assignments.  (Don't undo this since you've already
done the work.)

http://codereview.chromium.org/43115/diff/3002/3006#newcode212
Line 212: server_cert_error_   = 0;
Nit: use OK instead of 0.  Same value, but more readable.

http://codereview.chromium.org/43115/diff/3002/3006#newcode220
Line 220: // ReconnectIgnoringLastError() is supposed to only ignore the last
This is true, but I consider this just a nice-to-have.  If a
certificate has multiple errors (for example, untrusted issuer
and expired), it is fine for ReconnectIgnoringLastError to
succeed after we report the first error to the user.

http://codereview.chromium.org/43115/diff/3002/3006#newcode226
Line 226: SSL_InvalidateSession(nss_fd_);
Should we reset had_ignored_cert_ to false after invalidating
the SSL session?

http://codereview.chromium.org/43115/diff/3002/3006#newcode326
Line 326: ssl_info->cert = server_cert_;
Nit: you can skip the null check for server_cert_.

Style: in the net module, we have a local convention that
we don't use braces if there is only one statement in the
"if" body.  However, we always use braces when there is an
"else".   For example, we write:

  if (condition)
    foo();

and

  if (condition) {
    foo();
  } else {
    bar();
  }

http://codereview.chromium.org/43115/diff/3002/3006#newcode486
Line 486: // NSS calls this if an incoming certificate is invalid.
This comment needs to be updated.  You can say:
  // NSS calls this if an incoming certificate needs to be verified.

http://codereview.chromium.org/43115/diff/3002/3006#newcode490
Line 490: PRBool isServer) {
Nit: isServer => is_server

http://codereview.chromium.org/43115/diff/3002/3006#newcode498
Line 498: X509Certificate::SOURCE_FROM_NETWORK);
Nit: I have to admit I don't know how the style guide wants us
to format this assignment statement.  I'd format it like
this:

    that->server_cert_ = X509Certificate::CreateFromHandle(
        nss_cert, X509Certificate::SOURCE_FROM_NETWORK);

http://codereview.chromium.org/43115/diff/3002/3006#newcode516
Line 516: } else {
Style: the net module has another local convention that we
don't use else when the if block ends with a 'return' or
break/continue (in a loop).

http://codereview.chromium.org/43115/diff/3002/3005
File net/base/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/43115/diff/3002/3005#newcode69
Line 69: PRBool checksig, PRBool isServer);
Nit: let's rename isServer to follow our coding style: is_server.

http://codereview.chromium.org/43115/diff/3002/3005#newcode97
Line 97: std::set<scoped_refptr<X509Certificate> > ignored_bad_server_cert_;
Nit: perhaps this should be named ignored_bad_server_certs_,
plural, because it is a set of certs?

http://codereview.chromium.org/43115/diff/3002/3003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/43115/diff/3002/3003#newcode81
Line 81: STATE_WRITE_HEADERS : STATE_RECONNECT;
I think we need to handle the SSL tunnel case properly
before we check this in, otherwise there will be a regression.

This case is similar to the case that
DidDrainBodyForAuthRestart handles.  So I think we can
replace STATE_RECONNECT by STATE_INIT_CONNECTION.
However, in DoResolveHostComplete, we need to set
the next state to STATE_RECONNECT instead of STATE_CONNECT.
My thoughts aren't coherent here because I haven't thought
this through.

I think it'll take some time to sort this out.  To not block
your progress, we can put #if defined(OS_LINUX) around this
change.  Another (possibly better) option is to also check
using_tunnel_:

  // TODO(port): figure out how to reconnect when we're
  // using a tunnel.
  next_state_ = (connection_.socket()->IsConnected() || !using_tunnel_) ?
      STATE_WRITE_HEADERS : STATE_RECONNECT;

http://codereview.chromium.org/43115/diff/3002/3007
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/43115/diff/3002/3007#newcode260
Line 260: if (err_allowed) {
I think you meant if (!err_allowed) here, otherwise
the for loop is terminated after the first iteration.

It seems that your intention will be more clear if you use
an int i variable to control the termination of the for loop:

  bool err_allowed = true;
  for (int i = 0; i < 2; i++, err_allowed = !err_allowed) {

http://codereview.chromium.org/43115/diff/3002/3008
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/43115/diff/3002/3008#newcode78
Line 78: certificate_errors_(false),
Nit: how about renaming this have_certificate_errors_?

http://codereview.chromium.org/43115/diff/3002/3008#newcode163
Line 163: // Ignore SSL errors, we test the server is started and shut it down
by
This comment needs to be updated because you've modified the
code.

http://codereview.chromium.org/43115/diff/3002/3008#newcode181
Line 181: allow_certificate_errors_ = val; }
Nit: put the closing parenthesis on the next line.

[Markus (顧孟勤), 2009-03-19 03:12:11]

http://codereview.chromium.org/43115/diff/3002/3010
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/43115/diff/3002/3010#newcode467
Line 467: error->ContinueRequest();
On 2009/03/17 23:32:16, wtc wrote:
> Could you explain why we need to call ContinueRequest
> after AllowCertForHost?
> 
> Let's also reorder the CancelRequest and DenyCertForHost
> calls above for consistency.

Done.

http://codereview.chromium.org/43115/diff/3002/3006
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/43115/diff/3002/3006#newcode169
Line 169: ignored_bad_server_cert_.clear();
The unittest actually verifies that Connect() always fails on bad certificates,
no matter whether there have been any intermediate calls to
ReconnectIgnoringLastError(). It is possible that the unittest checks this
unintentionally, though. And I don't know whether this behavior is necessarily
the correct thing to do, but for now the unittests require it -- and the API
documentation in client_socket.h is a little vague on whether it is intentional.

Added a comment. So, at least it is clear to the reader why we are doing this.

http://codereview.chromium.org/43115/diff/3002/3006#newcode191
Line 191: ignored_bad_server_cert_.insert(server_cert_);
This is for chained certificates. Need to actually test, whether that works as
intended, though.

http://codereview.chromium.org/43115/diff/3002/3006#newcode220
Line 220: // ReconnectIgnoringLastError() is supposed to only ignore the last
I think you misunderstand what I was trying to say. But I am not sure how to
rephrase the comment.

I am trying to explain that our unittests verify that this sequence of calls
will complete:

CHECK(!Connect());      // Fails because of bad certificate
CHECK(ReconnectIgnoringLastError()); // Succeeds
CHECK(!Connect());      // Still fails

But if we don't invalidate the session, then the final call to Connect() will
actually succeed, as ReconnectIgnoringLastError() allowed the SSL session id to
become valid.

This makes the unittests fail, as they cannot test any further bad certificates
after the first call to ReconnectIgnoringLastError(). As I said earlier, that
might have been an unintentional side-effect in the unittest. If so, we need to
provide some other API for resetting the cache during tests.

http://codereview.chromium.org/43115/diff/3002/3006#newcode226
Line 226: SSL_InvalidateSession(nss_fd_);
Yes, that's probably a good idea. It did require moving a few other pieces of
code around a little bit.

http://codereview.chromium.org/43115/diff/3002/3007
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/43115/diff/3002/3007#newcode260
Line 260: if (err_allowed) {
On 2009/03/17 23:32:16, wtc wrote:
> I think you meant if (!err_allowed) here, otherwise
> the for loop is terminated after the first iteration.
> 
> It seems that your intention will be more clear if you use
> an int i variable to control the termination of the for loop:
> 
>   bool err_allowed = true;
>   for (int i = 0; i < 2; i++, err_allowed = !err_allowed) {

Done.

[dank, 2009-03-19 15:27:14]

Markus, this looks wonderful.

I didn't see a test of ReconnectIgnoringLastError
in net/base/ssl_client_socket_unittest.cc ?
Seems like we want one there...

[wtc, 2009-03-20 22:23:51]

Markus,

I just started to review your CL.  Sorry about the delay.
I've been busy with other code reviews and a 2.0 SSL
regression I'm struggling with.

I may not finish the review until Monday.  Just wanted to
give you a status update.  I will also try to test your CL
on Windows.  Thanks for tackling this hard and important
bug!

[Markus (顧孟勤), 2009-03-23 02:56:30]

I made a few more changes since last time. In particular, it turned out that
keeping the original code in HttpNetworkTransaction for switching between the
transport socket and the SSL socket was very awkward. It made the code very hard
to read, and I kept introducing subtle bugs when testing proxy operations.

I created a new object, that abstracts the concept of a stacked SSL client
socket which can be switched between TCP and SSL operations. This turns out to
be much cleaner and is easier to verify for correctness. Please review the API
and let me know, if this looks OK to you.

In the process, I did discover a bug in all the existing SSL backends that would
try to establish a new TCP connection without bothering to negotiate with the
proxy, whenever somebody calls SSLClientSocket::Connect() on a disconnected
socket. That is fixed, now.

I am not super happy with the new class hierarchies. SSLClientSocketStacker
should not be a subclass of the current SSLClientSocket class. Instead,
SSLClientSocket should be renamed SSLClientSocketImpl (or something similar),
and both SSLClientSocketImpl and SSLClientSocketStacker should inherit from a
new SSLClientSocket class that has a smaller API than the current class.

I didn't do any of that in this changelist as, a) I wanted to run it by you
first, and b) it requires renaming files in SVN. I am not sure, whether that is
OK or causes a lot of disruption for everybody.

Finally, I haven't had time testing operations with (bad) chained certificates,
yet. I suspect, we still have several corner cases in our SSL handling that need
tuning. But that can be done in separate changelists.


Markus

P.S.: Whew, this took most of the weekend to finish. It was much hairier than I
expected. I have a bunch of silly meetings on Monday. So, I probably won't be
able to work on this code for much of tomorrow, and I was hoping to get it done
for your final (?) review.

[wtc, 2009-03-24 00:36:46]

Hi Markus,

I will review your SSLClientSocketStacker API.

On 2009/03/23 02:56:30, Markus (顧孟勤) wrote:
> 
> In the process, I did discover a bug in all the existing SSL backends that
would
> try to establish a new TCP connection without bothering to negotiate with the
> proxy, whenever somebody calls SSLClientSocket::Connect() on a disconnected
> socket. That is fixed, now.

Nice catch, thanks.  I think we were relying on the fact that
TCPClientSocket::Connect() is a no-op if the TCPClientSocket is
already connected, and that's usually the case, which is why this
bug wasn't noticeable easily.

> Finally, I haven't had time testing operations with (bad) chained
certificates,
> yet.

I forgot to respond to this issue when you first raised this issue.
If an intermediate CA cert in the chain is bad, it causes the end
certificate to be bad, so it is sufficient to only with end
certificates when we do ReconnectIgnoringLastError.

[wtc, 2009-03-25 01:29:33]

Markus,

After thinking about this more and talking to Darin about
it, we'd like to propose this approach.

1. Remove the ReconnectIgnoringLastError method from the
ClientSocket interface.  We will reconnect by creating a
new client socket and connecting it.

2. In HttpNetworkTransaction::RestartIgnoringLastError,
if the connection is no longer alive, we will restart
using code similar to HandleSSLHandshakeError, whose
relevant code is reproduced here:

        ssl_config_.tls1_enabled = false;
        connection_.set_socket(NULL);
        connection_.Reset();
        next_state_ = STATE_INIT_CONNECTION;

We will need to replace
        ssl_config_.tls1_enabled = false;
by the code that instructs the new SSLClientSocket to
ignore errors with a particular certificate.

How do we do that?  Darin suggests that we stick the
certificate and the errors to ignore to ssl_config_,
which has the advantage of not having to add new parameters
to socket_factory_->CreateSSLClientSocket().

3. The remaining issue is who should stick the certificate
and the errors to ignore in ssl_config_.  There are two
options:
- We can pass a non-const pointer to ssl_config_, instead
  of a const reference to ssl_config_, to
  socket_factory_->CreateSSLClientSocket(), and let the
  SSLClientSocket stick the certificate and errors to
  ignore in ssl_config_; or
- we can let HttpNetworkTransaction stick the certificate
  and errors to ignore in ssl_config_ in
  HttpNetworkTransaction::HandleCertificateError(), after
  its ssl_socket->GetSSLInfo() call.  We can also do this
  elsewhere because response_.ssl_info has all the cert
  and its errors.
  
Do you have any questions about this proposal?

You may want to consider checking in an earlier version
of this CL first so that you have something working on
Linux. I think Patch Set 3, with #if defined(OS_LINUX)
around your change to
HttpNetworkTransaction::RestartIgnoringLastError(),
would be fine.

[Markus (顧孟勤), 2009-03-26 23:26:31]

I changed the code over to the new algorithm that you suggested. That helped
clean it up some, and I could remove a good amount of complexity and some
unneeded code.

[Markus (顧孟勤), 2009-03-27 22:03:20]

Sync'd to the head of the tree. The first time, that went wrong. So, had to
resubmit the changelist.

Also, found one potentially uninitialized variable that I accidentally
introduced in in ssl_client_socket_nss.cc. Fixed by initializing it to net::OK.

[wtc, 2009-03-27 22:30:27]

Markus,

I reviewed Patch Set 8, and only the most important files
http_network_transaction.{h,cc}.  Let me send you these
comments first.

I like your changes very much!  The new logic for connecting
and reconnecting is much clearer.

http://codereview.chromium.org/43115/diff/7001/7011
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/43115/diff/7001/7011#newcode86
Line 86: ResetStateForRestart();
Nit: if RestartIgnoringLastError is only used to ignore
an SSL certificate error, we don't need to call
ResetStateForRestart because none of those members reset
by ResetStateForRestart have been used when SSL connect
failed.

I think we need
  connection_.set_socket(NULL);
otherwise, the disconnected socket will be returned to
the connection pool.  The connection poll will discovered
that the socket is disconnected and throw it away anyway,
but it's best if we can avoid this extra work.

http://codereview.chromium.org/43115/diff/7001/7011#newcode426
Line 426: TRACE_EVENT_BEGIN(using_tunnel_ ? "http.ssl_tunnel" : "http.ssl",
I think it's fine to just have one event "http.ssl_connect".
Whether SSL is running over a plain TCP connection or a
proxy tunnel isn't important for this state.

http://codereview.chromium.org/43115/diff/7001/7011#newcode605
Line 605: // If we are using a direct SSL connection, then go ahead and create
the SSL
This comment needs to be updated to reflect the new code,
which doesn't create the SSL wrapper socket here.  You
can change "create the SSL wrapper socket" to "establish
the SSL connection".

http://codereview.chromium.org/43115/diff/7001/7011#newcode607
Line 607: if (using_ssl_ && !using_tunnel_)
There are two problems.

1. We should only proceed to DoSSLConnect() if result is OK.
You can move this into the if (result == OK) clause below.

2. For style reasons, don't call DoSSLConnect() directly.
Instead, set next_state_ to STATE_SSL_CONNECT and return OK.

http://codereview.chromium.org/43115/diff/7001/7011#newcode1145
Line 1145: ssl_config_.acceptable_bad_certs_.insert(response_.ssl_info.cert);
Nit: elsewhere in our source tree, we use the term "allow a
cert".  So let's rename this field "allowed_bad_certs_" to
be consistent.

On first sight, it is surprising that we insert the cert
into "allowed_bad_cert_" before consulting the user,
but this code is correct.  So I think a short comment would
be appropriate.

http://codereview.chromium.org/43115/diff/7001/7012
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/43115/diff/7001/7012#newcode75
Line 75: int DoConnect();
I suggest that we rename these methods DoTCPConnect,
DoTCPConnectComplete, and the corresponding states
STATE_TCP_CONNECT and STATE_TCP_CONNECT_COMPLETE,
to make them more distinguishable from DoSSLConnect.

[wtc, 2009-03-27 23:01:20]

OK, I have now reviewed all the files.  Very nice
cleanup.  (I didn't review the changes to
http_network_transaction_unittest.cc carefully.)

http://codereview.chromium.org/43115/diff/8019/7046
File net/base/ssl_client_socket.h (right):

http://codereview.chromium.org/43115/diff/8019/7046#newcode12
Line 12: struct SSLConfig;
Remove the forward declaration of struct SSLConfig
because it is not used by this header.

http://codereview.chromium.org/43115/diff/8019/7047
File net/base/ssl_client_socket_mac.cc (left):

http://codereview.chromium.org/43115/diff/8019/7047#oldcode454
Line 454: return transport_->Connect(&io_callback_);
A better fix is to get rid of DoConnect and
DoConnectComplete.  We don't need these two
states any more.

http://codereview.chromium.org/43115/diff/8019/7047
File net/base/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/43115/diff/8019/7047#newcode291
Line 291: if (transport_ != NULL)
Is this null check necessary?

transport_ can only be NULL if we pass a NULL transport_socket
to the constructor, right?

I don't mind adding a null check; I just wanted to know if
you actually experienced a crash because of null pointer
deferencing here.

(We could also add a DCHECK(transport_) to the constructor.

http://codereview.chromium.org/43115/diff/8019/7049
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/43115/diff/8019/7049#newcode202
Line 202: if (transport_ != NULL) {
Same comment about the null check for transport_ here.

http://codereview.chromium.org/43115/diff/8019/7049#newcode204
Line 204: transport_.reset(NULL);
Why do we need to reset transport_?  Perhaps this is why
you need to add a null check for transport_?

http://codereview.chromium.org/43115/diff/8019/7050
File net/base/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/43115/diff/8019/7050#newcode49
Line 49: X509Certificate *server_cert();
Nit: put * next to X509Certificate.

We use xxx_yyy() names only for simple accessor methods.
But why do we need a private getter?

http://codereview.chromium.org/43115/diff/8019/7051
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/43115/diff/8019/7051#newcode83
Line 83: rv = transport->Connect(&callback);
Just a comment: it does seem more cumbersome that we have
to manually do TCP connect now.  I can imagine if someone
were to use our network stack for non-HTTP (so that proxy
tunnels are not a concern), the original way of doing
SSL connect would be more convenient.

http://codereview.chromium.org/43115/diff/8019/7052
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/43115/diff/8019/7052#newcode449
Line 449: // The caller has to make sure that the transport socket is connected.
If
Same comment here -- we should eliminate DoConnect and
DoConnectComplete states.  Feel free to do that after this
CL is checked in.  (Don't want this CL to be blocked forever.)

http://codereview.chromium.org/43115/diff/8019/7059
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/43115/diff/8019/7059#newcode10
Line 10: #include "base/linked_ptr.h"
I don't see linked_ptr used in this header.  Should
we remove the "base/linked_ptr.h" inclusion?

http://codereview.chromium.org/43115/diff/8019/7059#newcode16
Line 16: class X509Certificate;
If this forward declaration is sufficient, remove
the "net/base/x509_certificate.h" inclusion above.
Otherwise remove the forward declaration.

http://codereview.chromium.org/43115/diff/8019/7059#newcode33
Line 33: std::set<scoped_refptr<X509Certificate> > acceptable_bad_certs_;
Please add a comment to define allowed_bad_certs_.

[Markus (顧孟勤), 2009-03-28 02:06:55]

I believe, I addressed all of your comments. Would you like to take another
look?

I will do the removal of the unneeded state in a separate changelist.

I also found the elusive unittest failure that only triggered on Windows. It was
a rather subtle bug in the unittest itself, where we would overrun the end of an
array of MockReads, but on Linux we would reliably find good data that just
happened to be exactly what we needed. Added an explicit "MockRead(false,
net::OK)" and fixed the typo that was the root cause (a missing "\r\n").

[wtc, 2009-03-30 18:18:56]

LGTM.

I reviewed the diffs between Patch Set 8 and Patch Set 10,
and the changes to http_network_transaction.{h,cc}.

Please fix the nits below before you check this in.

I have three comments on http_network_transaction_unittest.cc
below.  If you'd like to address them, you can do that in a
follow-up CL.

I am very happy with the end result.  The SSL-related code
in HttpNetworkTransaction is much simpler now, and the
duplicate code between the "direct SSL" and "SSL over a
tunnel" cases has been eliminated.

http://codereview.chromium.org/43115/diff/8022/7082
File net/base/ssl_config_service.h (right):

http://codereview.chromium.org/43115/diff/8022/7082#newcode31
Line 31: // trigger an ERR_CERT_*_INVALID error when calling
SSLClientSocket::Connect.
Nit: the error should be just ERR_CERT_* because not all
certificate error codes end in _INVALID.  Alternatively you
can just say "a certificate error".

http://codereview.chromium.org/43115/diff/8022/7077
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/43115/diff/8022/7077#newcode417
Line 417: TRACE_EVENT_BEGIN("http.connect", request_, request_->url.spec());
Nit: Should we rename this event "http.tcp_connect"?

I hope there are no scripts that depend on the current event
name "http.connect".

http://codereview.chromium.org/43115/diff/8022/7077#newcode422
Line 422: TRACE_EVENT_END("http.connect", request_, request_->url.spec());
Nit: Should we rename this event "http.tcp_connect"?

http://codereview.chromium.org/43115/diff/8022/7077#newcode604
Line 604: // SSL connection, now.  Otherwise, we need to first issue a CONNECT
request.
Nit: remove the comma (,) before "now".

http://codereview.chromium.org/43115/diff/8022/7079
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/43115/diff/8022/7079#newcode32
Line 32: MockConnect(bool a, int r) : async(a), result(r) { }
You may have intentionally named the constructor's parameters
'a' and 'r', right?  I also don't like constructor parameters
having the same names as the members.  I'm surprised that
the initializer list
    : async(async), result(result)
actually works.

http://codereview.chromium.org/43115/diff/8022/7079#newcode69
Line 69: MockSocket(MockRead* r, MockWrite* w) : reads(r), writes(w) { }
Not sure if we need this constructor, but it's ok.

http://codereview.chromium.org/43115/diff/8022/7079#newcode2949
Line 2949: mock_sockets[0] = &ssl_bad_certificate;
It's a little confusing for a socket to be named "ssl_bad_certificate".