59856b8c63
Adds the 2026-07-08 architecture review (00-overall + six domain reports) and a remediation/ tree: one design+implementation doc per domain covering every finding, plus 00-tracking.md as the master progress tracker. - 153 findings with stable IDs (GWC/WRK/IPC/SEC/CLI/TST), each with design rationale, implementation steps, tests, docs, and verification. - Tracker rolls findings up by severity and P0/P1/P2 roadmap tier, records cross-cutting clusters and per-finding status (all Not started). - Planning docs only; no source changes.
538 lines
51 KiB
Markdown
538 lines
51 KiB
Markdown
# Security, Dashboard & Observability — Remediation Design & Implementation
|
||
|
||
Source review: [40-security-dashboard.md](../40-security-dashboard.md) · Generated: 2026-07-09
|
||
|
||
This document turns every finding in the Security/Dashboard/Observability review into a buildable remediation entry. The crypto and interceptor foundations are sound; the work here is policy tightening (loopback/`DisableLogin`/scope gaps), cross-platform path hygiene, hot-path auth cost, and documentation reconciliation. IDs are re-sequenced under the domain prefix `SEC-NN` (most-severe first); each entry names the original review tag (e.g. *review STA-1*) so evidence stays traceable. Roadmap tiers follow `00-overall.md`: the P1 "Security policy pass" (item 9) and "cross-platform paths" (item 10), and the P2 per-session-ACL (item 12) and documentation-drift sweep (item 15). Findings not itemized in that roadmap but belonging to the same security-hardening theme (§theme 6) are tagged P1 with a note; the remainder are `—`.
|
||
|
||
## Finding index
|
||
|
||
| ID | Sev | Title | Roadmap | Effort | Depends on | Files |
|
||
|----|-----|-------|---------|--------|-----------|-------|
|
||
| SEC-01 | Medium | Windows-absolute default paths become relative files off-Windows | P1 | M | — | Configuration/AuthenticationOptions.cs, Configuration/TlsOptions.cs, Configuration/GatewayOptionsValidator.cs, appsettings.json |
|
||
| SEC-02 | Medium | `AllowAnonymousLocalhost` satisfies AdminOnly, not just Viewer | P1 | S | — | Dashboard/DashboardAuthorizationHandler.cs |
|
||
| SEC-03 | Medium | Dashboard cookie lost its `__Host-` prefix; four docs still promise it | P2 | S | — | Dashboard/DashboardAuthenticationDefaults.cs, gateway.md, docs/GatewayDashboardDesign.md, CLAUDE.md |
|
||
| SEC-04 | Medium | `DisableLogin` has no production guard | P1 | S | SEC-03 | Dashboard/DashboardServiceCollectionExtensions.cs, Configuration/GatewayOptionsValidator.cs |
|
||
| SEC-05 | Medium | Hub bearer tokens irrevocable for 30 min, carried in query string | P1 | M | — | Dashboard/HubTokenService.cs, Dashboard/HubTokenAuthenticationHandler.cs, Dashboard/DashboardEndpointRouteBuilderExtensions.cs |
|
||
| SEC-06 | Medium | LDAP plaintext-by-default with a committed service password | P1 | M | — | Configuration/LdapOptions.cs, appsettings.json, docs/GatewayConfiguration.md |
|
||
| SEC-07 | Medium | `QueryActiveAlarmsRequest` missing from scope resolver; tests mislabeled | P1 | S | — | Security/Authorization/GatewayGrpcScopeResolver.cs, Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs, docs/Authorization.md |
|
||
| SEC-08 | Medium | Per-RPC SQLite read + `last_used_utc` write; no verification cache | P1 | L | — | Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs, Security/Authentication/GatewayApiKeyIdentityMapper.cs |
|
||
| SEC-09 | Medium | Dashboard design-doc `GroupToRole` sample now fails startup validation | P2 | S | — | docs/GatewayDashboardDesign.md |
|
||
| SEC-10 | Medium | No API-key expiry | P1 | L | — | (shared `ZB.MOM.WW.Auth.ApiKeys`), Dashboard/DashboardApiKeyManagementService.cs |
|
||
| SEC-11 | Medium | No rate limiting or lockout on either auth surface | P1 | M | SEC-08 | Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs, Dashboard/DashboardEndpointRouteBuilderExtensions.cs |
|
||
| SEC-12 | Medium | Dashboard Close/Kill bypass the canonical audit store | P1 | M | — | Dashboard/DashboardSessionAdminService.cs |
|
||
| SEC-13 | Low | Redactor's credential-command list omits secured-bulk variants | — | S | SEC-30 | Diagnostics/GatewayLogRedactor.cs |
|
||
| SEC-14 | Low | `/metrics` + `/health` unauthenticated; a metric leaks session ids | — | S | SEC-02, SEC-20 | GatewayApplication.cs, Metrics/GatewayMetrics.cs |
|
||
| SEC-15 | Low | GET `/logout` skips antiforgery | — | S | — | Dashboard/DashboardEndpointRouteBuilderExtensions.cs |
|
||
| SEC-16 | Low | CLI accepts the pepper as a command-line argument | — | S | — | Program.cs |
|
||
| SEC-17 | Low | Interceptor does not override client-streaming/duplex handlers | — | S | — | Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs |
|
||
| SEC-18 | Low | Pepper-unavailable detection matches library message text | — | S | — | Dashboard/DashboardApiKeyManagementService.cs |
|
||
| SEC-19 | Low | Canonical audit store re-issues `CREATE TABLE` per write/read | — | S | — | Security/Audit/SqliteCanonicalAuditStore.cs |
|
||
| SEC-20 | Low | `session_id` metric tag is unbounded cardinality | P1 | S | — | Metrics/GatewayMetrics.cs, docs/Metrics.md |
|
||
| SEC-21 | Low | Snapshot publisher works every second regardless of audience | — | M | — | Dashboard/DashboardSnapshotService.cs |
|
||
| SEC-22 | Low | `docs/Authentication.md` documents types no longer in this repo | P2 | M | — | docs/Authentication.md |
|
||
| SEC-23 | Low | Validator ignores `DisableLogin`/`AutoLoginUser`/`RequireHttpsCookie`/`CookieName` | — | S | SEC-03, SEC-04 | Configuration/GatewayOptionsValidator.cs |
|
||
| SEC-24 | Low | Effective-config view omits the riskiest dashboard flags | — | S | SEC-04 | Configuration/GatewayConfigurationProvider.cs |
|
||
| SEC-25 | Low | Per-session EventsHub ACL is an acknowledged TODO | P2 | M | SEC-02 | Dashboard/Hubs/EventsHub.cs |
|
||
| SEC-26 | Low | Audit trail has no retention or pruning | — | M | — | Security/Audit/SqliteCanonicalAuditStore.cs |
|
||
| SEC-27 | Low | Dashboard `GatewayStatus` is hardcoded Healthy | — | S | — | Dashboard/DashboardSnapshotService.cs |
|
||
| SEC-28 | Info | Positive security observations (preserve under change) | — | S | — | (multiple) |
|
||
| SEC-29 | Info | UI-stack rule verified compliant | — | S | — | wwwroot/lib/ |
|
||
| SEC-30 | Info | Value-logging feature is unwired end-to-end | P2 | S | SEC-13 | Diagnostics/GatewayLogRedactor.cs, docs/Diagnostics.md |
|
||
|
||
---
|
||
|
||
## SEC-01 — Windows-absolute default paths become relative files off-Windows `Medium` · `P1`
|
||
|
||
**Finding.** *(review SEC-1)* Three option defaults are Windows-absolute string literals: `AuthenticationOptions.SqlitePath` (`Configuration/AuthenticationOptions.cs:9`, mirrored `appsettings.json:17`), `TlsOptions.SelfSignedCertPath` (`Configuration/TlsOptions.cs:11-12`), and Galaxy `SnapshotCachePath` (`appsettings.json:80`). On Unix these contain no path separator, so SQLite/PFX writers treat the whole string `C:\ProgramData\MxGateway\gateway-auth.db` as a single filename relative to the content root — which is how the stray auth DB materialized under `src/ZB.MOM.WW.MxGateway.Server/`. The validator does not catch it: `AddIfInvalidPath` only requires `Path.GetFullPath` to *succeed* (`Configuration/GatewayOptionsValidator.cs:383-410`), and a bare filename is a valid relative path.
|
||
|
||
**Impact.** The auth DB (and, if an HTTPS endpoint is configured, a private-key PFX) silently lands in the working directory, its location varying by host OS and launch CWD. Keys created under one CWD are invisible under another. A materialized key DB in the tree is protected only by the generic `*.db` gitignore; a PFX would not be.
|
||
|
||
**Design.** Derive defaults from `Environment.GetFolderPath(SpecialFolder.CommonApplicationData)` + `Path.Combine`, which resolves to `C:\ProgramData` on Windows and `/usr/share` (or the container equivalent) elsewhere, and make the validator *reject* non-rooted paths for these keys so a relative override fails fast rather than writing into CWD. Rejected alternative: keeping the literals and only fixing the validator — that still leaves a broken default for any non-Windows run and diverges appsettings from code. Rooting enforcement is chosen over auto-rooting because the paths are security-sensitive (auth DB, private key) and silent relocation is worse than a boot error.
|
||
|
||
**Implementation.**
|
||
- `Configuration/AuthenticationOptions.cs:9`, `Configuration/TlsOptions.cs:11-12`: replace the literal initializers with a static helper, e.g. `Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "MxGateway", "gateway-auth.db")`. Keep `appsettings.json` values as the explicit Windows deployment path (they override the code default on the production hosts).
|
||
- `Configuration/GatewayOptionsValidator.cs`: add an `AddIfNotRooted` check (`!Path.IsPathRooted(value)`) applied to `SqlitePath`, `SelfSignedCertPath`, and Galaxy `SnapshotCachePath`, alongside the existing `AddIfInvalidPath`.
|
||
- Delete the stray `src/ZB.MOM.WW.MxGateway.Server/C:\ProgramData\MxGateway\gateway-auth.db` file and add a guard test that fails if any `*.db` appears under `src/` (a repo-hygiene test in `ZB.MOM.WW.MxGateway.Tests`).
|
||
- Tests: extend `GatewayOptionsValidatorTests` with a non-rooted-path case per key; add the tree-hygiene test.
|
||
- Docs: note the derived defaults in `docs/GatewayConfiguration.md` and `docs/Authentication.md`.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayOptionsValidator"`.
|
||
|
||
---
|
||
|
||
## SEC-02 — `AllowAnonymousLocalhost` satisfies AdminOnly, not just Viewer `Medium` · `P1`
|
||
|
||
**Finding.** *(review SEC-2)* `Dashboard/DashboardAuthorizationHandler.cs:32-37` calls `context.Succeed(requirement)` for any loopback request before the role loop, and the same handler serves both `AdminOnly` and `AnyDashboardRole`. Lines 25-30 do the same for `Authentication.Mode == Disabled` — for **remote** requests too. `AllowAnonymousLocalhost` defaults `true` (`Configuration/DashboardOptions.cs:9`).
|
||
|
||
**Impact.** Anything gated solely by `MxGateway.Dashboard.Admin` is authorized for an anonymous local process; `HubClientsPolicy` uses the same requirement, so a loopback process reaches every SignalR hub — the snapshot hub pushes the API-key inventory and effective config every second, and `EventsHub.SubscribeSession` joins any session's raw `MxEvent` feed. Destructive surfaces are saved today only by service-layer re-checks (`Dashboard/DashboardApiKeyAuthorization.cs`, `Dashboard/DashboardSessionAdminService.cs:33-39`), i.e. defense-in-depth, not the policy.
|
||
|
||
**Design.** Restrict the loopback bypass to the Viewer requirement: succeed only when `requirement.RequiredRoles` contains `DashboardRoles.Viewer` (the read-only requirement) and not when it is the Admin-only requirement. Decide deliberately whether anonymous loopback reaches hubs at all — recommended: keep hub read access for loopback (dashboard-on-the-box is the primary local-ops UX) but never Admin. This preserves the documented "anonymous localhost is read-only" contract at the policy layer instead of by accident. Note the loopback test trusts `Connection.RemoteIpAddress` (`:56-61`); if forwarded-headers middleware is ever added, this must be revisited — capture that as a comment.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/DashboardAuthorizationHandler.cs`: in the loopback branch, succeed only if `requirement.RequiredRoles.Contains(DashboardRoles.Viewer)` (the `AnyDashboardRole` requirement includes Viewer; `AdminOnly` does not). Leave the `Mode == Disabled` branch as-is but document that it is a global kill-switch, or gate it behind the same Viewer check for consistency (open question: whether `Disabled` should still grant admin — recommend no).
|
||
- Tests: add cases to `DashboardAuthorizationHandlerTests` asserting loopback satisfies `AnyDashboardRole` but is denied `AdminOnly`.
|
||
- Docs: reconcile `docs/GatewayDashboardDesign.md` and `CLAUDE.md`'s "anonymous localhost bypasses auth on loopback" wording to say "read-only".
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~DashboardAuthorizationHandler"`.
|
||
|
||
---
|
||
|
||
## SEC-03 — Dashboard cookie lost its `__Host-` prefix; four docs still promise it `Medium` · `P2`
|
||
|
||
**Finding.** *(review SEC-3)* `Dashboard/DashboardAuthenticationDefaults.cs:38` sets `CookieName = "MxGatewayDashboard"`, but `gateway.md:211`, `docs/GatewayDashboardDesign.md:425`, `docs/GatewayProcessDesign.md:686`, `docs/ImplementationPlanGateway.md:454`, and `CLAUDE.md` all still claim `__Host-MxGatewayDashboard`. Only `docs/GatewayConfiguration.md:170` is correct.
|
||
|
||
**Impact.** The `__Host-` browser guarantees (Secure required, no `Domain`, `Path=/`) are not in effect. The cookie is still HttpOnly/SameSite=Strict/SecurePolicy-controlled via `ZbCookieDefaults.Apply` (`Dashboard/DashboardServiceCollectionExtensions.cs:93-109`), and `RequireHttpsCookie=false` plus the configurable `CookieName` are legitimate reasons the prefix was dropped — so this is primarily doc drift, but security docs overstate protections.
|
||
|
||
**Design.** Prefer restoring the guarantee conditionally over a pure doc fix: when `RequireHttpsCookie` is true and no explicit `CookieName` override is set, default to `__Host-MxGatewayDashboard`; otherwise keep the plain name (the `__Host-` prefix is incompatible with `SameAsRequest`/HTTP). This is a small change in the PostConfigure at `DashboardServiceCollectionExtensions.cs:119-133` and recovers the documented protection for the default secure deployment. Either way, update the four stale docs in the *same* change (repo rule).
|
||
|
||
**Implementation.**
|
||
- `Dashboard/DashboardServiceCollectionExtensions.cs`: in the `CookieAuthenticationOptions` PostConfigure, when `RequireHttpsCookie` and `CookieName` is null/blank, set `cookieOptions.Cookie.Name = "__Host-MxGatewayDashboard"`. Guard: never apply `__Host-` when `SecurePolicy != Always` (would be silently dropped — this ties to SEC-23's consistency check).
|
||
- `Dashboard/DashboardAuthenticationDefaults.cs`: keep `MxGatewayDashboard` as the non-secure fallback constant.
|
||
- Docs: update `gateway.md`, `docs/GatewayDashboardDesign.md`, `docs/GatewayProcessDesign.md`, `docs/ImplementationPlanGateway.md`, `CLAUDE.md` to describe the actual conditional contract.
|
||
- Tests: `DashboardServiceCollectionExtensionsTests` (or a cookie-options resolution test) asserting the name flips with `RequireHttpsCookie`.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the dashboard-config test filter.
|
||
|
||
---
|
||
|
||
## SEC-04 — `DisableLogin` has no production guard `Medium` · `P1`
|
||
|
||
**Finding.** *(review SEC-4)* `Dashboard/DashboardServiceCollectionExtensions.cs:63-90` swaps in `DashboardAutoLoginAuthenticationHandler` under the cookie scheme for **all** clients when `MxGateway:Dashboard:DisableLogin` is true; the handler authenticates every request, remote included (`Dashboard/DashboardAutoLoginAuthenticationHandler.cs:56-62`). `GatewayOptionsValidator.ValidateDashboard` (`Configuration/GatewayOptionsValidator.cs:219-253`) never inspects `DisableLogin`, and the warning fires only on first `GatewayOptions` resolution (not guaranteed at process start).
|
||
|
||
**Impact.** One copied config flag turns the whole dashboard — API-key CRUD and worker Kill included — into an unauthenticated admin surface on a 0.0.0.0-bound port.
|
||
|
||
**Design.** Fail fast in production: in `ValidateDashboard`, when `IHostEnvironment.IsProduction()` and `DisableLogin` is true, emit a validator error so startup aborts like every other misconfiguration. Rejected alternative: silently forcing the flag off — hides operator intent and diverges config from behavior. The dev warning stays for non-production. Co-design with SEC-03 (cookie reconciliation) and SEC-23 (validator coverage) so the dashboard-security fixes land together.
|
||
|
||
**Implementation.**
|
||
- `Configuration/GatewayOptionsValidator.cs`: inject/accept the environment name (the validator already runs at startup; thread `IHostEnvironment` in, or pass an `isProduction` flag) and add the check in `ValidateDashboard`.
|
||
- Tests: `GatewayOptionsValidatorTests` — `DisableLogin=true` + Production → invalid; + Development → valid.
|
||
- Docs: `docs/GatewayConfiguration.md` DisableLogin row notes the production hard-stop.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~GatewayOptionsValidator"`.
|
||
|
||
---
|
||
|
||
## SEC-05 — Hub bearer tokens irrevocable for 30 min, carried in query string `Medium` · `P1`
|
||
|
||
**Finding.** *(review SEC-5)* `Dashboard/HubTokenService.cs:29,44-52` mints a 30-minute data-protected token with no jti/revocation state and roles frozen at issue; `Dashboard/HubTokenAuthenticationHandler.cs:59-61` accepts `?access_token=` on the WebSocket upgrade. Logout (`Dashboard/DashboardEndpointRouteBuilderExtensions.cs:136-155`) clears the cookie but not outstanding tokens.
|
||
|
||
**Impact.** A token captured from an access log or proxy grants live snapshot/alarm/event access for up to 30 minutes after logout; role changes/revocations do not take effect until expiry.
|
||
|
||
**Design.** Two independent, low-cost mitigations: (1) shorten `TokenLifetime` to ~5 minutes — the factory refreshes per reconnect (`docs/GatewayDashboardDesign.md:497-499`), so the shorter window is transparent to clients and bounds exposure; (2) confirm no request-path logging captures query strings (Serilog request logging is not enabled — keep it off, or scrub `access_token` if it is ever added). Full server-side revocation (a jti denylist keyed on the data-protection payload) is the heavier option; recommend deferring it and relying on the short lifetime, since the tokens are already encrypted and single-purpose. State the open question: if per-session ACLs (SEC-25) land, tokens will need role/session binding and revocation becomes worthwhile.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/HubTokenService.cs:29`: `TokenLifetime = TimeSpan.FromMinutes(5)`.
|
||
- Add an XML-doc note at `HubTokenAuthenticationHandler` that query-string carriage is the SignalR pattern and must not be request-logged.
|
||
- Docs: `docs/GatewayDashboardDesign.md` token section — update the lifetime and the logout-vs-token caveat.
|
||
- Tests: `HubTokenServiceTests` — token invalid after lifetime; issue/validate round-trip.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~HubTokenService"`.
|
||
|
||
---
|
||
|
||
## SEC-06 — LDAP plaintext-by-default with a committed service password `Medium` · `P1`
|
||
|
||
**Finding.** *(review SEC-6)* `Configuration/LdapOptions.cs:49-61` defaults `Transport=None`, `AllowInsecure=true`, `ServiceAccountPassword="serviceaccount123"`; `appsettings.json:21-33` ships the same. `glauth.md:30,327` confirms dev LDAPS is disabled and binds send cleartext. The validator enforces the `Transport=None ⇒ AllowInsecure` consistency rule (`GatewayOptionsValidator.cs:82-85`) but nothing distinguishes dev from prod.
|
||
|
||
**Impact.** Every dashboard login sends the operator's password cleartext to `10.100.0.35:3893`, and a service-account credential is in source control.
|
||
|
||
**Design.** These are deliberate dev defaults (the shadow-options rationale at `LdapOptions.cs:20-28` is explicit that the shared library is secure-by-default). The fix is a production posture, not a default flip: (1) require `Transport=Ldaps`/`StartTls` + `AllowInsecure=false` in production deployment docs, and add an `IsProduction()` validator check (mirroring SEC-04) that rejects `Transport=None` in production; (2) move `ServiceAccountPassword` out of `appsettings.json` to env-var/secret configuration on the deployed hosts and rotate the committed value. LDAP-injection escaping is delegated to `ZB.MOM.WW.Auth.Ldap` (bind-then-search, `Dashboard/DashboardAuthenticator.cs:41-47`) and cannot be verified here — flag for review in the donor repo.
|
||
|
||
**Implementation.**
|
||
- `Configuration/GatewayOptionsValidator.cs`: in `ValidateLdap`, when Production and `Transport == None`, emit an error (co-locate with SEC-04's env plumbing).
|
||
- Deployment: keep `serviceaccount123` only for local GLAuth dev; document env-var override (`MxGateway__Ldap__ServiceAccountPassword`) for the NSSM-wrapped hosts; rotate the dev credential's reuse.
|
||
- Docs: `docs/GatewayConfiguration.md` Ldap section and a production hardening note referencing `glauth.md`.
|
||
- Tests: `GatewayOptionsValidatorTests` — `Transport=None` + Production → invalid.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the validator test filter.
|
||
|
||
---
|
||
|
||
## SEC-07 — `QueryActiveAlarmsRequest` missing from scope resolver; tests mislabeled `Medium` · `P1`
|
||
|
||
**Finding.** *(review STA-1)* `Security/Authorization/GatewayGrpcScopeResolver.cs:15-29` has arms for `StreamAlarmsRequest` (→ `EventsRead`) and `AcknowledgeAlarmRequest` but **no** `QueryActiveAlarmsRequest` arm, so that RPC (`Grpc/MxAccessGatewayService.cs`, proto `mxaccess_gateway.proto`) falls to the `_ => GatewayScopes.Admin` default. The two `ServerStreamingServerHandler_QueryActiveAlarms…` tests construct `new StreamAlarmsRequest()` instead (`Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs:322-359`), so they pass while testing the wrong message. `docs/Authorization.md`'s scope table omits the RPC.
|
||
|
||
**Impact.** Fail-closed (not a vulnerability) but a client holding `events:read` gets `PermissionDenied` demanding `admin` on `QueryActiveAlarms`, contradicting the stated design that alarm snapshot data shares the event surface.
|
||
|
||
**Design.** Add the missing arm `QueryActiveAlarmsRequest => GatewayScopes.EventsRead`, consistent with `StreamAlarmsRequest`. Fix both tests to use the real request type so they actually exercise the arm. Add the row to `docs/Authorization.md`. No parity or backward-compat concern — it only relaxes an over-strict requirement to the documented scope.
|
||
|
||
**Implementation.**
|
||
- `Security/Authorization/GatewayGrpcScopeResolver.cs:22`: add `QueryActiveAlarmsRequest => GatewayScopes.EventsRead,` next to `StreamAlarmsRequest`.
|
||
- `Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs:322-359`: change `new StreamAlarmsRequest()` to `new QueryActiveAlarmsRequest()` in both `QueryActiveAlarms` tests; keep the `events:read`-passes / missing-scope-denies assertions.
|
||
- Docs: `docs/Authorization.md` scope table — add `QueryActiveAlarms → events:read`; also `docs/Grpc.md` if it omits the RPC.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~GatewayGrpcAuthorizationInterceptor"`.
|
||
|
||
---
|
||
|
||
## SEC-08 — Per-RPC SQLite read + `last_used_utc` write; no verification cache `Medium` · `P1`
|
||
|
||
**Finding.** *(review PERF-1)* The interceptor calls `IApiKeyVerifier.VerifyAsync` per call (`Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs:69-71`); the verifier contract is find-by-id → hash → compare → `MarkKeyUsedAsync` per `docs/Authentication.md`, and the `last_used_utc` write "runs on every authenticated request". The interceptor also deserializes the constraints JSON per call (`Security/Authentication/GatewayApiKeyIdentityMapper.cs:41`).
|
||
|
||
**Impact.** WAL + busy-timeout keep it correct, but a per-call database **write** is the throughput ceiling on the hot path (bulk reads at high frequency are the primary workload), causing continuous WAL churn and making auth-store latency a per-RPC tail-latency contributor.
|
||
|
||
**Design.** Add a short-TTL (5–30 s) in-memory verification cache keyed by key id + presented-hash, invalidated on revoke/rotate — all mutations flow through in-process `ApiKeyAdminCommands`, so invalidation is a direct call, not a cross-process concern. Coalesce `last_used_utc` updates to at most one write per key per minute (track last-flushed-at in the cache entry). Cache the parsed constraints on the identity so the per-call JSON deserialize disappears. This is `L` effort and likely needs a small surface addition in `ZB.MOM.WW.Auth.ApiKeys` (a `MarkKeyUsed` batching hook or a verifier that returns the raw stored record so the gateway can cache it) — recommend coordinating with the donor library rather than shadowing its store. Open question: whether the cache lives gateway-side (wraps `IApiKeyVerifier`) or in the shared library; recommend gateway-side wrapper first (no library release dependency), keyed on a constant-time hash of the presented secret to avoid caching plaintext.
|
||
|
||
**Implementation.**
|
||
- New gateway-side `CachingApiKeyVerifier : IApiKeyVerifier` decorator registered in `Security/Authentication/AuthStoreServiceCollectionExtensions.cs`; memory cache (`IMemoryCache`) with per-entry TTL; invalidation entry point called from `ApiKeyAdminCommands` on create/revoke/rotate.
|
||
- Coalesce `MarkKeyUsedAsync`: only forward when the cached entry's last-marked timestamp is older than the coalesce window.
|
||
- `GatewayApiKeyIdentityMapper.cs:41`: cache the deserialized constraints on the mapped identity.
|
||
- Tests: `CachingApiKeyVerifierTests` — cache hit avoids store read; revoke invalidates; `last_used` coalesced to ≤1/min. Load-style assertion optional.
|
||
- Docs: `docs/Authentication.md` and `docs/Metrics.md` — document the cache TTL/coalesce and any new config knob.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~CachingApiKeyVerifier"`.
|
||
|
||
---
|
||
|
||
## SEC-09 — Dashboard design-doc `GroupToRole` sample fails startup validation `Medium` · `P2`
|
||
|
||
**Finding.** *(review CON-1)* `docs/GatewayDashboardDesign.md:515-519` shows `"GroupToRole": { "GwAdmin": "Admin", … }`, but `GatewayOptionsValidator.cs:233-238` accepts only `DashboardRoles.Admin` = `"Administrator"` (`Dashboard/DashboardRoles.cs:14`) or `"Viewer"`. Live `appsettings.json:66-69` correctly uses `"Administrator"`. The doc also calls the role `Admin` in prose (lines 412-413, 434).
|
||
|
||
**Impact.** An operator copying the documented sample gets a fail-fast boot error.
|
||
|
||
**Design.** Doc-only fix: update the sample and prose to the canonical `Administrator` value (the rename is annotated in `glauth.md:79-83`). No code change — the validator is correct.
|
||
|
||
**Implementation.**
|
||
- `docs/GatewayDashboardDesign.md`: change the `GroupToRole` sample values to `"Administrator"`/`"Viewer"` and align role prose.
|
||
|
||
**Verification.** Doc change; no build. Cross-check the sample against `Dashboard/DashboardRoles.cs`.
|
||
|
||
---
|
||
|
||
## SEC-10 — No API-key expiry `Medium` · `P1`
|
||
|
||
**Finding.** *(review UND-1)* The `api_keys` schema carries `created_utc`/`last_used_utc`/`revoked_utc` only (confirmed from the stray DB `sqlite_master` and `docs/Authentication.md`); no expiry field, no expiry check in the verification flow, no staleness surfacing beyond `LastUsedUtc`.
|
||
|
||
**Impact.** Keys live until an operator revokes; a leaked key is valid indefinitely.
|
||
|
||
**Design.** Add optional `expires_utc` to the shared `ZB.MOM.WW.Auth.ApiKeys` schema + verifier (an expired key verifies as failed, opaque to the client like every other auth failure). Surface age/staleness on the dashboard API Keys page. This is `L` because the store/verifier live in the donor package — coordinate a schema migration there; the gateway consumes it. Gateway-side work: expose an optional `--expires` on `apikey create`, display expiry/staleness in `DashboardApiKeyManagementService` summaries. Open question: default expiry (recommend none — opt-in — to preserve current behavior; backward-compatible).
|
||
|
||
**Implementation.**
|
||
- Shared library: `expires_utc` column + migration + verifier check + `ApiKeyPepperUnavailableException`-style typing (see SEC-18).
|
||
- Gateway: `Program.cs` apikey create option; `Dashboard/DashboardApiKeyManagementService.cs` summary projection; API Keys Razor page staleness badge.
|
||
- Docs: `docs/Authentication.md` token lifecycle + `docs/Authorization.md` if scopes interplay.
|
||
- Tests: verifier expiry test in the library; gateway summary test.
|
||
|
||
**Verification.** After the library bump: `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~DashboardApiKeyManagement"`.
|
||
|
||
---
|
||
|
||
## SEC-11 — No rate limiting or lockout on either auth surface `Medium` · `P1`
|
||
|
||
**Finding.** *(review UND-2)* The gRPC interceptor verifies unconditionally per call (`GatewayGrpcAuthorizationInterceptor.cs:54-91`); `/auth/login` has no throttle (`Dashboard/DashboardEndpointRouteBuilderExtensions.cs:38-43,99-134`). The only brake is dev GLAuth's per-IP 3-fail lockout (`glauth.md:35`), absent in production AD and hazardous behind shared NAT.
|
||
|
||
**Impact.** Unbounded online guessing of API-key secrets (each guess costs a SQLite read — compounds with SEC-08) and unthrottled LDAP credential stuffing relayed to the directory.
|
||
|
||
**Design.** Add ASP.NET Core rate limiting on `/auth/login` (fixed-window per remote IP) and a cheap per-peer failure counter in front of `VerifyAsync` (sliding-window limiter keyed on peer + presented key id) that short-circuits with `ResourceExhausted`/`Unauthenticated` before the store read. Co-design with SEC-08 (both sit on the auth hot path; the cache and the limiter share the peer key). Recommend the built-in `Microsoft.AspNetCore.RateLimiting` middleware for the dashboard and an in-process counter (bounded LRU) for gRPC to avoid a dependency in the interceptor. Note the NAT caveat from `glauth.md:323-325`: prefer per-key-id over pure per-IP for the gRPC counter.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/DashboardEndpointRouteBuilderExtensions.cs`: apply a named rate-limiter policy to the login POST route.
|
||
- `Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs` (or a decorator): per-peer failure counter checked before `VerifyAsync`; reset on success.
|
||
- Config: new `MxGateway:Security:LoginRateLimit*` / `ApiKeyFailureLimit*` keys bound in `GatewayOptions`, validated.
|
||
- Tests: interceptor test asserting Nth failed attempt short-circuits; dashboard integration test for 429 on burst.
|
||
- Docs: `docs/GatewayConfiguration.md`, `docs/Authorization.md`.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the interceptor + dashboard test filters.
|
||
|
||
---
|
||
|
||
## SEC-12 — Dashboard Close/Kill bypass the canonical audit store `Medium` · `P1`
|
||
|
||
**Finding.** *(review UND-3)* `Dashboard/DashboardSessionAdminService.cs:64-69,129-134` record close/kill via `ILogger` only, whereas API-key operations write `AuditEvent`s through `IAuditWriter` (`Dashboard/DashboardApiKeyManagementService.cs:242-264`).
|
||
|
||
**Impact.** Destructive operational actions (killing a worker mid-production) leave no durable, queryable audit row — invisible to the dashboard's recent-audit view and subject to log rotation.
|
||
|
||
**Design.** Emit `dashboard-close-session` / `dashboard-kill-worker` `AuditEvent`s through the existing `IAuditWriter`, mirroring the API-key pattern. Actor resolution helpers already exist in the service (`ResolveActor`, `ResolveRemoteAddress`). Keep the `ILogger` line too (operational tail). No new store — reuse `SqliteCanonicalAuditStore`.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/DashboardSessionAdminService.cs`: inject `IAuditWriter`; after a successful close/kill, write an `AuditEvent` with actor, session id, remote address, and `AlreadyClosed`. Failure paths write a denied/failed event as the API-key path does.
|
||
- Tests: `DashboardSessionAdminServiceTests` — successful close/kill produces the expected `AuditEvent`; assert against a fake `IAuditWriter`.
|
||
- Docs: `docs/Authorization.md` audit-event catalog — add the two action names.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~DashboardSessionAdmin"`.
|
||
|
||
---
|
||
|
||
## SEC-13 — Redactor's credential-command list omits secured-bulk variants `Low` · `—`
|
||
|
||
**Finding.** *(review SEC-7)* `Diagnostics/GatewayLogRedactor.cs:11-16` lists `AuthenticateUser`, `WriteSecured`, `WriteSecured2` but not `WriteSecuredBulk`/`WriteSecured2Bulk`, which exist as command kinds (`Security/Authorization/GatewayGrpcScopeResolver.cs:41-45`). Currently latent — `RedactCommandValue`/`IsCredentialBearingCommand` have no call sites (grep-verified), so no value logging occurs.
|
||
|
||
**Impact.** If value logging is ever wired (SEC-30), secured-bulk payloads (credential-bearing) would pass the unconditional-redaction check.
|
||
|
||
**Design.** Add the two bulk names now — trivial and forward-safe. Add a test asserting every `WriteSecured*` command kind is treated as credential-bearing, so the redactor cannot drift from the command catalog again.
|
||
|
||
**Implementation.**
|
||
- `Diagnostics/GatewayLogRedactor.cs:13-15`: add `"WriteSecuredBulk"`, `"WriteSecured2Bulk"` to `SensitiveCommandMethods`.
|
||
- Tests: `GatewayLogRedactorTests` — enumerate `MxCommandKind` names starting with `WriteSecured` and assert `IsCredentialBearingCommand` is true for each.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~GatewayLogRedactor"`.
|
||
|
||
---
|
||
|
||
## SEC-14 — `/metrics` + `/health` unauthenticated; a metric leaks session ids `Low` · `—`
|
||
|
||
**Finding.** *(review SEC-8)* `GatewayApplication.cs:196-197` maps `MapZbHealth()`/`MapZbMetrics()` (shared packages) with no `RequireAuthorization`; `Metrics/GatewayMetrics.cs:354` tags `mxgateway.heartbeats.failed` with `session_id`.
|
||
|
||
**Impact.** If the shared packages don't enforce auth internally (not verifiable here), an unauthenticated scraper on the gRPC/dashboard port can read telemetry including live session identifiers usable against the anonymous-localhost hub surface (SEC-02).
|
||
|
||
**Design.** Verify the shared packages' endpoint auth; if anonymous, either bind metrics to a loopback-only endpoint or gate with `RequireAuthorization`. Combine with SEC-20 (drop the `session_id` tag) so the leak closes even if the endpoint stays open. Depends-on SEC-02 (hub surface) and SEC-20 (tag).
|
||
|
||
**Implementation.**
|
||
- Confirm `MapZbMetrics`/`MapZbHealth` auth behavior in the shared package; if open, add `.RequireAuthorization()` or bind to a separate loopback Kestrel endpoint.
|
||
- Docs: `docs/Metrics.md`/`docs/Diagnostics.md` state the endpoint's auth posture.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; manual `curl` of `/metrics` on a running gateway to confirm the posture.
|
||
|
||
---
|
||
|
||
## SEC-15 — GET `/logout` skips antiforgery `Low` · `—`
|
||
|
||
**Finding.** *(review SEC-9)* `Dashboard/DashboardEndpointRouteBuilderExtensions.cs:53-58` intentionally skips antiforgery on GET `/logout` (comment acknowledges it). POST login/logout validate antiforgery (`:104,140`).
|
||
|
||
**Impact.** A third-party page can sign a dashboard operator out (nuisance CSRF; no state beyond the session).
|
||
|
||
**Design.** Acceptable as documented — logout is self-destructive with no side effects. No code change; if logout ever grows side effects, add a confirmation interstitial. Recorded so the tracking doc captures the accepted risk.
|
||
|
||
**Implementation.** None (documented acceptance). Optionally add a one-line note to `docs/GatewayDashboardDesign.md`.
|
||
|
||
**Verification.** N/A.
|
||
|
||
---
|
||
|
||
## SEC-16 — CLI accepts the pepper as a command-line argument `Low` · `—`
|
||
|
||
**Finding.** *(review SEC-10)* `Program.cs:37-40` maps `command.Pepper` → `MxGateway:ApiKeyPepper`.
|
||
|
||
**Impact.** The pepper lands in shell history and the process command line visible to other local users.
|
||
|
||
**Design.** Prefer an environment variable (`MxGateway__ApiKeyPepper`) or an interactive prompt over the CLI arg; keep the arg for scripted use but document the exposure in `--help`. Low-risk, small change.
|
||
|
||
**Implementation.**
|
||
- `Program.cs`: fall back to the `MxGateway__ApiKeyPepper` env var when `--pepper` is absent; note the risk in the apikey subcommand help text.
|
||
- Docs: CLAUDE.md apikey example already uses scopes; add a pepper-via-env note in `docs/Authentication.md`.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; run `apikey --help`.
|
||
|
||
---
|
||
|
||
## SEC-17 — Interceptor does not override client-streaming/duplex handlers `Low` · `—`
|
||
|
||
**Finding.** *(review STA-2)* `Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs:21-47` overrides only `UnaryServerHandler` and `ServerStreamingServerHandler`. All current RPCs are unary or server-streaming (verified against both `.proto` files), so nothing bypasses auth today.
|
||
|
||
**Impact.** A future duplex/client-streaming RPC would run with no authentication and no scope check, silently.
|
||
|
||
**Design.** Override `ClientStreamingServerHandler` and `DuplexStreamingServerHandler` to run the same auth path (fail-closed to `admin` via the resolver default) — a loud, safe default so any future streaming RPC is authenticated by construction rather than accidentally open.
|
||
|
||
**Implementation.**
|
||
- `Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs`: add the two overrides calling `AuthenticateAndAuthorizeAsync` then the continuation, mirroring the existing two.
|
||
- Tests: `GatewayGrpcAuthorizationInterceptorTests` — a client-streaming/duplex handler is denied without a key.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the interceptor test filter.
|
||
|
||
---
|
||
|
||
## SEC-18 — Pepper-unavailable detection matches library message text `Low` · `—`
|
||
|
||
**Finding.** *(review STA-3)* `Dashboard/DashboardApiKeyManagementService.cs:21,281-282` catches `InvalidOperationException` whose `Message` contains `"pepper unavailable"`.
|
||
|
||
**Impact.** A wording change in `ZB.MOM.WW.Auth.ApiKeys` turns the friendly "pepper not configured" result into an unhandled exception on the Blazor circuit.
|
||
|
||
**Design.** Ask the library to expose a typed `ApiKeyPepperUnavailableException` (the pre-cutover code had one per `docs/Authentication.md:68`) and catch that instead of string-matching. Coordinate with SEC-10 (also a library change). Until the library ships it, keep the string match but add a test pinning the expected message so drift is caught in CI.
|
||
|
||
**Implementation.**
|
||
- Shared library: add/restore the typed exception.
|
||
- `Dashboard/DashboardApiKeyManagementService.cs`: catch the typed exception; drop the message match.
|
||
- Tests: `DashboardApiKeyManagementServiceTests` — pepper-unavailable yields the friendly result, not an unhandled throw.
|
||
|
||
**Verification.** After library bump: `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the management-service test filter.
|
||
|
||
---
|
||
|
||
## SEC-19 — Canonical audit store re-issues `CREATE TABLE` per write/read `Low` · `—`
|
||
|
||
**Finding.** *(review STA-4)* `Security/Audit/SqliteCanonicalAuditStore.cs:54,94,131-136` runs `CREATE TABLE IF NOT EXISTS audit_event` on every write and read.
|
||
|
||
**Impact.** An extra round-trip per audit op, and a schema defined outside the migrator — a future column change has no migration path (the `IF NOT EXISTS` silently keeps the old shape).
|
||
|
||
**Design.** Move `audit_event` creation into the startup migration path (alongside the api-key schema migrator) and drop the per-call ensure. Ties to UND-5/SEC-26 (retention) — do both in the audit-store pass.
|
||
|
||
**Implementation.**
|
||
- `Security/Audit/SqliteCanonicalAuditStore.cs`: remove per-call `CREATE TABLE`; ensure the migration hosted service creates `audit_event`.
|
||
- Tests: `SqliteCanonicalAuditStoreTests` — write/read succeed post-migration; no `CREATE TABLE` on the hot path (assert via a command-counting fake or integration check).
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~CanonicalAudit"`.
|
||
|
||
---
|
||
|
||
## SEC-20 — `session_id` metric tag is unbounded cardinality `Low` · `P1`
|
||
|
||
**Finding.** *(review PERF-2)* `Metrics/GatewayMetrics.cs:354` tags `mxgateway.heartbeats.failed` with `session_id`; `docs/Metrics.md:47` documents it. The in-memory `EventsBySession` map is correctly pruned on close (`:310-313`) — only the exported tag is the problem.
|
||
|
||
**Impact.** Every session ever opened mints a new exporter time series; long-running gateways with churn bloat Prometheus/OTLP storage.
|
||
|
||
**Design.** Drop the `session_id` tag from the exported counter (keep the aggregate). Per-session attribution stays available via the dashboard snapshot and log scope. Co-design with SEC-08 (both land in the observability pass) and closes the leak flagged in SEC-14.
|
||
|
||
**Implementation.**
|
||
- `Metrics/GatewayMetrics.cs:354`: `_heartbeatFailuresCounter.Add(1)` without the tag.
|
||
- Docs: `docs/Metrics.md:47` — remove the `session_id` dimension.
|
||
- Tests: `GatewayMetricsTests` (if present) — counter increments without a `session_id` tag.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the metrics test filter.
|
||
|
||
---
|
||
|
||
## SEC-21 — Snapshot publisher works every second regardless of audience `Low` · `—`
|
||
|
||
**Finding.** *(review PERF-3)* `Dashboard/DashboardSnapshotService.cs:134-166` runs the publisher loop; `:256-279` (`RefreshApiKeySummariesAsync`) calls `IApiKeyAdminStore.ListAsync` every tick (gated, 2 s-timeboxed); `:76-105` rebuilds session/worker/metric/fault/config projections each tick. The Galaxy breakdown is properly memoized by sequence (`:107-129`).
|
||
|
||
**Impact.** Constant background SQLite reads and allocation churn on an idle gateway; the full snapshot (API-key summaries + effective config) is serialized to all hub clients every second.
|
||
|
||
**Design.** Refresh the API-key summary on a slower cadence or on mutation (all mutations are in-process via `ApiKeyAdminCommands`, so an invalidation hook is exact — shares the mechanism with SEC-08's cache invalidation). Skip publication when the hub has no connections. Keep the 1 s cadence for live session/metric projections.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/DashboardSnapshotService.cs`: gate `RefreshApiKeySummariesAsync` behind a mutation flag (set by `ApiKeyAdminCommands`) or a longer interval; short-circuit the publish when `IHubContext` has zero clients.
|
||
- Tests: `DashboardSnapshotServiceTests` — key summary refreshes on mutation, not every tick; no publish with zero connections.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the snapshot-service test filter.
|
||
|
||
---
|
||
|
||
## SEC-22 — `docs/Authentication.md` documents types no longer in this repo `Low` · `P2`
|
||
|
||
**Finding.** *(review CON-2)* The doc presents `ApiKeyParser`, `ApiKeySecretGenerator`, `SqliteApiKeyStore`, and a parameterless `AddSqliteAuthStore()` (`docs/Authentication.md:9-28,36-48,253-272`), but the real registration is the package-delegating two-parameter method (`Security/Authentication/AuthStoreServiceCollectionExtensions.cs:41-105`) and the store/verifier live in `ZB.MOM.WW.Auth.ApiKeys`. The doc also says `api_key_audit` is written on every denial (`:122,131`), but the override redirects all writes to `audit_event`, leaving `api_key_audit` unused (`Security/Audit/CanonicalForwardingApiKeyAuditStore.cs:21-25`).
|
||
|
||
**Impact.** Violates the "no stale prose" rule; a maintainer auditing hashing/storage from the doc looks for code that isn't there.
|
||
|
||
**Design.** Rewrite `docs/Authentication.md` as a consumer-side doc: token format, options binding, the audit-store override (`api_key_audit` → `audit_event`), and a pointer to the donor library for internals. Fold in SEC-10/SEC-18 (typed exception, expiry) as they land.
|
||
|
||
**Implementation.**
|
||
- `docs/Authentication.md`: rewrite per above; remove code excerpts of moved types.
|
||
|
||
**Verification.** Doc change; cross-check each referenced type against the current tree.
|
||
|
||
---
|
||
|
||
## SEC-23 — Validator ignores `DisableLogin`/`AutoLoginUser`/`RequireHttpsCookie`/`CookieName` `Low` · `—`
|
||
|
||
**Finding.** *(review CON-4)* `Configuration/GatewayOptionsValidator.cs:219-253` validates `GroupToRole`, snapshot interval, and the two limits, but ignores `DisableLogin`, `AutoLoginUser`, `RequireHttpsCookie`, and `CookieName` (all documented at `docs/GatewayConfiguration.md:169-177`). A `CookieName` beginning `__Host-` with `RequireHttpsCookie=false` yields a cookie browsers silently drop — no startup diagnosis.
|
||
|
||
**Impact.** Silent misconfiguration classes the validator exists to prevent.
|
||
|
||
**Design.** Add a `__Host-`/`RequireHttpsCookie` consistency check (reject `CookieName` starting `__Host-` unless `RequireHttpsCookie` is true) and the SEC-04 production guard for `DisableLogin`; validate `AutoLoginUser` semantics. Co-design with SEC-03 (which introduces the `__Host-` default) and SEC-04.
|
||
|
||
**Implementation.**
|
||
- `Configuration/GatewayOptionsValidator.cs` `ValidateDashboard`: add the checks.
|
||
- Tests: `GatewayOptionsValidatorTests` — `__Host-` + `RequireHttpsCookie=false` → invalid.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the validator test filter.
|
||
|
||
---
|
||
|
||
## SEC-24 — Effective-config view omits the riskiest dashboard flags `Low` · `—`
|
||
|
||
**Finding.** *(review CON-5)* `Configuration/GatewayConfigurationProvider.cs:57-64` projects `EffectiveDashboardConfiguration` without `DisableLogin`, `AutoLoginUser`, `RequireHttpsCookie`, or `CookieName`; TLS and Alarms sections are absent from `EffectiveGatewayConfiguration` entirely.
|
||
|
||
**Impact.** The Settings page cannot show an operator that login is disabled — the one flag they most need to see.
|
||
|
||
**Design.** Add the missing (non-secret) fields to the effective-config projection so the dashboard surfaces them. Depends-on SEC-04 (same flag). Optionally add TLS/Alarms sections; keep secrets redacted as the provider already does for the pepper/LDAP password.
|
||
|
||
**Implementation.**
|
||
- `Configuration/GatewayConfigurationProvider.cs`: extend `EffectiveDashboardConfiguration` with the four flags; consider TLS/Alarms sections.
|
||
- Tests: `GatewayConfigurationProviderTests` — effective config includes `DisableLogin` etc.; secrets still redacted.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the provider test filter.
|
||
|
||
---
|
||
|
||
## SEC-25 — Per-session EventsHub ACL is an acknowledged TODO `Low` · `P2`
|
||
|
||
**Finding.** *(review UND-4)* `Dashboard/Hubs/EventsHub.cs:29-44` carries `TODO(per-session-acl)` — any Viewer (and anonymous localhost per SEC-02) can `SubscribeSession` to any session's raw event feed, bypassing the per-gRPC-subscriber filtering (`docs/GatewayDashboardDesign.md:170`).
|
||
|
||
**Impact.** Acceptable per the in-code v1 rationale, but it is the single seam where tag values reach the least-privileged principals; combined with `ShowTagValues=false` expectations it can surprise operators.
|
||
|
||
**Design.** Keep the TODO tied to the tracked per-session-ACL work item (P2 roadmap item 12). As a near-term hardening, redact event values in the dashboard mirror when `ShowTagValues` is false, so the seam cannot leak values regardless of the ACL. Depends-on SEC-02 (loopback reaching hubs). Full ACL: introduce a role/scope that scopes a Viewer to a session/tenant and enforce it at `SubscribeSession`.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/Hubs/EventsHub.cs` / `DashboardEventBroadcaster`: honor `ShowTagValues` by redacting values in the mirror now; add the session-access check when the scoping role lands.
|
||
- Tests: broadcaster test asserting values redacted when `ShowTagValues=false`.
|
||
- Docs: `docs/GatewayDashboardDesign.md` — clarify the current v1 posture.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the events-hub/broadcaster test filter.
|
||
|
||
---
|
||
|
||
## SEC-26 — Audit trail has no retention or pruning `Low` · `—`
|
||
|
||
**Finding.** *(review UND-5)* `audit_event` is append-only with no cleanup path (`Security/Audit/SqliteCanonicalAuditStore.cs`); constraint denials append per denied bulk entry (`docs/Authorization.md:194-198`).
|
||
|
||
**Impact.** A misconfigured constrained client hammering denied reads grows the auth DB without bound.
|
||
|
||
**Design.** Add a retention sweep (age- or row-count-based, config-gated) run by a hosted service, or document the operational archive expectation. Recommend an age-based sweep with a generous default (e.g. 90 days) so the DB is self-limiting. Do this in the same audit-store pass as SEC-19.
|
||
|
||
**Implementation.**
|
||
- `Security/Audit/`: add a retention sweep (hosted service or piggyback the migration service) with `MxGateway:Security:AuditRetentionDays` config.
|
||
- Tests: sweep deletes rows older than the window; keeps recent.
|
||
- Docs: `docs/Authorization.md`/`docs/Diagnostics.md` retention note; `docs/GatewayConfiguration.md` new key.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the audit test filter.
|
||
|
||
---
|
||
|
||
## SEC-27 — Dashboard `GatewayStatus` is hardcoded Healthy `Low` · `—`
|
||
|
||
**Finding.** *(review UND-6)* `Dashboard/DashboardSnapshotService.cs:17,96` sets `GatewayStatus` to a `HealthyStatus` constant.
|
||
|
||
**Impact.** The home page headline status never reflects registered health checks (e.g. `AuthStoreHealthCheck` unhealthy while the banner says Healthy).
|
||
|
||
**Design.** Project `HealthCheckService` results into the snapshot so the banner reflects real health. Inject `HealthCheckService`, run/observe the aggregated status on the publish tick (cheap — health checks are cached), map to the dashboard status enum.
|
||
|
||
**Implementation.**
|
||
- `Dashboard/DashboardSnapshotService.cs`: inject `HealthCheckService`; set `GatewayStatus` from the aggregated `HealthReport`.
|
||
- Tests: `DashboardSnapshotServiceTests` — unhealthy check surfaces as non-Healthy status.
|
||
|
||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` and the snapshot-service test filter.
|
||
|
||
---
|
||
|
||
## SEC-28 — Positive security observations (preserve under change) `Info` · `—`
|
||
|
||
**Finding.** *(review SEC-11)* Verified-good behaviors to preserve: fail-closed unrecognized-request → `admin` (`GatewayGrpcScopeResolver.cs:28`); opaque auth failures (`GatewayGrpcAuthorizationInterceptor.cs:66-78`); effective-config redacts pepper name + LDAP password (`GatewayConfigurationProvider.cs`); Galaxy connection string rebuilt field-by-field for display (`Dashboard/DashboardConnectionStringDisplay.cs`); global identity-property masking seam (`Diagnostics/GatewayLogRedactorSeam.cs`); self-signed PFX generation hardens permissions before writing and clears the buffer (`Security/Tls/SelfSignedCertificateProvider.cs:160-193`); scope strings validated against the canonical catalog on every creation path; dashboard key-id input constrained to a safe charset; single generic login-failure message; `SanitizeReturnUrl` blocks open redirects. No secret-logging call site found.
|
||
|
||
**Impact.** These are the load-bearing controls; regressions here are high-severity.
|
||
|
||
**Design.** No action. Guard against regression: the tests named in other entries (redactor, scope catalog, opaque-failure) already pin most of these — keep them.
|
||
|
||
**Implementation.** None. Reference in review sign-off.
|
||
|
||
**Verification.** N/A.
|
||
|
||
---
|
||
|
||
## SEC-29 — UI-stack rule verified compliant `Info` · `—`
|
||
|
||
**Finding.** *(review CON-3)* `wwwroot/lib/` contains only local Bootstrap CSS/JS; grep for MudBlazor/Radzen/Syncfusion/Telerik across `.csproj`/`.razor`/`.cs` returns nothing. The shared `ZB.MOM.WW.Theme` package supplies CSS only.
|
||
|
||
**Impact.** Confirms the CLAUDE.md "no Blazor UI component libraries" invariant holds.
|
||
|
||
**Design.** No action. Optionally add a build-time or test guard that fails if a banned package reference appears, to keep the invariant enforced rather than remembered.
|
||
|
||
**Implementation.** Optional repo-hygiene test asserting no MudBlazor/Radzen/FluentUI/Telerik/Syncfusion package references.
|
||
|
||
**Verification.** N/A (or run the hygiene test if added).
|
||
|
||
---
|
||
|
||
## SEC-30 — Value-logging feature is unwired end-to-end `Info` · `P2`
|
||
|
||
**Finding.** *(review UND-7)* `GatewayLogRedactor.RedactCommandValue`/`IsCredentialBearingCommand` have no call sites in the server (grep-verified); the opt-in value-logging flag described in `docs/Diagnostics.md:124-148` has no knob in `GatewayOptions`.
|
||
|
||
**Impact.** Currently the safest state (no values logged anywhere), but the doc implies a capability that doesn't exist end-to-end.
|
||
|
||
**Design.** Either wire the flag (add a `MxGateway:Diagnostics:LogCommandValues` option, thread it to the redactor call sites, default off) or trim the doc to match. Recommend trimming the doc for now and deferring wiring; if wiring proceeds, land SEC-13 (bulk-secured names) first so secured payloads are covered. Depends-on SEC-13.
|
||
|
||
**Implementation.**
|
||
- If trimming: `docs/Diagnostics.md` — mark value logging as not-yet-implemented.
|
||
- If wiring: add the option + validation, call `RedactCommandValue` at the command-log site with the flag, tests asserting credential-bearing commands stay redacted even with the flag on.
|
||
|
||
**Verification.** If wired: `dotnet build src/ZB.MOM.WW.MxGateway.Server` and `dotnet test ...MxGateway.Tests --filter "FullyQualifiedName~GatewayLogRedactor"`. Otherwise doc-only.
|