dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity
Files
0f5e70fd35ae5aaa996ee6acf24da7126ee05bf4
ScadaBridge/archreview/07-ui-management-security.md
T

25 KiB
Raw Blame History

Architecture Review 07 — Management & Presentation Surface

Domain: Central UI (Blazor Server), Management Service (ManagementActor + HTTP API), CLI, Security & Auth Reviewed: src/ZB.MOM.WW.ScadaBridge.CentralUI, src/ZB.MOM.WW.ScadaBridge.ManagementService, src/ZB.MOM.WW.ScadaBridge.CLI, src/ZB.MOM.WW.ScadaBridge.Security, design docs docs/requirements/Component-CentralUI.md, Component-ManagementService.md, Component-CLI.md, Component-Security.md, and the corresponding test projects.

Scope

Deep read of the ManagementActor (all 3,157 lines), the HTTP management/audit endpoints and authenticator, the full Security project (JWT service, cookie session validator, authorization policies, role mapper, service registration), the secured-writes end-to-end path (UI page → SecuredWriteService → actor handlers), the debug-stream path (DebugStreamHub, DebugView.razor), representative Blazor pages (Alarm Summary, Health, Secured Writes, Notification Report), UI-side trust-boundary services (BrowseService), the CLI HTTP client and command wiring, and deployed configuration in docker/ and deploy/wonder-app-vd03/. Spec-vs-code comparison against the four component documents.

Maturity Verdict

This is a mature, carefully engineered surface with a small number of sharp edges. The security layer is genuinely good — fail-fast key validation, a single shared claim builder for login and refresh, a deliberate and well-documented idle/refresh model, DB-persisted Data Protection keys, curated-vs-generic error mapping that prevents internal-detail leaks, and a secured-writes implementation with a real CAS race guard, server-side no-self-approval, and a TOCTOU protocol re-check at execute time. The Blazor code shows unusual discipline for disposal, thread marshalling, and best-effort degradation. The problems that remain are not sloppiness but asymmetries: a production deployment artifact that ships with authentication disabled, a per-site artifact-deploy option that is silently ignored server-side, secret-projection hygiene applied to SMTP/SMS/API keys but not to data connections / external systems / DB connection strings, and a secured-write "Expired" state that exists in the UI and spec but nowhere in the code. Test depth is thin relative to the actor's ~150-command surface.

Findings — Dimension 1: Stability

[High] S1 — Bundle import via /management can 504 while the import keeps applying

ManagementEndpoints.HandleRequest Asks the actor with a configurable timeout that defaults to 30 s (ManagementEndpoints.cs:18, ResolveAskTimeout at :28-33), and neither docker/central-node-a/appsettings.Central.json nor deploy/wonder-app-vd03/appsettings.Central.json sets ScadaBridge:ManagementService:CommandTimeout. The CLI's bundle commands use a 5-minute client timeout (CLI/Commands/BundleCommands.cs:16) precisely because imports are slow — but the server-side Ask expires at 30 s, returns 504 TIMEOUT (ManagementEndpoints.cs:107-113), and the actor's ProcessCommand task keeps running (PipeTo is not cancelled), so HandleImportBundle (ManagementActor.cs:2949-3015) continues applying the bundle after the caller was told it timed out. Failure scenario: an operator imports a large bundle, gets a 504, retries — under --on-conflict rename the second import produces a duplicate -imported-<stamp> artifact set; under overwrite it double-applies. The same ambiguity applies to fleet-wide DeployArtifacts. Fix: raise the configured timeout for transport/deploy commands (per-command timeout in the envelope), or make import idempotent on a client-supplied import id.

[High] S2 — Pending secured writes never expire; the "Expired" status is fictional

The UI's terminal-status list and badge mapping include "Expired" (CentralUI/Components/Pages/Operations/SecuredWrites.razor:265, 411), and Component-CentralUI.md §Secured Writes lists "Executed / Failed / Rejected / Expired" — but no code anywhere sets a row to Expired (repo-wide grep: the only two hits are the UI file). HandleApproveSecuredWrite (ManagementActor.cs:1099-1246) will approve a Pending row of any age. Failure scenario on a live SCADA surface: an Operator submits a setpoint write, it sits unverified over a weekend while the process moves to a different state, and a Verifier approves the stale value days later — the confirmation dialog (SecuredWrites.razor:342-347) shows the value but not its age. A pending-write TTL (with a sweeper or an at-approve age check) is the obvious missing piece; the status enum already reserved the name.

[Medium] S3 — Poll timers can overlap their own refreshes

Health.razor:521-528 and AlarmSummary.razor:283-291 both use System.Threading.Timer with a fixed period whose callback does InvokeAsync(async () => { await RefreshAsync(); ... }) with no reentrancy guard. AlarmSummary.RefreshAsync fans out per-instance snapshot Asks (site offline ⇒ each fetch waits its full Ask timeout); with a 15 s period and a >15 s degraded fetch, ticks stack up on the dispatcher and the page issues overlapping fan-outs against an already-struggling site. A PeriodicTimer loop or an Interlocked in-flight flag would bound this. (Disposal itself is handled correctly on both pages — Health.razor:757-760, AlarmSummary.razor:382.)

[Medium] S4 — 200 MB request bodies are buffered as strings, then multiplied

The management endpoint raises the body cap to 200 MB for transport bundles (ManagementEndpoints.cs:50, 58-62) and then reads the whole body with StreamReader.ReadToEndAsync() into a string (:77-81); the actor then base64-decodes to a byte[] (ManagementActor.cs:3105-3119), wraps in a MemoryStream, and export goes the other way — stream.CopyToAsync(ms); ms.ToArray(); Convert.ToBase64String(bytes) (ManagementActor.cs:2902-2905), which is ~4 copies of the artifact resident simultaneously (bytes, MemoryStream buffer, base64 string, JSON envelope string). A concurrent pair of large imports on a central node is a realistic OOM/LOH-fragmentation path. Streaming multipart upload (as the audit export already streams downloads — AuditEndpoints.cs:183+) is the right long-term shape.

[Low] S5 — Fire-and-forget SignalR sends swallow failures silently

DebugStreamHub.SubscribeInstance pushes events with _ = hubClients.Client(connectionId).SendAsync(...) (DebugStreamHub.cs:204-221). Deliberate (documented) — but a persistently failing client produces zero telemetry. Elsewhere the hub is exemplary: transient-hub-instance pitfall avoided via IHubContext (:198-200), unsubscribe on disconnect (:255-259).

Positive stability observations

  • DebugView.razor is a model Blazor Server component: _disposed flag set before stream stop, SafeInvokeAsync guarding ObjectDisposedException, dictionary mutation marshalled onto the render thread with the race explicitly documented (DebugView.razor:511-544, 589-611).
  • Failover transparency is real: Data Protection keys persist to the config DB (ConfigurationDatabase/ServiceCollectionExtensions.cs:75-76), the cookie principal is self-contained, and the ManagementActor runs on every central node (stateless, per spec).
  • AlarmSummaryService implements the spec'd fan-out faithfully: SemaphoreSlim(8) cap, partial-results tolerance, caller-cancel propagation (AlarmSummaryService.cs:26, 68-84, 112-123).

Findings — Dimension 2: Performance

[Medium] P1 — Every CLI/management/audit HTTP request performs a full LDAP bind

ManagementAuthenticator.AuthenticateAsync binds against LDAP and re-runs role mapping on every request (ManagementAuthenticator.cs:107-118); the audit endpoints and DebugStreamHub.OnConnectedAsync (DebugStreamHub.cs:118-137) do the same. This is the documented stateless design, but there is no negative-result cache, rate limit, or lockout — so (a) a scripted CLI loop hammers the directory once per command, and (b) POST /management and POST /auth/token (AuthEndpoints.cs:99-148) are unauthenticated LDAP-bind oracles usable for password spraying at whatever rate the LDAP server tolerates. A short-TTL bind cache or a per-IP failure throttle would address both.

