diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index bc9c721..fe9be4f 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -161,4 +161,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-07-13 | Operator bring-up complete: dedicated CI ed25519 key installed in windev `administrators_authorized_keys` (authorized into `dohertj2`, which owns the working MXAccess/toolchain env — a fresh OS account would break the build; the key is independently revocable), Gitea secrets `WINDEV_SSH_KEY`/`WINDEV_SSH_KNOWN_HOSTS` + variable `WINDEV_SSH_USER=dohertj2` stored, runner→`10.100.0.48:22` egress verified on the `traefik` net, issue-write confirmed. **TST-25/TST-26 → `Done`:** credentialed `windows-x86` ran GREEN on `d769244` (Gitea run #37) — Linux runner SSHed windev, checked out the SHA in `C:\build\mxaccessgw-ci` under lock, ran the x86 Worker build + `Worker.Tests`, exit 0; `nightly-windev` correctly skipped on the push event. Branch merged to `main`. Follow-ups (old tracker): revisit **TST-05** (scheduled live smoke — now covered by `nightly-windev`) and **TST-24** (client wire tests) which this unlocks. | | 2026-07-13 | Ran the TST-25 acceptance checks (scripts/ci/README.md) — they caught **two real CI defects, both fixed** on `fix/tst-25-ci-key-log-leak`: (1) **CI SSH key leaked in cleartext** in the `windows-x86` step env echo (Gitea's line-oriented masker missed the multiline PEM) — rotated the CI key on windev (old pubkey revoked), stored the key **base64-encoded** so the masker redacts it to `***` (confirmed on run #38), taught `run-windev-ci.sh` to decode, dropped the redundant public known-hosts secret from the job env; (2) **bootstrap lock race** — `run-windev-ci.sh`'s pre-hand-off `git fetch`/`checkout` ran outside the worktree lock, so concurrent runs collided on `.git/index.lock`; the bootstrap now holds the lock (ps1 re-uses it via `MXGW_CI_LOCK_HELD`), retest confirmed clean serialization. Also **deflaked** `SessionManagerTests` fail-fast timing assertions (absolute `<100ms` wall-clock bound flaked under CI load; now anchored to the configured timeout / dropped for the zero-timeout case). Checks passed: unreachable-host fast-fail (exit 255/15s), deliberate-red propagation (Worker.Tests failure → exit 1), lock concurrency (2nd run waits), no-key-in-logs (masked). Merge target `df7e20d` verified GREEN via the local windev path (Worker build + 356 tests); merged to `main` `19cbf7b`. Check 6 (forced-failure nightly issue): issue endpoint+token proven live at bring-up (#124); in-CI forced-failure probe abandoned to shared-runner congestion (residual `if: failure()` gating is standard Actions). | | 2026-07-13 | New finding **TST-30** (`Low`/`P2`) added — surfaced during TST-25 acceptance: CI runs on a single shared `gitea-runner` (`maxParallel=1`, co-located `10.100.0.35`) interleaved with `dohertj2/lmxopcua`, and Gitea 1.26 exposes no run cancel/delete, so queue latency is unbounded under cross-repo contention and the runner is a single point of failure. Design: add a second/labelled runner + document the no-cancel reality and the `run-windev-ci.sh` bypass. Roll-ups updated (Testing Low 2→3, total 47→48; P2 9→10). | -| 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` (limiter suite 11 → 14). | +| 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. | diff --git a/docs/Authorization.md b/docs/Authorization.md index 52f1765..abb8dc3 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -96,7 +96,7 @@ Before the verification store read, the helper asks a cheap in-process failure c - **Composite `(transport peer, key id)` partitions.** Reaching `MxGateway:Security:ApiKeyFailureLimit` failures binds the throttle to the address that produced them. The key id alone is never the partition: key ids are not secret — they ride in every token and are listed on the dashboard — so keying on them let any network peer deny a key to its legitimate holder. The key id joins the partition only after a token-shape check (literal `mxgw` prefix, at least three non-empty `_` segments, key id of at most 64 characters), and one address may mint at most 32 key-id partitions before the overflow collapses onto that address's fallback partition. - **A per-key-id aggregate** across all peers (`ApiKeyFailureAggregateLimit`, default 30), which bounds a distributed or source-rotating sprayer that never trips any single partition. -An over-limit state is a valve, not a wall: one request per `ApiKeyFailureProbeIntervalSeconds` (default 5 s) is admitted through to the real verifier, and everything else is refused with `StatusCode.ResourceExhausted` before the store read. The slot is claimed atomically, so a burst arriving together at an interval boundary still yields exactly one admission. A successful verification resets both layers — which is why the reset path stays reachable while a key is under active spray. One exception: when the caller's key id was collapsed into its address's shared fallback partition by the per-peer cap, a success clears the key's aggregate but leaves that shared partition alone, since it also holds failures contributed by other key ids from the same address. The tracked partitions form a bounded LRU (`ApiKeyFailureTrackedPeers`) whose eviction prefers fully expired windows and never removes an over-limit partition below a 2x transient overshoot ceiling, so the cap bounds memory without becoming a reset button for an active block. `ResourceExhausted` reveals only that throttling is in effect, not whether any particular secret was valid, preserving the opaque-failure property. Refusals increment `mxgateway.auth.throttled`, tagged `stage=peer|aggregate` and nothing else — `/metrics` is unauthenticated, so neither key ids nor peer addresses may appear there. +An over-limit state is a valve, not a wall: one request per `ApiKeyFailureProbeIntervalSeconds` (default 5 s) is admitted through to the real verifier, and everything else is refused with `StatusCode.ResourceExhausted` before the store read. The slot is claimed atomically, so a burst arriving together at an interval boundary still yields exactly one admission, and a slot claimed for a request that a later layer then refuses is handed back under a per-state version stamp — never by timestamp comparison, which collides whenever a concurrent failure re-arms the same state on the same clock tick. A successful verification resets both layers — which is why the reset path stays reachable while a key is under active spray. One exception: when the caller's key id was collapsed into its address's shared fallback partition by the per-peer cap, a success clears the key's aggregate but leaves that shared partition alone, since it also holds failures contributed by other key ids from the same address. The tracked partitions form a bounded LRU (`ApiKeyFailureTrackedPeers`) whose eviction prefers fully expired windows and never removes an over-limit partition below a 2x transient overshoot ceiling, so the cap bounds memory without becoming a reset button for an active block. `ResourceExhausted` reveals only that throttling is in effect, not whether any particular secret was valid, preserving the opaque-failure property. Refusals increment `mxgateway.auth.throttled`, tagged `stage=peer|aggregate` and nothing else — `/metrics` is unauthenticated, so neither key ids nor peer addresses may appear there. The dashboard login surface is throttled independently: `POST /auth/login` carries a fixed-window ASP.NET Core rate-limiter policy keyed per remote IP (`MxGateway:Security:LoginRateLimit*`), rejecting a burst with HTTP 429 before the LDAP bind is relayed to the directory. See [GatewayConfiguration](./GatewayConfiguration.md#security-options). diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs index db5131d..ad28237 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs @@ -112,6 +112,18 @@ public sealed class ApiKeyFailureLimiter /// Gets the number of tracked per-key-id aggregates. Test seam. internal int TrackedAggregateCount => _aggregates.Count; + /// + /// Test seam invoked between a probe claim and its compensating release; + /// in production, where it costs one null check on the already-refused path. + /// + /// + /// The window this straddles is a few nanoseconds wide, so the interleaving it exists to cover — + /// a concurrent failure re-arming the same state between the two — cannot be produced reliably + /// by racing threads. Without this seam the version guard in would be + /// verifiable only by inspection. + /// + internal Action? ProbeReleaseInterleaveHook { get; set; } + /// Decides whether an authentication attempt may reach the verifier. /// The throttle partition derived from the request. /// The admission decision for this attempt. @@ -150,11 +162,11 @@ public sealed class ApiKeyFailureLimiter // Every over-limit layer must yield its probe slot for the request to pass. The slots are // claimed one at a time (holding two per-state locks at once would need a global ordering to // stay deadlock-free), so a claim is reserved and then compensated if a later layer refuses. - long peerProbeRestore = 0; + ProbeClaim peerClaim = default; bool peerProbeClaimed = false; if (peerOver) { - if (!TryConsumeProbe(peerState!, now, out peerProbeRestore)) + if (!TryConsumeProbe(peerState!, now, out peerClaim)) { return ApiKeyThrottleDecision.ThrottledByPeer; } @@ -166,7 +178,8 @@ public sealed class ApiKeyFailureLimiter { if (peerProbeClaimed) { - ReleaseProbe(peerState!, now, peerProbeRestore); + ProbeReleaseInterleaveHook?.Invoke(); + ReleaseProbe(peerState!, peerClaim); } return ApiKeyThrottleDecision.ThrottledByAggregate; @@ -264,10 +277,13 @@ public sealed class ApiKeyFailureLimiter state.LastActivityTicks = now; // Arm (or push out) the probe slot whenever the state is at or over its limit, so the - // attempt that trips the limit is not itself followed by an immediate free probe. + // attempt that trips the limit is not itself followed by an immediate free probe. This + // is a write of NextProbeAtTicks, so it bumps the version that identifies a probe claim + // — otherwise a release could mistake this re-arm for its own reservation. if (limit > 0 && state.FailureTicks.Count >= limit) { state.NextProbeAtTicks = now + _probeIntervalTicks; + state.ProbeVersion++; } } } @@ -292,35 +308,47 @@ public sealed class ApiKeyFailureLimiter /// arriving at the interval boundary observe "due" and all be admitted, which is exactly the /// unbounded-guessing burst the probe interval exists to prevent. /// - private bool TryConsumeProbe(WindowState state, long now, out long previousProbeAtTicks) + private bool TryConsumeProbe(WindowState state, long now, out ProbeClaim claim) { lock (state) { - previousProbeAtTicks = state.NextProbeAtTicks; + long previousProbeAtTicks = state.NextProbeAtTicks; if (now < previousProbeAtTicks) { + claim = default; return false; } state.NextProbeAtTicks = now + _probeIntervalTicks; state.LastActivityTicks = now; + claim = new ProbeClaim(previousProbeAtTicks, ++state.ProbeVersion); return true; } } /// /// Returns a probe slot claimed for a request that a later layer then refused, so the wasted - /// reservation does not cost the next arrival its slot. Only the caller's own reservation is - /// undone — a slot re-granted or re-armed in the meantime wins. + /// reservation does not cost the next arrival its slot. /// - private void ReleaseProbe(WindowState state, long now, long previousProbeAtTicks) + /// + /// The claim is identified by the per-state version stamped when it was made, never by the + /// timestamp it wrote. Every writer of NextProbeAtTicks bumps that version, so a re-arm + /// from a concurrent failure — which writes the identical now + interval expression, and + /// at ~1 ms clock resolution routinely lands on the same tick — cannot be mistaken for the + /// caller's own reservation and stomped back to a stale, already-due value. Restoring bumps the + /// version again so no other stale release can match either. + /// + private static void ReleaseProbe(WindowState state, ProbeClaim claim) { lock (state) { - if (state.NextProbeAtTicks == now + _probeIntervalTicks) + if (state.ProbeVersion != claim.Version) { - state.NextProbeAtTicks = previousProbeAtTicks; + return; } + + state.NextProbeAtTicks = claim.PreviousProbeAtTicks; + state.ProbeVersion++; } } @@ -504,8 +532,21 @@ public sealed class ApiKeyFailureLimiter public long LastActivityTicks; public long NextProbeAtTicks; + + /// + /// Monotonic stamp bumped by every writer of (probe claim and + /// failure re-arm alike). It is what lets a compensating release recognise its own + /// reservation without comparing timestamps, which collide whenever two writers share a + /// clock tick. + /// + public long ProbeVersion; } + /// A probe slot reservation: what to restore, and the stamp proving it is still ours. + /// The slot value replaced when the claim was made. + /// The stamped by this claim. + private readonly record struct ProbeClaim(long PreviousProbeAtTicks, long Version); + private sealed class PeerKeyIds { /// Key ids this transport peer has minted a partition for. diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs index 95d7935..d843b83 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs @@ -219,6 +219,54 @@ public sealed class ApiKeyFailureLimiterTests Assert.Equal(ApiKeyThrottleDecision.ProbeAdmitted, limiter.Check(holder)); } + /// + /// A compensating release must never undo a re-arm written by a concurrent failure on the same + /// partition. Both writers store the identical now + interval value when they share a + /// clock tick, so identifying the caller's own reservation by timestamp would let the release + /// stomp a fresh re-arm back to an already-due value and reopen the probe slot early. The clock + /// is deliberately held still here, which forces exactly that collision. + /// + [Fact] + public void ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 5); + ApiKeyThrottlePartition holder = new("ipv4:10.0.0.1:1", "victim"); + ApiKeyThrottlePartition other = new("ipv4:10.0.0.2:1", "victim"); + + // Trip both layers, then push the aggregate's slot one interval past the partition's, so + // every Check below claims the partition's slot and is then refused by the aggregate — the + // claim-and-compensate path under test. + RecordFailures(limiter, holder, 5); + clock.Advance(TimeSpan.FromSeconds(3)); + limiter.RecordFailure(other); + clock.Advance(TimeSpan.FromSeconds(2)); + + // Land a failure on the same partition inside the claim-to-release window — the interleaving + // a concurrent RecordFailure produces, forced here so the assertion is deterministic. It + // shares the frozen clock tick with the claim, so both write the identical slot value. + int interleaved = 0; + limiter.ProbeReleaseInterleaveHook = () => + { + if (Interlocked.Exchange(ref interleaved, 1) == 0) + { + limiter.RecordFailure(holder); + } + }; + + Assert.Equal(ApiKeyThrottleDecision.ThrottledByAggregate, limiter.Check(holder)); + Assert.Equal(1, interleaved); + limiter.ProbeReleaseInterleaveHook = null; + + // Drop the aggregate so the next decision reflects the composite partition alone. + limiter.Reset(other); + + // The interleaved failure pushed the slot one interval past the (still unadvanced) clock, so + // no probe may be due. Restoring over it would leave the already-due earlier value and hand + // the next arrival a free probe. + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(holder)); + } + /// A zero probe interval restores absolute blocking (documented as not recommended). [Fact] public void ProbeIntervalZero_BlocksAbsolutely()