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

Added forward proxy support and included an example #1893

Merged
merged 4 commits into from Nov 6, 2017

Conversation

Projects
None yet
2 participants
@mattremmel
Contributor

mattremmel commented Aug 17, 2017

Overview of changes:

  • Generalized 'HTTPReverseProxySettings' into 'HTTPProxySettings' and added a bool for setting reverse proxy mode.
  • Moved the 'handleRequest' code out of 'reverseProxyRequest' and into a general 'proxyRequest' function.
  • Created a 'forwardProxyRequest' function and updated the 'reverseProxyRequest' function to then call into 'proxyRequest' with the properly set 'HTTPProxySettings' where necessary.
  • Created new 'listenHTTPForwardProxy' methods to compliment the existing 'listenHTTPReverseProxy' functions.

Testing:
All unit tests pass. I tested both the existing 'http_reverse_proxy' example and the new 'http_forward_proxy' example to ensure they were both working. NOTE*: the 'http_forward_proxy' example will only work with HTTP sites, not HTTPS. Trying to access an HTTPS site will show error 'proxy is refusing connections'. Setting up the HTTP listener to use SSL will solve this, although that will drop support for regular HTTP sites.

Notes:
Ideally the example would be better off working with both HTTP and HTTPS, so it could truly be used like a general purpose forward proxy. This would require having a listener that can handle both normal and TLS encrypted TCP connections. I don't believe Vibe.d currently supports this (let me know if I'm wrong), but I have some ideas on how to add support for this.

If there are any desired changes or updates to this PR, feel free to let me know. I won't take any offense.

@mattremmel

This comment has been minimized.

Show comment
Hide comment
@mattremmel

mattremmel Aug 21, 2017

Contributor

I'm not sure what the best way to go about unit testing this is. It doesn't look like anything from the original proxy.d was being unit tested to begin with. I'm open to ideas.

Contributor

mattremmel commented Aug 21, 2017

I'm not sure what the best way to go about unit testing this is. It doesn't look like anything from the original proxy.d was being unit tested to begin with. I'm open to ideas.

@s-ludwig

Thanks, it's good to get the generic nature of the code available this way!

Ideally the example would be better off working with both HTTP and HTTPS, so it could truly be used like a general purpose forward proxy. This would require having a listener that can handle both normal and TLS encrypted TCP connections. I don't believe Vibe.d currently supports this (let me know if I'm wrong), but I have some ideas on how to add support for this.

The way to achieve this is to call listenHTTP twice with the same request handler, but with two different HTTPServerSettings, once with and once without TLS enabled. But shouldn't this be more or less irrelevant for the forward proxy, since HTTPS requests are usually forwarded using CONNECT requests tunneled through whichever protocol the proxy exposes?

Show outdated Hide outdated http/vibe/http/proxy.d
Show outdated Hide outdated http/vibe/http/proxy.d
Show outdated Hide outdated http/vibe/http/proxy.d
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 22, 2017

Member

I'm not sure what the best way to go about unit testing this is. It doesn't look like anything from the original proxy.d was being unit tested to begin with. I'm open to ideas.

I was under the impression that there was a high level test in the "tests" directory, but obviously it was just the TCP proxy test that I had in mind. I would approach this in a similar way, setting up one normal server, one forward/reverse proxy and then firing requests at the proxy and the normal server to compare the results (that would also test the proxy support in the HTTP client). Would be great to have a test, but since none existed up to now, I wouldn't see that as mandatory for this PR.

Member

s-ludwig commented Aug 22, 2017

I'm not sure what the best way to go about unit testing this is. It doesn't look like anything from the original proxy.d was being unit tested to begin with. I'm open to ideas.

I was under the impression that there was a high level test in the "tests" directory, but obviously it was just the TCP proxy test that I had in mind. I would approach this in a similar way, setting up one normal server, one forward/reverse proxy and then firing requests at the proxy and the normal server to compare the results (that would also test the proxy support in the HTTP client). Would be great to have a test, but since none existed up to now, I wouldn't see that as mandatory for this PR.

@s-ludwig s-ludwig added the needs input label Sep 3, 2017

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 25, 2017

Member

@mattremmel: Do you have the time to make these adjustments? If not, I could also adopt the branch and add the changes on top, as this opportunity for generalization IMO should definitely be taken.

Member

s-ludwig commented Oct 25, 2017

@mattremmel: Do you have the time to make these adjustments? If not, I could also adopt the branch and add the changes on top, as this opportunity for generalization IMO should definitely be taken.

@mattremmel

This comment has been minimized.

Show comment
Hide comment
@mattremmel

mattremmel Oct 25, 2017

Contributor

@s-ludwig Hey, sorry. I tabled this when work got busy and never circled back around. I can finish these changes either today or tomorrow. I'll update the PR when I am done. Thanks.

Contributor

mattremmel commented Oct 25, 2017

@s-ludwig Hey, sorry. I tabled this when work got busy and never circled back around. I can finish these changes either today or tomorrow. I'll update the PR when I am done. Thanks.

@mattremmel

This comment has been minimized.

Show comment
Hide comment
@mattremmel

mattremmel Oct 28, 2017

Contributor

So I took another pass at this and finished the changes you requested I believe. I've never worked on a library before, only applications, so the API backwards compatibility shamefully never crossed my mind, but that should all be taken care of now.

I liked the ProxyMode idea, so I've implemented that. I also generalized two of the core functions but added compatibility aliases for backwards compatibility, so let me know what you think. To me, it makes more sense now that a forward proxy mode is an option.

