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.

Sign up for GitHub

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

Jump to bottom

Otlp grpc tls certificates #4731

Closed
wants to merge 15 commits into from
Closed

Otlp grpc tls certificates #4731

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3e773a1
[OTLP] Implement client certificate options
dhhoang Jul 29, 2023
22e80db
[OTLP] Attempt to load X509 PEM for old .NET
dhhoang Jul 30, 2023
bcb71f3
[OTLP] Implement OTEL_EXPORTER_OTLP_CERTIFICATE option
dhhoang Jul 31, 2023
48b1081
[OTLP] Update Http/protobuf certificate test
dhhoang Jul 31, 2023
4249037
[OTLP] Remove certificate options for Http/Protobuf in old .NET
dhhoang Jul 31, 2023
a4bab30
[OTLP] throws PlatformNotSupportedException on unsupported targets, a…
dhhoang Aug 1, 2023
3250ab0
[OTLP] Fix using directive code style error
dhhoang Aug 2, 2023
0cdac9b
[OTLP] Pass certificate to GrpcChannelOptions.HttpHandler, add unit t…
dhhoang Aug 8, 2023
d8d2d70
[OTLP] remove static test Certificates
dhhoang Aug 8, 2023
1183adb
Merge branch 'main' into otlp-grpc-tls-certificates
dhhoang Aug 8, 2023
12014f0
[OTLP] add mTLS receiver for otel collector for integration test
dhhoang Aug 8, 2023
9a9cc91
[OTLP] Implement TLS certificates integration tests
dhhoang Aug 12, 2023
b8064a6
[OTLP] generate certificates for unit tests on Windows
dhhoang Aug 12, 2023
5f4346f
Merge branch 'main' into otlp-grpc-tls-certificates
dhhoang Aug 13, 2023
ef6e2b8
[OTLP] Fix 'Using directive is unnecessary' error when building test
dhhoang Aug 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...Telemetry.Exporter.OpenTelemetryProtocol/.publicApi/Stable/net462/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
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
OpenTelemetry.Logs.OtlpLogExporterHelperExtensions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions, OpenTelemetry.Logs.LogRecordExportProcessorOptions> configureExporterAndProcessor) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
Expand Down
6 changes: 6 additions & 0 deletions ...Telemetry.Exporter.OpenTelemetryProtocol/.publicApi/Stable/net6.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
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
OpenTelemetry.Logs.OtlpLogExporterHelperExtensions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions, OpenTelemetry.Logs.LogRecordExportProcessorOptions> configureExporterAndProcessor) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
Expand Down
6 changes: 6 additions & 0 deletions ...y.Exporter.OpenTelemetryProtocol/.publicApi/Stable/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
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
OpenTelemetry.Logs.OtlpLogExporterHelperExtensions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions, OpenTelemetry.Logs.LogRecordExportProcessorOptions> configureExporterAndProcessor) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
Expand Down
6 changes: 6 additions & 0 deletions ...y.Exporter.OpenTelemetryProtocol/.publicApi/Stable/netstandard2.1/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
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
OpenTelemetry.Logs.OtlpLogExporterHelperExtensions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
static OpenTelemetry.Logs.OtlpLogExporterHelperExtensions.AddOtlpExporter(this OpenTelemetry.Logs.OpenTelemetryLoggerOptions loggerOptions, System.Action<OpenTelemetry.Exporter.OtlpExporterOptions, OpenTelemetry.Logs.LogRecordExportProcessorOptions> configureExporterAndProcessor) -> OpenTelemetry.Logs.OpenTelemetryLoggerOptions
Expand Down
113 changes: 112 additions & 1 deletion src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

using System.Diagnostics;
using System.Reflection;
#if NET6_0_OR_GREATER
using System.Security.Cryptography.X509Certificates;
#endif
#if NETFRAMEWORK
using System.Net.Http;
#endif
Expand All @@ -39,6 +42,9 @@ public class OtlpExporterOptions
internal const string HeadersEnvVarName = "OTEL_EXPORTER_OTLP_HEADERS";
internal const string TimeoutEnvVarName = "OTEL_EXPORTER_OTLP_TIMEOUT";
internal const string ProtocolEnvVarName = "OTEL_EXPORTER_OTLP_PROTOCOL";
internal const string ClientKeyFileEnvVarName = "OTEL_EXPORTER_OTLP_CLIENT_KEY";
internal const string ClientCertificateFileEnvVarName = "OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE";
internal const string CertificateFileEnvVarName = "OTEL_EXPORTER_OTLP_CERTIFICATE";

