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
disable tunneling for https proxy #477
Conversation
Thanks for this! 🍰 Something sat uncomfortably with me about this patch for a while, and I finally worked out what it was. It's correct behavior for HTTP over a TLS connection to a proxy, but it raises questions about how HTTPS requests should work. To do HTTPS you really need to verify the certificate of the origin server. That requires CONNECT verbs. However, I don't think CONNECT really works when connecting over HTTPS to a proxy: you end up tunning TLS over TLS, which is just ridiculous and I doubt it's supported by anything. I think in principle this patch is fine, and it does improve the behaviour of HTTP over HTTPS proxies, but I think that rather than disable tunneling for HTTPS requests over HTTPS proxies we should just refuse to send them altogether. |
Having discussed with @phluid61 on Twitter (see here), I suspect that you can meaningfully use CONNECT to tunnel HTTPS through TLS in principle. In practice, I know of no way to get OpenSSL to do that, let alone to get I have absolutely no idea how to do that. I still think refusing to do anything at all is the way forward there. |
@Lukasa yea, i agree that:
seems to follow but i wasn't able to simply support it w/ urllib3 <-> httplib (initial poc worked but realized i didnt need it) and couldn't find examples of doing that w/ other tools. so like you i assumed its a rare enough requirement to leave unsupported for now (although it might be nice to doc as a known limitation). this patch just lets us use https proxies w/ http requests using normal (tunneling is n/a here) proxy semantics. did you agree w/ that addition (previously it would just fail @ runtime)? |
I think the new behaviour is totally reasonable, though I have to review the patch again to try to understand why it's necessary. |
@@ -109,6 +113,10 @@ def setUpClass(cls): | |||
cls.proxy_server, cls.proxy_port = run_tornado_app( | |||
app, cls.io_loop, None, 'http', cls.proxy_host) | |||
|
|||
app = web.Application([(r'.*', ProxyHandler)]) |
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.
Is there a reason why you are running two proxies in one test case? Would make more sense if you make a separate class as HTTPS proxy test case.
I don't think this is an acceptable solution. In this case the proxy can intercept all data from HTTPS connections because the proxy can encrypt all TLS data sent to it. BTW, some information about encrypted proxy connections can be found here. |
Thank you for creating this pull request, unfortunately, for one reason or |
i dont think https proxying http reqs works now anyway (added test fails without patch). by disabling tunneling for https proxies we can now: https proxy http requests (no http connect tunnel est needed).
closes #476