diff --git a/docs/backend/lifecycle-manifests-and-session-scope.md b/docs/backend/lifecycle-manifests-and-session-scope.md index 9eb7190a..a731b908 100644 --- a/docs/backend/lifecycle-manifests-and-session-scope.md +++ b/docs/backend/lifecycle-manifests-and-session-scope.md @@ -39,7 +39,9 @@ Every table in `Constants.TableNames` must belong to at least one deletion lifec - Unknown session → `TargetTenantId`, leaving each endpoint's own not-found semantics (404 / empty 200 / 400) in charge. - Delegated (MSP) callers deliberately get NO fallback — extending it to `AllowedTenantIds` is a pending user decision. -The lookup row is written at session registration (fail-soft), deleted by the cascade one step before the tombstone, and rebuildable from Sessions (not in the critical-backup set). The former lazy triggers (`events.Count == 0` → scan) are gone: empty-but-owned results no longer pay a table scan, and endpoints resolve upfront. +The lookup row is **claimed at registration, first-writer-wins** (`ClaimSessionTenantLookupAsync`, create-only `AddEntity` BEFORE any Sessions/SessionsIndex write): on 409 the existing owner is read back — same tenant ⇒ normal re-registration, different tenant ⇒ `SessionTenantConflictException` ⇒ `409 Conflict` with `ErrorCode=session_owner_mismatch` (owning tenant not disclosed) plus a `SessionTenantConflict` ops event, and nothing is written. The claim is deliberately NOT fail-soft. Before this, the row was an unconditional last-writer-wins upsert after the session write, so any tenant's device (self-service onboarding) that knew a victim session GUID (portal deep links, Teams/Slack notifications, progress-portal URLs) could re-point the mapping at its own tenant and have every no-`tenantId` Global Admin/Reader read — and GA writes such as the `globaladmin` annotation lane — silently served from a forged session. Session ids are 122-bit random, so a genuine cross-tenant collision does not occur; a conflict is an attack or an agent bug and fails loudly. The legacy self-heal write is Add-only too (an existing claim is the authority), and `scripts/Migration/Backfill-SessionTenantLookup.ps1` closes the pre-table window once (idempotent, Add-only) so no legacy session id is claimable. The row is deleted by the cascade one step before the tombstone (no tombstone is kept — a freed id gives a foreign tenant nothing a fresh GUID would not) and is rebuildable from Sessions (not in the critical-backup set). The former lazy triggers (`events.Count == 0` → scan) are gone: empty-but-owned results no longer pay a table scan, and endpoints resolve upfront. + +`requireGlobalAdmin: true` additionally corroborates the resolved tenant against its Sessions row (one point-read): a mapping whose session does not exist in the resolved tenant keeps `TargetTenantId`, so a stale or poisoned row can never steer a GA write — silently, without any operator confirmation step (a "confirm tenant GUID" dialog was rejected as unusable). Read paths keep the single point-read; their own session read is the corroboration (404). Related enforcement added with the migration: `RecomputeTriggerGate` — `?reanalyze=true` and `?rescan=true` on the MemberRead session routes are actions (delete + rewrite stored results) and now 403 for Viewer / Global Reader / cross-tenant tenant roles (Global Admin or own-tenant Admin/Operator only). diff --git a/docs/log.md b/docs/log.md index 94009899..3815041e 100644 --- a/docs/log.md +++ b/docs/log.md @@ -1,5 +1,9 @@ # Log +## 2026-08-30 (15) + +* **Update**: `backend/lifecycle-manifests-and-session-scope.md` — SessionTenantLookup is now claimed first-writer-wins at registration (create-only Add BEFORE any session write; foreign owner ⇒ 409 `session_owner_mismatch` + `SessionTenantConflict` ops event, nothing written; not fail-soft). Closes the poisoning path where any tenant's device could re-point a victim sessionId's mapping at its own tenant and hijack every no-tenantId Global Admin/Reader read and GA write (annotation lane). `ResolveSessionScopeAsync(requireGlobalAdmin: true)` corroborates the resolved tenant against its Sessions row (silent — no operator confirmation). Self-heal write is Add-only; `scripts/Migration/Backfill-SessionTenantLookup.ps1` closes the legacy window once. No tombstone on deletion (deliberate: unbounded growth for no gain). + ## 2026-08-30 (14) * **Update**: `agent/decision-engine.md` — new invariant: every payload-derived string stored into `DecisionState` is bounded to 256 chars by `FactStringBounds` before any dedupe comparison (RealmJoin package id/version/scope, failed app ids, `HelloOutcome`, `ImeMatchedPatternId`, `EspAdvisoryFailureCategory`, RJ product version/channel). Backend `ReducerVerifier` now replays decode-fold-discard instead of materialising all decoded signals, and `ISignalRepository.QueryBySessionAsync` carries a cumulative `PayloadJson` budget (`SignalQueryLimits`, 32 M chars) reflected in the `truncated` flag of `sessions/{id}/signals` and `sessions/{id}/reducer-verification`. diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/SessionScopeResolutionTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/SessionScopeResolutionTests.cs index af8cc9d5..e5d34cc6 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/SessionScopeResolutionTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/SessionScopeResolutionTests.cs @@ -8,6 +8,7 @@ using AutopilotMonitor.Functions.Helpers; using AutopilotMonitor.Functions.Services; using AutopilotMonitor.Shared; using AutopilotMonitor.Shared.DataAccess; +using AutopilotMonitor.Shared.Models; using Microsoft.Extensions.Logging.Abstractions; using Moq; using Xunit; @@ -96,10 +97,12 @@ public class SessionScopeResolutionTests } [Fact] - public async Task RequireGlobalAdmin_StillResolves_ForGlobalAdmin() + public async Task RequireGlobalAdmin_StillResolves_ForGlobalAdmin_WhenTheSessionRowCorroboratesTheLookup() { var repo = new Mock(); repo.Setup(r => r.ResolveSessionTenantIdAsync(SessionId)).ReturnsAsync(OtherTenant); + repo.Setup(r => r.GetSessionAsync(OtherTenant, SessionId)) + .ReturnsAsync(new SessionSummary { SessionId = SessionId, TenantId = OtherTenant }); var ctx = new RequestContext { TenantId = HomeTenant, TargetTenantId = HomeTenant, IsGlobalAdmin = true }; var result = await ctx.ResolveSessionScopeAsync(repo.Object, SessionId, requireGlobalAdmin: true); @@ -107,6 +110,38 @@ public class SessionScopeResolutionTests Assert.Equal(OtherTenant, result); } + [Fact] + public async Task RequireGlobalAdmin_KeepsTargetTenant_WhenTheLookupPointsAtATenantWithoutTheSessionRow() + { + // A lookup row whose tenant does not hold the Sessions row (poisoned or stale mapping) + // must never steer a GA WRITE into that tenant — silently keep the target tenant so the + // endpoint's own not-found handling answers; no operator confirmation dialog. + var repo = new Mock(); + repo.Setup(r => r.ResolveSessionTenantIdAsync(SessionId)).ReturnsAsync(OtherTenant); + repo.Setup(r => r.GetSessionAsync(OtherTenant, SessionId)).ReturnsAsync((SessionSummary?)null); + var ctx = new RequestContext { TenantId = HomeTenant, TargetTenantId = HomeTenant, IsGlobalAdmin = true }; + + var result = await ctx.ResolveSessionScopeAsync(repo.Object, SessionId, requireGlobalAdmin: true); + + Assert.Equal(HomeTenant, result); + } + + [Fact] + public async Task ReadPath_DoesNotCorroborate_TheLookupWithASessionRead() + { + // Reads keep the single point-read contract: the endpoint's own session read is the + // corroboration (404 when the row is missing in the resolved tenant). + var repo = new Mock(MockBehavior.Strict); + repo.Setup(r => r.ResolveSessionTenantIdAsync(SessionId)).ReturnsAsync(OtherTenant); + var ctx = new RequestContext { TenantId = HomeTenant, TargetTenantId = HomeTenant, IsGlobalAdmin = true }; + + var result = await ctx.ResolveSessionScopeAsync(repo.Object, SessionId); + + Assert.Equal(OtherTenant, result); + repo.VerifyAll(); + repo.VerifyNoOtherCalls(); + } + [Fact] public async Task DelegatedCaller_GetsNoFallback_ByDesign() { @@ -151,14 +186,31 @@ public class SessionScopeResolutionTests var result = await harness.Sut.ResolveSessionTenantIdAsync(SessionId); Assert.Equal(OtherTenant, result); - // Self-heal: the lookup row is written so the next resolve is a point-read. - harness.Lookup.Verify(t => t.UpsertEntityAsync( + // Self-heal: the lookup row is written ADD-ONLY so the next resolve is a point-read and + // a claim that landed meanwhile is never overwritten. + harness.Lookup.Verify(t => t.AddEntityAsync( It.Is(e => e.PartitionKey == SessionId && e.RowKey == "tenant" && e.GetString("TenantId") == OtherTenant), - It.IsAny(), It.IsAny()), Times.Once); + harness.Lookup.Verify(t => t.UpsertEntityAsync( + It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task SelfHeal_SwallowsAConcurrentClaim_AndStillReturnsTheScannedTenant() + { + var indexRow = new TableEntity(OtherTenant, "0000000000000000001_" + SessionId) { ["SessionId"] = SessionId }; + var harness = new ResolveHarness(lookupRow: null, indexRows: new[] { indexRow }); + harness.Lookup + .Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new RequestFailedException(409, "EntityAlreadyExists")); + + var result = await harness.Sut.ResolveSessionTenantIdAsync(SessionId); + + Assert.Equal(OtherTenant, result); } [Fact] @@ -169,8 +221,8 @@ public class SessionScopeResolutionTests var result = await harness.Sut.ResolveSessionTenantIdAsync(SessionId); Assert.Null(result); - harness.Lookup.Verify(t => t.UpsertEntityAsync( - It.IsAny(), It.IsAny(), It.IsAny()), + harness.Lookup.Verify(t => t.AddEntityAsync( + It.IsAny(), It.IsAny()), Times.Never); } @@ -246,7 +298,7 @@ public class SessionScopeResolutionTests .ThrowsAsync(new RequestFailedException(404, "Not Found")); } Lookup - .Setup(t => t.UpsertEntityAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) .ReturnsAsync(Mock.Of()); var page = Page.FromValues(indexRows ?? Array.Empty(), null, Mock.Of()); diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionReregistrationPreserveTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionReregistrationPreserveTests.cs index 1b43b86c..c94e0664 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionReregistrationPreserveTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionReregistrationPreserveTests.cs @@ -386,6 +386,7 @@ public class StoreSessionReregistrationPreserveTests private sealed class Harness { public Mock Sessions { get; } + public Mock Lookup { get; } = new(); public TableStorageService Sut { get; } public TableEntity? Written { get; private set; } @@ -423,8 +424,13 @@ public class StoreSessionReregistrationPreserveTests return Task.FromResult(new Mock().Object); }); + // Ownership claim (first-writer-wins lookup) — Add succeeds: this tenant owns the id. + Lookup.Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(new Mock().Object); + var mockServiceClient = new Mock(); mockServiceClient.Setup(s => s.GetTableClient(Constants.TableNames.Sessions)).Returns(Sessions.Object); + mockServiceClient.Setup(s => s.GetTableClient(Constants.TableNames.SessionTenantLookup)).Returns(Lookup.Object); // Events / SessionsIndex intentionally unconfigured → null client → SUT swallows. Sut = new TableStorageService(mockServiceClient.Object, NullLogger.Instance); diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionTenantLookupClaimTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionTenantLookupClaimTests.cs new file mode 100644 index 00000000..d31857a5 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/StoreSessionTenantLookupClaimTests.cs @@ -0,0 +1,144 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Azure; +using Azure.Data.Tables; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// SessionTenantLookup ownership claim in . +/// The lookup (PK = client-supplied sessionId → owning tenantId) is the authority behind every +/// global-scope cross-tenant session resolve. It used to be an unconditional last-writer-wins +/// upsert AFTER the Sessions row, so any tenant's device could re-point a victim sessionId at +/// its own tenant. Now it is a create-only claim BEFORE any session write: a foreign owner +/// refuses the registration (409, nothing written), the same owner re-registers as before. +/// +public class StoreSessionTenantLookupClaimTests +{ + private const string TenantA = "11111111-1111-1111-1111-111111111111"; + private const string TenantB = "44444444-4444-4444-4444-444444444444"; + private const string SessionId = "22222222-2222-2222-2222-222222222222"; + + private static SessionRegistration Registration(string tenantId) => new() + { + TenantId = tenantId, + SessionId = SessionId, + StartedAt = new DateTime(2026, 1, 1, 8, 0, 0, DateTimeKind.Utc), + SerialNumber = "SN-1", + }; + + [Fact] + public async Task FreshSessionId_ClaimsTheLookup_BeforeWritingTheSessionRow() + { + var harness = new Harness(existingOwner: null); + + var ok = await harness.Sut.StoreSessionAsync(Registration(TenantA)); + + Assert.True(ok); + Assert.Equal(new[] { "lookup-add", "session-write" }, harness.Order); + Assert.Equal(TenantA, harness.ClaimedTenant); + harness.Lookup.Verify(t => t.UpsertEntityAsync( + It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task SameTenantReregistration_KeepsTheExistingClaim_AndStoresNormally() + { + var harness = new Harness(existingOwner: TenantA.ToUpperInvariant()); + + var ok = await harness.Sut.StoreSessionAsync(Registration(TenantA)); + + Assert.True(ok); + Assert.Contains("session-write", harness.Order); + } + + [Fact] + public async Task ForeignTenant_IsRefused_AndNothingIsWritten() + { + // Attacker in tenant B registers the victim's (tenant A) sessionId. + var harness = new Harness(existingOwner: TenantA); + + var ex = await Assert.ThrowsAsync( + () => harness.Sut.StoreSessionAsync(Registration(TenantB))); + + Assert.Equal(SessionId, ex.SessionId); + Assert.Equal(TenantB, ex.RequestedTenantId); + Assert.Equal(TenantA, ex.OwningTenantId); + // The mapping still points at A, and B never got a Sessions row for the id. + Assert.DoesNotContain("session-write", harness.Order); + harness.Lookup.Verify(t => t.UpsertEntityAsync( + It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + harness.Lookup.Verify(t => t.UpdateEntityAsync( + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public async Task ClaimStorageFailure_IsNotFailSoft_TheSessionIsNotStored() + { + var harness = new Harness(existingOwner: null); + harness.Lookup.Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new RequestFailedException(503, "ServerBusy")); + + var ok = await harness.Sut.StoreSessionAsync(Registration(TenantA)); + + Assert.False(ok); + Assert.DoesNotContain("session-write", harness.Order); + } + + private sealed class Harness + { + public Mock Sessions { get; } = new(); + public Mock Lookup { get; } = new(); + public List Order { get; } = new(); + public string? ClaimedTenant { get; private set; } + public TableStorageService Sut { get; } + + public Harness(string? existingOwner) + { + // Sessions: no existing row; Add captures the write order. + Sessions.Setup(t => t.GetEntityAsync( + It.IsAny(), SessionId, It.IsAny>(), It.IsAny())) + .ThrowsAsync(new RequestFailedException(404, "ResourceNotFound")); + Sessions.Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) + .Returns((_, _) => + { + Order.Add("session-write"); + return Task.FromResult(new Mock().Object); + }); + + if (existingOwner is null) + { + Lookup.Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) + .Returns((e, _) => + { + Order.Add("lookup-add"); + ClaimedTenant = e.GetString("TenantId"); + return Task.FromResult(new Mock().Object); + }); + } + else + { + Lookup.Setup(t => t.AddEntityAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new RequestFailedException(409, "EntityAlreadyExists")); + Lookup.Setup(t => t.GetEntityAsync( + SessionId, "tenant", It.IsAny>(), It.IsAny())) + .ReturnsAsync(Response.FromValue( + new TableEntity(SessionId, "tenant") { ["TenantId"] = existingOwner }, Mock.Of())); + } + + var serviceClient = new Mock(); + serviceClient.Setup(s => s.GetTableClient(Constants.TableNames.Sessions)).Returns(Sessions.Object); + serviceClient.Setup(s => s.GetTableClient(Constants.TableNames.SessionTenantLookup)).Returns(Lookup.Object); + // Events / SessionsIndex intentionally unconfigured → null client → SUT swallows. + Sut = new TableStorageService(serviceClient.Object, NullLogger.Instance); + } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/RegisterSessionFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/RegisterSessionFunction.cs index 57018e50..46480e0f 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/RegisterSessionFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Sessions/RegisterSessionFunction.cs @@ -38,6 +38,7 @@ namespace AutopilotMonitor.Functions.Functions.Sessions private readonly SessionDeletionGuard _deletionGuard; private readonly AdminConfigurationService _adminConfigService; private readonly SessionOwnerBindingObserver _ownerBinding; + private readonly OpsEventService _opsEvents; public RegisterSessionFunction( ILogger logger, @@ -54,7 +55,8 @@ namespace AutopilotMonitor.Functions.Functions.Sessions BootstrapSessionService bootstrapSessionService, WebhookNotificationService webhookNotificationService, SessionDeletionGuard deletionGuard, - SessionOwnerBindingObserver ownerBinding) + SessionOwnerBindingObserver ownerBinding, + OpsEventService opsEvents) { _logger = logger; _sessionRepo = sessionRepo; @@ -71,6 +73,7 @@ namespace AutopilotMonitor.Functions.Functions.Sessions _webhookNotificationService = webhookNotificationService; _deletionGuard = deletionGuard; _ownerBinding = ownerBinding; + _opsEvents = opsEvents; } [Function("RegisterSession")] @@ -194,8 +197,26 @@ namespace AutopilotMonitor.Functions.Functions.Sessions var ownerDecision = _ownerBinding.Observe( req, registration.TenantId, registration.SessionId, guardSessionRow, validation, "agent/register-session"); - // Store session in Azure Table Storage - var stored = await _sessionRepo.StoreSessionAsync(registration, ownerDecision.OwnerToStamp); + // Store session in Azure Table Storage. The sessionId → tenantId lookup claim inside is + // first-writer-wins: a sessionId already owned by another tenant is refused with 409 so + // no tenant can poison the cross-tenant resolver (SessionTenantLookup) that Global + // Admin/Reader session reads and GA writes trust. + bool stored; + try + { + stored = await _sessionRepo.StoreSessionAsync(registration, ownerDecision.OwnerToStamp); + } + catch (SessionTenantConflictException conflict) + { + _logger.LogWarning( + "RegisterSession refused: session {SessionId} is owned by another tenant — requested tenant={TenantId} thumbprint={Thumbprint}", + registration.SessionId, registration.TenantId, validation.CertificateThumbprint); + _ = _opsEvents.RecordSessionTenantConflictAsync( + registration.TenantId, registration.SessionId, conflict.OwningTenantId, + validation.CertificateThumbprint, registration.AgentVersion, "agent/register-session") + .ContinueWith(t => _logger.LogWarning(t.Exception?.InnerException, "Fire-and-forget RecordSessionTenantConflictAsync failed"), TaskContinuationOptions.OnlyOnFaulted); + return new RegisterSessionOutput { HttpResponse = await WriteSessionTenantConflictAsync(req) }; + } if (!stored) { @@ -440,6 +461,25 @@ namespace AutopilotMonitor.Functions.Functions.Sessions registration.StartedAt = sanitized; } + /// + /// 409 for a sessionId already owned by another tenant. Carries + /// — the agent's correct reaction + /// is the same as for an owner-binding refusal: rotate the SessionId and register afresh. + /// The owning tenant is deliberately NOT disclosed to the caller. + /// + private static async Task WriteSessionTenantConflictAsync(HttpRequestData req) + { + var response = req.CreateResponse(HttpStatusCode.Conflict); + await response.WriteAsJsonAsync(new RegisterSessionResponse + { + Success = false, + Message = "Session id is already registered to another tenant; register with a new session id.", + RegisteredAt = DateTime.UtcNow, + ErrorCode = Constants.AgentErrorCodes.SessionOwnerMismatch, + }); + return response; + } + private static async Task WriteSessionLockedAsync(HttpRequestData req, SessionDeletionLockedException locked) { var response = req.CreateResponse(HttpStatusCode.Gone); diff --git a/src/Backend/AutopilotMonitor.Functions/Helpers/RequestContext.cs b/src/Backend/AutopilotMonitor.Functions/Helpers/RequestContext.cs index 9274df6d..d722a2e9 100644 --- a/src/Backend/AutopilotMonitor.Functions/Helpers/RequestContext.cs +++ b/src/Backend/AutopilotMonitor.Functions/Helpers/RequestContext.cs @@ -176,10 +176,18 @@ public static class RequestContextExtensions return ctx.TargetTenantId; var resolvedTenantId = await sessionRepo.ResolveSessionTenantIdAsync(sessionId); - if (resolvedTenantId != null && !string.Equals(resolvedTenantId, ctx.TargetTenantId, StringComparison.OrdinalIgnoreCase)) - return resolvedTenantId; + if (resolvedTenantId == null || string.Equals(resolvedTenantId, ctx.TargetTenantId, StringComparison.OrdinalIgnoreCase)) + return ctx.TargetTenantId; - return ctx.TargetTenantId; + // WRITE paths corroborate the lookup against the Sessions row of the resolved tenant: + // the lookup is first-writer-wins at registration (StoreSessionAsync), but a mapping + // whose session does not exist must never steer a Global Admin write into that tenant. + // Silent for the operator — a miss just keeps the target tenant, so the endpoint's own + // not-found handling answers. + if (requireGlobalAdmin && await sessionRepo.GetSessionAsync(resolvedTenantId, sessionId) == null) + return ctx.TargetTenantId; + + return resolvedTenantId; } /// Gets the correlation ID for this request (set by CorrelationIdMiddleware). diff --git a/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs b/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs index 3e23de7d..447f837b 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/OpsEventService.cs @@ -298,6 +298,20 @@ namespace AutopilotMonitor.Functions.Services /// re-enroll-without-wipe shape (same device, new certificate identity, old session id); /// serialMatch=false is a foreign device naming somebody else's session. /// + /// + /// A device/bootstrap-authenticated registration named a sessionId whose SessionTenantLookup + /// row belongs to ANOTHER tenant (first-writer-wins claim in StoreSessionAsync). + /// Session ids are random GUIDs — this is a forged registration or an agent bug, never a + /// legitimate collision. The registration was refused with 409. Recorded against the + /// requesting tenant; the owning tenant is only in the details. + /// + public Task RecordSessionTenantConflictAsync( + string requestedTenantId, string sessionId, string owningTenantId, string? certificateThumbprint, string? agentVersion, string endpoint) + => WriteAsync(OpsEventCategory.Security, "SessionTenantConflict", OpsEventSeverity.Warning, + $"Registration refused on {endpoint}: session {sessionId} is owned by another tenant (409)", + requestedTenantId, "System.SessionTenantLookup", + new { sessionId, owningTenantId, certificateThumbprint, agentVersion, endpoint }); + public Task RecordSessionOwnerMismatchAsync( string tenantId, string sessionId, string outcome, string callerKind, string ownerKind, bool serialMatch, string? agentVersion, string endpoint) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/SessionTenantConflictException.cs b/src/Backend/AutopilotMonitor.Functions/Services/SessionTenantConflictException.cs new file mode 100644 index 00000000..85cee2c9 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions/Services/SessionTenantConflictException.cs @@ -0,0 +1,28 @@ +using System; + +namespace AutopilotMonitor.Functions.Services +{ + /// + /// Thrown by TableStorageService.StoreSessionAsync when a registration names a sessionId + /// whose SessionTenantLookup row is already owned by a DIFFERENT tenant. The lookup is the + /// authority every global-scope cross-tenant session resolve trusts, so its write is + /// first-writer-wins: a tenant can never overwrite (poison) another tenant's mapping. + /// Session ids are 122-bit random — a genuine cross-tenant collision does not happen, so a + /// conflict is either a forged registration or an agent bug, and both must fail loudly + /// (409 Conflict) instead of silently retargeting the platform-operator data plane. + /// + public class SessionTenantConflictException : InvalidOperationException + { + public string SessionId { get; } + public string RequestedTenantId { get; } + public string OwningTenantId { get; } + + public SessionTenantConflictException(string sessionId, string requestedTenantId, string owningTenantId) + : base($"Session {sessionId} is already registered to another tenant; registration for tenant {requestedTenantId} refused") + { + SessionId = sessionId; + RequestedTenantId = requestedTenantId; + OwningTenantId = owningTenantId; + } + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs index 3e86e8c4..4443df06 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Sessions.cs @@ -459,6 +459,14 @@ namespace AutopilotMonitor.Functions.Services try { + // Ownership claim FIRST, before any Sessions/SessionsIndex write: the sessionId → + // tenantId lookup is first-writer-wins. A registration whose sessionId is already + // mapped to another tenant is refused here (SessionTenantConflictException → 409), + // so no tenant can poison the cross-tenant resolver and no foreign-owned sessionId + // ever gets a row in the caller's tenant. Not fail-soft: without the claim the + // session must not exist. + await ClaimSessionTenantLookupAsync(registration.SessionId, registration.TenantId); + var tableClient = _tableServiceClient.GetTableClient(Constants.TableNames.Sessions); // If the agent restarts with the same session ID, preserve timeline/progress fields @@ -818,13 +826,14 @@ namespace AutopilotMonitor.Functions.Services // Dual-write: upsert into SessionsIndex for time-sorted listing await UpsertSessionIndexAsync(entity, startedAt); - // Dual-write: sessionId → tenantId point-read lookup (fail-soft; legacy - // sessions heal lazily in ResolveSessionTenantIdAsync). - await UpsertSessionTenantLookupAsync(registration.SessionId, registration.TenantId); - _logger.LogInformation($"Stored session {registration.SessionId}"); return true; } + catch (SessionTenantConflictException) + { + // Caller maps this to 409 — never swallow it into a generic "store failed". + throw; + } catch (Exception ex) { _logger.LogError(ex, $"Failed to store session {registration.SessionId}"); @@ -1634,8 +1643,9 @@ namespace AutopilotMonitor.Functions.Services var scanned = await ScanSessionsIndexForTenantIdAsync(sessionId); if (scanned != null) { - // Self-heal: next resolve for this legacy session is a point-read. - await UpsertSessionTenantLookupAsync(sessionId, scanned); + // Self-heal: next resolve for this legacy session is a point-read. Add-only — + // a row that landed meanwhile (a registration claim) is the authority, never ours. + await TryAddSessionTenantLookupAsync(sessionId, scanned); } return scanned; } @@ -1664,24 +1674,75 @@ namespace AutopilotMonitor.Functions.Services } } + private static TableEntity NewSessionTenantLookupRow(string sessionId, string tenantId) + => new TableEntity(sessionId, SessionTenantLookupRowKey) { ["TenantId"] = tenantId }; + /// - /// Writes the sessionId → tenantId lookup row (fail-soft: resolution still works via - /// the scan fallback when this write is lost). + /// Claims the sessionId → tenantId lookup row for (first-writer-wins). + /// Create-only Add; on 409 the existing owner is read and must equal the caller's tenant + /// (normal re-registration), otherwise . + /// NOT fail-soft: a lost claim means a session without a trustworthy owner mapping, so + /// storage errors propagate and the registration fails as a whole. /// - private async Task UpsertSessionTenantLookupAsync(string sessionId, string tenantId) + private async Task ClaimSessionTenantLookupAsync(string sessionId, string tenantId) + { + var lookupClient = _tableServiceClient.GetTableClient(Constants.TableNames.SessionTenantLookup); + + // Two attempts cover the only benign race: the row is deleted (retention cascade) + // between our 409 and the read-back — the second Add then simply succeeds. + for (var attempt = 0; attempt < 2; attempt++) + { + try + { + await lookupClient.AddEntityAsync(NewSessionTenantLookupRow(sessionId, tenantId)); + return; + } + catch (RequestFailedException ex) when (ex.Status == 409) + { + // Existing claim — re-registration by the owner, or a foreign tenant. + } + + string? owner; + try + { + var existing = await lookupClient.GetEntityAsync(sessionId, SessionTenantLookupRowKey); + owner = existing.Value.GetString("TenantId"); + } + catch (RequestFailedException ex) when (ex.Status == 404) + { + continue; // deleted in between — retry the Add + } + + if (string.Equals(owner, tenantId, StringComparison.OrdinalIgnoreCase)) + return; + + _logger.LogWarning( + "SessionTenantLookup claim refused: session {SessionId} is owned by tenant {OwningTenantId}, registration from tenant {RequestedTenantId} rejected", + sessionId, owner, tenantId); + throw new SessionTenantConflictException(sessionId, tenantId, owner ?? string.Empty); + } + + throw new InvalidOperationException($"SessionTenantLookup claim for session {sessionId} did not converge"); + } + + /// + /// Self-heal write for legacy sessions (pre-lookup-table): Add-only, so it can never + /// overwrite an existing claim; fail-soft because resolution still works via the scan. + /// + private async Task TryAddSessionTenantLookupAsync(string sessionId, string tenantId) { try { var lookupClient = _tableServiceClient.GetTableClient(Constants.TableNames.SessionTenantLookup); - var row = new TableEntity(sessionId, SessionTenantLookupRowKey) - { - ["TenantId"] = tenantId - }; - await lookupClient.UpsertEntityAsync(row); + await lookupClient.AddEntityAsync(NewSessionTenantLookupRow(sessionId, tenantId)); + } + catch (RequestFailedException ex) when (ex.Status == 409) + { + // Already claimed — the existing row is the authority. } catch (Exception ex) { - _logger.LogWarning(ex, "Failed to upsert SessionTenantLookup for {SessionId}", sessionId); + _logger.LogWarning(ex, "Failed to add SessionTenantLookup for {SessionId}", sessionId); } } diff --git a/src/Shared/AutopilotMonitor.Shared/Constants.cs b/src/Shared/AutopilotMonitor.Shared/Constants.cs index 0d58643c..a802f206 100644 --- a/src/Shared/AutopilotMonitor.Shared/Constants.cs +++ b/src/Shared/AutopilotMonitor.Shared/Constants.cs @@ -259,7 +259,9 @@ namespace AutopilotMonitor.Shared /// SESSION-OWNER-BINDING: the session named by the request is bound to a different /// device identity than the caller presented. The agent's correct reaction is to /// rotate its SessionId and register afresh — NOT to count it as an auth failure. - /// Reserved in stage 1 (shadow); emitted only once enforcement is switched on. + /// Owner-binding: reserved in stage 1 (shadow), emitted once enforcement is on. + /// Already emitted by RegisterSession (409) when the session id is claimed by + /// another tenant (SessionTenantLookup first-writer-wins). /// public const string SessionOwnerMismatch = "session_owner_mismatch"; } 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 d76e6aa8..cd4c6f42 100644 --- a/src/Web/autopilot-monitor-web/app/admin/components/OpsAlertRulesSection.tsx +++ b/src/Web/autopilot-monitor-web/app/admin/components/OpsAlertRulesSection.tsx @@ -103,6 +103,10 @@ const OPS_EVENT_TYPES: Record = { // different device identity (SessionOwnerBindingObserver, throttled 1h per session+outcome). // Stage 1 observes only. Dual-register per memory feedback_ops_event_types_dual_register. "SessionOwnerMismatch", + // A registration named a sessionId already owned by ANOTHER tenant (first-writer-wins + // SessionTenantLookup claim) and was refused with 409. Dual-register per memory + // feedback_ops_event_types_dual_register. + "SessionTenantConflict", "EmbeddedCertExpiringSoon", "EmbeddedCertExpiringUrgent", "EmbeddedCertExpired",