diff --git a/CLAUDE.md b/CLAUDE.md index 164ec27d..d44125f4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -191,7 +191,7 @@ Other peers in the `scadaproj` family (see `scadaproj/CLAUDE.md` for details): ` - Cookie+JWT hybrid sessions: HttpOnly/Secure cookie carries an embedded JWT (HMAC-SHA256 shared symmetric key), 15-minute expiry with sliding refresh, 30-minute idle timeout. Cookies are the correct transport for Blazor Server (SignalR circuits). - LDAP failure: new logins fail; active sessions continue with current roles. - Load balancer in front of central UI; cookie-embedded JWT + shared Data Protection keys for failover transparency. -- Two-person MxGateway secured writes (M7): two new global roles — `Operator` (initiates) + `Verifier` (approves) — added alongside the canonical `Administrator`/`Designer`/`Deployer`/`Viewer`, with `RequireOperator`/`RequireVerifier` policies. An Operator submits a secured write from the Central UI Secured Writes page (`/operations/secured-writes`); it stays a `Pending` `PendingSecuredWrite` row until a *distinct* Verifier approves it (no-self-approval enforced server-side in the ManagementActor, plus a compare-and-swap race guard). Approval relays a `WriteTagRequest` to the site MxGateway; MxGateway-protocol connections only; each lifecycle event (submit/approve/reject/execute) emits a best-effort `AuditChannel.SecuredWrite` / `AuditKind.SecuredWrite*` central-direct-write row sharing the row id as `CorrelationId`. (SecuredWrite audit rows currently leave `SourceNode` NULL — a logged follow-up.) +- Two-person MxGateway secured writes (M7): two new global roles — `Operator` (initiates) + `Verifier` (approves) — added alongside the canonical `Administrator`/`Designer`/`Deployer`/`Viewer`, with `RequireOperator`/`RequireVerifier` policies. An Operator submits a secured write from the Central UI Secured Writes page (`/operations/secured-writes`); it stays a `Pending` `PendingSecuredWrite` row until a *distinct* Verifier approves it (no-self-approval enforced server-side in the ManagementActor, plus a compare-and-swap race guard). Approval relays a `WriteTagRequest` to the site MxGateway; MxGateway-protocol connections only; each lifecycle event (submit/approve/reject/execute) emits a best-effort `AuditChannel.SecuredWrite` / `AuditKind.SecuredWrite*` central-direct-write row sharing the row id as `CorrelationId`. (SecuredWrite audit rows currently leave `SourceNode` NULL — a logged follow-up.) Pending secured writes expire server-side after a configurable TTL (`ManagementServiceOptions.SecuredWritePendingTtl`, default 24 h): an overdue `Pending` row is CAS'd to `Expired` (never relayed) — enforced at approve/reject and swept opportunistically on list (arch-review S2, `AuditKind.SecuredWriteExpire`). ### Cluster & Failover - Keep-oldest split-brain resolver with `down-if-alone = on`, 15s stable-after. diff --git a/docs/requirements/Component-ManagementService.md b/docs/requirements/Component-ManagementService.md index 509c5896..175cea79 100644 --- a/docs/requirements/Component-ManagementService.md +++ b/docs/requirements/Component-ManagementService.md @@ -151,6 +151,8 @@ The two-person authorization workflow for writes through the MxAccess Gateway. B - **RejectSecuredWriteCommand** (`Verifier` role): marks a `Pending` row `Rejected` with the verifier's reason. - **ListSecuredWritesCommand** (any of `Operator` / `Verifier` / `Administrator`): pending queue + terminal history, global or per-site. Although read-only, the history table exposes process-sensitive tag values, so it is **not** open to any authenticated user — it is gated to the two Secured Write roles plus `Administrator` (any-of), aligning with the `RequireSecuredWriteAccess` Central UI policy (arch-review UA1). +**Server-side pending TTL (arch-review S2).** A `Pending` secured write is not valid indefinitely. Both `ApproveSecuredWriteCommand` and `RejectSecuredWriteCommand` call a shared `TryExpireIfStaleAsync` helper immediately after the `Pending`-status check: if the row's `SubmittedAtUtc` is older than the configured `SecuredWritePendingTtl` (`ManagementServiceOptions`, default **24 h**), the helper atomically transitions it `Pending → Expired` via `ISecuredWriteRepository.TryMarkExpiredAsync` (compare-and-swap — no verifier recorded, `VerifierUser` stays `NULL`), emits a best-effort `SecuredWriteExpire` audit row (system actor, `Discarded` status — the closest terminal `AuditStatus`), and the command fails with an "expired" error. A stale setpoint therefore can **never** be approved or executed days later, and no `WriteTagRequest` is relayed. A non-positive TTL disables expiry. + ### External Systems - **ListExternalSystems** / **GetExternalSystem**: Query external system definitions. @@ -254,6 +256,7 @@ The ManagementActor receives the following services and repositories via DI (inj |---------|--------------|----------| | `ScadaBridge:ManagementService` | `ManagementServiceOptions` | `CommandTimeout` (`TimeSpan`, default 30 s) — Ask timeout the HTTP endpoint applies when forwarding to the `ManagementActor`. A non-positive configured value falls back to the 30 s default. | | | | `LongRunningCommandTimeout` (`TimeSpan`, default 5 min) — Ask timeout applied to long-running commands (`ImportBundle`, `PreviewBundle`, `ExportBundle`, `MgmtDeployArtifacts`, `MgmtDeployInstance`); all other commands use `CommandTimeout`. A non-positive configured value falls back to the 5 min default. | +| | | `SecuredWritePendingTtl` (`TimeSpan`, default 24 h) — age after which a `Pending` secured write is transitioned to `Expired` and can no longer be approved/executed; enforced at approve/reject and swept opportunistically on list. A non-positive value disables expiry (arch-review S2). | ## Dependencies diff --git a/docs/requirements/Component-Security.md b/docs/requirements/Component-Security.md index 014a21a1..63c2baf3 100644 --- a/docs/requirements/Component-Security.md +++ b/docs/requirements/Component-Security.md @@ -138,7 +138,11 @@ Set in a local or docker-dev environment via the environment variable `ScadaBrid - Approve or reject a pending secured write from the Secured Writes page — the *approving* half of the two-person write workflow. - **Purpose**: The approving counterpart to **Operator**. Separation of duties is enforced **server-side**: the ManagementActor rejects any approval where the approving user equals the submitting Operator (no self-approval), so the two roles must be held by distinct principals for a write to execute. (See Component-ManagementService.md and Component-CentralUI.md.) -> **Two-person secured-write workflow.** `Operator` and `Verifier` are deliberately separate global roles so a single principal cannot both initiate and approve a write through the MxAccess Gateway. Both are coarse global roles like the others; any site scoping is layered on at the LDAP-mapping level. Note the dev `DisableLogin` caveat: with `DisableLogin` on, the auto-login principal holds **all** roles, so the two-person flow cannot be exercised end-to-end by a single identity — no-self-approval is covered by handler tests and real two-person use requires two real identities. +> **Two-person secured-write workflow.** `Operator` and `Verifier` are deliberately separate global roles so a single principal cannot both initiate and approve a write through the MxAccess Gateway. Both are coarse global roles like the others; any site scoping is layered on at the LDAP-mapping level. +> +> **Deployment-configuration hazard — never grant one principal both roles.** The whole control collapses if the same identity (or LDAP group) maps to *both* `Operator` and `Verifier`: server-side no-self-approval blocks approving the *exact* write you submitted, but a dual-role principal can still trivially pair-approve with a second submission, so a single compromised or careless account regains unilateral write. Keep `SCADA-Operators` and `SCADA-Verifiers` group membership disjoint. The dev `DisableLogin` caveat is the extreme of this: with `DisableLogin` on, the auto-login principal holds **all** roles, so the two-person flow cannot be exercised end-to-end by a single identity — which is exactly why `DisableLogin=true` is refused outside Development (see the `DisableLoginGuard` note above). No-self-approval is covered by handler tests; real two-person use requires two real, distinct-role identities. +> +> **Server-side pending TTL (arch-review S2).** A submitted secured write is not valid indefinitely. The ManagementActor enforces a configurable **`SecuredWritePendingTtl`** (`ManagementServiceOptions`, default **24 h**): a `Pending` row older than the TTL is atomically transitioned to **`Expired`** (compare-and-swap, so multi-node sweeps are idempotent) and can never be approved — the stale setpoint is never relayed to the site. Expiry is enforced eagerly at approve/reject (an overdue row fails with an "expired" error instead of executing) and opportunistically swept when the Secured Writes list is queried. A non-positive TTL disables expiry. This closes the "approve a days-old setpoint against a since-changed process" window. ## Authorization Policies diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Enums/AuditKind.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Enums/AuditKind.cs index 30005e2a..207e8754 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Enums/AuditKind.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Enums/AuditKind.cs @@ -24,6 +24,14 @@ public enum AuditKind SecuredWriteReject, SecuredWriteExecute, + /// + /// A Pending secured write aged past its server-side TTL and was transitioned to + /// Expired without ever executing — the stale setpoint is never relayed to + /// the site. Emitted best-effort by the system (no verifier) when the approve/reject + /// path or the opportunistic list sweep observes an overdue Pending row. (arch-review S2) + /// + SecuredWriteExpire, + /// /// A reconciliation pull row that failed to insert on every retry up to the /// permanent-abandon threshold, so central advanced its cursor past it and diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index dd1229cf..b64e72ce 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -5,6 +5,7 @@ using System.Text.Json.Serialization; using Akka.Actor; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Deployment; using ZB.MOM.WW.ScadaBridge.Commons.Entities.ExternalSystems; using ZB.MOM.WW.ScadaBridge.Commons.Entities.InboundApi; @@ -1129,6 +1130,28 @@ public class ManagementActor : ReceiveActor } } + /// If the Pending row is older than the configured TTL, CAS it to Expired + /// (multi-node idempotent), audit best-effort, and return true. A stale setpoint + /// must never be approvable days later (arch-review S2). + private static async Task TryExpireIfStaleAsync( + IServiceProvider sp, ISecuredWriteRepository repo, PendingSecuredWrite row) + { + var ttl = sp.GetService>()?.Value.SecuredWritePendingTtl + ?? TimeSpan.FromHours(24); + if (ttl <= TimeSpan.Zero || DateTime.UtcNow - row.SubmittedAtUtc <= ttl) return false; + var expiredAt = DateTime.UtcNow; + if (await repo.TryMarkExpiredAsync(row.Id, expiredAt)) + { + row.Status = "Expired"; + row.DecidedAtUtc = expiredAt; + // No Discarded-equivalent below Expired in the AuditStatus enum; Discarded is + // the closest terminal status (the write was abandoned, never executed). + await EmitSecuredWriteAuditAsync( + sp, AuditKind.SecuredWriteExpire, AuditStatus.Discarded, row, actor: "system"); + } + return true; + } + private static async Task HandleSubmitSecuredWrite( IServiceProvider sp, SubmitSecuredWriteCommand cmd, AuthenticatedUser user) { @@ -1182,6 +1205,14 @@ public class ManagementActor : ReceiveActor throw new ManagementCommandException( $"Secured write {cmd.Id} is '{row.Status}', not Pending; it cannot be approved."); + // Server-side TTL: a Pending write older than the configured age is transitioned + // to Expired and can never be approved — the stale setpoint is never relayed + // (arch-review S2). Checked BEFORE the self-approval/CAS so an overdue row never + // executes, regardless of who touches it. + if (await TryExpireIfStaleAsync(sp, repo, row)) + throw new ManagementCommandException( + $"Secured write {cmd.Id} expired (pending longer than the configured TTL) and can no longer be approved or rejected."); + // Separation of duties: a write may not be verified by its submitter. Checked // BEFORE the CAS so a self-approval never consumes the Pending->Approved // transition. @@ -1331,6 +1362,13 @@ public class ManagementActor : ReceiveActor throw new ManagementCommandException( $"Secured write {cmd.Id} is '{entity.Status}', not Pending; it cannot be rejected."); + // Server-side TTL: an overdue Pending write expires here too, so a reject on a + // stale row deterministically reports the expiry rather than recording a + // human decision (arch-review S2). + if (await TryExpireIfStaleAsync(sp, repo, entity)) + throw new ManagementCommandException( + $"Secured write {cmd.Id} expired (pending longer than the configured TTL) and can no longer be approved or rejected."); + // Separation of duties: the verifier must differ from the submitter. if (string.Equals(entity.OperatorUser, user.Username, StringComparison.OrdinalIgnoreCase)) throw new ManagementCommandException( diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementServiceOptions.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementServiceOptions.cs index 4aa8829d..2e3bc44a 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementServiceOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementServiceOptions.cs @@ -7,4 +7,7 @@ public class ManagementServiceOptions /// Maximum time to wait for a long-running transport/deploy command (import/preview/export bundle, deploy artifacts/instance) before returning a timeout error; default 5 minutes. public TimeSpan LongRunningCommandTimeout { get; set; } = TimeSpan.FromMinutes(5); + + /// Age after which a Pending secured write can no longer be approved; it is transitioned to Expired at the next decision/list touch. Default 24 hours. A non-positive value disables server-side expiry. + public TimeSpan SecuredWritePendingTtl { get; set; } = TimeSpan.FromHours(24); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs index 48fad4f8..852e40be 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/SecuredWriteHandlerTests.cs @@ -53,6 +53,12 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable _services.AddScoped(_ => _auditRepo); _services.AddSingleton(_comms); + // Server-side secured-write TTL (arch-review S2): the expiry helper reads + // SecuredWritePendingTtl off IOptions. Register the + // defaults (24 h TTL) so the TTL tests exercise the real threshold. + _services.AddSingleton>( + Options.Create(new ManagementServiceOptions())); + // #206: route SecuredWrite audit through the REAL CentralAuditWriter (the same // central-direct-write path Notification Outbox dispatch + Inbound API use) so // these tests exercise SourceNode stamping end-to-end — actor → writer → repo. @@ -607,6 +613,71 @@ public class SecuredWriteHandlerTests : TestKit, IDisposable Assert.Contains("not found", updated.ExecutionError); } + // ------------------------------------------------------------------------ + // Server-side TTL expiry — Task 15 (arch-review S2) + // ------------------------------------------------------------------------ + + [Fact] + public void Approve_PendingRowOlderThanTtl_ExpiresInsteadOfExecuting() + { + var row = SeedPendingWrite(1, operatorUser: "alice"); + row.SubmittedAtUtc = DateTime.UtcNow.AddHours(-25); // TTL default 24h + _securedWriteRepo.TryMarkExpiredAsync(1, Arg.Any(), Arg.Any()) + .Returns(true); + + var actor = CreateActor(); + actor.Tell(Envelope(new ApproveSecuredWriteCommand(1, null), "bob", "Verifier")); + + var resp = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("expired", resp.Error, StringComparison.OrdinalIgnoreCase); + _securedWriteRepo.DidNotReceive().TryMarkApprovedAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + // The stale setpoint is NEVER relayed to the site. + Assert.Equal(0, _comms.CallCount); + _securedWriteRepo.Received(1).TryMarkExpiredAsync(1, Arg.Any(), Arg.Any()); + } + + [Fact] + public void Reject_PendingRowOlderThanTtl_ExpiresInsteadOfRejecting() + { + var row = SeedPendingWrite(1, operatorUser: "alice"); + row.SubmittedAtUtc = DateTime.UtcNow.AddHours(-25); + _securedWriteRepo.TryMarkExpiredAsync(1, Arg.Any(), Arg.Any()) + .Returns(true); + + var actor = CreateActor(); + actor.Tell(Envelope(new RejectSecuredWriteCommand(1, null), "bob", "Verifier")); + + var resp = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Contains("expired", resp.Error, StringComparison.OrdinalIgnoreCase); + // The reject path never overwrites the row (the CAS expiry owns the transition). + _securedWriteRepo.DidNotReceiveWithAnyArgs().UpdateAsync(default!, default); + _securedWriteRepo.Received(1).TryMarkExpiredAsync(1, Arg.Any(), Arg.Any()); + } + + [Fact] + public void Approve_FreshPendingRow_StillExecutes() + { + // Submitted 1h ago → within the 24h TTL, so the existing approve path is unchanged. + var row = SeedPendingWrite(7, operatorUser: "alice"); + row.SubmittedAtUtc = DateTime.UtcNow.AddHours(-1); + ArmCasSuccess(7); + + PendingSecuredWrite? updated = null; + _securedWriteRepo + .When(r => r.UpdateAsync(Arg.Any(), Arg.Any())) + .Do(ci => updated = ci.Arg()); + + var actor = CreateActor(); + actor.Tell(Envelope(new ApproveSecuredWriteCommand(7, "approved"), "bob", "Verifier")); + + ExpectMsg(TimeSpan.FromSeconds(5)); + _securedWriteRepo.DidNotReceiveWithAnyArgs().TryMarkExpiredAsync(default, default, default); + Assert.Equal(1, _comms.CallCount); + Assert.NotNull(updated); + Assert.Equal("Executed", updated!.Status); + } + // ------------------------------------------------------------------------ // Audit emission (T14b — SecuredWrite AuditLog rows) // ------------------------------------------------------------------------