[Medium] P2 — Unbounded list handlers materialize whole tables

Several read handlers return GetAll* with no paging: HandleListTemplates (ManagementActor.cs:507-511), HandleListInstances (:718-729, post-filtered in memory for scoped users), HandleQueryDeployments unfiltered branch loads all deployment records and all instances (:2031-2060 — the N+1 was fixed, but it is still two full-table loads per query), and HandleExportBundle loads every template/script/system/site/instance to resolve names (:2818-2831). Fine at current fleet size; a 10k-instance estate will feel it on every CLI instance list. The audit surface shows the house already knows the right pattern (keyset paging, MaxPageSize 1000 — AuditEndpoints.cs:59-66).

[Medium] P3 — ListSecuredWrites hard-caps at 200 rows with no paging

HandleListSecuredWrites does QueryAsync(status, siteId, skip: 0, take: 200) (ManagementActor.cs:1280-1286) and the page loads everything then splits pending/history client-side (SecuredWrites.razor:303-308). Once >200 rows exist, history silently truncates — an audit-adjacent table silently losing visibility of older decisions. No pagination controls exist on the page.

[Low] P4 — Alarm Summary re-sorts/filters per render

FilteredRows().ToList() runs inside the render body (AlarmSummary.razor:170) — recomputed on every StateHasChanged, including each 15 s tick. Cheap at hundreds of rows; worth memoizing if sites grow. No virtualization on any table in the domain, but page sizes are mostly bounded (Notification Report pages at 50 — NotificationReport.razor:352).

Positive performance observations

  • Audit export streams server-side in 1000-row pages and the CLI streams to disk with ResponseHeadersRead (AuditEndpoints.cs:66, ManagementHttpClient.cs:209-210).
  • The actor never blocks its dispatcher: HandleEnvelope role-checks synchronously then PipeTos the async work (ManagementActor.cs:97-120), so the single actor instance is not a mailbox bottleneck for I/O-bound commands — commands execute concurrently on the thread pool with per-command DI scopes (:127-132). This is the correct pattern and worth calling out as such.

Findings — Dimension 3: Conventions & Correctness

[Critical] C1 — Production deployment artifact ships with authentication disabled

deploy/wonder-app-vd03/appsettings.Central.json:40 sets "DisableLogin": true (and :38 "RequireHttpsCookie": false). Per Component-Security.md §Dev Disable-Login Flag there is no environment guard — only a startup log warning — and the bypass is honoured by every surface: the cookie scheme (AutoLoginAuthenticationHandler), POST /management (ManagementAuthenticator.cs:52-76), the audit REST endpoints, and the debug-stream hub (DebugStreamHub.cs:82-91). On this host, every anonymous HTTP request is authenticated as multi-role with all roles system-wide — including Operator and Verifier, which nullifies the two-person secured-write control on a box that can relay MxGateway device writes. deploy/ is described by CLAUDE.md as "Production/on-host deployment artifacts". Even if wonder-app-vd03 is a staging box today, an unauthenticated SCADA control surface should not be one config-file copy away from production. Recommendation: add an environment guard (refuse DisableLogin=true unless ASPNETCORE_ENVIRONMENT=Development or an explicit IUnderstandThisDisablesAuth second key), and scrub the deploy artifact.

[High] C2 — deploy artifacts --site-id is silently ignored; artifacts always deploy fleet-wide

MgmtDeployArtifactsCommand carries int? SiteId (Commons/Messages/Management/DeploymentCommands.cs:3) and both CLI commands send it (CLI/Commands/SiteCommands.cs:209, CLI/Commands/DeployCommands.cs:51; documented in Component-CLI.md and the CLI README as [--site-id <id>]). But HandleDeployArtifacts never reads cmd.SiteId — it calls svc.DeployToAllSitesAsync(user) unconditionally (ManagementActor.cs:2006-2013), and ArtifactDeploymentService.DeployToAllSitesAsync(string user, CancellationToken) (ArtifactDeploymentService.cs:227-229) has no site parameter. Two consequences: (1) contract violation — an operator targeting one site pushes shared scripts/external systems/DB connections to every site; (2) authorization gap — the command also has no EnforceSiteScope call, so a site-scoped Deployer can trigger a fleet-wide artifact deployment. The Central UI's per-site "Deploy Artifacts" button presumably uses a different path; the management surface needs DeployToSiteAsync(siteId) plus a scope check.

