Update tlslite to 0.4.6.

Update tlslite to 0.4.6.

This lets us get drop many of our patches. It also adds NPN support for free.
On the downside, it introduces a handful of new bugs that need additional
patches.

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

[davidben, 2014-04-08 01:09:17]

OKAY. This time around, there is try coverage! Patch set 13 is upstream 0.4.6.
Diff between patch set 15 and patch set 13 to see our changes on top of that.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
File net/tools/testserver/testserver_base.py (right):

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver_base.py:76: if isinstance(value,
tlslite.errors.TLSClosedConnectionError):
rsleevi: This is the issue you forwarded to me; to silence a set of errors that
now appear differently.

[davidben, 2014-04-08 01:11:41]

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:49: # bots.
This should read

# Temporary hack to deal with tlslite 0.3.8 -> 0.4.6 upgrade.
#
# TODO(davidben): Remove this when it has cycled through all the bots and
developer checkouts or when http://crbug.com/356276 is resolved.

I'll fix that tomorrow.

[Ryan Sleevi, 2014-04-08 20:45:36]

Thanks for taking this on. I had previously reviewed against 0.4.3, so I'm
generally comfortable with us upreving. Some questions about changes you made in
the Chromium files below.

Additionally, if this needs to be reverted for any reason, what steps need to be
taken to ensure the bots aren't in a bad state? Anything special?

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:44: import pyftpdlib.ftpserver
Should this be moved down, as you did lines 42/43?

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:54: pass
Why do you do this here, rather than line 41?

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:1460: self.wfile.write('%s\t%s\n' % (action,
bytes(sessionID).encode('hex')))
Curious why you shuffled this around?

[wtc, 2014-04-08 22:29:08]

Patch set 15 LGTM.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:61: import tlslite.api

These are sandwiched between two lines (line 58 and line 62) related to
pywebsocket. Just wanted to make sure that is OK.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:1457: return True

Should we return False here?

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
File third_party/tlslite/README.chromium (right):

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
third_party/tlslite/README.chromium:25: -
patches/client_cipher_preferences.patch: honor client cipher preference's.

Nit: preference's => preferences

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/pa...
File third_party/tlslite/patches/ssl3_padding.patch (right):

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/pa...
third_party/tlslite/patches/ssl3_padding.patch:11: +                   
paddingLength = 0

This is how NSS does it:

    currentLength = len(b) + len(macBytes)
    blockLength = self._writeState.encContext.block_size
    paddingLength = blockLength-1-(currentLength % blockLength)

This code replaces lines 6-11.

[wtc, 2014-04-08 22:41:16]

More comments on patch set 15:

I just realized that I may not have reviewed everything in this CL because I
didn't review how you updated our patches. I think I am unlikely to review
everything, so please check your own changes.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:56: # Insert at the beginning of the path, we
want this to be used

Nit: this => these

or rephrase to be more specific, such as "our copies of the libraries".

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
File third_party/tlslite/README.chromium (right):

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
third_party/tlslite/README.chromium:30: client_cipher_preferences.patch.

Can you elaborate on this? Is it our tests or tlslite's tests? If we honor the
client's cipher preferences, why does it matter whether tlslite prefers srp +
cert over srp or not?

[davidben, 2014-04-08 23:30:35]

> Additionally, if this needs to be reverted for any reason, what steps need to
be
> taken to ensure the bots aren't in a bad state? Anything special?

Apart from the CL being large, I don't think it will need anything special to
revert? There will be the pyc file problem, but I think the new files are
unlikely to cause problems. (Hrmf. Maybe I'll get to look funny at the infra
people for rejecting the -B.)

Also updated the error suppression bit to include ECONNRESET.
SSLClientSocketTest.ConnectClientAuthCertRequested hits that one now.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:44: import pyftpdlib.ftpserver
On 2014/04/08 20:45:36, Ryan Sleevi wrote:
> Should this be moved down, as you did lines 42/43?

Rearranged this considerably. Hopefully this order is cleaner.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:49: # bots.
On 2014/04/08 01:11:42, David Benjamin wrote:
> This should read
> 
> # Temporary hack to deal with tlslite 0.3.8 -> 0.4.6 upgrade.
> #
> # TODO(davidben): Remove this when it has cycled through all the bots and
> developer checkouts or when http://crbug.com/356276 is resolved.
> 
> I'll fix that tomorrow.

Done.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:54: pass
On 2014/04/08 20:45:36, Ryan Sleevi wrote:
> Why do you do this here, rather than line 41?

I think it made more sense before I had to move testserver_base down or
something. Rearranged considerably.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:56: # Insert at the beginning of the path, we
want this to be used
On 2014/04/08 22:41:17, wtc wrote:
> 
> Nit: this => these
> 
> or rephrase to be more specific, such as "our copies of the libraries".

Done.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:61: import tlslite.api
On 2014/04/08 22:29:09, wtc wrote:
> 
> These are sandwiched between two lines (line 58 and line 62) related to
> pywebsocket. Just wanted to make sure that is OK.