I'm working on writing some unit tests, and I also want to do more on supporting HTTPS, and unit tests will probably help speed up and verify that. I'm not an expert by any means, but typically proxies like Charles or Burp Suite run on a single port (8080) and are capable of forwarding both HTTP and HTTPS traffic through the same port simultaneously. I don't believe this is the same as HTTP CONNECT, although that can be achieved via HTTP CONNECT. However, in my tests, enabling HTTP CONNECT did not solve that problem because my browser was trying to establish a HTTPS connection with the proxy which was failing because it was just an HTTP listener without TLS enabled. My understanding is that proxies like Charles or Burp Suite, will effectively run something like a multiplexer listener, that will detect whether a TLS connection is attempting to be established, and then dispatch the connection to either an SSL/TLS handler or just normal HTTP handler.

Firefox and most other browsers only allow configuring a single HTTP proxy to use, and don't allow specifying a proxy for TLS and a proxy for Non-TLS traffic. Perhaps I misunderstood your suggestion about calling listenHTTP twice, but that would only work if both instances could listen on the same port without interference. Maybe they can?

Contributor

mattremmel commented Oct 28, 2017

So I took another pass at this and finished the changes you requested I believe. I've never worked on a library before, only applications, so the API backwards compatibility shamefully never crossed my mind, but that should all be taken care of now.

I liked the ProxyMode idea, so I've implemented that. I also generalized two of the core functions but added compatibility aliases for backwards compatibility, so let me know what you think. To me, it makes more sense now that a forward proxy mode is an option.

I'm working on writing some unit tests, and I also want to do more on supporting HTTPS, and unit tests will probably help speed up and verify that. I'm not an expert by any means, but typically proxies like Charles or Burp Suite run on a single port (8080) and are capable of forwarding both HTTP and HTTPS traffic through the same port simultaneously. I don't believe this is the same as HTTP CONNECT, although that can be achieved via HTTP CONNECT. However, in my tests, enabling HTTP CONNECT did not solve that problem because my browser was trying to establish a HTTPS connection with the proxy which was failing because it was just an HTTP listener without TLS enabled. My understanding is that proxies like Charles or Burp Suite, will effectively run something like a multiplexer listener, that will detect whether a TLS connection is attempting to be established, and then dispatch the connection to either an SSL/TLS handler or just normal HTTP handler.

Firefox and most other browsers only allow configuring a single HTTP proxy to use, and don't allow specifying a proxy for TLS and a proxy for Non-TLS traffic. Perhaps I misunderstood your suggestion about calling listenHTTP twice, but that would only work if both instances could listen on the same port without interference. Maybe they can?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 5, 2017

Member

Perhaps I misunderstood your suggestion about calling listenHTTP twice, but that would only work if both instances could listen on the same port without interference. Maybe they can?

No you are right, that won't work on the same port without additional effort currently. I didn't check the traffic for proxy requests for quite a while, so maybe things have indeed changed. However, the question would be how a HTTPS request that simply goes through the proxy would specify its destination to the proxy and at the same time would remain securely encrypted end-to-end. What the mentioned proxies usually have to do is to generate a dummy certificate for the HTTPS site and basically act as a man in the middle. I'd still assume that they simply intercept CONNECT requests that way, but I'll double check that.

The changes look good now. The CI tests just currently fail due to the three trailing spaces in lines 59 to 61 and due to an unrelated error obviously caused by some changes in the Travis-CI setup. From my side we could merge this as a first step and add tackle the HTTPS and testing issues separately.

Member

s-ludwig commented Nov 5, 2017

Perhaps I misunderstood your suggestion about calling listenHTTP twice, but that would only work if both instances could listen on the same port without interference. Maybe they can?

No you are right, that won't work on the same port without additional effort currently. I didn't check the traffic for proxy requests for quite a while, so maybe things have indeed changed. However, the question would be how a HTTPS request that simply goes through the proxy would specify its destination to the proxy and at the same time would remain securely encrypted end-to-end. What the mentioned proxies usually have to do is to generate a dummy certificate for the HTTPS site and basically act as a man in the middle. I'd still assume that they simply intercept CONNECT requests that way, but I'll double check that.

The changes look good now. The CI tests just currently fail due to the three trailing spaces in lines 59 to 61 and due to an unrelated error obviously caused by some changes in the Travis-CI setup. From my side we could merge this as a first step and add tackle the HTTPS and testing issues separately.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 5, 2017

Member

Okay, so I checked with Wireshark on Windows and at least Firefox still performs a CONNECT if I set a proxy with the same port for all protocols. When this is merged I can have another look at where it may have gone wrong there.

Member

s-ludwig commented Nov 5, 2017

Okay, so I checked with Wireshark on Windows and at least Firefox still performs a CONNECT if I set a proxy with the same port for all protocols. When this is merged I can have another look at where it may have gone wrong there.

@s-ludwig s-ludwig removed the needs input label Nov 5, 2017

@s-ludwig s-ludwig merged commit 41ac1a4 into vibe-d:master Nov 6, 2017

3 of 4 checks passed

codecov/patch 0% of diff hit (target 2.164%)
Details
codecov/project 59.803% (+57.638%) compared to ba71a9a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

s-ludwig added a commit that referenced this pull request Nov 6, 2017

Fix proxy CONNECT request handling.
There were two issues:
- The destination resource ("host:port") was not parsed correctly
- The response header was missing

See #1893.
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 6, 2017

Member

Turned out that CONNECT handling was simply broken. See #1973 for the fix.

Member

s-ludwig commented Nov 6, 2017

Turned out that CONNECT handling was simply broken. See #1973 for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment