New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for TLS 1.3 to all HTTPSConnection implementations #1496
Changes from 4 commits
f59e6b3
417c333
eaa6f38
20b5eb5
ab1e014
fefc403
32de3a5
694b164
9939524
493d78f
9fe5269
522ed0c
b364e70
209873a
490251b
93f1d3a
95e5935
1ae8674
4f6f74d
a071345
a18f623
a97cefe
ca52ca5
7e4e485
5745dfb
2bc2742
6a4d3dc
9fc3c5a
423df77
4244f75
3817503
18001cd
eac9b3a
3b9c529
ccb3737
9e78231
15c3af7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,7 +124,7 @@ | |
# Basically this is simple: for PROTOCOL_SSLv23 we turn it into a low of | ||
# TLSv1 and a high of TLSv1.2. For everything else, we pin to that version. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the high is now TLSv1.3 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll update this. Also is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://developer.apple.com/documentation/security/sslprotocol/ktlsprotocol13?language=objc mentions macOS 10.13+, which is what Travis uses. That's all I know, sorry! |
||
_protocol_to_min_max = { | ||
ssl.PROTOCOL_SSLv23: (SecurityConst.kTLSProtocol1, SecurityConst.kTLSProtocol12), | ||
ssl.PROTOCOL_SSLv23: (SecurityConst.kTLSProtocol1, SecurityConst.kTLSProtocolMaxSupported), | ||
} | ||
|
||
if hasattr(ssl, "PROTOCOL_SSLv2"): | ||
|
@@ -147,6 +147,10 @@ | |
_protocol_to_min_max[ssl.PROTOCOL_TLSv1_2] = ( | ||
SecurityConst.kTLSProtocol12, SecurityConst.kTLSProtocol12 | ||
) | ||
if hasattr(ssl, "PROTOCOL_TLSv1_3"): | ||
_protocol_to_min_max[ssl.PROTOCOL_TLSv1_3] = ( | ||
SecurityConst.kTLSProtocol13, SecurityConst.kTLSProtocol13 | ||
) | ||
if hasattr(ssl, "PROTOCOL_TLS"): | ||
_protocol_to_min_max[ssl.PROTOCOL_TLS] = _protocol_to_min_max[ssl.PROTOCOL_SSLv23] | ||
|
||
|
@@ -667,6 +671,25 @@ def getpeercert(self, binary_form=False): | |
|
||
return der_bytes | ||
|
||
def version(self): | ||
protocol = Security.SSLProtocol() | ||
result = Security.SSLGetNegotiatedProtocolVersion(self.context, ctypes.byref(protocol)) | ||
_assert_no_error(result) | ||
if protocol == SecurityConst.kTLSProtocol13: | ||
return 'TLSv1.3' | ||
elif protocol == SecurityConst.kTLSProtocol12: | ||
return 'TLSv1.2' | ||
elif protocol == SecurityConst.kTLSProtocol11: | ||
return 'TLSv1.1' | ||
elif protocol == SecurityConst.kTLSProtocol1: | ||
return 'TLSv1' | ||
elif protocol == SecurityConst.kSSLProtocol3: | ||
return 'SSLv3' | ||
elif protocol == SecurityConst.kSSLProtocol2: | ||
return 'SSLv2' | ||
else: | ||
raise ssl.SSLError('Unknown TLS version: %r' % protocol) | ||
|
||
def _reuse(self): | ||
self._makefile_refs += 1 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -626,7 +626,7 @@ def socket_handler(listener): | |
# First request should fail. | ||
response = pool.urlopen('GET', '/', retries=0, | ||
preload_content=False, | ||
timeout=Timeout(connect=1, read=0.001)) | ||
timeout=Timeout(connect=1, read=0.1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😢 |
||
try: | ||
self.assertRaises(ReadTimeoutError, response.read) | ||
finally: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just for my own edification) what's the benefit of doing a classmethod wrapper here over a property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be mistaken because I've never tried it but properties aren't available to the class only the instance? We need it for
_start_server()
which is also aclassmethod
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why the change was necessary, I assumed the old
certs
would be available to the class too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a function here because we're making a copy of
DEFAULT_CERTS
and changing thessl_version
key per-test for the TLS version tests. If there's an alternate way to achieve this I could change this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're saying before we'd have to do something like...
Instead of...
?
Not obvious to me what the advantage of the latter one is, but I don't have strong feelings here. :)