From df710e18a9b855d915f4a150bb75ad3b09c44906 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 05:39:10 -0400 Subject: [PATCH 1/3] fix(SEC-31,SEC-32): re-partition the API-key failure limiter on (peer, key id) with probe admission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gRPC auth failure limiter partitioned on the key id parsed out of the *unauthenticated* token and rejected with ResourceExhausted before VerifyAsync ran. Key ids are not secret — they ride in every token and are listed on the dashboard — so any network peer could send 10 garbage-secret requests per minute and deny that key indefinitely: the legitimate holder's correct secret was refused before it was ever checked, and the success-path Reset that would clear the block sat behind the verification the block prevented (SEC-31). The tracked map was also flushable — any `a_b_c`-shaped junk minted a fresh partition (the `mxgw` literal was never compared), so ~4096 throwaway tokens evicted a blocked entry and reset the window (SEC-32). ApiKeyFailureLimiter moves from IsBlocked/RecordFailure/Reset(string peer) to a partition-pair API: Check/RecordFailure/Reset(ApiKeyThrottlePartition) with an ApiKeyThrottleDecision result. Two layers share one sliding window — a composite (transport peer, key id) partition at ApiKeyFailureLimit, and a per-key-id aggregate across all peers at the new ApiKeyFailureAggregateLimit (default 30) that bounds a source-rotating sprayer. An over-limit state is now a valve rather than a wall: one request per the new ApiKeyFailureProbeIntervalSeconds (default 5) is admitted through to the real verifier, so the correct secret always reaches the constant-time compare and resets both layers. Guarantees preserved: guessing stays bounded per window, and the failure path still spends no store read per attempt. SEC-32 rides the same change set: the interceptor validates token shape (literal `mxgw` prefix, >= 3 non-empty `_` segments, key id <= 64 chars) before minting a key-id partition, each transport peer may mint at most 32 of them before the overflow collapses onto its fallback partition, and eviction prefers fully expired windows and never drops an over-limit partition below a 2x transient overshoot ceiling. Throttled attempts increment mxgateway.auth.throttled, tagged stage=peer|aggregate only — /metrics is unauthenticated (open SEC-14), so no key material may appear there. Docs in the same commit: GatewayConfiguration limiter rows plus the two new keys, the Authentication hot-path paragraph, the Authorization SEC-11 section, and the limiter / SecurityOptions XML remarks (the old NAT rationale described the defective keying). Tracking rows flipped to Done with a change-log entry. Tests: new ApiKeyFailureLimiterTests (11) covering window pruning, composite vs aggregate trip points, probe cadence, absolute-block mode, reset across both layers, junk-spray eviction resistance, the per-peer cap, and expired-window eviction preference; GatewayGrpcAuthorizationInterceptorTests gains the four SEC-31 contract tests plus NonMxgwToken_FallsBackToTransportPeerPartition (20 total); GatewayOptionsValidatorTests covers both new keys including 0 as a supported disable value (66 total). --- .../2026-07-12/remediation/00-tracking.md | 9 +- .../remediation/40-security-dashboard.md | 4 +- docs/Authentication.md | 20 + docs/Authorization.md | 9 +- docs/GatewayConfiguration.md | 8 +- .../Configuration/GatewayOptionsValidator.cs | 12 + .../Configuration/SecurityOptions.cs | 42 +- .../Metrics/GatewayMetrics.cs | 15 + .../Authorization/ApiKeyFailureLimiter.cs | 437 +++++++++++++++--- .../Authorization/ApiKeyThrottleDecision.cs | 28 ++ .../Authorization/ApiKeyThrottlePartition.cs | 16 + .../GatewayGrpcAuthorizationInterceptor.cs | 98 ++-- .../GatewayOptionsValidatorTests.cs | 39 ++ .../ApiKeyFailureLimiterTests.cs | 265 +++++++++++ ...atewayGrpcAuthorizationInterceptorTests.cs | 267 ++++++++++- .../TestSupport/TestServerCallContext.cs | 16 +- 16 files changed, 1148 insertions(+), 137 deletions(-) create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottleDecision.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottlePartition.cs create mode 100644 src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 73bdcb8..f4c02ca 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -46,8 +46,8 @@ Sequenced by cluster; a cluster is one change set. | WRK-21 | Medium | M | owns IPC-23 fix; WRK-28 same batch | Not started | DrainEvents bound is count-based only; oversized reply kills the session and loses the drained events | | IPC-23 | Medium | S | WRK-21 | Not started | DrainEvents contract requirements (reply fits negotiated max, no event loss, drain-until-empty) + proto-comment/doc wave | | IPC-30 | Low | M | WRK-21 (same batch) | Not started | Oversized event frame stays session-fatal by design, but the death becomes structured (fault frame + logged identity) | -| SEC-31 | Medium | M | — | Not started | Failure limiter partitions on attacker-controlled key id and blocks before verification (lockout DoS) | -| SEC-32 | Low | S | SEC-31 | Not started | Failure-limiter LRU flushable by junk-token spray; token prefix never validated | +| SEC-31 | Medium | M | — | Done | Failure limiter partitions on attacker-controlled key id and blocks before verification (lockout DoS) | +| SEC-32 | Low | S | SEC-31 | Done | Failure-limiter LRU flushable by junk-token spray; token prefix never validated | | IPC-24 | Medium | S | — | Not started | CI's unconditional Java churn-revert masks real drift | | IPC-25 | Medium | M | — | Not started | Regenerate stale Go/Python worker bindings + add binding-freshness guard (Check 4) to check-codegen.ps1 | @@ -99,8 +99,8 @@ Full design + implementation for each row lives in the linked domain doc under i | ID | Sev | Tier | Eff | Dep | Status | Title | |---|---|:-:|:-:|---|---|---| -| SEC-31 | Medium | P0 | M | — | Not started | Failure limiter: composite (peer, key-id) partitions + cross-peer aggregate with probe admission | -| SEC-32 | Low | P0 | S | SEC-31 | Not started | Limiter LRU flushable by junk-token spray; validate token shape, cap per-peer partitions | +| 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) | @@ -161,3 +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. | diff --git a/archreview/2026-07-12/remediation/40-security-dashboard.md b/archreview/2026-07-12/remediation/40-security-dashboard.md index c17fd70..a5ea7c5 100644 --- a/archreview/2026-07-12/remediation/40-security-dashboard.md +++ b/archreview/2026-07-12/remediation/40-security-dashboard.md @@ -10,8 +10,8 @@ Repo rules that bind every entry: docs change in the same commit as the source ( | 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-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 | diff --git a/docs/Authentication.md b/docs/Authentication.md index 326a862..5304945 100644 --- a/docs/Authentication.md +++ b/docs/Authentication.md @@ -116,6 +116,26 @@ a dictionary lookup. Both windows are configurable and may be set to `0` to disa the respective mechanism; see [GatewayConfiguration](./GatewayConfiguration.md). +Failures are never cached — a wrong secret always reaches the store — so the +failure path is shielded by `ApiKeyFailureLimiter` instead, consulted before +`VerifyAsync` runs. It counts failures over one sliding +`MxGateway:Security:ApiKeyFailureWindowSeconds` window in two layers: a composite +`(transport peer, key id)` partition capped at `ApiKeyFailureLimit`, and a +per-key-id aggregate across all peers capped at `ApiKeyFailureAggregateLimit`. The +key id never partitions on its own — it is public, so an attacker-supplied one +would otherwise let any peer throttle a key it does not hold — and it joins the +partition only when the presented token is validly shaped +(`mxgw__`, key id at most 64 characters), with at most 32 key-id +partitions per address before the overflow collapses onto that address's fallback +partition. An over-limit state admits one probe per +`ApiKeyFailureProbeIntervalSeconds` through to the real verifier and refuses +everything else with `ResourceExhausted` before the store read, so a legitimate +holder presenting the correct secret always reaches the constant-time compare and +resets both layers; the counter's LRU eviction (`ApiKeyFailureTrackedPeers`) +prefers expired windows and will not drop an over-limit partition below a 2x +overshoot ceiling, so the memory bound cannot be turned into a way to clear an +active block. See [Authorization](./Authorization.md) for the enforcement path. + ## Storage API-key state lives in a dedicated SQLite database owned by the shared library. diff --git a/docs/Authorization.md b/docs/Authorization.md index 87c62de..c48851f 100644 --- a/docs/Authorization.md +++ b/docs/Authorization.md @@ -89,9 +89,14 @@ The flow is: The status codes are deliberately distinct: `Unauthenticated` signals "we do not know who you are," and `PermissionDenied` signals "we know who you are, but you cannot do this." Treating the two as the same code would make troubleshooting harder for client implementations. -### Rate limiting the auth surface (SEC-11) +### Rate limiting the auth surface (SEC-11, SEC-31, SEC-32) -Before the verification store read, the helper checks a cheap in-process per-peer failure counter (`ApiKeyFailureLimiter`). A peer that has accumulated more than `MxGateway:Security:ApiKeyFailureLimit` failed attempts inside the sliding `ApiKeyFailureWindowSeconds` window is short-circuited with `StatusCode.ResourceExhausted` — so online guessing of API-key secrets cannot spend a SQLite read (and, in a naive design, a cache miss) per attempt. The peer is keyed on the presented key id where the token parses, falling back to the transport peer address; keying on key id throttles a single abusive credential without penalizing co-located clients behind a shared NAT. A successful verification resets the peer's counter. The counter is a bounded LRU (`ApiKeyFailureTrackedPeers`) so it cannot grow without limit. `ResourceExhausted` reveals only that throttling is in effect, not whether any particular secret was valid, preserving the opaque-failure property. +Before the verification store read, the helper asks a cheap in-process failure counter (`ApiKeyFailureLimiter`) whether the attempt may proceed, so online guessing of API-key secrets cannot spend a SQLite read (and, in a naive design, a cache miss) per attempt. The counter has two layers over one sliding `ApiKeyFailureWindowSeconds` window: + +- **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. A successful verification resets both layers — which is why the reset path stays reachable while a key is under active spray. 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/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index e26406a..a4ed41f 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -361,9 +361,11 @@ model requires otherwise. | `MxGateway:Security:ApiKeyLastUsedCoalesceSeconds` | `60` | Coalescing window, in seconds, for the `last_used_utc` write. The library verifier writes `last_used` on every successful verification; this bounds the write to at most one per key per window, so a hammered key does not churn the WAL. `0` forwards every write. Must be zero or greater. | | `MxGateway:Security:LoginRateLimitPermitLimit` | `10` | Maximum `POST /auth/login` attempts permitted per remote IP within `LoginRateLimitWindowSeconds` before requests are rejected with HTTP 429. Throttles LDAP credential stuffing before the bind is relayed to the directory. Must be greater than zero. | | `MxGateway:Security:LoginRateLimitWindowSeconds` | `60` | Fixed-window length, in seconds, for the per-IP login rate limit. Must be greater than zero. | -| `MxGateway:Security:ApiKeyFailureLimit` | `10` | Failed API-key verifications, per peer, within `ApiKeyFailureWindowSeconds` that trip the in-process short-circuit. Once tripped, the gRPC auth path rejects further attempts with `ResourceExhausted` **before** the store read; a successful verification resets the peer's counter. The peer is keyed on the presented key id (falling back to the transport address) so a single abusive credential behind a shared NAT is throttled without locking out co-located clients. Must be greater than zero. | -| `MxGateway:Security:ApiKeyFailureWindowSeconds` | `60` | Sliding-window length, in seconds, over which API-key verification failures are counted per peer. Must be greater than zero. | -| `MxGateway:Security:ApiKeyFailureTrackedPeers` | `4096` | Maximum distinct peers tracked by the failure counter (a bounded LRU) so a spray of unique peer keys cannot grow memory without limit. Must be greater than zero. | +| `MxGateway:Security:ApiKeyFailureLimit` | `10` | Failed API-key verifications, per `(transport peer, key id)` partition, within `ApiKeyFailureWindowSeconds` that trip the in-process short-circuit. Once tripped, the gRPC auth path rejects further attempts from that partition with `ResourceExhausted` **before** the store read — except for the probe admitted every `ApiKeyFailureProbeIntervalSeconds` — and a successful verification resets the partition. The partition always includes the sender's transport address: key ids are public (they ride in every token and are listed on the dashboard), so keying on the key id alone let any peer deny a key to its legitimate holder. Must be greater than zero. | +| `MxGateway:Security:ApiKeyFailureWindowSeconds` | `60` | Sliding-window length, in seconds, over which API-key verification failures are counted, for both the per-partition and the per-key-id aggregate layer. Must be greater than zero. | +| `MxGateway:Security:ApiKeyFailureAggregateLimit` | `30` | Failed verifications for one key id counted across **all** transport peers within `ApiKeyFailureWindowSeconds` before that key id enters probe mode. This second layer bounds a distributed or source-rotating sprayer that never trips any single `(peer, key id)` partition. `0` disables the aggregate layer, leaving only per-partition counting. Must be zero or greater. | +| `MxGateway:Security:ApiKeyFailureProbeIntervalSeconds` | `5` | Minimum interval, in seconds, between probe admissions for an over-limit partition or key-id aggregate. An over-limit state is a valve rather than a wall: one request per interval reaches the real verifier, so the holder of the correct secret always gets through and clears the state, while everything else is still refused before the store read. `0` blocks absolutely instead — **not recommended**, because an unauthenticated peer can then deny the key to its holder for the whole window. Must be zero or greater. | +| `MxGateway:Security:ApiKeyFailureTrackedPeers` | `4096` | Maximum distinct partitions tracked by the failure counter (a bounded LRU) so a spray of unique tokens cannot grow memory without limit. It cannot be used to flush an active block either: only a validly shaped `mxgw__` token mints a key-id partition (everything else lands on the sender's transport-peer partition), each address may mint at most 32 key-id partitions before the overflow collapses onto that address's fallback partition, and eviction prefers fully expired windows, never removing an over-limit partition until the map exceeds twice this cap. Must be greater than zero. | ## Galaxy Options diff --git a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs index dcd60ba..93ab9e8 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Configuration/GatewayOptionsValidator.cs @@ -87,6 +87,18 @@ public sealed class GatewayOptionsValidator : OptionsValidatorBase - /// Gets the number of consecutive failed API-key verifications, per peer, within - /// that trips the in-process short-circuit. Once tripped, - /// the gRPC auth path rejects further attempts before the store read; a successful verification - /// resets the peer's counter. Default is 10. + /// Gets the number of failed API-key verifications, per (transport peer, key id) + /// partition, within that trips the in-process + /// short-circuit. Once tripped, the gRPC auth path rejects further attempts from that partition + /// before the store read, except for the probe admitted every + /// ; a successful verification resets the + /// partition. Default is 10. /// public int ApiKeyFailureLimit { get; init; } = 10; /// /// Gets the sliding-window length, in seconds, over which API-key verification failures are - /// counted per peer. Default is 60 seconds. + /// counted (for both the per-partition and the per-key-id aggregate layer). Default is 60 + /// seconds. /// public int ApiKeyFailureWindowSeconds { get; init; } = 60; /// - /// Gets the maximum number of distinct peers tracked by the API-key failure counter. The counter - /// is a bounded LRU so a spray of unique peer keys cannot grow memory without limit. Default is - /// 4096. + /// Gets the number of failed API-key verifications for one key id, counted across all + /// transport peers within , that puts the key id into + /// probe mode. This second layer bounds a distributed or source-rotating sprayer that never + /// trips any single (peer, key id) partition. Set to 0 to disable the aggregate + /// layer. Default is 30. + /// + public int ApiKeyFailureAggregateLimit { get; init; } = 30; + + /// + /// Gets the minimum interval, in seconds, between probe admissions for an over-limit partition + /// or key-id aggregate. An over-limit state is a valve rather than a wall: at most one request + /// per interval is admitted through to the real verifier, so the holder of the correct secret + /// can always reach the constant-time compare (and reset the state) while an attacker is + /// spraying. Set to 0 to block absolutely instead — not recommended, because an + /// unauthenticated peer can then deny the key to its legitimate holder for the whole window. + /// Default is 5 seconds. + /// + public int ApiKeyFailureProbeIntervalSeconds { get; init; } = 5; + + /// + /// Gets the maximum number of distinct partitions tracked by the API-key failure counter. The + /// counter is a bounded LRU so a spray of unique tokens cannot grow memory without limit, and — + /// since only a validly shaped token mints a key-id partition, capped per transport peer — + /// cannot be flushed to clear an active block either: eviction prefers fully expired windows and + /// never removes a partition that is currently over its limit, up to a transient overshoot + /// ceiling of twice this value. Default is 4096. /// public int ApiKeyFailureTrackedPeers { get; init; } = 4096; } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs b/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs index 859b73d..5f520cc 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs @@ -24,6 +24,7 @@ public sealed class GatewayMetrics : IDisposable private readonly Counter _streamDisconnectsCounter; private readonly Counter _retryAttemptsCounter; private readonly Counter _alarmProviderSwitchesCounter; + private readonly Counter _authThrottledCounter; private readonly Histogram _workerStartupLatencyHistogram; private readonly Histogram _commandLatencyHistogram; private readonly Histogram _eventStreamSendLatencyHistogram; @@ -80,6 +81,7 @@ public sealed class GatewayMetrics : IDisposable _streamDisconnectsCounter = _meter.CreateCounter("mxgateway.grpc.streams.disconnected"); _retryAttemptsCounter = _meter.CreateCounter("mxgateway.retries.attempted"); _alarmProviderSwitchesCounter = _meter.CreateCounter("mxgateway.alarms.provider_switches"); + _authThrottledCounter = _meter.CreateCounter("mxgateway.auth.throttled"); _workerStartupLatencyHistogram = _meter.CreateHistogram("mxgateway.workers.startup.duration", "s"); _commandLatencyHistogram = _meter.CreateHistogram("mxgateway.commands.duration", "s"); _eventStreamSendLatencyHistogram = _meter.CreateHistogram("mxgateway.events.stream_send.duration", "s"); @@ -342,6 +344,19 @@ public sealed class GatewayMetrics : IDisposable _queueOverflowsCounter.Add(1, new KeyValuePair("queue", queueName)); } + /// + /// Records that an API-key authentication attempt was short-circuited by the failure limiter + /// before verification. + /// + /// Limiter layer that refused the attempt: peer or aggregate. + public void RecordAuthThrottled(string stage) + { + // Tagged by stage only. Neither the key id nor the peer address may become a tag: key ids + // are credential material and peer addresses are unbounded cardinality, and /metrics is not + // authenticated (open SEC-14), so anything tagged here is world-readable. + _authThrottledCounter.Add(1, new KeyValuePair("stage", stage)); + } + /// /// Records that a fault occurred in the given category. /// 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 c5baea1..38df873 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs @@ -4,34 +4,62 @@ using ZB.MOM.WW.MxGateway.Server.Configuration; namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; /// -/// Cheap, in-process per-peer sliding-window failure counter for the gRPC auth path. It is -/// checked BEFORE the API-key verification store read and short-circuits a peer that has exceeded -/// failed attempts within -/// ; a successful verification resets the -/// peer's counter. +/// Cheap, in-process sliding-window failure counter for the gRPC auth path. It is checked BEFORE the +/// API-key verification store read, so online secret guessing cannot spend a SQLite read per attempt, +/// and it admits a periodic probe so a throttle can never deny a key to the holder of the correct +/// secret. /// /// /// -/// The peer key is the API key id when the presented token parses, falling back to the transport -/// peer address otherwise. Keying on key id (per the NAT caveat in glauth.md) means a single -/// abusive credential behind a shared NAT is throttled without locking out unrelated clients on the -/// same address. +/// Two layers share one window mechanism. Layer 1 partitions on the composite +/// (transport peer, key id): an attacker's failures bind to the address that produced them, +/// so a peer spraying guesses at a key id it does not own throttles only itself, while co-located +/// clients using other key ids are untouched. Layer 2 counts failures for a key id across all +/// peers (), which bounds a distributed or +/// source-rotating sprayer that never trips any single partition. /// /// -/// The tracked-peer set is a bounded LRU () so -/// a spray of unique peer keys cannot grow memory without limit. Successful peers are removed on -/// reset, so in steady state the map holds only peers with recent failures — the common success path -/// is a lock-free dictionary miss. +/// An over-limit state is a valve, not a wall (SEC-31): at most one request per +/// is admitted through to the real +/// verifier, and a successful verification resets both layers. The reset path therefore stays +/// reachable while throttled — the earlier design keyed solely on the attacker-supplied key id and +/// rejected before verification, so the legitimate holder could never clear the block. +/// +/// +/// The tracked partition set is a bounded LRU (). +/// Because only a validly shaped token mints a key-id partition and each transport peer may mint at +/// most of them (the overflow collapses onto that address's +/// fallback partition), the map bounds memory and resists a junk-token flush (SEC-32): +/// eviction prefers fully expired windows, never removes an over-limit partition below a transient +/// overshoot ceiling of twice the cap, and only then falls back to least-recently-active. Successful +/// peers are removed on reset, so in steady state the map holds only partitions with recent failures +/// — the common success path is a lock-free dictionary miss. /// /// public sealed class ApiKeyFailureLimiter { + /// + /// Maximum distinct key-id partitions one transport peer may mint. Not configurable: no + /// legitimate address fails against dozens of distinct keys inside one window, and the cap is + /// what bounds an attacker's total partitions to this value plus its fallback partition. + /// + internal const int MaxKeyIdPartitionsPerPeer = 32; + + // NUL appears in neither a gRPC peer string ("ipv4:10.0.0.1:5000") nor an ASCII metadata + // value, so a composite key can never collide with a transport-peer-only fallback key. + // A collision would merge two partitions, never widen an admission. + private const char PartitionSeparator = '\0'; + private readonly int _limit; + private readonly int _aggregateLimit; private readonly long _windowTicks; - private readonly int _maxPeers; + private readonly long _probeIntervalTicks; + private readonly int _maxPartitions; private readonly TimeProvider _clock; - private readonly ConcurrentDictionary _peers = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _partitions = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _aggregates = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _peerKeyIds = new(StringComparer.Ordinal); /// Initializes a new instance of the class. /// Security options carrying the failure-limit knobs. @@ -41,79 +69,219 @@ public sealed class ApiKeyFailureLimiter (security ?? throw new ArgumentNullException(nameof(security))).ApiKeyFailureLimit, TimeSpan.FromSeconds(security.ApiKeyFailureWindowSeconds), security.ApiKeyFailureTrackedPeers, + security.ApiKeyFailureAggregateLimit, + TimeSpan.FromSeconds(security.ApiKeyFailureProbeIntervalSeconds), clock) { } /// Initializes a new instance of the class. Test/explicit seam. - /// The maximum number of failures allowed within . + /// The maximum number of failures allowed per partition within . /// The sliding window over which failures are counted. - /// The maximum number of tracked peers before least-recently-active eviction kicks in. + /// The maximum number of tracked partitions before eviction kicks in. + /// The per-key-id cross-peer failure limit; 0 disables the aggregate layer. + /// The minimum interval between probe admissions; blocks absolutely. /// The time provider. - internal ApiKeyFailureLimiter(int limit, TimeSpan window, int maxPeers, TimeProvider clock) + internal ApiKeyFailureLimiter( + int limit, + TimeSpan window, + int maxPartitions, + int aggregateLimit, + TimeSpan probeInterval, + TimeProvider clock) { ArgumentNullException.ThrowIfNull(clock); _limit = limit; _windowTicks = window.Ticks; - _maxPeers = maxPeers; + _maxPartitions = maxPartitions; + _aggregateLimit = aggregateLimit; + _probeIntervalTicks = probeInterval.Ticks; _clock = clock; } - /// Returns whether the peer has reached the failure limit within the current window. - /// The peer key (key id or peer address). - /// when the peer should be short-circuited. - public bool IsBlocked(string peer) + /// Gets the number of tracked (peer, key id) partitions. Test seam. + internal int TrackedPartitionCount => _partitions.Count; + + /// Gets the number of tracked per-key-id aggregates. Test seam. + internal int TrackedAggregateCount => _aggregates.Count; + + /// Decides whether an authentication attempt may reach the verifier. + /// The throttle partition derived from the request. + /// The admission decision for this attempt. + public ApiKeyThrottleDecision Check(ApiKeyThrottlePartition partition) { - ArgumentNullException.ThrowIfNull(peer); + string peer = RequirePeer(partition); if (_limit <= 0) { - return false; - } - - if (!_peers.TryGetValue(peer, out PeerState? state)) - { - return false; + return ApiKeyThrottleDecision.Allowed; } long now = _clock.GetUtcNow().UtcTicks; - lock (state) + (string partitionKey, string? effectiveKeyId) = ResolvePartitionKey(peer, partition.KeyId, mint: false); + + WindowState? peerState = _partitions.TryGetValue(partitionKey, out WindowState? tracked) ? tracked : null; + WindowState? aggregateState = null; + if (effectiveKeyId is not null + && _aggregateLimit > 0 + && _aggregates.TryGetValue(effectiveKeyId, out WindowState? aggregate)) { - Prune(state, now); - return state.FailureTicks.Count >= _limit; + aggregateState = aggregate; } + + bool peerOver = peerState is not null && IsOverLimit(peerState, now, _limit); + bool aggregateOver = aggregateState is not null && IsOverLimit(aggregateState, now, _aggregateLimit); + if (!peerOver && !aggregateOver) + { + return ApiKeyThrottleDecision.Allowed; + } + + if (_probeIntervalTicks <= 0) + { + return peerOver ? ApiKeyThrottleDecision.ThrottledByPeer : ApiKeyThrottleDecision.ThrottledByAggregate; + } + + // Both over-limit layers must have a slot before either is consumed, so a request cannot burn + // the peer's probe and then be refused by the aggregate. + if (peerOver && !IsProbeDue(peerState!, now)) + { + return ApiKeyThrottleDecision.ThrottledByPeer; + } + + if (aggregateOver && !IsProbeDue(aggregateState!, now)) + { + return ApiKeyThrottleDecision.ThrottledByAggregate; + } + + if (peerOver) + { + ConsumeProbe(peerState!, now); + } + + if (aggregateOver) + { + ConsumeProbe(aggregateState!, now); + } + + return ApiKeyThrottleDecision.ProbeAdmitted; } - /// Records a failed verification attempt for the peer. - /// The peer key (key id or peer address). - public void RecordFailure(string peer) + /// Records a failed verification attempt against both limiter layers. + /// The throttle partition derived from the request. + public void RecordFailure(ApiKeyThrottlePartition partition) { - ArgumentNullException.ThrowIfNull(peer); + string peer = RequirePeer(partition); if (_limit <= 0) { return; } long now = _clock.GetUtcNow().UtcTicks; - PeerState state = _peers.GetOrAdd(peer, static _ => new PeerState()); + (string partitionKey, string? effectiveKeyId) = ResolvePartitionKey(peer, partition.KeyId, mint: true); + + WindowState state = _partitions.GetOrAdd(partitionKey, _ => new WindowState(peer, effectiveKeyId)); + RecordInto(state, now, _limit); + + // Only a key id that earned its own partition feeds the aggregate: an id squeezed out by the + // per-peer cap is spray, and letting it through would make aggregate cardinality unbounded. + if (effectiveKeyId is not null && _aggregateLimit > 0) + { + WindowState aggregate = _aggregates.GetOrAdd(effectiveKeyId, static _ => new WindowState(null, null)); + RecordInto(aggregate, now, _aggregateLimit); + } + + EvictIfOverCapacity(_partitions, now, _limit, releaseKeyIds: true); + EvictIfOverCapacity(_aggregates, now, _aggregateLimit, releaseKeyIds: false); + } + + /// Clears both limiter layers for the partition after a successful verification. + /// The throttle partition derived from the request. + public void Reset(ApiKeyThrottlePartition partition) + { + string peer = RequirePeer(partition); + (string partitionKey, _) = ResolvePartitionKey(peer, partition.KeyId, mint: false); + RemovePartition(partitionKey); + + // The aggregate is cleared on the presented key id even when the composite partition + // collapsed to the fallback: a verified secret is proof the key is not under successful + // attack, and clearing is what makes the block recoverable. + if (partition.KeyId is not null) + { + _aggregates.TryRemove(partition.KeyId, out _); + } + } + + /// Returns whether the partition currently has tracked failure state. Test seam. + /// The throttle partition to look up. + /// when the partition has a tracked window. + internal bool IsTracked(ApiKeyThrottlePartition partition) + { + string peer = RequirePeer(partition); + (string partitionKey, _) = ResolvePartitionKey(peer, partition.KeyId, mint: false); + + return _partitions.ContainsKey(partitionKey); + } + + private static string RequirePeer(ApiKeyThrottlePartition partition) + { + if (string.IsNullOrEmpty(partition.TransportPeer)) + { + throw new ArgumentException("A throttle partition requires a transport peer.", nameof(partition)); + } + + return partition.TransportPeer; + } + + private static string Composite(string peer, string keyId) => peer + PartitionSeparator + keyId; + + private void RecordInto(WindowState state, long now, int limit) + { lock (state) { Prune(state, now); state.FailureTicks.Enqueue(now); 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. + if (limit > 0 && state.FailureTicks.Count >= limit) + { + state.NextProbeAtTicks = now + _probeIntervalTicks; + } + } + } + + private bool IsOverLimit(WindowState state, long now, int limit) + { + if (limit <= 0) + { + return false; } - EvictIfOverCapacity(); + lock (state) + { + Prune(state, now); + return state.FailureTicks.Count >= limit; + } } - /// Clears the peer's failure count after a successful verification. - /// The peer key (key id or peer address). - public void Reset(string peer) + private bool IsProbeDue(WindowState state, long now) { - ArgumentNullException.ThrowIfNull(peer); - _peers.TryRemove(peer, out _); + lock (state) + { + return now >= state.NextProbeAtTicks; + } } - private void Prune(PeerState state, long now) + private void ConsumeProbe(WindowState state, long now) + { + lock (state) + { + state.NextProbeAtTicks = now + _probeIntervalTicks; + state.LastActivityTicks = now; + } + } + + private void Prune(WindowState state, long now) { while (state.FailureTicks.Count > 0 && now - state.FailureTicks.Peek() >= _windowTicks) { @@ -121,37 +289,186 @@ public sealed class ApiKeyFailureLimiter } } - private void EvictIfOverCapacity() + /// + /// Maps a partition onto its storage key, enforcing the per-peer key-id cap. Returns the + /// transport-peer fallback key (and a null effective key id) when the token carried no key id or + /// when this address has already minted of them. + /// + private (string PartitionKey, string? EffectiveKeyId) ResolvePartitionKey(string peer, string? keyId, bool mint) { - // Best-effort eviction: only runs when the map exceeds the cap (rare, since only peers with - // recent failures are tracked). Removes the least-recently-active peer. Racy under - // concurrency, which is acceptable for a bound rather than an exact policy. - while (_peers.Count > _maxPeers) + if (keyId is null) { - string? oldest = null; - long oldestTicks = long.MaxValue; - foreach (KeyValuePair entry in _peers) + return (peer, null); + } + + while (true) + { + if (!_peerKeyIds.TryGetValue(peer, out PeerKeyIds? keyIds)) { - long activity = Volatile.Read(ref entry.Value.LastActivityTicks); - if (activity < oldestTicks) + if (!mint) { - oldestTicks = activity; - oldest = entry.Key; + // Nothing tracked for this address yet, so the composite lookup simply misses. + return (Composite(peer, keyId), keyId); } + + keyIds = _peerKeyIds.GetOrAdd(peer, static _ => new PeerKeyIds()); } - if (oldest is null || !_peers.TryRemove(oldest, out _)) + lock (keyIds) { - break; + if (keyIds.Removed) + { + // Lost a race with cleanup; re-read the dictionary and try again. + continue; + } + + if (keyIds.KeyIds.Contains(keyId)) + { + return (Composite(peer, keyId), keyId); + } + + if (keyIds.KeyIds.Count >= MaxKeyIdPartitionsPerPeer) + { + return (peer, null); + } + + if (mint) + { + keyIds.KeyIds.Add(keyId); + } + + return (Composite(peer, keyId), keyId); } } } - private sealed class PeerState + private bool RemovePartition(string partitionKey) { + if (!_partitions.TryRemove(partitionKey, out WindowState? state)) + { + return false; + } + + if (state.TransportPeer is not null && state.KeyId is not null) + { + ReleaseKeyId(state.TransportPeer, state.KeyId); + } + + return true; + } + + private void ReleaseKeyId(string peer, string keyId) + { + if (!_peerKeyIds.TryGetValue(peer, out PeerKeyIds? keyIds)) + { + return; + } + + lock (keyIds) + { + keyIds.KeyIds.Remove(keyId); + if (keyIds.KeyIds.Count == 0) + { + keyIds.Removed = true; + _peerKeyIds.TryRemove(peer, out _); + } + } + } + + /// + /// Best-effort eviction, run only when a map exceeds the cap. Preference order: fully expired + /// windows, then least-recently-active entries that are still under their limit, and only above + /// the 2x transient-overshoot ceiling the oldest over-limit entry. Racy under concurrency, which + /// is acceptable for a bound rather than an exact policy. + /// + private void EvictIfOverCapacity( + ConcurrentDictionary map, + long now, + int limit, + bool releaseKeyIds) + { + while (map.Count > _maxPartitions) + { + bool overHardCeiling = map.Count > (long)_maxPartitions * 2; + string? expired = null; + string? leastRecent = null; + string? oldestOverLimit = null; + long leastRecentTicks = long.MaxValue; + long oldestOverLimitTicks = long.MaxValue; + + foreach (KeyValuePair entry in map) + { + int count; + long activity; + lock (entry.Value) + { + Prune(entry.Value, now); + count = entry.Value.FailureTicks.Count; + activity = entry.Value.LastActivityTicks; + } + + if (count == 0) + { + expired = entry.Key; + break; + } + + if (limit > 0 && count >= limit) + { + if (activity < oldestOverLimitTicks) + { + oldestOverLimitTicks = activity; + oldestOverLimit = entry.Key; + } + + continue; + } + + if (activity < leastRecentTicks) + { + leastRecentTicks = activity; + leastRecent = entry.Key; + } + } + + string? victim = expired ?? leastRecent ?? (overHardCeiling ? oldestOverLimit : null); + if (victim is null) + { + // Everything left is load-bearing and the ceiling is not breached: accept the + // documented transient overshoot rather than clearing an active block. + return; + } + + bool removed = releaseKeyIds ? RemovePartition(victim) : map.TryRemove(victim, out _); + if (!removed) + { + return; + } + } + } + + private sealed class WindowState(string? transportPeer, string? keyId) + { + /// Gets the transport peer this partition belongs to; null for key-id aggregates. + public string? TransportPeer { get; } = transportPeer; + + /// Gets the key id this partition belongs to; null for fallback partitions and aggregates. + public string? KeyId { get; } = keyId; + /// Timestamps (in ticks) of failures still within the sliding window. public Queue FailureTicks { get; } = new(); public long LastActivityTicks; + + public long NextProbeAtTicks; + } + + private sealed class PeerKeyIds + { + /// Key ids this transport peer has minted a partition for. + public HashSet KeyIds { get; } = new(StringComparer.Ordinal); + + /// Set once the entry has been dropped from the peer map, so racing minters retry. + public bool Removed { get; set; } } } diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottleDecision.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottleDecision.cs new file mode 100644 index 0000000..5a219d4 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottleDecision.cs @@ -0,0 +1,28 @@ +namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; + +/// +/// Outcome of an admission check for one authentication attempt. +/// +public enum ApiKeyThrottleDecision +{ + /// No layer is over its limit; the request proceeds to verification normally. + Allowed = 0, + + /// + /// At least one layer is over its limit, but this request took the probe slot for the current + /// interval and proceeds to verification. A success resets every layer it passed. + /// + ProbeAdmitted = 1, + + /// + /// The (transport peer, key id) partition is over its limit and no probe slot is + /// available; the request is rejected before the verification store read. + /// + ThrottledByPeer = 2, + + /// + /// The cross-peer aggregate for the presented key id is over its limit and no probe slot is + /// available; the request is rejected before the verification store read. + /// + ThrottledByAggregate = 3, +} diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottlePartition.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottlePartition.cs new file mode 100644 index 0000000..05dbe24 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyThrottlePartition.cs @@ -0,0 +1,16 @@ +namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; + +/// +/// Partition identity for the API-key failure limiter: the transport peer that sent the request and, +/// when the presented token is validly shaped, the key id it claims. +/// +/// +/// Both halves come from unauthenticated input, which is why neither may stand alone. The transport +/// peer is always present, so a throttle can never outlive the address that earned it; the key id is +/// present only after the shape check in +/// (literal mxgw prefix, at least three +/// non-empty underscore segments, bounded key-id length) so junk tokens cannot mint partitions. +/// +/// The gRPC transport peer address (ServerCallContext.Peer). +/// The key id claimed by a validly shaped token, or . +public readonly record struct ApiKeyThrottlePartition(string TransportPeer, string? KeyId); diff --git a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs index b88fd3a..c7c4ddf 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/GatewayGrpcAuthorizationInterceptor.cs @@ -3,6 +3,7 @@ using Grpc.Core.Interceptors; using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.ApiKeys; using ZB.MOM.WW.MxGateway.Server.Configuration; +using ZB.MOM.WW.MxGateway.Server.Metrics; using ZB.MOM.WW.MxGateway.Server.Security.Authentication; // The handler pushes the gateway's constraint-bearing identity; alias away the shared library's @@ -16,8 +17,13 @@ public sealed class GatewayGrpcAuthorizationInterceptor( GatewayGrpcScopeResolver scopeResolver, IGatewayRequestIdentityAccessor identityAccessor, IOptions options, - ApiKeyFailureLimiter failureLimiter) : Interceptor + ApiKeyFailureLimiter failureLimiter, + GatewayMetrics metrics) : Interceptor { + // Generated key ids are far shorter; the cap only exists to stop an invented "key id" of + // arbitrary length from becoming a limiter partition. + private const int MaxKeyIdLength = 64; + /// public override async Task UnaryServerHandler( TRequest request, @@ -64,14 +70,19 @@ public sealed class GatewayGrpcAuthorizationInterceptor( string? authorizationHeader = context.RequestHeaders.GetValue("authorization"); - // Short-circuit a peer that has already failed too many times inside the sliding - // window BEFORE the verification store read, so online guessing cannot spend a SQLite read - // per attempt. The peer key prefers the presented key id over the transport address (NAT - // caveat). ResourceExhausted signals throttling without revealing whether any particular - // secret was valid. - string peerKey = ResolvePeerKey(authorizationHeader, context); - if (failureLimiter.IsBlocked(peerKey)) + // Short-circuit a partition that has already failed too many times inside the sliding window + // BEFORE the verification store read, so online guessing cannot spend a SQLite read per + // attempt. The partition is the composite (transport peer, key id) — never the key id alone, + // which is public and would let any peer deny a key it does not own — plus a cross-peer + // aggregate for the key id. An over-limit state still admits one probe per interval, so the + // holder of the correct secret always reaches the verifier and resets the state. + // ResourceExhausted signals throttling without revealing whether any secret was valid. + ApiKeyThrottlePartition throttlePartition = ResolveThrottlePartition(authorizationHeader, context); + ApiKeyThrottleDecision decision = failureLimiter.Check(throttlePartition); + if (decision is ApiKeyThrottleDecision.ThrottledByPeer or ApiKeyThrottleDecision.ThrottledByAggregate) { + metrics.RecordAuthThrottled( + decision == ApiKeyThrottleDecision.ThrottledByPeer ? "peer" : "aggregate"); throw new RpcException(new Status( StatusCode.ResourceExhausted, "Too many authentication attempts. Try again later.")); @@ -86,15 +97,17 @@ public sealed class GatewayGrpcAuthorizationInterceptor( if (!verification.Succeeded || verification.Identity is null) { - failureLimiter.RecordFailure(peerKey); + failureLimiter.RecordFailure(throttlePartition); throw new RpcException(new Status( StatusCode.Unauthenticated, "Missing or invalid API key.")); } - // Successful authentication clears the peer's failure count so a legitimate client that - // fat-fingered a few attempts is not penalised once it recovers. - failureLimiter.Reset(peerKey); + // Successful authentication clears both limiter layers, so a legitimate client that + // fat-fingered a few attempts is not penalised once it recovers — and, because the check + // above admits a probe rather than blocking absolutely, this reset stays reachable while the + // key is under an active spray. + failureLimiter.Reset(throttlePartition); ApiKeyIdentity identity = GatewayApiKeyIdentityMapper.ToGatewayIdentity(verification.Identity); @@ -109,29 +122,50 @@ public sealed class GatewayGrpcAuthorizationInterceptor( return identity; } - // Resolves the failure-limiter partition key: the presented key id (token shape - // mxgw__) when the header parses, otherwise the transport peer address. Keying on - // key id throttles a single abusive credential without locking out co-located clients behind NAT. - private static string ResolvePeerKey(string? authorizationHeader, ServerCallContext context) + // Resolves the failure-limiter partition: always the transport peer, plus the presented key id + // when — and only when — the token is validly shaped. Both halves are unauthenticated input, so + // the peer half is what keeps a throttle bound to the address that earned it. + private static ApiKeyThrottlePartition ResolveThrottlePartition( + string? authorizationHeader, + ServerCallContext context) { - if (!string.IsNullOrWhiteSpace(authorizationHeader)) - { - ReadOnlySpan header = authorizationHeader.AsSpan().Trim(); - const string bearer = "Bearer "; - ReadOnlySpan token = header.StartsWith(bearer, StringComparison.OrdinalIgnoreCase) - ? header[bearer.Length..].Trim() - : header; + return new ApiKeyThrottlePartition(context.Peer, TryResolveKeyId(authorizationHeader)); + } - // mxgw__: the key id is the second underscore-delimited segment. The - // secret may itself contain underscores, but the key id is unaffected. - string tokenText = token.ToString(); - string[] parts = tokenText.Split('_'); - if (parts.Length >= 3 && parts[0].Length > 0 && parts[1].Length > 0) - { - return "key:" + parts[1]; - } + // Token shape mxgw__: the key id is the second underscore-delimited segment (the + // secret may itself contain underscores, which does not affect the key id). The shape is checked + // before a key-id partition is minted so a spray of invented tokens cannot mint one tracked + // partition each and flush the limiter's bounded map (SEC-32). Anything that fails the check + // falls back to the sender's transport-peer partition. + private static string? TryResolveKeyId(string? authorizationHeader) + { + if (string.IsNullOrWhiteSpace(authorizationHeader)) + { + return null; } - return "peer:" + context.Peer; + ReadOnlySpan header = authorizationHeader.AsSpan().Trim(); + const string bearer = "Bearer "; + ReadOnlySpan token = header.StartsWith(bearer, StringComparison.OrdinalIgnoreCase) + ? header[bearer.Length..].Trim() + : header; + + string[] parts = token.ToString().Split('_'); + if (parts.Length < 3) + { + return null; + } + + if (!string.Equals(parts[0], AuthStoreServiceCollectionExtensions.TokenPrefix, StringComparison.Ordinal)) + { + return null; + } + + if (parts[1].Length == 0 || parts[1].Length > MaxKeyIdLength || parts[2].Length == 0) + { + return null; + } + + return parts[1]; } } diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs index 07544d0..30b9bc0 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Configuration/GatewayOptionsValidatorTests.cs @@ -794,6 +794,45 @@ public sealed class GatewayOptionsValidatorTests Assert.Contains(result.Failures!, f => f.Contains("ApiKeyFailureTrackedPeers")); } + /// Verifies a negative per-key aggregate failure limit fails validation. + [Fact] + public void Validate_Fails_WhenApiKeyFailureAggregateLimitNegative() + { + ValidateOptionsResult result = new GatewayOptionsValidator().Validate( + null, + WithSecurity(new SecurityOptions { ApiKeyFailureAggregateLimit = -1 })); + Assert.True(result.Failed); + Assert.Contains(result.Failures!, f => f.Contains("ApiKeyFailureAggregateLimit")); + } + + /// Verifies a negative probe interval fails validation. + [Fact] + public void Validate_Fails_WhenApiKeyFailureProbeIntervalSecondsNegative() + { + ValidateOptionsResult result = new GatewayOptionsValidator().Validate( + null, + WithSecurity(new SecurityOptions { ApiKeyFailureProbeIntervalSeconds = -1 })); + Assert.True(result.Failed); + Assert.Contains(result.Failures!, f => f.Contains("ApiKeyFailureProbeIntervalSeconds")); + } + + /// + /// Zero is a supported (documented) value for both new limiter knobs: it disables the aggregate + /// layer and probe admission respectively, so validation must accept it. + /// + [Fact] + public void Validate_Succeeds_WhenAggregateLimitAndProbeIntervalAreZero() + { + ValidateOptionsResult result = new GatewayOptionsValidator().Validate( + null, + WithSecurity(new SecurityOptions + { + ApiKeyFailureAggregateLimit = 0, + ApiKeyFailureProbeIntervalSeconds = 0, + })); + Assert.True(result.Succeeded); + } + private static GatewayOptions WithWorkerAndProtocol(WorkerOptions worker, ProtocolOptions protocol) { GatewayOptions source = ValidOptions(); diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs new file mode 100644 index 0000000..715a0c0 --- /dev/null +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs @@ -0,0 +1,265 @@ +using ZB.MOM.WW.MxGateway.Server.Security.Authorization; +using ZB.MOM.WW.MxGateway.Tests.TestSupport; + +namespace ZB.MOM.WW.MxGateway.Tests.Security.Authorization; + +/// +/// Unit tests for the two-layer API-key failure limiter (SEC-31 / SEC-32): composite +/// (transport peer, key id) partitions, the cross-peer per-key-id aggregate, probe +/// admission, the per-peer key-id partition cap, and the eviction preference order. +/// +public sealed class ApiKeyFailureLimiterTests +{ + private static readonly TimeSpan Window = TimeSpan.FromSeconds(60); + private static readonly TimeSpan ProbeInterval = TimeSpan.FromSeconds(5); + + /// A partition below the failure limit is admitted without consulting a probe slot. + [Fact] + public void Check_BelowLimit_Allows() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + limiter.RecordFailure(partition); + limiter.RecordFailure(partition); + + Assert.Equal(ApiKeyThrottleDecision.Allowed, limiter.Check(partition)); + } + + /// Reaching the limit inside the window throttles the composite partition. + [Fact] + public void Check_AtLimit_ThrottlesCompositePartition() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + RecordFailures(limiter, partition, 3); + + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(partition)); + } + + /// Failures older than the sliding window are pruned, releasing the throttle. + [Fact] + public void Window_PrunesExpiredFailures_ReleasesThrottle() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + RecordFailures(limiter, partition, 3); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(partition)); + + clock.Advance(Window + TimeSpan.FromSeconds(1)); + + Assert.Equal(ApiKeyThrottleDecision.Allowed, limiter.Check(partition)); + } + + /// + /// The composite partition binds a throttle to the failing address: the same key id presented + /// from a different transport peer is unaffected. This is the structural half of the SEC-31 fix. + /// + [Fact] + public void CompositePartition_ThrottleDoesNotFollowKeyIdToAnotherPeer() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 1000); + + RecordFailures(limiter, new ApiKeyThrottlePartition("ipv4:10.0.0.1:1", "victim"), 3); + + Assert.Equal( + ApiKeyThrottleDecision.ThrottledByPeer, + limiter.Check(new ApiKeyThrottlePartition("ipv4:10.0.0.1:1", "victim"))); + Assert.Equal( + ApiKeyThrottleDecision.Allowed, + limiter.Check(new ApiKeyThrottlePartition("ipv4:10.0.0.2:1", "victim"))); + } + + /// + /// Failures for one key id spread across many peers trip the per-key aggregate layer, so a + /// rotating-source sprayer is still bounded even though no single composite partition trips. + /// + [Fact] + public void AggregateLayer_TripsAcrossDistinctPeers() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 100, aggregateLimit: 5); + + for (int peer = 0; peer < 5; peer++) + { + limiter.RecordFailure(new ApiKeyThrottlePartition($"ipv4:10.0.0.{peer}:1", "victim")); + } + + Assert.Equal( + ApiKeyThrottleDecision.ThrottledByAggregate, + limiter.Check(new ApiKeyThrottlePartition("ipv4:10.0.9.9:1", "victim"))); + + // The aggregate is per key id: an unrelated key from the same fresh peer is untouched. + Assert.Equal( + ApiKeyThrottleDecision.Allowed, + limiter.Check(new ApiKeyThrottlePartition("ipv4:10.0.9.9:1", "other-key"))); + } + + /// + /// A throttled partition is a valve, not a wall: exactly one request per probe interval is + /// admitted to the real verifier, so the holder of the correct secret can always get through. + /// + [Fact] + public void ProbeAdmission_AdmitsOneRequestPerInterval() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + RecordFailures(limiter, partition, 3); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(partition)); + + clock.Advance(ProbeInterval); + Assert.Equal(ApiKeyThrottleDecision.ProbeAdmitted, limiter.Check(partition)); + + // The granted slot is consumed: the next request inside the same interval is throttled. + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(partition)); + + clock.Advance(ProbeInterval); + Assert.Equal(ApiKeyThrottleDecision.ProbeAdmitted, limiter.Check(partition)); + } + + /// A zero probe interval restores absolute blocking (documented as not recommended). + [Fact] + public void ProbeIntervalZero_BlocksAbsolutely() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, probeInterval: TimeSpan.Zero); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + RecordFailures(limiter, partition, 3); + + // Well past several probe intervals but still inside the failure window: no slot opens. + clock.Advance(ProbeInterval + ProbeInterval + ProbeInterval); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(partition)); + } + + /// A successful verification clears both the composite partition and the key aggregate. + [Fact] + public void Reset_ClearsCompositeAndAggregateLayers() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 5); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + RecordFailures(limiter, partition, 3); + for (int peer = 1; peer < 3; peer++) + { + limiter.RecordFailure(new ApiKeyThrottlePartition($"ipv4:10.0.1.{peer}:1", "victim")); + } + + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(partition)); + + limiter.Reset(partition); + + Assert.Equal(ApiKeyThrottleDecision.Allowed, limiter.Check(partition)); + Assert.Equal(0, limiter.TrackedAggregateCount); + Assert.Equal( + ApiKeyThrottleDecision.Allowed, + limiter.Check(new ApiKeyThrottlePartition("ipv4:10.0.9.9:1", "victim"))); + } + + /// + /// SEC-32: a spray of unique junk partitions (junk tokens resolve to the sender's fallback + /// partition, one per address) must not evict a partition that is currently throttled — the + /// LRU cap bounds memory, it must not be a reset button for the block. + /// + [Fact] + public void JunkTokenSpray_DoesNotEvictBlockedEntry() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, maxPartitions: 8); + ApiKeyThrottlePartition blocked = new("ipv4:10.0.0.1:1", "victim"); + + RecordFailures(limiter, blocked, 3); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(blocked)); + + for (int i = 0; i < 16; i++) + { + clock.Advance(TimeSpan.FromMilliseconds(1)); + limiter.RecordFailure(new ApiKeyThrottlePartition($"ipv4:10.9.{i / 256}.{i % 256}:1", KeyId: null)); + } + + Assert.True(limiter.IsTracked(blocked)); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(blocked)); + } + + /// + /// SEC-32: one address may mint at most + /// key-id partitions; the overflow collapses into that address's fallback partition, which then + /// throttles the address wholesale instead of letting the spray mint unbounded state. + /// + [Fact] + public void UniqueMxgwKeyIdSpray_FromOnePeer_CollapsesAtPerPeerCap() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, maxPartitions: 4096); + const string peer = "ipv4:10.0.0.1:1"; + + for (int i = 0; i < 1000; i++) + { + limiter.RecordFailure(new ApiKeyThrottlePartition(peer, $"key{i}")); + } + + Assert.True( + limiter.TrackedPartitionCount <= ApiKeyFailureLimiter.MaxKeyIdPartitionsPerPeer + 1, + $"expected at most {ApiKeyFailureLimiter.MaxKeyIdPartitionsPerPeer + 1} partitions, saw {limiter.TrackedPartitionCount}"); + Assert.Equal( + ApiKeyThrottleDecision.ThrottledByPeer, + limiter.Check(new ApiKeyThrottlePartition(peer, "key999"))); + } + + /// + /// SEC-32 eviction preference: with the map at capacity a new failure evicts an entry whose + /// window has fully expired rather than an entry that is still counting. + /// + [Fact] + public void Eviction_PrefersExpiredWindows() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, maxPartitions: 2); + ApiKeyThrottlePartition expired = new("ipv4:10.0.0.1:1", KeyId: null); + ApiKeyThrottlePartition active = new("ipv4:10.0.0.2:1", KeyId: null); + ApiKeyThrottlePartition arriving = new("ipv4:10.0.0.3:1", KeyId: null); + + limiter.RecordFailure(expired); + clock.Advance(Window + TimeSpan.FromSeconds(1)); + limiter.RecordFailure(active); + limiter.RecordFailure(arriving); + + Assert.Equal(2, limiter.TrackedPartitionCount); + Assert.False(limiter.IsTracked(expired)); + Assert.True(limiter.IsTracked(active)); + Assert.True(limiter.IsTracked(arriving)); + } + + private static void RecordFailures(ApiKeyFailureLimiter limiter, ApiKeyThrottlePartition partition, int count) + { + for (int i = 0; i < count; i++) + { + limiter.RecordFailure(partition); + } + } + + private static ApiKeyFailureLimiter CreateLimiter( + TimeProvider clock, + int limit, + int aggregateLimit = 0, + int maxPartitions = 1024, + TimeSpan? probeInterval = null) + { + return new ApiKeyFailureLimiter( + limit, + Window, + maxPartitions, + aggregateLimit, + probeInterval ?? ProbeInterval, + clock); + } +} diff --git a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs index 189b8fa..55d3dd7 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/GatewayGrpcAuthorizationInterceptorTests.cs @@ -22,6 +22,12 @@ namespace ZB.MOM.WW.MxGateway.Tests.Security.Authorization; public sealed class GatewayGrpcAuthorizationInterceptorTests { + private const string AttackerPeer = "ipv4:203.0.113.7:5000"; + private const string HolderPeer = "ipv4:198.51.100.4:5000"; + + private static readonly TimeSpan FailureWindow = TimeSpan.FromMinutes(1); + private static readonly TimeSpan ProbeInterval = TimeSpan.FromSeconds(5); + /// Verifies that missing API key returns unauthenticated status. /// A task that represents the asynchronous operation. [Fact] @@ -359,21 +365,18 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests } /// - /// Once a peer has exceeded the failure limit, the interceptor short-circuits with - /// BEFORE calling the verifier, so an online guessing - /// loop stops spending a store read per attempt. A verifier that always fails is used; after the - /// limit is reached the verifier is no longer invoked. + /// SEC-31: once an attacking peer has exceeded the failure limit for a key id, the interceptor + /// short-circuits with BEFORE calling the verifier, so + /// an online guessing loop stops spending a store read per attempt. The composite + /// (peer, key id) partition keeps that bound per attacking address. /// /// A task that represents the asynchronous operation. [Fact] - public async Task UnaryServerHandler_ExceedsFailureLimit_ShortCircuitsBeforeVerify() + public async Task BruteForceBound_StillEnforcedPerAttackingPeer() { CountingFailureVerifier verifier = new(Failure(ApiKeyFailure.SecretMismatch)); - ApiKeyFailureLimiter limiter = new( - limit: 3, - window: TimeSpan.FromMinutes(1), - maxPeers: 16, - clock: TimeProvider.System); + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3); GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( verifier, new GatewayRequestIdentityAccessor(), @@ -385,7 +388,7 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests RpcException failure = await Assert.ThrowsAsync( () => interceptor.UnaryServerHandler( new OpenSessionRequest(), - ContextWithAuthorization("Bearer mxgw_operator01_bad-secret"), + ContextWithAuthorization("Bearer mxgw_operator01_bad-secret", AttackerPeer), (_, _) => Task.FromResult(new OpenSessionReply()))); Assert.Equal(StatusCode.Unauthenticated, failure.StatusCode); } @@ -396,13 +399,219 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests RpcException throttled = await Assert.ThrowsAsync( () => interceptor.UnaryServerHandler( new OpenSessionRequest(), - ContextWithAuthorization("Bearer mxgw_operator01_bad-secret"), + ContextWithAuthorization("Bearer mxgw_operator01_bad-secret", AttackerPeer), (_, _) => Task.FromResult(new OpenSessionReply()))); Assert.Equal(StatusCode.ResourceExhausted, throttled.StatusCode); Assert.Equal(3, verifier.CallCount); } + /// + /// SEC-31 (the lockout inversion): an attacker who floods failures for a victim's key id from its + /// own address must not deny that key to the legitimate holder. The holder presents the correct + /// secret from a different transport peer and authenticates on the first attempt — the verifier + /// is reached and the RPC succeeds. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AttackerSpamOnVictimKeyId_FromDifferentPeer_DoesNotBlockLegitimateHolderPresentingCorrectSecret() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 30); + GatewayGrpcAuthorizationInterceptor attacked = CreateInterceptor( + new CountingFailureVerifier(Failure(ApiKeyFailure.SecretMismatch)), + new GatewayRequestIdentityAccessor(), + failureLimiter: limiter); + + // Flood the victim's key id from the attacker's address until that partition is throttled. + StatusCode lastAttackerStatus = StatusCode.OK; + for (int attempt = 0; attempt < 6; attempt++) + { + RpcException failure = await Assert.ThrowsAsync( + () => attacked.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_guess", AttackerPeer), + (_, _) => Task.FromResult(new OpenSessionReply()))); + lastAttackerStatus = failure.StatusCode; + } + + Assert.Equal(StatusCode.ResourceExhausted, lastAttackerStatus); + + // The legitimate holder, on a different address, is verified and admitted immediately. + FakeApiKeyVerifier holderVerifier = new(SuccessWithScopes(GatewayScopes.SessionOpen)); + GatewayGrpcAuthorizationInterceptor holder = CreateInterceptor( + holderVerifier, + new GatewayRequestIdentityAccessor(), + failureLimiter: limiter); + + OpenSessionReply reply = await holder.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_correct", HolderPeer), + (_, _) => Task.FromResult(new OpenSessionReply { SessionId = "session-1" })); + + Assert.True(holderVerifier.WasCalled); + Assert.Equal("session-1", reply.SessionId); + } + + /// + /// SEC-31 layer 2: failures for one key id sprayed across more distinct peers than + /// ApiKeyFailureAggregateLimit put that key id into probe mode globally, so a + /// rotating-source attacker gets at most one verifier call per probe interval. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task AggregateSpray_AcrossManyPeers_TripsPerKeyProbeMode() + { + CountingFailureVerifier verifier = new(Failure(ApiKeyFailure.SecretMismatch)); + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 100, aggregateLimit: 5); + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + verifier, + new GatewayRequestIdentityAccessor(), + failureLimiter: limiter); + + for (int peer = 0; peer < 5; peer++) + { + await Assert.ThrowsAsync( + () => interceptor.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_guess", $"ipv4:10.9.0.{peer}:5000"), + (_, _) => Task.FromResult(new OpenSessionReply()))); + } + + Assert.Equal(5, verifier.CallCount); + + // A never-seen peer is now probe-limited: no composite failures of its own, but the key id's + // aggregate is tripped, so the request never reaches the verifier. + RpcException throttled = await Assert.ThrowsAsync( + () => interceptor.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_guess", "ipv4:10.9.1.1:5000"), + (_, _) => Task.FromResult(new OpenSessionReply()))); + + Assert.Equal(StatusCode.ResourceExhausted, throttled.StatusCode); + Assert.Equal(5, verifier.CallCount); + + // One probe slot opens per interval, and it is consumed by the first arrival. + clock.Advance(ProbeInterval); + + RpcException probed = await Assert.ThrowsAsync( + () => interceptor.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_guess", "ipv4:10.9.1.2:5000"), + (_, _) => Task.FromResult(new OpenSessionReply()))); + + Assert.Equal(StatusCode.Unauthenticated, probed.StatusCode); + Assert.Equal(6, verifier.CallCount); + + RpcException throttledAgain = await Assert.ThrowsAsync( + () => interceptor.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_guess", "ipv4:10.9.1.3:5000"), + (_, _) => Task.FromResult(new OpenSessionReply()))); + + Assert.Equal(StatusCode.ResourceExhausted, throttledAgain.StatusCode); + Assert.Equal(6, verifier.CallCount); + } + + /// + /// SEC-31: the success-reset path stays reachable while throttled. A throttled partition admits + /// one probe per interval; the correct secret rides that slot, authenticates, and fully clears + /// both limiter layers, so the next wrong attempt is + /// rather than . + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task CorrectSecret_DuringProbeMode_AuthenticatesViaProbeSlotAndResets() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 30); + GatewayGrpcAuthorizationInterceptor failing = CreateInterceptor( + new FakeApiKeyVerifier(Failure(ApiKeyFailure.SecretMismatch)), + new GatewayRequestIdentityAccessor(), + failureLimiter: limiter); + FakeApiKeyVerifier holderVerifier = new(SuccessWithScopes(GatewayScopes.SessionOpen)); + GatewayGrpcAuthorizationInterceptor succeeding = CreateInterceptor( + holderVerifier, + new GatewayRequestIdentityAccessor(), + failureLimiter: limiter); + + for (int attempt = 0; attempt < 3; attempt++) + { + await Assert.ThrowsAsync( + () => failing.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_bad", HolderPeer), + (_, _) => Task.FromResult(new OpenSessionReply()))); + } + + RpcException throttled = await Assert.ThrowsAsync( + () => failing.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_bad", HolderPeer), + (_, _) => Task.FromResult(new OpenSessionReply()))); + Assert.Equal(StatusCode.ResourceExhausted, throttled.StatusCode); + + clock.Advance(ProbeInterval); + + OpenSessionReply reply = await succeeding.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_correct", HolderPeer), + (_, _) => Task.FromResult(new OpenSessionReply { SessionId = "session-1" })); + + Assert.True(holderVerifier.WasCalled); + Assert.Equal("session-1", reply.SessionId); + + RpcException afterReset = await Assert.ThrowsAsync( + () => failing.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization("Bearer mxgw_operator01_bad", HolderPeer), + (_, _) => Task.FromResult(new OpenSessionReply()))); + + Assert.Equal(StatusCode.Unauthenticated, afterReset.StatusCode); + } + + /// + /// SEC-32: only a validly shaped mxgw_<keyId>_<secret> token mints a key-id + /// partition. Junk tokens of varied shapes all collapse onto the sender's transport-peer fallback + /// partition, so a spray cannot mint one tracked entry per invented token. + /// + /// A task that represents the asynchronous operation. + [Fact] + public async Task NonMxgwToken_FallsBackToTransportPeerPartition() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 1000); + GatewayGrpcAuthorizationInterceptor interceptor = CreateInterceptor( + new FakeApiKeyVerifier(Failure(ApiKeyFailure.MissingOrMalformed)), + new GatewayRequestIdentityAccessor(), + failureLimiter: limiter); + + string[] junkTokens = + [ + "Bearer garbage", + "Bearer a_b_c", + "Bearer notmxgw_operator01_secret", + "Bearer MXGW_operator01_secret", + "Bearer mxgw__secret", + "Bearer mxgw_operator01_", + "Bearer mxgw_" + new string('k', 65) + "_secret", + "Bearer mxgw_onlytwo", + ]; + + foreach (string token in junkTokens) + { + await Assert.ThrowsAsync( + () => interceptor.UnaryServerHandler( + new OpenSessionRequest(), + ContextWithAuthorization(token, AttackerPeer), + (_, _) => Task.FromResult(new OpenSessionReply()))); + } + + Assert.Equal(1, limiter.TrackedPartitionCount); + Assert.True(limiter.IsTracked(new ApiKeyThrottlePartition(AttackerPeer, KeyId: null))); + } + /// /// A successful verification resets the peer's failure counter, so accumulated failures /// from a fat-fingered secret do not lock out a client that subsequently authenticates. @@ -411,11 +620,7 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests [Fact] public async Task UnaryServerHandler_SuccessResetsFailureCounter() { - ApiKeyFailureLimiter limiter = new( - limit: 3, - window: TimeSpan.FromMinutes(1), - maxPeers: 16, - clock: TimeProvider.System); + ApiKeyFailureLimiter limiter = CreateLimiter(new ManualTimeProvider(DateTimeOffset.UnixEpoch), limit: 3); // Two failures against the same key id, then a success (which resets), then two more // failures — without the reset the fifth attempt would be blocked at the limit of 3. @@ -487,11 +692,23 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests Mode = authenticationMode } }), - failureLimiter ?? new ApiKeyFailureLimiter( - limit: 1000, - window: TimeSpan.FromMinutes(1), - maxPeers: 1000, - clock: TimeProvider.System)); + failureLimiter ?? CreateLimiter(TimeProvider.System, limit: 1000), + new GatewayMetrics()); + } + + private static ApiKeyFailureLimiter CreateLimiter( + TimeProvider clock, + int limit, + int aggregateLimit = 0, + int maxPartitions = 1024) + { + return new ApiKeyFailureLimiter( + limit, + FailureWindow, + maxPartitions, + aggregateLimit, + ProbeInterval, + clock); } private static ApiKeyVerification SuccessWithScopes(params string[] scopes) @@ -511,9 +728,11 @@ public sealed class GatewayGrpcAuthorizationInterceptorTests return new ApiKeyVerification(Succeeded: false, Identity: null, Failure: failure); } - private static TestServerCallContext ContextWithAuthorization(string authorizationHeader) + private static TestServerCallContext ContextWithAuthorization(string authorizationHeader, string? peer = null) { - return new TestServerCallContext([new Metadata.Entry("authorization", authorizationHeader)]); + return new TestServerCallContext( + [new Metadata.Entry("authorization", authorizationHeader)], + peer: peer); } /// Records whether the gateway service ran past the interceptor for composition tests. diff --git a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/TestServerCallContext.cs b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/TestServerCallContext.cs index b29958b..53ad38a 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/TestServerCallContext.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/TestSupport/TestServerCallContext.cs @@ -8,20 +8,32 @@ namespace ZB.MOM.WW.MxGateway.Tests.TestSupport; /// public sealed class TestServerCallContext : ServerCallContext { + private const string DefaultPeer = "ipv4:127.0.0.1:5000"; + private readonly Metadata _requestHeaders; private readonly Metadata _responseTrailers = []; private readonly Dictionary _userState = []; private readonly CancellationToken _cancellationToken; + private readonly string _peer; private Status _status; private WriteOptions? _writeOptions; /// Initializes the context with the supplied request headers and cancellation token. /// Request headers visible to the service; defaults to empty. /// Cancellation token surfaced to the service. - public TestServerCallContext(Metadata? requestHeaders = null, CancellationToken cancellationToken = default) + /// + /// Transport peer address surfaced as ; defaults to a + /// loopback address. Tests that exercise per-peer partitioning (for example the API-key failure + /// limiter) pass distinct values to model separate network sources. + /// + public TestServerCallContext( + Metadata? requestHeaders = null, + CancellationToken cancellationToken = default, + string? peer = null) { _requestHeaders = requestHeaders ?? []; _cancellationToken = cancellationToken; + _peer = peer ?? DefaultPeer; } /// @@ -31,7 +43,7 @@ public sealed class TestServerCallContext : ServerCallContext protected override string HostCore => "localhost"; /// - protected override string PeerCore => "ipv4:127.0.0.1:5000"; + protected override string PeerCore => _peer; /// protected override DateTime DeadlineCore => DateTime.UtcNow.AddMinutes(1); From acebe1877394ac34a721bc3759dc441f0128e0d2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 05:57:26 -0400 Subject: [PATCH 2/3] fix(SEC-31,SEC-32): make probe admission atomic and stop Reset clearing a shared fallback partition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found in code review of the limiter rework. Probe admission was check-then-act across two lock scopes: Check() read "probe due" under lock(state), released it, then re-acquired to advance NextProbeAtTicks. A burst of requests arriving together at an interval boundary could therefore all observe the slot as due and all be admitted, handing the verifier the very burst the interval exists to bound. The claim is now a single critical section (TryConsumeProbe). The two layers are still claimed one at a time — holding two per-state locks at once would need a global lock ordering to stay deadlock-free — so a slot claimed on the composite partition is compensated via ReleaseProbe when the aggregate then refuses, which otherwise silently spent the partition's next slot and pushed the legitimate holder out by a full interval. Reset() removed whatever partition the caller resolved to, including the address's shared fallback partition when the caller's key id had been collapsed into it by the per-peer cap (or when the token was junk-shaped). That bucket also carries failures contributed by other key ids from the same address, so one successful authentication became a reset button for an in-progress spray. Reset now clears only a partition the caller owns (effectiveKeyId == presented key id); the shared bucket decays by window expiry instead, and the caller still recovers through probe admission. The key's aggregate is cleared either way, as designed. Also applied from the review: closure-free GetOrAdd overload on _partitions, and a remarks paragraph acknowledging the best-effort O(n) eviction scan under sustained overflow. Threading the resolved partition key from Check through to RecordFailure/Reset was declined: Check resolves with mint:false and RecordFailure with mint:true, and the two can legitimately differ when a concurrent caller fills the per-peer cap in between — reusing Check's key would record into the wrong partition and bypass the cap, which is not worth saving one string concat. Tests (limiter suite 11 -> 14): ProbeAdmission_UnderConcurrentArrivals_ GrantsExactlyOneSlot (200 rounds x 8 barrier-released threads at the boundary), ProbeAdmission_WhenAggregateRefuses_ReturnsTheClaimedPeerSlot, and Reset_WithOverCapKeyId_DoesNotClearSharedFallbackPartition. The latter two were confirmed as genuine reds against the unfixed code; the concurrency test is a guard — it is deterministically green on the fixed structure but did not reproduce the original nanosecond-wide window on its own. --- .../2026-07-12/remediation/00-tracking.md | 2 +- docs/Authorization.md | 2 +- docs/GatewayConfiguration.md | 2 +- .../Authorization/ApiKeyFailureLimiter.cs | 89 ++++++++---- .../ApiKeyFailureLimiterTests.cs | 127 ++++++++++++++++++ 5 files changed, 196 insertions(+), 26 deletions(-) diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index f4c02ca..bc9c721 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. | +| 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). | diff --git a/docs/Authorization.md b/docs/Authorization.md index c48851f..52f1765 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. A successful verification resets both layers — which is why the reset path stays reachable while a key is under active spray. 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. 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/docs/GatewayConfiguration.md b/docs/GatewayConfiguration.md index a4ed41f..cd82100 100644 --- a/docs/GatewayConfiguration.md +++ b/docs/GatewayConfiguration.md @@ -364,7 +364,7 @@ model requires otherwise. | `MxGateway:Security:ApiKeyFailureLimit` | `10` | Failed API-key verifications, per `(transport peer, key id)` partition, within `ApiKeyFailureWindowSeconds` that trip the in-process short-circuit. Once tripped, the gRPC auth path rejects further attempts from that partition with `ResourceExhausted` **before** the store read — except for the probe admitted every `ApiKeyFailureProbeIntervalSeconds` — and a successful verification resets the partition. The partition always includes the sender's transport address: key ids are public (they ride in every token and are listed on the dashboard), so keying on the key id alone let any peer deny a key to its legitimate holder. Must be greater than zero. | | `MxGateway:Security:ApiKeyFailureWindowSeconds` | `60` | Sliding-window length, in seconds, over which API-key verification failures are counted, for both the per-partition and the per-key-id aggregate layer. Must be greater than zero. | | `MxGateway:Security:ApiKeyFailureAggregateLimit` | `30` | Failed verifications for one key id counted across **all** transport peers within `ApiKeyFailureWindowSeconds` before that key id enters probe mode. This second layer bounds a distributed or source-rotating sprayer that never trips any single `(peer, key id)` partition. `0` disables the aggregate layer, leaving only per-partition counting. Must be zero or greater. | -| `MxGateway:Security:ApiKeyFailureProbeIntervalSeconds` | `5` | Minimum interval, in seconds, between probe admissions for an over-limit partition or key-id aggregate. An over-limit state is a valve rather than a wall: one request per interval reaches the real verifier, so the holder of the correct secret always gets through and clears the state, while everything else is still refused before the store read. `0` blocks absolutely instead — **not recommended**, because an unauthenticated peer can then deny the key to its holder for the whole window. Must be zero or greater. | +| `MxGateway:Security:ApiKeyFailureProbeIntervalSeconds` | `5` | Minimum interval, in seconds, between probe admissions for an over-limit partition or key-id aggregate. An over-limit state is a valve rather than a wall: one request per interval reaches the real verifier — exactly one, even when a burst arrives together at the interval boundary — so the holder of the correct secret always gets through and clears the state, while everything else is still refused before the store read. `0` blocks absolutely instead — **not recommended**, because an unauthenticated peer can then deny the key to its holder for the whole window. Must be zero or greater. | | `MxGateway:Security:ApiKeyFailureTrackedPeers` | `4096` | Maximum distinct partitions tracked by the failure counter (a bounded LRU) so a spray of unique tokens cannot grow memory without limit. It cannot be used to flush an active block either: only a validly shaped `mxgw__` token mints a key-id partition (everything else lands on the sender's transport-peer partition), each address may mint at most 32 key-id partitions before the overflow collapses onto that address's fallback partition, and eviction prefers fully expired windows, never removing an over-limit partition until the map exceeds twice this cap. Must be greater than zero. | ## Galaxy 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 38df873..db5131d 100644 --- a/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs +++ b/src/ZB.MOM.WW.MxGateway.Server/Security/Authorization/ApiKeyFailureLimiter.cs @@ -35,6 +35,13 @@ namespace ZB.MOM.WW.MxGateway.Server.Security.Authorization; /// peers are removed on reset, so in steady state the map holds only partitions with recent failures /// — the common success path is a lock-free dictionary miss. /// +/// +/// Eviction is deliberately a best-effort full scan of the over-capacity map, run only on the +/// failure path and only once the cap is exceeded. Under a sustained overflow that scan is O(n) per +/// recorded failure, which is accepted: n is bounded by the cap, the work lands on attack traffic +/// rather than on authenticated calls, and an exact ordered structure would need a second index kept +/// consistent with the per-state locks for no security gain. +/// /// public sealed class ApiKeyFailureLimiter { @@ -140,26 +147,29 @@ public sealed class ApiKeyFailureLimiter return peerOver ? ApiKeyThrottleDecision.ThrottledByPeer : ApiKeyThrottleDecision.ThrottledByAggregate; } - // Both over-limit layers must have a slot before either is consumed, so a request cannot burn - // the peer's probe and then be refused by the aggregate. - if (peerOver && !IsProbeDue(peerState!, now)) - { - return ApiKeyThrottleDecision.ThrottledByPeer; - } - - if (aggregateOver && !IsProbeDue(aggregateState!, now)) - { - return ApiKeyThrottleDecision.ThrottledByAggregate; - } - + // 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; + bool peerProbeClaimed = false; if (peerOver) { - ConsumeProbe(peerState!, now); + if (!TryConsumeProbe(peerState!, now, out peerProbeRestore)) + { + return ApiKeyThrottleDecision.ThrottledByPeer; + } + + peerProbeClaimed = true; } - if (aggregateOver) + if (aggregateOver && !TryConsumeProbe(aggregateState!, now, out _)) { - ConsumeProbe(aggregateState!, now); + if (peerProbeClaimed) + { + ReleaseProbe(peerState!, now, peerProbeRestore); + } + + return ApiKeyThrottleDecision.ThrottledByAggregate; } return ApiKeyThrottleDecision.ProbeAdmitted; @@ -178,7 +188,10 @@ public sealed class ApiKeyFailureLimiter long now = _clock.GetUtcNow().UtcTicks; (string partitionKey, string? effectiveKeyId) = ResolvePartitionKey(peer, partition.KeyId, mint: true); - WindowState state = _partitions.GetOrAdd(partitionKey, _ => new WindowState(peer, effectiveKeyId)); + WindowState state = _partitions.GetOrAdd( + partitionKey, + static (_, owner) => new WindowState(owner.Peer, owner.KeyId), + (Peer: peer, KeyId: effectiveKeyId)); RecordInto(state, now, _limit); // Only a key id that earned its own partition feeds the aggregate: an id squeezed out by the @@ -198,8 +211,17 @@ public sealed class ApiKeyFailureLimiter public void Reset(ApiKeyThrottlePartition partition) { string peer = RequirePeer(partition); - (string partitionKey, _) = ResolvePartitionKey(peer, partition.KeyId, mint: false); - RemovePartition(partitionKey); + (string partitionKey, string? effectiveKeyId) = ResolvePartitionKey(peer, partition.KeyId, mint: false); + + // Clear only a partition this caller actually owns. When its key id was squeezed into the + // address's shared fallback bucket by the per-peer cap, that bucket also holds failures + // contributed by other key ids — and by junk-shaped tokens — from the same address, so + // removing it would let one successful authentication wipe an in-progress spray. The shared + // bucket decays by window expiry instead; the caller still recovers through probe admission. + if (string.Equals(effectiveKeyId, partition.KeyId, StringComparison.Ordinal)) + { + RemovePartition(partitionKey); + } // The aggregate is cleared on the presented key id even when the composite partition // collapsed to the fallback: a verified secret is proof the key is not under successful @@ -264,20 +286,41 @@ public sealed class ApiKeyFailureLimiter } } - private bool IsProbeDue(WindowState state, long now) + /// + /// Atomically claims this interval's probe slot. Checking and advancing must happen in one + /// critical section: a due-check that released the lock before advancing would let every request + /// 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) { lock (state) { - return now >= state.NextProbeAtTicks; + previousProbeAtTicks = state.NextProbeAtTicks; + if (now < previousProbeAtTicks) + { + return false; + } + + state.NextProbeAtTicks = now + _probeIntervalTicks; + state.LastActivityTicks = now; + return true; } } - private void ConsumeProbe(WindowState state, long now) + /// + /// 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. + /// + private void ReleaseProbe(WindowState state, long now, long previousProbeAtTicks) { lock (state) { - state.NextProbeAtTicks = now + _probeIntervalTicks; - state.LastActivityTicks = now; + if (state.NextProbeAtTicks == now + _probeIntervalTicks) + { + state.NextProbeAtTicks = previousProbeAtTicks; + } } } 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 715a0c0..95d7935 100644 --- a/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs +++ b/src/ZB.MOM.WW.MxGateway.Tests/Security/Authorization/ApiKeyFailureLimiterTests.cs @@ -125,6 +125,100 @@ public sealed class ApiKeyFailureLimiterTests Assert.Equal(ApiKeyThrottleDecision.ProbeAdmitted, limiter.Check(partition)); } + /// + /// The probe slot is claimed atomically: when a crowd of requests arrives at the same interval + /// boundary exactly one is admitted and the rest are still refused. A check-then-act grant would + /// let every arrival observe "due" and hand the whole burst through to the verifier. + /// + [Fact] + public void ProbeAdmission_UnderConcurrentArrivals_GrantsExactlyOneSlot() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 5); + ApiKeyThrottlePartition partition = new("ipv4:10.0.0.1:1", "victim"); + + // Five failures on one partition trip both layers (limit 3, aggregate 5), so the concurrent + // arrivals contend for the composite probe slot AND the aggregate's. + RecordFailures(limiter, partition, 5); + clock.Advance(ProbeInterval); + + // The unsafe window between reading "probe due" and advancing the slot is nanoseconds wide, + // so one burst can miss it by luck. Repeating the boundary makes the red reliable, while the + // atomic implementation must yield exactly one admission in every round. + const int arrivals = 8; + const int rounds = 200; + int totalAdmitted = 0; + + for (int round = 0; round < rounds; round++) + { + // Re-arm: the failures keep both layers over their limits, and recording pushes the next + // probe one interval out, which the advance below then reaches. + RecordFailures(limiter, partition, 5); + clock.Advance(ProbeInterval); + + ApiKeyThrottleDecision[] decisions = new ApiKeyThrottleDecision[arrivals]; + using (Barrier startLine = new(arrivals)) + { + Thread[] threads = new Thread[arrivals]; + for (int index = 0; index < arrivals; index++) + { + int slot = index; + threads[slot] = new Thread(() => + { + startLine.SignalAndWait(); + decisions[slot] = limiter.Check(partition); + }); + threads[slot].Start(); + } + + foreach (Thread thread in threads) + { + Assert.True(thread.Join(TimeSpan.FromSeconds(30)), "probe-contention thread did not finish"); + } + } + + int admitted = decisions.Count(decision => decision == ApiKeyThrottleDecision.ProbeAdmitted); + Assert.Equal( + arrivals - admitted, + decisions.Count(decision => decision is ApiKeyThrottleDecision.ThrottledByPeer + or ApiKeyThrottleDecision.ThrottledByAggregate)); + totalAdmitted += admitted; + } + + Assert.Equal(rounds, totalAdmitted); + } + + /// + /// The layers claim their probe slots one at a time, so a slot claimed on the composite partition + /// must be returned when the aggregate then refuses. Otherwise a refused request would silently + /// spend the partition's next slot and push the legitimate holder out by a full interval. + /// + [Fact] + public void ProbeAdmission_WhenAggregateRefuses_ReturnsTheClaimedPeerSlot() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 5); + ApiKeyThrottlePartition holder = new("ipv4:10.0.0.1:1", "victim"); + + // Both layers trip together, arming both slots for t0 + interval. + RecordFailures(limiter, holder, 5); + + // A failure from a second address re-arms only the aggregate (its own composite is far under + // the limit), so the aggregate's slot now opens one interval later than the partition's. + clock.Advance(TimeSpan.FromSeconds(3)); + limiter.RecordFailure(new ApiKeyThrottlePartition("ipv4:10.0.0.2:1", "victim")); + + // t0 + 5s: the partition's slot is due, the aggregate's is not — the request is refused and + // the partition's slot must be handed back rather than consumed. + clock.Advance(TimeSpan.FromSeconds(2)); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByAggregate, limiter.Check(holder)); + + // t0 + 8s: the aggregate's slot opens. The partition's slot was restored, so this passes; had + // it been consumed above it would not reopen until t0 + 10s and this would be ThrottledByPeer. + clock.Advance(TimeSpan.FromSeconds(3)); + Assert.Equal(ApiKeyThrottleDecision.ProbeAdmitted, limiter.Check(holder)); + } + /// A zero probe interval restores absolute blocking (documented as not recommended). [Fact] public void ProbeIntervalZero_BlocksAbsolutely() @@ -165,6 +259,39 @@ public sealed class ApiKeyFailureLimiterTests limiter.Check(new ApiKeyThrottlePartition("ipv4:10.0.9.9:1", "victim"))); } + /// + /// A success whose key id was squeezed into the address's shared fallback bucket must not clear + /// that bucket: it carries failures from other key ids at the same address, so clearing it would + /// make one successful authentication a reset button for an in-progress spray. + /// + [Fact] + public void Reset_WithOverCapKeyId_DoesNotClearSharedFallbackPartition() + { + ManualTimeProvider clock = new(DateTimeOffset.UnixEpoch); + ApiKeyFailureLimiter limiter = CreateLimiter(clock, limit: 3, aggregateLimit: 100, maxPartitions: 4096); + const string peer = "ipv4:10.0.0.1:1"; + + // Fill the per-peer key-id cap, then spray past it so the overflow lands on — and trips — + // the address's shared fallback partition. + for (int i = 0; i < ApiKeyFailureLimiter.MaxKeyIdPartitionsPerPeer; i++) + { + limiter.RecordFailure(new ApiKeyThrottlePartition(peer, $"key{i}")); + } + + for (int i = 0; i < 5; i++) + { + limiter.RecordFailure(new ApiKeyThrottlePartition(peer, $"overflow{i}")); + } + + ApiKeyThrottlePartition overCap = new(peer, "overflow0"); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(overCap)); + + limiter.Reset(overCap); + + Assert.True(limiter.IsTracked(overCap)); + Assert.Equal(ApiKeyThrottleDecision.ThrottledByPeer, limiter.Check(overCap)); + } + /// /// SEC-32: a spray of unique junk partitions (junk tokens resolve to the sender's fallback /// partition, one per address) must not evict a partition that is currently throttled — the From 5b681ee59b58e0712ae512f9ec50ae13d1008b64 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:10:14 -0400 Subject: [PATCH 3/3] fix(SEC-31,SEC-32): identify a probe-slot reservation by version, not by timestamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReleaseProbe recognised its own reservation by comparing NextProbeAtTicks to now + _probeIntervalTicks. RecordInto's rearm-on-trip writes that identical expression, so a concurrent RecordFailure on the same WindowState whose `now` lands on the claimer's tick — routine at ~1 ms clock resolution under load — was mistaken for the caller's own claim. The release then stomped the legitimate fresh re-arm back to the stale previousProbeAtTicks, which is already due, handing the next arrival a free probe the re-arm had just closed. WindowState gains a monotonic ProbeVersion bumped by every writer of NextProbeAtTicks (TryConsumeProbe's claim and RecordInto's re-arm alike). TryConsumeProbe returns the stamp it set as part of a ProbeClaim; ReleaseProbe restores the previous value only while the state's version still equals that stamp, checking and restoring in one lock(state) section and bumping the version again on restore so no other stale release can match either. Test: ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick, with the clock held still so the claim and the interleaved failure necessarily share a tick. Making it deterministic needed a seam — the claim-to-release window is a few nanoseconds and racing threads do not hit it (an earlier thread-based attempt passed against the defective guard three runs out of three, and its end state was ordering-dependent rather than correctness-dependent, so it was dropped rather than shipped as theatre). The seam is an internal ProbeReleaseInterleaveHook, null in production, costing one null check on the already-refused path. Verified as a genuine red against the timestamp guard: Expected ThrottledByPeer, Actual ProbeAdmitted. --- .../2026-07-12/remediation/00-tracking.md | 2 +- docs/Authorization.md | 2 +- .../Authorization/ApiKeyFailureLimiter.cs | 63 +++++++++++++++---- .../ApiKeyFailureLimiterTests.cs | 48 ++++++++++++++ 4 files changed, 102 insertions(+), 13 deletions(-) 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()