diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs index d926fac8..f5ba7022 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/AuthenticationMiddlewareTokenValidationTests.cs @@ -229,34 +229,92 @@ public class AuthenticationMiddlewareTokenValidationTests } // ── ResolveConfiguredClientIds: primary + optional AdditionalClientIds parsing ──────── + // Additional entries must be GUIDs (client IDs always are); the primary stays verbatim. + + private const string SecondId = "11111111-2222-3333-4444-555555555555"; + private const string ThirdId = "66666666-7777-8888-9999-aaaaaaaaaaaa"; + private const string FourthId = "bbbbbbbb-cccc-dddd-eeee-ffffffffffff"; [Theory] [InlineData("primary", null, new[] { "primary" })] // unset ⇒ today's behaviour [InlineData("primary", "", new[] { "primary" })] [InlineData("primary", " ", new[] { "primary" })] - [InlineData("primary", "second", new[] { "primary", "second" })] - [InlineData("primary", "second,third", new[] { "primary", "second", "third" })] - [InlineData("primary", "second; third , fourth", new[] { "primary", "second", "third", "fourth" })] - [InlineData("primary", "primary,second", new[] { "primary", "second" })] // dedupe vs primary + [InlineData("primary", SecondId, new[] { "primary", SecondId })] + [InlineData("primary", SecondId + "," + ThirdId, new[] { "primary", SecondId, ThirdId })] + [InlineData("primary", SecondId + "; " + ThirdId + " , " + FourthId, new[] { "primary", SecondId, ThirdId, FourthId })] + [InlineData(SecondId, SecondId + "," + ThirdId, new[] { SecondId, ThirdId })] // dedupe vs primary public void ResolveConfiguredClientIds_parses_trims_and_dedupes( string primary, string? additional, string[] expected) { - Assert.Equal(expected, AuthenticationMiddleware.ResolveConfiguredClientIds(primary, additional)); + Assert.Equal(expected, AuthenticationMiddleware.ResolveConfiguredClientIds(primary, additional, out var rejected)); + Assert.Empty(rejected); } [Fact] - public void ResolveConfiguredClientIds_dedupes_case_insensitively() + public void ResolveConfiguredClientIds_dedupes_case_insensitively_primary_form_wins() { + // Uppercase primary (verbatim) vs the same id lowercase in AdditionalClientIds: + // one entry survives, in the primary's original casing. + var primaryUpper = SecondId.ToUpperInvariant(); + Assert.Equal( - new[] { "Primary", "second" }, - AuthenticationMiddleware.ResolveConfiguredClientIds("Primary", "primary, second")); + new[] { primaryUpper, ThirdId }, + AuthenticationMiddleware.ResolveConfiguredClientIds(primaryUpper, $"{SecondId}, {ThirdId}", out var rejected)); + Assert.Empty(rejected); } [Fact] public void ResolveConfiguredClientIds_null_primary_falls_back_to_additional_only() { Assert.Equal( - new[] { "second" }, - AuthenticationMiddleware.ResolveConfiguredClientIds(null, "second")); + new[] { SecondId }, + AuthenticationMiddleware.ResolveConfiguredClientIds(null, SecondId, out var rejected)); + Assert.Empty(rejected); + } + + [Fact] + public void ResolveConfiguredClientIds_drops_and_reports_malformed_additional_entries() + { + // A typo'd/garbage entry could never match a token's aud claim — it is dropped + // (fail-closed) and surfaced via the out param so the middleware can log it. + var ids = AuthenticationMiddleware.ResolveConfiguredClientIds( + "primary", $"not-a-guid,{SecondId};11111111-2222", out var rejected); + + Assert.Equal(new[] { "primary", SecondId }, ids); + Assert.Equal(new[] { "not-a-guid", "11111111-2222" }, rejected); + } + + [Fact] + public void ResolveConfiguredClientIds_malformed_primary_is_kept_verbatim_not_rejected() + { + // The primary's handling is deliberately unchanged — validation covers ONLY the + // AdditionalClientIds seam. A broken primary breaks every login loudly on its own. + var ids = AuthenticationMiddleware.ResolveConfiguredClientIds("oops", SecondId, out var rejected); + + Assert.Equal(new[] { "oops", SecondId }, ids); + Assert.Empty(rejected); + } + + [Fact] + public void ResolveConfiguredClientIds_normalizes_additional_entries_to_canonical_guid_form() + { + // Entra aud claims are lowercase dashed GUIDs and audience comparison is ordinal — + // brace/uppercase variants an operator might paste must still match after resolve. + var ids = AuthenticationMiddleware.ResolveConfiguredClientIds( + "primary", $"{{{SecondId}}};{ThirdId.ToUpperInvariant()}", out var rejected); + + Assert.Equal(new[] { "primary", SecondId, ThirdId }, ids); + Assert.Empty(rejected); + } + + // ── TruncateForLog: rejected config entries are never logged in full ────────────────── + + [Theory] + [InlineData("short", "short")] + [InlineData("12345678", "12345678")] + [InlineData("not-a-guid-but-long", "not-a-gu…(19 chars)")] + public void TruncateForLog_caps_entry_at_eight_chars_plus_length(string entry, string expected) + { + Assert.Equal(expected, AuthenticationMiddleware.TruncateForLog(entry)); } } diff --git a/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs b/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs index aeeedaa7..d7790fcb 100644 --- a/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs +++ b/src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs @@ -164,8 +164,10 @@ public class AuthenticationMiddleware : IFunctionsWorkerMiddleware // seam that lets the API trust a SECOND app registration during a tenant-move window // (see tasks/migration-cross-tenant-runbook.md). Unset today ⇒ exactly the primary id // ⇒ zero behaviour change. + var additionalClientIdsRaw = _configuration["EntraId:AdditionalClientIds"]; var clientIds = ResolveConfiguredClientIds( - _configuration["EntraId:ClientId"], _configuration["EntraId:AdditionalClientIds"]); + _configuration["EntraId:ClientId"], additionalClientIdsRaw, out var rejectedAdditionalEntries); + AuditClientIdTrustSet(additionalClientIdsRaw, clientIds, rejectedAdditionalEntries); var validationParameters = BuildTokenValidationParameters( openIdConfig.SigningKeys, clientIds); @@ -321,24 +323,94 @@ public class AuthenticationMiddleware : IFunctionsWorkerMiddleware /// /// Resolves the configured accepted app (client) IDs: the primary EntraId:ClientId plus an - /// optional comma/semicolon/whitespace-separated EntraId:AdditionalClientIds. Trimmed, - /// empties dropped, de-duplicated case-insensitively (primary first). When AdditionalClientIds is - /// unset the result is exactly the primary id — today's behaviour, zero change — so adding a - /// second app registration's client ID to that setting is the ONLY step needed to trust a second - /// app during a tenant-move window (tasks/migration-cross-tenant-runbook.md). + /// optional comma/semicolon/whitespace-separated EntraId:AdditionalClientIds. Additional + /// entries must be GUIDs (a client ID is always a GUID); non-GUID entries are dropped — + /// fail-closed, they could never match a token's aud anyway — and reported via + /// so can make + /// the misconfiguration operator-visible. Accepted entries are normalized to the canonical + /// lowercase dashed GUID form (Entra aud claims are lowercase; audience comparison is + /// ordinal), so brace/case/no-dash variants an operator might paste still match. The result is + /// de-duplicated case-insensitively (primary first, verbatim — its handling is unchanged). When + /// AdditionalClientIds is unset the result is exactly the primary id — today's behaviour, zero + /// change — so adding a second app registration's client ID to that setting is the ONLY step + /// needed to trust a second app during a tenant-move window + /// (tasks/migration-cross-tenant-runbook.md). /// - internal static string[] ResolveConfiguredClientIds(string? primaryClientId, string? additionalClientIds) + internal static string[] ResolveConfiguredClientIds( + string? primaryClientId, string? additionalClientIds, out string[] rejectedAdditionalEntries) { var ids = new List { primaryClientId }; + var rejected = new List(); if (!string.IsNullOrWhiteSpace(additionalClientIds)) { - ids.AddRange(additionalClientIds.Split( - new[] { ',', ';', ' ', '\t', '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries)); + foreach (var entry in additionalClientIds.Split( + new[] { ',', ';', ' ', '\t', '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries)) + { + if (Guid.TryParse(entry.Trim(), out var clientId)) + { + ids.Add(clientId.ToString("D")); + } + else + { + rejected.Add(entry.Trim()); + } + } } + rejectedAdditionalEntries = rejected.ToArray(); return ids .Where(id => !string.IsNullOrWhiteSpace(id)) .Select(id => id!.Trim()) .Distinct(StringComparer.OrdinalIgnoreCase) .ToArray(); } + + /// + /// Sentinel-free "already audited" marker: the last EntraId:AdditionalClientIds raw value + /// this instance logged about. Initial null intentionally equals the unset-config case — + /// an unset seam has nothing to report, so skipping it is correct, and the first non-null value + /// (or any change) triggers a fresh audit. + /// + private string? _auditedAdditionalClientIds; + + /// + /// Operator-visible audit of the audience trust set (daily-review follow-up, 2026-07-14): + /// expanding the set of trusted app registrations via EntraId:AdditionalClientIds is + /// security-relevant and used to be completely silent — a typo'd GUID was indistinguishable + /// from "the new app registration doesn't work" during a tenant-move window. Logs once per + /// distinct config value per instance, at Warning because nothing below Warning reaches + /// App Insights from the worker. Rejected entries are truncated before logging in case an + /// operator accidentally pastes a secret into the setting. + /// + private void AuditClientIdTrustSet( + string? rawAdditionalClientIds, string[] clientIds, string[] rejectedAdditionalEntries) + { + if (string.Equals(_auditedAdditionalClientIds, rawAdditionalClientIds, StringComparison.Ordinal)) + { + return; + } + // Benign race on concurrent first requests: at worst a duplicate audit line. + _auditedAdditionalClientIds = rawAdditionalClientIds; + + if (rejectedAdditionalEntries.Length > 0) + { + _logger.LogWarning( + "[Auth Middleware] EntraId:AdditionalClientIds contains {Count} malformed (non-GUID) entries — ignored: {Entries}", + rejectedAdditionalEntries.Length, + string.Join(", ", rejectedAdditionalEntries.Select(TruncateForLog))); + } + if (clientIds.Length > 1) + { + _logger.LogWarning( + "[Auth Middleware] Token audience trust set expanded to {Count} client IDs via EntraId:AdditionalClientIds: {ClientIds}", + clientIds.Length, + string.Join(", ", clientIds)); + } + } + + /// + /// Truncates a rejected config entry for logging: enough to identify the typo, never the full + /// value (an operator could accidentally paste a client secret into the setting). + /// + internal static string TruncateForLog(string entry) => + entry.Length <= 8 ? entry : $"{entry.Substring(0, 8)}…({entry.Length} chars)"; }