Disable Nagle on Linux and TLS cut through support

Disable Nagle on Linux and TLS cut through support

  * Disables Nagle on Linux. This mirrors the behaviour on Windows.
  * Adds TLS cut through support. This allows us to start sending encrypted
    data before we have validated the server's Finished message. (This
    behaviour is already enabled on Android.)

    I've verified that this works using netem to add a 200ms delay on the
    loopback adaptor. I've also checked that an incorrect Finished message from
    the server causes an error by hacking the Go TLS server.

    Beward when looking at packet traces that the time taken in NSS's SQLite
    calls can exceed the RTT of the connection and make it appear that this
    code isn't functioning.
  * Adds DEBUG and TRACE defines to libssl when building Chromium in Debug
    mode. This means that setting SSLTRACE in the environment now works for
    debug builds.

[wtc, 2010-01-13 01:03:29]

LGTM on the DisableNagle change.  Please split it off
into another CL and commit it.  It's an independent change.

I still need to review your TLS cut-through NSS patch.

http://codereview.chromium.org/518065/diff/1001/1003
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/518065/diff/1001/1003#newcode355
net/socket/tcp_client_socket_libevent.cc:355: if (SetNonBlocking(socket_))
If SetNonBlocking fails, we should do
  close(socket_);
  socket_ = kInvalidSocket;
before returning MapPosixError(errno).  We need to be
careful to map the errno for SetNonBlocking failure
right away or save it in a local variable.

If DisableNagle fails, we could either fail or use the
socket with Nagle.

[wtc, 2010-02-17 19:40:00]

Thanks for the patch and sorry about the delay in reviewing it.

This looks good.  There is one important security issue
marked with "IMPORTANT" below.  It's fine for Chromium
because we only enable cipher suites that are 80 bits or
stronger, but we need to fix this for upstream NSS.
Because of this issue, I'd like to review a new Patch Set
before you check this in.

It would be nice to have Nelson review this patch...
I'll ping him in the upstream NSS bug.

I still suggest that you split the Disable Nagle change
into a separate CL.

http://codereview.chromium.org/518065/diff/1001/1005
File net/third_party/nss/nss.gyp (right):

http://codereview.chromium.org/518065/diff/1001/1005#newcode95
net/third_party/nss/nss.gyp:95: 'TRACE',
I thought some other .gyp file that we include already
define DEBUG for debug builds.  Is that not the case?

Hmm... the only thing I can find is _DEBUG, with a
leading underscore, in src/build/common.gypi.  This is a
surprise to me.

TRACE is not necessary, because DEBUG implies TRACE.  See
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslimpl.h#113

