Skip to content
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

Merged
merged 37 commits into from Feb 27, 2019

Conversation

sethmlarson
Copy link
Member

@sethmlarson sethmlarson commented Dec 7, 2018

TLS 1.3 support came in OpenSSL 1.1.1 and Python 3.7 and we should start testing against it.
TLS 1.3 will be available under the following conditions:

  • You're using Python 3.7+ compiled against OpenSSL 1.1.1+ OR
  • You're using SecureTransport on macOS 10.13+ with TLS 1.3 enabled OR
  • You're using pyOpenSSL with cryptography 2.5+

Most of our users will be using TLS 1.3 through pyOpenSSL.

If TLS 1.3 is available and you don't hard-code a protocol version it'll be used by default.

Closes #1368
Closes #1386
Closes #1269
Closes #1451
Closes #1500
Closes #1537

@codecov-io
Copy link

codecov-io commented Dec 7, 2018

Codecov Report

Merging #1496 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1496      +/-   ##
==========================================
- Coverage   99.94%   99.72%   -0.23%     
==========================================
  Files          22       22              
  Lines        1826     1810      -16     
==========================================
- Hits         1825     1805      -20     
- Misses          1        5       +4
Impacted Files Coverage Δ
src/urllib3/util/__init__.py 100% <ø> (ø) ⬆️
src/urllib3/util/ssl_.py 100% <100%> (ø) ⬆️
src/urllib3/connection.py 96.89% <0%> (-3.11%) ⬇️
src/urllib3/util/url.py 98.8% <0%> (-0.06%) ⬇️
src/urllib3/util/timeout.py 100% <0%> (ø) ⬆️
src/urllib3/response.py 100% <0%> (ø) ⬆️
src/urllib3/fields.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2a96c6...15c3af7. Read the comment docs.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the high is now TLSv1.3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update this. Also is kTLSProtocol13 not available? I'm not sure when it was added to SecureTransport but I'm getting Illegal Parameter errors for using it. :(

Copy link
Member

Choose a reason for hiding this comment

The 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!

Copy link
Member Author

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to still add the license of oscrypto to urllib3.contrib._securetransport.bindings and remove the TLS 1.3 ciphers from our default cipher list as they're set by a different function that's not exposed by Python yet.

@sethmlarson
Copy link
Member Author

I rolled the IPv6 feature into this PR to avoid a monster of a merge conflict if that's fine with reviewers. If we want to split that into it's own PR I can do so.

@sethmlarson
Copy link
Member Author

After seeing the latest build on master fail for the same reason as this PR makes me feel better about this issue. Will have to investigate on master, might be due to the new cryptography version?

@sethmlarson
Copy link
Member Author

cc @theacodes @shazow I think this PR is ready for another round of review. The failures on the requests downstream tests are failing both on our master and on requests master.

@sethmlarson sethmlarson requested review from shazow and theacodes and removed request for shazow February 27, 2019 15:49
Copy link
Contributor

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing sticks out to me here.

@sethmlarson
Copy link
Member Author

sethmlarson commented Feb 27, 2019

Unfortunately I can't squash+merge myself due to CI failures. Windows had a transient failure and macOS Python is now failing to build cryptography wheels which I'll investigate outside of this PR. I'm ready for this to be merged anytime.

Thanks reviewers! :)

@theacodes
Copy link
Member

theacodes commented Feb 27, 2019 via email

@sethmlarson
Copy link
Member Author

@theacodes Yes please! ✨

@theacodes theacodes merged commit 1e9ab5a into urllib3:master Feb 27, 2019
@sethmlarson sethmlarson deleted the tls-1.3 branch February 27, 2019 22:06
@webknjaz
Copy link

webknjaz commented Apr 8, 2019

Thanks for the fix!

@theacodes @shazow @sethmlarson It looks like the last release was ~5 months ago. Is there any chance to get this released anytime soon?

@sethmlarson
Copy link
Member Author

@webknjaz The next release is pending #1531 and python-hyper/rfc3986#50.

@webknjaz
Copy link

webknjaz commented Apr 8, 2019

@sethmlarson thanks for the hint!

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
…b3#1496)

* Add tests for specific TLS/SSL versions

* Add change and update bindings

* SSLSocket.version() not available sometimes

* Add support for kTLSProtocolMaxSupported

* Try setProtocolVersionMax again if error

* Get ctypes.c_uint.value for SSLSocket.version()

* Opt-in TLS 1.3 on macOS 10.13

* Update tornado to 5.1.1

* Add documentation updates for TLSv1.3

* Add wbond/oscrypto license to contrib/securetransport

* Remove all TLS 1.3 ciphersuites from DEFAULT_CIPHERS

* Experiment showing cipher list per protocol

* Update test_https.py

* Update test_https.py

* Update test_https.py

* Update changelog wording to exclude pyOpenSSL

* minor rewording

* Add support for IPv6 in subjectAltName

* Don't use OP_ALL

* Update CHANGES.rst

* No PROTOCOL_TLSv1_3

* Remove DSS, rearrange SecureTransport ciphers

* Use ECDSA before RSA with ECDHE

* ReviReorder ciphers

* ECDHE

* Update test_https.py

* Turns out we don't need version detection

* Reorder per Hyneks post and favor ephemeral

* Refactor HTTPS unit tests

* Fix up tests

* Test locking pytest-httpbin

* Update requests.sh

* remove whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants