Repository navigation
XS✔ ◾ fix(healthchecks): scope TLS validation bypass to local endpoints - #902
Neil (neilr81) wants to merge 2 commits into
Conversation
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
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
microsoft-omex
Bot
changed the title
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and tests correctly enforce the documented secure behavior.
0 open findings
What changed in this PR
Restricts health-check TLS certificate bypasses to Development, preserving validation elsewhere.
Changes:
- Adds environment-aware handler configuration with secure fallback.
- Adds tests for Development, deployed environments, and missing environment registration.
- Updates XML documentation.
| File | Description |
|---|---|
src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs |
Applies the TLS bypass only in Development. |
tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.cs |
Verifies environment-specific certificate validation behavior. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// <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) => |
There was a problem hiding this comment.
I think that this validation reduction may have been deliberate to support localhost loopback explicitly - where HTTPS validation won't work due to IP-based routing, rather than domain based routing.
If so, this change will break all localhost loopback health checks.
There was a problem hiding this comment.
Also, if someone has control of the service endpoints on localhost, HTTPS validation is the least of the security concerns.
Keith Cully (K-Cully)
left a comment
Keith Cully (K-Cully)
left a comment
There was a problem hiding this comment.
Pretty sure this will break every service and that the detection is actually a false-positive.
Suggest just updating the comments to make it clear of the execution context and localhost only use, and possibly making code edits to restrict the validation to only be skipped for localhost requests)
…ment 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
Neil (neilr81)
changed the title
microsoft-omex
Bot
changed the title
Why
AddHealthCheckHttpClientunconditionally setServerCertificateCustomValidationCallbackto a callback that always returnstrue, disabling TLS server certificate validation for every named health check client in every environment, including Production - even though the XML doc claimed the relaxation only applied "in development".Simply gating that bypass on
IsDevelopment()turned out to be too restrictive. Health checks typically target the local node, addressed either by a loopback host or by the raw node IP, and a certificate issued for a DNS name cannot match that address by construction. So the relaxation needs to be scoped by what is being called, not by which environment the service happens to run in.What changed
src/Diagnostics.HealthChecks/HealthCheckHttpClientServiceCollectionExtensions.cs:AddHealthCheckHttpClientnow decides per request. Certificate errors are ignored only when the request targets a loopback host (localhost,127.0.0.1,[::1]) or a bare IP literal. Requests addressed to a DNS name, meaning any external or FQDN endpoint, go through full validation.AddValidatedHealthCheckHttpClientfor health checks that should always validate, including loopback and IP endpoints.IHostEnvironmentis not registered, the code treats the app as non-Development.Routing is done inside the validation callback via
Uri.IsLoopbackandUri.HostNameType, so a single client can safely be pointed at a mix of local and remote endpoints.Tests
tests/Diagnostics.HealthChecks.UnitTests/HealthCheckHttpClientServiceCollectionExtensionsTests.csruns the registeredHttpMessageHandlerBuilderactions against a test builder, then invokes the resulting callback with real request URIs andSslPolicyErrorsvalues:localhost,127.0.0.1,[::1],10.0.0.4,[fd00::1]) in Production -> errors ignored.AddValidatedHealthCheckHttpClientin Production, Staging, EDog, Int, and with no registeredIHostEnvironment-> errors rejected even for loopback and IP, clean certificates still accepted.AddValidatedHealthCheckHttpClientin Development -> errors ignored.All 104 tests in
Microsoft.Omex.Extensions.Diagnostics.HealthChecks.UnitTestspass.Notes for reviewers
The main judgement call is treating any IP literal as local, not just loopback and private ranges. That matches how Service Fabric health checks address the local node (node IP, not loopback), but it does mean a health check deliberately pointed at a public IP would skip validation. Narrowing this to loopback plus RFC1918 / ULA ranges is a straightforward follow-up if reviewers prefer the tighter rule - happy to make that change here.
This is still a behavior change for any consumer relying on the old blanket bypass for FQDN addressed endpoints. Those health checks will now fail until the endpoint presents a trusted certificate, which is the intended outcome, and
AddValidatedHealthCheckHttpClientgives teams an explicit opt in to the strict behavior everywhere.IHostEnvironmentcomes in transitively via the existingMicrosoft.Omex.Extensions.Abstractionsproject reference, so no new package reference was needed.