Add tests for TLS fallback on connection reset and close.

Add tests for TLS fallback on connection reset and close.

The reset tests currently fail on OpenSSL and do not work on Android. But
otherwise this gives us slightly better test coverage here.

BUG=372849
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280188

[davidben, 2014-06-18 22:34:28]

Handing this over to you, Wan-Teh, to not tax Ryan's review bandwidth.

This is split off of https://codereview.chromium.org/280853002/. Ryan suggested
I split it up and I agree. So here's the first part. It just patches tlslite and
adds a bunch of tests. One of the tests does not pass right now in OpenSSL.
Unfortunately, it's also one of the two tests that I don't know how to make work
on Android's remote test server setup, so it's not super-useful bot-wise. But
once we get OpenSSL bots on other platforms, it'll be useful.

Next CL will move part of SSLClientSocketNSS's fallback code up a level, using
these tests to assert nothing's broken. Finally the rest of the stuff to get
OpenSSL in line with the test and NSS.

[wtc, 2014-06-24 21:33:29]

Patch set 2 LGTM.

I suggest using INSTANTIATE_TEST_CASE_P or similar to avoid duplicating the
TLSv1Fallback and SSLv3Fallback tests in
net/url_request/url_request_unittest.cc.

https://codereview.chromium.org/342793003/diff/20001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/342793003/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:75: std::string
GetTLSIntoleranceType(

Nit: should this function return a base::StringValue directly? Or do you think
returning a std::string is more general?

https://codereview.chromium.org/342793003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/342793003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:6601: #if !defined(USE_OPENSSL)

I think we should delete this check now. Please also remove the check from lines
6561-6562.

https://codereview.chromium.org/342793003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:6612:
SpawnedTestServer::SSLOptions::TLS_INTOLERANCE_RESET;

Rather than duplicating the code of TLSv1Fallback, can you try to iterate over
an array of ssl_options.tls_intolerance_type values?

You can either add a for loop inside TLSv1Fallback manually, or use
INSTANTIATE_TEST_CASE_P.

Note: Same for the SSLv3Fallback test below.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/READ...
File third_party/tlslite/README.chromium (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/READ...
third_party/tlslite/README.chromium:39: simulated handshake intolerance.

Nit: handshake intolerance => TLS version intolerance

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/handshakesettings.py (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:98: @ivar tlsIntolerant: TLS
version intolerance for servers.

The comment should point out the tuple represents a TLS version.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:101: intolerance by returning a
fatal handshake_failure alert or a TCP reset to

Nit: this comment doesn't mention the "close" type. You can either add it, or
just say "aborting the handshake".

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:106: TLS intolerance.

Nit: should we document the possible values here?

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/tlsconnection.py (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/tlsconnection.py:1414: yield result

Should we also call
    raise TLSUnsupportedError("Simulating version intolerance")
in this case?

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/tlsrecordlayer.py (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/tlsrecordlayer.py:531: struct.pack('ii', 1, 0))

Does 'ii' mean the two values are int, int?

[davidben, 2014-06-25 21:19:57]

https://codereview.chromium.org/342793003/diff/20001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/342793003/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:75: std::string
GetTLSIntoleranceType(
On 2014/06/24 21:33:29, wtc wrote:
> 
> Nit: should this function return a base::StringValue directly? Or do you think
> returning a std::string is more general?

Done. (I kind of wish we did away with these enums and passed things in as
strings. It takes a ton of boilerplate right now to add new flags to tlslite and
cutting down on the would be nice.)

https://codereview.chromium.org/342793003/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/342793003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:6601: #if !defined(USE_OPENSSL)
On 2014/06/24 21:33:29, wtc wrote:
> 
> I think we should delete this check now. Please also remove the check from
lines
> 6561-6562.

Good idea. We don't support building against old OpenSSLs anyway. Done.

https://codereview.chromium.org/342793003/diff/20001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:6612:
SpawnedTestServer::SSLOptions::TLS_INTOLERANCE_RESET;
On 2014/06/24 21:33:29, wtc wrote:
> 
> Rather than duplicating the code of TLSv1Fallback, can you try to iterate over
> an array of ssl_options.tls_intolerance_type values?
> 
> You can either add a for loop inside TLSv1Fallback manually, or use
> INSTANTIATE_TEST_CASE_P.
> 
> Note: Same for the SSLv3Fallback test below.

I moved it to a fixture but didn't use INSTANTIATE_TEST_CASE_P since it seemed
messier with the reset case being so different.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/READ...
File third_party/tlslite/README.chromium (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/READ...
third_party/tlslite/README.chromium:39: simulated handshake intolerance.
On 2014/06/24 21:33:29, wtc wrote:
> 
> Nit: handshake intolerance => TLS version intolerance

Done.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/handshakesettings.py (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:98: @ivar tlsIntolerant: TLS
version intolerance for servers.
On 2014/06/24 21:33:29, wtc wrote:
> 
> The comment should point out the tuple represents a TLS version.

Done.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:101: intolerance by returning a
fatal handshake_failure alert or a TCP reset to
On 2014/06/24 21:33:29, wtc wrote:
> 
> Nit: this comment doesn't mention the "close" type. You can either add it, or
> just say "aborting the handshake".

Done.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:106: TLS intolerance.
On 2014/06/24 21:33:29, wtc wrote:
> 
> Nit: should we document the possible values here?

Done.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/tlsconnection.py (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/tlsconnection.py:1414: yield result
On 2014/06/24 21:33:29, wtc wrote:
> 
> Should we also call
>     raise TLSUnsupportedError("Simulating version intolerance")
> in this case?

self._sendError actually raises TLSLocalAlert for you.

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/tlsrecordlayer.py (right):

https://codereview.chromium.org/342793003/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/tlsrecordlayer.py:531: struct.pack('ii', 1, 0))
On 2014/06/24 21:33:29, wtc wrote:
> 
> Does 'ii' mean the two values are int, int?

Yeah. See https://docs.python.org/2/library/struct.html

[wtc, 2014-06-25 23:32:22]

Patch set 3 LGTM. Good work!

https://codereview.chromium.org/342793003/diff/40001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/342793003/diff/40001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:86: return NULL;

Should we return new base::StringValue("")?

https://codereview.chromium.org/342793003/diff/40001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/handshakesettings.py (right):

https://codereview.chromium.org/342793003/diff/40001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:105: @type tlsIntoleranceType:
str @ivar tlsIntoleranceType: How the server

Should @ivar start on a new line?

[davidben, 2014-06-26 17:01:01]

https://codereview.chromium.org/342793003/diff/40001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/342793003/diff/40001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:86: return NULL;
On 2014/06/25 23:32:22, wtc wrote:
> 
> Should we return new base::StringValue("")?

Done.

https://codereview.chromium.org/342793003/diff/40001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/handshakesettings.py (right):

https://codereview.chromium.org/342793003/diff/40001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/handshakesettings.py:105: @type tlsIntoleranceType:
str @ivar tlsIntoleranceType: How the server
On 2014/06/25 23:32:22, wtc wrote:
> 
> Should @ivar start on a new line?

Oops. I blame M-q.

[davidben, 2014-06-26 17:01:12]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-06-26 17:03:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/342793003/60001

[wtc, 2014-06-26 18:07:33]

Patch set 4 LGTM.

[commit-bot: I haz the power, 2014-06-27 01:16:58]

Change committed as 280188