Fix tunnel proxy and support HTTPS#55
Conversation
| if isinstance(event, h11.Data): | ||
| yield bytes(event.data) | ||
| elif isinstance(event, h11.EndOfMessage): | ||
| elif isinstance(event, (h11.EndOfMessage, h11.PAUSED)): |
There was a problem hiding this comment.
From the h11 docs:
PAUSED is a little more subtle: [...] A successful CONNECT or Upgrade: request has caused the connection to switch to some other protocol (see Switching protocols). This PAUSED state is permanent; you should abandon this Connection and go do whatever it is you’re going to do with your new protocol.
| connect_url = self.proxy_origin + (target,) | ||
| connect_headers = self.proxy_headers | ||
| proxy_response = await connection.request( | ||
| connect_headers = self.proxy_headers or [(b"host", self.proxy_origin[1])] |
There was a problem hiding this comment.
The host header is required by h11 but not typically passed as part of the proxy headers.
Not sure if we want to merge the headers here or leave that to the caller to sort out.
yeraydiazdiaz
force-pushed
the
tunnel-proxy-https
branch
from
861c91f to
75e2e2f
Compare
Pending HTTPS targets
Working only with tunnel proxies Fails if ssl_insecure is False (default) because we're not sending SNI?
yeraydiazdiaz
force-pushed
the
tunnel-proxy-https
branch
from
75e2e2f to
c802c69
Compare
Upgrade to TLS using only the hostname
|
A pesky warning in mitmproxy led me to find and fix a bug, yay warnings! 😄 This is now ready for a final review. 🙏 |
|
Thanks! Haven't got around to reviewing this, but will try soon. Side note — I noticed the "and" in the PR title, is there a way "Fix tunnel proxy" and "HTTPS support" could be two separate, smaller-size (and easier to review?) PRs, or are these two too entangled? I'll admit I'm still a proxy noob so smaller diffs would help me provide better quality reviews on these matters. :) |
|
Fair enough, let me see what I can do. |
|
Closing in favor of #57. |
2 participants
Fixes #54
Probably helps with https://github.com/encode/httpx/issues/897
CONNECTis an HTTP method handled by h11.AsyncHTTPConnectionand create a new h11 connection to the target.Only HTTP is covered by the tests using mitmproxy, I couldn't quite get HTTPS requests to work using mitmproxy but it did work using Squid as @florimondmanca outlined in the HTTPX issue (thanks for that, it was very helpful 👍 )
These changes sit a bit at the edge of our abstractions since the socket is now exposed a few levels above what it used to. I'm keen to hear your thoughts on whether we need some refactoring.