[High] C3 — Secret projection is inconsistent: connection credentials leak to any authenticated user and into the audit log

The actor is scrupulous about SMTP credentials, Twilio auth tokens, and API-key hashes — all projected to credential-free shapes before response and audit (SmtpConfigPublicShape ManagementActor.cs:1787-1801, SmsConfigPublicShape :1841-1853, HandleListApiKeys :1955-1967). But three sibling entity families get no such treatment:

  • Data connections — HandleListDataConnections/HandleGetDataConnection (:1401-1416) return the raw entity, whose PrimaryConfiguration/BackupConfiguration JSON embeds OpcUaUserIdentityConfig.Password and CertificatePassword in plaintext (Commons/Types/DataConnections/OpcUaUserIdentityConfig.cs:13,17). List/Get are read-only commands with no role gate (GetRequiredRole default :240-241), so a Viewer- or Operator-only principal can read every site's OPC UA credentials via one CLI call. Create/Update also audit the full entity as afterState (:1429, :1445), persisting the passwords into the audit log readable by OperationalAudit roles.
  • External systems — AuthConfiguration (API keys / Basic credentials) returned raw to any authenticated user and audited verbatim (:1587-1624).
  • Database connections — full ConnectionString (SQL credentials) returned raw in List/Get (:2526-2536); the audit shape here was trimmed to {Id, Name} (:2544, :2557), which shows the team knows the pattern — it just wasn't applied to the response. Recommendation: apply the established *PublicShape pattern (config-with-secrets-elided + HasCredentials flag) to all three, and gate Get-with-secrets behind Admin if the UI editor needs round-tripping.

[Medium] C4 — Spec drift: browse/verify/cert-trust commands are documented as ManagementActor commands but only exist UI-side

Component-ManagementService.md §Remote Queries lists BrowseNodeCommand, SearchAddressSpaceCommand, VerifyEndpointCommand, TrustServerCertCommand/RemoveServerCertCommand/ListServerCertsCommand with Design/Admin role gates. The actor's DispatchCommand has no handlers for any of them — they fall through to NotSupportedException (ManagementActor.cs:423), and since ManagementCommandRegistry auto-registers every *Command in the namespace by reflection (ManagementCommandRegistry.cs:66-79), a CLI POST /management {"command":"TrustServerCert"} deserializes fine and then returns a generic COMMAND_FAILED internal error. The real implementations live in Central UI services (BrowseService.cs, EndpointVerificationService.cs, CertManagementService.cs) with hand-rolled HasClaim role guards (BrowseService.cs:48-55, 103-110). Net effect: the CLI cannot browse address spaces or manage cert trust (a gap for the automation story the CLI exists for), and the component doc misdescribes the enforcement point. Fix the doc or add actor handlers.

[Medium] C5 — Role-name comparison asymmetry between UI policies and the actor (acknowledged, still live)

ASP.NET RequireClaim values are compared ordinal case-sensitive (AuthorizationPolicies.cs:143-169), while the ManagementActor compares roles OrdinalIgnoreCase (ManagementActor.cs:105, 440 etc.), and ValidateMappingRole deliberately preserves the stored row's verbatim casing while validating membership case-insensitively — the comment at ManagementActor.cs:1904-1911 explicitly defers the asymmetry. Failure scenario: an admin creates a mapping row deployer (lowercase) via CLI; the affected users pass every ManagementActor gate but fail every RequireDeployment UI policy — a confusing split-brain where CLI works and the UI 403s. Since the write path already validates against Roles.All, the cheap fix is to canonicalize the stored casing at that single write path.