http://codereview.chromium.org/518065/diff/1001/1006
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/518065/diff/1001/1006#newcode124
net/third_party/nss/ssl/ssl.h:124: #define SSL_ENABLE_CUT_THROUGH         22 /*
Enable TLS cut through (off by */
Nit: TLS => SSL

I actually prefer your name for this feature, SSL false
start, because it's more accurate and suggests that it
violates the spec.  Is it too late to use that name?

Please document the following:
1. This option is applicable to client sockets only.
2. The benefits and risks of this option.
3. Cite the web page (does it exist?) that specifies this
performance enhancement.

http://codereview.chromium.org/518065/diff/1001/1007
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/518065/diff/1001/1007#newcode5574
net/third_party/nss/ssl/ssl3con.c:5574: !ss->opt.handshakeAsServer &&
It may be better to test ss->sec.isServer instead.  That's
what most of the libSSL code tests.

IMPORTANT: we must check the security of the negotiated
cipher suite is >= 80 bits (normalized to symmetric cipher
strength).

http://codereview.chromium.org/518065/diff/1001/1007#newcode5648
net/third_party/nss/ssl/ssl3con.c:5648: ss->clientFinishedSent = 1;
Here it's clear that
  ss->clientFinishedSent
is equivalent to
  (ss->ssl3.hs.ws == wait_new_session_ticket ||
   ss->ssl3.hs.ws == wait_change_cipher)

I think it's useful to use clientFinishedSent to make the
code easier to understand, but we should not need to test
both conditions (see my comment on ssl3gthr.c below).

http://codereview.chromium.org/518065/diff/1001/1007#newcode5650
net/third_party/nss/ssl/ssl3con.c:5650: /* Do the handshake callback for sslv3
here. */
We should call ss->handshakeCallback right
before line 5661 (return SECSuccess) for two reasons:
1. We will have set ss->ssl3.hs.ws correctly.
2. We will have released the XmitBufLock.

Cf: lines 8164-8167 in ssl3_HandleFinished below.

http://codereview.chromium.org/518065/diff/1001/1008
File net/third_party/nss/ssl/ssl3gthr.c (right):

http://codereview.chromium.org/518065/diff/1001/1008#newcode216
net/third_party/nss/ssl/ssl3gthr.c:216: (ss->ssl3.hs.ws == wait_change_cipher ||
Nit: let's test ss->ssl3.hs.ws first so that we can
short-circuit the ssl3_CanCutThrough(ss) call when it's
not necessary.

It seems that
  (ss->ssl3.hs.ws == wait_change_cipher ||
   ss->ssl3.hs.ws == wait_new_session_ticket)
implies
  ss->clientFinishedSent.  If so, it's not necessary to
test both.

http://codereview.chromium.org/518065/diff/1001/1009
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/518065/diff/1001/1009#newcode1021
net/third_party/nss/ssl/sslimpl.h:1021: unsigned long    clientFinishedSent;
How about 'sentClientFinished' (analogous to
recvdCloseNotify below and sentElements and sentFinished
in the sslConnectInfoStr structure)?

[wtc, 2010-02-17 19:48:22]

Your patch in the upstream NSS bug
(https://bugzilla.mozilla.org/show_bug.cgi?id=525092)
is the same as Patch Set 2 of this CL, but is marked
as "Still not ready for prime time".

Let's wait until you have addressed my comments to ask
Nelson to review the patch.  In the upstream NSS bug,
please generate the new patch using CVS.  Thanks!

[ian fette, 2010-02-17 19:55:54]

I remain concerned about disabling Nagle. It's designed to prevent congestive
failure of the internet. I realize we get some speed benefits from it, but it
feels a lot like saying "hell, let's throw out the airbags and seatbelts cause
we can save 5kg and go faster."

On 2010/02/17 19:48:22, wtc wrote:
> Your patch in the upstream NSS bug
> (https://bugzilla.mozilla.org/show_bug.cgi?id=525092)
> is the same as Patch Set 2 of this CL, but is marked
> as "Still not ready for prime time".
> 
> Let's wait until you have addressed my comments to ask
> Nelson to review the patch.  In the upstream NSS bug,
> please generate the new patch using CVS.  Thanks!

[agl, 2010-02-17 20:19:07]

On Wed, Feb 17, 2010 at 2:55 PM,  <ian@chromium.org> wrote:
> I remain concerned about disabling Nagle. It's designed to prevent
> congestive
> failure of the internet. I realize we get some speed benefits from it, but
> it
> feels a lot like saying "hell, let's throw out the airbags and seatbelts
> cause
> we can save 5kg and go faster."

The disabling Nagel patch was split off and committed a while back (36639).

Nagle doesn't affect congestion control, it's a balance between
latency and throughput and Chrome Windows has disabled it for ages.
This is just matching that on the other platforms. I think we're
pretty safe.


AGL


AGL

[agl, 2010-02-18 01:16:03]

PTAL

http://codereview.chromium.org/518065/diff/1001/1005
File net/third_party/nss/nss.gyp (right):

http://codereview.chromium.org/518065/diff/1001/1005#newcode95
net/third_party/nss/nss.gyp:95: 'TRACE',
On 2010/02/17 19:40:00, wtc wrote:
> I thought some other .gyp file that we include already
> define DEBUG for debug builds.  Is that not the case?
> 
> Hmm... the only thing I can find is _DEBUG, with a
> leading underscore, in src/build/common.gypi.  This is a
> surprise to me.
> 
> TRACE is not necessary, because DEBUG implies TRACE.  See
> http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslimpl.h#113

Done.

http://codereview.chromium.org/518065/diff/1001/1006
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/518065/diff/1001/1006#newcode124
net/third_party/nss/ssl/ssl.h:124: #define SSL_ENABLE_CUT_THROUGH         22 /*
Enable TLS cut through (off by */
On 2010/02/17 19:40:00, wtc wrote:
> I actually prefer your name for this feature, SSL false
> start, because it's more accurate and suggests that it
> violates the spec.  Is it too late to use that name?

I don't think so. We haven't really talked publically about it. Have changed the
name.

> Please document the following:
> 1. This option is applicable to client sockets only.
> 2. The benefits and risks of this option.
> 3. Cite the web page (does it exist?) that specifies this
> performance enhancement.

Done, although we don't have any web pages describing it. Should we? Where?

http://codereview.chromium.org/518065/diff/1001/1007
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/518065/diff/1001/1007#newcode5574
net/third_party/nss/ssl/ssl3con.c:5574: !ss->opt.handshakeAsServer &&
On 2010/02/17 19:40:00, wtc wrote:
> It may be better to test ss->sec.isServer instead.  That's
> what most of the libSSL code tests.
> 
> IMPORTANT: we must check the security of the negotiated
> cipher suite is >= 80 bits (normalized to symmetric cipher
> strength).

Done.

http://codereview.chromium.org/518065/diff/1001/1007#newcode5648
net/third_party/nss/ssl/ssl3con.c:5648: ss->clientFinishedSent = 1;
On 2010/02/17 19:40:00, wtc wrote:
> Here it's clear that
>   ss->clientFinishedSent
> is equivalent to
>   (ss->ssl3.hs.ws == wait_new_session_ticket ||
>    ss->ssl3.hs.ws == wait_change_cipher)
> 
> I think it's useful to use clientFinishedSent to make the
> code easier to understand, but we should not need to test
> both conditions (see my comment on ssl3gthr.c below).

I actually removed clientFinishedSent since I think you're right that it doesn't
add anything.

http://codereview.chromium.org/518065/diff/1001/1007#newcode5650
net/third_party/nss/ssl/ssl3con.c:5650: /* Do the handshake callback for sslv3
here. */
On 2010/02/17 19:40:00, wtc wrote:
> We should call ss->handshakeCallback right
> before line 5661 (return SECSuccess) for two reasons:
> 1. We will have set ss->ssl3.hs.ws correctly.
> 2. We will have released the XmitBufLock.
> 
> Cf: lines 8164-8167 in ssl3_HandleFinished below.

Done.

http://codereview.chromium.org/518065/diff/1001/1008
File net/third_party/nss/ssl/ssl3gthr.c (right):

http://codereview.chromium.org/518065/diff/1001/1008#newcode216
net/third_party/nss/ssl/ssl3gthr.c:216: (ss->ssl3.hs.ws == wait_change_cipher ||
On 2010/02/17 19:40:00, wtc wrote:
> Nit: let's test ss->ssl3.hs.ws first so that we can
> short-circuit the ssl3_CanCutThrough(ss) call when it's
> not necessary.
> 
> It seems that
>   (ss->ssl3.hs.ws == wait_change_cipher ||
>    ss->ssl3.hs.ws == wait_new_session_ticket)
> implies
>   ss->clientFinishedSent.  If so, it's not necessary to
> test both.

Done.

http://codereview.chromium.org/518065/diff/1001/1009
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/518065/diff/1001/1009#newcode1021
net/third_party/nss/ssl/sslimpl.h:1021: unsigned long    clientFinishedSent;
On 2010/02/17 19:40:00, wtc wrote:
> How about 'sentClientFinished' (analogous to
> recvdCloseNotify below and sentElements and sentFinished
> in the sslConnectInfoStr structure)?

Have actually removed this. I think you're correct that it's implied by the test
for ws.

[wtc, 2010-02-20 00:39:50]

LGTM.  Please attach an updated NSS patch to the NSS bug
and ask Nelson to review it.  Thanks!

http://codereview.chromium.org/518065/diff/1001/1006
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/518065/diff/1001/1006#newcode124
net/third_party/nss/ssl/ssl.h:124: #define SSL_ENABLE_CUT_THROUGH         22 /*
Enable TLS cut through (off by */
On 2010/02/18 01:16:03, agl wrote:
>
> We haven't really talked publically about it. Have changed the
> name.

What I meant is that it might be too late to change the
name if the OpenSSL patch has been checked in with the
"cut-through" name.

> Done, although we don't have any web pages describing it. Should we? Where?

We should.  Ideally it should be an Internet-Draft.

We can use http://dev.chromium.org to describe all of our
SSL efforts ("Overclocking SSL" is my proposed name).  Is
there a better site?  http://code.google.com?

http://codereview.chromium.org/518065/diff/10002/10006
File net/third_party/nss/patches/falsestart.patch (right):

http://codereview.chromium.org/518065/diff/10002/10006#newcode1
net/third_party/nss/patches/falsestart.patch:1: diff --git
a/mozilla/security/nss/cmd/strsclnt/strsclnt.c
b/mozilla/security/nss/cmd/strsclnt/strsclnt.c
Remove the NPN changes from this patch.

http://codereview.chromium.org/518065/diff/10002/10007
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/518065/diff/10002/10007#newcode135
net/third_party/nss/ssl/ssl.h:135: /* verifing the server's Finished message.
This means that we could end up  */
Typo: verifing => verifying

http://codereview.chromium.org/518065/diff/10002/10007#newcode137
net/third_party/nss/ssl/ssl.h:137: /* only the true server can decrypt the
session key. Thus, so long as the   */
Nit: "can decrypt the session key" isn't entirely accurate.
How about "can derive the session key" or "has the session key"?

http://codereview.chromium.org/518065/diff/10002/10007#newcode139
net/third_party/nss/ssl/ssl.h:139: /* occur on RSA ciphersuites where the
cipher's key length is >= 80 bits.   */
Why only RSA ciphersuites?

http://codereview.chromium.org/518065/diff/10002/10008
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/518065/diff/10002/10008#newcode5667
net/third_party/nss/ssl/ssl3con.c:5667: ss->ssl3.hs.kea_def->exchKeyType ==
kt_rsa;
I believe you should compare this with ssl_kea_rsa.  See

http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslimpl.h#666
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslimpl.h#72
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslt.h#70

See also
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslt.h#80
which says kt_rsa is for backward compatibility.

http://codereview.chromium.org/518065/diff/10002/10011
File net/third_party/nss/ssl/sslsecur.c (right):

http://codereview.chromium.org/518065/diff/10002/10011#newcode153
net/third_party/nss/ssl/sslsecur.c:153: (ss->ssl3.hs.ws == wait_change_cipher ||
Let's test ss->ssl3.hs.ws before ssl3_CanFalseStart(ss)
so that we can short-circuit ssl3_CanFalseStart(ss) when
it's not necessary.

[wtc, 2010-02-20 00:41:01]

Please update the Description of the CL to remove the
"Disable Nagle on Linux" part and update the description
about TRACE.