|
|
Created:
6 years, 9 months ago by davidben Modified:
6 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd DHE_RSA support to tlslite.
Needed for our test server to be able to False Start.
BUG=354132
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263169
Patch Set 1 #
Total comments: 26
Patch Set 2 : wtc comments #
Total comments: 2
Patch Set 3 : spaces #
Total comments: 31
Patch Set 4 : wtc comments. #
Total comments: 2
Patch Set 5 : Rebase and address wtc comment. #Patch Set 6 : Update patch. #
Messages
Total messages: 20 (0 generated)
Context for domino chain: https://crbug.com/354132#c1 Sending this out for review ahead of time, but note that it applies on top of tlslite 0.4.6 with some of our patches. Until the prerequisite CLs get in, there won't be try coverage. So feel free to wait until then for the final review.
Quick review LG, but I'll need to mentally page in more of TLSLite 0.4.3 https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:42: return None is None right? Should it be NotImplemented? https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:49: # Bleichenbacher's "million message" attack heheheh. This doesn't really do that, but ok [Yes, aware it's pre-existing code]
https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:42: return None On 2014/03/31 20:43:57, Ryan Sleevi wrote: > is None right? Should it be NotImplemented? RSA doesn't send a ServerKeyExchange. This method is supposed to either return a ServerKeyExchange to send or None to indicate that this key exchange doesn't involve sending one. So it is implemented and intentionally returns None. _serverCertKeyExchange calls makeServerKeyExchange regardless of what cipherSuite you're using. Possibly that should be documented in a comment somewhere. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:49: # Bleichenbacher's "million message" attack On 2014/03/31 20:43:57, Ryan Sleevi wrote: > heheheh. > > This doesn't really do that, but ok [Yes, aware it's pre-existing code] :-P I make no claims as to whether that comment was actually accurate. (I can also remove the comment or the logic altogether if you'd prefer. *shrug*)
Patch set 1 LGTM. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/README.c... File third_party/tlslite/README.chromium (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/README.c... third_party/tlslite/README.chromium:31: - patches/dhe_rsa.patch: Implement DHE_RSA-based key exchanges. If tlslite supports ECDHE_RSA, that is also sufficient for False Start. Does tlslite support ECDHE_RSA? https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/handshakesettings.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/handshakesettings.py:104: self.keyExchangeNames = KEY_EXCHANGE_NAMES Nit: list the self.keyExchangeNames assignment after the self.macNames assignment, to match the order of lines 15-17, unless there is a better reason to assign self.keyExchangeNames first. This comment also applies to the two changes below in this file. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/messages.py (left): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/messages.py:557: bytes = clientRandom + serverRandom + self.write()[4:] Do you know why the original code uses [4:] after self.write()? That seems wrong. Do you know why the original code uses a try block? https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:57: #Tolerate buggy IE clients This is a very old bug of IE. I suspect this bug has been fixed. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:58: if versionCheck != self.serverHello.server_version: Is self.serverHello.server_version correct? The original code uses self.version. I think it is correct. Just wanted to doublecheck. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:64: # in Python is far far far too slow. IMPORTANT: please use one of the well-known DH group parameters recommended in RFC 5246. The recommendation is not easy to find. It is in Appendix F.1.1.3, in the [IKEALG] or [MODP] reference. I would also suggest using a 2048-bit group. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:92: if dh_Yc % self.dh_p == 0: Is this check recommended by RFC 5246? I'm just curious where this check comes from. This check differs from the public key validation in https://tools.ietf.org/html/rfc2631#section-2.1.5. (We can't do the second step of that algorithm unless we have the 'q' parameter.) https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:1281: # Perform the RSA key exchange Nit: this comment should say "Perform the RSA or DHE_RSA key exchange". https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:1577: #Send ServerHello, Certificate[, CertificateRequest], Nit: add the optional ServerKeyExchange method to this comment. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:1654: for result in self._sendError(alert.description, alert.message): IMPORTANT: Should |alert| be |e|?
https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/README.c... File third_party/tlslite/README.chromium (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/README.c... third_party/tlslite/README.chromium:31: - patches/dhe_rsa.patch: Implement DHE_RSA-based key exchanges. On 2014/04/01 22:00:01, wtc wrote: > > If tlslite supports ECDHE_RSA, that is also sufficient for False Start. Does > tlslite support ECDHE_RSA? Nope. I figured DHE_RSA would be easier, but we should probably have ECDHE_RSA in our test server too at some point. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/handshakesettings.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/handshakesettings.py:104: self.keyExchangeNames = KEY_EXCHANGE_NAMES On 2014/04/01 22:00:01, wtc wrote: > > Nit: list the self.keyExchangeNames assignment after the self.macNames > assignment, to match the order of lines 15-17, unless there is a better reason > to assign self.keyExchangeNames first. > > This comment also applies to the two changes below in this file. Done. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/messages.py (left): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/messages.py:557: bytes = clientRandom + serverRandom + self.write()[4:] On 2014/04/01 22:00:01, wtc wrote: > > Do you know why the original code uses [4:] after self.write()? That seems > wrong. > > Do you know why the original code uses a try block? The try/finally thing is just so the cipherSuite gets fixed no matter what exceptions through. It's a reasonable common Python thing since you're never really sure what might throw in a dynamic language. As for the [4:], I think it's to strip off the handshake type and length. write_params doesn't include it, so we don't need it. But I'm not sure the old code worked at all anyway. It sets self.cipherSuite to None temporarily, probably so self.signature wouldn't be added, but self.write() doesn't write the parameters if self.cipherSuite is None. I'm pretty sure it's always been hashing the empty string. It just didn't break because both client and server were doing that. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:57: #Tolerate buggy IE clients On 2014/04/01 22:00:01, wtc wrote: > > This is a very old bug of IE. I suspect this bug has been fixed. Yeah, this block was just moved over. I can also just drop it if that's preferable. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:58: if versionCheck != self.serverHello.server_version: On 2014/04/01 22:00:01, wtc wrote: > > Is self.serverHello.server_version correct? The original code uses self.version. > I think it is correct. Just wanted to doublecheck. Should be. I made sure it passed all tlslite's original tests and also that the RSA key exchange still worked in our tests. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:64: # in Python is far far far too slow. On 2014/04/01 22:00:01, wtc wrote: > > IMPORTANT: please use one of the well-known DH group parameters recommended in > RFC 5246. The recommendation is not easy to find. It is in Appendix F.1.1.3, in > the [IKEALG] or [MODP] reference. > > I would also suggest using a 2048-bit group. Done. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:92: if dh_Yc % self.dh_p == 0: On 2014/04/01 22:00:01, wtc wrote: > > Is this check recommended by RFC 5246? I'm just curious where this check comes > from. This check differs from the public key validation in > https://tools.ietf.org/html/rfc2631#section-2.1.5. (We can't do the second step > of that algorithm unless we have the 'q' parameter.) I just copied the DH code from _serverAnonKeyExchange. I've switched it to the first half of the RFC2631 check. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:1281: # Perform the RSA key exchange On 2014/04/01 22:00:01, wtc wrote: > > Nit: this comment should say "Perform the RSA or DHE_RSA key exchange". Done. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:1577: #Send ServerHello, Certificate[, CertificateRequest], On 2014/04/01 22:00:01, wtc wrote: > > Nit: add the optional ServerKeyExchange method to this comment. Done. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:1654: for result in self._sendError(alert.description, alert.message): On 2014/04/01 22:00:01, wtc wrote: > > IMPORTANT: Should |alert| be |e|? Oops. Done.
Patch set 2 LGTM. https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:49: # Bleichenbacher's "million message" attack On 2014/03/31 20:43:57, Ryan Sleevi wrote: > > This doesn't really do that, but ok If self.privateKey.decrypt does RSA blinding, then this should be sufficient, right? https://codereview.chromium.org/212883008/diff/1/third_party/tlslite/tlslite/... third_party/tlslite/tlslite/tlsconnection.py:57: #Tolerate buggy IE clients On 2014/04/01 23:25:18, David Benjamin wrote: > > Yeah, this block was just moved over. I can also just drop it if that's > preferable. It's fine to keep this. We should only drop this if we have verified this. But when you submit this patch to tlslite upstream, you can mention that this IE bug may have been fixed. https://codereview.chromium.org/212883008/diff/20001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/20001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:104: if not 2 <= dh_Yc <= self.dh_p-1: Nit: add spaces around '-'?
https://codereview.chromium.org/212883008/diff/20001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/20001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:104: if not 2 <= dh_Yc <= self.dh_p-1: On 2014/04/02 03:34:20, wtc wrote: > > Nit: add spaces around '-'? Done.
Patch set 3 LGTM. I reviewed this CL carefully again because it is fun to read it :-) Therefore I have some more comments. Could you please submit your patch to the tlslite upstream? Thanks. https://codereview.chromium.org/212883008/diff/40001/net/tools/testserver/tes... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/212883008/diff/40001/net/tools/testserver/tes... net/tools/testserver/testserver.py:2177: 'Valid values are "rsa", "dhe_rsa". If ' "srp_sha, "srp_sha_rsa", "dh_anon" are also supported by tlslite. Should they also be documented here? https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/READ... File third_party/tlslite/README.chromium (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/READ... third_party/tlslite/README.chromium:31: - patches/dhe_rsa.patch: Implement DHE_RSA-based key exchanges. Nit: key exchanges => cipher suites? https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/handshakesettings.py (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/handshakesettings.py:14: # issues such as timing attacks Nit: You can make this change in a separate CL to the tlslite upstream. This comment may need to be updated given the "On the Security of RC4 in TLS and WPA" attack (http://www.isg.rhul.ac.uk/tls/). I said "may" rather than "should" because I'm not sure if the CBC implementation in tlslite is constant-time. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/handshakesettings.py:153: raise ValueError("Unknown cipher name: '%s'" % s) Do you know why the original code doesn't check other.macNames? Seems like an oversight. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:28: def __init__(self, cipherSuite, clientHello, serverHello, privateKey): Nit: it would be nice to document what privateKey is. This is only ambiguous when the server has two key pairs. In that case I think this privateKey should be the DH private key rather than the signing private key. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:34: def makeServerKeyExchange(): Nit: document what this method returns, and what it means for this method to return None. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:36: def processClientKeyExchange(clientKeyExchange): Nit: document what this method returns (the premaster secret). https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:39: Nit: use only one blank line here and before line 27? https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:52: if not premasterSecret: Hmm... On line 51 we already dereferenced premasterSecret[0] and premasterSecret[1], before checking premasterSecret is not null and has at least two elements. This seems wrong. It seems that the code should read: # On decryption failure randomize premaster secret to avoid # Bleichenbacher's "million message" attack randomPreMasterSecret = getRandomBytes(48) if not premasterSecret: premasterSecret = randomPreMasterSecret elif len(premasterSecret)!=48: premasterSecret = randomPreMasterSecret else: versionCheck = (premasterSecret[0], premasterSecret[1]) if versionCheck != self.clientHello.client_version: #Tolerate buggy IE clients if versionCheck != self.serverHello.server_version: premasterSecret = randomPreMasterSecret return premasterSecret https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:81: 15728E5A 8AACAA68 FFFFFFFF FFFFFFFF""") Nit: should lines 71-81 be indented? https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:103: # First half of RFC 2631, Section 2.1.5. Nit: Add "Validate the client's public key." This DH group doesn't have a 'q' (subprime) parameter, so we can't do the second half of the public key validation. Perhaps that check is not necessary for this group. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:106: "Suspicious dh_Yc value") Nit: Suspicious => Invalid https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:587: # Client DHE_RSA not supported. Nit: Add "TODO" to this comment. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:1600: serverKeyExchange = keyExchange.makeServerKeyExchange() Nit: the function call you use to create the ServerKeyExchange message is different from how the other messages are created. The other messages are all created with a .create() method call on the message class. I don't know how hard it is to follow that pattern. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:1666: for result in self._sendError(e.description, e.message): Nit: rename |e| to |alert|?
https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:28: def __init__(self, cipherSuite, clientHello, serverHello, privateKey): On 2014/04/02 19:11:29, wtc wrote: > > Nit: it would be nice to document what privateKey is. This is only ambiguous > when the server has two key pairs. In that case I think this privateKey should > be the DH private key rather than the signing private key. Sorry, I forgot to update this comment. I believe in that case this privateKey should be the signing private key.
lgtm
> Could you please submit your patch to the tlslite upstream? Thanks. Yeah, I'll submit it along with the pile of fixes that this one depends on. The whole chain's currently blocking on the first CL in my domino chain getting some attention from an infra person. :-) https://codereview.chromium.org/212883008/diff/40001/net/tools/testserver/tes... File net/tools/testserver/testserver.py (right): https://codereview.chromium.org/212883008/diff/40001/net/tools/testserver/tes... net/tools/testserver/testserver.py:2177: 'Valid values are "rsa", "dhe_rsa". If ' On 2014/04/02 19:11:29, wtc wrote: > > "srp_sha, "srp_sha_rsa", "dh_anon" are also supported by tlslite. Should they > also be documented here? _serverGetClientHello only enables certain sets of cipher suites depending on what we pass in. For the configuration we've chosen, only those two actually work. (Others require other sets of parameters, and they're currently mutually exclusive.) https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/READ... File third_party/tlslite/README.chromium (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/READ... third_party/tlslite/README.chromium:31: - patches/dhe_rsa.patch: Implement DHE_RSA-based key exchanges. On 2014/04/02 19:11:29, wtc wrote: > > Nit: key exchanges => cipher suites? Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/handshakesettings.py (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/handshakesettings.py:14: # issues such as timing attacks On 2014/04/02 19:11:29, wtc wrote: > > Nit: You can make this change in a separate CL to the tlslite upstream. > > This comment may need to be updated given the "On the Security of RC4 in TLS and > WPA" attack (http://www.isg.rhul.ac.uk/tls/). I said "may" rather than "should" > because I'm not sure if the CBC implementation in tlslite is constant-time. Heh. Yeah, let's do that separately or so. Though I doubt most of this code is particularly "production-grade" anyway. If none of the C-based modules are available, it falls back to pure-Python crypto implementations. For our purposes, we probably don't actually care. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/handshakesettings.py:153: raise ValueError("Unknown cipher name: '%s'" % s) On 2014/04/02 19:11:29, wtc wrote: > > Do you know why the original code doesn't check other.macNames? Seems like an > oversight. Apparently it's because FOOBAR_NAMES doubles as both the default set of names and all valid ones. And MAC_NAMES has them different. I've tweaked it so the check actually works. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:28: def __init__(self, cipherSuite, clientHello, serverHello, privateKey): On 2014/04/02 19:13:46, wtc wrote: > > On 2014/04/02 19:11:29, wtc wrote: > > > > Nit: it would be nice to document what privateKey is. This is only ambiguous > > when the server has two key pairs. In that case I think this privateKey should > > be the DH private key rather than the signing private key. > > Sorry, I forgot to update this comment. I believe in that case this privateKey > should be the signing private key. > Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:34: def makeServerKeyExchange(): On 2014/04/02 19:11:29, wtc wrote: > > Nit: document what this method returns, and what it means for this method to > return None. Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:36: def processClientKeyExchange(clientKeyExchange): On 2014/04/02 19:11:29, wtc wrote: > > Nit: document what this method returns (the premaster secret). Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:39: On 2014/04/02 19:11:29, wtc wrote: > > Nit: use only one blank line here and before line 27? Done. Although this code doesn't seem to be very consistent. :-) https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:52: if not premasterSecret: On 2014/04/02 19:11:29, wtc wrote: > > Hmm... On line 51 we already dereferenced premasterSecret[0] and > premasterSecret[1], before checking premasterSecret is not null and has at least > two elements. This seems wrong. > > It seems that the code should read: > > # On decryption failure randomize premaster secret to avoid > # Bleichenbacher's "million message" attack > randomPreMasterSecret = getRandomBytes(48) > if not premasterSecret: > premasterSecret = randomPreMasterSecret > elif len(premasterSecret)!=48: > premasterSecret = randomPreMasterSecret > else: > versionCheck = (premasterSecret[0], premasterSecret[1]) > if versionCheck != self.clientHello.client_version: > #Tolerate buggy IE clients > if versionCheck != self.serverHello.server_version: > premasterSecret = randomPreMasterSecret > return premasterSecret Haha. I guess that code never actually worked then. Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:81: 15728E5A 8AACAA68 FFFFFFFF FFFFFFFF""") On 2014/04/02 19:11:29, wtc wrote: > > Nit: should lines 71-81 be indented? *shrug* It does actually change what's in the string, but it doesn't really matter I guess; _hexStringToNumber is going to strip it all away anyway. But I suppose if I wanted to be really faithful to the string in the RFC, lines 70 and 71 should be merged. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:103: # First half of RFC 2631, Section 2.1.5. On 2014/04/02 19:11:29, wtc wrote: > > Nit: Add "Validate the client's public key." > > This DH group doesn't have a 'q' (subprime) parameter, so we can't do the second > half of the public key validation. Perhaps that check is not necessary for this > group. Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:106: "Suspicious dh_Yc value") On 2014/04/02 19:11:29, wtc wrote: > > Nit: Suspicious => Invalid Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:587: # Client DHE_RSA not supported. On 2014/04/02 19:11:29, wtc wrote: > > Nit: Add "TODO" to this comment. Done. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:1600: serverKeyExchange = keyExchange.makeServerKeyExchange() On 2014/04/02 19:11:29, wtc wrote: > > Nit: the function call you use to create the ServerKeyExchange message is > different from how the other messages are created. The other messages are all > created with a .create() method call on the message class. > > I don't know how hard it is to follow that pattern. Hrm. Yeah, I'm not sure we can follow that pattern very well and still allow keyExchange to decide whether or not to send a ServerKeyExchange. I didn't want to duplicate _serverCertKeyExchange for every key exchange mechanism; if anything, probably the other _server*KeyExchange functions should get adapted to that interface. The constructor probably will need changing through. https://codereview.chromium.org/212883008/diff/40001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:1666: for result in self._sendError(e.description, e.message): On 2014/04/02 19:11:29, wtc wrote: > > Nit: rename |e| to |alert|? Done.
Patch set 4 LGTM. Thanks! https://codereview.chromium.org/212883008/diff/60001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/60001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:39: key exchange. If the key exchange method does not send ServerKeyExchange Nit: in the key exchange => in the handshake ?
Aaaaand rebased onto trunk now that 0.4.6 is in. https://codereview.chromium.org/212883008/diff/60001/third_party/tlslite/tlsl... File third_party/tlslite/tlslite/tlsconnection.py (right): https://codereview.chromium.org/212883008/diff/60001/third_party/tlslite/tlsl... third_party/tlslite/tlslite/tlsconnection.py:39: key exchange. If the key exchange method does not send ServerKeyExchange On 2014/04/03 19:10:03, wtc wrote: > > Nit: in the key exchange => in the handshake ? Done.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/212883008/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) nacl_integration, sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/212883008/100001
Message was sent while issue was closed.
Change committed as 263169 |