Skip to content

Conversation

sandy2008
Copy link
Contributor

@sandy2008 sandy2008 commented Oct 23, 2024

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)

@github-actions github-actions bot added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Oct 23, 2024
@sandy2008 sandy2008 changed the title feat(): Add mTLS Support for OTLP Exporter [otlp] Add mTLS Support for OTLP Exporter Oct 23, 2024
return new ExportRequestContent(exportRequest);
}

private static HttpClient ModifyHttpClient(OtlpExporterOptions options, HttpClient httpClient)
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

Attention: Patch coverage is 39.53488% with 26 lines in your changes missing coverage. Please review.

Project coverage is 86.11%. Comparing base (06ef557) to head (ef7db36).

Files with missing lines Patch % Lines
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 54.83% 14 Missing ⚠️
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 0.00% 12 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5918      +/-   ##
==========================================
- Coverage   86.20%   86.11%   -0.10%     
==========================================
  Files         259      259              
  Lines       11854    11899      +45     
==========================================
+ Hits        10219    10247      +28     
- Misses       1635     1652      +17     
Flag Coverage Δ
unittests-Project-Experimental 86.00% <39.53%> (-0.18%) ⬇️
unittests-Project-Stable 86.05% <39.53%> (-0.10%) ⬇️

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

Files with missing lines Coverage Δ
...mentation/ExportClient/Grpc/GrpcProtocolHelpers.cs 51.28% <ø> (ø)
...ol/Implementation/ExportClient/OtlpExportClient.cs 100.00% <ø> (ø)
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs 87.09% <0.00%> (-8.40%) ⬇️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 89.28% <54.83%> (-9.80%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sandy2008
Copy link
Contributor Author

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

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

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

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

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

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

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!

{
var trustedCertificate = X509Certificate2.CreateFromPemFile(this.CertificateFile);

handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) =>

Choose a reason for hiding this comment

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

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

@sandy2008
Copy link
Contributor Author

sandy2008 commented Nov 8, 2024

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

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

Copy link
Contributor

github-actions bot commented May 8, 2025

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 8, 2025
@sandy2008
Copy link
Contributor Author

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

This is still active!

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 14, 2025
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 21, 2025
@sandy2008
Copy link
Contributor Author

Hi! This is still active.

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 22, 2025
@rajkumar-rangaraj
Copy link
Member

@sandy2008 Thank you for working on mTLS support for OTLP. This PR has been open since October and is causing maintenance overhead. I will close this PR. When you have a complete working proposal, please create a new PR.

@sandy2008
Copy link
Contributor Author

@rajkumar-rangaraj Please take a look at the new PR: #6343

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add OTLP Exporter TLS/mTLS configuration options

5 participants