[WIP] HTTP exporter support ssl credentials #938

pavanshahm · 2021-08-03T15:07:34Z

Fixes # (issue)

Changes

We needed to include ssl credentials if we wanted to use the http exporter so we added an additional option in http_exporter_options in order to allow it.

We also needed to update the curl.BUILD files in order to build cURL with the https protocol supported. This was done with a lot of trial and error and copied from the tensorflow build file, so I'm not sure which options are actually necessary and which ones are not.

Unit Tests are pending, but let me know if this is the right approach before I write those tests.

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

linux-foundation-easycla · 2021-08-03T15:07:37Z

❌ - Pavan The commit (45a438e) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

codecov · 2021-08-03T16:02:56Z

Codecov Report

Merging #938 (45a438e) into main (d6c187c) will decrease coverage by 0.06%.
The diff coverage is 55.56%.

@@            Coverage Diff             @@
##             main     #938      +/-   ##
==========================================
- Coverage   95.36%   95.30%   -0.05%     
==========================================
  Files         160      160              
  Lines        6779     6786       +7     
==========================================
+ Hits         6464     6467       +3     
- Misses        315      319       +4

Impacted Files	Coverage Δ
...nclude/opentelemetry/ext/http/client/http_client.h	`93.34% <ø> (ø)`
...ntelemetry/ext/http/client/curl/http_client_curl.h	`89.63% <33.34%> (-1.72%)`	⬇️
...lemetry/ext/http/client/curl/http_operation_curl.h	`77.34% <66.67%> (-0.74%)`	⬇️
...pi/include/opentelemetry/context/runtime_context.h	`97.50% <0.00%> (+0.04%)`	⬆️

owent · 2021-08-04T09:12:39Z

I have thought using civetweb to setup a temporary HTTP server for unit test. Because prometheus-cpp also depend it. We can also use it without import another library or binary.
It maybe need more discussion about how to test HTTP requests. @maxgolov @lalitb

lalitb · 2021-08-04T13:05:21Z

I have thought using civetweb to setup a temporary HTTP server for unit test. Because prometheus-cpp also depend it. We can also use it without import another library or binary.
It maybe need more discussion about how to test HTTP requests. @maxgolov @lalitb

@owent - This looks good - production-ready embeddable web-server, but just wondering whether we really need this for unit tests. What advantage do you think we will get from this over our internal http server specifically for unit-tests?

owent

I have thought using civetweb to setup a temporary HTTP server for unit test. Because prometheus-cpp also depend it. We can also use it without import another library or binary.
It maybe need more discussion about how to test HTTP requests. @maxgolov @lalitb

@owent - This looks good - production-ready embeddable web-server, but just wondering whether we really need this for unit tests. What advantage do you think we will get from this over our internal http server specifically for unit-tests?

We need HTTPS here.If implementation SSL support for http server need a lot codes, maybe it's easier to use a third party library?

owent · 2021-08-04T08:52:46Z

ext/include/opentelemetry/ext/http/client/http_client.h

@@ -125,6 +125,8 @@ class Request

  virtual void SetTimeoutMs(std::chrono::milliseconds timeout_ms) noexcept = 0;

+  virtual void IncludePermissionsFilePath(std::string filepath) noexcept = 0;


Maybe we should use nosdt::string_view instead of std::string here?

owent · 2021-08-04T08:54:54Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h

@@ -68,6 +68,9 @@ struct OtlpHttpExporterOptions

  // TODO: Enable/disable to verify SSL certificate
  std::chrono::milliseconds timeout = std::chrono::milliseconds(30000);
+
+  // filepath of the sll certifcate, this is empty if there is none. 
+  std::string sslCertPath;


Should we use lower case to keep the same style as before?

owent · 2021-08-04T08:59:47Z

ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h

