f4919ba97c
Six domain reports + overall synthesis. All 47 cycle-2 Done claims verified at HEAD with zero false Dones and zero regressions; 34 new findings (0 Critical, 0 High, 4 Medium, 25 Low, 5 Info): GWC-31..38, WRK-32..34, IPC-33..39, SEC-37..41, CLI-46..50, TST-31..36.
111 lines
30 KiB
Markdown
111 lines
30 KiB
Markdown
# Security & Dashboard — Re-review (Cycle 3)
|
||
|
||
**Date:** 2026-08-19 · **Commit:** `8b3e3b3` (`main`, clean) · **Baseline:** `4f5371f` (2026-07-12 cycle) · **Method:** static review on the macOS working tree (no builds, no runtime probes, no source modifications). Every claim cites a `path:line` read at HEAD. Prior findings are the `SEC-NN` IDs from `archreview/remediation/40-security-dashboard.md` (cycle 1) and `archreview/2026-07-12/40-security-dashboard.md` / `archreview/2026-07-12/remediation/40-security-dashboard.md` (cycle 2); new findings continue from SEC-37. This domain absorbed the most new machinery since the baseline (~55 of ~247 commits touched Security/Dashboard/Configuration): the SEC-31/32 failure-limiter re-partition, SEC-33/34/36 remediations, the SEC-25 tag-ACL epic (`DashboardTags` constraints, session tag inheritance, `GroupToTag`/`UntaggedSessionVisibility`, events-hub + in-process ACL), the `ShowTagValues` three-seam closure, the secrets-store adoption (`${secret:}` expansion, `/admin/secrets`), the bounded async audit writer with retention, and the snapshot-feed/alarm-page reliability rework.
|
||
|
||
## Prior-finding verification
|
||
|
||
### Cycle-2 findings (SEC-31…SEC-36)
|
||
|
||
| ID | Claimed | Verified? | Evidence (file:line at HEAD) | Notes |
|
||
|----|---------|-----------|------------------------------|-------|
|
||
| SEC-31 | Done | **Yes** | Composite `(transport peer, key id)` partition + cross-peer aggregate + probe admission, exactly per the remediation design: `Security/Authorization/ApiKeyFailureLimiter.cs:14-27` (two-layer remarks), `:143-204` (`Check` with probe claim/compensation), `:206-235` (`RecordFailure` into both layers), `:237-276` (`Reset` reachable-while-throttled); interceptor threads the partition and reuses the check-time resolution (`GatewayGrpcAuthorizationInterceptor.cs:83-115`, `:133-137`); new knobs validated (`SecurityOptions.ApiKeyFailureAggregateLimit`/`ApiKeyFailureProbeIntervalSeconds`, wired at `ApiKeyFailureLimiter.cs:74-83`); throttle metric tagged by stage only (`Metrics/GatewayMetrics.cs:350-356`, with an explicit SEC-14/SEC-20 cardinality comment). `ApiKeyFailureLimiterTests.cs` exists. The lockout inversion is structurally closed: a correct secret always reaches the verifier via a probe slot and resets both layers. | Probe-claim compensation is version-stamped (`:341-383`), closing the same-tick stomp the follow-up commits (`5b681ee`, `acebe18`) addressed. Two new residual corners found in the rework — see **SEC-38** (aggregate-layer bypass via the per-peer key-id cap) and **SEC-39** (null==null fallback-partition reset). Neither reopens the SEC-31 lockout: they weaken bounds, they do not deny the legitimate holder. |
|
||
| SEC-32 | Done | **Yes** | Token shape checked before a key-id partition is minted: `GatewayGrpcAuthorizationInterceptor.cs:149-193` (`TryResolveKeyId` — `mxgw` must be the entire first segment, key id ≤ 64 chars, non-empty third segment; internal for direct test pinning). Per-peer key-id cap `MaxKeyIdPartitionsPerPeer = 32` with overflow collapsing to the address's fallback partition (`ApiKeyFailureLimiter.cs:48-53`, `:393-444`). Eviction never removes load-bearing state below a 2× ceiling and prefers expired windows (`:479-548`). | The junk-token flush is closed as designed. The cap's overflow-collapse rule has a side effect the design did not anticipate — see **SEC-38**. |
|
||
| SEC-33 | Done | **Yes** | `IsRootedForAnyPlatform` is gone; rooting is host-meaningful `Path.IsPathRooted` in the shared `Configuration/GatewayConfigPathRules.cs:14-39` (used by both validators, with the SEC-01/SEC-33 mechanism named in the doc-comment). `appsettings.json` no longer carries either Windows literal (Authentication section `:20-24` has no `SqlitePath`; Galaxy section `:79-84` has no `SnapshotCachePath`). Gateway-side Galaxy default seeded pre-bind (`GatewayApplication.cs:156-168` — the init-only-property deviation is documented in place) and validated by the new `Configuration/GalaxyRepositoryOptionsValidator.cs:44-72` registered with `ValidateOnStart` (`GatewayApplication.cs:172-176`). No stray `C:*` artifact under `src/` (`find`-verified). | Went beyond the finding: `882c7ca` added `AddIfUnderContentRoot` (`GatewayConfigPathRules.cs:93-124`) after a real 2026-08-09 production key-loss incident (store inside the renamed app directory), and the secrets store got the same absolute default treatment (`GatewayApplication.cs:201-239`). |
|
||
| SEC-34 | Done | **Yes** | Invalidate race closed with the per-key generation counter: bump-before-evict (`Security/Authentication/CachingApiKeyVerifier.cs:181-199`), snapshot-before-inner + set-then-recheck with self-evict (`:128-141`, `:147-178`); `TryParseKeyId` correctness argument (full key id, `_` forbidden by both key-creation paths) documented at `:205-218`. Expiry window took the documented fallback per the recorded outcome: the library verification identity carries no `ExpiresUtc`, so the ≤ TTL expiry window is documented in the remarks (`:49-60`) with the donor-library ask. | Matches the remediation outcome exactly, including the deviation. Cache-key design unchanged (SHA-256 of the token, success-only). |
|
||
| SEC-35 | N/A (doc-only) | **Yes** | `docs/GatewayConfiguration.md:282` — the "hard-stops key on the exact environment name" contract is documented (both guards fire only on `IHostEnvironment.IsProduction()`). Validator unchanged: `GatewayOptionsValidator.cs:38`, `:211-214` (LDAP), `:387-391` (DisableLogin). | Note the consequence is now live: windev runs as `Staging` (NEXT-07 relabel), so neither guard applies there — that is the posture gap promoted as **SEC-37**. |
|
||
| SEC-36 | Done | **Yes (repo side + rotation executed)** | `appsettings.json:33` ships `"ServiceAccountPassword": "${secret:ldap/mxgateway/bind}"` — a fail-closed encrypted-store reference expanded pre-host (`GatewayApplication.cs:75-106`), not a literal. `git grep` for the retired value returns nothing in tracked files. `glauth.md` samples use placeholders; `docs/runbooks/SEC-36-ldap-credential-rotation.md` records the 2026-08-07 rotation (GLAuth `passsha256` replaced in `scadaproj` commit `aada53b`, instance recreated, dashboard `/login` verified through the real `DashboardAuthenticator` path post-NEXT-07). `<UserSecretsId>mxaccessgw-server</UserSecretsId>` present in the csproj; the blank-password validator message names both channels. | The load-bearing half (rotation) is done: the historically recoverable value no longer binds `dc=zb,dc=local`. Fully closed. |
|
||
|
||
### Cycle-1 residuals (still-open SEC-13…SEC-27 era)
|
||
|
||
| ID | Prior status | At HEAD | Evidence | Notes |
|
||
|----|--------------|---------|----------|-------|
|
||
| SEC-13 | Open | **Open, impact reduced** | `Diagnostics/GatewayLogRedactor.cs:11-17` still lists only `AuthenticateUser`/`WriteSecured`/`WriteSecured2` — the `WriteSecuredBulk`/`WriteSecured2Bulk` command kinds (`Grpc/MxAccessGatewayService.cs:364-381`) are still missing. However the redactor is no longer latent: `GatewayLogRedactorSeam` is registered as the telemetry `ILogRedactor` (`GatewayApplication.cs:139`) and masks **every** non-null `CommandValue` because the seam exposes no value-logging opt-in (`Diagnostics/GatewayLogRedactorSeam.cs:38-56`; honestly documented at `docs/Diagnostics.md:120`). The Bulk omission only bites if a future opt-in knob passes `valueLoggingEnabled: true`. | Fix the two missing names when (if) the opt-in knob lands; until then fail-closed. |
|
||
| SEC-14 | Open | **Confirmed open** | `GatewayApplication.cs:347-348` — `MapZbHealth()`/`MapZbMetrics()` with no `RequireAuthorization`. | Mitigating discipline held: the new throttle counter deliberately carries no key/peer tags because "/metrics is not authenticated (open SEC-14)" (`GatewayMetrics.cs:352-354`). Note `/health/active` has off-repo consumers (family Overview dashboard), so any auth change needs cross-repo coordination. |
|
||
| SEC-15 | Open (accepted) | **Confirmed open** | GET `/logout` still skips antiforgery, intentional (`Dashboard/DashboardEndpointRouteBuilderExtensions.cs:92-99`). | Accepted risk; unchanged. |
|
||
| SEC-16 | Open | **Confirmed open** | `Program.cs:37-39` still maps `command.Pepper` → `MxGateway:ApiKeyPepper` (pepper on the CLI command line). | |
|
||
| SEC-17 | Open | **Confirmed open** | `GatewayGrpcAuthorizationInterceptor.cs:27-54` overrides only unary + server-streaming. All current RPCs remain those shapes. | |
|
||
| SEC-18 | Open | **Confirmed open** | `Dashboard/DashboardApiKeyManagementService.cs:22,289` — `"pepper unavailable"` message-substring match. | |
|
||
| SEC-19 | Open | **Resolved** (this period) | `Security/Audit/SqliteCanonicalAuditStore.cs:23-31` — the `CREATE TABLE IF NOT EXISTS` is latched: `AuditDrainService` runs `EnsureInitializedAsync` once at startup and later operations skip the DDL round-trip (self-bootstrap kept for the CLI/DI-free paths; the latch is deliberately racy-idempotent). Landed with the `e2ac5d1`/`7b2d046` audit rework (bounded async writer, batched inserts). | |
|
||
| SEC-21 | Open | **Resolved** (this period) | `Dashboard/DashboardSnapshotService.cs:25,38,42,287-310` — API-key summaries refresh at most every 15 s behind a zero-timeout gate with a 2 s store timeout, and the snapshot tick itself is idle-gated (`77c5731`, `DashboardSnapshotFeed` idle gate). The per-tick unconditional store read is gone. | |
|
||
| SEC-23 | Open | **Confirmed open** | `GatewayOptionsValidator.ValidateDashboard` (`:380-455`) gained GroupToRole/GroupToTag/UntaggedSessionVisibility shape checks and the Production `DisableLogin` stop, but still ignores `AutoLoginUser`, `RequireHttpsCookie`, and `CookieName` — an explicit `CookieName` starting `__Host-` with `RequireHttpsCookie=false` is still applied unconditionally (`DashboardServiceCollectionExtensions.cs:154-158`) with no validator diagnosis. | The runtime guard against *deriving* `__Host-` without Secure is correct (`:159-162`); only the explicit-override consistency check is missing. |
|
||
| SEC-24 | Open | **Confirmed open (narrowed)** | `Configuration/GatewayConfigurationProvider.cs:58-74` / `EffectiveDashboardConfiguration.cs:3-12` — the projection gained `ShowTagValues`, `GroupToRole`, `GroupToTag` (with comparer propagation, `ff16c69`), and `UntaggedSessionVisibility`, and the settings page renders them (`SettingsPage.razor:53-104`); it still omits `DisableLogin`/`AutoLoginUser`/`RequireHttpsCookie`/`CookieName` and the `MxGateway:Security` limiter keys the SEC-31 design asked to absorb here. Secrets stay redacted (`:20,30`). | |
|
||
| SEC-26 | Open | **Resolved** (this period) | Retention sweep exists: `SqliteCanonicalAuditStore.cs:172` (`DELETE FROM audit_event` with a documented NULL-timestamp keep rule at `:251-253`), driven by `SecurityOptions.AuditRetentionDays` (default 90, `SecurityOptions.cs:96-99`) from the audit drain service. | |
|
||
| SEC-27 | Open | **Confirmed open** | `DashboardSnapshotService.cs:17,114` — `GatewayStatus` is still the hardcoded `"Healthy"` constant. | |
|
||
|
||
### SEC-25 epic verification (the tag-ACL machinery is real and fail-closed)
|
||
|
||
Not a table row above because cycle 2 closed SEC-25 as "mitigation, ACL deferred"; the deferred ACL has now shipped and was reviewed as new code:
|
||
|
||
- **Tags come from the owning key, never the request:** `Grpc/MxAccessGatewayService.cs:36-46` (owner identity from `identityAccessor`, `owner?.EffectiveConstraints.DashboardTags` into `OpenSessionAsync`); `Sessions/GatewaySession.cs:207-212,270` (immutable ordinal-ignore-case `FrozenSet`); `Security/Authentication/ApiKeyConstraints.cs:35-44` (defensive copy on init; rides the constraints blob to avoid a schema migration; explicitly visibility-only, never data-access).
|
||
- **ACL decision table:** `Dashboard/DashboardSessionAcl.cs:50-84` — Admin (authenticated) bypass; phantom session denied; untagged session follows `UntaggedSessionVisibility` (default `AdminOnly`, fail-closed); otherwise ordinal-ignore-case tag intersection over `DashboardTagClaimType` claims; `null` principal denied outright, even under `AllViewers` (`IDashboardSessionAcl.cs:25-32`). Anonymous localhost is an empty-grant Viewer.
|
||
- **Both subscribe seams gated identically:** `Dashboard/Hubs/EventsHub.cs:59-79` (ACL before group join, registry-before-group ordering, opaque `HubException`), and the session-details in-process seam `SessionDetailsPage.razor:321-361` (ACL before any field write or `Subscribe`). The `7ec0b35` re-entrancy fix is sound: the attach generation is claimed synchronously before the first await (`:217-233`), a stale continuation returns without attaching or detaching (`:340-343`), and the pump's `ReferenceEquals` batch guard drops cross-session batches on the dispatcher (`:383-407`).
|
||
- **Claims plumbing:** tags stamped at login (`DashboardAuthenticator.cs:114-158`) and re-resolved from group claims at hub-token mint so a token bounds grant staleness to 5 minutes (`HubTokenService.cs:80-98`); token `Validate` still rejects identity-less payloads (`:118-126`) and rehydrates tag claims for the hub-path ACL (`:139-145`).
|
||
- **Owner-scoped gRPC attach (the threat-model anchor) holds:** `Grpc/EventStreamService.cs:52-63` — reattach is `PermissionDenied` unless the caller's key id ordinal-equals the session owner; null owner matches only a null caller.
|
||
- **Redaction (second, independent layer) covers all three value seams:** events-hub mirror deep-clone (`Dashboard/Hubs/DashboardEventBroadcaster.cs:91,247-259` — top-level `value` plus alarm `CurrentValue`/`LimitValue`, source never mutated), alarms hub deep-clone on both value-bearing arms (`AlarmsHubPublisher.cs:59,103-126` — the cycle-2 closeout's named "alarms-hub redaction gap" residual, closed by `eff17d1`/TST-16 on 2026-08-17), and `/browse` live values at the service boundary with the `'-'`-placeholder-on-failed-reads fix (`DashboardLiveDataService.cs:132-140`; `1ea6f60`). `docs/GatewayConfiguration.md:195` documents all three seams and the "redaction applies to successful reads only" rule accurately.
|
||
- **Tests exist for the corners:** `Tests/Gateway/Dashboard/DashboardSessionAclTests.cs`, `EventsHubTests.cs`, `SessionDetailsPageEventAclTests.cs`, `EventsHubViewerRegistryTests.cs`, `HubTokenServiceTests.cs`, `AlarmsHubPublisherTests.cs`, `DashboardGroupTagMappingTests.cs`; live-LDAP ACL proof via `IntegrationTests/DashboardLdapLiveTests.cs:186-243` (real bind → real group claims → GroupToTag → ACL admit/deny, plus Admin bypass).
|
||
- **Docs honesty:** the short-name `GroupToRole`/`GroupToTag` key constraint (full-DN keys can never match the pre-stripped groups) is stated at the mapping site (`DashboardGroupTagMapping.cs:8-13,41-47`) and in the class summaries (`db7b1db`, `4b6f907`).
|
||
|
||
## New findings
|
||
|
||
### SEC-37 · Medium (Security / deployment posture) — The shared GLAuth offers no TLS, so the Production plaintext-LDAP hard-stop is unsatisfiable for every host that authenticates against it (promotion of NEXT-08)
|
||
|
||
**Files:** `src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs:208-214`; `docs/GatewayConfiguration.md:264-265`; `glauth.md:30,357-360`; `archreview/2026-07-12/remediation/90-candidate-findings-next-cycle.md` (NEXT-08).
|
||
|
||
The SEC-06 guard refuses to start a `Production`-environment host with `Ldap:Transport=None`, and the config doc instructs "Deployed hosts must set `Ldaps` or `StartTls`" — but the only shared directory (`10.100.0.35:3893`) has LDAPS disabled, port 3894 closed, and answers StartTLS with `protocolError` (per the NEXT-07/08 record), and `glauth.md:30` documents "LDAPS | disabled in dev". Neither permitted value can work, so the instruction is unsatisfiable: every deployed gateway that binds this directory must either run plaintext (which `glauth.md:358-360` itself warns sends passwords cleartext) or avoid the `Production` label. That is exactly what happened — windev was relabelled `Staging` on 2026-08-07 to boot past the guard. The net posture is that the hard-stop protects no real deployment: hosts that would trip it are renamed around it, and the SEC-35 documentation note now formalizes the escape hatch. The dashboard admin surface (API-key CRUD, session Kill, secrets UI) sits behind credentials that transit this plaintext bind.
|
||
|
||
**Recommendation:** resolve the posture, in one of two written directions: (a) enable LDAPS (or StartTLS) on the shared GLAuth — certificate issuance plus a trust story on each gateway host, source of truth `scadaproj/infra/glauth/` — and flip the deployed hosts' `Transport`; or (b) record an explicit decision that production gateways bind a different, TLS-capable directory and that the shared GLAuth is dev/staging-only, and say so in `docs/GatewayConfiguration.md`'s `Transport` row and `glauth.md`. Until one lands, every "must use Ldaps/StartTls" sentence is aspirational and should be labeled as such. (Cross-repo: coordinate with the family per the umbrella-index rule.)
|
||
|
||
### SEC-38 · Low (Security) — The failure limiter's cross-peer aggregate layer can be sidestepped by pre-filling the per-peer key-id cap, restoring the `limit × addresses` guessing bound the layer was built to remove
|
||
|
||
**Files:** `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs:225-231` (aggregate fed only when `effectiveKeyId` is non-null), `:393-444` (`ResolvePartitionKey` returns the `(peer, null)` fallback once the address has minted `MaxKeyIdPartitionsPerPeer = 32` key ids), `:153-166` (`Check` consults the aggregate only via `effectiveKeyId`).
|
||
|
||
The SEC-32 cap collapses the 33rd-and-later key id from one address onto that address's fallback partition, and — per the comment's cardinality rationale — such failures deliberately do not feed the per-key-id aggregate. The side effect: an attacker who first mints 32 junk `mxgw`-shaped key-id partitions from an address (32 requests) makes every subsequent guess against the *victim's real key id* from that address resolve to `(peer, null)` — so those failures neither feed the victim's aggregate (`RecordFailure`, `:227`) nor consult it (`Check`, `:158-163`). A source-rotating attacker (the IPv6-/64 scenario the aggregate was designed for, per the SEC-31 remediation design's G1 analysis) thereby recovers `ApiKeyFailureLimit` guesses per window per address with the aggregate fully bypassed, at ~32 extra requests per address — instead of the designed bound of `ApiKeyFailureAggregateLimit` total followed by ~1 probe per interval globally. Severity is Low, not Medium: gateway secrets are library-generated high-entropy values, so online guessing remains computationally hopeless and the limiter is defense-in-depth plus store-read cost shielding (which still holds per-address); the finding is that the *documented* distributed-attack bound (`ApiKeyFailureLimiter.cs:17-19`, `docs/Authentication.md` limiter section) is not the actual bound.
|
||
|
||
**Recommendation:** feed (and consult) the aggregate for any *validly shaped* key id regardless of whether it earned a composite partition — the cardinality concern the comment cites is already handled by `_aggregates`' own capped eviction (`:234`), whose policy protects over-limit entries and preferentially evicts single-failure junk. Alternatively, count cap-overflow failures into the aggregate of the presented key id only when that aggregate already exists (zero new cardinality, closes the consult-bypass for hot keys). Update the class remarks and `docs/Authentication.md` either way.
|
||
|
||
### SEC-39 · Info (Security) — `Reset` treats "no key id" as owning the shared fallback partition: a verified token that fails the interceptor's key-id parse clears the address's fallback state the guard comment promises to preserve
|
||
|
||
**Files:** `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs:252-276`; `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs:149-193`.
|
||
|
||
`Reset`'s ownership check is `string.Equals(effectiveKeyId, partition.KeyId)` (`:264`), which is true when both are null — and a partition with `KeyId = null` resolves to the transport-peer fallback key (`:398-403`), so the reset then `RemovePartition(peer)`s the *shared* fallback bucket, the exact wipe the comment above it (`:259-263`) explains must not happen because that bucket holds junk-spray failures from the same address. A null `KeyId` on a *successful* verification requires a token the library verifier accepts but `TryResolveKeyId` rejects — the identifiable shapes are a key id longer than 64 chars (`:178`) and a secret whose first character is `_` (`:187`, matching the old `Split`-based rejection); whether the shared verifier accepts either depends on library parsing not readable in this repo. Practical impact is minimal (the attacker must hold a *valid* key of an unusual shape merely to clear their own address's junk-spray throttle), hence Info — but the null==null branch contradicts the stated invariant and costs one line to close.
|
||
|
||
**Recommendation:** make the fallback non-resettable explicitly: `if (effectiveKeyId is not null && string.Equals(effectiveKeyId, partition.KeyId, StringComparison.Ordinal))`, and pin with a test (successful reset with a null-key-id partition leaves the fallback partition tracked).
|
||
|
||
### SEC-40 · Low (Security) — Alarm visibility is not tag-scoped: the session-less alarms hub (and alarms page) broadcasts galaxy-wide alarm metadata to every Viewer, including empty-grant Viewers and anonymous localhost
|
||
|
||
**Files:** `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Hubs/AlarmsHub.cs:12-25` (`[Authorize(HubClientsPolicy)]`, auto-join `__alarms__` on connect); `AlarmsHubPublisher.cs:48-63` (unfiltered `StreamAsync(alarmFilterPrefix: null)` re-broadcast to all clients); `Dashboard/DashboardAuthorizationHandler.cs:39-53` (loopback/disabled bypasses satisfy the Viewer requirement the hub policy uses); `docs/GatewayDashboardDesign.md:277` ("all clients receive every message").
|
||
|
||
The SEC-25 ACL scopes *session event* visibility by tag intersection, and CLAUDE.md/docs describe dashboard event visibility as tag-scoped — but the alarm feed is a parallel, session-less surface with no per-caller scoping at all: any principal satisfying `HubClientsPolicy` (any Viewer role, the `Authentication:Mode=Disabled` bypass, or anonymous localhost under the default `AllowAnonymousLocalhost=true`) receives every `AlarmFeedMessage` the central monitor emits. Values are stripped by default (`ShowTagValues`, verified above), but the surviving metadata — alarm reference (a full tag path), state, severity, category, area, and acknowledging-operator usernames — discloses process structure and operator activity for the whole subscription expression to callers the tag model would deny every single session. A Viewer with an empty tag grant, whom the ACL was explicitly designed to show nothing, still sees the full plant alarm picture. The alarm monitor being gateway-central (not per-session) means tag scoping does not map mechanically, so this may well be a deliberate posture — but no design doc states it against the ACL: `GatewayDashboardDesign.md:277` describes the fan-out without a scoping rationale, and the ACL plan (`docs/plans/2026-07-10-dashboard-session-acl-tst15.md`) does not carve alarms out.
|
||
|
||
**Recommendation:** decide and document. If deliberate, add one paragraph to the ACL section of `docs/GatewayDashboardDesign.md` (and the `GroupToTag` row of `docs/GatewayConfiguration.md`) stating that alarm metadata is intentionally visible to every dashboard Viewer and why (central monitor, no per-session provenance). If not, the cheap tightening is a dedicated policy on `AlarmsHub` (e.g. require a real role claim, excluding the anonymous-loopback bypass) or an area/tag-prefix filter derived from the caller's grant.
|
||
|
||
### SEC-41 · Low (Security) — Tag grants revoked by a `GroupToTag` config change survive on cookie principals across the restart that applies the change: the ACL trusts login-stamped tag claims for up to the 8-hour sliding cookie life, while the hub-token path re-resolves at mint
|
||
|
||
**Files:** `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardAuthenticator.cs:145-149` (tags stamped as claims at login); `Dashboard/DashboardSessionAcl.cs:75-81` (ACL reads the *stamped* tag claims); `Dashboard/HubTokenService.cs:84-90` (token mint *re-resolves* tags from group claims against current options — the asymmetry); `Dashboard/DashboardServiceCollectionExtensions.cs:114-130` (`ZbCookieDefaults.Apply(..., idleTimeout: 8h)` — sliding, so an active session renews indefinitely).
|
||
|
||
`GatewayOptions` binds once at startup (no reload — noted at `DashboardSnapshotService.cs:45-49`), so changing `GroupToTag` requires a gateway restart. A restart tears down circuits and hub connections, but **not** the auth cookie: data-protection keys persist, and a browser returning after the restart re-authenticates from the cookie, whose `DashboardTagClaimType` claims were resolved under the *old* map at login. `DashboardSessionAcl` consults exactly those claims, so a user whose group's tag grant was removed from config keeps observing the corresponding sessions' event streams — hub joins and the session-details in-process seam alike — until they log out or the sliding 8-hour window finally lapses, renewable indefinitely by activity. The hub-token seam does not have this gap for the *grant* (mint re-resolves from group claims, bounding staleness to 5 minutes, exactly as its comment at `:41-47` claims) — but that comment's promise that "revoking a GroupToTag entry takes effect ... within one lifetime" holds only for token-authenticated connections, and the cookie-authenticated in-process seam it shares an ACL with is the weaker path. LDAP group-membership revocations have the same 8-hour cookie bound on both paths (standard cookie-auth behavior, but worth stating for a *security grant*).
|
||
|
||
**Recommendation:** have the ACL (or a claims transformation) re-resolve tags from the principal's `LdapGroupClaimType` claims against current options — the same one-call `DashboardGroupTagMapping.MapGroupsToTags` the token mint already uses — instead of trusting login-stamped tag claims; the group claims are already on the cookie. That makes a config-revoked grant take effect at the next subscribe after restart, symmetrically with the token path. At minimum, document the revocation bounds (restart + re-login for cookies, 5 minutes for tokens, never for an already-open subscription) in `docs/GatewayDashboardDesign.md`'s ACL section.
|
||
|
||
## Severity roll-up (new findings)
|
||
|
||
| Severity | Count | IDs |
|
||
|----------|-------|-----|
|
||
| Medium | 1 | SEC-37 |
|
||
| Low | 3 | SEC-38, SEC-40, SEC-41 |
|
||
| Info | 1 | SEC-39 |
|
||
|
||
Prior-backlog movement this period: SEC-19, SEC-21, SEC-26 resolved; SEC-31…SEC-36 all verified Done (SEC-36 including the operator rotation); still open from cycle 1: SEC-13 (impact reduced — redactor now wired fail-closed), SEC-14, SEC-15 (accepted), SEC-16, SEC-17, SEC-18, SEC-23, SEC-24 (narrowed), SEC-27.
|
||
|
||
## Still good / preserved
|
||
|
||
The load-bearing controls all survive at HEAD, several strengthened:
|
||
|
||
- **Fail-closed authorization core:** unrecognized request → `admin` scope (`GatewayGrpcScopeResolver`); opaque auth failures that never disclose the failing stage (`GatewayGrpcAuthorizationInterceptor.cs:96-109`); constant-time compare stays inside the shared verifier on every cache miss with only SHA-256 digests in memory (`CachingApiKeyVerifier.cs:36-39,284-286`); the throttle message reveals nothing about secret validity.
|
||
- **The SEC-25 ACL is genuinely fail-closed on every branch** (unknown session, empty grant, untagged-session default `AdminOnly`, null principal), enforced identically on both subscribe seams, with the decision-table corners and the live-LDAP group-shape proof pinned by tests; session tags are immutable and inherited only from the owning key's constraints, never the client request; the gRPC event-stream attach stays owner-scoped (`EventStreamService.cs:52-63`).
|
||
- **Read-only environment bypasses stay read-only:** the `Disabled`-mode and anonymous-loopback bypasses satisfy only requirements containing Viewer, never `AdminOnly` (`DashboardAuthorizationHandler.cs:37-53`); API-key CRUD and session Close/Kill keep their service-layer Admin re-checks (`DashboardApiKeyAuthorization.cs:10-18`, `DashboardSessionAdminService`) beneath the policy layer; the secrets UI mounts behind its own `secrets:manage` policy (`GatewayApplication.cs:147-150`), which the dashboard bypass handler cannot satisfy (different requirement type).
|
||
- **Value redaction is now a complete, tested, three-seam system** (events mirror, alarms hub, `/browse`), clone-only so gRPC clients and the replay ring are untouched, with the config doc telling the truth about all three seams and the failed-read placeholder rule.
|
||
- **Secret handling improved materially:** the committed LDAP literal is gone and rotated; config secrets ride fail-fast `${secret:}` references expanded pre-host; the effective-config projection redacts pepper name and LDAP password; hub tokens remain 5-minute, data-protected, identity-required; no new call site logging credentials, tag values, or grant contents was found in the changed files (the ACL logs decisions, never tag values — `DashboardAuthenticator.cs:145-146`).
|
||
- **Credential-store paths are now defended in depth:** host-meaningful rooting, the under-content-root rule born of a real outage, and absolute defaults for all three stores (auth DB, secrets store, Galaxy snapshot) via one shared rule set (`GatewayConfigPathRules`).
|
||
- **Dashboard hygiene held:** local Bootstrap only (no UI component libraries in any new page); antiforgery on POST login/logout; `SanitizeReturnUrl` still blocks open redirects (`DashboardEndpointRouteBuilderExtensions.cs:238-249`); the login limiter stays remote-IP-partitioned; the viewer registry / snapshot feed / broadcaster concurrency reworks (`EventsHubViewerRegistry.cs`, `DashboardSnapshotFeed.cs`, `DashboardEventBroadcaster.cs`) are careful, generation-guarded code with the subtle orderings documented in place and covered by dedicated tests.
|