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 {