[Medium] C6 — Area-management role gate contradicts both design docs

CreateAreaCommand/UpdateAreaCommand/DeleteAreaCommand are gated Designer (ManagementActor.cs:187, 207), but Component-ManagementService.md §Authorization ("Admin role required for: site management, area management…") and Component-Security.md §Administrator ("Manage area definitions per site") both assign areas to Admin, and Component-CentralUI.md titles the workflow "Area Management (Admin Role)". Either the code or three documents are wrong; per the repo's own propagate-together rule this should be reconciled in one change.

[Low] C7 — HandleQueryEventLogs passes InstanceName into a positional slot commented "InstanceId filter"

ManagementActor.cs:2757-2766 builds EventLogQueryRequest(..., cmd.InstanceName, /* InstanceId filter */ ...). If the site-side filter expects an id, name-based CLI filtering silently matches nothing; if it expects a name, the comment/param name is wrong. Worth a five-minute verification against SiteEventLogging.

[Low] C8 — Stale CLAUDE.md note on SecuredWrite SourceNode

CLAUDE.md says "SecuredWrite audit rows currently leave SourceNode NULL — a logged follow-up", but EmitSecuredWriteAuditAsync now routes through ICentralAuditWriter, which stamps SourceNode from INodeIdentityProvider (documented at ManagementActor.cs:999-1013). The follow-up appears done; the project memory should be updated so future sessions don't re-fix it.

Positive convention observations

  • No third-party Blazor component libraries — the CentralUI csproj references only Roslyn packages and the in-family ZB.MOM.WW.Theme (ZB.MOM.WW.ScadaBridge.CentralUI.csproj:19-21); tables, tree views, dialogs, toasts, and the KPI chart are all custom, per the repo rule.
  • Options pattern honoured throughout; component libraries take no IConfiguration (Security/ServiceCollectionExtensions.cs:26-37); SecurityOptionsValidator fail-fast with a genuinely well-reasoned invariant (refresh threshold < idle window, SecurityOptions.cs:118-142).
  • Error-surface hygiene is exemplary: MapFault returns curated ManagementCommandException messages verbatim and collapses everything else to a correlation-id-bearing generic message (ManagementActor.cs:137-158) — no raw exception text reaches the CLI/UI for unanticipated faults.
  • The secured-write approve handler is defensive in exactly the right places: no-self-approval before the CAS (:1113-1115), CAS via TryMarkApprovedAsync (:1121), MxGateway protocol re-checked at execute against submit-time reconfiguration (:1138-1165), value-type and decode failures contained so a row is never stuck Approved (:1171-1202), transport exceptions contained (:1206-1226).
  • ResolveRolesCommand deliberately not dispatched with the security rationale documented (:418-422) — good trust-boundary thinking.
  • JWT implementation pins issuer/audience, zero clock skew, disables in/outbound claim mapping symmetrically, enforces ≥256-bit keys at construction (JwtTokenService.cs:54-78, 124-171); the cookie path's login and mid-session refresh share SessionClaimBuilder so claim shapes cannot drift (CookieSessionValidator.cs:200-210, AuthEndpoints.cs:84-89); refresh failures fail-open to current roles and only idle-timeout rejects (CookieSessionValidator.cs:132-152) — matching the documented policy exactly.
  • LDAP is delegated to the shared hardened ZB.MOM.WW.Auth.Ldap library (bind-then-search, escaping, fail-closed — per Security/ServiceCollectionExtensions.cs:49-59); no bespoke LDAP string assembly exists in this repo, so injection surface is centralized in the audited library.

