From 7933659b0d82b0f0bc8a5464b2c24a311595faad Mon Sep 17 00:00:00 2001 From: Steve Gordon Date: Fri, 10 Nov 2023 08:40:10 +0000 Subject: [PATCH] Prevent server certificate callback runtime exception (#2213) * Prevent server certificate callback runtime exception On .NET Full framework <4.7.2, HttpClientHandler.ServerCertificateCustomValidationCallback is not available. Due to dependency binding issues introduced with netstandard, the incorrect System.Net.Http package may be resolved and lead to exceptions at runtime. This fix prevents calling this API on net462 targets which includes any .NET Framework version < .NET 4.7.2. This introduces the net472 target so that we can resume using this API from that version, where it was included in the box. * Update documentation * Tweak docs --- docs/configuration.asciidoc | 12 +++++ docs/setup-auto-instrumentation.asciidoc | 4 +- docs/supported-technologies.asciidoc | 10 ++++- .../BackendComm/BackendCommUtils.cs | 44 ++++++++++++++----- src/Elastic.Apm/Elastic.Apm.csproj | 10 ++++- .../Serialization/DefaultContractResolver.cs | 4 +- .../Serialization/JsonTypeReflector.cs | 4 +- .../XUnit/DisabledOnWindowsFact.cs | 14 ++++++ .../ServerCertificateTests.cs | 5 ++- 9 files changed, 88 insertions(+), 19 deletions(-) diff --git a/docs/configuration.asciidoc b/docs/configuration.asciidoc index 676928f48..24f7c6b24 100644 --- a/docs/configuration.asciidoc +++ b/docs/configuration.asciidoc @@ -704,6 +704,12 @@ Verification can be disabled by changing this setting to false. | `true` | Boolean |============ +[NOTE] +==== +This configuration setting has no effect on .NET Framework versions 4.6.2-4.7.1. +We recommend upgrading to .NET Framework 4.7.2 or newer to use this configuration setting. +==== + [float] [[config-server-cert]] ==== `ServerCert` (added[1.9]) @@ -726,6 +732,12 @@ the trust store, such as a self-signed certificate. | `` | String |============ +[NOTE] +==== +This configuration setting has no effect on .NET Framework versions 4.6.2-4.7.1. +We recommend upgrading to .NET Framework 4.7.2 or newer to use this configuration setting. +==== + [float] [[config-flush-interval]] ==== `FlushInterval` (added[1.1]) diff --git a/docs/setup-auto-instrumentation.asciidoc b/docs/setup-auto-instrumentation.asciidoc index 738bebb17..5a3fcbc27 100644 --- a/docs/setup-auto-instrumentation.asciidoc +++ b/docs/setup-auto-instrumentation.asciidoc @@ -20,7 +20,7 @@ This approach works with the following |x64 -|.NET Framework 4.6.2+ +|.NET Framework 4.6.2+* .NET Core 2.1+ @@ -31,6 +31,8 @@ This approach works with the following .NET 5+ |=== +_* Due to binding issues introduced by Microsoft, we recommend at least .NET Framework 4.7.2 for best compatibility._ + NOTE: The Profiler based agent only supports 64-bit applications. 32-bit applications aren't supported. It instruments the following assemblies: diff --git a/docs/supported-technologies.asciidoc b/docs/supported-technologies.asciidoc index 77fabbf70..8576e9f63 100644 --- a/docs/supported-technologies.asciidoc +++ b/docs/supported-technologies.asciidoc @@ -28,7 +28,15 @@ the agent will not capture transactions. === .NET versions The agent works on every .NET flavor and version that supports .NET Standard 2.0. -This means .NET Core 2.0 or newer, and .NET Framework 4.6.2 or newer. +This means .NET Core 2.0 or newer, and .NET Framework 4.6.2* or newer. + +_* Due to binding issues introduced by Microsoft, we recommend at least .NET Framework 4.7.2 for best compatibility._ + +[IMPORTANT] +==== +While this library *should* work on .NET Core 2.0+, we limit our support to only those +versions currently supported by Microsoft - .NET 6.0 and newer. +==== [float] [[supported-web-frameworks]] diff --git a/src/Elastic.Apm/BackendComm/BackendCommUtils.cs b/src/Elastic.Apm/BackendComm/BackendCommUtils.cs index d174f7287..47da859a2 100644 --- a/src/Elastic.Apm/BackendComm/BackendCommUtils.cs +++ b/src/Elastic.Apm/BackendComm/BackendCommUtils.cs @@ -7,12 +7,12 @@ using System.Net; using System.Net.Http; using System.Net.Http.Headers; -using System.Net.Security; using System.Reflection; -#if NETSTANDARD2_0 || NET6_0_OR_GREATER +#if !NET462 using System.Security.Authentication; -#endif +using System.Net.Security; using System.Security.Cryptography.X509Certificates; +#endif using System.Text; using System.Text.RegularExpressions; using Elastic.Apm.Api; @@ -126,8 +126,21 @@ private static void ConfigServicePoint(Uri serverUrlBase, IApmLogger logger) => private static HttpClientHandler CreateHttpClientHandler(IConfiguration configuration, IApmLogger logger) { +#if NET462 + try + { + var systemNetHttpVersion = typeof(HttpClientHandler).Assembly.GetName().Version; + logger.Trace()?.Log("System.Net.Http assembly version if {Version}.", systemNetHttpVersion); + } + catch (Exception ex) + { + logger.Error()?.LogException(ex, "Could not determine the assembly version of System.Net.Http."); + } +#endif + +#if !NET462 Func serverCertificateCustomValidationCallback = null; - var useWindowsCredentials = configuration.UseWindowsCredentials; + if (!configuration.VerifyServerCert) { serverCertificateCustomValidationCallback = (_, _, _, policyError) => @@ -192,18 +205,29 @@ private static HttpClientHandler CreateHttpClientHandler(IConfiguration configur return false; }; } +#endif var httpClientHandler = new HttpClientHandler { - ServerCertificateCustomValidationCallback = serverCertificateCustomValidationCallback, - UseDefaultCredentials = useWindowsCredentials + UseDefaultCredentials = configuration.UseWindowsCredentials }; -#if NETFRAMEWORK - ServicePointManager.SecurityProtocol |= SecurityProtocolType.Tls12; - logger.Info()?.Log($"CreateHttpClientHandler - SslProtocols: {ServicePointManager.SecurityProtocol}"); -#else + + // Due to the potential for binding issues (e.g.https://github.com/dotnet/runtime/issues/29314) + // and runtime exceptions on .NET Framework versions <4.7.2, we don't attempt to set the certificate + // validation callback or SSL protocols on net462, which should also apply to .NET Framework <4.7.2 runtimes + // which resolve to that target. +#if !NET462 + httpClientHandler.ServerCertificateCustomValidationCallback = serverCertificateCustomValidationCallback; + logger.Info()?.Log("CreateHttpClientHandler - Setting ServerCertificateCustomValidationCallback"); + httpClientHandler.SslProtocols |= SslProtocols.Tls12; logger.Info()?.Log($"CreateHttpClientHandler - SslProtocols: {httpClientHandler.SslProtocols}"); +#else + // We don't set the ServerCertificateCustomValidationCallback on ServicePointManager here as it would + // apply to the whole AppDomain and that may not be desired. A consumer can set this themselves if they + // need custom validation behaviour. + ServicePointManager.SecurityProtocol |= SecurityProtocolType.Tls12; + logger.Info()?.Log($"CreateHttpClientHandler - SslProtocols: {ServicePointManager.SecurityProtocol}"); #endif return httpClientHandler; } diff --git a/src/Elastic.Apm/Elastic.Apm.csproj b/src/Elastic.Apm/Elastic.Apm.csproj index 3570a8165..d1f7093c5 100644 --- a/src/Elastic.Apm/Elastic.Apm.csproj +++ b/src/Elastic.Apm/Elastic.Apm.csproj @@ -1,6 +1,6 @@ - + - netstandard2.0;net462;net6.0 + netstandard2.0;net462;net472;net6.0 true @@ -67,6 +67,11 @@ + + + + + @@ -83,6 +88,7 @@ + diff --git a/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/DefaultContractResolver.cs b/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/DefaultContractResolver.cs index b8923bcaf..4e511779b 100644 --- a/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/DefaultContractResolver.cs +++ b/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/DefaultContractResolver.cs @@ -1,4 +1,4 @@ -#region License +#region License // Copyright (c) 2007 James Newton-King // @@ -1259,7 +1259,7 @@ protected virtual IValueProvider CreateMemberValueProvider(MemberInfo member) // warning - this method use to cause errors with Intellitrace. Retest in VS Ultimate after changes IValueProvider valueProvider; -#if !(PORTABLE40 || PORTABLE || DOTNET || NETSTANDARD2_0 || NET5_0_OR_GREATER) +#if !(PORTABLE40 || PORTABLE || DOTNET || NET472 || NETSTANDARD2_0 || NET5_0_OR_GREATER) if (DynamicCodeGeneration) { valueProvider = new DynamicValueProvider(member); diff --git a/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/JsonTypeReflector.cs b/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/JsonTypeReflector.cs index e37548d67..59c5d832c 100644 --- a/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/JsonTypeReflector.cs +++ b/src/Elastic.Apm/Libraries/Newtonsoft.Json/Serialization/JsonTypeReflector.cs @@ -1,4 +1,4 @@ -#region License +#region License // Copyright (c) 2007 James Newton-King // @@ -468,7 +468,7 @@ public static ReflectionDelegateFactory ReflectionDelegateFactory { get { -#if !(PORTABLE40 || PORTABLE || DOTNET || NETSTANDARD2_0 || NET5_0_OR_GREATER) +#if !(PORTABLE40 || PORTABLE || DOTNET || NET472 || NETSTANDARD2_0 || NET5_0_OR_GREATER) if (DynamicCodeGeneration) { return DynamicReflectionDelegateFactory.Instance; diff --git a/test/Elastic.Apm.Tests.Utilities/XUnit/DisabledOnWindowsFact.cs b/test/Elastic.Apm.Tests.Utilities/XUnit/DisabledOnWindowsFact.cs index 315edd040..78c89bea2 100644 --- a/test/Elastic.Apm.Tests.Utilities/XUnit/DisabledOnWindowsFact.cs +++ b/test/Elastic.Apm.Tests.Utilities/XUnit/DisabledOnWindowsFact.cs @@ -8,6 +8,7 @@ namespace Elastic.Apm.Tests.Utilities.XUnit; +#pragma warning disable IDE0021 // Use expression body for constructor public sealed class DisabledOnWindowsFact : FactAttribute { public DisabledOnWindowsFact() @@ -22,11 +23,22 @@ public sealed class DisabledOnFullFrameworkFact : FactAttribute public DisabledOnFullFrameworkFact() { #if NETFRAMEWORK + Skip = "This test is disabled on .NET Full Framework"; #endif } } +public sealed class DisabledOnNet462FrameworkFact : FactAttribute +{ + public DisabledOnNet462FrameworkFact() + { +#if NET462 + Skip = "This test is disabled on .NET Framework 4.6.2"; +#endif + } +} + /// /// May be applied to tests which depend on Linux Docker images which will not be /// available when running on Windows images from GitHub actions. @@ -58,3 +70,5 @@ public DisabledOnFullFrameworkTheory() #endif } } + +#pragma warning restore IDE0021 // Use expression body for constructor diff --git a/test/Elastic.Apm.Tests/ServerCertificateTests.cs b/test/Elastic.Apm.Tests/ServerCertificateTests.cs index 1f1d2529c..f1d20ee92 100644 --- a/test/Elastic.Apm.Tests/ServerCertificateTests.cs +++ b/test/Elastic.Apm.Tests/ServerCertificateTests.cs @@ -16,6 +16,7 @@ using Elastic.Apm.Logging; using Elastic.Apm.Tests.MockApmServer; using Elastic.Apm.Tests.Utilities; +using Elastic.Apm.Tests.Utilities.XUnit; using FluentAssertions; using Xunit; @@ -72,7 +73,9 @@ public void ServerCert_Should_Allow_Https_To_Apm_Server() transaction.Context.Labels.MergedDictionary.Should().ContainKey("self_signed_cert"); } - [Fact] + // We don't support certificate validation on net462 due to .NET Framework binding issues with + // the APIs used. + [DisabledOnNet462FrameworkFact] public void VerifyServerCert_Should_Allow_Https_To_Apm_Server() { var configuration = new MockConfiguration(