Audit authentication failures and webhook denials - #5874
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5874 +/- ##
==========================================
+ Coverage 72.08% 72.13% +0.05%
==========================================
Files 719 720 +1
Lines 74487 74744 +257
==========================================
+ Hits 53691 53914 +223
+ Misses 16964 16960 -4
- Partials 3832 3870 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
A few questions from reviewing the middleware repositioning — all non-blocking, mostly checking whether some behaviors are intended. The ordering change itself looks correct on all three chains.
github-actions
Bot
added
size/L
|
/retest |
github-actions
Bot
added
size/L
rdimitrov
left a comment
rdimitrov
left a comment
There was a problem hiding this comment.
Reviewed end-to-end — solid, well-tested change. The holder-carrier pattern is sound, the ordering invariants are pinned by tests on both builder paths, and the docs/diagrams are thorough. Two minor, non-blocking observations, both in the new deferred stream-open logging, plus one scarier concern I chased down and cleared.
Minor 1 — stream-open event lost on a panic before the first write (vMCP path)
pkg/audit/auditor.go (the stream-open branch in Middleware)
Pre-PR the SSE/stream-open event was logged on arrival (logSSEConnectionEvent(r) before next.ServeHTTP), so it survived a downstream panic. Now it's logged on the first write, or via the trailing sw.logOnce(http.StatusOK) after ServeHTTP returns.
On the vMCP path recovery is the outermost wrapper (pkg/vmcp/server/server.go:664, outside audit), so a panic in an inner layer before any byte is written unwinds straight past audit — recovery returns 500 and no stream-open event is recorded. Niche (needs a panic before first write on a GET stream), but a behavior change vs. the old arrival-time logging, and it undercuts the "every request that passes the size cap is audited" guarantee for that case. A defer around the log would restore it.
The runner/proxy path is unaffected: recovery is innermost there (pkg/runner/middleware.go:264-269), so a handler panic is caught, the 500 flows back out through streamOpenWriter, and logOnce(500) fires.
Minor 2 — streamOpenWriter.Flush() bypasses logOnce
pkg/audit/auditor.go (streamOpenWriter.Flush)
A handler that flushes headers to establish the stream and then blocks waiting for events sends the implicit 200 to the client through Flush() without ever hitting WriteHeader/Write, so the connection event isn't logged until a later data write or connection close. ToolHive's own SSE proxies write before flushing, so logOnce fires promptly there — this is latent, only affecting flush-before-write handlers. Calling logOnce(http.StatusOK) from Flush() closes the gap and pairs with the fix above.
Cleared — no wrong-subject / data race from the WithIdentity side effect
I chased the concern that the new auth.WithIdentity holder-write could report a wrong principal or race under the concurrent backend fan-out (default_aggregator.go, up to 10 goroutines deriving from the request context that now carries the holder). It's safe: the fallback roundtrippers (pkg/vmcp/client/client.go:325, pkg/vmcp/session/internal/backend/mcp_session.go:92) capture fallbackIdentity, _ := auth.IdentityFromContext(ctx) and inject it only when !hasIdentity. Since auth is configured server-wide, those conditions are mutually exclusive on any holder-carrying request context (identity present ⇒ fallback skipped; auth off ⇒ fallback nil). The fallback only fires on mcp-go's Close() DELETE built from context.Background(), which carries no holder. The admission-seam WithIdentity calls re-attach the same authenticated identity. No overwrite, no race.
Nice work — nothing blocking here.
The audit middleware ran inside auth, the webhooks, and rate limiting, so their rejections never produced audit events: authentication failures (401), webhook policy denials (403), and rate-limit rejections were invisible to the audit trail. The point of audit is to record every request outcome, especially rejections. Audit could not simply move outward because it reads the identity and parsed MCP request from context values that auth and the parser create for downstream handlers only. Extend the mutable-carrier pattern the audit package already uses for BackendInfo: audit injects an auth.IdentityHolder and an mcp.ParsedRequestHolder before calling the inner chain; auth.WithIdentity (the single point every auth middleware goes through) and the MCP parser fill them; audit reads them back after the chain returns. Move audit to directly inside the body-size limit on both runner chain-builder paths, and outside the auth middleware on the vMCP Serve path. Body-limit (413) and origin-validation (403) rejections remain outside audit by design: body-limit must reject oversized bodies before audit buffers request data, and origin validation is a pre-auth DNS-rebind guard. Stream-open events (SSE / streamable GET) are now logged on the first response write instead of on arrival, so they carry the real outcome — including denied stream attempts, previously invisible — and the authenticated identity. Fixes #5873 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017attizo5HSwpLcPoimd6Xi
The comment implied rate-limit rejections were among the "denied" outcomes; they are audited but map to outcome "failure" (429 falls in the generic 4xx bucket; only 401/403 map to "denied"). State the mapping explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017attizo5HSwpLcPoimd6Xi
A rate-limit rejection (429) audits as failure, not denied: denied is reserved for identity/policy decisions (401/403), while throttling is a load condition. Add the missing table case to TestDetermineOutcome so the mapping is pinned like the 401/403 paths. Also document next to the middleware ordering rules that with includeRequestData enabled, audit buffers the request body before authentication, so unauthenticated requests that will 401 pay the bounded buffer cost — an inherent tradeoff of auditing rejections.
9707a8b to
bbb90e2
Compare
github-actions
Bot
added
size/L
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Both follow-ups land cleanly - 429 mapping is pinned in TestDetermineOutcome and the pre-auth buffering tradeoff is documented. rdimitrov's two minors (stream-open event on panic, Flush bypassing logOnce) are fine as fast-follow. LGTM.
Stacked on #5874. Two fast-follows from rdimitrov's review: - Flush() now logs the stream connection event: a handler that establishes the stream by flushing headers and then blocks never hits WriteHeader/Write, so the event was delayed until the first data write or lost entirely. - The stream-open log moved into a defer registered before ServeHTTP, so a panic in an inner handler still produces the event during unwinding. On chains whose recovery middleware runs outside audit (the vMCP Serve path) the event previously vanished.
Stacked on #5874. Two fast-follows from rdimitrov's review: - Flush() now logs the stream connection event: a handler that establishes the stream by flushing headers and then blocks never hits WriteHeader/Write, so the event was delayed until the first data write or lost entirely. - The stream-open log moved into a defer registered before ServeHTTP, so a panic in an inner handler still produces the event during unwinding. On chains whose recovery middleware runs outside audit (the vMCP Serve path) the event previously vanished.
3 participants
Summary
Follow-up to #5872. That fix made authorization denials produce audit events, but the audit middleware still ran inside auth, the webhooks, and rate limiting — so authentication failures (401), webhook policy denials (403), and rate-limit rejections (429) produced no audit events. The point of the audit trail is to record every request outcome, especially rejections.
Audit couldn't simply move outward: it reads the identity and the parsed MCP request from context values that the auth middleware and MCP parser create for downstream handlers only (context flows down, not up). This PR extends the mutable-carrier pattern the audit package already established with
BackendInfo:auth.IdentityHolder(new,pkg/auth/context.go): audit injects an empty holder;auth.WithIdentity— the single point every auth middleware (OIDC, local, anonymous) goes through — fills it; audit reads it back after the inner chain returnsmcp.ParsedRequestHolder(new,pkg/mcp/parser.go): same pattern, filled byParsingMiddleware, drives audit event typing and target extractionPopulateMiddlewareConfigs(operator/proxyrunner path) andWithMiddlewareFromFlags(CLI path), and outside the auth middleware on the vMCP Serve path — so every request that passes the size cap is audited no matter which middleware rejects iteventTypes/excludeEventTypesfilter like all other eventsNow audited: auth 401s (outcome
denied,anonymoussubject), token-exchange/upstream-swap failures (with the real subject — auth filled the holder before they run), webhook policy denials, rate-limit rejections, tool-filter rejections, and authorization denials. Deliberately still outside audit: body-limit 413s (audit must not buffer oversized bodies) and origin-validation 403s (pre-auth DNS-rebind guard) — documented indocs/middleware.md.Fixes #5873
Type of change
Test plan
task test)task test-e2e)task lint-fix)New coverage: holder plumbing and 401/denial semantics (
TestMiddlewareAuditsInnerChainOutcomes), deferred stream-open logging including denied and never-writes cases (TestStreamOpenAuditEvents), full-chain webhook denial (TestWebhookDenialIsAudited), ordering pins on both builder paths (Test*_AuditWrapsChain), and vMCP integration tests for auth-failure auditing and identity-via-holder on denials.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.No CRD changes. Two new exported carrier types (
auth.IdentityHolder,mcp.ParsedRequestHolder) with context accessors.Does this introduce a user-facing change?
Yes. Audit logs now include events for authentication failures (outcome
denied, subjectanonymous), webhook policy denials, rate-limit rejections, and token-exchange failures — previously none of these were audited. Stream-open (SSE) events now carry the real outcome and identity instead of an unconditionalsuccesslogged on arrival. WithincludeRequestData, bodies of rejected (including unauthenticated) requests are now captured up tomaxDataSize.Special notes for reviewers
http_request. AneventTypesallowlist containing onlymcp_*types would drop exactly these events — documented indocs/middleware.md.auth.WithIdentitycontract: the holder publish is last-write-wins and must stay on the request goroutine; both invariants are documented on the function. All current callers (including the vMCP admission seam and the upstream-token refresh path) satisfy them — the refresh-failure 401 is audited with the real subject because the identity was attached before the failure.http_requestviaexcludeEventTypesif needed.