Findings — Dimension 4: Underdeveloped Areas

  1. Secured-write lifecycle — no expiry (S2), no paging (P3), history visible to any authenticated user (ListSecuredWritesCommand falls through to the no-role gate, ManagementActor.cs:238-241) — tag values on the history table may themselves be process-sensitive. Also nothing prevents the same person holding both Operator and Verifier roles via two LDAP groups; the control is only "not the same row's submitter", which is the documented design but worth restating as a deployment-configuration hazard.
  2. ManagementService test depth — 11 test files (tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/) against a 3,157-line actor dispatching ~150 commands. SecuredWriteHandlerTests exists (good — covers the flagship control), but there is no visible exhaustive matrix test asserting GetRequiredRole for every command type — precisely the test that would have caught C2's missing scope check and would freeze the authz table against regressions. Security.Tests is 9 files; CookieSessionValidator is covered.
  3. CLI.Tests is not in the slnx (ZB.MOM.WW.ScadaBridge.slnx:24 includes only the CLI project) — a documented gotcha, but it remains a standing risk that a green solution run hides never-compiled CLI tests.
  4. Deferred follow-ups visible in code/docs: aggregated live alarm stream for Alarm Summary (poll-only today — Component-CentralUI.md §Alarm Summary "logged as a follow-up"); central persistence of cert trust; live LDAP group re-query mid-session (Component-Security.md "Residual limitation"); Parquet export returns 501 (AuditEndpoints.cs:202-210); audit verify-chain is a no-op pending the hash chain. All are honestly documented — the gap is that no single tracking list ties them together.
  5. Login endpoints lack throttling — /auth/login, /auth/token, /management, and the debug hub all do per-request LDAP binds with no failure throttle or lockout (P1). /auth/login also reflects the LDAP failure kind into the redirect query string (AuthEndpoints.cs:41-45); confirm LdapAuthFailureMessages does not distinguish "unknown user" from "bad password" (user enumeration).
  6. Accessibility/UX debt — sortable headers are <th role="button" @onclick> without keyboard handlers or aria-sort (AlarmSummary.razor:176-179); status conveyed by color+text badges is fine, but the custom tables have no scope/caption markup. Minor for an internal tool, but it is uniform debt across every custom grid.
  7. docker/central-node-a/logs/ committed runtime logs sit inside the deploy topology folder (noticed while scanning configs) — noise that will eventually leak something.

Prioritized Recommendations

  1. [Critical] Guard DisableLogin and scrub deploy/wonder-app-vd03 — refuse the flag outside a Development environment (or require a second explicit ack key); remove DisableLogin: true from the on-host deploy artifact. (C1)
  2. [High] Implement per-site artifact deployment or reject the option — honour MgmtDeployArtifactsCommand.SiteId (add DeployToSiteAsync) and add EnforceSiteScope; until then, make the handler fail when SiteId is supplied rather than silently deploying fleet-wide. (C2)
  3. [High] Apply the *PublicShape secret-elision pattern to DataConnection, ExternalSystem, and DatabaseConnection reads and audits; consider role-gating those List/Get commands. (C3)
  4. [High] Add a pending-secured-write TTL that transitions stale rows to the already-reserved Expired status, plus paging on the list. (S2, P3)
  5. [High] Fix the long-command timeout mismatch — per-command Ask timeouts (transport/deploy get minutes), configure CommandTimeout in shipped configs, and/or make bundle import idempotent on an import id so a 504-then-retry is safe. (S1)
  6. [Medium] Add a GetRequiredRole matrix test enumerating every registered command and asserting its gate — freezes the authorization table and catches future C2-style gaps mechanically.
  7. [Medium] Canonicalize role casing at the mapping write path to close the UI-vs-actor case-sensitivity split. (C5)
  8. [Medium] Reconcile the area-management role gate (code says Designer, three docs say Admin) and the browse/cert-command placement (docs say actor, code says UI-only) in one doc+code pass. (C4, C6)
  9. [Medium] Add LDAP-bind failure throttling (per-IP or per-username backoff) in ManagementAuthenticator and the auth endpoints. (P1)
  10. [Low] Housekeeping — reentrancy guards on the two poll timers (S3), update the stale SecuredWrite SourceNode memory note (C8), verify the InstanceName/InstanceId event-log filter slot (C7), pull docker/*/logs out of the tree, and put CLI.Tests into the slnx or CI explicitly.
Reference in New Issue View Git Blame Copy Permalink