[otlp] Add mTLS Support for OTLP Exporter #5918

sandy2008 · 2024-10-23T02:28:18Z

Fixes #2009
Based on PR

Changes

This pull request introduces support for mutual TLS (mTLS) in the OpenTelemetry Protocol (OTLP) Exporter, allowing secure gRPC connections over HTTPS and mTLS supports for HTTP Protocol.

Key Changes:

Certificate file. OTEL_EXPORTER_OTLP_CERTIFICATE
Client key file. OTEL_EXPORTER_OTLP_CLIENT_KEY
Client certificate file. OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

CodeBlanch · 2024-10-23T18:05:11Z

...etry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/OtlpHttpTraceExportClient.cs

@@ -28,6 +28,26 @@ protected override HttpContent CreateHttpContent(OtlpCollector.ExportTraceServic
        return new ExportRequestContent(exportRequest);
    }

+    private static HttpClient ModifyHttpClient(OtlpExporterOptions options, HttpClient httpClient)


We can't just throw away the HttpClient and make a new one. Users are able to configure SSL/TLS today using factory:

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

Line 134 in 5dff99f

public Func<HttpClient> HttpClientFactory

This change will break any user doing that or doing anything else to the HttpClient they are intending to use here.

I modified a little bit, does it look fine? @CodeBlanch

If the design looks ok, I will start to write tests :)

…9CertificateLoader

codecov · 2024-10-27T04:42:49Z

Codecov Report

Attention: Patch coverage is 73.07692% with 35 lines in your changes missing coverage. Please review.

Project coverage is 85.12%. Comparing base (84e6afb) to head (2006fbf).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	68.65%	21 Missing ⚠️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs	77.77%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5918      +/-   ##
==========================================
- Coverage   85.15%   85.12%   -0.03%     
==========================================
  Files         272      272              
  Lines       12420    12548     +128     
==========================================
+ Hits        10576    10682     +106     
- Misses       1844     1866      +22

