From b449fe201a3ea973a2eaed0ee0d59755867c5eb1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 11:37:36 -0400 Subject: [PATCH] fix(transport): curate BundleLockedException on the management surface (register row 51) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ManagementService references Commons but not Transport, so MapFault could not see BundleLockedException and the per-bundle 3-strike lockout reached the CLI as "An internal error occurred (CorrelationId=...)" — the operator got a correlation id instead of the reason, and no hint that further attempts on that bundle would be refused. Applies the pattern register row 44 established for BundleUnlockThrottledException: - Relocate BundleLockedException from Transport/Import/ to Commons/Types/Transport/, beside BundleUnlockThrottledException. Both consumers (BundleImporter, CentralUI TransportImport) already carried the Commons.Types.Transport using, so no call-site edits were needed. - Add curated catch arms in ManagementActor.HandlePreviewBundle and HandleImportBundle that rethrow as ManagementCommandException, so the message survives MapFault's disclosure filter. - Mirror the row-44 throttle fault-mapping test for both handlers. - Document the relocation in docs/components/Transport.md and docs/requirements/Component-Transport.md. Tests: ManagementService 499 passed (+2 new), Transport 167 passed. --- docs/components/Transport.md | 4 +- docs/requirements/Component-Transport.md | 2 +- .../Types/Transport/BundleLockedException.cs | 44 ++++++++++++++++++ .../ManagementActor.cs | 22 +++++++++ .../Import/BundleLockedException.cs | 31 ------------- .../ManagementActorTests.cs | 45 +++++++++++++++++++ 6 files changed, 114 insertions(+), 34 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleLockedException.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleLockedException.cs diff --git a/docs/components/Transport.md b/docs/components/Transport.md index c005d609..65dc5d24 100644 --- a/docs/components/Transport.md +++ b/docs/components/Transport.md @@ -122,7 +122,7 @@ Passphrase lockout operates at two levels: per-bundle (3-strike counter in `Bund The two levels interact in a specific order, and it is load-bearing (register row 44 — the limiter had been registered, injected and unit-tested but never called, so only the per-bundle lockout was real): -1. **Per-bundle lockout first.** An already-locked bundle throws `BundleLockedException` without registering a rate-limiter attempt, so hammering a locked bundle cannot spend the client's hourly budget. +1. **Per-bundle lockout first.** An already-locked bundle throws `BundleLockedException` (Commons — register row 51 moved it out of `Transport.Import` for the same reason the throttle exception lives there: `ManagementService` does not reference `Transport`, so a lockout used to reach the CLI as `MapFault`'s generic "An internal error occurred" plus a correlation id) without registering a rate-limiter attempt, so hammering a locked bundle cannot spend the client's hourly budget. 2. **Per-IP throttle next, before the decrypt.** Over budget throws `BundleUnlockThrottledException` (Commons, so the management surface can catch it and return a curated message). Because it precedes the decrypt, a denial burns **no** per-bundle strike — otherwise the throttle would be a way to lock out a bundle someone is legitimately importing. Every unlock attempt is registered, successful ones included: the documented budget counts attempts, not failures. 3. Only then the AES-GCM decrypt, whose failure increments the per-bundle counter as before. @@ -255,7 +255,7 @@ A template's scripts reference a `SharedScript` or `ExternalSystem` that exists ### Passphrase lockout -After 3 wrong passphrase attempts against the same bundle (keyed by `ContentHash`), `BundleImporter.LoadAsync` throws `BundleLockedException`. The session is unusable. Re-upload the bundle file to get a new session with a fresh counter. A `BundleImportUnlockFailed` audit row is written on each failed attempt. +After 3 wrong passphrase attempts against the same bundle (keyed by `ContentHash`), `BundleImporter.LoadAsync` throws `BundleLockedException`. The session is unusable. Re-upload the bundle file to get a new session with a fresh counter. A `BundleImportUnlockFailed` audit row is written on each failed attempt. Both the Central UI and the CLI surface the lockout reason verbatim — the exception lives in `Commons/Types/Transport/` so `ManagementActor` can curate it into a `ManagementCommandException` (register row 51). ### Unlock throttled ("Too many bundle unlock attempts") diff --git a/docs/requirements/Component-Transport.md b/docs/requirements/Component-Transport.md index a1b7287e..8dba53b0 100644 --- a/docs/requirements/Component-Transport.md +++ b/docs/requirements/Component-Transport.md @@ -336,7 +336,7 @@ Imports are all-or-nothing per bundle. A bundle either applies fully or not at a - **AES-256-GCM** for content encryption with **PBKDF2-SHA256 / 600 000 iterations** (OWASP 2023+ guidance), per-bundle random salt, random IV per encryption. The GCM auth tag is verified before decryption — a wrong passphrase fails cleanly. - **Passphrase never persisted.** It lives only inside the export/import service call path and is discarded after use. No environment variable, no log line. -- **Failed-unlock rate limit:** per-bundle 3-strike lockout (keyed by `ContentHash`, server-side); per-IP-per-hour cap (`MaxUnlockAttemptsPerIpPerHour`, default 10, configurable) to deter brute force against a stolen bundle. Each failed attempt produces a `BundleImportUnlockFailed` audit row. The two are evaluated in order — bundle lockout, then per-IP throttle, then decrypt — so hammering an already-locked bundle spends no hourly budget, and a throttle denial (`BundleUnlockThrottledException`, fail-closed with a curated message on both the UI and CLI surfaces) burns **no** per-bundle strike, since it never reaches the decryptor. Successful unlocks count against the hourly budget too: it caps attempts, not failures. The client key is the caller's remote IP — from `CircuitClientAddressProvider` (snapshotted at Blazor circuit establishment, since `HttpContext` is unavailable mid-circuit) for the Central UI, and from `ManagementEnvelope.ClientIp` for the CLI; a caller with no attributable address shares a single stricter bucket rather than bypassing the throttle. Per-client isolation is only real behind a ForwardedHeaders-trusted proxy, the same caveat the login throttle carries in `Component-Security.md`. (Register row 44: the limiter existed, was configured and unit-tested, but was never invoked until this was wired.) +- **Failed-unlock rate limit:** per-bundle 3-strike lockout (keyed by `ContentHash`, server-side); per-IP-per-hour cap (`MaxUnlockAttemptsPerIpPerHour`, default 10, configurable) to deter brute force against a stolen bundle. Each failed attempt produces a `BundleImportUnlockFailed` audit row. The two are evaluated in order — bundle lockout, then per-IP throttle, then decrypt — so hammering an already-locked bundle spends no hourly budget, and a throttle denial (`BundleUnlockThrottledException`, fail-closed with a curated message on both the UI and CLI surfaces) burns **no** per-bundle strike, since it never reaches the decryptor. Both typed exceptions live in `Commons/Types/Transport/` — `ManagementService` references Commons but not Transport, so a type declared beside the importer cannot be curated by `MapFault` and reaches the CLI as a generic internal-error message (register rows 44 and 51). Successful unlocks count against the hourly budget too: it caps attempts, not failures. The client key is the caller's remote IP — from `CircuitClientAddressProvider` (snapshotted at Blazor circuit establishment, since `HttpContext` is unavailable mid-circuit) for the Central UI, and from `ManagementEnvelope.ClientIp` for the CLI; a caller with no attributable address shares a single stricter bucket rather than bypassing the throttle. Per-client isolation is only real behind a ForwardedHeaders-trusted proxy, the same caveat the login throttle carries in `Component-Security.md`. (Register row 44: the limiter existed, was configured and unit-tested, but was never invoked until this was wired.) - **Bundle size cap** on upload (default 100 MB, configurable) to bound memory. - **In-transit:** existing HTTPS to the Central UI; no new channel. - **Audit trail is the chain of custody.** Every export, every import (including aborted ones at validation), and every unlock failure is audit-logged with source env, content hash, encrypted yes/no, and artifact summary. diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleLockedException.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleLockedException.cs new file mode 100644 index 00000000..682abcca --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Types/Transport/BundleLockedException.cs @@ -0,0 +1,44 @@ +namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Transport; + +/// +/// T-003: thrown by IBundleImporter.LoadAsync when an encrypted bundle has +/// exceeded the configured failed-unlock attempt limit +/// (TransportOptions.MaxUnlockAttemptsPerSession, default 3). The lockout is tracked +/// server-side keyed by , so a second tab / CLI caller +/// re-uploading the same bytes hits the same counter and cannot side-step the limit. +/// +/// Distinct from on purpose: this one means +/// "this BUNDLE has burned its strikes" and is keyed by the bundle's content hash; that one +/// means "this CLIENT has attempted too many unlocks, across any bundles" and is keyed by +/// the caller's IP. +/// +/// +/// Register row 51: lives in Commons rather than beside the importer for the same reason +/// does — it is part of the +/// IBundleImporter.LoadAsync contract, which is declared here, and the management +/// surface (which references Commons but not Transport) must be able to catch it to return +/// a clear, curated error instead of a generic internal-failure message plus a correlation id. +/// +/// +public sealed class BundleLockedException : Exception +{ + /// Number of recorded unlock failures for this bundle. + public int FailedAttempts { get; } + + /// SHA-256 (hex) of the bundle's content bytes, the lockout's tracking key. + public string BundleContentHash { get; } + + /// + /// Initializes a new . + /// + /// SHA-256 hex from . + /// Number of failures recorded against this bundle. + public BundleLockedException(string bundleContentHash, int failedAttempts) + : base( + $"Bundle is locked after {failedAttempts} failed unlock attempts. " + + "Wait for the lockout window to expire or re-export the bundle to obtain a new content hash.") + { + BundleContentHash = bundleContentHash ?? throw new ArgumentNullException(nameof(bundleContentHash)); + FailedAttempts = failedAttempts; + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs index 5a6c8f66..7261c3b8 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs @@ -3567,6 +3567,17 @@ public class ManagementActor : ReceiveActor { throw new ManagementCommandException(ex.Message); } + catch (BundleLockedException ex) + { + // Register row 51: the per-bundle 3-strike lockout. LoadAsync raises + // it from inside its own CryptographicException handler, so without + // this arm the operator saw MapFault's generic internal-error message + // plus a correlation id, never the reason. Listed ahead of the + // CryptographicException arm to keep the read order matching the + // importer's throw order (the two types are unrelated, so the + // ordering is documentation, not dispatch). + throw new ManagementCommandException(ex.Message); + } catch (System.Security.Cryptography.CryptographicException) { throw new ManagementCommandException("Wrong passphrase or bundle was tampered."); @@ -3632,6 +3643,17 @@ public class ManagementActor : ReceiveActor { throw new ManagementCommandException(ex.Message); } + catch (BundleLockedException ex) + { + // Register row 51: the per-bundle 3-strike lockout. LoadAsync raises + // it from inside its own CryptographicException handler, so without + // this arm the operator saw MapFault's generic internal-error message + // plus a correlation id, never the reason. Listed ahead of the + // CryptographicException arm to keep the read order matching the + // importer's throw order (the two types are unrelated, so the + // ordering is documentation, not dispatch). + throw new ManagementCommandException(ex.Message); + } catch (System.Security.Cryptography.CryptographicException) { throw new ManagementCommandException("Wrong passphrase or bundle was tampered."); diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleLockedException.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleLockedException.cs deleted file mode 100644 index 987563a4..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleLockedException.cs +++ /dev/null @@ -1,31 +0,0 @@ -namespace ZB.MOM.WW.ScadaBridge.Transport.Import; - -/// -/// T-003: thrown by when an encrypted bundle has -/// exceeded the configured failed-unlock attempt limit -/// (). The lockout is tracked -/// server-side keyed by BundleManifest.ContentHash, so a second tab / CLI caller -/// re-uploading the same bytes hits the same counter and cannot side-step the limit. -/// -public sealed class BundleLockedException : Exception -{ - /// Number of recorded unlock failures for this bundle. - public int FailedAttempts { get; } - - /// SHA-256 (hex) of the bundle's content bytes, the lockout's tracking key. - public string BundleContentHash { get; } - - /// - /// Initializes a new . - /// - /// SHA-256 hex from BundleManifest.ContentHash. - /// Number of failures recorded against this bundle. - public BundleLockedException(string bundleContentHash, int failedAttempts) - : base( - $"Bundle is locked after {failedAttempts} failed unlock attempts. " - + "Wait for the lockout window to expire or re-export the bundle to obtain a new content hash.") - { - BundleContentHash = bundleContentHash ?? throw new ArgumentNullException(nameof(bundleContentHash)); - FailedAttempts = failedAttempts; - } -} diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs index 84ff8003..d7f7c165 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs @@ -3219,6 +3219,51 @@ public class ManagementActorTests : TestKit, IDisposable Assert.Contains("Too many bundle unlock attempts", response.Error); } + [Fact] + public void ImportBundleCommand_LockedBundle_SurfacesTheCuratedReason() + { + // Register row 51: the per-bundle 3-strike lockout used to reach the CLI as + // MapFault's generic "An internal error occurred (CorrelationId=...)" because + // BundleLockedException lived in Transport, which ManagementService does not + // reference. It now lives in Commons alongside BundleUnlockThrottledException + // and is curated the same way. + var (_, importer) = AddBundleSubstitutes(); + + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + throw new Commons.Types.Transport.BundleLockedException("abc123", 3)); + + var payload = Convert.ToBase64String(new byte[] { 0x01, 0x02, 0x03 }); + var actor = CreateActor(); + actor.Tell(Envelope(new ImportBundleCommand(payload, "wrong", "skip"), "Administrator")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("locked after 3 failed unlock attempts", response.Error); + Assert.DoesNotContain("An internal error occurred", response.Error); + } + + [Fact] + public void PreviewBundleCommand_LockedBundle_SurfacesTheCuratedReason() + { + // Register row 51: preview shares the same LoadAsync fault surface as import, + // so it needs the same curated arm. + var (_, importer) = AddBundleSubstitutes(); + + importer.LoadAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(_ => + throw new Commons.Types.Transport.BundleLockedException("abc123", 3)); + + var payload = Convert.ToBase64String(new byte[] { 0x01, 0x02, 0x03 }); + var actor = CreateActor(); + actor.Tell(Envelope(new PreviewBundleCommand(payload, "wrong"), "Administrator")); + + var response = ExpectMsg(TimeSpan.FromSeconds(5)); + Assert.Equal("COMMAND_FAILED", response.ErrorCode); + Assert.Contains("locked after 3 failed unlock attempts", response.Error); + Assert.DoesNotContain("An internal error occurred", response.Error); + } + // ======================================================================== // Native alarm source CRUD (Task 21) // ========================================================================