From 479247cdaebfbe074647a21c9971f9e2c5d54fbe Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Wed, 22 Jul 2026 09:48:33 +0200 Subject: [PATCH] fix(ratelimit): stop agent fleet sharing one anonymous user bucket Device and anonymous routes published the "anonymous" placeholder as RequestContext.UserPrincipalName. UserRateLimitMiddleware skips only on an EMPTY UPN, so every agent of every tenant was bucketed under user_ratelimit_anonymous and throttled against the 120/min per-user limit. On 2026-07-21 21:00-21:30 UTC that rejected 1821 agent requests with 429 (/api/agent/telemetry, config, register-session, error); the 7-day trend shows it recurring and growing with fleet size. - PolicyEnforcementMiddleware: RequestContext.UserPrincipalName carries a genuine JWT UPN or nothing. decision.UserIdentifier keeps the placeholder for log lines. - Device/bootstrap routes already hold their own limit in SecurityValidator (per cert thumbprint / per bootstrap token), so they lose nothing. - stats/platform and diagnostics/download had NO limiter of their own and were riding on the accidental shared bucket. Both now limit per trusted client IP (60/min and 30/min) via ClientIpExtractor, matching the HealthCheck pattern. - 13 new tests pin the empty-UPN contract across all 12 device/anonymous routes, plus a guard that an authenticated caller still surfaces its real UPN. Solution suite 5552 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PolicyEnforcementMiddlewareTests.cs | 59 +++++++++++++++++++ .../DiagnosticsTicketDownloadFunction.cs | 33 ++++++++++- .../Metrics/GetPlatformStatsFunction.cs | 33 ++++++++++- .../Middleware/PolicyEnforcementMiddleware.cs | 10 +++- .../Middleware/UserRateLimitMiddleware.cs | 6 +- 5 files changed, 135 insertions(+), 6 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs index 6b4bc426..50b4e947 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PolicyEnforcementMiddlewareTests.cs @@ -868,4 +868,63 @@ public class PolicyEnforcementMiddlewareTests Assert.False(result.Allowed); Assert.Equal(403, result.StatusCode); } + + // ── Empty-UPN contract for device / anonymous routes ─────────────────────────── + // + // UserRateLimitMiddleware buckets on RequestContext.UserPrincipalName and skips only when it is + // EMPTY. Publishing the "anonymous" placeholder there collapsed every agent of every tenant into a + // single shared user_ratelimit_anonymous bucket, which 429'd agent telemetry fleet-wide once the + // per-user limit was reached. These tests pin the contract: no JWT ⇒ empty UPN. + + [Theory] + [InlineData("POST", "/api/agent/telemetry")] + [InlineData("GET", "/api/agent/config")] + [InlineData("POST", "/api/agent/register-session")] + [InlineData("POST", "/api/agent/upload-url")] + [InlineData("POST", "/api/agent/error")] + [InlineData("POST", "/api/bootstrap/register-session")] + [InlineData("GET", "/api/bootstrap/config")] + [InlineData("POST", "/api/bootstrap/error")] + public async Task DeviceRoute_WithoutJwt_PublishesEmptyUpn(string method, string path) + { + var h = BuildHarness(); + + var result = await h.Middleware.DecideAsync(method, path, null, principal: null); + + Assert.True(result.Allowed); + // The rate-limit bucket key: empty means "no user bucket applies to this call". + Assert.Equal(string.Empty, result.Context!.UserPrincipalName); + // The placeholder still renders in log lines — it just never reaches the RequestContext. + Assert.Equal("anonymous", result.UserIdentifier); + } + + [Theory] + [InlineData("GET", "/api/health")] + [InlineData("GET", "/api/stats/platform")] + [InlineData("POST", "/api/agent/distress")] + [InlineData("GET", "/api/diagnostics/download")] + public async Task AnonymousRoute_WithoutJwt_PublishesEmptyUpn(string method, string path) + { + var h = BuildHarness(); + + var result = await h.Middleware.DecideAsync(method, path, null, principal: null); + + Assert.True(result.Allowed); + Assert.Equal(string.Empty, result.Context!.UserPrincipalName); + } + + [Fact] + public async Task AuthenticatedRoute_StillPublishesTheRealUpn() + { + // Guard the other direction: the fix must not blank the UPN for genuine JWT callers, or + // per-user rate limiting and presence tracking would silently stop working. + const string upn = "admin@contoso.com"; + var h = BuildHarness(); + h.AsTenantAdmin(TenantA, upn); + + var result = await h.Middleware.DecideAsync("GET", "/api/notifications", null, AuthedPrincipal(TenantA, upn)); + + Assert.True(result.Allowed); + Assert.Equal(upn, result.Context!.UserPrincipalName); + } } diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsTicketDownloadFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsTicketDownloadFunction.cs index a3ed41c5..c4359e0f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsTicketDownloadFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Diagnostics/DiagnosticsTicketDownloadFunction.cs @@ -1,5 +1,7 @@ using System.Net; using System.Web; +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Functions.Services.Diagnostics; using AutopilotMonitor.Shared.Diagnostics; using Microsoft.Azure.Functions.Worker; @@ -17,21 +19,29 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics /// from the query string, so a ticket cannot be retargeted. /// /// Replay is bounded by the 10-min ticket TTL, the admin size cap, and the fact that the ticket - /// only ever points at the minting caller's own authorized blob; HMAC forgery is infeasible, so - /// no additional rate limiting is layered here. + /// only ever points at the minting caller's own authorized blob; HMAC forgery is infeasible. On top + /// of that the route carries a per-client-IP limit as defense in depth — it is unauthenticated and + /// streams whole blobs, so a valid ticket replayed in a loop would otherwise be unbounded egress. /// /// public class DiagnosticsTicketDownloadFunction { + // Bounds replay of a still-valid ticket. A real client downloads a package once; the ceiling + // only has to stay clear of a legitimate retry-after-timeout. + private const int MaxRequestsPerMinutePerIp = 30; + private readonly ILogger _logger; private readonly DiagnosticsBlobStreamer _streamer; + private readonly RateLimitService _rateLimitService; public DiagnosticsTicketDownloadFunction( ILogger logger, - DiagnosticsBlobStreamer streamer) + DiagnosticsBlobStreamer streamer, + RateLimitService rateLimitService) { _logger = logger; _streamer = streamer; + _rateLimitService = rateLimitService; } [Function("DiagnosticsTicketDownload")] @@ -40,6 +50,23 @@ namespace AutopilotMonitor.Functions.Functions.Diagnostics { try { + // Rightmost X-Forwarded-For hop only — leftmost entries are caller-controlled. + var clientIp = ClientIpExtractor.GetTrustedClientIp(req); + var rateLimitResult = _rateLimitService.CheckRateLimit( + $"diag_ticket_download_{clientIp}", MaxRequestsPerMinutePerIp); + + if (!rateLimitResult.IsAllowed) + { + _logger.LogWarning("DiagnosticsTicketDownload rate limit exceeded for IP {ClientIp} ({Count} requests)", + clientIp, rateLimitResult.RequestsInWindow); + + var tooMany = req.CreateResponse(HttpStatusCode.TooManyRequests); + if (rateLimitResult.RetryAfter.HasValue) + tooMany.Headers.Add("Retry-After", ((int)rateLimitResult.RetryAfter.Value.TotalSeconds).ToString()); + await tooMany.WriteAsJsonAsync(new { success = false, message = "Rate limit exceeded." }); + return tooMany; + } + var ticket = HttpUtility.ParseQueryString(req.Url.Query)["t"]; if (string.IsNullOrEmpty(ticket)) { diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetPlatformStatsFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetPlatformStatsFunction.cs index 8599f497..fe599832 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetPlatformStatsFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Metrics/GetPlatformStatsFunction.cs @@ -1,4 +1,6 @@ using System.Net; +using AutopilotMonitor.Functions.Security; +using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Shared.DataAccess; using Microsoft.Azure.Functions.Worker; using Microsoft.Azure.Functions.Worker.Http; @@ -9,16 +11,27 @@ namespace AutopilotMonitor.Functions.Functions.Metrics /// /// Public endpoint - no authentication required. /// Returns pre-computed platform stats for the landing page. + /// Rate-limited per client IP: the route is unauthenticated and hits Table Storage on every call, + /// so it needs a limit of its own — no user bucket applies to an anonymous caller. /// public class GetPlatformStatsFunction { + // Landing-page widget: one call per page load. Generous enough that a shared corporate NAT + // never trips it, tight enough to bound an unauthenticated storage read. + private const int MaxRequestsPerMinutePerIp = 60; + private readonly ILogger _logger; private readonly IMetricsRepository _metricsRepo; + private readonly RateLimitService _rateLimitService; - public GetPlatformStatsFunction(ILogger logger, IMetricsRepository metricsRepo) + public GetPlatformStatsFunction( + ILogger logger, + IMetricsRepository metricsRepo, + RateLimitService rateLimitService) { _logger = logger; _metricsRepo = metricsRepo; + _rateLimitService = rateLimitService; } [Function("GetPlatformStats")] @@ -27,6 +40,24 @@ namespace AutopilotMonitor.Functions.Functions.Metrics { try { + // Rightmost X-Forwarded-For hop only — leftmost entries are caller-controlled and would + // let one client rotate the rate-limit key on every request. + var clientIp = ClientIpExtractor.GetTrustedClientIp(req); + var rateLimitResult = _rateLimitService.CheckRateLimit( + $"platform_stats_{clientIp}", MaxRequestsPerMinutePerIp); + + if (!rateLimitResult.IsAllowed) + { + _logger.LogWarning("Platform stats rate limit exceeded for IP {ClientIp} ({Count} requests)", + clientIp, rateLimitResult.RequestsInWindow); + + var tooMany = req.CreateResponse(HttpStatusCode.TooManyRequests); + if (rateLimitResult.RetryAfter.HasValue) + tooMany.Headers.Add("Retry-After", ((int)rateLimitResult.RetryAfter.Value.TotalSeconds).ToString()); + await tooMany.WriteAsJsonAsync(new { error = "Rate limit exceeded" }); + return tooMany; + } + var stats = await _metricsRepo.GetPlatformStatsAsync(); var response = req.CreateResponse(HttpStatusCode.OK); diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs index cdad5a83..de63888c 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/PolicyEnforcementMiddleware.cs @@ -214,7 +214,15 @@ public class PolicyEnforcementMiddleware : IFunctionsWorkerMiddleware { TenantId = jwtTenantId, TargetTenantId = targetTenantId, - UserPrincipalName = decision.UserIdentifier, + // ONLY a genuine JWT UPN lands here — never the "anonymous" placeholder that + // decision.UserIdentifier carries on device/anonymous routes. Downstream consumers treat a + // non-empty UPN as "an identified user made this call": UserRateLimitMiddleware buckets on + // it, so the placeholder collapsed the entire agent fleet (every tenant) into one shared + // user_ratelimit_anonymous bucket and 429'd agent telemetry once the fleet exceeded the + // per-user limit. Device/bootstrap routes carry their own per-cert / per-token limit in + // SecurityValidator; anonymous routes limit themselves per IP. The placeholder stays on + // decision.UserIdentifier for log lines, where "anonymous" is the useful rendering. + UserPrincipalName = upn ?? string.Empty, IsGlobalAdmin = isGlobalAdmin, IsGlobalReader = isGlobalReader, // Delegated flags reflect the caller's role for THIS request (null unless a delegated grant diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/UserRateLimitMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/UserRateLimitMiddleware.cs index af839482..ac414db1 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/UserRateLimitMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/UserRateLimitMiddleware.cs @@ -11,7 +11,11 @@ namespace AutopilotMonitor.Functions.Middleware; /// /// Per-user rate limiting for authenticated (JWT) requests. /// Runs after PolicyEnforcementMiddleware so RequestContext (UPN, IsGlobalAdmin) is already resolved. -/// Agent/device routes have no RequestContext and are automatically skipped. +/// Device/bootstrap and anonymous routes resolve to an EMPTY RequestContext.UserPrincipalName and are +/// skipped here — they carry their own limits (per cert thumbprint / per bootstrap token in +/// SecurityValidator, per client IP on the public endpoints). That empty-UPN contract is what keeps the +/// whole agent fleet out of one shared bucket; see the note on RequestContext construction in +/// PolicyEnforcementMiddleware. /// public class UserRateLimitMiddleware : IFunctionsWorkerMiddleware {