-
Notifications
You must be signed in to change notification settings - Fork 796
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
[otlp] Add mTLS Support for OTLP Exporter #5918
base: main
Are you sure you want to change the base?
Changes from 43 commits
00c903c
b10b3fa
0d0aa98
e7c6f5b
3b43fd3
4b6d3cd
5864dbf
0e38ba3
3733dee
f09a650
8630876
2e7e412
afc8df6
c5101b1
84a4d5b
716949c
2781534
31ef9aa
9df6f06
6e940d1
dc39de9
2006fbf
87737eb
4d56a9a
c4ec895
0ad1e13
7a378e6
6bddb6b
9b44067
a0bd2f9
f9fcd24
6675646
9b13d2e
cb5ccdf
d71b483
8694bb9
9d6e67a
7aa2ea3
a09e608
e77d6e1
797816c
0691873
8471933
a9c0e90
5beccbf
8a9639f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
OpenTelemetry.Exporter.OtlpExporterOptions.CertificateFile.get -> string! | ||
OpenTelemetry.Exporter.OtlpExporterOptions.CertificateFile.set -> void | ||
OpenTelemetry.Exporter.OtlpExporterOptions.ClientCertificateFile.get -> string! | ||
OpenTelemetry.Exporter.OtlpExporterOptions.ClientCertificateFile.set -> void | ||
OpenTelemetry.Exporter.OtlpExporterOptions.ClientKeyFile.get -> string! | ||
OpenTelemetry.Exporter.OtlpExporterOptions.ClientKeyFile.set -> void |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,9 @@ | |
using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation; | ||
using OpenTelemetry.Internal; | ||
using OpenTelemetry.Trace; | ||
#if NET6_0_OR_GREATER | ||
using System.Security.Cryptography.X509Certificates; | ||
#endif | ||
|
||
namespace OpenTelemetry.Exporter; | ||
|
||
|
@@ -28,6 +31,10 @@ public class OtlpExporterOptions : IOtlpExporterOptions | |
internal const string DefaultHttpEndpoint = "http://localhost:4318"; | ||
internal const OtlpExportProtocol DefaultOtlpExportProtocol = OtlpExportProtocol.Grpc; | ||
|
||
internal const string CertificateFileEnvVarName = "OTEL_EXPORTER_OTLP_CERTIFICATE"; | ||
internal const string ClientKeyFileEnvVarName = "OTEL_EXPORTER_OTLP_CLIENT_KEY"; | ||
internal const string ClientCertificateFileEnvVarName = "OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE"; | ||
|
||
internal static readonly KeyValuePair<string, string>[] StandardHeaders = new KeyValuePair<string, string>[] | ||
{ | ||
new("User-Agent", GetUserAgentString()), | ||
|
@@ -68,13 +75,26 @@ internal OtlpExporterOptions( | |
|
||
this.DefaultHttpClientFactory = () => | ||
{ | ||
#if NET6_0_OR_GREATER | ||
var handler = new HttpClientHandler(); | ||
HttpClient client = this.AddCertificatesToHttpClient(handler); | ||
client.Timeout = TimeSpan.FromMilliseconds(this.TimeoutMilliseconds); | ||
return client; | ||
#else | ||
// For earlier .NET versions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is incorrect; only the latest .NET versions use |
||
return new HttpClient | ||
{ | ||
Timeout = TimeSpan.FromMilliseconds(this.TimeoutMilliseconds), | ||
}; | ||
#endif | ||
}; | ||
|
||
this.BatchExportProcessorOptions = defaultBatchOptions!; | ||
|
||
// Load certificate-related environment variables | ||
this.CertificateFile = Environment.GetEnvironmentVariable(CertificateFileEnvVarName) ?? string.Empty; | ||
this.ClientKeyFile = Environment.GetEnvironmentVariable(ClientKeyFileEnvVarName) ?? string.Empty; | ||
this.ClientCertificateFile = Environment.GetEnvironmentVariable(ClientCertificateFileEnvVarName) ?? string.Empty; | ||
} | ||
|
||
/// <inheritdoc/> | ||
|
@@ -142,6 +162,21 @@ public Func<HttpClient> HttpClientFactory | |
} | ||
} | ||
|
||
/// <summary> | ||
/// Gets or sets the trusted certificate to use when verifying a server's TLS credentials. | ||
/// </summary> | ||
public string CertificateFile { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the path to the private key to use in mTLS communication in PEM format. | ||
/// </summary> | ||
public string ClientKeyFile { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the path to the certificate/chain trust for client's private key to use in mTLS communication in PEM format. | ||
/// </summary> | ||
public string ClientCertificateFile { get; set; } | ||
|
||
/// <summary> | ||
/// Gets a value indicating whether or not the signal-specific path should | ||
/// be appended to <see cref="Endpoint"/>. | ||
|
@@ -220,6 +255,41 @@ internal OtlpExporterOptions ApplyDefaults(OtlpExporterOptions defaultExporterOp | |
return this; | ||
} | ||
|
||
#if NET6_0_OR_GREATER | ||
internal HttpClient AddCertificatesToHttpClient(HttpClientHandler handler) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How you plan to handle errors within this method? |
||
{ | ||
// Configure server certificate validation if CertificateFile is provided | ||
if (!string.IsNullOrEmpty(this.CertificateFile)) | ||
{ | ||
// Load the certificate from the file | ||
var trustedCertificate = X509Certificate2.CreateFromPemFile(this.CertificateFile); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation does not verify if CertificateFile, ClientCertificateFile, or ClientKeyFile exist before attempting to load them. |
||
|
||
// Set custom server certificate validation callback | ||
handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
{ | ||
if (cert != null && chain != null) | ||
{ | ||
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; | ||
chain.ChainPolicy.CustomTrustStore.Add(trustedCertificate); | ||
return chain.Build(cert); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
return false; | ||
}; | ||
} | ||
|
||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
handler.ClientCertificates.Add(clientCertificate); | ||
} | ||
|
||
// Create and return an HttpClient with the modified handler | ||
return new HttpClient(handler); | ||
} | ||
#endif | ||
|
||
private static string GetUserAgentString() | ||
{ | ||
var assembly = typeof(OtlpExporterOptions).Assembly; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.