Rearranged this considerably. Hopefully this order is cleaner.

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:1457: return True
On 2014/04/08 22:29:09, wtc wrote:
> 
> Should we return False here?

Hrm, I don't think so? The old code returns True. I think False means something
like "I didn't handle this request, try another handler".

https://codereview.chromium.org/210323002/diff/1310001/net/tools/testserver/t...
net/tools/testserver/testserver.py:1460: self.wfile.write('%s\t%s\n' % (action,
bytes(sessionID).encode('hex')))
On 2014/04/08 20:45:36, Ryan Sleevi wrote:
> Curious why you shuffled this around?

So, the actual change was to do a bytes(sessionID) first because
sessionID.encode now gives an AttributeError. But that got masked because of the
try/except. I moved it out so that only self.server.session_cache.log would
trigger the except block; that's a proxy for whether --https-record-resume. But
if there's an AttributeError in the loop, that's an actual bug and we shouldn't
catch the exception.

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
File third_party/tlslite/README.chromium (right):

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
third_party/tlslite/README.chromium:25: -
patches/client_cipher_preferences.patch: honor client cipher preference's.
On 2014/04/08 22:29:09, wtc wrote:
> 
> Nit: preference's => preferences

Done.

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/RE...
third_party/tlslite/README.chromium:30: client_cipher_preferences.patch.
On 2014/04/08 22:41:17, wtc wrote:
> 
> Can you elaborate on this? Is it our tests or tlslite's tests? If we honor the
> client's cipher preferences, why does it matter whether tlslite prefers srp +
> cert over srp or not?

Yeah, it's tlslite's tests. Done.

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/pa...
File third_party/tlslite/patches/ssl3_padding.patch (right):

https://codereview.chromium.org/210323002/diff/1310001/third_party/tlslite/pa...
third_party/tlslite/patches/ssl3_padding.patch:11: +                   
paddingLength = 0
On 2014/04/08 22:29:09, wtc wrote:
> 
> This is how NSS does it:
> 
>     currentLength = len(b) + len(macBytes)
>     blockLength = self._writeState.encContext.block_size
>     paddingLength = blockLength-1-(currentLength % blockLength)
> 
> This code replaces lines 6-11.

Oh, that's much tidier. Done.

[wtc, 2014-04-09 15:11:56]

Patch set 17 LGTM.

https://codereview.chromium.org/210323002/diff/1340001/net/tools/testserver/t...
File net/tools/testserver/testserver_base.py (right):

https://codereview.chromium.org/210323002/diff/1340001/net/tools/testserver/t...
net/tools/testserver/testserver_base.py:90: print "testserver.py: Connection
reset"

Nit: should we add "by peer" to the error message? "Connection reset by peer" is
the conventional error message for this error code.

[davidben, 2014-04-09 16:40:18]

+antrim for chrome/browser/chromeos/login/test/https_forwarder.py
+mnissler for chrome/browser/policy/test/policy_testserver.py

Context for API changes: X509CertChain.parsePemList no longer returns self, so
we can't chain things. The byte array object in tlslite changed from an
array.array to a bytearray.

(I'll wait for Ryan's review before landing this; just probably worth getting
OWNERs stamps now that one of the main reviews is done.)

https://codereview.chromium.org/210323002/diff/1340001/net/tools/testserver/t...
File net/tools/testserver/testserver_base.py (right):

https://codereview.chromium.org/210323002/diff/1340001/net/tools/testserver/t...
net/tools/testserver/testserver_base.py:90: print "testserver.py: Connection
reset"
On 2014/04/09 15:11:57, wtc wrote:
> 
> Nit: should we add "by peer" to the error message? "Connection reset by peer"
is
> the conventional error message for this error code.

Done.

[Mattias Nissler (ping if slow), 2014-04-09 16:57:33]

policy_testserver.py LGTM

[Ryan Sleevi, 2014-04-09 17:29:36]

lgtm

[Denis Kuznetsov (DE-MUC), 2014-04-10 13:08:30]

chrome/browser/policy/test/policy_testserver.py 
lgtm

[davidben, 2014-04-10 14:43:40]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-04-10 14:43:44]

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

[commit-bot: I haz the power, 2014-04-10 15:23:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-10 15:23:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[davidben, 2014-04-10 15:43:36]

On 2014/04/10 15:23:49, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

Sigh. Of course that wouldn't be the end of it. Even more infrastructure bugs!

1. tests/httpsserver.sh is executable in upstream, but upstream wrote "#
/bin/sh" instead of "#!/bin/sh". Somehow it gets the executable bit anyway,
probably by virtue of ending in .sh.

2. tests/httpsclient.py lost the executable bit. Perhaps because Rietveld
convinced git it was a copy of some png file and binary files and executable
bits don't interact? It would be in the mode line off the diff, but there isn't
going to be a diff.

I'm going to try to fix this, but if it takes too long, I think we're better off
just deleting those files. They're for testing some TACK thing which we don't
really care about.

[davidben, 2014-04-10 15:57:35]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-04-10 15:57:44]

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

[commit-bot: I haz the power, 2014-04-10 17:56:47]

Change committed as 263026