Implement extended master secret in tlslite

Implement extended master secret in tlslite

BUG=467312
Committed: https://crrev.com/8a61fc9eaf016b964a6adc42dd2288f9193f70cd
Cr-Commit-Position: refs/heads/master@{#344031}

[nharper, 2015-08-12 22:36:09]

nharper@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-08-14 22:31:18]

Sorry about the delay. I'm kind of amazed it's as small as it is.

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/constants.py (right):

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/constants.py:58: extended_master_secret = 23  #
RFC-ietf-tls-session-hash-06
Nit: RFC => draft

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/tlsrecordlayer.py (right):

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/tlsrecordlayer.py:119: self.handshakeMessages = []
You should be able to just use the handshake digests below rather than enable a
handshake buffer I think. (Handshake buffer in TLS is only needed in TLS 1.2
before ServerHello and for client auth  which this doesn't implement.)

They're sampled in _calcFinished in tlsconnection.py. I would suggest factoring
that out into a method like _handshakeHash or _getHandshakeHash whatever the
right naming convention is.

[nharper, 2015-08-15 00:37:43]

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/constants.py (right):

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/constants.py:58: extended_master_secret = 23  #
RFC-ietf-tls-session-hash-06
On 2015/08/14 22:31:18, David Benjamin wrote:
> Nit: RFC => draft

Done.

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
File third_party/tlslite/tlslite/tlsrecordlayer.py (right):

https://codereview.chromium.org/1283373002/diff/1/third_party/tlslite/tlslite...
third_party/tlslite/tlslite/tlsrecordlayer.py:119: self.handshakeMessages = []
On 2015/08/14 22:31:18, David Benjamin wrote:
> You should be able to just use the handshake digests below rather than enable
a
> handshake buffer I think. (Handshake buffer in TLS is only needed in TLS 1.2
> before ServerHello and for client auth  which this doesn't implement.)
> 
> They're sampled in _calcFinished in tlsconnection.py. I would suggest
factoring
> that out into a method like _handshakeHash or _getHandshakeHash whatever the
> right naming convention is.

Ah, I should have read the surrounding code and seen the handshake digests that
were already there.

I created a _getHandshakeHash method to use with extended master secret. I could
do more refactoring to use it elsewhere as well, but that might make it harder
to cleanly apply the patch in the future.

[davidben, 2015-08-17 17:10:22]

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/pat...
File third_party/tlslite/patches/extended_master_secret.patch (right):

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/pat...
third_party/tlslite/patches/extended_master_secret.patch:1: diff --git
a/third_party/tlslite/tlslite/constants.py
b/third_party/tlslite/tlslite/constants.py
[I'm assuming this patch is accurate.]

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/tls...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/tls...
third_party/tlslite/tlslite/messages.py:95: self.rawMessage = bytearray(0)
This and the bit below look like they're not needed.

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

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/tls...
third_party/tlslite/tlslite/tlsconnection.py:1530:
clientHello.extended_master_secret
To confirm, this will make every connection we make negotiate EMS. Is this fine?
I don't know what kind of tests you wish to write. Do you want to be able to
test against EMS-less servers?

(If so, putting it in HandshakeSettings is probably the simplest. Otherwise you
need to add a parameter everywhere. A lot of our stuff should have been added
there.)

[nharper, 2015-08-18 00:03:31]

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/pat...
File third_party/tlslite/patches/extended_master_secret.patch (right):

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/pat...
third_party/tlslite/patches/extended_master_secret.patch:1: diff --git
a/third_party/tlslite/tlslite/constants.py
b/third_party/tlslite/tlslite/constants.py
On 2015/08/17 17:10:21, David Benjamin wrote:
> [I'm assuming this patch is accurate.]

I've been updating it with git diff after each patchset.

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/tls...
File third_party/tlslite/tlslite/messages.py (right):

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/tls...
third_party/tlslite/tlslite/messages.py:95: self.rawMessage = bytearray(0)
On 2015/08/17 17:10:22, David Benjamin wrote:
> This and the bit below look like they're not needed.

Done.

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

https://codereview.chromium.org/1283373002/diff/20001/third_party/tlslite/tls...
third_party/tlslite/tlslite/tlsconnection.py:1530:
clientHello.extended_master_secret
On 2015/08/17 17:10:22, David Benjamin wrote:
> To confirm, this will make every connection we make negotiate EMS. Is this
fine?
> I don't know what kind of tests you wish to write. Do you want to be able to
> test against EMS-less servers?
> 
> (If so, putting it in HandshakeSettings is probably the simplest. Otherwise
you
> need to add a parameter everywhere. A lot of our stuff should have been added
> there.)

I added a setting to HandshakeSettings, since I'll probably want to test that
the client site token binding code behaves correctly when a server is bad and
supports token binding without extended master secret.

[davidben, 2015-08-18 21:02:14]

lgtm

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

https://codereview.chromium.org/1283373002/diff/40001/third_party/tlslite/tls...
third_party/tlslite/tlslite/handshakesettings.py:136:
self.enableExtendedMasterSecret = True
[Note that this will change the behavior of the test server. I think this is
fine and correct, but pointing it out to make sure we're on the same page.]

[nharper, 2015-08-18 21:05:39]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2015-08-18 21:05:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1283373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1283373002/40001

[commit-bot: I haz the power, 2015-08-18 21:15:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-18 21:16:37]

Patchset 3 (id:??) landed as
https://crrev.com/8a61fc9eaf016b964a6adc42dd2288f9193f70cd
Cr-Commit-Position: refs/heads/master@{#344031}

[jbudorick, 2015-08-19 04:35:00]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1299153002/ by jbudorick@chromium.org.

The reason for reverting is: Broke
SSLClientSocketOpenSSLClientAuthTest.SendGoodCert on Android Tests and Android
Tests (dbg):

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/21769
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%....