diff --git a/CHANGELOG.md b/CHANGELOG.md index 80fdb7d..10f1de6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,18 @@ its code behaves exactly as it did. to completion without interleaving anything else, which is the guarantee a read-then-write cannot give. Durability is whatever the Redis is configured for, and the package README says so rather than implying more. +- **A startup warning when a surface that can change a checker is reachable without + authenticating.** `AddHealthieController` still does not require authorization unless asked, and + the dashboard's `AllowMutations` still defaults to `true` -- both are deliberate and changing + either would break every application that maps them. What was missing was anyone being told: an + application that maps one and stops there lets whoever can reach it stop a checker or clear a + failing streak, which hides an incident rather than reporting one. Logged once, at `Warning`, on + application start. + + Asked of the endpoints rather than of the flags that built them, so an application that applied + authorization its own way -- `RequireAuthorization()`, an endpoint group, its own attribute -- is + not warned at. A warning that fires on correctly secured applications gets filtered out, and then + it is not there for the one that needs it. - **Schedules.** `PulseSchedule` says either "every this long" or "on this cron expression", and sits alongside `PulseInterval` rather than replacing it. The enum stopped at five minutes, which is short of what a certificate-expiry or disk-space check wants. Cron is standard Unix syntax and diff --git a/README.md b/README.md index dc43153..2af2d89 100644 --- a/README.md +++ b/README.md @@ -834,7 +834,9 @@ and both are deliberate as they stand: - **`Healthie.Api` requires no authorization unless the host asks for it**, and the dashboard's `HealthieUIOptions.AllowMutations` defaults to `true`. A host that maps either and does nothing else exposes read *and* write control of its checkers. Both are documented, and changing either - default is a behaviour break for every existing consumer. + default is a behaviour break for every existing consumer -- so instead of changing them, both now + log a `Warning` at startup naming exactly what is exposed. The warning reads the endpoints, not + the flags, so securing them any way at all silences it. - **Restore is not pinned by hash.** The fix is NuGet lock files, which means every package change needs the lock updated and CI running in locked mode. diff --git a/src/Healthie.Api/Diagnostics/Log.cs b/src/Healthie.Api/Diagnostics/Log.cs new file mode 100644 index 0000000..5bbfba2 --- /dev/null +++ b/src/Healthie.Api/Diagnostics/Log.cs @@ -0,0 +1,28 @@ +using Microsoft.Extensions.Logging; + +namespace Healthie.Api.Diagnostics; + +/// +/// The log messages writes. +/// +/// +/// Source-generated, as in the other packages, and with event ids in their own 4000 range so a +/// filter written against them keeps meaning what it meant. +/// +internal static partial class Log +{ + /// + /// Warning, and only once at startup. It describes a configuration an operator chose and can + /// change, not something going wrong at runtime, so repeating it per request would bury the + /// logs it is trying to be noticed in. + /// + [LoggerMessage( + EventId = 4001, + Level = LogLevel.Warning, + Message = "Healthie: {Count} endpoint(s) that can change a pulse checker are reachable " + + "without authenticating -- {Routes}. Anyone who can reach this application can stop a " + + "checker or clear a failing streak, which hides an incident rather than reporting one. " + + "Pass requireAuthorization: true to AddHealthieController, or apply your own " + + "authorization to these endpoints.")] + public static partial void MutatingEndpointsAreUnauthenticated(ILogger logger, int count, string routes); +} diff --git a/src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs b/src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs new file mode 100644 index 0000000..eb6c21f --- /dev/null +++ b/src/Healthie.Api/Diagnostics/UnauthenticatedSurfaceWarning.cs @@ -0,0 +1,92 @@ +using Healthie.Api.Routes; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Authorization; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Healthie.Api.Diagnostics; + +/// +/// Says so, once and loudly, when the endpoints that can change a checker are reachable without +/// authenticating. +/// +/// +/// +/// AddHealthieController does not require authorization unless it is asked to, which is a +/// deliberate default -- this controller ships into someone else's MVC pipeline, and demanding a +/// policy it knows nothing about would break every application that maps it. The consequence is +/// that an application which maps it and does nothing else lets anyone stop a checker or clear a +/// failing streak. That is not a wrong default so much as one worth being told about. +/// +/// +/// Asked of the endpoints rather than of the flag that built them: the host may have applied +/// authorization some other way -- a group, middleware, an +/// of its own -- and a warning that cried wolf at a correctly +/// secured application would be scrolled past within a week, which costs more than saying nothing. +/// +/// +internal sealed class UnauthenticatedSurfaceWarning( + EndpointDataSource endpoints, + IHostApplicationLifetime lifetime, + ILogger logger) : IHostedService +{ + /// The methods that change something, as opposed to reporting it. + private static readonly string[] MutatingMethods = ["POST", "PUT", "PATCH", "DELETE"]; + + public Task StartAsync(CancellationToken cancellationToken) + { + // After the application has started, because that is when the endpoints exist. Reading them + // during StartAsync races the routing system that builds them. + lifetime.ApplicationStarted.Register(Warn); + + return Task.CompletedTask; + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + private void Warn() + { + var unprotected = endpoints.Endpoints + .OfType() + .Where(IsHealthieRoute) + .Where(Mutates) + .Where(endpoint => !IsProtected(endpoint)) + .Select(endpoint => endpoint.RoutePattern.RawText) + .Where(route => route is not null) + .Distinct(StringComparer.OrdinalIgnoreCase) + .Order(StringComparer.OrdinalIgnoreCase) + .ToArray(); + + if (unprotected.Length == 0) + { + return; + } + + Log.MutatingEndpointsAreUnauthenticated(logger, unprotected.Length, string.Join(", ", unprotected!)); + } + + /// + /// Whether anything on this endpoint requires an authenticated caller. + /// + /// + /// Two shapes, because authorization arrives two ways and only one of them is metadata. + /// RequireAuthorization() and [Authorize] put an on + /// the endpoint. AddHealthieController(requireAuthorization: true) adds an + /// through an MVC convention, and that is an + /// IFilterMetadata rather than an -- so looking only for the + /// latter warned about the one configuration that had asked for authorization by name. + /// + private static bool IsProtected(Endpoint endpoint) => + endpoint.Metadata.GetMetadata() is not null + || endpoint.Metadata.GetMetadata() is not null; + + private static bool IsHealthieRoute(RouteEndpoint endpoint) => + endpoint.RoutePattern.RawText?.StartsWith(RoutesConstants.HealthieApiRoute, StringComparison.OrdinalIgnoreCase) + ?? false; + + private static bool Mutates(RouteEndpoint endpoint) => + endpoint.Metadata.GetMetadata() is { } methods + && methods.HttpMethods.Any(method => MutatingMethods.Contains(method, StringComparer.OrdinalIgnoreCase)); +} diff --git a/src/Healthie.Api/StartupExtensions.cs b/src/Healthie.Api/StartupExtensions.cs index 0172e27..880a9fb 100644 --- a/src/Healthie.Api/StartupExtensions.cs +++ b/src/Healthie.Api/StartupExtensions.cs @@ -1,6 +1,9 @@ using Healthie.Api.Controllers; using Healthie.Api.Conventions; +using Healthie.Api.Diagnostics; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Hosting; namespace Healthie.Api; @@ -37,6 +40,11 @@ public static IMvcBuilder AddHealthieController( // Ensure HealthCheckersController from Healthie.Api assembly is discovered. mvcBuilder.AddApplicationPart(typeof(HealthCheckersController).Assembly); + // Says so at startup if the endpoints that can change a checker end up reachable without + // authenticating. TryAdd because calling this twice should not warn twice. + services.TryAddEnumerable( + ServiceDescriptor.Singleton()); + return mvcBuilder; } } diff --git a/src/Healthie.Dashboard/Diagnostics/Log.cs b/src/Healthie.Dashboard/Diagnostics/Log.cs new file mode 100644 index 0000000..82f8be5 --- /dev/null +++ b/src/Healthie.Dashboard/Diagnostics/Log.cs @@ -0,0 +1,26 @@ +using Microsoft.Extensions.Logging; + +namespace Healthie.Dashboard.Diagnostics; + +/// +/// The log messages the dashboard writes. +/// +/// +/// Source-generated, as in the other packages, and with event ids in their own 5000 range so a +/// filter written against them keeps meaning what it meant. +/// +internal static partial class Log +{ + /// + /// Warning, and only once at startup. It describes a configuration an operator chose and can + /// change, not something going wrong at runtime. + /// + [LoggerMessage( + EventId = 5001, + Level = LogLevel.Warning, + Message = "Healthie: the dashboard at {Path} is reachable without authenticating and its " + + "controls are on, so anyone who can reach this application can pause a checker or reset " + + "a failing streak. Chain RequireAuthorization() onto MapHealthieUI(), or set " + + "HealthieUIOptions.AllowMutations to false to serve it read-only.")] + public static partial void DashboardIsUnauthenticatedAndWritable(ILogger logger, string path); +} diff --git a/src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs b/src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs new file mode 100644 index 0000000..786c697 --- /dev/null +++ b/src/Healthie.Dashboard/Diagnostics/UnauthenticatedDashboardWarning.cs @@ -0,0 +1,68 @@ +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Healthie.Dashboard.Diagnostics; + +/// +/// Says so, once and loudly, when the dashboard is reachable without authenticating and its +/// controls are on. +/// +/// +/// +/// defaults to true because the dashboard +/// exists to manage checkers and a read-only default would make every first run look broken. It is +/// not authorization and does not pretend to be: it decides which controls are rendered, for +/// everyone, and RequireAuthorization on the mapped endpoint is the other half. An +/// application that maps the dashboard and stops there has given anyone who can reach it the +/// ability to pause a checker or reset a failing streak. +/// +/// +/// Asked of the endpoint rather than assumed from the option, so an application that secured it -- +/// by chaining RequireAuthorization, by an endpoint group, by its own attribute -- is not +/// warned at. A warning that fires on correctly secured applications gets filtered out, and then it +/// is not there for the one that needs it. +/// +/// +internal sealed class UnauthenticatedDashboardWarning( + EndpointDataSource endpoints, + HealthieUIOptions options, + IHostApplicationLifetime lifetime, + ILogger logger) : IHostedService +{ + public Task StartAsync(CancellationToken cancellationToken) + { + // Nothing to say when the controls are not rendered: reaching the board then shows health + // and changes nothing. + if (!options.AllowMutations) + { + return Task.CompletedTask; + } + + // After the application has started, because that is when the endpoints exist. + lifetime.ApplicationStarted.Register(Warn); + + return Task.CompletedTask; + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + + private void Warn() + { + var dashboard = endpoints.Endpoints + .OfType() + .FirstOrDefault(endpoint => string.Equals( + "/" + endpoint.RoutePattern.RawText?.TrimStart('/'), + StartupExtensions.DashboardPath, + StringComparison.OrdinalIgnoreCase)); + + // Not mapped at all, or already behind an authorization policy. + if (dashboard is null || dashboard.Metadata.GetMetadata() is not null) + { + return; + } + + Log.DashboardIsUnauthenticatedAndWritable(logger, StartupExtensions.DashboardPath); + } +} diff --git a/src/Healthie.Dashboard/StartupExtensions.cs b/src/Healthie.Dashboard/StartupExtensions.cs index e3d7b25..a4431de 100644 --- a/src/Healthie.Dashboard/StartupExtensions.cs +++ b/src/Healthie.Dashboard/StartupExtensions.cs @@ -1,8 +1,11 @@ +using Healthie.Dashboard.Diagnostics; using Healthie.Dashboard.Services; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Hosting; using System.Net; namespace Healthie.Dashboard; @@ -42,6 +45,11 @@ public static IServiceCollection AddHealthieUI( services.AddScoped(); services.AddScoped(); + // Says so at startup if the board ends up reachable without authenticating while its + // controls are on. TryAdd because calling this twice should not warn twice. + services.TryAddEnumerable( + ServiceDescriptor.Singleton()); + return services; } diff --git a/tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs b/tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs new file mode 100644 index 0000000..111865f --- /dev/null +++ b/tests/Healthie.Tests.Unit/UnauthenticatedSurfaceWarningTests.cs @@ -0,0 +1,191 @@ +using Healthie.Abstractions.Scheduling; +using Healthie.Api; +using Healthie.Dashboard; +using Healthie.DependencyInjection; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; + +namespace Healthie.Tests.Unit; + +/// +/// The startup warning for surfaces that can change a checker without anyone authenticating. +/// +/// +/// Driven through a real host so the endpoints exist and carry the metadata the warning reads. +/// Asserting on the option instead would only restate the code: the point is that the check follows +/// what was actually applied, including authorization the host added its own way. +/// +public class UnauthenticatedSurfaceWarningTests +{ + /// Captures what was logged, so a warning can be asserted on rather than eyeballed. + private sealed class CapturingProvider : ILoggerProvider + { + private readonly List<(LogLevel Level, string Message)> _entries = []; + + public IReadOnlyList<(LogLevel Level, string Message)> Entries + { + get + { + lock (_entries) + { + return [.. _entries]; + } + } + } + + public ILogger CreateLogger(string categoryName) => new Capturing(this); + + public void Dispose() + { + } + + private void Add(LogLevel level, string message) + { + lock (_entries) + { + _entries.Add((level, message)); + } + } + + private sealed class Capturing(CapturingProvider owner) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) => + owner.Add(logLevel, formatter(state, exception)); + } + } + + private static async Task> RunAsync( + Action configureServices, + Action configureApp) + { + var capture = new CapturingProvider(); + + var builder = WebApplication.CreateBuilder(); + // Port 0 lets the OS pick a free one, so parallel tests cannot collide and no + // TestHost package is needed to get real endpoints built. + builder.WebHost.UseUrls("http://127.0.0.1:0"); + builder.Logging.ClearProviders(); + builder.Logging.AddProvider(capture); + + builder.Services.AddHealthie(typeof(UnauthenticatedSurfaceWarningTests).Assembly); + builder.Services.AddAuthorization(); + configureServices(builder.Services); + + var app = builder.Build(); + configureApp(app); + + await app.StartAsync(); + await app.StopAsync(); + await app.DisposeAsync(); + + return capture.Entries; + } + + private static bool Warned(IReadOnlyList<(LogLevel Level, string Message)> entries, string fragment) => + entries.Any(e => e.Level == LogLevel.Warning && e.Message.Contains(fragment, StringComparison.Ordinal)); + + [Fact] + public async Task AnUngatedApi_WarnsAboutTheEndpointsThatCanChangeAChecker() + { + var entries = await RunAsync( + services => services.AddHealthieController(), + app => app.MapControllers()); + + Assert.True( + Warned(entries, "can change a pulse checker are reachable"), + "mapping the controller with no authorization should have warned"); + } + + /// + /// The warning follows what was applied, not the flag that was passed, so a host that required + /// authorization must not be warned at. + /// + [Fact] + public async Task AnApiThatRequiresAuthorization_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieController(requireAuthorization: true), + app => app.MapControllers()); + + Assert.False(Warned(entries, "can change a pulse checker are reachable")); + } + + /// + /// And a host that secured the endpoints its own way -- not through the flag -- is also not + /// warned at. This is the case that makes the check worth doing over the endpoints. + /// + [Fact] + public async Task AnApiSecuredByTheHostItself_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieController(), + app => app.MapControllers().RequireAuthorization()); + + Assert.False(Warned(entries, "can change a pulse checker are reachable")); + } + + [Fact] + public async Task AnUngatedDashboardWithControlsOn_Warns() + { + var entries = await RunAsync( + services => services.AddHealthieUI(), + app => app.MapHealthieUI()); + + Assert.True( + Warned(entries, "controls are on"), + "mapping a writable dashboard with no authorization should have warned"); + } + + [Fact] + public async Task AReadOnlyDashboard_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieUI(options => options.AllowMutations = false), + app => app.MapHealthieUI()); + + Assert.False(Warned(entries, "controls are on")); + } + + [Fact] + public async Task ADashboardBehindAuthorization_IsNotWarnedAt() + { + var entries = await RunAsync( + services => services.AddHealthieUI(), + app => app.MapHealthieUI().RequireAuthorization()); + + Assert.False(Warned(entries, "controls are on")); + } + + /// + /// The reads are not the concern: seeing health without authenticating is a choice an operator + /// can reasonably make, and warning about it would drown the case that matters. + /// + [Fact] + public async Task TheWarning_NamesOnlyTheMutatingRoutes() + { + var entries = await RunAsync( + services => services.AddHealthieController(), + app => app.MapControllers()); + + var warning = entries.Single(e => + e.Level == LogLevel.Warning && e.Message.Contains("can change a pulse checker", StringComparison.Ordinal)); + + Assert.Contains("trigger", warning.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("reset", warning.Message, StringComparison.OrdinalIgnoreCase); + + // "intervals" is the read-only listing endpoint. + Assert.DoesNotContain("healthie/intervals", warning.Message, StringComparison.OrdinalIgnoreCase); + } +}