Reintegrate certificate selection in HttpNetworkTransaction DoLoop

Reintegrate certificate selection in HttpNetworkTransaction DoLoop

The HttpNetworkTransaction refactor intercepts the client auth
handling and moves it out of DoLoop. Because HandleCertificateRequest
often switches states, this caused a DCHECK and crash in some
circumstances.

This reintegrates it and adds unit tests to catch the DCHECK. We really
want to test sending a legitimate certificate, as well as more
checking interesting errors, but we cannot import temporary keys yet.

We also add a patch for tlslite to send a non-empty certificate_types.
Apple's SSL implementation raises a protocol error otherwise.

BUG=52744, 51132, 52778
TEST=SSLClientSocketTest.ConnectClientAuth*,URLRequestTest.ClientAuthTest

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

[davidben, 2010-08-19 20:16:20]

URLRequestTest.ClientAuthTest currently does not pass.

See http://crbug.com/52744

[wtc, 2010-08-19 22:02:52]

Please have mbelshe review the one-line change in
http_network_transaction.cc.

I reviewed the other two files.  They look good to me.
Two nits below.

http://codereview.chromium.org/3141026/diff/3001/4002
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/3141026/diff/3001/4002#newcode214
net/socket/ssl_client_socket_unittest.cc:214: TEST_F(SSLClientSocketTest,
ConnectClientAuthEmptyCert) {
I find NoCert and EmptyCert very confusing.  How about
CertRequested and SendNullCert?

At least add a comment to point out the differences between
these two tests (line 231, ssl_config.send_client_cert is
set to true, and line 245, the expected rv of sock->Connect()
is different).  These tests contain a lot of similar boilerplate
code, so a comment can help people see the differences.

http://codereview.chromium.org/3141026/diff/3001/4003
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/3141026/diff/3001/4003#newcode450
net/url_request/url_request_unittest.cc:450: // - Sending a non-empty
certificate back.
Nit: remove "non-empty"; it's confusing.

[davidben, 2010-08-19 23:07:27]

http://codereview.chromium.org/3141026/diff/3001/4002
File net/socket/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/3141026/diff/3001/4002#newcode214
net/socket/ssl_client_socket_unittest.cc:214: TEST_F(SSLClientSocketTest,
ConnectClientAuthEmptyCert) {
On 2010/08/19 22:02:52, wtc wrote:
> I find NoCert and EmptyCert very confusing.  How about
> CertRequested and SendNullCert?
> 
> At least add a comment to point out the differences between
> these two tests (line 231, ssl_config.send_client_cert is
> set to true, and line 245, the expected rv of sock->Connect()
> is different).  These tests contain a lot of similar boilerplate
> code, so a comment can help people see the differences.

Done.

http://codereview.chromium.org/3141026/diff/3001/4003
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/3141026/diff/3001/4003#newcode450
net/url_request/url_request_unittest.cc:450: // - Sending a non-empty
certificate back.
On 2010/08/19 22:02:52, wtc wrote:
> Nit: remove "non-empty"; it's confusing.

Done.

[davidben, 2010-08-20 02:56:33]

Mike: Wan-Teh and I looked over this code and the related crash. It seems I
split the bugs too early. This version should take care of the problems.

[Mike Belshe, 2010-08-20 03:29:07]

LGTM -- thanks for picking this up.

Go automated tests :-)

http://codereview.chromium.org/3141026/diff/17001/18001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3141026/diff/17001/18001#newcode582
net/http/http_network_transaction.cc:582: result =
HandleCertificateRequest(result);
nit:  so should we set next_state_ = STATE_NONE just before line 582? 
HandleCertificateRequest *should* set the state, right?  We could dcheck after
582 that either result is still an error or that next_state_ != STATE_NONE.

[wtc, 2010-08-20 18:39:29]

LGTM.

http://codereview.chromium.org/3141026/diff/17001/18001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3141026/diff/17001/18001#newcode582
net/http/http_network_transaction.cc:582: result =
HandleCertificateRequest(result);
DoLoop() sets next_state_ = STATE_NONE before calling any
DoFooState() function, so we don't need to set
next_state_ = STATE_NONE in this function.

http://codereview.chromium.org/3141026/diff/17001/18001#newcode1019
net/http/http_network_transaction.cc:1019: // session cache.
Nit: session cache => SSL session cache

"session" has several meanings in the network stack, so we
need to be specific.

[davidben, 2010-08-20 20:16:58]

Alright. Here's a new revision that also adds a patch for tlslite. The tests
were failing on the Mac try bots because Apple's SSL implementation rejects
empty certificate_types field in the CertificateRequest.

    charPtr = message.data;
    typeCount = *charPtr++;
    if (typeCount < 1 || message.length < 3 + typeCount) {
        sslErrorLog("SSLProcessCertificateRequest: length decode error 2\n");
        return errSSLProtocol;
    }
    for (i = 0; i < typeCount; i++)
    {   if (*charPtr++ == 1)
            ctx->x509Requested = 1;
    }

Arguably the correct response there is to send no certificate, but whatever. (As
an aside: does /no one/ pay attention to the actual contents of that thing on
the client? :-D)

http://codereview.chromium.org/3141026/diff/17001/18001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3141026/diff/17001/18001#newcode1019
net/http/http_network_transaction.cc:1019: // session cache.
On 2010/08/20 18:39:29, wtc wrote:
> Nit: session cache => SSL session cache
> 
> "session" has several meanings in the network stack, so we
> need to be specific.

Done.

[wtc, 2010-08-20 20:50:41]

LGTM++ Excellent conclusion of your internship!

http://codereview.chromium.org/3141026/diff/26001/19005
File third_party/tlslite/README.chromium (right):

http://codereview.chromium.org/3141026/diff/26001/19005#newcode13
third_party/tlslite/README.chromium:13: SSL implementation rejects an empty list
and raises an SSL protocol error.
Nit: SSL implementation => Secure Transport library

http://codereview.chromium.org/3141026/diff/26001/19008
File third_party/tlslite/tlslite/messages.py (right):

http://codereview.chromium.org/3141026/diff/26001/19008#newcode349
third_party/tlslite/tlslite/messages.py:349: #Apple's implementation rejects
empty certificate_types, so
Nit: Apple's implementation => Apple's Secure Transport library

[wtc, 2010-08-20 20:58:57]

On 2010/08/20 20:16:58, David Benjamin wrote:
>
> Arguably the correct response there is to send no certificate, but whatever.
(As
> an aside: does /no one/ pay attention to the actual contents of
[certificate_types
> field in the CertificateRequest] on the client? :-D)

NSS doesn't return the certificate_types field to the
client auth callback.  I remember SChannel doesn't return
that info to the caller either.

[davidben, 2010-08-21 05:30:32]

Tree is closed right now, so I'll go ahead and update the CL. I rebased on top
of trunk (only affects the README file) and addressed the nits below.

The previous try bot runs should still apply, so I guess I'll land this sometime
tomorrow, assume the tree is open.

http://codereview.chromium.org/3141026/diff/26001/19005
File third_party/tlslite/README.chromium (right):

http://codereview.chromium.org/3141026/diff/26001/19005#newcode13
third_party/tlslite/README.chromium:13: SSL implementation rejects an empty list
and raises an SSL protocol error.
On 2010/08/20 20:50:41, wtc wrote:
> Nit: SSL implementation => Secure Transport library

Done.

http://codereview.chromium.org/3141026/diff/26001/19008
File third_party/tlslite/tlslite/messages.py (right):

http://codereview.chromium.org/3141026/diff/26001/19008#newcode349
third_party/tlslite/tlslite/messages.py:349: #Apple's implementation rejects
empty certificate_types, so
On 2010/08/20 20:50:41, wtc wrote:
> Nit: Apple's implementation => Apple's Secure Transport library

Done.