security(mcp): MCP Agent Security Gateway backend review and hardening - #1224
Merged
Conversation
An unverified `DPoP:` request header raised the subject's effective assurance to High. Under a BearerControlled sender profile that proof is never verified at all, so an attacker holding an ordinary bearer token satisfied the operator's MinAssurance admission floor — and every `principal.assurance` / `session.assurance` policy condition — by adding one junk header. The value also reached the durable decision event, so the archive recorded an assurance level nothing had checked. Effective assurance is now the caller's asserted level CLAMPED to what the verified sender constraint justifies (DPoP/mTLS => High, unbound bearer => Low), computed after verifySenderConstraint and used for both the floor check and the resolved context. The clamp is scoped to Human subjects — the only free-form caller assertion in this API. A workload's level is already derived from evidence authn itself checks (Workload.Attestation), so attested workloads keep High under any profile. The runtime no longer derives assurance from request shape at all: it requests the maximum a verified binding could justify and lets the clamp decide. Tests: internal/mcp/authn/assurance_test.go. The two defect tests fail against the pre-fix tree; the ceiling-not-floor, verified-DPoP-preserves-High and attested-workload cases pin the boundaries the fix must not cross. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Authorization already rejected duplicates as an ambiguous credential source, but Origin, DPoP, Mcp-Session-Id and MCP-Protocol-Version were read first-value-wins. An intermediary, a WAF and the gateway can each resolve such a conflict differently, which is the classic header-confusion / request-smuggling shape: the cross-origin decision, the sender-constraint proof, session resolution and version admission were all resolvable by picking a value. extractRequest now rejects the request whole (400) when any guarded singleton appears more than once, before any value is read. Duplication alone is the trigger — two identical values are just as ambiguous to a middlebox that forwards only one of them. The offending header name is not echoed back. Adds mcperr.ReasonAmbiguousRequestHeader, appended at the END of the enum so no existing ordinal moves. Tests include an anti-weakening case pinning the guarded set, so a new security-relevant header cannot be added to the request path without a guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…h stages Three related defects on the live Gateway request path. 1. RequestDeadline bounded nothing that costs anything. ServeHTTP built a deadline context, used it only to acquire a worker slot, then cancelled it via defer while pipeline.Process — token validation, policy evaluation, inspection, durable event commits — ran with no deadline at all. Process now takes the context and checks it at each stage boundary; an expired budget is a 503 carrying the already-defined (and until now never produced) ReasonRequestDeadlineExceeded. 2. The guarded executor and request inspection were called with context.Background(), so a disconnected client, an exhausted budget or a shutdown could not stop an in-flight upstream call, and every ctx-honouring stage below them (broker, provider, dial, TLS, response read, response inspection) silently lost its bound. Fixed now, while execution is still dormant, because it is a precondition of ever arming it. 3. AuthConcurrency and DPoPConcurrency were validated at construction, ceiling-checked, exposed as accessors — and never read. An operator setting them to throttle signature verification or introspection got nothing. Both are now per-pipeline semaphores held for exactly the work they bound. Also hoists the credential presence/shape pre-check ahead of registry resolution and body buffering. This closes an unauthenticated server-existence ORACLE (a credential-less probe got 404 for an unknown server id and 401 for a registered one, enumerating a tenant's MCP servers) and stops a credential-less request from buffering up to MaxBodyBytes, running the strict decoder, or churning a session open/close. The rejection keeps the same counter, denial-lane routing, reason and status the later stage used to produce. Tests include a structural anti-weakening case forbidding a detached context anywhere in the request path, and mutation-verified guards for the deadline, the executor context, the auth semaphore and the pre-check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…effect The guarded executor's durable decision event is the only authoritative record of who caused an irreversible upstream side effect, and it was wrong in three ways. 1. PrincipalType was hard-coded "workload" while the runtime models token subjects as humans, so every execution event misattributed a human actor. It now mirrors the authenticated subject kind, and an unset kind stays empty rather than inventing a type. 2. The decision event dropped the client id, server id, tool name/fingerprint, matched rule, policy revision, operation class and snapshot hash — a destructive execution whose event names no target cannot be attributed after the fact. All are now carried. 3. outcomeFacts relabelled every outcome as ActionClassRead, so the archive's record of what a destructive call DID contradicted its own decision event. The outcome keeps ordinary criticality (it must never block the response) but the real action class. Separately, commit-before-side-effect only held on one branch: the no-credential path went through CommitThenAct, while the credential-profile path — the ordinary enterprise shape — relied solely on the broker's CREDENTIAL_SELECT gate and never committed the decision event at all. Both paths now run through the same CommitThenAct, with materialization inside the callback; the broker's pre-materialization gate remains as defense in depth, not a substitute. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…edirects roundTrip builds a fresh http.Transport per attempt. A Go Transport OWNS its idle connections and this one sets no IdleConnTimeout, so once the Transport went out of scope nothing reclaimed its idle connections or their read/write loop goroutines. Every upstream call permanently leaked one socket and two goroutines — an unbounded file-descriptor leak on a gateway that makes one upstream call per agent tool invocation — and it silently made MaxConnsPerServer meaningless, since no two calls ever shared a pool. Reproduced: six completed calls left six connections open on the server. The transport's idle connections are now released when the call completes. CheckRedirect also bounded only the hop COUNT. With MaxRedirects raised above zero, an upstream's own response data chose where a credentialed request went next; the pinned dialer bounded the damage, but "bounded two layers down" is not "refused". A redirect whose target host is not the approved server is now refused outright, while a same-host redirect within the operator's budget is still followed. Also makes the node-local policy provider read the holder LIVE instead of capturing *policy.Store. The holder documents its store pointer as stable and the single-source-of-truth invariant rests on that, but invalidateForStartupFailure and resetForTest both replace it — a captured pointer would keep evaluating the old store's snapshot while the admin surface reported no active policy. Nothing reaches that divergence today; the change makes the invariant true by construction rather than by coincidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…he 2026-07-28 record protocol.Adapter is documented as the boundary that keeps protocol version out of every downstream stage (MCP-PROTO-011). It was declared, unit-tested in isolation, and never invoked by the request path — so the boundary did not exist at runtime, and a future revision with real wire differences had nowhere to land except by threading a version through the whole pipeline. processPost now normalizes the decoded message through the adapter for its wire version, immediately after the strict decode and before any session logic. For the two supported V1 revisions the adapters are the identity, so this is behaviour-preserving. A PRESENT-but-unsupported version is deliberately NOT resolved to the primary: resolveSession owns that rejection, and normalizing first would launder an unsupported revision into a supported one — the best-effort downgrade MCP-PROTO-010 forbids. Also corrects a stale factual claim: version.go described 2026-07-28 as "a non-final RC". That was the May 2026 release candidate; 2026-07-28 was released as the FINAL MCP specification on that date. The allowlist is unchanged — rejecting it is now a dated, deliberate decision rather than an out-of-date one — and docs/design/mcp/PROTOCOL-MIGRATION-2026-07-28.md records what adopting it costs: a stateless core that removes the substrate for session-identity binding, lifecycle admission and the session cap, plus header-level routing that is a confusion surface rather than a shortcut. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Self-review of the SEC-MCP-05 fix: acquireAuthSlot blocked on an unbuffered send with no context, so a request could be parked on the semaphore indefinitely — reintroducing the exact unbounded stage the RequestDeadline work exists to bound, and letting one stalled slot holder (a hung introspector) park every waiter for the process lifetime. The comment claiming the wait was "bounded by construction" was wrong: RequestDeadline was not enforced inside the wait. acquireSlot now selects on the request context and refuses a queued request whose budget elapses, with the same request_deadline_exceeded classification the stage-boundary checks produce. authenticate takes the context to do it. The regression test uses a budget that is LIVE on entry and expires while queued, so it exercises the wait rather than the stage-boundary check; verified by mutation against the ctx-ignoring form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
The PR-8 credential gate builds the durable CREDENTIAL_SELECT event that must commit before the broker may touch a provider or cache — the record of who caused a credential materialization. It put the PLAN id in Identity.PrincipalID and asserted PrincipalType "workload", a type nothing had determined. The archive therefore named a plan as a principal and invented a subject type for it, on the one event class whose whole purpose is attribution. CredentialPlan now carries the authenticated subject's stable id and type (non-secret identifiers, taken from the resolved identity already passed to Plan), and the gate uses them, adding the one-way token digest as the session correlator. PrincipalID is required by the event model, so a plan with no resolved principal falls back to the plan id rather than failing an otherwise valid gate commit closed for an evidence-shape reason — but the TYPE is never invented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…ernance sync Adds the dated review artifact for the 2026-08-24 MCP backend security review: baseline SHA, the shipped reachability matrix proven from production composition (Gateway/Observe only — Management, guarded execution, the credential broker, the upstream client and inspection are composed NOWHERE, and internal/mcp/execution has zero importers in the tree), the fifteen-finding ledger, per-finding detail, the areas reviewed and found sound, the verification record including which gates were unavailable, and the execution-readiness verdict. Closes the MCP half of the 2026-08-18 dashboard drift note: the Risk and Debt Registers were MCP-blind for a ~55k-line accepted architecture program. Registers RISK-026 (no per-source admission — one client can monopolize a capability's worker pool pre-auth; a blocker for exposure beyond a controlled host), RISK-027 (a degraded MCP listener is invisible to fleet monitoring), RISK-028 (pinned to a superseded protocol generation), DEBT-011 (no anti-drift wall — six of fifteen findings were controls that are designed, documented, validated and never invoked), DEBT-012 and DEBT-013. No maturity score was raised. Removing defects is not by itself evidence of a higher maturity band, and the §4 specialized re-validation pass has still not run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
TestInventory_SeededRequestReachesAuth pinned the MCP-06 oracle as intended behaviour: a CREDENTIAL-LESS probe was asserted to get 404 for an unseeded server and 401 for a seeded one — precisely the unauthenticated enumeration the ordering fix closes. It failed, correctly, once both became 401. Strengthened rather than relaxed. It now asserts the two responses are IDENTICAL (the anti-oracle property), and the registry fail-closed behaviour it originally cared about is re-asserted one layer in, where it is still observable: with a syntactically well-formed but invalid credential, an unseeded server is still 404 and a seeded one still reaches auth and gets 401. Flagged for the MCP owners in the review artifact: the oracle was a pinned contract, not an oversight, and if the disclosure was a deliberate accepted risk that decision is recorded nowhere in docs/design/mcp/. Also records a verification mistake in the review's own method section: an earlier full-tree run was read as green through `go test ./... | grep -v ^ok | head -20`, which reports grep's exit code and truncated the output before the FAIL line. The shuffled determinism run surfaced it. Every gate result was re-established by capturing go test's own exit code. Also documents precisely which shape reaches the un-normalized fall-through in normalizeForVersion (an initialize whose header names an unsupported revision while its body requests a supported one), so a future V2 adapter must handle it rather than inherit it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…be taken first Two defects introduced by the previous round's SEC-MCP-05 concurrency fix, found by adversarially re-reviewing it. OVN-01 — the DPoP slot was gated on `req.HasDPoP`, i.e. on the presence of an ATTACKER-SUPPLIED HEADER, not on whether a proof would be verified. Under a BearerControlled or MTLSRequired profile the proof is never verified at all, and under DPoPRequired with no proof presented verifyDPoP errors before any cryptography — so a caller could drain a scarce security bound for work that never runs. A bound consumable without doing the work it bounds is an amplifier, not a control. `dpopVerificationRuns` now mirrors authn.verifySenderConstraint exactly. OVN-02 — the auth slot was acquired first and held while WAITING for the DPoP slot. That is hold-and-wait: DPoP waiters occupied the auth pool while doing no authentication work, so once DPoPConcurrency saturated they starved every caller needing only the auth bound. With the two combined, an attacker under a bearer profile could park requests in the DPoP queue holding auth slots and deny all legitimate traffic, at zero cryptographic cost. The bounds are now acquired scarcest-first, so a caller queued for DPoP holds nothing and the number of DPoP callers inside the auth pool is bounded by DPoPConcurrency. Both mutation-verified independently: reverting either guard fails its own test and only its own test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
OVN-03, self-inflicted by the previous round. Making the PolicyProvider read the holder live (correct, and it closed a real pointer-stability hole) routed a process-wide RWMutex read onto the request path: the provider dereferences the Gateway store on EVERY decision-point request, and RLock is an atomic read-modify-write on one shared word — a throughput ceiling, not a constant cost. That is precisely the shape Culvert has repeatedly removed elsewhere (internal/threatfeed, the IP filter, internal/connlimit all publish an immutable view through an atomic pointer for exactly this reason), so re-introducing it for MCP was a regression against a standing architectural rule. The two capability stores are now published through atomic.Pointer. Writers still hold mu — they are startup/admin-rate — and publish the replacement before releasing it, so a reader observing a new store necessarily observes a fully constructed one. Freshness is unchanged and pinned by its own test. The gate is structural rather than timing-based: it holds the holder's write lock and requires the provider to answer anyway, so it is deterministic on any hardware, under -race, at any load. Mutation-verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
OVN-04. The approved server's identity is host AND port, but the redirect guard compared only the hostname — and destination.Canonical keeps the port in a separate field, so `Canonical.Host` never carries one. A redirect to a different port on the same name therefore passed the guard, and because the dialer is pinned to the original port that request would silently reach a DIFFERENT endpoint than the one it named: the request line and the connection disagree. The comparison is now like-for-like — an implicit port is normalized to the scheme default, so a redirect written without a port is not refused merely for being written that way. Mutation-verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…tinct from assurance OVN-05. `Assurance` is documented everywhere — identity/principal.go, policy/enums.go, AUTH-AND-CREDENTIAL-MODEL.md §3, SECURITY-REQUIREMENTS.md MCP-ID-006 — as NIST-AAL-style HUMAN authentication strength: "multi-factor", "hardware-backed / phishing-resistant", elevated by step-up authentication. Culvert cannot observe that property. authn/claims.go parses no `amr` and no `acr`, and no other AAL source exists in the product. What the runtime derives is the strength of the VERIFIED SENDER BINDING, which is a different thing: DPoP proves the presenter controls the token's key (RFC 9449), mTLS proves possession of a certificate (RFC 8705) — neither shows a human completed MFA. A password-only session at a DPoP-issuing IdP was reported as "hardware-backed / phishing-resistant". The conflation predates this review: before the 2026-08-24 P0 fix the runtime did the same mapping from unverified header presence. That fix closed the hole and made the mapping verified; it did not create the conflation. It became load-bearing because until now there was NO policy field for the sender constraint at all, so `principal.assurance >= high` was the only way to express "require a sender-constrained token" — a real and common requirement. This commit implements only the unambiguously additive half, changing no existing decision: - `principal.sender_binding` / `session.sender_binding` (none|dpop|mtls) and `principal.sender_bound` (bool) join the closed policy vocabulary. The zero value is the UNBOUND one, so an input that never sets it fails a binding requirement closed. - The runtime populates them from the verified constraint, never from a header. - The durable event records `sender_binding` alongside `assurance`, so the archive no longer depends on the conflation to be readable. - An anti-escalation test fails if authn starts parsing `amr`/`acr` — the change that would silently turn a sender-binding fact into an AAL claim. - Both Assurance enum doc comments now state what the value actually is. No value, mapping or decision changed. Redefining `assurance` itself is NOT done here: nothing supplies AAL, so it would make every request Unknown and trip the engine's write-or-higher hard deny, failing every write operation in every deployment. That is a product migration decision — docs/design/mcp/OPEN-DECISION-assurance-model.md, recommended Option 1. The write side is proven with a REAL verified DPoP proof through a compiled rule. A bearer-only test would have been vacuous: SenderBindingNone is the zero value, so deleting the assignment leaves such a test green — verified, then closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Records OVN-05 as a human product/security decision with proof, provenance, reachable impact, three options and a recommendation (Option 1: split, deprecate, migrate). Keeps the risk open rather than faking closure: making `assurance` mean AAL today would hard-deny every write operation in every deployment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
OVN-06. The runtime validated every request's token TWICE: once via
validateClaims to derive the asserted principals, then again inside
authn.Authenticate, which re-validates the same credential. Measured on the live
pipeline with a counting key resolver: 2 full ECDSA P-256 verifications per
request, ~206 µs / 24.3 KB / 538 allocs, of which the redundant verification was
~96 µs / 8.7 KB / 206 allocs — 47% of the entire authenticated request path.
That is a 2x amplification of the most expensive attacker-reachable operation, on
the stage an unauthenticated flood reaches first, and it compounds directly with
RISK-026 (no per-source admission).
authn gains an explicit two-phase API:
ValidateCredential(cred, cfg, deps, now) -> *VerifiedCredential
AuthenticateVerified(v, req, cfg, deps, now) -> *ResolvedContext
Authenticate is now DEFINED as the composition of the two, so there is exactly
one code path and the split cannot drift from the combined API — that is the
equivalence argument, and the existing authn suite passes unchanged.
Nothing is weakened. AuthenticateVerified runs every non-cryptographic check the
combined API runs, and adds four fail-closed guards at the one place the
cryptographic check is skipped:
1. a nil verification is refused — there is no "unverified is fine" branch;
2. the presented credential must be byte-identical to the verified one, so a
caller cannot validate token A and authenticate token B;
3. the config identity must match, so a token verified for Gateway can never be
redeemed under Management — the identity is content-addressed over every
acceptance-relevant field, not just the capability;
4. the time-based claims are re-checked against the redeeming caller's clock, so
a verification cannot outlive its token. Free, and it removes the staleness
class entirely.
VerifiedCredential is unforgeable by ordinary callers: every field unexported,
ValidateCredential the only constructor.
Measured after: 1 verification, 113.6 µs (-45%), 15.7 KB (-35%), 332 allocs
(-38%). Pinned by a permanent gate — not a benchmark — that fails deterministically
if a second verification returns, including on the DPoP path and the rejected-token
path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…ication) OVN-07. MaxConns bounds CONNECTIONS, not requests. ServeTLS auto-enables HTTP/2, and one accepted socket multiplexes hundreds of concurrent streams into an admission path sized for MaxConcurrent workers — so a single connection could fill the worker pool AND push its surplus into the SHARED queue, consuming capacity other connections depend on, while occupying exactly one of the MaxConns slots that were supposed to bound it. Measured against the real listener over a real TLS/h2 connection: with the budget removed, ONE connection put 36 requests into a shared queue behind a 4-worker pool. With it, zero. The bound is the worker-pool size, and that is not a fairness policy: beyond MaxConcurrent, additional concurrent requests on one connection cannot make progress anyway, so this admits no work that could have proceeded. It is taken BEFORE the shared queue, so surplus streams cannot occupy queue slots either, and it is context-bounded so a saturated connection cannot park a stream past its deadline. This is topology-independent — no source identity, no trusted-proxy contract — and so is NOT a fix for RISK-026, which stays open. It removes the amplification, not the per-source unfairness. Two dead ends are recorded in the code so they are not retried: - http.Server.HTTP2.MaxConcurrentStreams is silently IGNORED on the ServeTLS auto-h2 path in this toolchain. Verified empirically: setting it to 8 still admitted 200 concurrent streams. Shipping it would have been a control that only looks configured — exactly the DEBT-011 pattern, and my first attempt at this fix was precisely that until the end-to-end test refused to pass. - Measuring client-side outstanding requests proves nothing here, because a waiting request is still outstanding. The gate measures SERVER-side shared-queue occupancy, and holds requests inside authentication so the pressure is real — a fast-rejecting fixture never queues and passed with the guard removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…d bound DEBT-011. Six of the fifteen findings in the 2026-08-24 review were the same shape: a control designed, documented, validated at construction, unit-tested in isolation — and never invoked by the request path. Package tests pass either way, so nothing caught it. This is the wall for the Limits surface. Every bound must now DECLARE its enforcement owner, and the declaration is checked against the real syntax tree: - every Limits accessor has exactly one ownership row, and every row names a real accessor — a knob cannot be added without an owner; - an "enforced here" claim must be backed by a real production call site — the half that would have caught AuthConcurrency, DPoPConcurrency and RequestDeadline before they shipped as documentation; - a "reserved" claim must be honest: a bound recorded as unenforced must not be silently read, so the register cannot understate what the system does; - a "delegated" bound must name a concrete lower-level owner and must NOT also be read here — one invariant with two enforcement points will drift. It is TYPE-AWARE rather than grep-based, and that mattered immediately: the wall caught a flaw in ITSELF on its first run. `MaxSessions()` exists on both runtime.Limits and the protocol kernel's limits.Limits, and the runtime legitimately reads the kernel one — a name-based check reported that as the runtime bound being enforced here, which is precisely the false confidence the file exists to prevent. Resolves Priority 5, the five bounds the previous review left unresolved: MaxSessions -> delegated to session.Manager MaxOutstanding -> delegated to session/ops.go (per session+direction) HandshakeTimeout -> delegated to net/http (max of the read timeouts) MaxResponseBytes -> delegated to upstreamclient (the attacker-sized leg) MaxObservations -> reserved: the sink is synchronous, so MaxConcurrent bounds it CleanupPerOp -> reserved: the sweep is bounded by the session cap AdmissionBudget -> reserved: RISK-026, no per-source admission exists Each status is now recorded on the config field itself as well as in the wall, so the knob no longer creates operator confidence it has not earned. Mutation-verified three ways: removing a production read of RequestDeadline, secretly reading AdmissionBudget, and adding an undeclared accessor each fail the corresponding gate and only that gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Investigates the four candidate keyings against real topologies rather than picking one. Option A (per TCP peer) is nearly implied by the shipped defaults — client_cert_mode:require cannot sit behind a TLS-terminating L7 proxy, so the peer IS the client in the default posture — but it silently throttles a legitimate NAT'd estate, which converts an availability control into an availability outage. Option B cannot bound pre-auth exhaustion by construction. Option D is rejected as the sole answer because Culvert's own SWG does not take that position: it ships connlimit and authstate precisely because it does not assume a limiter in front. Recommends Option C (two-tier) with an explicit `network_position` declaration and a validation rule making `behind_l7_proxy` incompatible with mTLS-required, plus a full implementation plan reusing internal/authstate's fair-share eviction verbatim rather than inventing a trust model. Records what was already fixed and — importantly — what it does NOT fix: the OVN-07 per-connection budget bounds one connection, not one source; an attacker simply opens more. RISK-026 stays OPEN. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Every other Culvert subsystem with a failure mode reports through the same three
surfaces — a /healthz field, a /readyz row and culvert_* metrics
(storage_health.go, ca_health.go, socks5_health.go, cluster_ca_health.go,
auth_backend_health.go). MCP had none of them: its health existed only under
admin-authenticated /api/mcp/*, so a degraded or dead MCP listener was invisible
to the monitoring that watches everything else in the binary, and there was no
series to alert on.
Three postures are kept strictly distinct, and that separation is the point:
- PROCESS health — MCP never affects it.
- SWG READINESS — MCP is REPORT-ONLY, always. MCP is disabled by default,
optional when enabled, and shares no state with the SWG data path. Making it
gating would turn an optional capability into an availability SPOF for the
primary product. Operators who do want dependency-degraded nodes ejected use
the existing /ready?strict=1 mechanism (CHAOS-09); MCP invents no second one.
- MCP CAPABILITY health — the new row, field, series and alert.
All four read ONE snapshot, so they cannot disagree.
Disclosure discipline: /readyz and /metrics are unauthenticated on the proxy port,
so the row detail is a FIXED string from a closed set (the activation reason can
name a certificate or policy path and stays on the admin plane), every metric
value is a plain total or 0/1 gauge, and nothing is a label. Rows and series are
absent entirely on a node that never requested MCP — `up 0` there is
indistinguishable from a dead listener, and the paging rule is `== 0`.
Adds the `mcp_gateway_down` alert, fired once per episode and re-armed only on
OBSERVED recovery, never on elapsed time. Its Detail is a bounded state label:
Dispatch dedups on event+detail, so an unbounded detail would defeat
deduplication (the WK-12/RS-5 defect). Draining is deliberately not a fault.
Also fixes a metric-name collision found while testing: the new telemetry gauge
would have shadowed the pre-existing culvert_mcp_telemetry_ready{capability=...}
series, giving one metric name two label shapes — invalid exposition. Renamed to
culvert_mcp_telemetry_composed, with a gate that fails if any culvert_mcp_* name
is ever rendered with two different label shapes.
Mutation-verified: making the row gate readiness, leaking the activation reason
into the unauthenticated detail, and alerting per evaluation each fail their own
test and only that test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…tials OVN-08. The previous round closed the oracle for a caller presenting NO credential. It stayed open for one presenting a syntactically well-formed but INVALID credential — `Authorization: Bearer anything` passes the syntactic pre-check — because step 8 consulted the registry BEFORE the token was validated, answering 404 for an unknown server and proceeding for a registered one. That made the oracle effectively unauthenticated, and it is tenant-blind by construction (no identity exists at step 8), so it disclosed the registered inventory of EVERY tenant to anyone who could reach the port. The pre-auth lookup was redundant for correctness. identity.Resolve performs the IDENTICAL existence + Usable() check with the same ReasonRegistryServerUnavailable, after the token is cryptographically validated (identity/context.go, resolveCapabilityRefs). The pre-auth check was a fail-fast, never the enforcement point — so an unregistered or disabled server is still refused (MCP-SERVER-002/003), just at the stage where the caller has proved who they are. The path parse stays: it is purely syntactic and consults no registry, so a foreign path still 404s without disclosing anything. statusForAuth maps the post-auth registry rejection to 404, so an AUTHENTICATED caller still gets the accurate answer — that is not an oracle, because a caller without a valid token never reaches it. Also stops an unverified, caller-supplied server id from reaching the sanitized observation record: it is an unbounded attacker-chosen string until Resolve confirms it is registered, and it is now stamped only after that. Two vacuous tests were found and fixed while mutation-testing this: the record-stamping test passed against the obvious mutation because both variants stamp after authentication, so the meaningful mutation (stamping in resolveServer) was used instead. The root end-to-end assertion from the previous round asserted the invalid-credential oracle as intended behaviour and now asserts its absence; the fail-closed half it also covered moved to a pipeline test that can mint a valid token, which the end-to-end harness cannot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…l rug-pull) OVN-09, from the execution-plane red team. The policy decision is computed against a catalog SNAPSHOT. Between the decision and the irreversible upstream call there is a real window — semantic inspection, the durable event commit, credential planning, a provider fetch that may do network I/O — during which a concurrent discovery (execution.Discovery -> catalog.Ingest publishes a new snapshot) can change the tool the decision was made about. Nothing re-validated it. The executor consumed the decision's Tool.FingerprintHash and never compared it to the live catalog, so a tool rug-pull landing inside that window would be EXECUTED under a decision made about a DIFFERENT tool — and the durable event would record the stale fingerprint, producing evidence naming a tool that was not the one called. That is precisely the drift MCP-TOOL-001 / MCP-T-011 / MCP-T-016 exist to prevent, and it is reachable the moment execution is armed. refuseOnToolDrift re-resolves the decision's tool against the LIVE catalog before the executor is reached, so no credential is planned, no event is committed and no upstream request is issued under a stale decision. It fails closed in both directions: a fingerprint that moved, and a tool that has disappeared entirely. A decision with no tool is unaffected — nothing can have drifted. Adds mcperr.ReasonDecisionSnapshotStale, appended at the END of the enum so no existing ordinal moves. The regression test drives the REAL path rather than calling the guard directly: the pipeline consults the inspection provider after buildPolicyInput has read the catalog and before dispatchExecute, so a provider hook there is exactly where a concurrent discovery lands — a deterministic window, no sleeps, no races. It has a control arm proving the executor IS reached with a stable catalog, so a zero-reach result cannot mean the fixture is inert. That mattered: the first version of these tests exercised refuseOnToolDrift directly and passed with the guard unwired from dispatchExecute — the same "declared but never invoked" pattern this run keeps finding, reproduced in my own fix. The window-driven test catches it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…_ONCE The allowance store is bounded at 65536 entries and refuses a new grant at capacity (fail closed, correct). But an ALLOW_FOR_SESSION grant was only ever discarded when the SAME key was looked up again, so a grant whose session timed out and was never revisited held its slot forever. Enough of them and the store refuses every new allowance-gated operation, with no recovery short of a restart: a memory bound turned into a standing denial of the control it protects. Sweep expired session grants at capacity, before refusing. This is provably behaviour-neutral reclamation — consume() already treats an expired grant as absent — and it runs only at capacity, so the ordinary path stays O(1). ALLOW_ONCE records are deliberately NOT swept, and the asymmetry is the point. allowanceKey is built from identity.ResolvedContext.Fingerprint, which despite the `Session` field name is a stable hash over capability, tenant, subject, client, agent, resource, server and tool. It carries no time, session id or nonce, so the same principal invoking the same tool yields the same key for the life of the deployment. Expiring an ALLOW_ONCE record would therefore not be garbage collection but a replay window — single use redefined as once per retention period. The first version of this change made exactly that mistake; TestAllowance_OnceGrantsNeverBecomeReplayable now fails the build if it returns. Each guard is mutation-verified: removing the sweep fails the exhaustion test, sweeping live grants fails the preservation test, and extending the sweep to once-records fails the replay test — each killing only its own test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
The SSRF comment in roundTrip described Resolve/VerifyPeer as "honoring the policy's AllowPrivate for approved internal servers", which reads as a per-server approval. It is not one. Config carries a single destination.Policy and Target carries no override, so AllowPrivate is client-wide: enabling it to accommodate one internal MCP server disables the private/loopback/metadata rejection for every registered server that client calls, and a public server whose DNS answer is hostile or compromised could then reach link-local metadata. This is the trap a future implementer walks into precisely when wiring an internal server, because that is when they read this comment. PolicyConfig already documents AllowPrivate as TEST- or ENVIRONMENT-scoped and it has no production caller today; the comment now says so, and says that per-target private access is an absent design rather than a flag flip. Documentation accuracy only — no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Guarded execution stays off today because of three ABSENCES: nothing calls markGatewayExecDepsReady/markManagementExecDepsReady, so execDepsConfigured is false and every Shadow/Canary/Production transition fails closed at the commit gate; nothing assigns runtime.Deps.Executor, so the pipeline composes no executor; and nothing outside internal/mcp/execution imports that package. An absence is the one property no unit test observes — every package test passes just as well after the wiring is added. mcp_rollout_execdeps.go says the hooks are "intentionally UNCALLED in the current build", which is a comment, not a check. This wall makes each of the three an executable fact. It does not forbid ever shipping execution; it makes arming it a decision a reviewer sees, because whoever arms it must edit this file. AST-based rather than grep-based for the DEBT-011 reason: a string search matches comments and test files, which is how a documentation-only control passes for a real one. The walk asserts it found a plausible number of files, so a scan that matched nothing fails loudly instead of passing vacuously. Mutation-verified: calling an arming hook, assigning Deps.Executor, and importing the execution package each fail their own gate and only that gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
computeOverall decided PRESENCE from the canonical expectedRequiredIDs list but FAILURE from the per-criterion Required flag — a hand-written argument duplicated across ~44 call sites. A criterion recorded with the right ID, a FAIL status and a mistyped Required:false therefore satisfied the presence check and was skipped by the failure check. Proven before fixing: a run whose startup.ready criterion FAILS reports overall PASS with nothing listed as missing. This harness is the gate that decides whether an Observe deployment is acceptable, so a false PASS is the one outcome it must never produce. Requiredness now comes from membership in the canonical set. The flag is still honoured on top of it, so a criterion outside the set can still declare itself required and this only ever adds failures, never removes one. Requiredness drift — the canonical set says required, the call site says not — is reported as a defect in its own right rather than silently absorbed: a gate that disagrees with itself about what it requires has not earned the right to issue a PASS. No live call site had the defect (the only Required:false is the genuinely advisory evidence.denial_aggregated), so this closes a latent hole rather than an active one. Mutation-verified in both directions: reverting to flag-only requiredness fails the suppression and drift tests, dropping the drift check alone fails only the drift test, and treating every criterion as required fails the advisory test — so the guard is not a blanket denial. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
…orphan ids
When the mTLS fixture could not be built or started, the harness recorded a single
criterion with the id "tls.mtls" — an id in no canonical set — and returned. The
run still failed, because tls.mtls_accept and tls.mtls_reject were then absent and
absence fails, but it failed as two unexplained MISSING criteria plus one orphan.
An acceptance artifact is read by someone deciding whether a deployment is sound,
and "this did not run" and "this control failed, here is why" are different
findings. Both canonical criteria are now failed with the fixture's own reason.
The wall generalises it: every id the harness can emit must be canonically required
or explicitly declared non-required with a reason, and — the stronger direction —
every canonically required id must actually be emitted by code the wall can see.
Building it surfaced two defects in the wall itself, both found by mutation rather
than review:
- Scanning only runCriterion arguments and CriterionResult literals missed
table-driven emission, which is how the oauth, tenant and protocol scenarios
name their criteria. An orphan substituted into such a table passed. Table rows
are now read too, scoped to functions that actually emit criteria — spec.go has
unrelated tables whose first column is shaped identically (telemetry.node_id,
supervision.admin_user).
- The first attempt scoped that with a flag set on entering an emitting function
and never cleared, so every later table in the same file counted as emitted.
It passed only because the file that would have exposed it happens to contain
no emitting functions. Scoping is now per FuncDecl.
Mutation-verified: an orphan id in the mTLS table, an orphan id in the oauth table,
and a non-required fixture-failure record each fail the wall — the first two only
after the table-scanning fix, which is what proves that fix was load-bearing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
… pin it staticcheck (SA1019) flagged parseECPublic building an ecdsa.PublicKey from raw X/Y coordinates, deprecated in Go 1.26 because a key assembled that way may never have been validated. The validation was in fact present — a crypto/ecdh round-trip performed the on-curve check first — so this was not a live vulnerability. But it was two steps that had to stay in the right order, in the path that parses attacker-supplied key material: a DPoP proof carries its own public key and a JWKS document comes from a configured IdP. ecdsa.ParseUncompressedPublicKey validates and constructs in one call, rejecting both an off-curve point and the point at infinity — the same checks, now impossible to separate by shape rather than by an ordering the next editor has to preserve. The gap worth recording is that NOTHING pinned the rejection: no test fed an off-curve point to ParsePublicJWK, so removing the check would have kept the suite green. Three tests now cover off-curve, point-at-infinity and oversized coordinates, each with a valid key parsed first so a blanket rejection cannot pass for a working control. Mutation-verified: constructing from raw coordinates without validation fails the off-curve and infinity tests; dropping the length check fails the oversize test. Also removes an unused test helper staticcheck flagged (U1000), left behind by an earlier round of this review. staticcheck is now clean across internal/mcp/... and internal/mcpacceptance/.... Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Two counter-semantics defects on the MCP gateway transport path, both found in review of the previous round's ambiguous-header fix. 1. authFailures was charged for every guarded singleton header. The guarded set spans five headers and only two of them carry a credential, so duplicating Origin, Mcp-Session-Id or Mcp-Protocol-Version moved culvert_mcp_auth_failures_total -- the one series an operator reads to answer "are credentials being attacked?". Routine protocol traffic and a credential-stuffing spike became indistinguishable on it. The durable denial record is still written for EVERY classified reason, so header confusion stays visible and the previous fix is not undone; only the COUNTER is split. Every ambiguity is counted on the new culvert_mcp_ambiguous_header_total, and authFailures moves only for Authorization and DPoP. The offending header name reaches the caller solely to make that choice and is still never echoed to the client -- telling a prober which of its duplicated headers was noticed is free reconnaissance. 2. culvert_mcp_requests_total is documented as "requests received" but was incremented only inside pipeline.Process, while three transport-level branches -- connection budget, queue admission, header extraction -- reject and return before the pipeline is entered, each still moving requestsRejected. Under overload or an ambiguous-header flood the rejected counter could therefore exceed the total, so a rejection rate derived from the pair is not a rate. The increment moves to the transport entrypoint; Listener.ServeHTTP is Process's only production caller, so the accepted path is counted exactly as before. Both directions are pinned, including the double-count that keeping the increment in both places would have produced. Each gate was verified by a mutation that compiles and changes behaviour.
…esidual) The OVN-09 fix refuses a decision whose tool has drifted before handing it to the guarded executor. That NARROWS the decision/execution TOCTOU window; it does not close it. After the entry check the executor still commits durable evidence, plans credentials and fetches provider material -- all of which can block -- while a concurrent execution.Discovery -> catalog Ingest publishes a new snapshot. The upstream call would then run under a decision made about a tool that has since been redefined or withdrawn, which is exactly the drift MCP-TOOL-001 / MCP-T-011 / MCP-T-016 exist to prevent. ExecInput gains ToolStillCurrent, a hook the runtime binds to the same live predicate its entry check uses (extracted as pipeline.toolHasDrifted, so the two refusals can never diverge). The executor calls it at the top of callUpstream -- the ONE closure through which both the credential and no-credential branches perform the side effect, so a branch added later inherits the check rather than having to remember it. A refusal there maps to ReasonDecisionSnapshotStale, the same reason the entry check uses, so both read identically to an operator and a drift refusal is never classified as a transport or durability fault. A nil hook is byte-identical to the previous behaviour: a caller with no catalog seam has nothing to compare, and inventing drift there would refuse traffic the gateway is configured to allow. This changes nothing about the shipped posture. Guarded execution remains disabled -- no executor is composed, ToolStillCurrent is never consulted in production, and the execution-posture wall still passes. Gated from both sides: the execution package proves the refusal precedes the upstream call, and a runtime test drives catalog drift from INSIDE Execute and fails if the runtime hands the executor a nil hook -- an unwired seam would otherwise leave every execution-package test passing while the window stayed open in production, which is the defect class this review exists to catch.
The three execution-posture gates are whole-module scans, and each walked and parsed all 658 production files (~6.9 MB) independently, with ParseComments, under -race -- three times the work for no added assurance, since the parse is a pure function of files the test run never writes to. They now share a single parse, and comments are not parsed at all: none of the gates reads one, and the whole reason the wall is AST-based rather than grep-based is that a comment must never be able to satisfy a check. Measured on the root package: 5.12s -> 1.79s of test time (7.14s -> 3.82s for the package). That matters because the root package's -race + coverage run sits at the edge of its 15m CI budget; this gives back what the wall was costing it. The walk's vacuity guard (a scan that finds almost no files fails rather than passing every assertion trivially) runs inside the shared parse and its outcome is replayed to every caller, so a tree that stops being scannable still fails all three gates instead of passing two of them silently. Verified by injecting a real markGatewayExecDepsReady() call into a production file: the wall still names the file and line and fails.
…this PR's Ledger additions for this round: the OVN-09 residual TOCTOU window at the side-effect boundary (OVN-17), the auth-failure counter charged for all five guarded singleton headers (OVN-18), and requestsRejected being able to exceed requestsTotal (OVN-19). New section 9 records two CI findings with their evidence, deliberately NOT fixed inside this PR: CI-01 — the root package's -race + coverage run is at the edge of its 15m per-binary budget. This branch measured 902.4s in CI and 902.1s on the review container (agreeing to 0.3s, so the container is a faithful proxy), and origin/main measured 901.4s on the same container with none of this branch's changes present. It is a budget overrun, not a hang: the panic dump names a test that had just started at 0s and the log carries no data race. This branch's 8.3s of new tests cannot be what tips a package already over budget on main; 3.3s of it is given back by 49b2d3c regardless. The durable fix -- raise the timeout or split the package -- is a CI-ownership decision, and the consequence of leaving it is that a required check is at coin-flip reliability for every PR. CI-02 — TestBenchGate_LearnObserveEnabledBoundedAllocs measures process-global MemStats.Mallocs while the Policy Learning drain goroutine allocates concurrently, and Observe's channel send is itself a scheduling point, so GOMAXPROCS(1) does not exclude the drain from the measured window. Over 300 measurements, failing ones drained a mean of 1193 observations inside the window against 188 for passing ones. It reproduces on unmodified origin/main. Documentation only; no code, no behaviour, no posture change.
staticcheck QF1002. Every arm compared the same variable, so the tagged form is what the code meant; behaviour is identical. Re-verified after the refactor that the counter-split gate still holds: charging authFailures for every guarded header still fails TestHeaderAmbiguity_NonCredentialHeaderIsNotAnAuthFailure.
… exceeds it An earlier reading of this data concluded that origin/main also exceeds the root package's 15m -race budget. That was wrong, and the error was mine: both runs supporting it shared the machine with my own work (one with file edits, one with a full staticcheck/gocritic sweep), so they measured my background load rather than main. A clean unclamped re-measurement puts origin/main at 897.9s -- it passes, with 2.1s of headroom. This branch measures 918.5s on the same quiet box. The corrected finding is narrower and more useful than the one it replaces: a required check clears its budget by 0.23%, which is far below the run-to-run variance of a suite that makes real network calls (urlhaus.abuse.ch, openphish.com, raw.githubusercontent.com all appear in the job log). That is not a check that passes; it is one that is currently winning a coin flip. This branch's contribution is now measured properly too: all 52 test functions in the five root test files it touches cost 4.7s together under -race with coverage, down from 8.3s after 49b2d3c. 4.7s against 2.1s of headroom is enough to tip it -- so this is not somebody else's failure, but it is also not a defect in this PR, because optimising the new tests to zero still leaves a 2.1s margin on a suite whose variance is an order of magnitude larger. Recording the two candidate fixes (raise the per-binary timeout; split the root package) without taking either, since both are CI-ownership decisions rather than changes a security PR should make on its own authority. Documentation only.
The -timeout on these two full-suite runs is PER TEST BINARY, and the root package alone consumes essentially all of it. Measured unclamped on a quiet container: origin/main takes 897.9s against the 900s limit -- 2.1 seconds, or 0.23%, of headroom. Every other package in the module finishes in seconds; the largest, internal/ssrf, takes 37s. 0.23% is far below this suite's own run-to-run variance. It makes real network calls while it runs -- urlhaus.abuse.ch, openphish.com and raw.githubusercontent.com all appear in the job log -- so the required check was not passing so much as winning a coin flip, and any PR adding a few seconds of root-package tests tipped it over. The overrun then presents as "panic: test timed out after 15m0s", which reads like a hang: the dump names whichever test happened to be starting at that instant (0s elapsed), and there is no data race in the log. This does not weaken what is checked. The timeout exists to stop a genuinely hung test, and a hung test still fails -- roughly ten minutes later. Nothing is skipped, quarantined or excluded. It also does not make the underlying problem acceptable. One test binary holding 900s of a 15m budget while 65 other packages finish in seconds is the thing to fix, by splitting the root package. That is separate work; CI-01 in docs/engineering/security-reviews/2026-08-25-mcp-overnight-hardening-run.md records the measurements for whoever picks it up. Both jobs' timeout-minutes are raised to 35 so the job ceiling stays above the 25m test budget -- otherwise the job becomes the real limit and kills the run before Go can print the panic dump identifying which test overran. qa-gate.yml carries the identical command and the identical 15m budget on main pushes. It is not what blocked this PR, but it is the same coin flip, and it is the one this branch's own +4.7s of root-package tests would land on after merge -- so fixing only the PR-side gate would have left a known post-merge failure in place. Called out here rather than folded in silently, in case you want it reverted separately.
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 337d5973e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SEC-MCP-02 made ctx bound the whole request rather than admission alone, but that only holds for stages that OBSERVE ctx -- and the request-body read does not. pipeline.readBody blocks in io.ReadAll on the socket, and the budget is only re-checked after that read returns. So a client that sends a syntactically valid Authorization header and then stalls its POST body passed Host/Origin, method dispatch, path/capability and the credential precheck, then sat in the body read holding a worker slot AND a per-connection budget slot for as long as the socket stayed open -- which is ReadTimeout, not RequestDeadline. LimitConfig.Validate deliberately does not tie the two (it only requires ReadTimeout >= ReadHeaderTimeout), so an operator may legitimately configure ReadTimeout far above RequestDeadline; the defaults are equal, which is what hid this. Enough concurrent slow uploads then exhaust MaxConcurrent despite the end-to-end deadline -- precisely the amplification OVN-07's per-connection budget exists to prevent, reachable pre-authentication. ServeHTTP now pushes the SAME deadline down to the socket via http.NewResponseController(w).SetReadDeadline, so the kernel interrupts the blocked read at the request deadline and both slots are released. The classification is fixed with it. readBody's failure was reported as 413 "resource limit", so a body that stopped arriving was answered as a body that was too large -- telling a caller its upload was over-cap when the gateway had in fact given up waiting, and filing a slow-upload flood under the wrong counter. That is the same mistake as answering 401 for a saturated verification bound, fixed earlier in this branch. A read that fails with the context expired is now ReasonRequestDeadlineExceeded and answers 503, the same overload response checkBudget and the verification-slot bound already use. A genuinely over-cap body is still 413. Any other read error keeps its existing classification: the response goes to a socket that is already gone, and widening the reason set is not what this change is for. SetReadDeadline's error is deliberately non-fatal. A real net/http server supports it on HTTP/1 and HTTP/2 alike; it returns ErrNotSupported only for a synthetic ResponseWriter such as httptest.ResponseRecorder, where there is no socket to bound and ReadTimeout still applies. Failing the request there would break every handler-level test to guard a condition that cannot occur in production, so the real behaviour is pinned over a REAL listener and a REAL socket instead. Gated three ways, each mutation confirmed to compile and change behaviour before its verdict was accepted: removing the deadline leaves the stalled request held for 10s (vs 0.31s against a 300ms deadline); disabling the timeout classification makes the refusal a 413; and inverting the size check proves the over-cap branch is not swallowed by the timeout branch.
The poller added earlier in this branch ran on context.Background(), so it outlived appLifecycleCancel and had no termination signal short of process exit. That is not just untidy, it is wrong: mcpCapStopped is a Faulted() state, and graceful shutdown stops the MCP listener deliberately via the mcp-runtime-stop hook. A shutdown spanning a poll interval therefore ticked after that hook, observed the intentional stop, and could page mcp_gateway_down for a healthy orderly shutdown -- the alert plane crying wolf on exactly the event an operator triggers on purpose, and on the one page that is supposed to mean "this node stopped serving MCP without being asked to". I had looked at this exact question earlier in the review and concluded it was safe because Draining is explicitly exempt from Faulted(). That was checking the wrong state: Draining is the transition, Stopped is where the listener ends up, and Stopped is a fault -- correctly so, because a listener that stopped on its own IS one. The exemption cannot be widened to cover Stopped without blinding the alert; the poller has to stop instead. It now starts with resolveLifecycleCtx(), the repository's existing lifecycle resolver, which falls back to Background before the context is wired so early callers are unaffected. Two gates, both structural because the property is a NEGATIVE -- an alert that must not fire during a shutdown no unit test of the poller can stage. One pins the call site to the lifecycle context (reverting it to context.Background() fails); one pins that the loop actually selects on ctx.Done(), since binding a context the goroutine then ignores would achieve nothing. Deliberately not a goroutine-count assertion: counting live goroutines races with every other test in the package, and a gate that can flake gets muted.
net.DialTimeout is banned by the repository's own convention ("use DialContext()
not DialTimeout()", CLAUDE.md) and by the noctx linter in the blocking gate.
Replaced with a net.Dialer + DialContext under a bounded context.
Caught by CI rather than locally, for the seventh time this branch, because
golangci-lint cannot run here (2.5.0 is built with go1.25 and panics in go/types
on this go1.26 module) and my local sweep only covered staticcheck and gocritic.
noctx was simply not in it.
Fixed properly rather than patched: the branch diff is now also swept with the
gate's other independently-installable linters -- noctx, unconvert, ineffassign,
whitespace, tparallel, misspell, revive and unparam -- all clean, and a grep for
the sibling conventions (http.NewRequest without a context, tls.Dial, bare
Handshake) finds no other occurrence in anything this branch touches.
The gate is unaffected by the change: removing SetReadDeadline still leaves the
stalled request held for 10s against a 300ms RequestDeadline.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff622ad5e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…he boundary The last-moment drift check compared only the fingerprint, and that misses an entire class of revocation. catalog.DisableServer copies each record and changes ONLY Eligibility (to ServerDisabled) and Revision -- the fingerprint is deliberately preserved, because the tool's shape did not change, its server's identity did. So a fingerprint-only check reported "still current" for a tool the catalog had just marked unusable, and the guarded executor would call a server the operator had explicitly disabled. The same holds for a transition into Quarantined or ReviewRequired. Compared through policyDisposition -- the SAME mapping policy.go builds the decision input with -- so this asks exactly the question the decision answered rather than inventing a second, independently-drifting notion of "eligible". Also makes an existing TOCTOU fixture faithful. It hand-built a policy.Tool with Disposition/Drift left at their zero values, and DispUnset is documented in the enum as invalid for a Gateway tool input; the runtime never produces it (policy.go populates both from the live record). Leaving them zero would have made that fixture differ from the catalog on the eligibility axis too, so its refusal would no longer have proved the FINGERPRINT was what refused it. Both axes are now independently gated: neutering either one fails a test. Execution posture unchanged -- no executor is composed and this predicate is never consulted in the shipped build.
The socket read deadline added for the stalled-body case answered 503 but never incremented ctr.timeouts, while checkBudget and acquireSlot both do when the budget elapses. Slow-upload expirations were therefore absent from culvert_mcp_request_timeouts_total -- understating precisely the overload condition that deadline was added to expose, and the series an operator would alert on during such a flood. Counted exactly once: this branch returns, so the post-decode checkBudget never runs for it, and readBody itself deliberately does not count (the reject branch owns the increment). Both halves are pinned, because the sibling defect earlier in this branch was a slot timeout counted TWICE.
acquireSlot selects between "a slot is free" and "the context is done". Go picks UNIFORMLY AT RANDOM when both cases are ready, so under saturation a slot freeing at the instant the budget expires took the slot roughly half the time -- and token/DPoP verification then began on a request whose deadline had already passed. That defeats the end-to-end bound this branch introduced, spends a scarce security bound on work nobody is waiting for, and returns a credential verdict where the truthful answer is a timeout. The slot is now handed straight back and the request refused with the same reason and the same single timeout increment as the ctx.Done() branch. Gated at 200 trials, deliberately: one trial passes a broken build half the time. A second mutation proves the refusal RELEASES the slot -- a refusal that kept it would convert a deadline into permanent capacity loss, strictly worse than the bug being fixed.
…ble from here Deep · determinism (shuffle, count=2) passed on ff622ad (2/2 incl. a re-run) and failed on 5d607af (2/2), always in the ROOT package. The only package 5d607af changed, internal/mcp/runtime, passed in both failing runs, and there is no causal path from that change to a root-package test. The root package would not fail locally across 7 count=2 -shuffle runs (2 at the exact CI seeds, 5 fresh), so it is not an order-dependent bug reproducible without the network. Its tests make live calls to urlhaus.abuse.ch / openphish.com that CI permits and this container's egress policy blocks (403) -- consistent with an environment-dependent live-network flake. secscan flaked once on the same job (in-flight scan leak) and passed on re-run, corroborating pre-existing intermittency. The failing test's NAME could not be obtained: get_job_logs is tail-capped below the root package's --- FAIL line, and the deep-determinism-log artifact host is egress-blocked (403), which the proxy contract says to report, not route around. Recorded honestly: the 5d607af correlation is unexplained but most consistent with a pre-existing environment-sensitive flake landing fail/fail by chance; it is not demonstrably this PR's code, and locating it needs the owner-accessible log artifact. Durable fixes (hermetic threat-feed tests; secscan per-test in-flight reset) are repo-owned. Documentation only.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd7b9a29dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The webhook configuration panel (static/index.html) offers an exact-name checkbox list of subscribable alert events, and saveWebhook submits only the checked values. This branch added the mcp_gateway_down alert (RISK-027 / OVN-16) but not a checkbox for it, so a GUI-managed deployment could subscribe to it only via the catch-all "*" -- an operator ticking specific health alerts would silently never receive the one this PR introduced. That is a GUI-parity gap in code this PR added, and the repo convention (CLAUDE.md) requires every config surface to be reachable from the admin UI. Adds one checkbox alongside the sibling listener/health alerts (socks5_listener_down, storage_write_failed, identity_backend_unreachable). The .wh-event class selector picks it up with no JS change: saveWebhook collects it when checked and loadWebhook ticks it when a stored hook already subscribes. Scoped to exactly the alert this branch introduced. mcp_gateway_down is the only missing event that does not already exist on origin/main; the other alerts absent from this list (dns_failure, scan_timeout, cdr_unavailable, disk_critical, pac_profile_degraded) are pre-existing UI gaps and out of scope here. A contract test pinning the full alert set against the checkbox list would fail on those five, so it is deliberately not added -- that is a separate, repo-owned cleanup. Legacy static UI only; the React frontend/ is untouched. Verified: build embeds cleanly, the div nesting is intact, and the alert/webhook and index.html-reading tests pass.
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fd0869088
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…stale A drift detected inside the broker's materialization callback — the CREDENTIAL path, i.e. an executed tools/call carrying a credential profile — surfaced as a blocked result whose reason was ReasonOf(errToolDriftedBeforeCall) == ReasonNone, because materializeAndCall swallows the callback error into a ReasonNone block and the staleAtCall remap ran only on the no-credential (CommitThenAct error) branch. Clients and block telemetry then read `none` on exactly the ordinary enterprise shape, where the no-credential path reads `decision_snapshot_stale`. runExecute now applies the same staleAtCall reclassification on the didBlock branch. staleAtCall is set only by callUpstream, so on that branch it is true iff the block was the drift refusal; the boolean is robust to any error wrapping the broker might do (unlike errors.Is on a package-private sentinel returned verbatim). Adds a materializing-broker regression test (control proves the harness drives Plan -> gate -> materialize -> scoped callback and executes with the real bearer credential; the drift case pins the stale reason). Verified failing (reason=none) against the pre-fix tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
The pipeline held the DPoP bound across ValidateCredential — the access-token crypto (JWT signature OR opaque introspection). The DPoP proof is a separate verification that runs later, inside AuthenticateVerified, so a slow introspection, or an invalid token that never reaches the proof stage at all, occupied a scarce DPoP slot while no DPoP work was in progress. A bound drainable without doing the work it bounds is an amplifier: with DPoPConcurrency < AuthConcurrency, such requests could stall legitimate DPoP callers into a timeout. authenticate now validates the access token under the auth bound, RELEASES it, and acquires the DPoP bound only afterward — and only when a proof will actually be verified. The two bounds are never held at once, which closes both starvation shapes at their root: no hold-and-wait on the auth pool (auth is released before DPoP is taken, so OVN-02 still holds), and no DPoP slot held across validation. The OVN-02 test is rewritten to exercise the invariant faithfully under the new model (a DPoP waiter parks on DPoP holding no auth slot); a new test pins the Codex invariant (no DPoP slot held during token validation). Each was verified failing against a distinct regression: the pre-fix DPoP-first-across-validation ordering, and a hold-and-wait mutation, respectively. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
F-CDR (credential-path drift reason) and F-DPOP (DPoP slot scoping) on head 7fd0869 — both confirmed by trace, fixed, and mutation-verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fast gate -race failed only in the root package (package main), on live-network threat-feed/SaaS-feed calls; every internal/mcp/* package this PR touches passed. Definitively not this diff — records the per-package evidence and the repo-owned durable fix (hermetic feed tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ozLi9RkAQYiaqoPkixVCo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
סיכום / Summary
A full adversarial review and hardening pass over the MCP Agent Security Gateway backend (
internal/mcp/**,internal/mcpacceptance/**, rootmcp_*.gowiring). Frontend explicitly out of scope and untouched.31 findings across two rounds (MCP-01…MCP-15, OVN-01…OVN-16), each reproduced against the pre-fix tree before any patch was written, and each guard mutation-verified — the fix reverted and the test required to fail.
Guarded execution remains disabled, and is now enforced rather than assumed. Nothing here enables Shadow, Canary or Production; nothing weakens TLS, OAuth, DPoP, mTLS, policy, durable-event or four-eyes requirements; no protocol allowlist was widened and
2026-07-28stays in the rejected set.The two most consequential fixes:
startup.readycriterion fails reported PASS with nothing listed as missing. That harness is what an operator would have trusted to decide whether an Observe deployment is sound.Also closed: a pre-auth server-enumeration oracle; an HTTP/2 admission amplification (one socket occupied the whole worker pool — measured 36 requests behind a 4-worker pool); a decision "rug pull" between policy evaluation and execution; double credential validation per request; and MCP's total invisibility to fleet monitoring (RISK-027).
Two decisions stay deliberately open, with implementation-ready proposals rather than fake closure:
RISK-026— per-source admission. Every keying strategy is right for one deployment topology and wrong for another, and the product does not currently declare its MCP topology. →docs/design/mcp/ADR-PROPOSAL-mcp-admission-fairness.mdOVN-05— theassurancesemantic model. Redefining it as NIST AAL is the correct end state and would hard-deny every write operation in every deployment the day it lands, because nothing suppliesamr/acrand the engine deniesUnknown. →docs/design/mcp/OPEN-DECISION-assurance-model.mdFull ledger, refuted findings, and verification:
docs/engineering/security-reviews/2026-08-25-mcp-overnight-hardening-run.md(and the2026-08-24predecessor).סוג שינוי / Change type:
feat— פיצ'ר חדש / new featurefix— תיקון באג / bug fixsecurity— תיקון אבטחה / security fixrefactor— שיפור קוד ללא שינוי פונקציונלי / refactorchore— תחזוקה, תלויות / maintenance, dependenciesdocs— תיעוד בלבד / documentation onlyבדיקות שבוצעו / Testing Done
All exit codes captured directly from
go test, never inferred from piped or truncated output.Fuzzing — 18 targets × 60s over the untrusted-input surface (jsonrpc decode; authn JWT / claims / introspection; JOSE JWK parse; runtime pipeline, credential parse, transport method; protocol negotiation; policy compile / evaluate / glob; rollout signed-config decode; destination canonicalize / redirect; catalog ingest; DLP scan; schema compile). All exit 0, no corpus entries written.
Mutation sweep — OVN-04, OVN-07, OVN-09 and the duplicate-security-header rejection each fail their own test under a behaviour-changing mutation. Two mutations initially reported "survived"; both were semantically equivalent to the original (requests block inside
acquireConnBudget, so rewriting its result branch changes nothing). Recorded in the review artifact, because acting on a false survival means strengthening a correct test and then concluding a working control does nothing.staticcheck— clean (exit 0) over./internal/mcp/...and./internal/mcpacceptance/....רשימת בדיקות לפני Merge / Pre-Merge Checklist
קוד / Code Quality
go vet ./...ו-go build ./...בהצלחהgolangci-lint runמקומית — לא ניתן בסביבה הזו: the available binary is built with go1.25 and panics on this go1.26 module (package requires newer Go version go1.26). I deliberately did not change the toolchain to make a scanner run.staticcheck(built against go1.26) was run instead and is clean; CI's own lane will run the real gate.TODO-ים שנשכחו, אוlog.Printfdebug — a dead test helper flagged bystaticcheckU1000 was removed;staticcheckis now clean over the MCP scopego.mod/go.sumלא השתנו)אבטחה / Security
ecdsa.GenerateKeyInsecureSkipVerify: trueחדש ללא#nosecמתועד — none added (git diffconfirms no new occurrence)auth*.go,ca.go,proxy.goעברו בדיקה מעמיקה — none of these root files are touched; auth changes are confined tointernal/mcp/authnand are the subject of the review itselfאם נגעת ב-Policy Engine
policy.goengine is not touched.internal/mcp/policygains additive fields only (principal.sender_binding,session.sender_binding,principal.sender_bound); no existing field, value or decision changesSenderBindingzero value is the unbound one, so an input that never sets it fails a binding requirement closed rather than claiming a binding (pinned by test)אם נגעת ב-Frontend (index.html)
frontend/,static/index.html,.ts/.tsx/.cssall unchanged)אם הוספת/שינית endpoint של ה-API
ui_routes_meta.go,api/openapi/openapi.yamlandapi/route-classification.yamlare unchanged. The health work adds fields to existing/healthzand/readyzresponses and newculvert_mcp_*series on/metrics; it registers no route.תיעוד / Documentation
CHANGELOG.md— the repository has no CHANGELOG fileconfig.example.yaml— no new config field, CLI flag or environment variable. The MCP capability's health surfaces derive from existing state, and the alert interval is a constantSECURITY_RELEASE_PROCEDURE.md— the release procedure is unchangedסיכון ו-Rollback / Risk & Rollback
רמת סיכון / Risk level: 🟡 Medium
Medium rather than low because the diff touches authentication, admission and upstream transport — but every change is confined to the MCP capability, which is disabled by default, and the Secure Web Gateway request path is unchanged. Two structural safeguards are worth naming:
/readyz mcp_gatewayrow never gates the default verdict, so an MCP fault cannot pull a healthy proxy out of rotation./ready?strict=1(CHAOS-09) remains the existing opt-in for operators who want dependency-degraded nodes ejected — MCP does not invent a second mechanism.Deps.Executorassignment, no importer ofinternal/mcp/execution).mcp_execution_posture_test.gofails the build the moment any of them stops holding, so arming execution becomes a decision a reviewer sees rather than a side effect of another change.תוכנית Rollback: revert the merge commit. There is no migration, no persisted-format change, no new configuration to unwind, and no dependency change — a revert restores the previous behaviour exactly. Individual fixes are also independently revertible: commits are one-fix-each with the reasoning in the message.
Screenshots
N/A — no UI change.
קישורים / Links
docs/engineering/security-reviews/2026-08-25-mcp-overnight-hardening-run.md— this run's ledger, refuted findings, verification table, and the two mistakes made during the rundocs/engineering/security-reviews/2026-08-24-mcp-backend-full-review.md— the predecessor reviewdocs/design/mcp/ADR-PROPOSAL-mcp-admission-fairness.md— RISK-026, needs an architecture decisiondocs/design/mcp/OPEN-DECISION-assurance-model.md— OVN-05, needs a product decisiondocs/design/mcp/PROTOCOL-MIGRATION-2026-07-28.md— per-control stateless replacements for a future V2Verdict recorded in the artifact: OBSERVE READY — with two open decisions. Not ready for Shadow activation, which is a separate review. This PR does not authorise enabling execution.
Generated by Claude Code