dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(transport): zero a bundle session's content when it expires, not only on apply/abandon (review 2026-10-21 04/C3)

Browse Source
This commit is contained in:
Joseph Doherty
2026-10-22 09:18:37 -04:00
parent fad08f7ce7
commit 55cebf2397
3 changed files with 76 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/requirements/Component-Transport.md
+1 -1
View File
@@ -136,7 +136,7 @@ flowchart TD
The component is central-hosted. It is registered in `ZB.MOM.WW.ScadaBridge.Host` for central roles only, never for site roles — even when a bundle carries site-scoped config (T18), Transport only writes the central configuration database, never the site clusters. All persistence flows through existing audited repository interfaces in `ZB.MOM.WW.ScadaBridge.ConfigurationDatabase` — the component does not call `DbContext.SaveChangesAsync` directly. `BundleSessionStore` is in-process on the active central node (matching Blazor Server circuit affinity): 30-minute TTL, eviction on expiry, 3-strike passphrase lockout per session, and a `MaxConcurrentImportSessions` cap (default 8; arch-review 05 Task 24 — startup-validated as `> 0` by `TransportOptionsValidator`, since a zero/negative cap would reject every import session forever) bounding the decrypted-content footprint since each open session pins a fully-decrypted bundle in memory. **The cap is observable (register row 53):** open (unexpired) sessions are published as the `scadabridge.central.transport.import_sessions.open` gauge on the `ZB.MOM.WW.ScadaBridge` meter — already in the `ObservedMeters` allowlist, so no telemetry config change is needed — and a rejection at the cap logs at Warning with the current count, the cap and the TTL. Reclaiming a store full of *expired* sessions is not a cap hit and logs nothing. **A log line is not alertable, so register row 66 adds the counter `scadabridge.central.transport.import_sessions.rejected`** on the same meter, incremented at the same single genuine-rejection point (the post-eviction recheck — never on the reclaim-then-succeed path, and never when re-opening an already-open session id). Alert on the counter; interpret it against the gauge — rejections rising while the gauge stays below the cap means genuine concurrency (raise the cap), whereas rejections rising while the gauge is pinned at the cap means a leaked session (raising the cap only defers it).
**Session release is explicit, not TTL-only (register row 43).** `ApplyAsync` releases the session it applied (zero-then-remove, T-007), and `IBundleImporter.Abandon(sessionId)` does the same for a session that will never be applied. Every abandonment path calls it: the Central UI wizard on new upload / Back-to-Upload / `ResetSessionState` / lockout reset / `Dispose` (navigation away or circuit teardown), the CLI `transport preview` handler (a preview never applies), and the CLI `transport import` handler for the aborts that precede apply (blocker rows, unresolved mappings). `Abandon` is idempotent — double release, an unknown id, and an already-expired id are all silent no-ops — and since session ids are random GUIDs handed only to their opener, abandoning the id you hold can never evict another operator's in-progress import. Relying on the TTL alone was an availability *and* secret-lifetime defect: eight abandoned wizards exhausted `MaxConcurrentImportSessions` and locked out every import on the active node for 30 minutes, each still holding its decrypted bundle content.
**Session release is explicit, not TTL-only (register row 43).** `ApplyAsync` releases the session it applied (zero-then-remove, T-007), and `IBundleImporter.Abandon(sessionId)` does the same for a session that will never be applied. Every abandonment path calls it: the Central UI wizard on new upload / Back-to-Upload / `ResetSessionState` / lockout reset / `Dispose` (navigation away or circuit teardown), the CLI `transport preview` handler (a preview never applies), and the CLI `transport import` handler for the aborts that precede apply (blocker rows, unresolved mappings). `Abandon` is idempotent — double release, an unknown id, and an already-expired id are all silent no-ops — and since session ids are random GUIDs handed only to their opener, abandoning the id you hold can never evict another operator's in-progress import. Relying on the TTL alone was an availability *and* secret-lifetime defect: eight abandoned wizards exhausted `MaxConcurrentImportSessions` and locked out every import on the active node for 30 minutes, each still holding its decrypted bundle content. **TTL expiry zeroes too (review 2026-10-21 04/C3):** a session dropped because it expired — lazily on `Get`, or by the `EvictExpired` sweep — has its content buffer zeroed by `BundleSessionStore` as it is removed, so every release path (apply, abandon, expiry) follows the same zero-then-remove discipline.
**Preview→apply window (recorded decision).** The diff/preview an operator reviews is a point-in-time snapshot; there is **no optimistic concurrency** between preview and apply. If the target config changes in the window between `PreviewAsync` and `ApplyAsync` (another admin edits a template, say), the apply proceeds against the current state — the resolutions are keyed by `(EntityType, Name)`, so they still target the right rows, but the operator may not have seen the latest state in the diff. This is accepted for v1 (single-operator import is the expected workflow, and the terminal Confirm step already gates a typo-resistant environment-name entry). The bundle-format `version` fields are reserved for a future optimistic-concurrency check but are not yet enforced (Underdeveloped 5).