dohertj2/mxaccessgw
1
0
Fork 0
Code Issues 14 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix(SEC-33,SEC-34): host-meaningful path rooting; verification-cache invalidate race

Browse Source 
SEC-33: make rooting host-meaningful and stop shipping foreign-platform literals.
- Delete IsRootedForAnyPlatform; AddIfNotRooted now uses Path.IsPathRooted (current OS).
- Promote AddIfNotRooted/AddIfInvalidPath to shared GatewayConfigPathRules so the new
  Galaxy validator reuses them and the two validators cannot drift.
- Remove Authentication:SqlitePath and Galaxy:SnapshotCachePath Windows literals from
  appsettings.json; the CommonApplicationData-derived code defaults take over. The
  Galaxy default is seeded as a configuration value before AddZbGalaxyRepository
  (SnapshotCachePath is init-only, so a PostConfigure mutation cannot compile).
- New GalaxyRepositoryOptionsValidator (ValidateOnStart) enforces a valid, host-rooted
  SnapshotCachePath when PersistSnapshot is true.
- Root-cause the stray junk-named auth DB: host start eagerly builds
  AuthSqliteConnectionFactory; under the non-rooted Windows literal on macOS SQLite
  wrote it relative to the test bin CWD. The three real-host-start tests now pin
  SqlitePath to a temp path.

SEC-34: verification cache Invalidate-vs-in-flight-repopulation race closed with a
per-key generation counter (bump-before-evict, snapshot-then-recheck). The expiry
cap (window 2) takes the documented fallback: the library verification identity
carries no ExpiresUtc, so the cache cannot cap at the key's expiry (donor-library ask).

GWC-24 rider: cap MxGateway:Events:QueueCapacity at int.MaxValue/2 so the derived
checked(2 * EventChannelCapacity) in WorkerClient cannot overflow at session creation.

SEC-35 (doc-only): note IsProduction() env-name semantics in GatewayConfiguration.md.

