Repository navigation
fix(vmcp): end sessions whose stored credentials backends reject on rebuild - #6
Open
CiraciNicolo wants to merge 1 commit into
Open
CiraciNicolo wants to merge 1 commit into
CiraciNicolo wants to merge 1 commit into
Conversation
…ebuild Background rebuilds (health or capability triggers, and the 5-minute lacking-backend reconcile) reuse the identity captured when the session was created. Once its token expires, or is missing because the session was restored from storage, every backend that needs it rejects the rebuild. The session loses those backends and the reconcile retries them every 5 minutes, one 401 each time, until the idle sweep ends the session 24-36 h after the client's last request. In one deployment this reached hundreds of sessions and most of the traffic one backend received. Record, per build, the backends that rejected the credentials: a 401 from the backend, or no token for its upstream_inject strategy (ErrUpstreamTokenNotFound, and a new ErrCallerTokenEmpty sentinel with the same message as before). The session manager keeps the list from the creation build and carries it through refreshes and restores. After a background rebuild, a backend that rejects the credentials but did not reject them at creation means they went stale. Terminate the session instead of publishing the degraded rebuild, and drop its registrations. The client's next request gets 404 and it starts a new session with its current credentials. If terminating fails, roll back to the previous session as for any other refresh failure. Sessions without a subject, and backends that already rejected the credentials at creation, keep today's behaviour: a backend that never accepts a caller cannot end every new session of that caller. A 403 and failures of the auth strategy itself (a token exchange that cannot be reached) do not count as rejections. Signed-off-by: Nicolò Ciraci <nicolo.ciraci@docplanner.com>
CiraciNicolo
force-pushed
the
fix/vmcp-end-sessions-with-stale-credentials
branch
from
September 24, 2026 12:15
c47f2ba to
33579d1
Compare
CiraciNicolo
marked this pull request as ready for review
September 24, 2026 12:15
MatteoManzoni
approved these changes
Sep 24, 2026
carlos-lopez-vecino
pushed a commit
that referenced
this pull request
Oct 8, 2026
…k#5563) * Wire MCPAuthzConfig references into the MCPServer controller An MCPServer that sets spec.authzConfigRef now resolves and enforces the referenced MCPAuthzConfig at runtime, mirroring the OIDCConfigRef pattern and building on the Stage-1 controllerutil helpers. Backend-agnostic: the proxy runner's authz factory handles cedarv1 and httpv1 alike. - handleAuthzConfig: fetch + validate-ready, track AuthzConfigHash, set the AuthzConfigRefValidated condition; on nil-ref it clears the hash AND removes the condition so a stale "valid" signal does not linger (unlike the OIDC version). Wired into Reconcile after handleOIDCConfig. - mapAuthzConfigToServers watch (extracted as a named method) + Watches on MCPAuthzConfig so a config change re-reconciles referencing servers. - Runtime resolution via AddAuthzConfigRefOptions in the runconfig builder; ConfigMap materialization via EnsureAuthzConfigMapFromRef and a mounted volume via GenerateAuthzVolumeConfigFromRef. Inline spec.authzConfig is untouched and remains mutually exclusive (CRD XValidation). - Add the mcpauthzconfigs get;list;watch RBAC marker. - Unit tests for handleAuthzConfig and an envtest integration suite proving both backends, the watch re-reconcile, ConfigMap materialization, and the invalid case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Guard ref authz volume against duplicate volume name Belt-and-suspenders for review feedback: only add the spec.authzConfigRef volume when the inline authz volume was not added. Inline and ref share the "authz-config" volume name and are mutually exclusive via CRD XValidation, so a hypothetical CEL regression now degrades to "inline wins" rather than producing an invalid pod spec with a duplicate volume name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address review feedback on MCPServer authz wiring - Document fail-stale-on-revocation semantics on handleAuthzConfig: a previously-valid ref that later becomes invalid/missing leaves an already- running workload enforcing its last-applied policy (fail-stale, not fail-open) while the MCPServer is marked Failed/Ready=False. (#2) - Note on GenerateAuthzVolumeConfigFromRef that the mounted ConfigMap is not the active enforcement path in operator deployments (the proxy reads authz from the RunConfig-embedded config); the mount is for inline parity / future file-based consumption. (#3) - Reword the status-write error messages to name the MCPServer (the object being written), not the MCPAuthzConfig. (#4) - Add a transition unit test (valid -> invalid -> valid) covering the needsUpdate bookkeeping, plus an integration It asserting the valid -> invalid flip propagates to the MCPServer via the watch. (#1) - Soften the watch test's "via the watch" phrasing to claim only the observable outcome. (#5) - Tighten the unit not-found/not-valid assertions with ErrorContains. (#6) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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
Type of change
What we saw
initializewith the token captured at creation, which had expired hours before./mcprequests that backend received, all 401. vMCP logs them asFailed to initialise backend for session; continuing without it … unauthorized (401).sessionTTL, since the sweep runs everysessionTTL/2. The reconcile'sGetMultiSessionrefreshes the Redis TTL, not the sweep's activity clock.Change
backend.IsCredentialRejection, matched witherrors.Is):mcptransport.ErrUnauthorized);ErrUpstreamTokenNotFound, and a newErrCallerTokenEmpty, returned byupstream_injectwith the unchanged message.vmcp.IsAuthenticationErroris not used. It matches every strategy failure, throughauthentication failed for backend …, and it misses mcp-go's 403 (request failed with status 403).httptestbackend checks that the sentinels survive the auth round tripper,net/httpand mcp-go.vmcp.backend.rejected_ids: the backends that rejected this build, written by the factory on every build (absent when none did);vmcp.backend.rejected_ids_at_creation: the same list from the creation build.CreateSessionrecords it,RefreshSessioncarries it (never from the rebuild),RestoreSessioncopies it. An absent key reads as empty, so sessions created before this change heal too.refreshSessionCapabilities, for every background rebuild whatever triggered it (the scheduler merges lacking and containing triggers intorefreshAllSessions). If the session has a subject and a backend rejected the rebuild without being on the creation list:Terminatethe session (storage delete, the rebuilt session is closed), drop itsactiveClientSessionsentry and unregister it from the SDK server, then close the previous session after the usual grace period;Terminatefails, roll back as for any other refresh failure; the next refresh tries again.refreshSessionForStaleBackend) are unchanged.runBackendRefreshreportsendednext tofailed. Each ended session logsended session: backends rejected its stored credentialswithbackend_ids.Why the creation list
ShouldAllowAnonymousnor the token hash can tell that it has an owner.Behaviour changes
Before merging
upstream_injectbackend;Verification
go test ./pkg/vmcp/...: the new tests pass. Two failures also happen ondp-stableand are not touched here:TestRestoreHijackPrevention_AuthenticatedRoundTripexpects a different token with the same subject to be rejected; sessions are bound to the subject since e492cbf;-race, a data race inTestDefaultAggregator_QueryAllCapabilities.golangci-lintv2.13.2 on the changed packages: no findings beyond thosedp-stablealready has.%wchain;ShouldAllowAnonymous;Not in this PR
RefreshSessionpassesShouldAllowAnonymous(CreatorIdentity()), which is true for a restored identity (subject, no token);PreventSessionHijackingthen writes an empty token hash;