docs(archreview): 2026-07-12 follow-up review + remediation designs
Six-domain re-review at the P2 merge (4f5371f): all prior Done claims
verified (none false, 10 partial), 47 new findings (1 High, 14 Medium),
with per-finding design/implementation plans and a new tracking register.
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,214 @@
|
||||
# Security & Dashboard — Remediation Design & Implementation (2026-07-12 cycle)
|
||||
|
||||
Source review: [40-security-dashboard.md](../40-security-dashboard.md) · Baseline: `main` @ `4f5371f` · Generated: 2026-07-13
|
||||
|
||||
This document turns the six NEW findings of the 2026-07-12 security/dashboard re-review (SEC-31…SEC-36) into buildable remediation entries, in the same per-finding format as the prior cycle's [archreview/remediation/40-security-dashboard.md](../../remediation/40-security-dashboard.md). Tiers follow the [00-overall.md](../00-overall.md) roadmap: SEC-31/32 are the P0 failure-limiter re-partition (roadmap item 3), SEC-33/36 ride the P1 residual sweep (item 8), SEC-34 is a P2 Low (item 9), SEC-35 is doc-only. The still-open prior backlog is referenced where a fix should co-locate: **SEC-23** (validator coverage of dashboard/cookie flags) shares the validator pass with SEC-33, **SEC-24** (effective-config view omissions) should absorb the new `MxGateway:Security` keys introduced here, and **SEC-14** (unauthenticated `/metrics`) constrains what the new limiter telemetry may expose.
|
||||
|
||||
Repo rules that bind every entry: docs change in the same commit as the source (`CLAUDE.md`), never log or commit secrets (this document references the committed LDAP credential **by location only**, never by value), `TreatWarningsAsErrors=true` (new public members need XML docs), and targeted `dotnet test --filter` runs per task — not the full suite.
|
||||
|
||||
## Register
|
||||
|
||||
| ID | Sev | Tier | Eff | Dep | Status | Title |
|
||||
|----|-----|------|-----|-----|--------|-------|
|
||||
| SEC-31 | Medium | P0 | M | — | Not started | Failure limiter partitions on attacker-controlled key id and blocks before verification (lockout DoS) |
|
||||
| SEC-32 | Low | P0 | S | SEC-31 | Not started | Failure-limiter LRU is flushable by junk-token spray; token prefix never validated |
|
||||
| SEC-33 | Low | P1 | M | — (co-locate SEC-23) | Not started | Any-platform path-rooting acceptance re-opens SEC-01 on Unix; Galaxy `SnapshotCachePath` unvalidated |
|
||||
| SEC-34 | Low | P2 | S | — | Not started | Verification cache: expiry outlives TTL; `Invalidate` races in-flight repopulation |
|
||||
| SEC-35 | Info | — | S | — | N/A (doc-only note) | Production hard-stops key on the exact `Production` environment name |
|
||||
| SEC-36 | Low | P1 | M | cross-repo (`scadaproj/infra/glauth`) | Not started | Committed dev LDAP service-account password: remove from repo and rotate |
|
||||
|
||||
---
|
||||
|
||||
## SEC-31 — Failure limiter partitions on attacker-controlled key id and blocks before verification `Medium` · `P0`
|
||||
|
||||
**Finding.** `ResolvePeerKey` derives the limiter partition from the **unauthenticated** presented token — `parts[1]` of `mxgw_<keyId>_<secret>` (`src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs:115-136`) — and `IsBlocked` short-circuits with `ResourceExhausted` *before* `VerifyAsync` runs (`:72-78`). The success-path `Reset` (`:97`) is only reachable after a verification, which a blocked partition never gets. Defaults: 10 failures per 60 s window (`src/ZB.MOM.WW.MxGateway.Server/Configuration/SecurityOptions.cs:51,57`), limiter internals at `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs:65-114`.
|
||||
|
||||
**Impact.** Key ids are not secrets: they are embedded in every token, shown to every dashboard Viewer, and pushed to anonymous-loopback hub clients in the snapshot's API-key inventory. Any network peer that knows (or enumerates) a key id can send 10 garbage-secret requests per minute and deny that key **indefinitely** — the legitimate client presenting the *correct* secret is rejected before its credential is ever checked, and because rejection precedes verification, the reset that would clear the block is unreachable. The NAT-caveat comment (`GatewayGrpcAuthorizationInterceptor.cs:113-114`) optimized for not locking out co-located clients and instead handed remote attackers a per-key kill switch. Ten packets per minute is all it costs to keep it held down forever.
|
||||
|
||||
**Design.** This is a re-partition plus an admission-valve redesign. The limiter exists for two guarantees that any fix must preserve: (G1) online secret-guessing against a key is bounded (~`ApiKeyFailureLimit` per window), and (G2) the failure path does not spend a SQLite read per attempt (failures are never cached by `CachingApiKeyVerifier`, so without the pre-verify short-circuit every guess hits the store). The new guarantee is (G3): no unauthenticated input may indefinitely deny a key to a holder of the correct secret.
|
||||
|
||||
*Options considered:*
|
||||
|
||||
1. **Pure transport-peer partition** (`context.Peer`, mirroring the dashboard login limiter). Satisfies G3 — an attacker only blocks their own address. Rejected as the sole partition for two reasons: **NAT sharing** — all clients behind one NAT share one budget, so a single fat-fingering (or malicious) neighbor locks the address for every key (the exact scenario the current keying was built to avoid, per `glauth.md`'s NAT caveat); and **IPv6 rotation** — an attacker holding a /64 mints fresh source addresses at will, so the per-key guessing bound (G1) collapses to `limit × addresses`, i.e. effectively unbounded.
|
||||
|
||||
2. **Post-verification-only failure counting** (verify first, never block pre-verify). Satisfies G3 by construction — the correct secret always reaches the constant-time compare. Rejected as the sole mechanism because it abandons G2 entirely: every failed guess costs the full library verify (SQLite read + peppered hash + compare), which is precisely the resource the limiter shields (the interceptor comment at `:67-71` states this as the design intent).
|
||||
|
||||
3. **Composite `(transport peer, key id)` partition** for the pre-verify check. An attacker's spam only trips (their address × that key); the legitimate holder on any other address is untouched, and co-located NAT clients using *different* keys are untouched. Rejected as the sole mechanism because IPv6 rotation again weakens G1: `limit` guesses per window per minted address.
|
||||
|
||||
4. **Chosen: composite partition + per-key-id aggregate, with probe admission instead of absolute blocking.** Two layers over one shared window mechanism:
|
||||
- **Layer 1 — composite `(peer, keyId)` partition**, checked pre-verify as today. Bounds per-address guessing at `ApiKeyFailureLimit`/window and preserves G2 for the common single-source case.
|
||||
- **Layer 2 — per-key-id aggregate**: failures for a (validly-shaped, see SEC-32) key id are also counted across *all* peers. When the aggregate exceeds `ApiKeyFailureAggregateLimit` (new, default 30/window), the key id enters **probe mode**.
|
||||
- **Probe admission**: an over-limit state (either layer) is not an absolute wall — at most one request per `ApiKeyFailureProbeIntervalSeconds` (new, default 5) is admitted through to the real verifier; all others still short-circuit with `ResourceExhausted`. A successful verification resets both the composite partition and the aggregate (making the success-reset path *reachable while throttled*, which is the structural fix for the lockout inversion).
|
||||
|
||||
*Resulting bounds:* a single attacking peer gets `limit`/window + 1 probe per interval (≈ 10 + 12 per minute at defaults) — the same order as the current intent. A distributed/rotating attacker gets at most `ApiKeyFailureAggregateLimit` guesses in the first window before the aggregate trips, then ~1 probe per interval **globally for that key** — which is *stronger* than option 3 and within the same order as today's per-key bound (G1 holds). A legitimate holder presenting the correct secret authenticates immediately from any un-tripped address, and under active spray recovers within a small number of probe intervals (it competes for probe slots, but a win resets all state; the attacker must then re-trip the aggregate from scratch). Residual accepted: a high-rate distributed sprayer can *delay* (not deny) the legitimate holder — but that now requires sustained, metrically-visible attack traffic instead of 10 packets/min, and G3's "indefinitely" is eliminated.
|
||||
|
||||
*Also rejected:* trusting the key id after a cheap local check (any format check is attacker-satisfiable — the design must assume key ids are public); allowlisting known key ids by querying the store pre-verify (reintroduces the per-attempt store read G2 forbids).
|
||||
|
||||
*Telemetry note:* add throttle counters tagged only by stage (`peer`/`aggregate`) — never by key id or peer address (cardinality per the SEC-20 precedent, and `/metrics` is still unauthenticated per open **SEC-14**, so counters must carry no key material). When prior-cycle **SEC-24** (effective-config view) is picked up, the new `MxGateway:Security` keys added here belong in that projection.
|
||||
|
||||
**Implementation.**
|
||||
|
||||
1. `src/ZB.MOM.WW.MxGateway.Server/Configuration/SecurityOptions.cs`: add `int ApiKeyFailureAggregateLimit { get; init; } = 30;` (0 disables the aggregate layer) and `int ApiKeyFailureProbeIntervalSeconds { get; init; } = 5;` (0 disables probe admission → absolute block, documented as not recommended). Full XML docs (warnings-as-errors).
|
||||
2. `src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs` `ValidateSecurity`: `AddIfNegative` for both new keys, with messages naming the config paths (`MxGateway:Security:ApiKeyFailureAggregateLimit`, `...:ApiKeyFailureProbeIntervalSeconds`).
|
||||
3. `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs`: rework the public surface from `IsBlocked/RecordFailure/Reset(string peer)` to a partition-pair API, e.g. `ThrottleDecision Check(ApiKeyThrottlePartition partition)` / `RecordFailure(partition)` / `Reset(partition)` where `ApiKeyThrottlePartition` carries `TransportPeer` (required) and `KeyId` (optional, only when the token is validly shaped per SEC-32). Internal state: `_partitions` keyed `"{peer}|{keyId}"` (or `"{peer}"` fallback) and `_aggregates` keyed by key id, both reusing the existing sliding-window `PeerState` plus a `NextProbeAtTicks` field; probe slots are granted under the per-state lock. Keep the `TimeProvider` seam and the internal test constructor. Rewrite the class remarks — the current NAT rationale (`:13-26`) describes the defective keying.
|
||||
4. `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs`: replace `ResolvePeerKey` with a `ResolveThrottlePartition(authorizationHeader, context)` that returns the pair (with the SEC-32 shape check); thread the probe-admission decision through `AuthenticateAndAuthorizeAsync` (a probe-admitted request proceeds to `VerifyAsync`; a throttled one throws `ResourceExhausted` with the existing opaque message). `RecordFailure`/`Reset` take the partition pair. Update the comment block at `:67-78,112-114`.
|
||||
5. Optional but recommended: a `mxgateway.auth.throttled` counter in `Metrics/GatewayMetrics.cs` tagged `stage=peer|aggregate` only.
|
||||
6. Docs, same commit: rewrite `docs/GatewayConfiguration.md:282-284` (the three existing limiter rows describe key-id keying and "resets on success" semantics that change here) and add rows for the two new keys; update the failure-limiter paragraph in `docs/Authentication.md` (hot-path section, ~`:85-115`) to describe the two-layer partition and probe admission.
|
||||
7. Tests: new `src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs` (window pruning, composite-vs-aggregate trip points, probe cadence, reset clears both layers); update the two existing limiter-related interceptor tests (`GatewayGrpcAuthorizationInterceptorTests.cs:363-460`) for the new API.
|
||||
|
||||
New tests (names are the contract):
|
||||
- `AttackerSpamOnVictimKeyId_FromDifferentPeer_DoesNotBlockLegitimateHolderPresentingCorrectSecret` — flood failures for key id K from peer A until blocked; a call from peer B with the correct secret for K verifies successfully on the first attempt (assert the verifier **was** called and the RPC succeeded).
|
||||
- `BruteForceBound_StillEnforcedPerAttackingPeer` — same peer, same key id: attempt `limit`+1 wrong secrets inside the window; assert the final attempt maps to `ResourceExhausted` **before** the verifier is called (reuse `CountingFailureVerifier`).
|
||||
- `AggregateSpray_AcrossManyPeers_TripsPerKeyProbeMode` — failures for one key id from > `AggregateLimit` distinct peers; assert subsequent attempts from a fresh peer are probe-limited (at most one verifier call per probe interval, others `ResourceExhausted`).
|
||||
- `CorrectSecret_DuringProbeMode_AuthenticatesViaProbeSlotAndResets` — while throttled, advance the fake `TimeProvider` past the probe interval; the correct secret is admitted, succeeds, and fully resets the state (next wrong attempt is `Unauthenticated`, not `ResourceExhausted`).
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` then:
|
||||
```
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~ApiKeyFailureLimiter"
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayGrpcAuthorizationInterceptor"
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayOptionsValidator"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## SEC-32 — Failure-limiter LRU is flushable by junk-token spray; token prefix never validated `Low` · `P0` · depends on SEC-31
|
||||
|
||||
**Finding.** The tracked-peer map evicts the least-recently-active entry once it exceeds `ApiKeyFailureTrackedPeers` (default 4096) — `src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs:124-148` — and the partition key is minted from attacker-chosen token text with **no prefix check**: any `a_b_c`-shaped garbage creates a fresh `key:b` partition (`GatewayGrpcAuthorizationInterceptor.cs:127-132`; the `mxgw` literal is never compared).
|
||||
|
||||
**Impact.** A guesser interleaves real guesses with ~4096 unique throwaway tokens to evict their own `PeerState` (or a SEC-31 victim's blocked state) and reset the window, resuming past the 10-per-minute bound at a cost of ~4096 requests per flush cycle. The documented guarantee ("a spray of unique peer keys cannot grow memory without limit", `SecurityOptions.cs:59-64`) is true but silently trades away the *blocking* guarantee it exists to serve.
|
||||
|
||||
**Design.** Three reinforcing changes, landed inside the SEC-31 rework (same types, same tests, one commit train):
|
||||
|
||||
1. **Validate token shape before minting a key-id partition.** Only a token whose first segment is the literal `mxgw`, with ≥ 3 non-empty `_`-segments and a key id within a sane length cap (≤ 64 chars — generated key ids are far shorter), gets a `KeyId` on its throttle partition. Everything else falls to the transport-peer fallback partition — so a junk spray from one address accumulates in **one** partition (which then throttles that address wholesale) instead of minting thousands.
|
||||
2. **Cap key-id partitions per transport peer.** SEC-31's composite keying already binds every partition to the sender's address, but a single address can still mint one partition per unique `mxgw`-shaped key id it invents. Cap distinct key-id partitions per transport peer at a small constant (32; not config — there is no legitimate reason for one address to fail against dozens of distinct keys inside one window); overflow collapses into that address's fallback partition. This bounds the total partitions one attacker can mint to 33 per source address.
|
||||
3. **Eviction never removes load-bearing state.** `EvictIfOverCapacity` preference order: (a) entries whose windows are empty (fully expired) first; (b) never evict an entry that is currently over-limit / in probe mode — such entries decay naturally within `ApiKeyFailureWindowSeconds`, so the exemption cannot pin memory indefinitely; (c) otherwise least-recently-active. Allow a documented transient overshoot of `ApiKeyFailureTrackedPeers` for non-evictable over-limit entries with a hard ceiling of 2× the cap (beyond which oldest-blocked is evicted anyway — availability of the bound beats perfection of the block under a distributed attack that large).
|
||||
|
||||
*Rejected alternatives:* validating the key id against the store pre-verify (reintroduces the per-attempt SQLite read); a cryptographic/entropy check on the key-id segment (attacker-satisfiable, adds nothing over the length/prefix check); simply raising `ApiKeyFailureTrackedPeers` (changes the flush price, not the defect).
|
||||
|
||||
**Implementation.**
|
||||
|
||||
1. `GatewayGrpcAuthorizationInterceptor.ResolveThrottlePartition` (from SEC-31 step 4): add the `mxgw` prefix + segment + key-id-length checks; on failure return a partition with `KeyId = null`.
|
||||
2. `ApiKeyFailureLimiter`: per-transport-peer key-id partition counter (cap 32, collapse to fallback); eviction policy per design point 3 (`EvictIfOverCapacity` rewrite; over-limit detection reuses the same window state the `Check` path computes).
|
||||
3. Update the limiter remarks and `SecurityOptions.ApiKeyFailureTrackedPeers` XML doc (the "cannot grow memory without limit" sentence gains the "and cannot be flushed" half plus the 2× transient note).
|
||||
4. Docs, same commit: `docs/GatewayConfiguration.md` `ApiKeyFailureTrackedPeers` row — describe the eviction preference and the flush resistance.
|
||||
5. Tests (in `ApiKeyFailureLimiterTests` / `GatewayGrpcAuthorizationInterceptorTests`):
|
||||
- `NonMxgwToken_FallsBackToTransportPeerPartition` — junk tokens of varied shapes all land on the sender's fallback partition (assert one tracked entry).
|
||||
- `JunkTokenSpray_DoesNotEvictBlockedEntry` — trip a block for (peer A, key K); spray 2× `TrackedPeers` unique tokens from other peers; assert (A, K) is still blocked.
|
||||
- `UniqueMxgwKeyIdSpray_FromOnePeer_CollapsesAtPerPeerCap` — 1000 unique `mxgw`-shaped key ids from one address mint ≤ 32 + 1 partitions and end up throttling the address's fallback partition.
|
||||
- `Eviction_PrefersExpiredWindows` — with the map at capacity, a new failure evicts an entry whose window has fully expired, not an active one.
|
||||
|
||||
**Verification.** Same filters as SEC-31 (`~ApiKeyFailureLimiter`, `~GatewayGrpcAuthorizationInterceptor`) after `dotnet build src/ZB.MOM.WW.MxGateway.Server`.
|
||||
|
||||
---
|
||||
|
||||
## SEC-33 — Any-platform path-rooting acceptance re-opens SEC-01 on Unix; Galaxy `SnapshotCachePath` unvalidated `Low` · `P1` · co-locate with open SEC-23
|
||||
|
||||
**Finding.** `IsRootedForAnyPlatform` (`src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs:541-560`) deliberately passes Windows drive-qualified/UNC paths on Unix so the shipped `appsettings.json` validates cross-platform. But the validator runs in the same process that then *uses* the path: on Unix, `C:\ProgramData\MxGateway\gateway-auth.db` (`appsettings.json:17`) contains no Unix separator, SQLite treats it as one relative filename, and the credential store lands in the CWD — the original SEC-01 mechanism, now behind a validator message promising it "never lands in the launch working directory" (`:112`). Evidence at HEAD: a stray auth DB dated 2026-07-09 (post-SEC-01-fix) under `src/ZB.MOM.WW.MxGateway.Tests/bin/Debug/net10.0/` with the literal Windows path as its filename, invisible to the hygiene test's bin/obj filter (`src/ZB.MOM.WW.MxGateway.Tests/ProjectStructure/GatewayTreeHygieneTests.cs:30-35`). Additionally Galaxy `SnapshotCachePath` (`appsettings.json:80`) gets **no** rooting validation: the shared `ZB.MOM.WW.GalaxyRepository` package binds it (`GatewayApplication.cs:103`) and deliberately ships no validator — per `A2-galaxyrepository-adoption-handoff.md:157-160` the gateway was supposed to own that validation, and doesn't.
|
||||
|
||||
**Impact.** Any non-Windows run (dev box, tests, future container) that binds the shipped config silently writes the API-key credential store — and the Galaxy snapshot — as junk-named files relative to whatever the CWD happens to be. The validator's guarantee is false on the platform where it matters; the hygiene test cannot see the failure because the artifacts land in gitignored build output.
|
||||
|
||||
**Design.** Make rooting **host-meaningful** and stop shipping foreign-platform literals, instead of teaching the validator to bless paths the process cannot use:
|
||||
|
||||
1. **Validator checks the running OS.** `AddIfNotRooted` uses plain `Path.IsPathRooted` (current platform); delete `IsRootedForAnyPlatform`. A Windows drive path on Unix becomes a fail-fast startup error naming the offending key — exactly how every other misconfiguration surfaces. The "validate prod config offline on macOS" rationale in the doc-comment dissolves with change 2, and no offline-validation tooling exists that would need the any-platform mode.
|
||||
2. **Remove the Windows literals from `appsettings.json`.** Drop `Authentication:SqlitePath` (`:17`) and `Galaxy:SnapshotCachePath` (`:80`); the `SpecialFolder.CommonApplicationData`-derived code defaults (SEC-01's own mechanism, `Configuration/AuthenticationOptions.cs:16-19`) resolve to the **identical** `C:\ProgramData\MxGateway\...` on Windows and a platform-appropriate path elsewhere. Deployed hosts are unaffected: production config rides NSSM environment variables and the Windows code default is byte-identical to the removed literal. For Galaxy, add a gateway-side `PostConfigure` default (`Path.Combine(CommonApplicationData, "MxGateway", "galaxy-snapshot.json")`) applied when the bound value is blank.
|
||||
3. **Gateway-side Galaxy options validation.** New `Configuration/GalaxyRepositoryOptionsValidator.cs` registered as `IValidateOptions<GalaxyRepositoryOptions>` (the package type — the lib binds only, by design): when `PersistSnapshot` is true, `SnapshotCachePath` must be non-blank, a valid path, and rooted on the current OS (reuse the `AddIfInvalidPath`/`AddIfNotRooted` helpers — promote them to a shared internal helper if the new validator can't reach them).
|
||||
4. **Root-cause the stray file.** Identify the test/tool run that bound the shipped `appsettings.json` and materialized the DB under the test bin dir; pin it to a temp path (the `TempDatabaseDirectory` helper exists in `Tests/Security/Authentication/`). After changes 1–2 the failure mode is loud (validation error) rather than silent, and on macOS the code default may point at an unwritable `/usr/share/...` — any test exercising the real startup path must override `SqlitePath`. Delete the stray gitignored file.
|
||||
|
||||
*Rejected alternatives:* auto-rooting/rewriting foreign-platform paths to a host default (silent relocation of a credential store — worse than a boot error, per the SEC-01 design note at `GatewayOptionsValidator.cs:515-521`); keeping `IsRootedForAnyPlatform` in the validator and adding a second use-site runtime check (two checks to keep aligned, and the validator's promise stays false); extending the hygiene test to scan bin/obj (treats the symptom; build output is transient).
|
||||
|
||||
**Implementation.**
|
||||
|
||||
1. `Configuration/GatewayOptionsValidator.cs`: `AddIfNotRooted` → `Path.IsPathRooted`; delete `IsRootedForAnyPlatform` (`:533-560`) and its doc comment.
|
||||
2. `src/ZB.MOM.WW.MxGateway.Server/appsettings.json`: remove the `SqlitePath` and `SnapshotCachePath` lines (code defaults take over).
|
||||
3. New `Configuration/GalaxyRepositoryOptionsValidator.cs` (+ registration next to `AddZbGalaxyRepository` in `GatewayApplication.cs:99-103`) and a `PostConfigure<GalaxyRepositoryOptions>` default for `SnapshotCachePath`.
|
||||
4. Find and fix the offending test per design point 4; `rm` the stray `.../bin/Debug/net10.0/C:\ProgramData\MxGateway\gateway-auth.db`.
|
||||
5. Docs, same commit: `docs/GatewayConfiguration.md` — `SqlitePath` / `SnapshotCachePath` rows now document the derived per-OS default and the rooted-on-this-host rule; `docs/Authentication.md` default-path sentence if it names the Windows literal.
|
||||
6. Tests: `Tests/Configuration/GatewayOptionsValidatorTests.cs` — on the running platform, a foreign-platform-rooted path fails (`C:\x\y.db` on Unix; assert via the injected message), a host-rooted path passes, a bare filename fails; new `GalaxyRepositoryOptionsValidatorTests` — `PersistSnapshot=true` + blank/relative path → invalid, disabled persistence skips the check, blank path gets the PostConfigure default.
|
||||
|
||||
Note for the executor: this pass touches the same `ValidateDashboard`-adjacent validator file as still-open **SEC-23** (cookie/`__Host-`/`AutoLoginUser` coverage) — if SEC-23 is picked up this cycle, land them as one validator commit.
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` (on macOS this also proves the shipped config still boots validation-clean after the literal removal) then:
|
||||
```
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayOptionsValidator"
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GalaxyRepositoryOptionsValidator"
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayTreeHygiene"
|
||||
```
|
||||
Post-run, verify no new `C:\*` file exists under any `bin/` (manual `find src -name 'C:*'`).
|
||||
|
||||
---
|
||||
|
||||
## SEC-34 — Verification cache: expiry outlives TTL; `Invalidate` races in-flight repopulation `Low` · `P2`
|
||||
|
||||
**Finding.** Three bounded staleness windows in `src/ZB.MOM.WW.MxGateway.Server/Security/Authentication/CachingApiKeyVerifier.cs`: (1) a CLI-issued revoke (separate process) keeps authenticating ≤ TTL — documented (`:41-47`); (2) a key whose `ExpiresUtc` passes while cached keeps working ≤ 15 s past expiry (`SecurityOptions.cs:21`) — expiry is enforced by the inner library verifier, which a cache hit never reaches, and this case is **not** in the documented backstop rationale; (3) race: a `VerifyAsync` that passed the inner verifier just before a dashboard revoke can `_cache.Set` (`:110-117`) *after* `Invalidate(keyId)` ran (`:123-137` — no generation check), re-caching the revoked identity for a full TTL despite the "gateway-initiated mutations take effect immediately" contract (`:11-13`).
|
||||
|
||||
**Impact.** All three are bounded (≤ 15 s; ~30 s worst case for the race), so exposure is small — but (2) silently contradicts the SEC-10 expiry feature's semantics, and (3) contradicts the class's own stated contract, which is the kind of gap the next review flags as "documented behavior is false".
|
||||
|
||||
**Design.**
|
||||
|
||||
- **Expiry (window 2): eliminate, don't document.** The shared `ApiKeyIdentity` (0.1.4) carries `ExpiresUtc`; when caching a success, cap the entry lifetime at the key's expiry: `AbsoluteExpiration = min(now + ttl, ExpiresUtc)` (skip caching entirely if already ≤ now). A cached hit can then never outlive the key. Confirm during implementation that the library verifier populates `ExpiresUtc` on the returned identity; if it does not, fall back to documenting the ≤ TTL window in the remarks and `docs/Authentication.md` and file a donor-library ask.
|
||||
- **Invalidate race (window 3): per-key generation check.** `ConcurrentDictionary<string, long> _generations`; `Invalidate(keyId)` increments the generation **before** evicting cache keys. `VerifyAsync` parses the key id from the token up front (same split the interceptor does — cheap, no store access), snapshots `g0` before calling the inner verifier, and after a success only `Set`s when the generation still equals `g0` — then re-reads the generation after the `Set` and self-evicts if it moved (bump-before-evict + set-then-recheck closes the remaining interleaving). Unparseable tokens skip caching already (`TryComputeCacheKey`).
|
||||
- **CLI window (1): accept and keep documented** — cross-process invalidation is out of scope by design; the TTL is the backstop and the remarks already say so.
|
||||
|
||||
*Rejected alternatives:* a lock around verify+set (serializes the auth hot path the cache exists to speed up); switching to a distributed/shared cache (massive machinery for a 15 s window); "accept and document" for the race (viable and cheap, but the generation counter is ~20 lines and makes the stated contract true — prefer honest code over honest caveats).
|
||||
|
||||
**Implementation.**
|
||||
|
||||
1. `Security/Authentication/CachingApiKeyVerifier.cs`: expiry-capped `MemoryCacheEntryOptions` in `VerifyAsync` (`:110-117`); `_generations` + snapshot/recheck logic; `Invalidate` bumps generation first (`:123-137`). Update the remarks (`:29-47`): add the expiry cap and the generation mechanism; keep the CLI-window caveat.
|
||||
2. Docs, same commit: `docs/Authentication.md` hot-path caching section (~`:92-105`) — one sentence each for the expiry cap and the invalidate-vs-in-flight guarantee.
|
||||
3. Tests, `src/ZB.MOM.WW.MxGateway.Tests/Security/Authentication/CachingApiKeyVerifierTests.cs`:
|
||||
- `CacheEntry_DoesNotOutliveKeyExpiry` — identity expiring 5 s from now with a 15 s TTL: hit at +4 s served from cache, attempt at +6 s reaches the inner verifier.
|
||||
- `AlreadyExpiredIdentity_IsNotCached` — inner returns success with `ExpiresUtc` ≤ now (defensive); nothing is cached.
|
||||
- `Invalidate_DuringInFlightVerification_DiscardsStaleRepopulation` — inner verifier gated on a `TaskCompletionSource`; call `Invalidate(keyId)` while `VerifyAsync` is awaiting inner, then release; assert the follow-up `VerifyAsync` calls the inner verifier again (no stale cache hit).
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server` then:
|
||||
```
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~CachingApiKeyVerifier"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## SEC-35 — Production hard-stops key on the exact `Production` environment name `Info` · `—` (N/A: doc-only)
|
||||
|
||||
**Finding.** The `DisableLogin` and plaintext-LDAP guards fire only when `IHostEnvironment.IsProduction()` (`src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs:23-27,161-165,314-318`) — i.e. `ASPNETCORE_ENVIRONMENT` unset (defaults to Production; the NSSM-deployed hosts are covered) or exactly `Production`. A host launched as `Staging`, `Prod`, or any custom name silently keeps the permissive dev posture. The fallback-environment wiring (`Configuration/GatewayConfigurationServiceCollectionExtensions.cs:24,38-51`) is correct.
|
||||
|
||||
**Impact.** None at current deployments; a latent surprise for a future operator who invents an environment name and assumes the guards travel with it.
|
||||
|
||||
**Design.** N/A for code, by deliberate acceptance: the review itself rates this acceptable-as-designed, ASP.NET Core's environment-name semantics are the platform convention, and inverting to "not Development ⇒ production-like" would hard-stop legitimate permissive staging rigs (the shared dev GLAuth is plaintext-only, so a `Staging` host pointed at it would refuse to boot) — a behavior change with real downside for an Info-severity conventions note. The remediation is the documentation contract the review asked for.
|
||||
|
||||
**Implementation.** `docs/GatewayConfiguration.md` (environment/hard-stop area, near `:244-254`): one short paragraph stating that the two production hard-stops key on `IHostEnvironment.IsProduction()` — the exact `Production` name or an unset `ASPNETCORE_ENVIRONMENT` — and that any other environment name retains the permissive dev posture, so production-like deployments must use the literal `Production`. Same-commit rider on whichever SEC-33/SEC-36 change touches that doc first.
|
||||
|
||||
**Verification.** Doc-only; no build. Cross-read against `GatewayOptionsValidator.cs:23-27`.
|
||||
|
||||
---
|
||||
|
||||
## SEC-36 — Committed dev LDAP service-account password: remove from repo and rotate `Low` · `P1` · cross-repo dependency
|
||||
|
||||
**Finding.** The SEC-06 remediation shipped the production transport hard-stop and the env-var override documentation, but the dev LDAP service-account credential itself is still committed at HEAD: `src/ZB.MOM.WW.MxGateway.Server/appsettings.json:29` (`Ldap:ServiceAccountPassword`), with the same value repeated in `glauth.md`'s samples (`:33,65,102-103,135-136,245`) and acknowledged as "should be rotated" by `docs/GatewayConfiguration.md:248`. (Value intentionally not reproduced here.)
|
||||
|
||||
**Impact.** As long as the shared GLAuth instance (`10.100.0.35:3893`) honors this credential, the repo — and its full git history — discloses a live directory service account with LDAP search capability over `dc=zb,dc=local`. Removal alone is insufficient: the value is permanently recoverable from history, so **rotation is the load-bearing half** of the fix.
|
||||
|
||||
**Design.** Rotate first, then remove; give dev a supported secret channel so the removal doesn't break the local login flow.
|
||||
|
||||
- **Rotation (cross-repo).** The GLAuth source of truth is `scadaproj/infra/glauth/` (per `glauth.md`; note `scadaproj` is a shared monorepo — stage explicit paths only). Generate a new service-account secret there, redeploy GLAuth on `10.100.0.35`, and record the rotation (date, not value) in `glauth.md`. The new value must never be committed to *this* repo in any file — `glauth.md`'s samples switch to a `<service-account-password>` placeholder with a pointer at the source of truth.
|
||||
- **Removal.** Delete the `ServiceAccountPassword` line from `appsettings.json`. The validator already fails a blank password when `Ldap.Enabled=true` (`GatewayOptionsValidator.cs:134-137`), so a host without the secret fails fast at startup instead of failing binds at runtime — extend that validation message to name the two supported channels.
|
||||
- **Where the dev secret lives instead.** Two supported channels, both flowing through the existing `MxGateway:Ldap:ServiceAccountPassword` binding:
|
||||
1. **Deployed hosts (unchanged):** the env-var override `MxGateway__Ldap__ServiceAccountPassword` set in the NSSM service environment — already the documented production mechanism (`docs/GatewayConfiguration.md:248`) and already how deployed config works.
|
||||
2. **Dev boxes (new):** .NET user-secrets — add `<UserSecretsId>` to `ZB.MOM.WW.MxGateway.Server.csproj`; `WebApplicationBuilder` loads user-secrets automatically in Development, so `dotnet user-secrets set "MxGateway:Ldap:ServiceAccountPassword" <value>` (value obtained from the GLAuth source of truth) is a one-time per-machine step. Secrets live under the user profile, outside the tree.
|
||||
- **Cutover order** (avoids a broken window): set the env var on the deployed hosts with the *new* value → rotate GLAuth → verify dashboard login on the deployed hosts → land the repo change (removal + docs) → devs set user-secrets on next pull (startup error message tells them exactly what to do). Check the second deployment (`wonder-app-vd03`) for an LDAP binding before assuming it needs the var (its dashboard is disabled; if `Ldap.Enabled=false` there, nothing to do).
|
||||
|
||||
*Rejected alternatives:* committing a placeholder string instead of deleting the key (passes blank-validation with a wrong value and fails later at bind time — a silent misconfiguration where fail-fast is available); `appsettings.Development.json` (still a committed file — same defect relocated); rotating prod-only and keeping the dev credential committed (the finding *is* the committed live dev credential); encrypting the value in config (key-management theater for a dev secret with a working out-of-band channel).
|
||||
|
||||
**Implementation.**
|
||||
|
||||
1. *(cross-repo, first)* Rotate in `scadaproj/infra/glauth/` and redeploy GLAuth; pre-stage `MxGateway__Ldap__ServiceAccountPassword` in the NSSM environment on `10.100.0.48` (and `wonder-app-vd03` only if LDAP is enabled there); verify dashboard login post-rotation.
|
||||
2. `src/ZB.MOM.WW.MxGateway.Server/appsettings.json:29`: delete the `ServiceAccountPassword` entry.
|
||||
3. `src/ZB.MOM.WW.MxGateway.Server/ZB.MOM.WW.MxGateway.Server.csproj`: add `<UserSecretsId>mxaccessgw-server</UserSecretsId>`.
|
||||
4. `Configuration/GatewayOptionsValidator.cs:134-137`: extend the blank-password message to point at user-secrets (dev) and the env-var override (deployed).
|
||||
5. Docs, same commit as 2–4: `docs/GatewayConfiguration.md:248` — replace the "dev-only committed value / should be rotated" wording with the two channels and the rotation note; `glauth.md` — placeholder in all samples (`:33,65,102-103,135-136,245`), rotation recorded, pointer to `scadaproj/infra/glauth/`; `CLAUDE.md`'s `glauth.md` summary line if it references the credential.
|
||||
6. Repo scrub check: `git grep` for the old value across tracked files must return nothing (run manually — do not encode the secret into a test).
|
||||
|
||||
**Verification.** `dotnet build src/ZB.MOM.WW.MxGateway.Server`; then
|
||||
```
|
||||
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~GatewayOptionsValidator"
|
||||
```
|
||||
(asserts the blank-password validation still fires with the updated message). Manual: with user-secrets set on the dev box, `dotnet run --project src/ZB.MOM.WW.MxGateway.Server/...` and a dashboard `/login` as `multi-role` succeeds against the rotated GLAuth; the deployed-host login re-check from step 1 counts as the production verification. Live-LDAP integration tests (`MXGATEWAY_RUN_LIVE_LDAP_TESTS=1`) only where the GLAuth instance is reachable; otherwise document skipped per the testing matrix.
|
||||
Reference in New Issue
Block a user