Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.
861c91f to
75e2e2fCompare
Pending HTTPS targets
Working only with tunnel proxies Fails if ssl_insecure is False (default) because we're not sending SNI?
75e2e2f to
c802c69CompareUpgrade to TLS using only the hostname
yeraydiazdiaz
commented
Apr 12, 2020
A pesky warning in mitmproxy led me to find and fix a bug, yay warnings! 😄 This is now ready for a final review. 🙏 |
florimondmanca
commented
Apr 12, 2020
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. :) |
yeraydiazdiaz
commented
Apr 12, 2020
Fair enough, let me see what I can do. |
yeraydiazdiaz
commented
Apr 12, 2020
Closing in favor of #57. |
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 @florimondmancaoutlined 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.