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
Choose a base branch
from

Conversation

rustamserg
Copy link

Add x509 certificate chain validation for HTTP client on Linux OS

@msftclas
Copy link

msftclas commented May 30, 2020

CLA assistant check
All CLA requirements met.

@BillyONeal
Copy link
Member

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

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 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

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

@rustamserg
Copy link
Author

@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

#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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants