Commit bedd6ba
Bind vMCP sessions to OIDC identity, not raw token bytes (#5378)
* Add pkg/vmcp/session/binding leaf package
The vMCP session-binding format needs to be parsed and produced in two
places: pkg/vmcp/session/types (from ShouldAllowAnonymous) and
pkg/vmcp/session/internal/security (from BindSession / validateCaller).
A private helper in each would drift over time, so this commit
introduces a single-owner leaf package.
Format encodes a bound identity as iss + "\x00" + sub, rejecting empty
halves and stray NULs. Parse mirrors Format strictly — including
rejecting trailing NULs in the sub half so values that did not pass
through Format (e.g. direct writes to Redis) fail loudly. A literal
"unauthenticated" sentinel covers sessions created without an
authenticated identity.
No callers wired yet — those land in the next commits.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Add MetadataKeyIdentityBinding key and tighten ShouldAllowAnonymous
The hijack-prevention decorator needs a stable per-identity key in
session metadata. Add MetadataKeyIdentityBinding alongside the existing
token-hash keys (the legacy keys stay temporarily so already-running
sessions can be invalidated cleanly during the migration; final removal
happens in the operator-side follow-up).
ShouldAllowAnonymous treated every empty-token identity as anonymous,
which lumped LocalUserMiddleware identities into the same equivalence
class even when they carried distinct (iss, sub) claims — letting one
local user reuse another's session ID. Tighten the rule so any identity
with a valid (iss, sub) pair from Claims goes through the bound path,
even when Token is empty. Pull iss and sub from Identity.Claims rather
than Identity.Subject so the introspection path and the JWT path
canonicalize against the same source.
Fail-closed on non-string iss/sub claims: a misbehaving validator that
stores a numeric or array value is treated as bound (not anonymous),
with a WARN logged so the misbehavior surfaces to operators.
The new key is unused until the security and factory layers are wired in
the next commit.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Bind sessions to (iss, sub), not raw token bytes
The hijack-prevention decorator hashed the incoming bearer token at
session creation and rejected any subsequent request whose token hashed
differently. Every legitimate OAuth refresh produces a new access token
with different bytes — same identity, same iss, same sub — so the
decorator misclassified the refresh as a hijack and terminated the
session with Unauthorized: caller identity does not match session owner.
Drop the HMAC plumbing entirely and bind to a stable (iss, sub) tuple
extracted from the OIDC identity's Claims. The binding lives in session
metadata under MetadataKeyIdentityBinding, written exclusively by the
new BindSession constructor (renamed from PreventSessionHijacking).
Validation reads the caller's claims through the same path so the JWT
and introspection code paths canonicalize against the same source. The
session-upgrade defense (anonymous session, caller presents a token)
moves to the unauthenticated-sentinel branch and works exactly as
before.
RestoreSession reconstructs identity from the stored binding rather
than from a separate identity-subject key. Sessions written under the
legacy token-hash schema return the bare transportsession.ErrSessionNotFound
sentinel so the client receives the standard "re-initialise" signal one
forced re-auth at deploy is preferable to a rebind-on-first-use window
that would re-introduce the hijack the check exists to block.
Downstream callers of the removed WithHMACSecret option (cli/serve.go)
and the Phase-2 marker switch (sessionmanager) follow in the next
commits; tests catch up after that.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Use MetadataKeyIdentityBinding as the Phase-2 marker
BindSession writes MetadataKeyIdentityBinding on every successful
session creation (sentinel for anonymous, real binding for
authenticated), so its presence is the new way to distinguish a
fully-initialised Phase-2 session from a Generate()-only placeholder.
Without this swap, the now-unwritten MetadataKeyTokenHash would always
read as absent and Terminate would take the placeholder path on every
session, breaking termination semantics.
The behaviour for legacy sessions still in Redis from before the
migration is intentional and documented in the plan: Terminate writes
the placeholder-terminated marker (Update with MetadataKeyTerminated)
which sits for the TTL; loadSession returns
transportsession.ErrSessionNotFound so the client transparently
re-initialises.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Drop HMAC plumbing from serve.go; document Redis trust boundary
createSessionFactory no longer takes an HMAC secret or a Kubernetes
detection flag. VMCP_SESSION_HMAC_SECRET stays readable from the
environment for one deploy cycle (DEBUG-logged and ignored) so the
operator-side env-var injection can be removed in a follow-up PR
without forcing a coordinated cut-over.
Add a startup WARN when incoming auth is configured as "anonymous":
AnonymousMiddleware populates the same (iss, sub) for every request,
so all callers collide on one identity binding and per-identity
hijack prevention degrades to dev-only behaviour. Surface that to
operators rather than letting them assume the binding still scopes
per user.
Document the trust boundary in docs/arch/13-vmcp-scalability.md: the
new scheme stores plaintext (iss, sub) at rest in Redis/Valkey,
which trades the HMAC's at-rest opacity for refresh correctness.
Operators must layer Redis ACLs or NetworkPolicies if a Redis dump
revealing identity is unacceptable.
Add a TODO on extractBindingID for the forward-looking RFC 7662 case
(probe IdP introspection responses for iss + sub) so it surfaces if
that becomes a top-level incoming-auth type.
The five HMAC-specific serve_test.go cases collapse to one
table-driven test of the new signature; the removed cases targeted
HMAC validation that no longer exists.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>1 parent db82aef commit bedd6ba
27 files changed
Lines changed: 1713 additions & 1405 deletions
File tree
- docs/arch
- pkg/vmcp
- cli
- server
- sessionmanager
- session
- binding
- internal/security
- mocks
- types
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
83 | 83 | | |
84 | 84 | | |
85 | 85 | | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
86 | 110 | | |
87 | 111 | | |
88 | 112 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
252 | 252 | | |
253 | 253 | | |
254 | 254 | | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
255 | 264 | | |
256 | 265 | | |
257 | 266 | | |
| |||
336 | 345 | | |
337 | 346 | | |
338 | 347 | | |
339 | | - | |
340 | | - | |
341 | | - | |
342 | | - | |
343 | | - | |
344 | | - | |
345 | | - | |
346 | | - | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
347 | 351 | | |
| 352 | + | |
348 | 353 | | |
349 | 354 | | |
350 | 355 | | |
| |||
645 | 650 | | |
646 | 651 | | |
647 | 652 | | |
648 | | - | |
649 | | - | |
650 | | - | |
651 | | - | |
652 | | - | |
653 | | - | |
654 | | - | |
655 | | - | |
656 | | - | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
657 | 656 | | |
658 | | - | |
659 | | - | |
660 | 657 | | |
661 | 658 | | |
662 | | - | |
663 | | - | |
664 | | - | |
665 | | - | |
| 659 | + | |
| 660 | + | |
666 | 661 | | |
667 | 662 | | |
668 | 663 | | |
669 | | - | |
670 | | - | |
671 | | - | |
672 | | - | |
673 | | - | |
674 | | - | |
675 | | - | |
676 | | - | |
677 | | - | |
678 | | - | |
679 | | - | |
680 | | - | |
681 | | - | |
682 | | - | |
683 | | - | |
684 | | - | |
685 | | - | |
686 | | - | |
687 | | - | |
688 | | - | |
689 | | - | |
690 | | - | |
691 | | - | |
692 | | - | |
693 | | - | |
694 | | - | |
| 664 | + | |
695 | 665 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| 20 | + | |
20 | 21 | | |
21 | 22 | | |
22 | 23 | | |
| |||
165 | 166 | | |
166 | 167 | | |
167 | 168 | | |
168 | | - | |
| 169 | + | |
169 | 170 | | |
170 | | - | |
171 | | - | |
172 | | - | |
173 | | - | |
174 | | - | |
175 | | - | |
176 | | - | |
177 | | - | |
178 | | - | |
179 | | - | |
180 | | - | |
181 | | - | |
182 | | - | |
183 | | - | |
184 | | - | |
185 | | - | |
186 | | - | |
187 | | - | |
188 | | - | |
189 | | - | |
190 | | - | |
191 | | - | |
192 | | - | |
193 | | - | |
194 | | - | |
195 | | - | |
196 | | - | |
197 | | - | |
198 | | - | |
199 | | - | |
200 | | - | |
201 | | - | |
202 | | - | |
203 | | - | |
204 | | - | |
205 | | - | |
206 | | - | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
207 | 190 | | |
208 | 191 | | |
209 | 192 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
506 | 506 | | |
507 | 507 | | |
508 | 508 | | |
509 | | - | |
510 | | - | |
| 509 | + | |
| 510 | + | |
511 | 511 | | |
512 | 512 | | |
513 | 513 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
51 | | - | |
52 | | - | |
| 51 | + | |
| 52 | + | |
53 | 53 | | |
54 | 54 | | |
55 | 55 | | |
| |||
90 | 90 | | |
91 | 91 | | |
92 | 92 | | |
93 | | - | |
94 | | - | |
| 93 | + | |
| 94 | + | |
95 | 95 | | |
96 | | - | |
97 | | - | |
98 | | - | |
99 | | - | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
100 | 99 | | |
101 | 100 | | |
102 | 101 | | |
| |||
105 | 104 | | |
106 | 105 | | |
107 | 106 | | |
108 | | - | |
| 107 | + | |
109 | 108 | | |
110 | 109 | | |
111 | 110 | | |
| |||
Lines changed: 19 additions & 11 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | | - | |
32 | | - | |
33 | | - | |
34 | 31 | | |
35 | 32 | | |
36 | 33 | | |
| |||
63 | 60 | | |
64 | 61 | | |
65 | 62 | | |
66 | | - | |
67 | | - | |
68 | | - | |
| 63 | + | |
| 64 | + | |
69 | 65 | | |
70 | 66 | | |
71 | 67 | | |
| |||
75 | 71 | | |
76 | 72 | | |
77 | 73 | | |
78 | | - | |
79 | 74 | | |
80 | 75 | | |
81 | 76 | | |
| |||
215 | 210 | | |
216 | 211 | | |
217 | 212 | | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
218 | 216 | | |
219 | | - | |
220 | | - | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
221 | 224 | | |
222 | 225 | | |
223 | | - | |
224 | | - | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
225 | 233 | | |
226 | 234 | | |
227 | 235 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
304 | 304 | | |
305 | 305 | | |
306 | 306 | | |
307 | | - | |
308 | | - | |
309 | | - | |
310 | | - | |
311 | 307 | | |
312 | 308 | | |
313 | 309 | | |
314 | 310 | | |
315 | | - | |
316 | | - | |
317 | | - | |
318 | | - | |
| 311 | + | |
319 | 312 | | |
320 | 313 | | |
321 | 314 | | |
| |||
482 | 475 | | |
483 | 476 | | |
484 | 477 | | |
485 | | - | |
| 478 | + | |
486 | 479 | | |
487 | 480 | | |
488 | 481 | | |
| |||
701 | 694 | | |
702 | 695 | | |
703 | 696 | | |
704 | | - | |
705 | | - | |
706 | | - | |
707 | | - | |
| 697 | + | |
| 698 | + | |
| 699 | + | |
| 700 | + | |
| 701 | + | |
708 | 702 | | |
709 | 703 | | |
710 | 704 | | |
711 | 705 | | |
712 | 706 | | |
713 | | - | |
| 707 | + | |
714 | 708 | | |
715 | 709 | | |
716 | 710 | | |
| |||
0 commit comments