3f854d6cbf53635847255c778a22299ecdad1c6e
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Joseph Doherty
|
5b681ee59b |
fix(SEC-31,SEC-32): identify a probe-slot reservation by version, not by timestamp
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. |
||
|
Joseph Doherty
|
acebe18773 |
fix(SEC-31,SEC-32): make probe admission atomic and stop Reset clearing a shared fallback partition
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. |
||
|
Joseph Doherty
|
df710e18a9 |
fix(SEC-31,SEC-32): re-partition the API-key failure limiter on (peer, key id) with probe admission
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). |