-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 HTTP/1.1 ALPN support #1894
Conversation
b6b41bd
to
c7c4c5c
Compare
I wonder if it makes sense for us to unconditionally add urllib3 only speaks HTTP/1.1 (or 1.0 if server is too but we prefer not to) so I don't see the need for this to be configurable. |
+1 to avoid adding yet another knob if we can avoid it |
Good job so far, thanks! |
54abdb6
to
9457440
Compare
Codecov Report
@@ Coverage Diff @@
## master #1894 +/- ##
===========================================
+ Coverage 99.95% 100.00% +0.04%
===========================================
Files 23 23
Lines 2049 2054 +5
===========================================
+ Hits 2048 2054 +6
+ Misses 1 0 -1
Continue to review full report at Codecov.
|
I've removed API to customize ALPN protocols and most of What do you think? |
f35fc64
to
a47ea10
Compare
a47ea10
to
037975f
Compare
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.
This is stellar, thank you for all this work! 🎉 I left a few comments :)
@@ -374,6 +376,19 @@ def _set_ciphers(self): | |||
) | |||
_assert_no_error(result) | |||
|
|||
def _set_alpn_protocols(self, protocols): |
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.
Would be great to confirm this works via capturing a TLS handshake on a supported platform, have we tried that yet?
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.
There is a test in TestALPN::test_alpn_protocol_in_first_request_packet
, is that what you meant?
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 meant it'd be great to have an indication this all works on a live macOS 10.12+ system by verifying manually w/ Wireshark. I don't have a macOS machine so I can't help here but we can do that outside this PR though :)
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.
Looks good to me :)
Added ALPN support, resolves #1892.
Sending ALPN protocols (
["http/1.1"]
) by default, if backend support is available.TODO:
TestHTTPS_ALPN
introduced too much tests.Support:
Requires
Python>=2.7.9
(forSSLContext
) and backend support:OpenSSL>=1.0.2
OpenSSL>=1.0.2, PyOpenSSL>=0.15, cryptography>=0.5
macOS >= 10.12