-    // TODO: support ssl cert verification for https request
-    curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYPEER, 0);  // 1L
-    curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYHOST, 0);  // 2L
+    if (ssl_cert_path.empty()){


I think it's better to use a option use_ssl_credentials to control whether to ignore SSL verification which just like in OtlpGrpcExporterOptions.
And we should also privide options to set CURLOPT_SSLKEY and CURLOPT_SSLCERT .

lalitb · 2021-08-04T14:52:13Z

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

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.
instead of adding a certificate path, can we leverage the EventHandler class to pass the certificate:

opentelemetry-cpp/ext/include/opentelemetry/ext/http/client/http_client.h

Line 205 in dc25ea6

virtual void OnConnecting(const SSLCertificate &) noexcept {}

lalitb · 2021-08-04T14:58:40Z

We need HTTPS here.If implementation SSL support for http server need a lot codes, maybe it's easier to use a third party library?

OK. I thought we don't really need to test the ssl-handshake as part of the unit test here, as it should work correctly with curl as long as the correct certificate is passed. We can locally add unit-test tests to ensure that the SSL certificate is set correctly through our HTTP client library. But we can discuss if you feel it is really required?

owent · 2021-08-04T16:52:48Z

We need HTTPS here.If implementation SSL support for http server need a lot codes, maybe it's easier to use a third party library?

OK. I thought we don't really need to test the ssl-handshake as part of the unit test here, as it should work correctly with curl as long as the correct certificate is passed. We can locally add unit-test tests to ensure that the SSL certificate is set correctly through our HTTP client library. But we can discuss if you feel it is really required?

I don't think we need to test ssl-handshake either. But I think we should test whether the new options (sslCertPath) works. Is there any better suggestion to approach it?

lalitb · 2021-08-05T13:23:02Z

We need HTTPS here.If implementation SSL support for http server need a lot codes, maybe it's easier to use a third party library?

OK. I thought we don't really need to test the ssl-handshake as part of the unit test here, as it should work correctly with curl as long as the correct certificate is passed. We can locally add unit-test tests to ensure that the SSL certificate is set correctly through our HTTP client library. But we can discuss if you feel it is really required?

I don't think we need to test ssl-handshake either. But I think we should test whether the new options (sslCertPath) works. Is there any better suggestion to approach it?

ok, I was thinking to avoid testing the HTTPS request path as part of the unit test if it needs us to build and link with a third-party library. On second thought - I had a quick look at civetweb usage in prometheus-cpp. Perhaps it can be a good replacement for internal http_server for both unit tests ( for http/https requests) and pages. But needs more discussion with other community members.

lalitb · 2021-08-09T15:43:21Z

@pavanshahm - just wondering if you have plans to continue on this PR? As this has been at WIP state for some time now.

lalitb · 2021-09-14T06:01:09Z

@pavanshahm - just wondering if you have plans to continue on this PR? As this has been at WIP state for some time now.

@pavanshahm - Checking one more time if you have plans to work on this PR. While it would be really good to get it merged, we also don't want to keep it stale for long.

sirzooro · 2022-11-21T21:31:23Z

exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter.h

@@ -68,6 +68,9 @@ struct OtlpHttpExporterOptions

  // TODO: Enable/disable to verify SSL certificate
  std::chrono::milliseconds timeout = std::chrono::milliseconds(30000);
+
+  // filepath of the sll certifcate, this is empty if there is none. 
+  std::string sslCertPath;


mTLS support (requested by someone else here in another issue) will require two more fields for paths to private and public key. To avoid confusion please rename this field to make it more clear that this is path to trusted root CA certificates, e.g. sslCaCertPath.
In my issue I also asked about way to specify minimum TLS version (CURLOPT_SSLVERSION) and allowed ciphers (CURLOPT_SSL_CIPHER_LIST). Below I also mentioned another one to disable certificate validation. All of them would need corresponding fields here.
BTW, you also have typo in comment above - sll -> ssl

sirzooro · 2022-11-21T21:33:20Z

ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h

@@ -58,12 +58,20 @@ class Request : public http_client::Request
    timeout_ms_ = timeout_ms;
  }

+  void IncludePermissionsFilePath(std::string filepath) noexcept override 


Other methods here uses nostd::string_view for parameters instead of std::string, please make it consistent.

