Skip to content

Conversation

@vadz
Copy link
Contributor

@vadz vadz commented Sep 8, 2017

Calling request() twice on the same client configured to connect to a
server via HTTPS didn't work under Unix because we issues CONNECT for
every request, meaning that, for the second one, we sent CONNECT via an
already established connection to the end server itself which,
unsurprisingly, didn't work at all.

Fix this by only setting up SSL tunnelling for a new connection but not
for the already used one.


The problem can be easily reproduced with the following patch to the BingRequest sample:

diff --git a/Release/samples/BingRequest/bingrequest.cpp b/Release/samples/BingRequest/bingrequest.cpp
index 726d6d19..23fdab06 100644
--- a/Release/samples/BingRequest/bingrequest.cpp
+++ b/Release/samples/BingRequest/bingrequest.cpp
@@ -62,15 +62,17 @@ int main(int argc, char *args[])
     const string_t searchTerm = args[1];
     const string_t outputFileName = args[2];

+        http_client client(U("https://www.bing.com/"), client_config_for_proxy());
+
+        for(int i = 0; i < 2;i++) {
     // Open a stream to the file to write the HTTP response body into.
     auto fileBuffer = std::make_shared<streambuf<uint8_t>>();
-    file_buffer<uint8_t>::open(outputFileName, std::ios::out).then([=](streambuf<uint8_t> outFile) -> pplx::task<http_response>
+    file_buffer<uint8_t>::open(outputFileName, std::ios::out).then([=,&client](streambuf<uint8_t> outFile) -> pplx::task<http_response>
     {
         *fileBuffer = outFile;

         // Create an HTTP request.
         // Encode the URI query since it could contain special characters like spaces.
-        http_client client(U("http://www.bing.com/"), client_config_for_proxy());
         return client.request(methods::GET, uri_builder(U("/search")).append_query(U("q"), searchTerm).to_string());
     })

@@ -90,6 +92,7 @@ int main(int argc, char *args[])

     // Wait for the entire response body to be written into the file.
     .wait();
+        }

     return 0;
 }

Running the sample without this PR would result in something like

terminate called after throwing an instance of 'web::http::http_exception'
  what():  Expected a 200 response from proxy, received: 403

where 403 is the status returned by the actual server (and not the proxy) when it receives CONNECT.

Calling request() twice on the same client configured to connect to a
server via HTTPS didn't work under Unix because we issues CONNECT for
every request, meaning that, for the second one, we sent CONNECT via an
already established connection to the end server itself which,
unsurprisingly, didn't work at all.

Fix this by only setting up SSL tunnelling for a new connection but not
for the already used one.
@deeringc
Copy link
Contributor

It might also be worth updating the https_proxy manual test case (in proxy_tests.cpp) to make 2 requests on the same client object.

@leetal
Copy link
Contributor

leetal commented Nov 8, 2018

@BillyONeal This issue is a dealbreaker for us right now. We currently patch the source locally since this fix works as expected on any ASIO implementation. If @vadz does not supply a test-case we are probably better off creating a new PR that contains this fix and the required reuse-connection test. Or, just skip the test, merge this, and add a test later on 🙈? This PR will by far fix the reuse connection issue through proxies over HTTPS anyway... We have been using it in production for over a year without issues.

@BillyONeal
Copy link
Member

@leetal The problem is that we don't understand what's going on here, so without a test this is likely to be regressed.

@vadz
Copy link
Contributor Author

vadz commented Nov 12, 2018

@BillyONeal Do you have any specific questions? I don't remember all the details one year on neither, but AFAIR, this was relatively straightforward: you just can't CONNECT via an existing proxied connection.

The example to the sample should allow you to reproduce the problem and I can try making this into an automated test if this is really indispensable for the PR to be merged, although I admit I have no idea when will I have time for this...

…euse

# Conflicts:
#	Release/src/http/client/http_client_asio.cpp
@BillyONeal
Copy link
Member

Looking at this again I think it makes sense enough to merge. I still would prefer to see tests though :)

@BillyONeal BillyONeal merged commit 5b32e16 into microsoft:master Jun 13, 2019
@vadz vadz deleted the fix-proxy-reuse branch June 13, 2019 21:38
@vadz
Copy link
Contributor Author

vadz commented Jun 13, 2019

Thanks for merging and sorry for never finding time for this test...

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants