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

[EXPORTER] Add OTLP HTTP SSL support #1793

Merged
merged 90 commits into from
Apr 5, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Nov 23, 2022

Fixes #389 Enable http(s) ssl verification for curl based http_client implementation
Fixes #1402 [Trace SDK] OTLP Trace Exporter secure connection configuration options
Fixes #1756 TLS settings for OtlpHttpExporter

Changes in Exporters environment variables

Added all the helpers required to parse experimental TLS environment variables:

  • GetOtlpDefaultTracesSslTlsMinVersion();
  • GetOtlpDefaultMetricsSslTlsMinVersion();
  • GetOtlpDefaultLogsSslTlsMinVersion();
  • GetOtlpDefaultTracesSslTlsMaxVersion();
  • GetOtlpDefaultMetricsSslTlsMaxVersion();
  • GetOtlpDefaultLogsSslTlsMaxVersion();

// For TLS 1.0, 1.1, 1.2

  • GetOtlpDefaultTracesSslTlsCipher();
  • GetOtlpDefaultMetricsSslTlsCipher();
  • GetOtlpDefaultLogsSslTlsCipher();

// For TLS 1.3

  • GetOtlpDefaultTracesSslTlsCipherSuite();
  • GetOtlpDefaultMetricsSslTlsCipherSuite();
  • GetOtlpDefaultLogsSslTlsCipherSuite();

Changes in Exporters options

  • In OtlpHttpExporterOptions, added the following members:

    • ssl_insecure_skip_verify
    • ssl_ca_cert_path
    • ssl_ca_cert_string
    • ssl_client_key_path
    • ssl_client_key_string
    • ssl_client_cert_path
    • ssl_client_cert_string
  • Added a feature flag ENABLE_OTLP_HTTP_SSL, because this is an SDK ABI change.

    • There is an existing spec for SSL and mTLS, only the implementation is missing
  • In OtlpHttpExporterOptions, added the following members:

    • ssl_min_tls
    • ssl_max_tls
    • ssl_cipher
    • ssl_cipher_suite
  • Added a sub feature flag ENABLE_OTLP_HTTP_SSL_TLS, because this is an SDK ABI change.

    • There are no spec for TLS version environment variables, and there will be no spec, because there is a global freeze on new configuration variables.
    • Using per language (OTEL_CPP_) environment variables.
  • Likewise for metrics in OtlpHttpMetricExporterOptions

  • Likewise for logs in OtlpHttpLogRecordExporterOptions

Changes in Http client

  • Fixed every call to CURL, to check for errors
  • Improved error reporting for CURL, to include detailed error messages
  • Implemented added CURL calls to support SSL and TLS

Functional tests

  • Implemented a test client to cover various connections configurations
  • Tested against no server
  • Tested against an opentelemetry-collector in http mode (without SSL)
  • Tested against an opentelemetry-collector in https mode (with SSL)
  • Integrated tests in CI

Changes in documentation

  • Added step by step instructions to generate certificate and keys, to use when testing with OLTP HTTP SSL.

For significant contributions please make sure you have completed the following items:

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

@marcalff marcalff requested a review from a team November 23, 2022 00:16
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #1793 (8a47cf8) into main (9bcfcb9) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1793   +/-   ##
=======================================
  Coverage   87.19%   87.19%           
=======================================
  Files         166      166           
  Lines        4784     4784           
=======================================
  Hits         4171     4171           
  Misses        613      613           

@owent
Copy link
Member

owent commented Nov 29, 2022

There are several compatibility problems when using options of libcurl.
CURLOPT_ISSUERCERT_BLOB is available from 7.71.0
CURLOPT_SSLKEY_BLOB is available from 7.71.0
CURLOPT_SSLCERT_BLOB is available from 7.71.0

And also, there are some SSL options for ALPN, Custom CA, password for certification files, TLS cipher/TLS 1.3 cipher and certification files just for proxy. Some of them also just work on a high version.

Can we adapt these options by LIBCURL_VERSION_NUM? So that we can still use HTTP Client on a old system with the curl in package manager?(For example: Cent OS 7/curl 7.29.0)

@lalitb
Copy link
Member

lalitb commented Nov 30, 2022

Just a thought, for CURL version 7.70.0 or lesser, if CA certificate for validation is provided as string, we should enforce the SSL handshake to fails, probably by overriding the default system path to read the certificates from:

(pseducode)

#if LIBCURL_VERSION_NUM >=0x071f00  and ! defined OTEL_HTTPS_INSECURE_ENABLE
if (input_ssl_ca_cert_string.size()) {
       curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, 1L);
       curl_easy_setopt(curl, CURLOPT_CAINFO, "<inaccessible_file>");
       curl_easy_setopy(curl, CURLOPT_CANPATH, "<inaccessible_path>");
}
#endif 

@marcalff
Copy link
Member Author

Can we adapt these options by LIBCURL_VERSION_NUM? So that we can still use HTTP Client on a old system with the curl in package manager?(For example: Cent OS 7/curl 7.29.0)

Thanks @owent

Implemented LIBCURL_VERSION_NUM checks.

tools/setup-protobuf.sh Outdated Show resolved Hide resolved
Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and thanks.

owent added a commit to owent/opentelemetry-cpp that referenced this pull request Mar 15, 2023
…ix the unit test in open-telemetry#1793 .

Signed-off-by: owent <admin@owent.net>
owent added a commit to owent/opentelemetry-cpp that referenced this pull request Mar 16, 2023
…ix the unit test in open-telemetry#1793 .

Signed-off-by: owent <admin@owent.net>
owent added a commit to owent/opentelemetry-cpp that referenced this pull request Mar 20, 2023
…ix the unit test in open-telemetry#1793 .

Signed-off-by: owent <admin@owent.net>
@sirzooro
Copy link

LGTM too.

api/CMakeLists.txt Outdated Show resolved Hide resolved
api/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are done well. Have few comments, but nothing blocker. Thanks for the PR :)

@marcalff
Copy link
Member Author

All comments to date are addressed.

Given the size of the patch, waiting for @ThomsonTan and/or @esigo to comment, and will only merge after an ok-to-merge flag.

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for the great work :)

@lalitb lalitb added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Apr 4, 2023
@marcalff
Copy link
Member Author

marcalff commented Apr 4, 2023

Thanks. Planning to merge after the SIG meeting

@ThomsonTan ThomsonTan merged commit 0e52dfd into open-telemetry:main Apr 5, 2023
@lalitb lalitb mentioned this pull request Apr 26, 2023
3 tasks
ays7 added a commit to ays7/opentelemetry-cpp that referenced this pull request May 18, 2023
* commit '7887d32da60f54984a597abccbb0c883f3a51649': (82 commits)
  [RELEASE] Release version 1.9.0 (open-telemetry#2091)
  Use sdk_start_ts for MetricData start_ts for instruments having cumulative aggregation temporality. (open-telemetry#2086)
  [SEMANTIC CONVENTIONS] Upgrade to version 1.20.0 (open-telemetry#2088)
  [EXPORTER] Add OTLP HTTP SSL support (open-telemetry#1793)
  Make Windows build environment parallel (open-telemetry#2080)
  make some hints (open-telemetry#2078)
  Make some targets parallel in CI pipeline (open-telemetry#2076)
  [Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-telemetry#2064)
  Upgraded semantic conventions to 1.19.0 (open-telemetry#2017)
  Bump actions/stale from 7 to 8 (open-telemetry#2070)
  Include directory path added for Zipkin exporter example (open-telemetry#2069)
  Ignore more warning of generated protobuf files than not included in `-Wall` and `-Wextra` (open-telemetry#2067)
  Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. (open-telemetry#2000)
  Remove unused 'alerting' section from prometheus.yml in examples (open-telemetry#2055)
  Clean warnings in ETW exporters (open-telemetry#2063)
  Fix default value of `OPENTELEMETRY_INSTALL_default`. (open-telemetry#2062)
  [EXPORTER] GRPC endpoint scheme should take precedence over OTEL_EXPORTER_OTLP_TRACES_INSECURE (open-telemetry#2060)
  Fix view names in Prometheus example (open-telemetry#2034)
  Fix some docs typo (open-telemetry#2057)
  Checking indices before dereference (open-telemetry#2040)
  ...

# Conflicts:
#	exporters/ostream/CMakeLists.txt
#	sdk/src/metrics/state/metric_collector.cc
#	sdk/src/metrics/state/temporal_metric_storage.cc
@marcalff marcalff deleted the fix_otlp_http_ssl_1402 branch July 4, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
6 participants