internal static readonly KeyValuePair<string, string>[] StandardHeaders = new KeyValuePair<string, string>[]
{
Expand Down Expand Up @@ -84,6 +90,21 @@ internal OtlpExporterOptions(
this.TimeoutMilliseconds = timeout;
}

if (configuration.TryGetStringValue(ClientCertificateFileEnvVarName, out var clientCertificateFile))
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to add a TryGetFilePath method. Similar to the TryGetUriValue it could validate that the string is a file path for an existing file and log a message if it isn't.

opentelemetry-dotnet/src/Shared/Options/ConfigurationExtensions.cs

Lines 69 to 73 in 8f38400

if (!Uri.TryCreate(stringValue, UriKind.Absolute, out value))
{
LogInvalidEnvironmentVariable?.Invoke(key, stringValue!);
return false;
}

Copy link
Author

Choose a reason for hiding this comment

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

After trying this I think a TryGetFilePath() method doesn't make much sense here. The TryGet... methods simply check if the value is syntactically valid and not whether the resource actually exists (e.g. URI).
If the certificate file with specified path doesn't exist then TryGetFilePath() would return false and the option would be set to null and treated as if no option has been set by the developer.
I think from a developer PoV, if the file doesn't exist I'd prefer to be notified with a FileNotFoundException.

Copy link
Member

Choose a reason for hiding this comment

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

Syntactic validation is what I had in mind - using something like Path.GetInvalidPathChars or something. Though file paths may be tricky to validate syntactically in a meaningful/helpful way, so I'm fine with not doing anything more in this PR.

{
this.ClientCertificateFile = clientCertificateFile;
}

if (configuration.TryGetStringValue(ClientKeyFileEnvVarName, out var clientKeyFile))
{
this.ClientKeyFile = clientKeyFile;
}

if (configuration.TryGetStringValue(CertificateFileEnvVarName, out var certificateFile))
{
this.CertificateFile = certificateFile;
}

if (configuration.TryGetValue<OtlpExportProtocol>(
ProtocolEnvVarName,
OtlpExportProtocolParser.TryParse,
Expand All @@ -94,7 +115,7 @@ internal OtlpExporterOptions(

this.HttpClientFactory = this.DefaultHttpClientFactory = () =>
{
return new HttpClient
return new HttpClient(this.CreateDefaultHttpMessageHandler(), disposeHandler: true)
{
Timeout = TimeSpan.FromMilliseconds(this.TimeoutMilliseconds),
};
Expand Down Expand Up @@ -159,6 +180,48 @@ public Uri Endpoint
/// <remarks>Note: This only applies when exporting traces.</remarks>
public BatchExportProcessorOptions<Activity> BatchExportProcessorOptions { get; set; }

#if NET6_0_OR_GREATER
/// <summary>
/// Gets or sets the trusted certificate to use when verifying a server's TLS credentials.
/// </summary>
#else
/// <summary>
/// Gets or sets the trusted certificate to use when verifying a server's TLS credentials.
/// </summary>
/// <remarks>
/// This option does not affect <see cref="OtlpExportProtocol.HttpProtobuf"/> protocol.
alanwest marked this conversation as resolved.
Show resolved Hide resolved
/// </remarks>
#endif
public string CertificateFile { get; set; }

#if NET6_0_OR_GREATER
/// <summary>
/// Gets or sets the path to the private key to use in mTLS communication in PEM format.
/// </summary>
#else
/// <summary>
/// Gets or sets the path to the private key to use in mTLS communication in PEM format.
/// </summary>
/// <remarks>
/// This option does not affect <see cref="OtlpExportProtocol.HttpProtobuf"/> protocol.
/// </remarks>
#endif
public string ClientKeyFile { get; set; }

#if NET6_0_OR_GREATER
/// <summary>
/// Gets or sets the path to the certificate/chain trust for clients private key to use in mTLS communication in PEM format.
/// </summary>
#else
/// <summary>
/// Gets or sets the path to the certificate/chain trust for clients private key to use in mTLS communication in PEM format.
/// </summary>
/// <remarks>
/// This option does not affect <see cref="OtlpExportProtocol.HttpProtobuf"/> protocol.
/// </remarks>
#endif
public string ClientCertificateFile { get; set; }

/// <summary>
/// Gets or sets the factory function called to create the <see
/// cref="HttpClient"/> instance that will be used at runtime to
Expand Down Expand Up @@ -205,6 +268,54 @@ internal static void RegisterOtlpExporterOptionsFactory(IServiceCollection servi
sp.GetRequiredService<IOptionsMonitor<BatchExportActivityProcessorOptions>>().Get(name)));
}

internal HttpClientHandler CreateDefaultHttpMessageHandler()
{
var handler = new HttpClientHandler();

if (this.CertificateFile != null)
{
#if NET6_0_OR_GREATER
var trustedCA = LoadCertificateFromPemFile(this.CertificateFile, null);
alanwest marked this conversation as resolved.
Show resolved Hide resolved

handler.ServerCertificateCustomValidationCallback = (message, serverCert, chain, policyErrs) =>
{
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
chain.ChainPolicy.CustomTrustStore.Add(trustedCA);
var buildResult = chain.Build(serverCert);
return buildResult;
};
Comment on lines +280 to +286
Copy link
Member

Choose a reason for hiding this comment

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

I'm not an expert on these things, so still need to do some of my own research to understand if this is sufficient.

One thing that leaves me a bit unsure here is that policyErrs is ignored. My understanding is the "out-of-the-box" validations would be reflected in policyErrs (e.g., is the cert signed by a trusted CA, is the cert expired, etc). I suspect we want the custom validation here to be in addition to the out-of-the-box validations.

Copy link
Author

@dhhoang dhhoang Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You're correct that policyErrs would contain the error that the "default" validation method returns. The default validator would trust the certificates in the root trust store (aka certificates "installed" on the machine).
If user is manually trusting a certificate, it means the certificate might not be in the machine root store and we need to at least ignore that error.

The other validation criteria (e.g. expiration, revocation) is encoded in the X509Chain parameter. In this implementation I keep them as default settings (other than TrustMode of course). This means that other failure criteria would still work (e.g. the trusted cert still fails if it's expired).

#else
throw new PlatformNotSupportedException("Client certificate option is not supported for HTTP/Protobuf protocol.");
#endif
}

if (this.ClientCertificateFile != null)
{
#if NET6_0_OR_GREATER
handler.ClientCertificates.Add(LoadCertificateFromPemFile(this.ClientCertificateFile, this.ClientKeyFile));
#else
throw new PlatformNotSupportedException("Trusted certificate option is not supported for HTTP/Protobuf protocol.");
#endif
}

return handler;
}

#if NET6_0_OR_GREATER
private static X509Certificate2 LoadCertificateFromPemFile(string certificatePath, string privateKeyPath)
{
// API here is a bit tricky, apparently CreateFromPemFile requires private key,
// while CreateFromPem can simply load the public cert
using var pemCert = privateKeyPath is null
? X509Certificate2.CreateFromPem(File.ReadAllText(certificatePath))
: X509Certificate2.CreateFromPemFile(certificatePath, privateKeyPath);

// loading ephemeral pem files have problem on Windows
// https://github.com/dotnet/runtime/issues/23749
return new X509Certificate2(pemCert.Export(X509ContentType.Pkcs12));
}
#endif

private static string GetUserAgentString()
{
try
Expand Down
22 changes: 20 additions & 2 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptionsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,30 @@ public static Channel CreateChannel(this OtlpExporterOptions options)
}

#if NETSTANDARD2_1 || NET6_0_OR_GREATER
return GrpcChannel.ForAddress(options.Endpoint);
var channelOptions = new GrpcChannelOptions();

if (options.ClientCertificateFile != null || options.CertificateFile != null)
{
// Setting certificate in SslCredentials won't work: https://aka.ms/aspnet/grpc/certauth
channelOptions.HttpHandler = channelOptions.HttpHandler = options.CreateDefaultHttpMessageHandler();
channelOptions.DisposeHttpClient = true;
}

return GrpcChannel.ForAddress(options.Endpoint, channelOptions);
#else
var rootCertificate = options.CertificateFile is null ? null : File.ReadAllText(options.CertificateFile);
KeyCertificatePair clientCertificatePair = options.ClientCertificateFile switch
{
null => null,
string f => new KeyCertificatePair(File.ReadAllText(f), options.ClientKeyFile == null ? null : File.ReadAllText(options.ClientKeyFile)),
};

ChannelCredentials channelCredentials;
if (options.Endpoint.Scheme == Uri.UriSchemeHttps)
{
channelCredentials = new SslCredentials();
channelCredentials = new SslCredentials(
rootCertificates: rootCertificate,
keyCertificatePair: clientCertificatePair);
}
else
{
Expand Down
4 changes: 4 additions & 0 deletions test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/IntegrationTest/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# Self-signed cert generated by integration test
otel-collector.crt
otel-collector.key
otel-client.crt
otel-client.key
otel-untrusted-collector.crt
otel-untrusted-collector.key
Loading
Loading