From cfa1cbbed240567af90b78df7cab29c4c2054928 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 4 Sep 2026 14:54:50 +0200 Subject: [PATCH] security: cap the privileged-denial alarm so a valid token cannot flood the operator Any signed-in user holds a token the GA-only routes refuse with 403, so the assume-breach alarm was a push-flood vector: one event per caller AND route per hour, multiplied by Flex instances. PrivilegedDenialReporter now keys the throttle on the caller alone (a sweep over the whole GA surface is one event; every attempt still lands in the Warning trace) and carries a fixed per-instance window budget: 5 events per hour, then ONE Critical storm marker (same event type, so the same Telegram rule pushes it) and trace-only silence until the window resets. Worst case per instance and hour: 6 pushes. Global Reader slips count against the budget too. Tests pin the per-caller throttle, the storm marker and the Reader accounting; D-185 amended. Co-Authored-By: Claude Fable 5.1 --- internal | 2 +- .../PrivilegedDenialReporterTests.cs | 59 ++++++++++++-- .../Services/OpsEventService.cs | 18 +++++ .../Services/PrivilegedDenialReporter.cs | 76 ++++++++++++++----- .../admin/components/OpsAlertRulesSection.tsx | 5 +- 5 files changed, 132 insertions(+), 28 deletions(-) diff --git a/internal b/internal index f3adc51d..3a9c1203 160000 --- a/internal +++ b/internal @@ -1 +1 @@ -Subproject commit f3adc51def32aa0837c132b0754058cab847639d +Subproject commit 3a9c120303a7acec7dc6db2ad1e116bb6d1b7d08 diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/PrivilegedDenialReporterTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/PrivilegedDenialReporterTests.cs index 5dbaf21b..04298d09 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/PrivilegedDenialReporterTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/PrivilegedDenialReporterTests.cs @@ -118,21 +118,64 @@ public class PrivilegedDenialReporterTests } [Fact] - public void Same_caller_and_path_is_throttled_but_another_path_is_not() + public void One_event_per_caller_per_window_whatever_path_another_caller_is_reported() { var (reporter, saved) = Rig(); reporter.Report(Denial()); - reporter.Report(Denial(mcpTool: "query_table")); // same path → throttled - reporter.Report(Denial(path: "/api/global/raw/tables")); // new path → reported + reporter.Report(Denial(mcpTool: "query_table")); // same caller → throttled + reporter.Report(Denial(path: "/api/global/raw/tables")); // same caller, other route → throttled (a sweep is ONE event) reporter.Report(Denial(callerId: "oid-2")); // new caller → reported - var events = Settle(saved, 3); - Assert.Equal(3, events.Count); - Assert.Contains(events, e => e.Message.Contains("/api/global/raw/tables")); - // The second attempt on the same (caller, path) carried a different tool name and was swallowed. + var events = Settle(saved, 2); + Assert.Equal(2, events.Count); + Assert.DoesNotContain(events, e => e.Message.Contains("/api/global/raw/tables")); Assert.DoesNotContain(events, e => e.Message.Contains("query_table")); - Assert.Equal(2, events.Count(e => e.Message.Contains("/api/global/raw/logs"))); + Assert.All(events, e => Assert.Contains("/api/global/raw/logs", e.Message)); + } + + [Fact] + public void Instance_budget_ends_in_one_storm_marker_then_silence() + { + var (reporter, saved) = Rig(); + + // Six distinct callers: five normal events, the sixth trips the storm marker. + for (var i = 1; i <= 6; i++) + reporter.Report(Denial(callerId: $"oid-{i}", path: $"/api/global/raw/route-{i}")); + // Everything after that stays trace-only for the window — new callers included. + for (var i = 7; i <= 40; i++) + reporter.Report(Denial(callerId: $"oid-{i}")); + + var events = Settle(saved, PrivilegedDenialReporter.MaxEventsPerWindow + 1); + Assert.Equal(PrivilegedDenialReporter.MaxEventsPerWindow + 1, events.Count); + Assert.All(events, e => Assert.Equal(OpsEventTypes.PrivilegedRouteDenied, e.EventType)); + + var normal = events.Take(PrivilegedDenialReporter.MaxEventsPerWindow).ToList(); + Assert.All(normal, e => Assert.DoesNotContain("storm", e.Message)); + + var storm = events.Last(); + Assert.Equal(OpsEventSeverity.Critical, storm.Severity); + Assert.Contains("denial storm", storm.Message); + Assert.Contains("/api/global/raw/route-6", storm.Message); + using var details = JsonDocument.Parse(storm.Details!); + Assert.True(details.RootElement.GetProperty("storm").GetBoolean()); + Assert.Equal(PrivilegedDenialReporter.MaxEventsPerWindow, details.RootElement.GetProperty("cap").GetInt32()); + Assert.Equal("oid-1", details.RootElement.GetProperty("lastOid").GetString()); + } + + [Fact] + public void Reader_denials_count_against_the_budget_too() + { + var (reporter, saved) = Rig(); + + for (var i = 1; i <= PrivilegedDenialReporter.MaxEventsPerWindow + 3; i++) + reporter.Report(Denial(callerId: $"reader-{i}", callerRole: Constants.GlobalRoles.GlobalReader)); + + var events = Settle(saved, PrivilegedDenialReporter.MaxEventsPerWindow + 1); + Assert.Equal(PrivilegedDenialReporter.MaxEventsPerWindow + 1, events.Count); + // The five Reader slips are Warning (no push under a Critical rule); the storm marker is Critical. + Assert.Equal(PrivilegedDenialReporter.MaxEventsPerWindow, events.Count(e => e.Severity == OpsEventSeverity.Warning)); + Assert.Equal(OpsEventSeverity.Critical, events.Last().Severity); } [Fact] diff --git a/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs b/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs index 903fed44..b91397ef 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs @@ -349,6 +349,24 @@ namespace AutopilotMonitor.Functions.Services }); } + /// + /// The reporter's per-instance budget is spent: one Critical marker (same event type, so the same + /// alert rule pushes it) that says further denials this window are trace-only. A storm is a + /// stronger signal than any single denial — either someone is sweeping the GA surface with a + /// valid token, or an operator script is misconfigured; both deserve the one push, neither + /// deserves fifty. + /// + public Task RecordPrivilegedRouteDenialStormAsync(PrivilegedDenial last, int cap, TimeSpan window) + => WriteAsync(OpsEventCategory.Security, OpsEventTypes.PrivilegedRouteDenied, OpsEventSeverity.Critical, + $"Privileged-route denial storm: {cap} events in {window.TotalMinutes:0} min on this instance; further denials stay in the Warning trace until the window resets (last: {last.Method} {last.Path} by {last.Upn ?? "(no upn)"})", + last.TenantId, last.Upn ?? "System.PolicyEnforcement", + new + { + storm = true, cap, windowMinutes = (int)window.TotalMinutes, + lastMethod = last.Method, lastPath = last.Path, lastUpn = last.Upn, lastOid = last.ObjectId, lastTid = last.TenantId, + lastClientSource = last.ClientSource, lastMcpToolName = last.McpToolName, + }); + /// /// Fired by when a Kill signal was actually SERVED to /// an agent (as opposed to DeviceBlocked/VersionBlocked, which fire when the admin diff --git a/src/Backend/AutopilotMonitor.Functions/Services/PrivilegedDenialReporter.cs b/src/Backend/AutopilotMonitor.Functions/Services/PrivilegedDenialReporter.cs index 4756cda9..243edeb7 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/PrivilegedDenialReporter.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/PrivilegedDenialReporter.cs @@ -40,15 +40,34 @@ namespace AutopilotMonitor.Functions.Services /// route into a PrivilegedRouteDenied ops event (Critical, or Warning for a Global Reader) /// that operators route to a push channel. The authorization decision itself is not touched — this /// only makes a refused probe visible with the backend's own view of the caller's identity. + /// + /// FLOOD CONTROL. Any holder of a valid token for the API (every signed-in tenant user) can produce + /// these denials at will, so an unbounded alarm would hand an attacker the operator's attention as a + /// denial-of-service target. Two caps, both per instance: + /// + /// one event per CALLER per hour, whatever route or tool they try — a sweep over every GA route + /// is one event, not one per route (every attempt still lands in the Warning trace); + /// at most events per hour in total; the next denial produces ONE + /// Critical "storm" marker and everything after it stays trace-only until the window resets. + /// + /// Worst case per instance and hour: + 1 pushes. Flex Consumption + /// fans a burst over several instances, so the fleet-wide figure is that times the instances that saw + /// traffic — bounded, and the storm marker is itself the stronger signal. + /// /// public sealed class PrivilegedDenialReporter : IPrivilegedDenialReporter { - /// One event per (caller, path) per hour; the rest stays in the Warning trace. - internal static readonly TimeSpan Throttle = TimeSpan.FromHours(1); + /// Per-caller and per-instance budget window. + internal static readonly TimeSpan Window = TimeSpan.FromHours(1); + /// Normal events an instance emits per window before it switches to one storm marker. + internal const int MaxEventsPerWindow = 5; + + private const string BudgetKey = "prd:budget"; private readonly OpsEventService _opsEvents; private readonly IMemoryCache _cache; private readonly ILogger _logger; + private readonly object _gate = new(); public PrivilegedDenialReporter(OpsEventService opsEvents, IMemoryCache cache, ILogger logger) { @@ -61,22 +80,38 @@ namespace AutopilotMonitor.Functions.Services { try { - // The throttle is an in-process IMemoryCache, i.e. PER FUNCTION INSTANCE. On Flex - // Consumption a burst from one caller can fan out across instances, so the same - // (caller, path) can surface once per instance that saw it. Deliberately accepted — - // the count is a "how many instances noticed" figure, not an incident count, and a - // distributed throttle is not worth a storage round-trip on a deny path. Keyed on the - // path, not the tool name: a scripted sweep over every GA route still yields one - // event per route, while a repeat on the same route stays quiet for an hour. - var key = $"prd:{denial.CallerId}:{denial.Path}"; - if (_cache.TryGetValue(key, out _)) - return; - _cache.Set(key, true, Throttle); + Task? write; + lock (_gate) + { + // Per caller, not per path: the key is the identity, so a scripted sweep over the + // whole GA surface yields one event carrying the first path it touched. + var callerKey = $"prd:{denial.CallerId}"; + if (_cache.TryGetValue(callerKey, out _)) + return; - _ = _opsEvents.RecordPrivilegedRouteDeniedAsync(denial) - .ContinueWith( - t => _logger.LogWarning(t.Exception?.InnerException, "PrivilegedRouteDenied ops event failed"), - TaskContinuationOptions.OnlyOnFaulted); + // Per-instance budget, FIXED window from the first event (IMemoryCache is per Function + // instance — deliberately no storage round-trip on a deny path; see the class comment + // for the fleet arithmetic). + var now = DateTimeOffset.UtcNow; + var budget = _cache.TryGetValue(BudgetKey, out var open) && open != null && open.EndsAt > now + ? open + : new BudgetWindow(now.Add(Window)); + budget.Count++; + _cache.Set(BudgetKey, budget, budget.EndsAt - now); + + var count = budget.Count; + if (count > MaxEventsPerWindow + 1) + return; // storm already marked this window: trace-only from here + + _cache.Set(callerKey, true, Window); + write = count <= MaxEventsPerWindow + ? _opsEvents.RecordPrivilegedRouteDeniedAsync(denial) + : _opsEvents.RecordPrivilegedRouteDenialStormAsync(denial, MaxEventsPerWindow, Window); + } + + _ = write.ContinueWith( + t => _logger.LogWarning(t.Exception?.InnerException, "PrivilegedRouteDenied ops event failed"), + TaskContinuationOptions.OnlyOnFaulted); } catch (Exception ex) { @@ -84,5 +119,12 @@ namespace AutopilotMonitor.Functions.Services _logger.LogWarning(ex, "PrivilegedRouteDenied reporting failed"); } } + + private sealed class BudgetWindow + { + public BudgetWindow(DateTimeOffset endsAt) => EndsAt = endsAt; + public DateTimeOffset EndsAt { get; } + public int Count { get; set; } + } } } diff --git a/src/Web/autopilot-monitor-web/app/admin/components/OpsAlertRulesSection.tsx b/src/Web/autopilot-monitor-web/app/admin/components/OpsAlertRulesSection.tsx index 8e5b8c88..4546afb0 100644 --- a/src/Web/autopilot-monitor-web/app/admin/components/OpsAlertRulesSection.tsx +++ b/src/Web/autopilot-monitor-web/app/admin/components/OpsAlertRulesSection.tsx @@ -99,8 +99,9 @@ const OPS_EVENT_TYPES: Record = { "ExcessiveSessionEventsAutoActioned", // Assume-breach layer for the Global-Admin-only surface: an authenticated caller without the // GlobalAdmin role hit a GlobalAdminOnly route (or, via the MCP probe, a GA-only tool). - // Critical for callers without any platform role, Warning for a Global Reader; throttled 1h - // per caller+path (PrivilegedDenialReporter). Route it to Telegram at MinSeverity Critical. + // Critical for callers without any platform role, Warning for a Global Reader. Flood-capped + // per instance: one event per caller per hour and at most 5 per hour in total, then ONE + // Critical "storm" marker (PrivilegedDenialReporter). Route it to Telegram at MinSeverity Critical. "PrivilegedRouteDenied", "VersionBlocked", // Delivery confirmation: a Kill signal was actually SERVED to an agent (config or