sirzooro · 2022-11-21T21:36:52Z

ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h

-    // TODO: support ssl cert verification for https request
-    curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYPEER, 0);  // 1L
-    curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYHOST, 0);  // 2L
+    if (ssl_cert_path.empty()){


Empty CA cert path should mean that user wants to use default system path, not disable certificate validation. Ability to disable validation is another use case. Please add another option to disable certificate validation.

sirzooro · 2022-11-21T21:38:03Z

ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h

+      curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYHOST, 0);  // 2L
+    } else {
+      curl_easy_setopt(curl_, CURLOPT_CAPATH, ssl_cert_path.c_str());
+      curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYPEER, 1);


This option is set by default to 1, so this line can be skipped.

support curl operations with ssl credentials

45a438e

pavanshahm requested a review from a team August 3, 2021 15:07

pavanshahm mentioned this pull request Aug 3, 2021

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

Closed

owent reviewed Aug 4, 2021

View reviewed changes

owent mentioned this pull request Sep 14, 2021

Make Otlp exporter configuration environment variables specs-compliant #974

Merged

3 tasks

github-actions bot added the Stale label Nov 14, 2021

github-actions bot closed this Nov 21, 2021

lalitb mentioned this pull request May 17, 2022

[Trace SDK] OTLP Trace Exporter secure connection configuration options #1402

Closed

sirzooro reviewed Nov 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] HTTP exporter support ssl credentials #938

[WIP] HTTP exporter support ssl credentials #938

pavanshahm commented Aug 3, 2021 •

edited

Loading

linux-foundation-easycla bot commented Aug 3, 2021

codecov bot commented Aug 3, 2021

owent commented Aug 4, 2021 •

edited

Loading

lalitb commented Aug 4, 2021

owent left a comment

owent Aug 4, 2021

owent Aug 4, 2021

owent Aug 4, 2021

lalitb commented Aug 4, 2021

lalitb commented Aug 4, 2021

owent commented Aug 4, 2021 •

edited

Loading

lalitb commented Aug 5, 2021 •

edited

Loading

lalitb commented Aug 9, 2021

lalitb commented Sep 14, 2021

sirzooro Nov 21, 2022

sirzooro Nov 21, 2022

sirzooro Nov 21, 2022

sirzooro Nov 21, 2022

		@@ -125,6 +125,8 @@ class Request

		virtual void SetTimeoutMs(std::chrono::milliseconds timeout_ms) noexcept = 0;

		virtual void IncludePermissionsFilePath(std::string filepath) noexcept = 0;

[WIP] HTTP exporter support ssl credentials #938

[WIP] HTTP exporter support ssl credentials #938

Conversation

pavanshahm commented Aug 3, 2021 • edited Loading

Changes

linux-foundation-easycla bot commented Aug 3, 2021

codecov bot commented Aug 3, 2021

Codecov Report

owent commented Aug 4, 2021 • edited Loading

lalitb commented Aug 4, 2021

owent left a comment

Choose a reason for hiding this comment

owent Aug 4, 2021

Choose a reason for hiding this comment

owent Aug 4, 2021

Choose a reason for hiding this comment

owent Aug 4, 2021

Choose a reason for hiding this comment

lalitb commented Aug 4, 2021

lalitb commented Aug 4, 2021

owent commented Aug 4, 2021 • edited Loading

lalitb commented Aug 5, 2021 • edited Loading

lalitb commented Aug 9, 2021

lalitb commented Sep 14, 2021

sirzooro Nov 21, 2022

Choose a reason for hiding this comment

sirzooro Nov 21, 2022

Choose a reason for hiding this comment

sirzooro Nov 21, 2022

Choose a reason for hiding this comment

sirzooro Nov 21, 2022

Choose a reason for hiding this comment

pavanshahm commented Aug 3, 2021 •

edited

Loading

owent commented Aug 4, 2021 •

edited

Loading

owent commented Aug 4, 2021 •

edited

Loading

lalitb commented Aug 5, 2021 •

edited

Loading