Support CURL based HTTP client #1182

franksinankaya · 2019-07-10T19:37:55Z

This is a curl based HTTP client that allows us to use custom SSL engines. Adaptation layer is a stripped down version of http_client_winhttp.cpp. WinHTTP functions (aka winhttppal) has been written from scratch against the MULTI curl interface.

While the current code is not complete, it is %95 functional.

1700 test pass
4 test failures

compression_tests:compress_client_server
connections_and_errors:server_close_without_responding
outside_tests:request_headers
progress_handler_tests:set_progress_handler_request_timeout

3 test not supported
outside_tests:server_hostname_host_override
outside_tests:server_hostname_host_override_after_upgrade
client_construction:ssl_context_callback_https

proxy and authentication tests not run.

reneme · 2019-07-11T13:18:44Z

That sounds like a cool extension of cpprestsdk. Particularly, because it might have the potential to replace the somewhat crude HTTP protocol implementation of http_client_asio.cpp.

A while ago we proposed a pull request to integrate the ASIO client with Botan to remove the dependency of OpenSSL (and SChannel/WinHTTP for that matter). At the time, this PR was actually meant as a draft and @garethsb-sony correctly noted that it might be time to rethink the HTTP/TLS backend architecture of cpprestsdk.

In case this pull request is meant in a similar way, we would be interested to join the discussion and potentially help to get our Botan backend upstream one or the other way. Currently we maintain it as a custom fork of cpprestsdk, but maybe it might be helpful for the community as well.

For the matter of ideas: Wrapping boost Beast under cpprestsdk would probably be a great addition as well.

franksinankaya · 2019-07-11T14:38:55Z

Thanks for the interest. We actually want to go to OpenSSL route using CURL as the http backend so that we can use our custom OpenSSL engine during HTTP negotiation.

Primary driving decision is to abstract the primary keys from the application for security reasons. Our investigation showed that this is not possible with boost ASIO or boost beast. We didn't have any concerns about the complexity involved in ASIO layer since we never looked at it.

CURL seems to be the right fit. It is small and scalable.

The only reason why we stacked curl on top of winhttp is because winhttp interface is well documented. We just had to emulate winhttp API just enough with CURL to get to functional state.

We could have written a curl only client but we see bigger interest in having a light WinHTTP layer on other applications. We are planning to post a winhttppal project in github in the future.

BillyONeal

This change needs associated changes to azure-pipelines.yaml to ensure tests are passing for this backend, and so that future maintenance doesn't regress its behavior.

I'm not a fan of now 3 layers of wrappers (cpprestsdk/curl-winhttp-wrapper/curl) in particular because the names the winhttp wrapper use conflict with the real winhttp function names, meaning the backend can't be compiled or used on Windows.

Release/CMakeLists.txt

BillyONeal · 2019-07-12T02:23:10Z

Release/include/cpprest/http_client.h

        , m_tlsext_sni_enabled(true)
 #endif
-#if defined(_WIN32) && !defined(__cplusplus_winrt)
+#if (defined(_WIN32) && !defined(__cplusplus_winrt)) || defined(CPPREST_FORCE_HTTP_CLIENT_CURL)


Hmmm I think with this we are moving to a place where the backend is not just a function of the platform, it's something someone might want to trigger from. We should probably change the build infrastructure to set CPPREST_FORCE_HTTP_CLIENT_Xxx where Xxx can be one of the backends, and remove all of this repeated platform testing (here (defined(_WIN32) && !defined(__cplusplus_winrt))) in the codebase.

That's not directly related to this review of course but I want to investigate doing that before merging this.

Release/src/CMakeLists.txt

Release/src/http/client/http_client_curl.cpp

Release/src/http/client/winhttppal/winhttppal.cpp

Release/src/http/client/http_client_curl.cpp

Release/src/http/client/winhttppal/winhttppal.cpp

Release/src/http/client/http_client_curl.cpp

franksinankaya · 2019-07-27T17:49:33Z

I believe I addressed all concerns in the review. Can you please take a look one more time?

barcharcraz · 2019-08-06T18:16:47Z

Release/src/http/client/winhttppal/winhttppal.cpp

+{
+    WinHttpBase *base = static_cast<WinHttpBase *>(hInternet);
+
+


what happens if "offers" is a tls version higher than the used libcurl understands? wouldn't this line cause curl to use a sslversion < what was offered?

That's true. Offered is not what is actually used. Upper level gives a list of options that the transport protocol can use. What actually gets used depends on the curl version.

If there is a complete mismatch in terms of capabilities, CURL will use the CURL_SSLVERSION_DEFAULT as a minimum and CURL_SSLVERSION_MAX_DEFAULT as a maximum.

These definitions come from the current curl library in use.

Are you interested in enforcing the offered as required?

BillyONeal · 2019-08-27T02:23:55Z

The only reason why we stacked curl on top of winhttp is because winhttp interface is well documented. We just had to emulate winhttp API just enough with CURL to get to functional state.

The time to merge things to master is not when they're 'enough to be in a functional state'. Pushing the merge button means that the owners of cpprestsdk are taking responsibility for maintaining and shipping the code in the PR, and we are not interested in maintaining a Curl-To-WinHTTP abstraction layer.

We could have written a curl only client but we see bigger interest in having a light WinHTTP layer on other applications. We are planning to post a winhttppal project in github in the future.

If you want to make this as a separate project and then point cpprestsdk to it as an external dependency, that would be far more acceptable. But it's failing some of the test cases for the actual WinHTTP implementation (e.g. handling of the Host: header), meaning this isn't actually meeting WinHTTP's contract somewhere.

franksinankaya · 2019-08-27T02:30:18Z

We could have written a curl only client but we see bigger interest in having a light WinHTTP layer on other applications. We are planning to post a winhttppal project in github in the future.

If you want to make this as a separate project and then point cpprestsdk to it as an external dependency, that would be far more acceptable. But it's failing some of the test cases for the actual WinHTTP implementation (e.g. handling of the Host: header), meaning this isn't actually meeting WinHTTP's contract somewhere.

The project is now public.
https://github.com/microsoft/WinHttpPAL

I'll take your suggestion and create a dependency there.

I actually debugged the host header failure. It is failing because libcurl sends "accept encoding" by default and test is expecting to observe only the headers it sent. I can fix the failing test. That's the only issue here.

BillyONeal

Based on your comments I don't really think our interests are aligned here.

Release/CMakeLists.txt

Release/src/http/client/winhttppal/winhttppal.h

BillyONeal · 2019-08-27T02:16:23Z

Release/tests/functional/http/client/outside_tests.cpp

    TEST(server_hostname_mismatch) { test_failed_ssl_cert(U("https://wrong.host.badssl.com/")); }

-#if !defined(__cplusplus_winrt)
+#if !defined(__cplusplus_winrt) && !defined(CPPREST_FORCE_HTTP_CLIENT_CURL)


These tests failing suggests that you aren't handling the Host: header correctly in TLS.

Release/src/http/client/winhttppal/winhttppal.cpp

Release/src/http/client/http_client_curl.cpp

BillyONeal · 2019-08-27T02:57:20Z

Release/src/http/client/http_client_curl.cpp

+
+    void on_send_request_validate_cn()
+    {
+        // we do the validation inside curl


I don't think so because you're failing these tests.

If this is the case then you should just remove the function entirely rather than leaving a noop.

Release/src/http/client/http_client_curl.cpp

BillyONeal · 2019-08-27T22:21:01Z

If you want to make this as a separate project and then point cpprestsdk to it as an external dependency, that would be far more acceptable. But it's failing some of the test cases for the actual WinHTTP implementation (e.g. handling of the Host: header), meaning this isn't actually meeting WinHTTP's contract somewhere.

The project is now public.
https://github.com/microsoft/WinHttpPAL

I'll take your suggestion and create a dependency there.

OK, then can you structure it like this:

Change the build script to find that as a dependency (implies getting that dependency into vcpkg ?)
Delete the version of those bits you have from this PR and use the dependency instead.
Rather than copy/pasting the winhttp backend, use an ifdef, use a macro like _CPPREST_USE_WINHTTPPAL, and make those associated changes to the actual WinHTTP backend? That way bugfixes that concern the WinHTTP backend are also applied to the curl one. The azure pipelines configuration using the abstraction layer will serve as a backstop to avoid breaking your layer accidentally.
Rename references to "curl" here with "winhttpal"?

I think that will put us much closer to something we can consider merging.

/cc @barcharcraz

barcharcraz · 2019-08-27T22:34:51Z

If you want to make this as a separate project and then point cpprestsdk to it as an external dependency, that would be far more acceptable. But it's failing some of the test cases for the actual WinHTTP implementation (e.g. handling of the Host: header), meaning this isn't actually meeting WinHTTP's contract somewhere.

The project is now public.
https://github.com/microsoft/WinHttpPAL
I'll take your suggestion and create a dependency there.

OK, then can you structure it like this:

Change the build script to find that as a dependency (implies getting that dependency into vcpkg ?)

Delete the version of those bits you have from this PR and use the dependency instead.

Rather than copy/pasting the winhttp backend, use an ifdef, use a macro like _CPPREST_USE_WINHTTPPAL, and make those associated changes to the actual WinHTTP backend? That way bugfixes that concern the WinHTTP backend are also applied to the curl one. The azure pipelines configuration using the abstraction layer will serve as a backstop to avoid breaking your layer accidentally.

Rename references to "curl" here with "winhttpal"?

I think that will put us much closer to something we can consider merging.

/cc @barcharcraz

isn't _CPPREST_USE_WINHTTPPAL a reserved name?

also: I'd be OK with billy's suggested actions, or with a new "curl only" backend that didn't depend on anything to do with winhttp.

Also, the build script should make the winhttppal an opt in thing. And when opted in it should apply on all platforms (so if you ask for the pal you get it everywhere, not just on platforms without winhttp).

franksinankaya · 2019-08-27T22:47:35Z

isn't _CPPREST_USE_WINHTTPPAL a reserved name?

Why is winhttppal reserved? WinHttp is an existing client library in windows but winhttppal doesn't exist anywhere else except the github page I linked

also: I'd be OK with billy's suggested actions, or with a new "curl only" backend that didn't depend on anything to do with winhttp.

Yeah, I'll follow the actions. As I said, we are seeing the winhttppal being used in multiple projects that are trying to migrate to linux. It has a big benefit for a lot of folks.

Also, the build script should make the winhttppal an opt in thing. And when opted in it should apply on all platforms (so if you ask for the pal you get it everywhere, not just on platforms without winhttp).

Both winhttppal and curl works on windows as well as linux. Code was developed on windows using visual studio and c++ 11 primitives only. Later tested on linux with ubuntu 16.04 and ubuntu 18.04. Code is cross-platform compatible.

BillyONeal · 2019-08-27T23:02:54Z

isn't _CPPREST_USE_WINHTTPPAL a reserved name?

Yes, sorry, I had my 'STL maintainer' hat on :P. CPPREST_USE_WINHTTPPAL without _Ugly underscore please :)

(I don't actually care what the macro is as long as it's descriptive)

barcharcraz · 2019-08-28T00:06:29Z

Both winhttppal and curl works on windows as well as linux. Code was developed on windows using visual studio and c++ 11 primitives only. Later tested on linux with ubuntu 16.04 and ubuntu 18.04. Code is cross-platform compatible.

I just mean that I'd prefer we don't try and have the buildsystem guess if you want/need winhttppal.

…tions

- static analysis - hide copy constructors - Remove terminatestring function and size the buffers appropriately - No need for empty string initialization - Use references instead of copies where possible - replace atoi with stoi - No need to check null pointer on delete - Fix copy paste mistake - Support older curl versions - More read function unifications

Platform category

franksinankaya · 2019-09-09T17:33:28Z

@BillyONeal : can you review the changes? I think I didn't understand some of the preprocessor macro comments you were mentioning about.

BillyONeal

LGTM, thanks for your contribution.

@barcharcraz Can you take a look?

BillyONeal · 2019-09-27T23:27:53Z

Thanks for your contribution!

franksinankaya force-pushed the frkaya/curlhttp branch from 6388246 to 0b259d8 Compare July 10, 2019 22:12

BillyONeal requested changes Jul 12, 2019

View reviewed changes

franksinankaya commented Jul 18, 2019

View reviewed changes

Release/src/http/client/http_client_curl.cpp Outdated Show resolved Hide resolved

barcharcraz reviewed Aug 6, 2019

View reviewed changes

BillyONeal requested changes Aug 27, 2019

View reviewed changes

franksinankaya added 15 commits September 2, 2019 18:39

Support CURL based HTTP client

7efa590

Include buffer request for CURL based client

606565f

Rename to curl_xyz functions and remove if 0s

ea3a590

Restore ifdef

2814029

Remove conflicting definitions

ca838be

Prefer utility::char_t to TCHAR

2ceebcb

Bring writedata optimization by removing the intermediate buffering

bdc6efc

Use std::string where possible

e7858d7

Unify trace functions and reduce dynamic_cast calls

2b9e778

Add ubuntu 18.04 azure pipeline with no tests for the moment

17c50d1

Use ubuntu 16.04 but update the curl in it

fb85f5a

pipelines: needs sudo for install

50ac9d4

Remove reference to undefined type

55bc08b

Eliminate size conversations

ee4c0cc

Formatting changes and convert MultiByte functions to C++ native func…

06b5d13

…tions

franksinankaya added 15 commits September 2, 2019 18:39

Use const where possible, convert reinterpret cast to static cast

817dd16

Use nullize_newlines

44ca34a

Structure TRACE output for each request

e2ced5a

Use queue instead of locks

7055a34

Simplify reads, add breadcrumbs on failure

df774d5

Ignore http 100 continue

0b8609a

Remove buffering support

95bfd6b

Cleanup repeating patterns and add null checks

593954c

tests: CURL doesn't support hostname override

0345652

More function unification and prevent premature read completion

422655c

Fix typo

1cd5cd1

Support proxies

89ed468

Fix spacing

8a4d808

Move winhttppal to its own library

2124784

franksinankaya force-pushed the frkaya/curlhttp branch from 462b710 to 2124784 Compare September 2, 2019 20:09

franksinankaya added 7 commits September 2, 2019 23:36

Delta

78ef46b

Platform category

Share parse_headers_string

df34d8f

Implement auth scheme

4332fe5

Remove http_client_winhttppal.cpp

fce2480

sudo install

17a75fc

Undo warning suppress

8db16da

Fix capitalization

cdb2244

BillyONeal approved these changes Sep 27, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into frkaya/curlhttp

22b34f9

BillyONeal merged commit f4c863b into microsoft:master Sep 27, 2019

franksinankaya deleted the frkaya/curlhttp branch September 27, 2019 23:28

garethsb mentioned this pull request Dec 12, 2019

Fix http_client test case compile failure #1288

Merged

Support CURL based HTTP client #1182

Support CURL based HTTP client #1182

Uh oh!

Conversation

franksinankaya commented Jul 10, 2019

Uh oh!

reneme commented Jul 11, 2019

Uh oh!

franksinankaya commented Jul 11, 2019

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BillyONeal Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

franksinankaya commented Jul 27, 2019

Uh oh!

barcharcraz Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

franksinankaya Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Aug 27, 2019

Uh oh!

franksinankaya commented Aug 27, 2019

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BillyONeal Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BillyONeal Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BillyONeal commented Aug 27, 2019

Uh oh!

barcharcraz commented Aug 27, 2019

Uh oh!

franksinankaya commented Aug 27, 2019

Uh oh!

BillyONeal commented Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barcharcraz commented Aug 28, 2019

Uh oh!

franksinankaya commented Sep 9, 2019

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Sep 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

BillyONeal commented Aug 27, 2019 •

edited

Loading