The Wayback Machine - https://web.archive.org/web/20201201005537/https://github.com/microsoft/cpprestsdk/pull/1437
Skip to content
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

SSL support for Linux clients #1437

Open
wants to merge 3 commits into
base: master
from

Conversation

@rustamserg
Copy link

@rustamserg rustamserg commented May 30, 2020

Add x509 certificate chain validation for HTTP client on Linux OS

@msftclas
Copy link

@msftclas msftclas commented May 30, 2020

CLA assistant check
All CLA requirements met.

rustamserg added 2 commits May 30, 2020
This reverts commit b19ea13.
@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Jun 13, 2020

Hello @rustamserg ! Thanks for your contribution. Note though that this code is not called on the Linux platform; there we rely on asio's integration with OpenSSL that already does the right thing. Do you have a specific target use case for this?

@rustamserg
Copy link
Author

@rustamserg rustamserg commented Jun 16, 2020

Hello @BillyONeal we don't use cpprest directly and thus cannot choose asio http client. Our network code is generated from swagger spec using swagger codegen for cpprest https://github.com/fraglab/swagger-codegen/tree/master/modules/swagger-codegen/src/main/resources/cpprest

This implementation uses http client API not asio version.

@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Jun 16, 2020

The http client on all non-Windows systems is powered by asio. We only have this platform specific callback for systems where the native TLS provider is not OpenSSL, and therefore OpenSSL can't validate the certificate chain (because it doesn't have the root certificates). See the caller here:

// OpenSSL calls the verification callback once per certificate in the chain,

Notably we have tests that we indeed reject bad certificates already here:

TEST(server_selfsigned_cert) { test_failed_ssl_cert(U("https://self-signed.badssl.com/")); }
which are passing on Linux.
@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Jun 16, 2020

(Also notably, there's nothing calling this function you added right now)

@rustamserg
Copy link
Author

@rustamserg rustamserg commented Jun 16, 2020

@BillyONeal thank you for the explanation, now I'm really confused. I just performed a test with setting breakpoint in this function under GDB to see where it is called. The top of the callstack as follows:
#0 web::http::client::details::verify_cert_chain_platform_specific (verifyCtx=..., hostName=...) at /home/frag/lmbr/Gems/Game01BackendCommon/Code/Source/cpprest/src/http/client/x509_cert_utilities.cpp:49 #1 0x0000000009daf67a in web::http::client::details::asio_context::write_request()::{lambda(bool, asio::ssl::verify_context&)#1}::operator()(bool, asio::ssl::verify_context&) const ( this=0x7fff98000b28, preverified=true, verify_context=...) at /home/frag/lmbr/Gems/Game01BackendCommon/Code/Source/cpprest/src/http/client/http_client_asio.cpp:1084 #2 asio::ssl::detail::verify_callback<web::http::client::details::asio_context::write_request()::{lambda(bool, asio::ssl::verify_context&)#1}>::call(bool, asio::ssl::verify_context&) ( this=0x7fff98000b20, preverified=true, ctx=...) at /home/frag/3rdparty/asio/1.12.2/include/asio/ssl/detail/verify_callback.hpp:49

So as I can see the callback is triggered in my case from http_client_asio.cpp:1084. However as I mentioned before our setup is not pure because of we use combination of swagger codegen + cpprest based auto generated code + Lumberyard engine environment which provides with asio and openssl libraries. If this PR is not actual then please ignore it.

@rustamserg rustamserg closed this Jun 16, 2020
@BillyONeal
Copy link
Member

@BillyONeal BillyONeal commented Jun 16, 2020

#0 web::http::client::details::verify_cert_chain_platform_specific (verifyCtx=..., hostName=...) at /home/frag/lmbr/Gems/Game01BackendCommon/Code/Source/cpprest/src/http/client/x509_cert_utilities.cpp:49 #1 0x0000000009daf67a in web::http::client::details::asio_context::write_request()::{lambda(bool, asio::ssl::verify_context&)#1}::operator()(bool, asio::ssl::verify_context&) const ( this=0x7fff98000b28, preverified=true, verify_context=...) at /home/frag/lmbr/Gems/Game01BackendCommon/Code/Source/cpprest/src/http/client/http_client_asio.cpp:1084 #2

I'm not sure, your line numbers don't match up here :(

However as I mentioned before our setup is not pure because of we use combination of swagger codegen + cpprest based auto generated code + Lumberyard engine environment which provides with asio and openssl libraries.

I see, that probably explains it, I'd be willing to bet the OpenSSL provided does not have trusted root certificates or is looking for them in the wrong place, meaning the real effect here is the /etc/pki/tls/certs/ca-bundle.crt path you're searching. Perhaps lumberyard's SSL should be fixed to look in that place?

If this PR is not actual then please ignore it.

The PR might still be useful, I just want to be hyper paranoid on anything we merge related to TLS because it's security sensitive and make sure we understand everything going on here.

@BillyONeal BillyONeal reopened this Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.