Enable http(s) ssl verification for curl based http_client implementation

[lalitb]

trafficstars

Currently, the curl based http client implementation (https://github.com/open-telemetry/opentelemetry-cpp/pull/385) doesn't support ssl( client and server) verification. This needs to be implemented.

[maxgolov]

@lalitb - I am a bit concerned here. From libcurl perspective - if you install one coming with the distro with SSL support, the only difference is in https:// and http:// URL. The rest is handled by CURL in very opaque way, and you'd only need bindings to SSL cert check (optional). The rest is rather simple and intuitive. Do you mean installing the necessary dependencies, e.g. libcurl+openssl or libcurl+gnutls or alike? brew on Mac already installs CURL with SSL, and the one available in Apple should have SSL too. Same with Android OS (platform), which by default compiles CURL with BoringSSL.

Are you anticipating some code changes for this issue or just the build infra setup changes / CI changes?

[maxgolov]

My point is we do not need to worry specifically about the TLS/SSL, unless there is a strong need to provide a callback for end-point certificate pinning / validation, and for the client authorization. Should this be more of a build infra item rather than a bug?

[lalitb]

@maxgolov - agree it should work seamlessly unless we want to support client authentication ( and hence provide Client certificate through our callback) which needs to be tested. The issue was actually raised to enable SSL (server ) certificate verification. I agree this is optional, but I am not sure if there are any security implication of bypassing this certificate check. Although traffic is still encrypted, there is no way to validate we are talking with correct server. I will anyway update the issue description.

[maxgolov]

@lalitb - got it. We may also need to supplement the test server we have in our repo with TLS / SSL support too. That'd be quite a bit of work though.

[pavanshahm]

I've started some work on something related to this, because my organization needed ssl support in order to use the http exporter. The changes include the bare minimum that we needed to get things working, please advise on if its the correct approach and what we can do so we can contribute to the project.

https://github.com/open-telemetry/opentelemetry-cpp/pull/938

[lalitb]

@pavanshahm - Thanks for this. It would be good to have SSL certificate check integrated. Couple of comments here:

1. Can we avoid adding code to build curl with SSL support as part of the otel-cpp build? I am not a bazel expert, but with cmake, we can build curl + SSL through vcpkg toolchain.
2. instead of adding a certificate path, can we leverage the EventHandler class to pass the certificate: https://github.com/open-telemetry/opentelemetry-cpp/blob/dc25ea686707dee2ea993f49664abf2b88e6f58a/ext/include/opentelemetry/ext/http/client/http_client.h#L205

[github-actions[bot]]

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

[github-actions[bot]]

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

[github-actions[bot]]

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

[lalitb]

#1756 depends on it.