Audit authentication failures and webhook denials

[JAORMX]

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 returns
• mcp.ParsedRequestHolder (new, pkg/mcp/parser.go): same pattern, filled by ParsingMiddleware, drives audit event typing and target extraction
• Placement: audit moves to directly inside the body-size limit in PopulateMiddlewareConfigs (operator/proxyrunner path) and WithMiddlewareFromFlags (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 it
• 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
• Stream connection events now honor the eventTypes/excludeEventTypes filter like all other events

Now audited: auth 401s (outcome denied, anonymous subject), 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 in docs/middleware.md.

Fixes #5873

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (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

• This PR does not break the v1beta1 API, OR the api-break-allowed label 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, subject anonymous), 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 unconditional success logged on arrival. With includeRequestData, bodies of rejected (including unauthenticated) requests are now captured up to maxDataSize.

Special notes for reviewers

• Event typing for pre-parser rejections: a 401-rejected request never reaches the MCP parser, so its event is typed http_request. An eventTypes allowlist containing only mcp_* types would drop exactly these events — documented in docs/middleware.md.
• auth.WithIdentity contract: 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.
• Rollout: same as Audit authorization denials on the proxy runner path #5872 — the middleware order is serialized into RunConfigs, so operator-managed workloads pick this up on operator upgrade (reconcile regenerates the ConfigMap and rolls pods); CLI workloads on re-create.
• Log volume: unauthenticated scanner traffic now generates audit events by design; operators can exclude http_request via excludeEventTypes if needed.

[codecov]

Codecov Report

❌ Patch coverage is 81.31868% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.37%. Comparing base (1c60f0f) to head (9707a8b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/audit/auditor.go	76.19%	11 Missing and 4 partials ⚠️
pkg/runner/middleware.go	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5874      +/-   ##
==========================================
+ Coverage   71.32%   71.37%   +0.05%     
==========================================
  Files         693      693              
  Lines       70587    70651      +64     
==========================================
+ Hits        50347    50428      +81     
+ Misses      16605    16586      -19     
- Partials     3635     3637       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

/retest

[jhrozek]

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.

[jhrozek]

Now that audit sits outside auth, this io.ReadAll runs before the request is authenticated whenever includeRequestData is on. So an unauthenticated caller can make us buffer a full body (up to the 8MB body-limit) on requests that are just going to 401. It's bounded per-request, so not a big deal, but it's a new cost we didn't pay when audit ran inside auth. Is that an acceptable tradeoff we should just document next to the ordering note in docs/middleware.md, or worth deferring the read until we know the outcome?

[JAORMX]

Right, and it's a conscious tradeoff — auditing rejected requests inherently means touching the body before knowing the outcome. Three bounds keep it tame: it only happens when includeRequestData is explicitly enabled (default off), body-limit sits outside audit and caps the read, and the io.ReadAll itself isn't new (audit always buffered the body to restore it; only the pre-auth timing is new). Documented in docs/middleware.md under the ordering rules. If we ever want to harden further we could skip request-data capture until the identity holder is filled, but that would drop body capture for exactly the denied requests this PR sets out to record.

[jhrozek]

Rate limiting is listed here as one of the now-audited rejections, but determineOutcome in auditor.go buckets 429 into 400-499 → failure (only 401/403 map to denied). So a throttled request audits as failure, not denied. Was that the intent? If a rate-limit rejection should read as denied, it'd need adding to the 401 || 403 case. Either way there's no test pinning the 429 path like there is for 401/403 — worth one to lock in whichever label we pick.

[JAORMX]

You're right that the phrasing overreached — the intent was "rate-limit rejections are now audited", not "audited as denied". 429 stays in the generic 4xx→failure bucket; only 401/403 map to denied, which I'd keep as-is: denied means an identity/policy decision rejected the request, while throttling is a load condition, and remapping 429 would blur that line for consumers that alert on denied. If a distinct outcome for throttling turns out to be useful we can add one deliberately (it extends the event vocabulary, so better as its own change). Clarified the comment to state the mapping explicitly in 9707a8b.

[jhrozek]

This write leans on the "request goroutine only" invariant in the doc comment. Just flagging one spot that could break it: on the vMCP composite path, dag_executor.executeLevel runs steps concurrently, and identityPropagatingRoundTripper.RoundTrip calls WithIdentity(fallback) when the context has no identity. That only lines up if there's no incoming auth middleware (all three auth types set an identity, so normally the fallback branch is skipped), so I don't think it's reachable in a normal vMCP deploy — but it's narrow enough to be worth an atomic.Pointer on the holder as cheap insurance. Not blocking.

[JAORMX]

Good instinct to check that path. Traced it: identityPropagatingRoundTripper.RoundTrip writes only when IdentityFromContext is absent (the #5323 guard, client.go:324). A context that carries the holder always carries the identity by the time any backend call can run — audit injects the holder, auth attaches the identity immediately after, and every middleware/handler that can reach a RoundTrip runs after auth (a 401 means nothing downstream runs). The identity-less contexts the fallback exists for (mcp-go's Close() DELETE built from context.Background()) are exactly the holder-less ones, so the concurrent DAG steps never trigger a holder write — their request contexts have the identity and skip the branch entirely. The invariant is documented on WithIdentity so a future writer that breaks the pattern has a signpost.

[JAORMX]

/retest

[rdimitrov]

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.