diff --git a/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs b/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs index a6c8195f8..44b1fe8e6 100644 --- a/src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs +++ b/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 { /// /// 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 - /// health checks, as the name parameter in input is mandatory. - /// 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(() => 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()?.IsDevelopment() == true; + + return new HttpClientHandler + { + 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 new file mode 100644 index 000000000..89a19586f --- /dev/null +++ b/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 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 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.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 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(); + + if (environmentName != null) + { + Mock 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>() + .Get(ClientName); + + TestHttpMessageHandlerBuilder builder = new(ClientName, provider); + foreach (Action 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 AdditionalHandlers { get; } = new List(); + + public override IServiceProvider Services { get; } + + public override HttpMessageHandler Build() => PrimaryHandler; + } + } +}