XS✔ ◾ fix(healthchecks): scope TLS validation bypass to local endpoints

src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs

@@ -1,8 +1,11 @@
 // 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;
 
 namespace Microsoft.Omex.Extensions.Diagnostics.HealthChecks;
 
@@ -14,19 +17,74 @@ public static class HealthCheckHttpClientServiceCollectionExtensions
 {
 	/// <summary>
 	/// Defines a <seealso cref="HttpClient"/> 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
-	/// health checks, as the name parameter in input is mandatory.
-	/// Every other uses of this extension methods apart from health checks should be avoided.
+	/// <para>
+	/// Server certificate errors are ignored only for requests that target the local machine, meaning a
+	/// loopback host such as <c>localhost</c>, <c>127.0.0.1</c> or <c>[::1]</c>, 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.
+	/// </para>
+	/// <para>
+	/// Requests that target a DNS name, meaning any external or FQDN addressed endpoint, are fully
+	/// validated. Use <see cref="AddValidatedHealthCheckHttpClient"/> when the health check only ever
+	/// calls remote endpoints and the local relaxation is not wanted at all.
+	/// </para>
+	/// <para>
+	/// In the Development environment all certificate errors are ignored, including for DNS hosts, to
+	/// support self signed certificates on developer machines.
+	/// </para>
+	/// <para>
+	/// Every other use of this extension method apart from health checks should be avoided.
+	/// </para>
 	/// </summary>
 	/// <param name="services">The services.</param>
 	/// <param name="name">The logical name of the <see cref="HttpClient"/> to configure.</param>
 	/// <returns>The <see cref="IHttpClientBuilder"/>.</returns>
 	public static IHttpClientBuilder AddHealthCheckHttpClient(this IServiceCollection services, string name) =>
+		services.AddHealthCheckHttpClient(name, allowLocalCertificateErrors: true);
+
+	/// <summary>
+	/// Defines a <seealso cref="HttpClient"/> in the DI designed for Health Checks that always validates
+	/// server certificates, including for loopback and IP addressed endpoints.
+	/// <para>
+	/// The only exception is the Development environment, where certificate errors are ignored to support
+	/// self signed certificates on developer machines.
+	/// </para>
+	/// <para>
+	/// Prefer this method when the health check targets an endpoint that presents a certificate valid for
+	/// the address being called.
+	/// </para>
+	/// </summary>
+	/// <param name="services">The services.</param>
+	/// <param name="name">The logical name of the <see cref="HttpClient"/> to configure.</param>
+	/// <returns>The <see cref="IHttpClientBuilder"/>.</returns>
+	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(() => new HttpClientHandler
+			.ConfigurePrimaryHttpMessageHandler(provider =>
 			{
-				ServerCertificateCustomValidationCallback = (_, _, _, _) => true
+				// If the environment cannot be resolved we keep the secure default of treating the
+				// application as non Development.
+				bool isDevelopment = provider.GetService<IHostEnvironment>()?.IsDevelopment() == true;
+
+				return new HttpClientHandler
+				{
+					ServerCertificateCustomValidationCallback = (request, _, _, errors) =>
+						errors == SslPolicyErrors.None
+						|| isDevelopment
+						|| (allowLocalCertificateErrors && IsLocalEndpoint(request.RequestUri))
+				};
 			});
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	private static bool IsLocalEndpoint(Uri? requestUri) =>
+		requestUri != null
+		&& (requestUri.IsLoopback
+			|| requestUri.HostNameType == UriHostNameType.IPv4
+			|| requestUri.HostNameType == UriHostNameType.IPv6);
 }

tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs

@@ -0,0 +1,156 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license.
+
+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;
+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]
+		[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)
+		{
+			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<HttpRequestMessage, SslPolicyErrors, bool> callback = CertificateCallback(Environments.Production, validated: false);
+
+			Assert.IsFalse(
+				Validate(callback, url, SslPolicyErrors.RemoteCertificateNameMismatch),
+				FormattableString.Invariant($"Certificate errors should be rejected for DNS endpoint '{url}'"));
+
+			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]
+		[DataRow("Production")]
+		[DataRow("Staging")]
+		[DataRow("EDog")]
+		[DataRow("Int")]
+		[DataRow(null)]
+		public void AddValidatedHealthCheckHttpClient_OutsideDevelopment_ValidatesLocalEndpoints(string? environmentName)
+		{
+			Func<HttpRequestMessage, SslPolicyErrors, bool> 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 ?? "<unregistered>"}'"));
+
+			Assert.IsFalse(
+				Validate(callback, "https://10.0.0.4:8080/health", SslPolicyErrors.RemoteCertificateNameMismatch),
+				FormattableString.Invariant($"IP certificate errors should be rejected in '{environmentName ?? "<unregistered>"}'"));
+
+			Assert.IsTrue(
+				Validate(callback, "https://localhost:8080/health", SslPolicyErrors.None),
+				"A valid certificate should always be accepted");
+		}
+
+		[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<HttpRequestMessage, SslPolicyErrors, bool> callback, string url, SslPolicyErrors errors)
+		{
+			using HttpRequestMessage request = new(HttpMethod.Get, url);
+			return callback(request, errors);
+		}
+
+		private static Func<HttpRequestMessage, SslPolicyErrors, bool> CertificateCallback(string? environmentName, bool validated)
+		{
+			ServiceCollection services = new();
+
+			if (environmentName != null)
+			{
+				Mock<IHostEnvironment> environmentMock = new();
+				environmentMock.SetupGet(e => e.EnvironmentName).Returns(environmentName);
+				services.AddSingleton(environmentMock.Object);
+			}
+
+			if (validated)
+			{
+				services.AddValidatedHealthCheckHttpClient(ClientName);
+			}
+			else
+			{
+				services.AddHealthCheckHttpClient(ClientName);
+			}
+
+			IServiceProvider provider = services.BuildServiceProvider();
+
+			HttpClientFactoryOptions options = provider
+				.GetRequiredService<IOptionsMonitor<HttpClientFactoryOptions>>()
+				.Get(ClientName);
+
+			TestHttpMessageHandlerBuilder builder = new(ClientName, provider);
+			foreach (Action<HttpMessageHandlerBuilder> action in options.HttpMessageHandlerBuilderActions)
+			{
+				action(builder);
+			}
+
+			Assert.IsInstanceOfType(builder.PrimaryHandler, typeof(HttpClientHandler));
+			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
+		{
+			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<DelegatingHandler> AdditionalHandlers { get; } = new List<DelegatingHandler>();
+
+			public override IServiceProvider Services { get; }
+
+			public override HttpMessageHandler Build() => PrimaryHandler;
+		}
+	}
+}