Fix more http test build fails in certain configurations #1332
Conversation
| @@ -173,7 +173,7 @@ SUITE(client_construction) | |||
| VERIFY_ARE_EQUAL(baseclient2.base_uri(), m_uri); | |||
| } | |||
|
|
|||
| #if defined(CPPREST_FORCE_HTTP_LISTENER_ASIO) | |||
| #if !defined(_WIN32) && !defined(__cplusplus_winrt) || defined(CPPREST_FORCE_HTTP_CLIENT_ASIO) | |||
There was a problem hiding this comment.
Can this condition be factored out into a separate named setting that describes it? It seems like we should be already setting CPPREST_FORCE_HTTP_CLIENT_ASIO on non-Windows platforms?
There was a problem hiding this comment.
The CMake config does I think, but looking at how both this and the similar LISTENER preprocessor symbol are used throughout the code, the assumption in almost every case is that the !defined(_WIN32) check is the primary condition since WinHTTP/HttpSys can't be used, and FORCE is only applied on Windows.
There was a problem hiding this comment.
@BillyONeal Please would you let me know what you would like to be addressed before merging this?
Searching the project for CPPREST_FORCE_HTTP_CLIENT_ASIO and for CPPREST_FORCE_HTTP_LISTENER_ASIO show that in other places, these conditions are used as I've suggested here.
Thanks!
| client_config.set_ssl_context_callback( | ||
| [&](boost::asio::ssl::context& ctx) { ctx.add_certificate_authority(cert); }); | ||
|
|
||
| #else | ||
| client_config.set_validate_certificates(false); |
There was a problem hiding this comment.
Looking at this again I don't think we can do this -- it is better to fail to build than to product an implementation failing to meet basic HTTPS guarantees, which include validating the certificate. Unless there's something I'm missing that makes this OK?
There was a problem hiding this comment.
The ASIO-based client allows programmatic addition of certs. For the WinHTTP-based client, certs have to be added to the Windows cert store, which C++ REST SDK doesn't provide any means to do. It has to be done via alternative means. I guess if the necessary cert is installed on the test system, then the call to disable validation can just be removed.
There was a problem hiding this comment.
Thinking about it, the best thing to do right now is to keep the #if which will fix the build but remove the #else that disables the cert validation, which will mean the test fails in this weird build config.
Would that be acceptable, @BillyONeal?
Adding C++ REST SDK support for working with a temporary cert store for WinHTTP could be on somebody's to do list. Wrapping that entirely new functionality and the ASIO backend-specific mechanism via set_ssl_context_callback behind the same façade would then be next on the list.
…build config (Boost.ASIO-based http_listener with WinHTTP-based http_client)
|
Thanks for your contribution! |
No description provided.