diff --git a/CaddyManager.Services/Caddy/CaddyService.cs b/CaddyManager.Services/Caddy/CaddyService.cs index bb4755e..6bf0d63 100644 --- a/CaddyManager.Services/Caddy/CaddyService.cs +++ b/CaddyManager.Services/Caddy/CaddyService.cs @@ -40,6 +40,13 @@ public class CaddyService( /// public string GetCaddyConfigurationContent(string configurationName) { + // Reading went unguarded while saving, renaming and deleting all validated the name, so a + // caller could walk out of the configuration directory and read any *.caddy file on disk + if (configurationName != CaddyGlobalConfigName && IsInvalidFileName(configurationName)) + { + return string.Empty; + } + var path = configurationName == CaddyGlobalConfigName ? Path.Combine(Configurations.ConfigDir, CaddyGlobalConfigName) : Path.Combine(Configurations.ConfigDir, $"{configurationName}.caddy"); diff --git a/CaddyManager.Tests/Services/Caddy/CaddyServiceTests.cs b/CaddyManager.Tests/Services/Caddy/CaddyServiceTests.cs index 8b250af..b6c9da0 100644 --- a/CaddyManager.Tests/Services/Caddy/CaddyServiceTests.cs +++ b/CaddyManager.Tests/Services/Caddy/CaddyServiceTests.cs @@ -210,6 +210,64 @@ public class CaddyServiceTests : IDisposable result.Should().Be(string.Empty); } + /// + /// Tests that the Caddy service refuses to read a configuration whose name walks out of the configuration directory. + /// Setup: Writes a .caddy file in the parent of the configuration directory and asks for it through a traversing name. + /// Expectation: The service should return an empty string rather than the file's content, so a caller cannot use the read path to reach files the configuration directory does not own — saving, renaming and deleting already reject the same names. + /// + [Theory] + [InlineData("../outside")] + [InlineData("../../outside")] + [InlineData("subdir/../../outside")] + public void GetCaddyConfigurationContent_WithTraversingName_ReturnsEmptyString(string configurationName) + { + // Arrange + var parentDir = Directory.GetParent(_tempConfigDir)!.FullName; + var outsidePath = Path.Combine(parentDir, "outside.caddy"); + File.WriteAllText(outsidePath, "secret { reverse_proxy 10.0.0.1:80 }"); + + try + { + // Act + var result = _service.GetCaddyConfigurationContent(configurationName); + + // Assert + result.Should().Be(string.Empty); + } + finally + { + File.Delete(outsidePath); + } + } + + /// + /// Tests that an absolute path given as a configuration name cannot escape the configuration directory. + /// Setup: Passes an absolute path to a .caddy file that exists outside the configuration directory. + /// Expectation: The service should return an empty string, because Path.Combine would otherwise discard the configuration directory entirely and read the absolute path. + /// + [Fact] + public void GetCaddyConfigurationContent_WithAbsolutePath_ReturnsEmptyString() + { + // Arrange + var parentDir = Directory.GetParent(_tempConfigDir)!.FullName; + var outsidePath = Path.Combine(parentDir, "absolute-outside.caddy"); + File.WriteAllText(outsidePath, "secret { reverse_proxy 10.0.0.1:80 }"); + + try + { + // Act + var result = _service.GetCaddyConfigurationContent( + Path.Combine(parentDir, "absolute-outside")); + + // Assert + result.Should().Be(string.Empty); + } + finally + { + File.Delete(outsidePath); + } + } + /// /// Tests that the Caddy service correctly retrieves the content of the global Caddyfile configuration. /// Setup: Creates a global Caddyfile with known content in the configuration directory. diff --git a/CaddyManager/Program.cs b/CaddyManager/Program.cs index e41682c..c08d10f 100644 --- a/CaddyManager/Program.cs +++ b/CaddyManager/Program.cs @@ -1,3 +1,4 @@ +using System.Net; using CaddyManager.Api; using CaddyManager.Components; using Microsoft.AspNetCore.Components.Server; @@ -64,10 +65,17 @@ builder.Services.Configure(options => { options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto | ForwardedHeaders.XForwardedHost; - // The proxy is another container on a Docker network, so its address is not known up front; - // this app is only ever meant to be reached through that proxy + // The proxy is another container on a Docker network, so its address is not known up front. + // Trusting *any* peer's X-Forwarded-* was too broad: if the port is ever published beyond the + // proxy, a caller can dictate the scheme, host and client IP the app believes in. Docker + // networks and loopback are all private, so trusting only private space keeps the intended + // deployment working while ignoring headers from anywhere else. options.KnownIPNetworks.Clear(); options.KnownProxies.Clear(); + foreach (var network in PrivateProxyNetworks) + { + options.KnownIPNetworks.Add(network); + } }); builder.Services.AddMudServices(config => @@ -104,3 +112,20 @@ app.MapOpenApi(); app.MapScalarApiReference(); app.Run(); + +/// +/// Address space a reverse proxy in front of this app can legitimately come from: loopback, the +/// RFC 1918 ranges Docker hands out, RFC 4193 unique local addresses and IPv6 loopback +/// +public partial class Program +{ + internal static readonly System.Net.IPNetwork[] PrivateProxyNetworks = + [ + new(IPAddress.Parse("127.0.0.0"), 8), + new(IPAddress.Parse("10.0.0.0"), 8), + new(IPAddress.Parse("172.16.0.0"), 12), + new(IPAddress.Parse("192.168.0.0"), 16), + new(IPAddress.Parse("::1"), 128), + new(IPAddress.Parse("fc00::"), 7), + ]; +} diff --git a/README.md b/README.md index 535ead6..2bf35c0 100644 --- a/README.md +++ b/README.md @@ -236,6 +236,38 @@ yourself (the UI warns about this too). > Note: the app redirects HTTP to HTTPS, so a direct `curl http://...` against the container port > gets a `307`. Add `-L`, call it over HTTPS, or go through your reverse proxy. +### Security model + +Read this before exposing the container port anywhere. + +**The web UI has no authentication of its own.** The `X-Api-Key` check applies to `/api/*` only. +Everything else — the whole Blazor UI, which can rewrite any `*.caddy` file, replace the global +Caddyfile, and reload or restart Caddy — is served to whoever can open the port. The container also +mounts the Docker socket in order to reload Caddy, so control of the UI is control of the Docker +daemon on that host. + +So: **put an authenticating reverse proxy in front of this app, and do not publish its port past +that proxy.** Any of the usual options works — the maintainer's own deployment uses +[Authentik](https://goauthentik.io/) forward auth: + +```caddy +example.com { + route { + import authentik_forwardauth + reverse_proxy localhost:8080 + } +} +``` + +Two things worth checking in your own setup: + +- If you expose a second hostname for machine callers (so scripts can use `X-Api-Key` without going + through SSO), scope it to `/api/*` and refuse the rest, or that hostname serves the unauthenticated + UI as well. A hostname whose only extra directive is a `tls` line is **not** access control. +- `X-Forwarded-*` headers are honoured only from loopback and private address space (RFC 1918 / + RFC 4193). A proxy on a Docker network or on the host satisfies this; a proxy reaching the app from + a public address does not, and will see the app fall back to the real peer address and `http`. +

(back to top)