From edf2d4387380c6b454089b5f3ad58fa400ee02dd Mon Sep 17 00:00:00 2001 From: Neil Richardson Date: Wed, 14 Oct 2026 14:45:38 +0100 Subject: [PATCH 1/2] fix(healthchecks): only bypass TLS validation in Development AddHealthCheckHttpClient set ServerCertificateCustomValidationCallback to always return true, disabling server certificate validation in every environment including Production, despite the XML doc claiming the bypass applied only in development. Resolve IHostEnvironment from the service provider and apply the bypass only when IsDevelopment() is true. When IHostEnvironment is not registered the secure default of validating certificates is preserved. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: db9ceb21-549a-40e3-aba5-9b587fa6d8ed --- ...ckHttpClientServiceCollectionExtensions.cs | 22 ++++- ...pClientServiceCollectionExtensionsTests.cs | 95 +++++++++++++++++++ 2 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs diff --git a/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs b/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs index a6c8195f8..451c00c84 100644 --- a/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs +++ b/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs @@ -3,6 +3,7 @@ using System.Net.Http; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; namespace Microsoft.Omex.Extensions.Diagnostics.HealthChecks; @@ -14,10 +15,12 @@ public static class HealthCheckHttpClientServiceCollectionExtensions { /// /// Defines a in the DI designed for Health Checks. - /// The main difference is that these clients in development will ignore SSL errors. - /// This is an acceptable compromise considering that the health checks are executed in the same machine - /// as the Service Fabric service itself, and that each client will be specifically used for the + /// The main difference is that these clients will ignore SSL errors when, and only when, the application + /// is running in the Development environment. + /// This is an acceptable compromise in development considering that the health checks are executed in the + /// same machine as the Service Fabric service itself, and that each client will be specifically used for the /// health checks, as the name parameter in input is mandatory. + /// In every other environment the default certificate validation is preserved. /// Every other uses of this extension methods apart from health checks should be avoided. /// /// The services. @@ -25,8 +28,17 @@ public static class HealthCheckHttpClientServiceCollectionExtensions /// The . public static IHttpClientBuilder AddHealthCheckHttpClient(this IServiceCollection services, string name) => services.AddHttpClient(name) - .ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler + .ConfigurePrimaryHttpMessageHandler(provider => { - ServerCertificateCustomValidationCallback = (_, _, _, _) => true + HttpClientHandler handler = new(); + + // Certificate validation is only bypassed in Development. If the environment cannot be + // resolved we keep the secure default of validating server certificates. + if (provider.GetService()?.IsDevelopment() == true) + { + handler.ServerCertificateCustomValidationCallback = (_, _, _, _) => true; + } + + return handler; }); } diff --git a/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs b/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs new file mode 100644 index 000000000..e855a2ff0 --- /dev/null +++ b/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs @@ -0,0 +1,95 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. + +using System; +using System.Collections.Generic; +using System.Net.Http; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Http; +using Microsoft.Extensions.Options; +using Microsoft.Omex.Extensions.Testing.Helpers; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; + +namespace Microsoft.Omex.Extensions.Diagnostics.HealthChecks.UnitTests +{ + [TestClass] + public class HealthCheckHttpClientServiceCollectionExtensionsTests + { + private const string ClientName = "TestHealthCheckClient"; + + [TestMethod] + public void AddHealthCheckHttpClient_InDevelopment_IgnoresCertificateErrors() + { + HttpClientHandler handler = BuildPrimaryHandler(Environments.Development); + + NullableAssert.IsNotNull( + handler.ServerCertificateCustomValidationCallback, + "Certificate validation should be bypassed in Development"); + } + + [TestMethod] + [DataRow("Production")] + [DataRow("Staging")] + [DataRow("EDog")] + [DataRow("Int")] + [DataRow(null)] + public void AddHealthCheckHttpClient_OutsideDevelopment_ValidatesCertificates(string? environmentName) + { + HttpClientHandler handler = BuildPrimaryHandler(environmentName); + + Assert.IsNull( + handler.ServerCertificateCustomValidationCallback, + FormattableString.Invariant($"Certificate validation should not be bypassed in '{environmentName ?? ""}'")); + } + + private static HttpClientHandler BuildPrimaryHandler(string? environmentName) + { + ServiceCollection services = new(); + + if (environmentName != null) + { + Mock environmentMock = new(); + environmentMock.SetupGet(e => e.EnvironmentName).Returns(environmentName); + services.AddSingleton(environmentMock.Object); + } + + services.AddHealthCheckHttpClient(ClientName); + + IServiceProvider provider = services.BuildServiceProvider(); + + HttpClientFactoryOptions options = provider + .GetRequiredService>() + .Get(ClientName); + + TestHttpMessageHandlerBuilder builder = new(ClientName, provider); + foreach (Action action in options.HttpMessageHandlerBuilderActions) + { + action(builder); + } + + Assert.IsInstanceOfType(builder.PrimaryHandler, typeof(HttpClientHandler)); + return (HttpClientHandler)builder.PrimaryHandler; + } + + private sealed class TestHttpMessageHandlerBuilder : HttpMessageHandlerBuilder + { + public TestHttpMessageHandlerBuilder(string name, IServiceProvider services) + { + Name = name; + Services = services; + } + + public override string? Name { get; set; } + + public override HttpMessageHandler PrimaryHandler { get; set; } = new HttpClientHandler(); + + public override IList AdditionalHandlers { get; } = new List(); + + public override IServiceProvider Services { get; } + + public override HttpMessageHandler Build() => PrimaryHandler; + } + } +} From dd67478ee3cd58dcab26c34b80fee7254b6c1f6e Mon Sep 17 00:00:00 2001 From: Neil Richardson Date: Thu, 15 Oct 2026 11:55:32 +0100 Subject: [PATCH 2/2] refactor(healthchecks): relax TLS validation by endpoint, not environment Health checks commonly target the local node by loopback host or by raw node IP, where a certificate issued for a DNS name cannot match the address being called. Gating the relaxation purely on the Development environment broke those checks everywhere else. AddHealthCheckHttpClient now ignores certificate errors only for loopback hosts and bare IP literals. Requests addressed to a DNS name, meaning external or FQDN endpoints, are fully validated. Development continues to ignore all certificate errors for self signed developer certificates. Add AddValidatedHealthCheckHttpClient for health checks that should always validate, including for loopback and IP addressed endpoints, with the same Development exception. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: db9ceb21-549a-40e3-aba5-9b587fa6d8ed --- ...ckHttpClientServiceCollectionExtensions.cs | 76 ++++++++++++---- ...pClientServiceCollectionExtensionsTests.cs | 87 ++++++++++++++++--- 2 files changed, 135 insertions(+), 28 deletions(-) diff --git a/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs b/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs index 451c00c84..44b1fe8e6 100644 --- a/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs +++ b/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs @@ -1,7 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. +using System; using System.Net.Http; +using System.Net.Security; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; @@ -15,30 +17,74 @@ public static class HealthCheckHttpClientServiceCollectionExtensions { /// /// Defines a in the DI designed for Health Checks. - /// The main difference is that these clients will ignore SSL errors when, and only when, the application - /// is running in the Development environment. - /// This is an acceptable compromise in development considering that the health checks are executed in the - /// same machine as the Service Fabric service itself, and that each client will be specifically used for the - /// health checks, as the name parameter in input is mandatory. - /// In every other environment the default certificate validation is preserved. - /// Every other uses of this extension methods apart from health checks should be avoided. + /// + /// Server certificate errors are ignored only for requests that target the local machine, meaning a + /// loopback host such as localhost, 127.0.0.1 or [::1], or a bare IP literal. + /// This is an acceptable compromise because health checks are executed on the same machine as the + /// Service Fabric service itself and are commonly addressed by node IP rather than by the subject + /// name present on the certificate, so validation cannot succeed by construction. + /// + /// + /// Requests that target a DNS name, meaning any external or FQDN addressed endpoint, are fully + /// validated. Use when the health check only ever + /// calls remote endpoints and the local relaxation is not wanted at all. + /// + /// + /// In the Development environment all certificate errors are ignored, including for DNS hosts, to + /// support self signed certificates on developer machines. + /// + /// + /// Every other use of this extension method apart from health checks should be avoided. + /// /// /// The services. /// The logical name of the to configure. /// The . public static IHttpClientBuilder AddHealthCheckHttpClient(this IServiceCollection services, string name) => + services.AddHealthCheckHttpClient(name, allowLocalCertificateErrors: true); + + /// + /// Defines a in the DI designed for Health Checks that always validates + /// server certificates, including for loopback and IP addressed endpoints. + /// + /// The only exception is the Development environment, where certificate errors are ignored to support + /// self signed certificates on developer machines. + /// + /// + /// Prefer this method when the health check targets an endpoint that presents a certificate valid for + /// the address being called. + /// + /// + /// The services. + /// The logical name of the to configure. + /// The . + public static IHttpClientBuilder AddValidatedHealthCheckHttpClient(this IServiceCollection services, string name) => + services.AddHealthCheckHttpClient(name, allowLocalCertificateErrors: false); + + private static IHttpClientBuilder AddHealthCheckHttpClient(this IServiceCollection services, string name, bool allowLocalCertificateErrors) => services.AddHttpClient(name) .ConfigurePrimaryHttpMessageHandler(provider => { - HttpClientHandler handler = new(); + // If the environment cannot be resolved we keep the secure default of treating the + // application as non Development. + bool isDevelopment = provider.GetService()?.IsDevelopment() == true; - // Certificate validation is only bypassed in Development. If the environment cannot be - // resolved we keep the secure default of validating server certificates. - if (provider.GetService()?.IsDevelopment() == true) + return new HttpClientHandler { - handler.ServerCertificateCustomValidationCallback = (_, _, _, _) => true; - } - - return handler; + ServerCertificateCustomValidationCallback = (request, _, _, errors) => + errors == SslPolicyErrors.None + || isDevelopment + || (allowLocalCertificateErrors && IsLocalEndpoint(request.RequestUri)) + }; }); + + /// + /// Determines whether a request targets the local machine, either through a loopback host or through + /// a bare IP literal, in which case a certificate issued for a DNS name cannot match the address used. + /// + private static bool IsLocalEndpoint(Uri? requestUri) => + requestUri != null + && (requestUri.IsLoopback + || requestUri.HostNameType == UriHostNameType.IPv4 + || requestUri.HostNameType == UriHostNameType.IPv6); } diff --git a/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs b/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs index e855a2ff0..89a19586f 100644 --- a/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs +++ b/tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.Net.Http; +using System.Net.Security; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Http; @@ -20,13 +21,42 @@ public class HealthCheckHttpClientServiceCollectionExtensionsTests private const string ClientName = "TestHealthCheckClient"; [TestMethod] - public void AddHealthCheckHttpClient_InDevelopment_IgnoresCertificateErrors() + [DataRow("https://localhost:8080/health")] + [DataRow("https://127.0.0.1:8080/health")] + [DataRow("https://[::1]:8080/health")] + [DataRow("https://10.0.0.4:8080/health")] + [DataRow("https://[fd00::1]:8080/health")] + public void AddHealthCheckHttpClient_ForLocalOrIpEndpoint_IgnoresCertificateErrors(string url) { - HttpClientHandler handler = BuildPrimaryHandler(Environments.Development); + Assert.IsTrue( + Validate(CertificateCallback(Environments.Production, validated: false), url, SslPolicyErrors.RemoteCertificateNameMismatch), + FormattableString.Invariant($"Certificate errors should be ignored for local endpoint '{url}'")); + } + + [TestMethod] + [DataRow("https://service.contoso.com/health")] + [DataRow("https://my-service.westus.cloudapp.azure.com/health")] + public void AddHealthCheckHttpClient_ForDnsEndpoint_ValidatesCertificate(string url) + { + Func callback = CertificateCallback(Environments.Production, validated: false); + + Assert.IsFalse( + Validate(callback, url, SslPolicyErrors.RemoteCertificateNameMismatch), + FormattableString.Invariant($"Certificate errors should be rejected for DNS endpoint '{url}'")); - NullableAssert.IsNotNull( - handler.ServerCertificateCustomValidationCallback, - "Certificate validation should be bypassed in Development"); + Assert.IsTrue( + Validate(callback, url, SslPolicyErrors.None), + FormattableString.Invariant($"A valid certificate should be accepted for DNS endpoint '{url}'")); + } + + [TestMethod] + [DataRow("https://service.contoso.com/health")] + [DataRow("https://localhost:8080/health")] + public void AddHealthCheckHttpClient_InDevelopment_IgnoresCertificateErrors(string url) + { + Assert.IsTrue( + Validate(CertificateCallback(Environments.Development, validated: false), url, SslPolicyErrors.RemoteCertificateChainErrors), + FormattableString.Invariant($"Certificate errors should be ignored in Development for '{url}'")); } [TestMethod] @@ -35,16 +65,36 @@ public void AddHealthCheckHttpClient_InDevelopment_IgnoresCertificateErrors() [DataRow("EDog")] [DataRow("Int")] [DataRow(null)] - public void AddHealthCheckHttpClient_OutsideDevelopment_ValidatesCertificates(string? environmentName) + public void AddValidatedHealthCheckHttpClient_OutsideDevelopment_ValidatesLocalEndpoints(string? environmentName) { - HttpClientHandler handler = BuildPrimaryHandler(environmentName); + Func callback = CertificateCallback(environmentName, validated: true); + + Assert.IsFalse( + Validate(callback, "https://localhost:8080/health", SslPolicyErrors.RemoteCertificateNameMismatch), + FormattableString.Invariant($"Local certificate errors should be rejected in '{environmentName ?? ""}'")); + + Assert.IsFalse( + Validate(callback, "https://10.0.0.4:8080/health", SslPolicyErrors.RemoteCertificateNameMismatch), + FormattableString.Invariant($"IP certificate errors should be rejected in '{environmentName ?? ""}'")); - Assert.IsNull( - handler.ServerCertificateCustomValidationCallback, - FormattableString.Invariant($"Certificate validation should not be bypassed in '{environmentName ?? ""}'")); + Assert.IsTrue( + Validate(callback, "https://localhost:8080/health", SslPolicyErrors.None), + "A valid certificate should always be accepted"); } - private static HttpClientHandler BuildPrimaryHandler(string? environmentName) + [TestMethod] + public void AddValidatedHealthCheckHttpClient_InDevelopment_IgnoresCertificateErrors() => + Assert.IsTrue( + Validate(CertificateCallback(Environments.Development, validated: true), "https://localhost:8080/health", SslPolicyErrors.RemoteCertificateChainErrors), + "Certificate errors should be ignored in Development"); + + private static bool Validate(Func callback, string url, SslPolicyErrors errors) + { + using HttpRequestMessage request = new(HttpMethod.Get, url); + return callback(request, errors); + } + + private static Func CertificateCallback(string? environmentName, bool validated) { ServiceCollection services = new(); @@ -55,7 +105,14 @@ private static HttpClientHandler BuildPrimaryHandler(string? environmentName) services.AddSingleton(environmentMock.Object); } - services.AddHealthCheckHttpClient(ClientName); + if (validated) + { + services.AddValidatedHealthCheckHttpClient(ClientName); + } + else + { + services.AddHealthCheckHttpClient(ClientName); + } IServiceProvider provider = services.BuildServiceProvider(); @@ -70,7 +127,11 @@ private static HttpClientHandler BuildPrimaryHandler(string? environmentName) } Assert.IsInstanceOfType(builder.PrimaryHandler, typeof(HttpClientHandler)); - return (HttpClientHandler)builder.PrimaryHandler; + HttpClientHandler handler = (HttpClientHandler)builder.PrimaryHandler; + + NullableAssert.IsNotNull(handler.ServerCertificateCustomValidationCallback, "A validation callback should be configured"); + + return (request, errors) => handler.ServerCertificateCustomValidationCallback(request, null, null, errors); } private sealed class TestHttpMessageHandlerBuilder : HttpMessageHandlerBuilder