Flag	Coverage Δ
unittests-Project-Experimental	`85.06% <73.07%> (-0.09%)`	⬇️
unittests-Project-Stable	`85.03% <73.07%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs	`85.77% <77.77%> (-2.76%)`	⬇️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	`87.50% <68.65%> (-11.59%)`	⬇️

... and 5 files with indirect coverage changes

…9CertificateLoader

sandy2008 · 2024-10-27T16:29:51Z

Just for your info, AddCertificatesToHttpClient is not needed, so will delete it in the next commit.
@alanwest @CodeBlanch @sokoide Does other changes look ok?

rajkumar-rangaraj · 2024-10-28T17:05:33Z

Just for your info, AddCertificatesToHttpClient is not needed, so will delete it in the next commit. @alanwest @CodeBlanch @sokoide Does other changes look ok?

@sandy2008 I recommend putting this on hold for a month. We are working on a custom serialization (#5730) and also we plan to handle GRPC without relying on Grpc.Net packages and removing dependencies on Google Protobuf and GRPC libraries. This will impact the part where you are adding support for mTLS.

sandy2008 · 2024-10-29T03:38:03Z

Just for your info, AddCertificatesToHttpClient is not needed, so will delete it in the next commit. @alanwest @CodeBlanch @sokoide Does other changes look ok?

@sandy2008 I recommend putting this on hold for a month. We are working on a custom serialization (#5730) and also we plan to handle GRPC without relying on Grpc.Net packages and removing dependencies on Google Protobuf and GRPC libraries. This will impact the part where you are adding support for mTLS.

Hi :) @rajkumar-rangaraj
Since there is also http part as well, shall I change this PR to support HTTP first, and add GRPC support afterwards?

rajkumar-rangaraj · 2024-10-30T05:21:14Z

Since there is also http part as well, shall I change this PR to support HTTP first, and add GRPC support afterwards?

Today, we had a discussion on this topic in the SIG meeting. We all feel it is better to hold off on this work as this space is expected to change drastically in the coming weeks. We thought it would be beneficial if you could join the next SIG meeting to discuss it further.

sandy2008 · 2024-10-30T05:36:34Z

Since there is also http part as well, shall I change this PR to support HTTP first, and add GRPC support afterwards?

Today, we had a discussion on this topic in the SIG meeting. We all feel it is better to hold off on this work as this space is expected to change drastically in the coming weeks. We thought it would be beneficial if you could join the next SIG meeting to discuss it further.

Thank you for the update. @rajkumar-rangaraj

Could you please let me know the time and participation details for the next SIG meeting? I think it’s important for @sokoide , as a co-author, to attend as well, since I might not be able to cover everything alone.

rajkumar-rangaraj · 2024-10-30T05:39:32Z

Could you please let me know the time and participation details for the next SIG meeting? I think it’s important for @sokoide , as a co-author, to attend as well, since I might not be able to cover everything alone.

Please check the details here - https://github.com/open-telemetry/opentelemetry-dotnet?tab=readme-ov-file#contributing

sandy2008 · 2024-11-01T16:52:59Z

Could you please let me know the time and participation details for the next SIG meeting? I think it’s important for @sokoide , as a co-author, to attend as well, since I might not be able to cover everything alone.

Please check the details here - https://github.com/open-telemetry/opentelemetry-dotnet?tab=readme-ov-file#contributing

Thank you for the details! We’ll join the SIG meeting scheduled for November 5 at 16:00 PT (November 6, 11:00 JST). Looking forward to it!

psmulovics · 2024-11-06T17:51:36Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+            {
+                var trustedCertificate = X509Certificate2.CreateFromPemFile(this.CertificateFile);
+
+                handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) =>


Do you want users of this API being able to add their custom validation callbacks next to this one?

sandy2008 · 2024-11-08T05:41:14Z

@rajkumar-rangaraj @alanwest

    public static void TryEnableIHttpClientFactoryIntegration(this OtlpExporterOptions options, IServiceProvider serviceProvider, string httpClientName)
    {
        if (serviceProvider != null
            && options.Protocol == OtlpExportProtocol.HttpProtobuf
            && options.HttpClientFactory == options.DefaultHttpClientFactory)
        {
            options.HttpClientFactory = () =>
            {
                Type? httpClientFactoryType = Type.GetType("System.Net.Http.IHttpClientFactory, Microsoft.Extensions.Http", throwOnError: false);
                if (httpClientFactoryType != null)
                {
                    object? httpClientFactory = serviceProvider.GetService(httpClientFactoryType);
                    if (httpClientFactory != null)
                    {
                        MethodInfo? createClientMethod = httpClientFactoryType.GetMethod(
                            "CreateClient",
                            BindingFlags.Public | BindingFlags.Instance,
                            binder: null,
                            new Type[] { typeof(string) },
                            modifiers: null);

                        if (createClientMethod != null)
                        {
                            HttpClient? client = (HttpClient?)createClientMethod.Invoke(httpClientFactory, new object[] { httpClientName });

                            if (client != null)
                            {
                                client.Timeout = TimeSpan.FromMilliseconds(options.TimeoutMilliseconds);

                                // Set up a new HttpClientHandler to configure certificates and callbacks
                                var handler = new HttpClientHandler();

    #if NET6_0_OR_GREATER
                                // Add server certificate validation
                                if (!string.IsNullOrEmpty(options.CertificateFile))
                                {
                                    var trustedCertificate = X509Certificate2.CreateFromPemFile(options.CertificateFile);
                                    handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) =>
                                    {
                                        if (cert != null && chain != null)
                                        {
                                            chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
                                            chain.ChainPolicy.CustomTrustStore.Add(trustedCertificate);
                                            return chain.Build(cert);
                                        }
                                        return false;
                                    };
                                }

                                // Add client certificate
                                if (!string.IsNullOrEmpty(options.ClientCertificateFile) && !string.IsNullOrEmpty(options.ClientKeyFile))
                                {
                                    var clientCertificate = X509Certificate2.CreateFromPemFile(options.ClientCertificateFile, options.ClientKeyFile);
                                    handler.ClientCertificates.Add(clientCertificate);
                                }
    #else
                                throw new PlatformNotSupportedException("mTLS support requires .NET 6.0 or later.");
    #endif

                                // Re-create HttpClient using the custom handler
                                return new HttpClient(handler) { Timeout = client.Timeout };
                            }
                        }
                    }
                }

                return options.();
            };
        }
    }

Something like this would work?

rajkumar-rangaraj · 2024-11-08T22:14:24Z

@sandy2008 Sorry, I don't have the bandwidth to review this now. As we discussed in the SIG, I will get to this part once the dependencies on Google.Protobuf and Grpc.Net are removed. Additionally, until we have a solution in OpenTelemetry, you could implement a custom solution for HTTP/protobuf.

sandy2008 · 2025-02-24T15:38:37Z

@rajkumar-rangaraj Hi! Does the correct logics look good?

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptionsExtensions.cs:39

Reading certificate files using File.ReadAllText without any error handling may lead to unhandled exceptions if the file is missing or inaccessible. Consider adding try/catch blocks or validating file existence before reading.

if (!string.IsNullOrEmpty(options.ClientCertificateFile) && !string.IsNullOrEmpty(options.ClientKeyFile))

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/IntegrationTest/IntegrationTests.cs:230

The mTLS branch is not exercised by any test cases since none of the InlineData provide a true value for 'useMtls'. Consider adding an InlineData test case that explicitly sets useMtls to true.

if (useMtls)

sandy2008 · 2025-02-25T02:52:02Z

I will fix the comments by Copilot first :) @rajkumar-rangaraj Thank you!

rajkumar-rangaraj · 2025-02-25T02:54:56Z

@rajkumar-rangaraj Hi! Does the correct logics look good?

Could you also update the documentation on how it can be used? Additionally, updating the PR description with testing steps for the OpenTelemetry Collector would be helpful.

rajkumar-rangaraj · 2025-02-25T02:56:29Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/.publicApi/Stable/PublicAPI.Unshipped.txt

@@ -0,0 +1,6 @@
+OpenTelemetry.Exporter.OtlpExporterOptions.CertificateFile.get -> string!


Shall we keep this internal and rely only on the environment variable? If there is a request, we can work on exposing these as public APIs.

rajkumar-rangaraj · 2025-02-25T03:09:39Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+            client.Timeout = TimeSpan.FromMilliseconds(this.TimeoutMilliseconds);
+            return client;
+#else
+            // For earlier .NET versions


This is incorrect; only the latest .NET versions use HttpClient. .NET Framework and older versions of .NET Core still rely on Grpc.Core.

rajkumar-rangaraj · 2025-02-25T03:12:41Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+        if (!string.IsNullOrEmpty(this.CertificateFile))
+        {
+            // Load the certificate from the file
+            var trustedCertificate = X509Certificate2.CreateFromPemFile(this.CertificateFile);


The implementation does not verify if CertificateFile, ClientCertificateFile, or ClientKeyFile exist before attempting to load them.

rajkumar-rangaraj · 2025-02-25T03:13:36Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

@@ -220,6 +255,41 @@ internal OtlpExporterOptions ApplyDefaults(OtlpExporterOptions defaultExporterOp
        return this;
    }

+#if NET6_0_OR_GREATER
+    internal HttpClient AddCertificatesToHttpClient(HttpClientHandler handler)


How you plan to handle errors within this method?

rajkumar-rangaraj · 2025-02-25T03:15:17Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+                {
+                    chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
+                    chain.ChainPolicy.CustomTrustStore.Add(trustedCertificate);
+                    return chain.Build(cert);


chain.Build(cert) might fail if the certificate chain is incomplete or missing necessary intermediate certificates. Consider checking chain.ChainStatus for errors before returning true. Also, logging failures can help diagnose why validation failed

rajkumar-rangaraj · 2025-02-25T03:17:36Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+            var trustedCertificate = X509Certificate2.CreateFromPemFile(this.CertificateFile);
+
+            // Set custom server certificate validation callback
+            handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) =>


Using a custom server certificate validation callback can introduce security risks if not handled properly. Can you list what measures have been taken to mitigate potential risks, such as unintended trust, proper error handling, revocation checks, and restricting trust scope?

rajkumar-rangaraj · 2025-02-25T03:20:11Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+        // Add client certificate if both files are provided
+        if (!string.IsNullOrEmpty(this.ClientCertificateFile) && !string.IsNullOrEmpty(this.ClientKeyFile))
+        {
+            var clientCertificate = X509Certificate2.CreateFromPemFile(this.ClientCertificateFile, this.ClientKeyFile);


Loading a client certificate from a PEM file with a private key should be done cautiously. How do you ensure or inform customers that the private key file should be stored securely and access-restricted?

rajkumar-rangaraj

This PR introduces potential security risks, particularly in certificate validation and private key handling, which need further clarification. There are concerns about trusting unverified certificates and missing security measures for private key storage. Before proceeding, clear guidance on mitigating these risks and best practices for secure implementation should be provided. Until these concerns are addressed, I am blocking this PR to ensure security is not compromised. Looking forward to further clarifications.

sandy2008 · 2025-02-25T04:02:00Z

var clientCertificate = X509Certificate2.CreateFromPemFile(this.ClientCertificateFile, this.ClientKeyFile);

Thank you for your reply! Will review on security :)

feat(): Add mTLS Support for OTLP Exporter

00c903c

github-actions bot added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Oct 23, 2024

sandy2008 changed the title ~~feat(): Add mTLS Support for OTLP Exporter~~ [otlp] Add mTLS Support for OTLP Exporter Oct 23, 2024

Merge branch 'main' into main

b10b3fa

CodeBlanch reviewed Oct 23, 2024

View reviewed changes

sandy2008 added 4 commits October 27, 2024 11:29

feat(): feat: Add certificate handling in HttpClientFactory using X50…

0d0aa98

…9CertificateLoader

feat(): feat: Add certificate handling in HttpClientFactory using X50…

e7c6f5b

…9CertificateLoader

feat(): feat: Add certificate handling in HttpClientFactory using X50…

3b43fd3

…9CertificateLoader

feat(): feat: Add certificate handling in HttpClientFactory using X50…

4b6d3cd

…9CertificateLoader

sandy2008 added 2 commits October 27, 2024 20:00

feat(): feat: Add certificate handling in HttpClientFactory using X50…

5864dbf

…9CertificateLoader

Merge branch 'main' into main

0e38ba3

Merge branch 'main' into main

3733dee

psmulovics reviewed Nov 6, 2024

View reviewed changes

Merge branch 'main' into main

f09a650

sandy2008 added 6 commits November 11, 2024 18:11

Merge branch 'main' into main

8630876

feat(): Add mTLS Support for OTLP Exporter

2e7e412

feat(): Add mTLS Support for OTLP Exporter

afc8df6

feat(): Add mTLS Support for OTLP Exporter

c5101b1

feat(): Add mTLS Support for OTLP Exporter

84a4d5b

feat(): Add mTLS Support for OTLP Exporter

716949c

sandy2008 added 5 commits February 24, 2025 23:42

fix(): support new grpc client

d71b483

fix(): support new grpc client

8694bb9

fix(): support new grpc client

9d6e67a

fix(): support new grpc client

7aa2ea3

fix(): otlp exporter

a09e608

sandy2008 force-pushed the main branch from 7caa374 to a09e608 Compare February 24, 2025 15:02

sandy2008 added 3 commits February 25, 2025 00:06

fix(): support new grpc client

e77d6e1

fix(): support new grpc client

797816c

fix(): support new grpc client

0691873

sandy2008 requested review from CodeBlanch and psmulovics February 24, 2025 15:38

fix(): support new grpc client

8471933

rajkumar-rangaraj requested review from Copilot and removed request for psmulovics February 25, 2025 02:47

Merge branch 'main' into main

a9c0e90

Copilot AI reviewed Feb 25, 2025

View reviewed changes

sandy2008 added 2 commits February 25, 2025 11:48

fix(): support new grpc client

5beccbf

Merge branch 'main' of https://github.com/sandy2008/opentelemetry-dotnet

8a9639f

rajkumar-rangaraj reviewed Feb 25, 2025

View reviewed changes

rajkumar-rangaraj requested changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[otlp] Add mTLS Support for OTLP Exporter #5918

[otlp] Add mTLS Support for OTLP Exporter #5918

sandy2008 commented Oct 23, 2024 •

edited

Loading

CodeBlanch Oct 23, 2024

sandy2008 Oct 27, 2024

sandy2008 Oct 27, 2024

codecov bot commented Oct 27, 2024 •

edited

Loading

sandy2008 commented Oct 27, 2024

rajkumar-rangaraj commented Oct 28, 2024

sandy2008 commented Oct 29, 2024

rajkumar-rangaraj commented Oct 30, 2024

sandy2008 commented Oct 30, 2024

rajkumar-rangaraj commented Oct 30, 2024

sandy2008 commented Nov 1, 2024

psmulovics Nov 6, 2024

sandy2008 commented Nov 8, 2024 •

edited

Loading

rajkumar-rangaraj commented Nov 8, 2024

sandy2008 commented Feb 24, 2025

sandy2008 commented Feb 25, 2025

rajkumar-rangaraj commented Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj Feb 25, 2025

rajkumar-rangaraj left a comment

sandy2008 commented Feb 25, 2025

		@@ -0,0 +1,6 @@
		OpenTelemetry.Exporter.OtlpExporterOptions.CertificateFile.get -> string!

[otlp] Add mTLS Support for OTLP Exporter #5918

Are you sure you want to change the base?

[otlp] Add mTLS Support for OTLP Exporter #5918

Conversation

sandy2008 commented Oct 23, 2024 • edited Loading

Changes

Key Changes:

Merge requirement checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 27, 2024 • edited Loading

Codecov Report

sandy2008 commented Oct 27, 2024

rajkumar-rangaraj commented Oct 28, 2024

sandy2008 commented Oct 29, 2024

rajkumar-rangaraj commented Oct 30, 2024

sandy2008 commented Oct 30, 2024

rajkumar-rangaraj commented Oct 30, 2024

sandy2008 commented Nov 1, 2024

Choose a reason for hiding this comment

sandy2008 commented Nov 8, 2024 • edited Loading

rajkumar-rangaraj commented Nov 8, 2024

sandy2008 commented Feb 24, 2025

Choose a reason for hiding this comment

sandy2008 commented Feb 25, 2025

rajkumar-rangaraj commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

sandy2008 commented Feb 25, 2025

sandy2008 commented Oct 23, 2024 •

edited

Loading

codecov bot commented Oct 27, 2024 •

edited

Loading

sandy2008 commented Nov 8, 2024 •

edited

Loading