diff --git a/src/BuildingBlocks/Web/Extensions.cs b/src/BuildingBlocks/Web/Extensions.cs index 50c6568fda..5a270d4553 100644 --- a/src/BuildingBlocks/Web/Extensions.cs +++ b/src/BuildingBlocks/Web/Extensions.cs @@ -20,8 +20,10 @@ using FSH.Framework.Web.RateLimiting; using FSH.Framework.Web.Realtime; using FSH.Framework.Web.Security; +using FSH.Framework.Web.TrustedProxy; using FSH.Framework.Web.Versioning; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.HttpOverrides; using Microsoft.AspNetCore.ResponseCompression; using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.Configuration; @@ -29,6 +31,7 @@ using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Hosting; using Mediator; +using System.Net; namespace FSH.Framework.Web; @@ -63,6 +66,51 @@ public static IHostApplicationBuilder AddHeroPlatform(this IHostApplicationBuild } builder.Services.AddHttpContextAccessor(); + + // The app runs behind a reverse proxy (e.g. cloudflared → Caddy → app), so the real client IP + // and scheme arrive via X-Forwarded-*. Without this, RemoteIpAddress is the proxy's container + // IP, which collapses the rate-limit partition into one bucket and records useless audit IPs. + // Trust is bound to the configured ingress CIDRs/proxies (see TrustedProxyOptions): forwarded + // headers from any other source are ignored, so a client reaching the app directly cannot forge + // its IP/scheme. With nothing configured, the framework default (loopback only) stands. + var trustedProxy = builder.Configuration + .GetSection(nameof(TrustedProxyOptions)).Get() ?? new TrustedProxyOptions(); + builder.Services.Configure(forwarded => + { + forwarded.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; + forwarded.ForwardLimit = trustedProxy.ForwardLimit; + + if (trustedProxy.KnownProxies.Length == 0 && trustedProxy.KnownNetworks.Length == 0) + { + return; + } + + forwarded.KnownProxies.Clear(); + forwarded.KnownIPNetworks.Clear(); + + foreach (var proxy in trustedProxy.KnownProxies) + { + if (!IPAddress.TryParse(proxy, out var address)) + { + throw new InvalidOperationException( + $"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownProxies)} contains \"{proxy}\", which is not a valid IP address (for example \"10.0.0.5\")."); + } + + forwarded.KnownProxies.Add(address); + } + + foreach (var network in trustedProxy.KnownNetworks) + { + if (!System.Net.IPNetwork.TryParse(network, out var parsedNetwork)) + { + throw new InvalidOperationException( + $"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownNetworks)} contains \"{network}\", which is not a valid CIDR network (for example \"10.0.0.0/8\")."); + } + + forwarded.KnownIPNetworks.Add(parsedNetwork); + } + }); + builder.Services.AddHeroDatabaseOptions(builder.Configuration); builder.Services.AddHeroRateLimiting(builder.Configuration); @@ -150,6 +198,11 @@ public static WebApplication UseHeroPlatform(this WebApplication app, Action +/// Trusted reverse-proxy configuration for X-Forwarded-* processing. Behind an ingress +/// (e.g. cloudflared → Caddy → app) the real client IP and scheme arrive via forwarded headers; +/// these settings bound which upstream sources are trusted so a client reaching the app from +/// outside the proxy network cannot forge its own IP/scheme. When no proxies or networks are +/// configured, the framework default (loopback only) stands and forwarded headers from any other +/// source are ignored. +/// +public sealed class TrustedProxyOptions +{ + /// Individual upstream proxy IP addresses whose X-Forwarded-* headers are trusted. + public string[] KnownProxies { get; init; } = []; + + /// Trusted upstream networks in CIDR notation (e.g. "10.0.0.0/8", "172.16.0.0/12"). + public string[] KnownNetworks { get; init; } = []; + + /// + /// Number of proxy hops to unwind from X-Forwarded-For. Must match the real ingress hop count + /// (cloudflared → Caddy → app is 2). The framework default of 1 reads only the rightmost hop, + /// which yields the nearest proxy's IP (or an attacker-injected value) in a multi-hop topology. + /// + public int ForwardLimit { get; init; } = 1; +} diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 0d38b28190..7674befa8f 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -143,5 +143,12 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> + + \ No newline at end of file diff --git a/src/Host/FSH.Starter.Api/appsettings.Production.json b/src/Host/FSH.Starter.Api/appsettings.Production.json index 332724534b..8cf660327d 100644 --- a/src/Host/FSH.Starter.Api/appsettings.Production.json +++ b/src/Host/FSH.Starter.Api/appsettings.Production.json @@ -91,6 +91,11 @@ "Ip": { "PermitLimit": 300, "WindowSeconds": 60, "QueueLimit": 0 }, "Auth": { "PermitLimit": 10, "WindowSeconds": 60, "QueueLimit": 0 } }, + "TrustedProxyOptions": { + "KnownProxies": [], + "KnownNetworks": [], + "ForwardLimit": 1 + }, "Storage": { "Provider": "local" } diff --git a/src/Host/FSH.Starter.Api/appsettings.json b/src/Host/FSH.Starter.Api/appsettings.json index 293fdfebb6..6a0a968ba2 100644 --- a/src/Host/FSH.Starter.Api/appsettings.json +++ b/src/Host/FSH.Starter.Api/appsettings.json @@ -156,6 +156,11 @@ "QueueLimit": 0 } }, + "TrustedProxyOptions": { + "KnownProxies": [], + "KnownNetworks": [], + "ForwardLimit": 1 + }, "Storage": { "Provider": "local" }, diff --git a/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs new file mode 100644 index 0000000000..d4204524d9 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/TrustedProxyOptionsBindingTests.cs @@ -0,0 +1,97 @@ +using FSH.Framework.Web; +using FSH.Framework.Web.TrustedProxy; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; + +namespace Framework.Tests.Web; + +/// +/// Pins the TrustedProxyOptions -> ForwardedHeadersOptions binding that AddHeroPlatform registers: which +/// upstreams end up trusted, that an unconfigured section keeps the framework's loopback-only default, and +/// that a malformed entry surfaces a message naming the offending setting rather than a bare FormatException. +/// +public sealed class TrustedProxyOptionsBindingTests +{ + private const string ProxyIp = "192.0.2.10"; + + private static ForwardedHeadersOptions Resolve(Dictionary settings) + { + // DisableDefaults keeps the host's environment-variable and appsettings providers out, so an ambient + // TrustedProxyOptions__* on the machine or CI runner can't change what "nothing configured" resolves to. + var builder = Host.CreateApplicationBuilder(new HostApplicationBuilderSettings + { + DisableDefaults = true, + }); + builder.Configuration.AddInMemoryCollection(settings); + builder.AddHeroPlatform(); + + using var provider = builder.Services.BuildServiceProvider(); + return provider.GetRequiredService>().Value; + } + + #region Trust boundary + + [Fact] + public void ForwardedHeaders_Should_KeepFrameworkLoopbackDefault_When_NothingConfigured() + { + // Act + var options = Resolve([]); + + // Assert - clearing the framework default here would make every caller a trusted proxy. + options.KnownProxies.ShouldNotBeEmpty(); + options.KnownIPNetworks.ShouldNotBeEmpty(); + } + + [Fact] + public void ForwardedHeaders_Should_TrustOnlyConfiguredProxy_When_KnownProxiesSet() + { + // Act + var options = Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownProxies)}:0"] = ProxyIp, + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.ForwardLimit)}"] = "2", + }); + + // Assert + options.KnownProxies.ShouldBe([System.Net.IPAddress.Parse(ProxyIp)]); + options.KnownIPNetworks.ShouldBeEmpty(); + options.ForwardLimit.ShouldBe(2); + } + + #endregion + + #region Malformed configuration + + [Fact] + public void ForwardedHeaders_Should_NameTheSetting_When_KnownProxyMalformed() + { + // Act + var exception = Should.Throw(() => Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownProxies)}:0"] = "not-an-ip", + })); + + // Assert + exception.Message.ShouldContain("TrustedProxyOptions:KnownProxies"); + exception.Message.ShouldContain("not-an-ip"); + } + + [Fact] + public void ForwardedHeaders_Should_NameTheSetting_When_KnownNetworkMalformed() + { + // Act + var exception = Should.Throw(() => Resolve(new Dictionary + { + [$"{nameof(TrustedProxyOptions)}:{nameof(TrustedProxyOptions.KnownNetworks)}:0"] = "10.0.0.0/999", + })); + + // Assert + exception.Message.ShouldContain("TrustedProxyOptions:KnownNetworks"); + exception.Message.ShouldContain("10.0.0.0/999"); + } + + #endregion +} diff --git a/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs b/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs index ab8cfe3c65..99b621799c 100644 --- a/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs +++ b/src/Tests/Integration.Tests/Infrastructure/FshWebApplicationFactory.cs @@ -153,6 +153,22 @@ protected override void ConfigureWebHost(IWebHostBuilder builder) builder.ConfigureServices(services => { + // Stamp the connection IP from a test header so forwarded-headers trust checks are testable. + services.AddSingleton(); + + // The production TrustedProxyOptions read happens eagerly, before the test config overlay + // applies (same quirk as storage below), so bind the trusted upstream here instead. This + // exercises the real UseForwardedHeaders trust boundary against TestConstants.TrustedProxyIp. + services.PostConfigure(forwarded => + { + forwarded.ForwardedHeaders = Microsoft.AspNetCore.HttpOverrides.ForwardedHeaders.XForwardedFor + | Microsoft.AspNetCore.HttpOverrides.ForwardedHeaders.XForwardedProto; + forwarded.ForwardLimit = 1; + forwarded.KnownProxies.Clear(); + forwarded.KnownIPNetworks.Clear(); + forwarded.KnownProxies.Add(System.Net.IPAddress.Parse(TestConstants.TrustedProxyIp)); + }); + // Remove hosted services that need unavailable infra or race migrations (RolePermissionSync, // Hangfire server + stale-lock cleanup, OutboxDispatcher); we register our own InMemory server below. var hostedServicesToRemove = services diff --git a/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs b/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs index d5a2a6e49c..2b49c3398a 100644 --- a/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs +++ b/src/Tests/Integration.Tests/Infrastructure/TestConstants.cs @@ -6,6 +6,10 @@ public static class TestConstants public const string RootAdminEmail = "admin@root.com"; public const string DefaultPassword = "123Pa$$word!"; + // Documentation IP ranges (RFC 5737) so the trusted-proxy fixture never collides with a real host. + public const string TrustedProxyIp = "192.0.2.10"; + public const string UntrustedSourceIp = "198.51.100.9"; + public const string JwtIssuer = "fsh.local"; public const string JwtAudience = "fsh.clients"; public const string JwtSigningKey = "integration-test-signing-key-that-is-at-least-32-chars-long!!"; diff --git a/src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs b/src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs new file mode 100644 index 0000000000..118c96a91b --- /dev/null +++ b/src/Tests/Integration.Tests/Infrastructure/TestRemoteIpStartupFilter.cs @@ -0,0 +1,35 @@ +using System.Net; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; + +namespace Integration.Tests.Infrastructure; + +/// +/// TestServer has no real socket, so Connection.RemoteIpAddress is null and the +/// forwarded-headers trust check (known proxies/networks) can't be exercised. This filter runs +/// before the app pipeline (hence before UseForwardedHeaders) and stamps the connection IP from the +/// X-Test-Remote-Ip header so a test can present itself as a trusted or untrusted upstream. +/// Inert for requests that don't carry the header. +/// +public sealed class TestRemoteIpStartupFilter : IStartupFilter +{ + public const string RemoteIpHeader = "X-Test-Remote-Ip"; + + public Action Configure(Action next) => + app => + { + app.Use(async (context, nextMiddleware) => + { + var header = context.Request.Headers[RemoteIpHeader].FirstOrDefault(); + if (!string.IsNullOrEmpty(header) && IPAddress.TryParse(header, out var ip)) + { + context.Connection.RemoteIpAddress = ip; + } + + await nextMiddleware(); + }); + + next(app); + }; +} diff --git a/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs new file mode 100644 index 0000000000..997c15ab85 --- /dev/null +++ b/src/Tests/Integration.Tests/Tests/Security/ForwardedHeadersIpTests.cs @@ -0,0 +1,106 @@ +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; +using FSH.Framework.Shared.Multitenancy; +using FSH.Modules.Identity.Data; +using Integration.Tests.Infrastructure; + +namespace Integration.Tests.Tests.Security; + +/// +/// Runtime repro for audit finding API-02 (no UseForwardedHeaders → proxy IP collapses the real +/// client IP) plus its security boundary. Token issuance persists a UserSession whose IpAddress comes +/// from RequestContextService.IpAddress => Connection.RemoteIpAddress. The forwarded-headers config +/// trusts only the configured upstream (TestConstants.TrustedProxyIp), so: +/// - a request arriving from the trusted proxy has its X-Forwarded-For honored (real client IP), and +/// - a request arriving from any other source has X-Forwarded-For ignored (spoofing is blocked). +/// TestServer has no socket, so the connection IP is stamped via the X-Test-Remote-Ip header (see +/// TestRemoteIpStartupFilter). +/// +[Collection(FshCollectionDefinition.Name)] +public sealed class ForwardedHeadersIpTests +{ + private const string ForwardedClientIp = "203.0.113.7"; + + private readonly FshWebApplicationFactory _factory; + + public ForwardedHeadersIpTests(FshWebApplicationFactory factory) + { + _factory = factory; + } + + [Fact] + public async Task TokenIssue_Should_RecordForwardedClientIp_When_RequestArrivesFromTrustedProxy() + { + var recordedIp = await IssueTokenAndReadSessionIpAsync( + connectionIp: TestConstants.TrustedProxyIp, + forwardedFor: ForwardedClientIp); + + recordedIp.ShouldBe( + ForwardedClientIp, + "behind a trusted proxy the persisted session IP should be the real client IP from " + + "X-Forwarded-For."); + } + + [Fact] + public async Task TokenIssue_Should_IgnoreForwardedClientIp_When_RequestArrivesFromUntrustedSource() + { + // Both arms send the SAME X-Forwarded-For and differ only in the source address. Asserting the + // untrusted outcome alone would pass even with forwarded-header processing removed entirely - the + // connection IP gets persisted either way - so the trusted arm is what makes this a boundary test. + var fromUntrusted = await IssueTokenAndReadSessionIpAsync( + connectionIp: TestConstants.UntrustedSourceIp, + forwardedFor: ForwardedClientIp); + + var fromTrusted = await IssueTokenAndReadSessionIpAsync( + connectionIp: TestConstants.TrustedProxyIp, + forwardedFor: ForwardedClientIp); + + fromUntrusted.ShouldBe( + TestConstants.UntrustedSourceIp, + "X-Forwarded-For from a source outside the trusted-proxy set must be ignored; the persisted " + + "IP should be the connection IP, never the attacker-supplied forwarded value."); + fromUntrusted.ShouldNotBe(ForwardedClientIp); + + fromTrusted.ShouldBe( + ForwardedClientIp, + "the identical header from the trusted proxy must be honored - otherwise the assertion above " + + "passes vacuously, satisfied by forwarded headers never being processed at all."); + } + + private async Task IssueTokenAndReadSessionIpAsync(string connectionIp, string forwardedFor) + { + using var client = _factory.CreateClient(); + using var request = new HttpRequestMessage(HttpMethod.Post, $"{TestConstants.IdentityBasePath}/token/issue"); + request.Headers.Add("tenant", TestConstants.RootTenantId); + request.Headers.Add(TestRemoteIpStartupFilter.RemoteIpHeader, connectionIp); + request.Headers.Add("X-Forwarded-For", forwardedFor); + request.Content = JsonContent.Create(new + { + email = TestConstants.RootAdminEmail, + password = TestConstants.DefaultPassword, + }); + + using var response = await client.SendAsync(request); + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + return await GetNewestSessionIpAsync(); + } + + private async Task GetNewestSessionIpAsync() + { + using var scope = _factory.Services.CreateScope(); + + var tenantStore = scope.ServiceProvider.GetRequiredService>(); + var tenant = await tenantStore.GetAsync(TestConstants.RootTenantId); + scope.ServiceProvider.GetRequiredService().MultiTenantContext = + new MultiTenantContext(tenant); + + var db = scope.ServiceProvider.GetRequiredService(); + var session = await db.UserSessions + .AsNoTracking() + .OrderByDescending(s => s.CreatedAt) + .FirstOrDefaultAsync(); + + return session?.IpAddress; + } +}