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

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

Closed
lalitb opened this issue Nov 12, 2020 · 10 comments · Fixed by #1793
Closed

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

lalitb opened this issue Nov 12, 2020 · 10 comments · Fixed by #1793
Assignees
Labels
bug Something isn't working do-not-stale priority:p2 Issues that are not blocking

Comments

@lalitb
Copy link
Member

lalitb commented Nov 12, 2020

Currently, the curl based http client implementation (#385) doesn't support ssl( client and server) verification. This needs to be implemented.

@lalitb lalitb added bug Something isn't working good first issue Good for newcomers labels Nov 12, 2020
@maxgolov
Copy link
Contributor

@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
Copy link
Contributor

maxgolov commented Nov 17, 2020

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
Copy link
Member Author

lalitb commented Nov 18, 2020

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

@lalitb lalitb changed the title SSL/TLS support for curl based http_client implementation Enable http(s) ssl verification for curl based http_client implementation Nov 18, 2020
@maxgolov
Copy link
Contributor

maxgolov commented Dec 1, 2020

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

@lalitb lalitb added the priority:p2 Issues that are not blocking label Dec 16, 2020
@pavanshahm
Copy link

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.

#938

@lalitb
Copy link
Member Author

lalitb commented Aug 3, 2021

@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:
    virtual void OnConnecting(const SSLCertificate &) noexcept {}

@github-actions
Copy link

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 github-actions bot added the Stale label Nov 23, 2021
@lalitb lalitb removed the Stale label Nov 23, 2021
@github-actions
Copy link

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

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

@lalitb
Copy link
Member Author

lalitb commented Nov 9, 2022

#1756 depends on it.

@lalitb lalitb added the help wanted Good for taking. Extra help will be provided by maintainers label Nov 16, 2022
@marcalff marcalff self-assigned this Nov 21, 2022
@marcalff marcalff removed help wanted Good for taking. Extra help will be provided by maintainers good first issue Good for newcomers labels Mar 14, 2023
@marcalff marcalff removed the issue:blocking This issue is preventing other fixes label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working do-not-stale priority:p2 Issues that are not blocking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants