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) <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-07-22 09:48:33 +02:00
parent b6ba87f072
commit 479247cdae
5 changed files with 135 additions and 6 deletions
@@ -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);
}
}
@@ -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.
/// <para>
/// 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.
/// </para>
/// </summary>
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<DiagnosticsTicketDownloadFunction> _logger;
private readonly DiagnosticsBlobStreamer _streamer;
private readonly RateLimitService _rateLimitService;
public DiagnosticsTicketDownloadFunction(
ILogger<DiagnosticsTicketDownloadFunction> 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))
{
@@ -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
/// <summary>
/// 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.
/// </summary>
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<GetPlatformStatsFunction> _logger;
private readonly IMetricsRepository _metricsRepo;
private readonly RateLimitService _rateLimitService;
public GetPlatformStatsFunction(ILogger<GetPlatformStatsFunction> logger, IMetricsRepository metricsRepo)
public GetPlatformStatsFunction(
ILogger<GetPlatformStatsFunction> 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);
@@ -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
@@ -11,7 +11,11 @@ namespace AutopilotMonitor.Functions.Middleware;
/// <summary>
/// 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.
/// </summary>
public class UserRateLimitMiddleware : IFunctionsWorkerMiddleware
{