net: add a test to ensure that our TLS handshake doesn't get too large.

net: add a test to ensure that our TLS handshake doesn't get too large.

(I would like the test to assert that we have some headroom too, but we don't
have any to assert!)

BUG=none
R=rsleevi@chromium.org, wtc@chromium.org

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

[Ryan Sleevi, 2013-07-23 20:50:01]

https://codereview.chromium.org/19557004/diff/3001/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/19557004/diff/3001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:5420:
HttpStreamFactory::EnableNpnSpdy4a2();
Need a comment explaining why this is being called - and why the test is leaving
this state enabled if any ASSERTs fail / when the test ends.

https://codereview.chromium.org/19557004/diff/3001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:5441: strlen("www.wellsfargo.com");
I don't quite get why you chose this, or incorporated into the length check.

It seems we'd run just as much risk with "www2.wellsfargo.com" hitting the 256
byte limit.

Would we kill a ciphersuite for that server? If not, I'm not sure why the SNI
overhead check.

[agl, 2013-07-24 18:09:18]

https://codereview.chromium.org/19557004/diff/3001/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/19557004/diff/3001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:5420:
HttpStreamFactory::EnableNpnSpdy4a2();
On 2013/07/23 20:50:02, Ryan Sleevi wrote:
> Need a comment explaining why this is being called - and why the test is
leaving
> this state enabled if any ASSERTs fail / when the test ends.

Done. This will be reset by NetTestSuite; have noted that in the comment.

https://codereview.chromium.org/19557004/diff/3001/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:5441: strlen("www.wellsfargo.com");
On 2013/07/23 20:50:02, Ryan Sleevi wrote:
> I don't quite get why you chose this, or incorporated into the length check.
> 
> It seems we'd run just as much risk with "www2.wellsfargo.com" hitting the 256
> byte limit.
> 
> Would we kill a ciphersuite for that server? If not, I'm not sure why the SNI
> overhead check.

www.wellsfargo.com is probably the biggest site with this issue, so we would
want to know if we're going to break it.

I agree, it would be nice to assert a stronger bound so that www2.wellsfargo.com
doesn't have a problem, but we need to do some work to shrink the ClientHello
first.

[Ryan Sleevi, 2013-07-24 18:13:32]

lgtm

[wtc, 2013-07-24 19:25:10]

Patch set 3 LGTM.

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5410: // cause issues.

Nit: we usually put this kind of comment before the test, like this:

// Check that our ClientHello doesn't get too large because that's known to
// cause issues.
TEST_F(HTTPSRequestTest, ClientHelloTest) {

Also, it would be nice to be specific about what is too large (the 256u constant
at the end of the test).

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5443: strlen("www.wellsfargo.com");

Why do we add the SNI overhead? Is it because this unit test is using an IP
address (127.0.0.1) in the URL?

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5446: EXPECT_LT(client_hello_length +
sni_overhead + session_id_overhead, 256u);

Nit: it may be better to reverse this:
  EXPECT_GT(256u, client_hello_length + sni_overhead + session_id_overhead);

because the EXPECT_xxx arguments are (expect, actual).

https://codereview.chromium.org/19557004/diff/25001/third_party/tlslite/tlsli...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/19557004/diff/25001/third_party/tlslite/tlsli...
third_party/tlslite/tlslite/messages.py:165: self.client_hello_length =
len(p.bytes) - p.index

I didn't verify the correctness of this expression because I don't know
this file well, but this expression looks correct and it also appears on
line 496.

[agl, 2013-07-29 16:58:36]

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5410: // cause issues.
On 2013/07/24 19:25:10, wtc wrote:
> 
> Nit: we usually put this kind of comment before the test, like this:
> 
> // Check that our ClientHello doesn't get too large because that's known to
> // cause issues.
> TEST_F(HTTPSRequestTest, ClientHelloTest) {
> 
> Also, it would be nice to be specific about what is too large (the 256u
constant
> at the end of the test).

Done.

https://codereview.chromium.org/19557004/diff/25001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:5443: strlen("www.wellsfargo.com");
On 2013/07/24 19:25:10, wtc wrote:
> 
> Why do we add the SNI overhead? Is it because this unit test is using an IP
> address (127.0.0.1) in the URL?

Yes, have noted so in comment.

[commit-bot: I haz the power, 2013-07-29 17:02:42]

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

[commit-bot: I haz the power, 2013-07-30 00:58:35]

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

[commit-bot: I haz the power, 2013-07-30 11:02:16]

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

[commit-bot: I haz the power, 2013-07-31 01:33:11]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ryan Sleevi, 2013-08-06 19:37:03]

agl: I think you need to poke
https://code.google.com/p/chromium/codesearch#chromium/src/chrome_frame/test/...
in order to disable these tests for Chrome Frame (we disable SSL tests for CF)

[agl, 2013-08-08 20:36:29]

Committed patchset #5 manually as r216444.

[frankf, 2013-08-08 23:16:06]

On 2013/08/08 20:36:29, agl wrote:
> Committed patchset #5 manually as r216444.

The Android trybot failed. Why did you commit? I reverted this.

[Peter Kasting, 2013-08-09 00:11:43]

(Also, perhaps a bug can be filed for this before it gets relanded, so there is
somewhere to post about issues with it)