Docs updated same commit (GatewayConfiguration.md, Authentication.md) and tracking
registers/change-log flipped (00-tracking.md, 40-security-dashboard.md).
This commit is contained in:
Joseph Doherty
2026-08-07 06:36:01 -04:00
parent 3f854d6cbf
commit 7e7f7cad84
15 changed files with 548 additions and 112 deletions
Download Patch File Download Diff File Expand all files Collapse all files
archreview/2026-07-12/remediation/00-tracking.md
+4 -3
View File
@@ -101,9 +101,9 @@ Full design + implementation for each row lives in the linked domain doc under i
|---|---|:-:|:-:|---|---|---|
| SEC-31 | Medium | P0 | M | — | Done | Failure limiter: composite (peer, key-id) partitions + cross-peer aggregate with probe admission |
| SEC-32 | Low | P0 | S | SEC-31 | Done | Limiter LRU flushable by junk-token spray; validate token shape, cap per-peer partitions |
| SEC-33 | Low | P1 | M | old SEC-23 (co-locate) | Not started | Host-meaningful path rooting; drop Windows literals from appsettings; validate Galaxy `SnapshotCachePath` |
| SEC-34 | Low | P2 | S | — | Not started | Verification cache: expiry outlives TTL; `Invalidate` races in-flight repopulation |
| SEC-35 | Info | — | S | — | N/A | Production hard-stops key on exact `Production` environment name (doc-only) |
| SEC-33 | Low | P1 | M | old SEC-23 (co-locate) | Done | Host-meaningful path rooting; drop Windows literals from appsettings; validate Galaxy `SnapshotCachePath` |
| SEC-34 | Low | P2 | S | — | Done | Verification cache: expiry outlives TTL; `Invalidate` races in-flight repopulation |
| SEC-35 | Info | — | S | — | N/A (doc note discharged 2026-08-07) | Production hard-stops key on exact `Production` environment name (doc-only) |
| SEC-36 | Low | P1 | M | cross-repo `scadaproj/infra/glauth` | Not started | Committed dev LDAP service-account password: rotate, remove, move dev channel to user-secrets |
### Clients — [50-clients.md](50-clients.md)
@@ -168,4 +168,5 @@ Sequence these together rather than piecemeal — several are one change set spa
| 2026-08-07 | Code review of `fix/gwc-26-27-alarm-attach` surfaced a **known pre-existing characteristic, now documented**: the alarm monitor's reconcile-derived feed repairs are **at-least-once, not exactly-once**. A reconcile reads the worker's current state while the matching live transition may still be buffered in the monitor's internal lease, so both broadcast and the duplicates are indistinguishable on the alarm feed (`StreamAlarms` + dashboard alarm hub). This pre-dates GWC-26 — the Raise/Clear presence repair has always had it, since nothing serializes a reconcile pass against the in-flight live stream — so closing it (reconcile/live serialization or transition-timestamp dedup) was ruled out of scope for a P2 fix. Documented instead in `GatewayAlarmMonitor.ApplyReconcile`, `gateway.md`, and `docs/Sessions.md`, with the consumer-side contract stated explicitly (apply transitions idempotently — "set this alarm to this state", never increment/toggle). **Candidate finding for the next review cycle.** |
| 2026-08-07 | **CLI-37 + CLI-38 -> `Done`** (branch `fix/cli-37-38-conformance`), one cross-client conformance commit; **closes old-tracker CLI-08**. Canonical rules landed everywhere: an `MxStatusProxy` entry fails iff `category != MX_STATUS_CATEGORY_OK` (`success` is the raw COM member, diagnostics only; absent entry = success, present entry with `UNSPECIFIED` = failure), and a reply fails on HRESULT iff `hresult` is present and `< 0` (so `S_FALSE = 1` passes). Edits: .NET `MxStatusProxyExtensions.IsSuccess` (drop the `Success != 0` conjunct) + `MxCommandReplyExtensions` (`!= 0` -> `< 0`); Go `StatusSucceeded` (category) + `errors.go` (`< 0`); Java `MxStatuses.succeeded` (category, Javadoc corrected) + `MxGatewayErrors` (`< 0`); Python `errors.py` (category); Rust `ensure_mxaccess_success` (category, doc comment corrected). Four shared fixtures added under `clients/proto/fixtures/behavior/command-replies/` (`write.status-category-{error-success-set,ok-success-zero}.reply.json`, `write.hresult-{s-false,e-fail}.reply.json`) + manifest + `docs/ClientBehaviorFixtures.md`; each of the five suites now runs all four fixture-driven, plus a per-language table test for the two edges fixtures cannot express (nil/null entry, `UNSPECIFIED` category). Docs same commit: `ClientLibrariesDesign.md` per-item rule sentence (its existing HRESULT `< 0` claim is now true), .NET/Go/Java README error sections. Also fixed a Java test fake that built a status with a bare `setSuccess(1)` and no category. Verification: dotnet build 0 warnings + 110 passed/1 skipped; `gofmt -l` clean, `go build ./...`, `go test ./...` all ok; `gradle test` BUILD SUCCESSFUL with **no** generated-file churn to revert this time (no `.proto` changed and `generateProto` stayed up to date); `python -m pytest` 155 passed/1 skipped; `cargo fmt` (no unrelated reformat), `cargo check`, `cargo test --workspace` 100 passed, `cargo clippy --all-targets -- -D warnings` clean. Gateway-side `ClientBehaviorFixtureTests` 8/8 re-run because the new fixtures are validated there. |
| 2026-08-07 | **CLI-45 → `Done`** on `fix/cli-45-credential-envvar`. All five CLIs now share one credential contract for `authenticate-user`: flags `--password` / `--password-env` (Go: `-password` / `-password-env`) defaulting to env `MXGATEWAY_VERIFY_PASSWORD`, resolution flag-then-env, and a resolved credential that is missing **or empty** is a usage error naming the flag and the variable — never the value, never sent to the wire. Go and Java previously authenticated with an empty password: Go now returns the guard error before dialing, Java throws a picocli `ParameterException` instead of falling back to `""`. Python's `--password-env` gained the canonical default (its `UsageError` was already conformant) and its message now names the resolved variable. Rust treats an empty `--password` or empty env value as missing (resolution extracted into a testable `resolve_verify_user_password`). .NET adopted the canonical flags and keeps its pre-existing names as **deprecated aliases for one release** — order: `--password`, `--verify-user-password`, the variable named by `--password-env` (or the deprecated `--verify-user-password-env`; default `MXGATEWAY_VERIFY_PASSWORD`), then `MXGATEWAY_VERIFY_USER_PASSWORD`. Tests: `TestRunAuthenticateUser{RejectsEmptyPassword,ReadsPasswordFromCanonicalEnv}` (Go), 3 picocli cases (Java), 3 click cases (Python), 2 clap/resolver cases (Rust), 4 xUnit cases covering the canonical flag, both env-name paths, the deprecated flag+env aliases, and the missing/empty failure (.NET). Docs same commit: `docs/CrossLanguageSmokeMatrix.md` gained a "Credential contract for `authenticate-user`" section **and** the per-CLI subcommand-coverage table — the half of this finding that is documented rather than fixed (.NET exposes all nine single-item session commands; Rust `unregister` + the credential pair; Go/Python/Java the credential pair only; verified against each dispatch table, and every gap is CLI surface only since all five *libraries* implement all nine helpers). All five client READMEs name the canonical variable and the fail-fast rule; the .NET README gained an `authenticate-user` credentials section carrying the deprecation note. **Deviation:** Java keeps `isBlank()` (per this design's "null or blank" wording for Java) where the other four test emptiness, so a whitespace-only credential is additionally rejected there. Verification (all five, on macOS): Go `gofmt -l .` clean, `go build ./...` clean, `go test ./...` ok; Java `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` BUILD SUCCESSFUL, CLI suite 51 tests / 0 failures — **no generated-tree churn appeared this run**, `git status` for `clients/java/**/generated` clean with no revert needed (no `.proto` changed); Python `python -m pytest` 148 passed / 1 skipped (TLS opt-in); .NET `dotnet build …Client.slnx` 0 warnings / 0 errors and client tests 108 passed / 1 skipped (live-gateway opt-in); Rust `cargo fmt` (diff confined to the new code), `cargo check --workspace`, `cargo test --workspace` 100 tests across 6 targets all green, `cargo clippy --all-targets -- -D warnings` clean. |
| 2026-08-07 | **SEC-33 + SEC-34 → `Done`, SEC-35 discharged (doc-only)** (branch `fix/sec-33-34`). SEC-33: `IsRootedForAnyPlatform` deleted and rooting made host-meaningful (`Path.IsPathRooted`); `AddIfNotRooted`/`AddIfInvalidPath` promoted to a shared `GatewayConfigPathRules` helper; both `C:\ProgramData\...` literals (`Authentication:SqlitePath`, `Galaxy:SnapshotCachePath`) removed from `appsettings.json` so the `CommonApplicationData`-derived code defaults take over; new `GalaxyRepositoryOptionsValidator` (`ValidateOnStart`) enforces a valid, host-rooted `SnapshotCachePath` when `PersistSnapshot`; the Galaxy default is seeded as a configuration value before `AddZbGalaxyRepository` (deviation: the package's `SnapshotCachePath` is init-only, so a `PostConfigure` mutation would not compile — same effect). Stray-file root cause: host start eagerly builds `AuthSqliteConnectionFactory`, which under the Windows literal materialized a junk-named relative auth DB under the test `bin/` on macOS; the three real-host-start tests now pin `SqlitePath` to a temp path (`find src -name 'C:*'` empty). SEC-34: window-3 `Invalidate` race fixed with a per-key generation counter (bump-before-evict, snapshot-then-recheck); window-2 expiry cap took the **documented fallback** because the library verification identity carries no `ExpiresUtc` (donor-library ask recorded) — so only `Invalidate_DuringInFlightVerification_DiscardsStaleRepopulation` was added, not the two expiry-cap tests. GWC-24 rider: `MxGateway:Events:QueueCapacity` gained an `int.MaxValue/2` upper bound so `checked(2 * EventChannelCapacity)` in `WorkerClient` cannot overflow at session creation (+ two validator tests). SEC-35: doc note added to `docs/GatewayConfiguration.md` (`IsProduction()` env-name semantics). Docs same commit: `GatewayConfiguration.md`, `Authentication.md`. Evidence (macOS): `dotnet build …Server` 0 warnings/0 errors; `--filter ~GatewayOptionsValidator` 69/69, `~GalaxyRepositoryOptionsValidator` 5/5, `~CachingApiKeyVerifier` 10/10, `~GatewayTreeHygiene` 1/1. |
| 2026-08-07 | **SEC-31 + SEC-32 → `Done`** (branch `fix/sec-31-32-limiter`, one change set as planned). `ApiKeyFailureLimiter` reworked from `IsBlocked/RecordFailure/Reset(string peer)` to a partition-pair API (`Check/RecordFailure/Reset(ApiKeyThrottlePartition)` returning `ApiKeyThrottleDecision`): layer 1 is the composite `(transport peer, key id)` partition, layer 2 a per-key-id aggregate across peers (`ApiKeyFailureAggregateLimit`, default 30), and an over-limit state now admits one probe per `ApiKeyFailureProbeIntervalSeconds` (default 5) instead of blocking absolutely — so a success can reset the state while throttled, killing the 10-packets-per-minute lockout. SEC-32 rides along: the interceptor validates token shape (`mxgw` prefix, ≥3 non-empty `_` segments, key id ≤ 64 chars) before minting a key-id partition, each peer may mint at most 32 of them (overflow collapses to its fallback partition), and eviction prefers expired windows, never dropping an over-limit partition below a 2× transient overshoot ceiling. New counter `mxgateway.auth.throttled` tagged `stage=peer\|aggregate` only (no key material — `/metrics` is still unauthenticated per open SEC-14). Docs updated in the same commit (`docs/GatewayConfiguration.md` limiter rows + two new keys, `docs/Authentication.md` hot-path paragraph, `docs/Authorization.md` SEC-11 section, limiter/`SecurityOptions` XML remarks). Evidence: `dotnet build …Server` clean; `--filter ~ApiKeyFailureLimiter` 11/11 passed (new `ApiKeyFailureLimiterTests`), `--filter ~GatewayGrpcAuthorizationInterceptor` 20/20 passed (incl. the four SEC-31 contract tests and `NonMxgwToken_FallsBackToTransportPeerPartition`), `--filter ~GatewayOptionsValidator` 66/66 passed. Full suite on macOS: 804 passed / 44 failed — all 44 are the pre-existing named-pipe fake-worker classes (`WorkerClientTests`, `FakeWorkerHarnessTests`, `SessionWorkerClientFactoryFakeWorkerTests`, `GatewayEndToEnd*`), verified identical (44) on the unmodified tree. Follow-up unchanged: the new `MxGateway:Security` keys belong in old **SEC-24**'s effective-config projection when that is picked up. **Code review of the branch found two defects in the first pass, both fixed before merge:** (1) probe admission was check-then-act across two lock scopes, so a burst arriving at an interval boundary could all observe "due" and all be admitted — the claim is now a single critical section (`TryConsumeProbe`), and because the two layers are claimed one at a time, a slot claimed on the partition is compensated (`ReleaseProbe`) when the aggregate then refuses; (2) `Reset` on a success whose key id had been collapsed into the address's shared fallback partition removed that shared partition, letting one authentication wipe an in-progress spray from the same address — it is now left to decay by window expiry, while the key's aggregate is still cleared. Tests added: `ProbeAdmission_UnderConcurrentArrivals_GrantsExactlyOneSlot`, `ProbeAdmission_WhenAggregateRefuses_ReturnsTheClaimedPeerSlot`, `Reset_WithOverCapKeyId_DoesNotClearSharedFallbackPartition`. **A second review pass found a residual defect in that compensation path:** the release identified its own reservation by comparing `NextProbeAtTicks` to `now + interval`, the identical expression a failure re-arm writes — so a concurrent `RecordFailure` on the same state sharing a clock tick (routine at ~1 ms resolution) was mistaken for the caller's own claim and stomped back to the stale, already-due value, prematurely reopening the probe slot. Replaced with a monotonic per-state `ProbeVersion` bumped by every writer of `NextProbeAtTicks` (claim and re-arm alike); the release restores only when the version still matches the one its claim stamped, and bumps it again on restore so no other stale release can match. Covered by `ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick`, made deterministic by a new `internal ProbeReleaseInterleaveHook` test seam (null in production, one null check on the refused path) because the claim-to-release window is nanoseconds wide and racing threads cannot hit it reliably — verified as a genuine red against the timestamp guard (`Expected: ThrottledByPeer / Actual: ProbeAdmitted`). Limiter suite 11 → 15. |
archreview/2026-07-12/remediation/40-security-dashboard.md
+9 -3
View File
@@ -12,9 +12,9 @@ Repo rules that bind every entry: docs change in the same commit as the source (
|----|-----|------|-----|-----|--------|-------|
| SEC-31 | Medium | P0 | M | — | Done | Failure limiter partitions on attacker-controlled key id and blocks before verification (lockout DoS) |
| SEC-32 | Low | P0 | S | SEC-31 | Done | 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-33 | Low | P1 | M | — (co-locate SEC-23) | Done | Any-platform path-rooting acceptance re-opens SEC-01 on Unix; Galaxy `SnapshotCachePath` unvalidated |
| SEC-34 | Low | P2 | S | — | Done | Verification cache: expiry outlives TTL; `Invalidate` races in-flight repopulation |
| SEC-35 | Info | — | S | — | N/A (doc-only note discharged 2026-08-07) | 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 |
---
@@ -135,6 +135,8 @@ dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --fil
```
Post-run, verify no new `C:\*` file exists under any `bin/` (manual `find src -name 'C:*'`).
**Outcome (2026-08-07 — Done).** Implemented as designed. `IsRootedForAnyPlatform` deleted; `AddIfNotRooted`/`AddIfInvalidPath` promoted to a shared `GatewayConfigPathRules` internal helper (used by both validators) and now use `Path.IsPathRooted` (current OS). Both Windows literals removed from `appsettings.json`; the Galaxy `SnapshotCachePath` default is applied gateway-side via a configuration value seeded before `AddZbGalaxyRepository` (the package's `SnapshotCachePath` is **init-only**, so a `PostConfigure` mutation does not compile — deviation from the design's "PostConfigure default"; same effect). New `GalaxyRepositoryOptionsValidator` registered with `ValidateOnStart`. **Stray-file root cause:** starting the full host eagerly constructs `AuthSqliteConnectionFactory`, which creates the auth DB path; with the shipped Windows literal that path is non-rooted on macOS, so SQLite materialized `C:\ProgramData\MxGateway\gateway-auth.db` as a junk-named relative file under the test's `bin/` CWD (invisible to the hygiene test's bin/obj filter). After the literal removal the code default resolves under an unwritable `/usr/share` on macOS, so the three tests that start the real host (`GatewayApplicationTests.Build_MapsMetricsEndpoint`, `.StartAsync_InvalidGatewayConfiguration_FailsStartup`, `GatewayTlsBootstrapTests`) now pin `SqlitePath` to a temp path. No stray file remains (`find src -name 'C:*'` empty).
---
## SEC-34 — Verification cache: expiry outlives TTL; `Invalidate` races in-flight repopulation `Low` · `P2`
@@ -165,6 +167,8 @@ Post-run, verify no new `C:\*` file exists under any `bin/` (manual `find src -n
dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --filter "FullyQualifiedName~CachingApiKeyVerifier"
```
**Outcome (2026-08-07 — Done).** Window 3 (Invalidate race) implemented exactly as designed: per-key generation counter, bump-before-evict in `Invalidate`, snapshot-before-inner + set-then-recheck in `VerifyAsync`, key id parsed from the token up front. Covered by `Invalidate_DuringInFlightVerification_DiscardsStaleRepopulation`. **Window 2 (expiry cap) took the documented fallback**, not the cap: the design's confirmation step failed — the library verification identity (`ZB.MOM.WW.Auth.Abstractions.ApiKeys.ApiKeyIdentity`, the type on `ApiKeyVerification.Identity`) carries **no** `ExpiresUtc` (that property is on `ApiKeyRecord`, the store row, not the returned identity), so the cache cannot cap an entry at the key's expiry. Per the design's contingency, the ≤ TTL expiry window is documented in the class remarks and `docs/Authentication.md`, with a donor-library ask (surface expiry on the verification identity). Consequently the two expiry-cap tests (`CacheEntry_DoesNotOutliveKeyExpiry`, `AlreadyExpiredIdentity_IsNotCached`) are **not** added — they cannot be written against a type with no expiry field; window 1 (CLI) accepted and documented as before.
---
## SEC-35 — Production hard-stops key on the exact `Production` environment name `Info` · `—` (N/A: doc-only)
@@ -179,6 +183,8 @@ dotnet test src/ZB.MOM.WW.MxGateway.Tests/ZB.MOM.WW.MxGateway.Tests.csproj --fil
**Verification.** Doc-only; no build. Cross-read against `GatewayOptionsValidator.cs:23-27`.
**Outcome (2026-08-07 — discharged).** The documentation contract landed as a rider on the SEC-33/34 commit: `docs/GatewayConfiguration.md` gained a "Production hard-stops key on the exact environment name (SEC-35)" subsection stating that both hard-stops fire only on `IHostEnvironment.IsProduction()` (unset `ASPNETCORE_ENVIRONMENT` or the exact `Production` name) and that any other name keeps the permissive dev posture. No code change, as designed.
---
## SEC-36 — Committed dev LDAP service-account password: remove from repo and rotate `Low` · `P1` · cross-repo dependency