net: add test for TLS_FALLBACK_SCSV

net: add test for TLS_FALLBACK_SCSV

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

[wtc, 2013-12-10 16:28:27]

Patch set 2 LGTM.

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

https://codereview.chromium.org/109563002/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:410:
arguments->Set("fallback-scsv", base::Value::CreateNullValue());

It would be nice to move this up, to follow the tls_intolerant code. (See my
comment in base_test_server.h about avoiding merge conflicts with Emilia's CL.)

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

https://codereview.chromium.org/109563002/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:160: // TLS_FALLBACK_SCSV cipher
suites. This cipher suite is sent by Chrome

Nit: "cipher suites" should be singular. There is only one fallback SCSV.

https://codereview.chromium.org/109563002/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:164: bool fallback_scsv_enabled;

Emilia is adding a new option for OCSP stapling here. To avoid merge conflicts,
I suggest we list fallback_scsv_enabled after "TLSIntolerantLevel
tls_intolerant" (which happens to be related).

Emilia's CL: https://codereview.chromium.org/92443002/

https://codereview.chromium.org/109563002/diff/20001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/109563002/diff/20001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2099:
self.option_parser.add_option('--fallback-scsv', dest='fallback_scsv',

Nit: should these be named --fallback-scsv-enabled and fallback_scsv_enabled
(with "enabled" at the end) for consistency with the other files?

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

https://codereview.chromium.org/109563002/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/TLSConnection.py:1145:
AlertDescription.handshake_failure):

NSS has a special case for a handshake_failure alert received in response to its
ClientHello: it gets reported as SSL_ERROR_NO_CYPHER_OVERLAP, which Chrome maps
to net::ERR_SSL_VERSION_OR_CIPHER_MISMATCH.

If you prefer that the new unit test gets net::ERR_SSL_PROTOCOL_ERROR, we can
send the illegal_parameter alert here. Some intolerant servers send
illegal_parameter.

https://codereview.chromium.org/109563002/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/TLSConnection.py:1149: if clientHello.client_version
> settings.maxVersion:

Could you explain why you changed this "elif" to "if"? Is it because the
previous if branch returns from the function? I guess that's what the "yield
result" statement does. (This shows how little I know about Python.)

[agl, 2013-12-10 17:53:42]

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

https://codereview.chromium.org/109563002/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:410:
arguments->Set("fallback-scsv", base::Value::CreateNullValue());
On 2013/12/10 16:28:27, wtc wrote:
> 
> It would be nice to move this up, to follow the tls_intolerant code. (See my
> comment in base_test_server.h about avoiding merge conflicts with Emilia's
CL.)

Done.

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

https://codereview.chromium.org/109563002/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:160: // TLS_FALLBACK_SCSV cipher
suites. This cipher suite is sent by Chrome
On 2013/12/10 16:28:27, wtc wrote:
> 
> Nit: "cipher suites" should be singular. There is only one fallback SCSV.

Done.

https://codereview.chromium.org/109563002/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:164: bool fallback_scsv_enabled;
On 2013/12/10 16:28:27, wtc wrote:
> 
> Emilia is adding a new option for OCSP stapling here. To avoid merge
conflicts,
> I suggest we list fallback_scsv_enabled after "TLSIntolerantLevel
> tls_intolerant" (which happens to be related).
> 
> Emilia's CL: https://codereview.chromium.org/92443002/

Done.

https://codereview.chromium.org/109563002/diff/20001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/109563002/diff/20001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2099:
self.option_parser.add_option('--fallback-scsv', dest='fallback_scsv',
On 2013/12/10 16:28:27, wtc wrote:
> 
> Nit: should these be named --fallback-scsv-enabled and fallback_scsv_enabled
> (with "enabled" at the end) for consistency with the other files?

I think the style for arguments and variables is different. As an argument,
having "enabled" seems very odd, but I think it makes things clearer in code. I
don't think that I have a good reason here, but I think it's consistent with
these argument names.

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

https://codereview.chromium.org/109563002/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/TLSConnection.py:1145:
AlertDescription.handshake_failure):
On 2013/12/10 16:28:27, wtc wrote:
> If you prefer that the new unit test gets net::ERR_SSL_PROTOCOL_ERROR, we can
> send the illegal_parameter alert here. Some intolerant servers send
> illegal_parameter.

I think any fatal alert will do. Since everything is working with this one, I
don't see a reason to change it.

https://codereview.chromium.org/109563002/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/TLSConnection.py:1149: if clientHello.client_version
> settings.maxVersion:
On 2013/12/10 16:28:27, wtc wrote:
> 
> Could you explain why you changed this "elif" to "if"? Is it because the
> previous if branch returns from the function? I guess that's what the "yield
> result" statement does. (This shows how little I know about Python.)

I think the "elif" was a mistake, perhaps from when I inserted the tlsIntolerant
code. Originally I guess it was linked to line 1130, above but it doesn't
actually make any difference to the logic of the program as the conditions are
mutually exclusive.

[wtc, 2013-12-10 18:30:22]

Patch set 3 LGTM.

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

https://codereview.chromium.org/109563002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:6064:
SpawnedTestServer::SSLOptions::TLS_INTOLERANT_TLS1_0;

Just curious: why did you lower the intolerant version from TLS 1.1 to TLS 1.0?

[agl, 2013-12-11 16:35:14]

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

https://codereview.chromium.org/109563002/diff/40001/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:6064:
SpawnedTestServer::SSLOptions::TLS_INTOLERANT_TLS1_0;
On 2013/12/10 18:30:22, wtc wrote:
> 
> Just curious: why did you lower the intolerant version from TLS 1.1 to TLS
1.0?

ChromeOS failed the test in a way that suggested that the fallback never
happened. So it's just an attempt to see if I can trigger it by reducing the
fallback version.

Still playing with tryservers to see.

[commit-bot: I haz the power, 2013-12-11 18:47:02]

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

[commit-bot: I haz the power, 2013-12-11 20:16:15]

Change committed as 240163

[wtc, 2013-12-13 16:19:21]

https://codereview.chromium.org/109563002/diff/60001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/TLSConnection.py (right):

https://codereview.chromium.org/109563002/diff/60001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/TLSConnection.py:941: fallbackSCSV=False):

Nit: just noticed that we should document the new fallbackSCSV parameter below.
The tlsIntolerant parameter isn't documented either.

[agl, 2013-12-16 15:56:04]

https://codereview.chromium.org/109563002/diff/60001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/TLSConnection.py (right):

https://codereview.chromium.org/109563002/diff/60001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/TLSConnection.py:941: fallbackSCSV=False):
On 2013/12/13 16:19:21, wtc wrote:
> 
> Nit: just noticed that we should document the new fallbackSCSV parameter
below.
> The tlsIntolerant parameter isn't documented either.

Done in http://src.chromium.org/viewvc/chrome?revision=240907&view=revision