Implement HTTPS proxy support

python-hyper / hyper

HTTP/2 for Python.

http://hyper.rtfd.org/en/latest/

MIT License

1.05k stars 192 forks source link

Implement HTTPS proxy support #322

Closed KostyaEsmukov closed 7 years ago

KostyaEsmukov commented 7 years ago

Related issue: #304

~~This is a very work in progress. It seems to work, but I would like to get your feedback whether I'm on a right track or not.~~

~~The main concern I have is about httplib: is it OK to use it for just sending CONNECT method to a proxy?~~ Though HTTP/2 standard defines CONNECT method as well, I guess it is an overkill to use it for just getting a raw socket from a proxy.

TODO:

[x] Requests adapter: respect custom request ports when choosing a proxy
[x] Validate proxy support on h2 w/o tls (h2c) (should we even think about it? I'm not sure)
[x] ~~raise appropriate IO exceptions to distinguish between connection failure to proxy and a general IO exception~~ (to be done in a follow-up PR)
[x] Implement proxy authorization support
[x] ~~Strip Proxy-* headers from response on HTTP proxying~~ (seems unneeded. requests doesn't do it)

KostyaEsmukov commented 7 years ago

Thank you for your feedback, I appreciate it! I guess I have addressed all of the notes. I'm going to implement the last two points of my TODO today and then we may start polishing it for a final state 😊

Lukasa commented 7 years ago

Cool, in that case I'll hold off reviewing until you deal with your last few TODOs. Try to reduce the amount of back-and-forth we do. :smile:

KostyaEsmukov commented 7 years ago

Eww, this diff is already big! I guess I've done with this feature. There's more I want to do in hyper, but lets hold it for another PR(s). As for now proxy support is complete IMO. Waiting for a review 😊

KostyaEsmukov commented 7 years ago

No problem! Should I force-push to this branch, or open a new PR?

I'm asking because I don't know how would GitHub handle the comments in this PR once these commits will be gone (or maybe it is OK to lose them? 😄)

Lukasa commented 7 years ago

You can force push to this branch, it'll work just fine.

KostyaEsmukov commented 7 years ago

Hi @Lukasa , is anything missing here?

Lukasa commented 7 years ago

Ok, if we bring this up-to-date with current master we should be ready to go.

Lukasa commented 7 years ago

Very nicely done! Thanks for spending the time getting this into perfect shape: I know it wasn't easy, and I want you to know I appreciate it greatly.