From f2257a4b02de35de6e24d07dd562ac7d4a97c613 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 8 May 2026 16:16:12 +0200 Subject: [PATCH] fix(backend/security): cert-expiry watcher uses bucket-level logic, not per-cert 77fde56f's per-cert alarm fires for every expiring cert independently. That is wrong: when a Root or Intermediate has a fresh successor in the bundle (e.g. intune-intermediate-mdm-2026.pem alongside intune-intermediate-mdm-2028 .pem), the older cert is just a chain-continuity helper for in-flight device certs. Alarming on it would page the operator to "rotate" something that is already rotated. Refactor to bucket-level: per role (Root, Intermediate), pick the cert with the MAXIMUM NotAfter and classify only that. Older expiring/expired siblings are silent. Operator only gets paged when the freshest itself nears expiry, i.e. when there is genuinely no successor yet. Concrete behaviour change for current bundle (today 2026-05-08): Roots: freshest is intune-root-2030.pem (1591d) -> silent Intermediates: freshest is intune-intermediate-mdm-2028.pem (859d) -> silent The expired intune-intermediate-mdm-2026.pem (-3d) no longer triggers a spurious Critical because mdm-2028 has it covered. Tests: 7 new EvaluateBucket cases (empty, single fresh, single soon, old+ fresh successor stays silent, old+soon successor fires on successor, all expired falls back to freshest-expired, two future successors). Total 38 cert-related tests, full suite 1105/1105. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../MaintenanceServiceCertExpiryTests.cs | 117 ++++++++++++++++++ .../Services/MaintenanceService.CertExpiry.cs | 102 +++++++++------ .../Services/OpsEventService.cs | 8 +- 3 files changed, 188 insertions(+), 39 deletions(-) diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/MaintenanceServiceCertExpiryTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/MaintenanceServiceCertExpiryTests.cs index 403feebd..656c222a 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/MaintenanceServiceCertExpiryTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/MaintenanceServiceCertExpiryTests.cs @@ -1,3 +1,5 @@ +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; using AutopilotMonitor.Functions.Services; namespace AutopilotMonitor.Functions.Tests; @@ -115,4 +117,119 @@ public class MaintenanceServiceCertExpiryTests var json = """{"thumbprint":12345}"""; Assert.Equal("", MaintenanceService.ExtractThumbprint(json)); } + + // --- EvaluateBucket (bucket-level: only the freshest cert drives the alarm) --- + // Correctness invariant under test: an old cert in the same role bucket must NOT + // generate its own warning as long as a fresh successor is present. The bucket is + // judged by its newest member's NotAfter, because that's when "no chain helper + // ever again" actually arrives. + + [Fact] + public void EvaluateBucket_EmptyBucket_ReturnsNullEverything() + { + var eval = MaintenanceService.EvaluateBucket(Array.Empty(), DateTime.UtcNow); + + Assert.Null(eval.EventType); + Assert.Null(eval.Freshest); + } + + [Fact] + public void EvaluateBucket_SingleFreshCert_StaysSilent() + { + var now = new DateTime(2026, 5, 8, 12, 0, 0, DateTimeKind.Utc); + using var fresh = MakeCert("CN=Fresh Root", notAfter: now.AddDays(180)); + + var eval = MaintenanceService.EvaluateBucket(new[] { fresh }, now); + + Assert.Null(eval.EventType); + Assert.Equal(fresh.Thumbprint, eval.Freshest!.Thumbprint); + } + + [Fact] + public void EvaluateBucket_SingleSoonCert_FiresExpiringSoon() + { + var now = new DateTime(2026, 5, 8, 12, 0, 0, DateTimeKind.Utc); + using var soon = MakeCert("CN=Soon", notAfter: now.AddDays(60)); + + var eval = MaintenanceService.EvaluateBucket(new[] { soon }, now); + + Assert.Equal("EmbeddedCertExpiringSoon", eval.EventType); + Assert.Equal(soon.Thumbprint, eval.Freshest!.Thumbprint); + Assert.Equal(60, eval.DaysUntilExpiry); + } + + [Fact] + public void EvaluateBucket_OldExpiredAndFreshSuccessor_StaysSilent() + { + // The user's correctness ask: bundle has both intune-intermediate-mdm-2026 + // (expired) AND intune-intermediate-mdm-2028 (fresh). The old one is just a + // chain helper for in-flight device certs; the bucket is healthy because the + // successor is in place. NO alarm should fire. + var now = new DateTime(2026, 5, 8, 12, 0, 0, DateTimeKind.Utc); + using var oldExpired = MakeCert("CN=Old Intermediate", notAfter: now.AddDays(-3)); + using var freshSuccessor = MakeCert("CN=New Intermediate", notAfter: now.AddDays(800)); + + var eval = MaintenanceService.EvaluateBucket(new[] { oldExpired, freshSuccessor }, now); + + Assert.Null(eval.EventType); + Assert.Equal(freshSuccessor.Thumbprint, eval.Freshest!.Thumbprint); + } + + [Fact] + public void EvaluateBucket_OldExpiredAndSoonSuccessor_FiresOnSuccessor() + { + // Bundle has an expired old + a soon-expiring "newest". The alarm fires for + // the soon one (it IS the bucket's successor, so its expiry IS the bucket's + // expiry). Critical for the old cert is suppressed. + var now = new DateTime(2026, 5, 8, 12, 0, 0, DateTimeKind.Utc); + using var oldExpired = MakeCert("CN=Old", notAfter: now.AddDays(-30)); + using var soonSuccessor = MakeCert("CN=Soon Successor", notAfter: now.AddDays(50)); + + var eval = MaintenanceService.EvaluateBucket(new[] { oldExpired, soonSuccessor }, now); + + Assert.Equal("EmbeddedCertExpiringSoon", eval.EventType); + Assert.Equal(soonSuccessor.Thumbprint, eval.Freshest!.Thumbprint); + } + + [Fact] + public void EvaluateBucket_AllExpired_FiresExpiredOnFreshestExpired() + { + // Edge case: bundle has only expired certs, no successor. Freshest is still + // the latest-expiring one. Alarm fires Critical, message reflects the freshest. + var now = new DateTime(2026, 5, 8, 12, 0, 0, DateTimeKind.Utc); + using var ancient = MakeCert("CN=Ancient", notAfter: now.AddDays(-100)); + using var recent = MakeCert("CN=Recent", notAfter: now.AddDays(-3)); + + var eval = MaintenanceService.EvaluateBucket(new[] { ancient, recent }, now); + + Assert.Equal("EmbeddedCertExpired", eval.EventType); + Assert.Equal(recent.Thumbprint, eval.Freshest!.Thumbprint); + Assert.True(eval.DaysUntilExpiry < 0); + } + + [Fact] + public void EvaluateBucket_TwoFreshSuccessors_StaysSilentOnLatest() + { + // Bundle has two valid successors (e.g. a current root and a future root). + // Both are far from expiry; pick the latest by NotAfter. + var now = new DateTime(2026, 5, 8, 12, 0, 0, DateTimeKind.Utc); + using var nearFuture = MakeCert("CN=2026 Root", notAfter: new DateTime(2026, 8, 12, 0, 0, 0, DateTimeKind.Utc)); + using var farFuture = MakeCert("CN=2030 Root", notAfter: new DateTime(2030, 9, 15, 0, 0, 0, DateTimeKind.Utc)); + + var eval = MaintenanceService.EvaluateBucket(new[] { nearFuture, farFuture }, now); + + Assert.Null(eval.EventType); + Assert.Equal(farFuture.Thumbprint, eval.Freshest!.Thumbprint); + } + + private static X509Certificate2 MakeCert(string subjectCn, DateTime notAfter, DateTime? notBefore = null) + { + // Self-signed cert factory for unit tests. The validator's chain check is + // not part of this code path - we only care about NotBefore/NotAfter and + // Thumbprint for bucket evaluation. + using var rsa = RSA.Create(2048); + var req = new CertificateRequest($"CN={subjectCn}", rsa, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); + var nb = (notBefore ?? notAfter.AddYears(-1)).ToUniversalTime(); + return req.CreateSelfSigned(new DateTimeOffset(nb, TimeSpan.Zero), new DateTimeOffset(notAfter.ToUniversalTime(), TimeSpan.Zero)); + } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/MaintenanceService.CertExpiry.cs b/src/Backend/AutopilotMonitor.Functions/Services/MaintenanceService.CertExpiry.cs index e31c90dd..8d4a3a99 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/MaintenanceService.CertExpiry.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/MaintenanceService.CertExpiry.cs @@ -44,11 +44,47 @@ namespace AutopilotMonitor.Functions.Services return null; } + /// + /// Bucket-level evaluation for a role group (e.g. all "Root" certs). + /// Picks the cert with the MAXIMUM NotAfter and classifies its tier; + /// older certs in the same bucket are chain-helpers for in-flight + /// device certs (e.g. a soon-to-expire intermediate that already has + /// a successor in the bundle) and do NOT trigger their own alarm. + /// + /// This is the correctness invariant: if a freshness-successor is + /// already embedded for this role, the bundle is healthy regardless + /// of how many older expiring/expired siblings ride along for chain + /// continuity. The operator only gets paged when the freshest itself + /// nears expiry, i.e. when there is genuinely no successor yet. + /// + internal static CertBucketEvaluation EvaluateBucket( + IReadOnlyList certs, DateTime nowUtc) + { + if (certs.Count == 0) + return new CertBucketEvaluation(null, null, 0); + + var freshest = certs + .OrderByDescending(c => c.NotAfter.ToUniversalTime()) + .First(); + var daysLeft = (int)Math.Floor( + (freshest.NotAfter.ToUniversalTime() - nowUtc).TotalDays); + var tier = ClassifyCertExpiryTier(daysLeft); + + return new CertBucketEvaluation(tier, freshest, daysLeft); + } + + internal sealed record CertBucketEvaluation( + string? EventType, + X509Certificate2? Freshest, + int DaysUntilExpiry); + /// /// Inspects the embedded Intune CA bundle (roots + intermediates) and - /// emits Warning/Error/Critical OpsEvents for any cert near expiry. - /// Dedup: one event per cert per tier per UTC day, by querying today's - /// Security ops events. + /// emits Warning/Error/Critical OpsEvents when the FRESHEST cert in + /// each role bucket nears expiry. Older expiring siblings stay silent + /// as long as a successor is present (they're chain-continuity helpers). + /// Dedup: one event per role-bucket per tier per UTC day, by querying + /// today's Security ops events. /// public async Task CheckEmbeddedCertExpiryAsync() { @@ -58,49 +94,45 @@ namespace AutopilotMonitor.Functions.Services if (roots.Count == 0) { - if (seenToday.Add($"EmbeddedCertBundleEmpty|")) + if (seenToday.Add("EmbeddedCertBundleEmpty|")) await _opsEventService.RecordEmbeddedCertBundleEmptyAsync(); _logger.LogError("Embedded Intune root bundle is empty - agent mTLS will fail closed"); return; } - await EvaluateCertBucketAsync(roots, "Root", seenToday); - await EvaluateCertBucketAsync(intermediates, "Intermediate", seenToday); + await EmitBucketAlertIfNeededAsync(roots, "Root", seenToday); + await EmitBucketAlertIfNeededAsync(intermediates, "Intermediate", seenToday); } - private async Task EvaluateCertBucketAsync( + private async Task EmitBucketAlertIfNeededAsync( IReadOnlyList certs, string role, HashSet seenToday) { - var now = DateTime.UtcNow; - foreach (var cert in certs) + var eval = EvaluateBucket(certs, DateTime.UtcNow); + if (eval.EventType is null || eval.Freshest is null) return; + + var freshest = eval.Freshest; + var dedupKey = $"{eval.EventType}|{freshest.Thumbprint}"; + if (!seenToday.Add(dedupKey)) { - var notAfterUtc = cert.NotAfter.ToUniversalTime(); - var daysLeft = (int)Math.Floor((notAfterUtc - now).TotalDays); - var eventType = ClassifyCertExpiryTier(daysLeft); - if (eventType is null) continue; + _logger.LogDebug("Cert bucket alert already emitted today: {Key}", dedupKey); + return; + } - var dedupKey = $"{eventType}|{cert.Thumbprint}"; - if (!seenToday.Add(dedupKey)) - { - _logger.LogDebug("Cert expiry already emitted today: {Key}", dedupKey); - continue; - } - - switch (eventType) - { - case "EmbeddedCertExpired": - await _opsEventService.RecordEmbeddedCertExpiredAsync( - role, cert.Subject, cert.Thumbprint, notAfterUtc, daysLeft); - break; - case "EmbeddedCertExpiringUrgent": - await _opsEventService.RecordEmbeddedCertExpiringUrgentAsync( - role, cert.Subject, cert.Thumbprint, notAfterUtc, daysLeft); - break; - case "EmbeddedCertExpiringSoon": - await _opsEventService.RecordEmbeddedCertExpiringSoonAsync( - role, cert.Subject, cert.Thumbprint, notAfterUtc, daysLeft); - break; - } + var notAfterUtc = freshest.NotAfter.ToUniversalTime(); + switch (eval.EventType) + { + case "EmbeddedCertExpired": + await _opsEventService.RecordEmbeddedCertExpiredAsync( + role, freshest.Subject, freshest.Thumbprint, notAfterUtc, eval.DaysUntilExpiry); + break; + case "EmbeddedCertExpiringUrgent": + await _opsEventService.RecordEmbeddedCertExpiringUrgentAsync( + role, freshest.Subject, freshest.Thumbprint, notAfterUtc, eval.DaysUntilExpiry); + break; + case "EmbeddedCertExpiringSoon": + await _opsEventService.RecordEmbeddedCertExpiringSoonAsync( + role, freshest.Subject, freshest.Thumbprint, notAfterUtc, eval.DaysUntilExpiry); + break; } } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs b/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs index bfa0f1e6..0d2db78f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs @@ -89,21 +89,21 @@ namespace AutopilotMonitor.Functions.Services public Task RecordEmbeddedCertExpiringSoonAsync(string role, string subject, string thumbprint, DateTime notAfterUtc, int daysUntilExpiry) => WriteAsync(OpsEventCategory.Security, "EmbeddedCertExpiringSoon", OpsEventSeverity.Warning, - $"Embedded Intune {role.ToLowerInvariant()} '{subject}' expires in {daysUntilExpiry}d ({notAfterUtc:u}) - rotate the embedded PEM bundle before {notAfterUtc:yyyy-MM-dd}", + $"Newest embedded Intune {role.ToLowerInvariant()} '{subject}' expires in {daysUntilExpiry}d ({notAfterUtc:u}) - source a successor PEM and embed it before {notAfterUtc:yyyy-MM-dd}", null, "System.Maintenance", new { role, subject, thumbprint, notAfterUtc = notAfterUtc.ToString("u"), daysUntilExpiry }); public Task RecordEmbeddedCertExpiringUrgentAsync(string role, string subject, string thumbprint, DateTime notAfterUtc, int daysUntilExpiry) => WriteAsync(OpsEventCategory.Security, "EmbeddedCertExpiringUrgent", OpsEventSeverity.Error, - $"URGENT: embedded Intune {role.ToLowerInvariant()} '{subject}' expires in {daysUntilExpiry}d ({notAfterUtc:u}) - agent mTLS will break without bundle rotation", + $"URGENT: newest embedded Intune {role.ToLowerInvariant()} '{subject}' expires in {daysUntilExpiry}d ({notAfterUtc:u}) and no successor is in the bundle - agent mTLS will break without rotation", null, "System.Maintenance", new { role, subject, thumbprint, notAfterUtc = notAfterUtc.ToString("u"), daysUntilExpiry }); public Task RecordEmbeddedCertExpiredAsync(string role, string subject, string thumbprint, DateTime notAfterUtc, int daysUntilExpiry) => WriteAsync(OpsEventCategory.Security, "EmbeddedCertExpired", OpsEventSeverity.Critical, daysUntilExpiry < 0 - ? $"CRITICAL: embedded Intune {role.ToLowerInvariant()} '{subject}' EXPIRED {-daysUntilExpiry}d ago ({notAfterUtc:u}) - agent mTLS validation broken" - : $"CRITICAL: embedded Intune {role.ToLowerInvariant()} '{subject}' expires in {daysUntilExpiry}d ({notAfterUtc:u})", + ? $"CRITICAL: newest embedded Intune {role.ToLowerInvariant()} '{subject}' EXPIRED {-daysUntilExpiry}d ago ({notAfterUtc:u}) and no successor is embedded - agent mTLS validation broken" + : $"CRITICAL: newest embedded Intune {role.ToLowerInvariant()} '{subject}' expires in {daysUntilExpiry}d ({notAfterUtc:u}) and no successor is embedded", null, "System.Maintenance", new { role, subject, thumbprint, notAfterUtc = notAfterUtc.ToString("u"), daysUntilExpiry });