Repository navigation
Add backend-scoped vMCP authorization - #6487
Conversation
98169e2 to
13fa0c6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6487 +/- ##
==========================================
- Coverage 80.07% 80.04% -0.04%
==========================================
Files 838 839 +1
Lines 85299 85318 +19
==========================================
- Hits 68307 68290 -17
- Misses 16987 17023 +36
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amirejaz
left a comment
There was a problem hiding this comment.
Nice work on the trust boundary here — sourcing the backend identity from vmcp.Tool.BackendID on the aggregated capability rather than the advertised name is exactly right, and TestCedarAdmission_BackendScopedPolicy's name-spoof case is the test I most wanted to see. The list/call parity through a single metadata path is also the correct shape.
I'm requesting changes on one point: the Backend entity materialization and the IsAuthorized merge carve-out are both dead code, and the reasoning that justifies them doesn't match how cedar-go evaluates in.
From entityInOne (cedar-go@v1.8.0/internal/eval/evalers.go:935):
if fe, ok := env.Entities.Get(candidate); ok {
if fe.Parents.Contains(parent) { return true } // direct hit — parent entity never fetched
for k := range fe.Parents.All() {
p, ok := env.Entities.Get(k)
if !ok || p.Parents.Len() == 0 || ... { continue } // zero-parent placeholder is SKIPPED
todo = append(todo, k)
}
}- Direct
resource in Backend::"x"resolves onfe.Parents.Contains(parent)against theToolentity alone. TheBackendentity is never looked up. - Transitive
resource in BackendGroup::"prod"needs aBackendentity with parents. The materialized placeholder is built withParents: cedar.NewEntityUIDSet()—Parents.Len() == 0— so the evaluator explicitly skips it. Only theentities_jsonentity ever works, which is whatTestAuthorizeWithJWTClaims_BackendHierarchyPreservedis actually exercising.
I verified this rather than reasoning about it. Checking out this branch, deleting the entity insert at entity.go:207-212 and the carve-out at core.go:530-538 (keeping only resourceParents = append(resourceParents, backendUID)), then running ./pkg/authz/authorizers/... ./pkg/vmcp/core/...:
--- FAIL: TestCreateEntitiesForRequest_BackendParent/MCP_and_Backend_parents
Messages: Backend entity must be materialized for Cedar hierarchy traversal
--- FAIL: TestCreateEntitiesForRequest_BackendParent/Backend_parent_does_not_require_MCP_parent
ok github.com/stacklok/toolhive/pkg/vmcp/core
Every behavioural test passes — including the transitive-hierarchy test and all three TestCedarAdmission_BackendScopedPolicy subtests. The only failures are the two assertions on the materialization itself.
This matters beyond line count: IsAuthorized is the generic Cedar primitive, and the carve-out gives Backend inverted collision precedence versus every other entity type. Dropping both restores the invariant EntityFactory already documents at entity.go:181-185 — request entities attach parent UIDs; parent entities come from entities_json. Net effect: ~30 lines become ~3, and the change stops contradicting the comment 20 lines above it.
Two smaller things, neither blocking:
Deferring resources/prompts is the right call — and for a sharper reason than the description gives. vmcp.Resource and vmcp.Prompt do carry BackendID (types.go:428,466), so the filter side would have been trivial — and wrong, because authorizeResourceRead/authorizePromptGet build stubs from URI/name alone (core_checks.go:95,109) with no BackendID. Filter-side-only plumbing would reopen the exact list-allows/call-denies gap this seam exists to close. Worth recording in the follow-up issue that core_checks.go's stub construction has to be fixed first.
Composite tools are deniable-by-omission — footgun or intended? An admin whose only policy is permit(..., resource in Backend::"github-mcp") silently loses every composite tool, since composites get an empty BackendID at core_vmcp.go:651. Fail-closed is right and it's documented, but it'll surface as "my workflows vanished after I tightened policy." Any appetite for a reserved parent (e.g. Backend::"__composite") so composites are addressable rather than only excludable?
Process notes: the branch is 55 commits behind main and needs a rebase. And since the PR adds unit tests, please run task test and check that box rather than reporting a direct go test invocation — the changed packages run in under a second each.
For what it's worth, I confirmed separately that this feature is live in production: server.New routes unconditionally through core.New (pkg/vmcp/server/server.go:477), the legacy AuthzMiddleware field is built but never inserted into any handler chain (pkg/vmcp/server/derive.go:61-65), and CheckToolCall resolves the real aggregated tool via findAdvertisedTool (core_checks.go:37) so BackendID reaches the pre-dispatch 403 gate too. The stale "#5442 hasn't landed" comments in admission.go:33-37 are pre-existing and not this PR's problem.
|
One question about the scoping direction, since the tests here cover permit and I could not find the forbid case.
if backendID != "" {
backendUID := cedar.NewEntityUID(EntityTypeBackend, cedar.String(backendID))
resourceParents = append(resourceParents, backendUID)
}
For a permit that is the safe direction. My question is the other direction. I traced this through the diff and the converter rather than executing it, because I do not have a Go toolchain here, so treat it as a reading until someone runs it. The check I would run is whether the resource entity has any Backend parent when entities, _ := f.createEntitiesForRequest(
"User::\"alice\"", "Action::\"call_tool\"", "Tool::\"deploy_and_pay\"",
map[string]interface{}{}, map[string]interface{}{}, []string{}, "srv", "",
)If the Tool entity comes back with no Backend parent, the forbid has nothing to bind to. If it does get one, I am wrong and this can be ignored.
I have spent a while recently on a policy evaluator that returned allow for a policy permitting one principal, because a constraint it did not parse became no constraint rather than an unmet one. Same shape, different cause: there the constraint was dropped at parse time, here it cannot attach for want of a parent entity. That is the only reason I noticed this one. |
|
Following my own comment, because I found the precedent for it in your docs and it makes the question sharper than I put it.
That is the same mechanism as the one I asked about. There an absent attribute makes the The mitigation you already recommend for the first case is the interesting part:
If that guidance is the right one, and I think it is, then backend scoping needs the same sentence next to it, because a reader who has internalised "scope with So the question narrows to: is a composite tool intended to be outside backend scope entirely, which is defensible since it belongs to no single backend, or is it intended to be covered by a forbid naming any backend its steps reach? Either answer is fine and they need different docs. The second also needs the parent set, presumably to every backend the workflow touches. Worth adding that the existing caveat is about a claim that may be absent at runtime, which is inherently hard to see coming. This one is structural: Still a reading rather than a run; I have no Go toolchain to hand. The check in my previous comment falsifies it in about a minute if the parent turns out to be there. |
13fa0c6 to
c1504ce
Compare
|
@arian-gogani Thanks for digging into this. Following up on your original comment and the clarification: your reading was correct. I chose the first interpretation for this PR: composites remain outside Backend scope because they have no single originating backend. Therefore neither a backend-scoped
|
|
Thanks for checking it and making the behavior explicit. The new forbid regression and the documentation answer the question I raised: composites are outside Backend scope, backend-scoped forbids do not follow their internal steps, and composite Tool entities need separate authorization. That is a defensible boundary now that policy authors can see it and the test pins it. My original reading is resolved. |
aponcedeleonch
left a comment
There was a problem hiding this comment.
Thanks @Sanskarzz, every point from the review is addressed in c1504ce, and the composite forbid case is now pinned by a test and documented. Approving.
One thing before this can merge: the branch is quite far behind main, so please rebase it to get a fresh CI run on the current base.
|
Thanks, @aponcedeleonch, for reviewing. Now CI is green; the PR is ready to merge |
Summary
VirtualMCPServer currently exposes each tool as a Cedar
Toolentity whose only resource parent is the vMCP itself. As a result, administrators cannot grant access to every tool from one discovered backend without enumerating tools or relying on conflict-resolution name prefixes.BackendIDinto Cedar authorization.Toolentity's parents while preserving its existing vMCPMCPparent.entities_jsonas the source of Backend attributes and transitive parent hierarchies.Fixes #5483
Type of change
Test plan
task test)task test-e2e)task lint-fix)Verified the rebased change with the repository's Taskfile commands:
The full race-enabled unit suite passes. Coverage includes direct backend membership without a request-time Backend entity, different-backend denial despite a misleading tool name, list/call parity, configured transitive Backend hierarchies, unified metadata compatibility, and composite behavior under backend-scoped
permitandforbidpolicies.task lint-fixreports zero issues; the build and license-header checks also pass.Changes
pkg/vmcp/core/admission.gopkg/authz/authorizers/pkg/authz/authorizers/cedar/pkg/vmcp/core/admission_test.goforbidbehavior.docs/authz.md,docs/arch/10-virtual-mcp-architecture.mdDoes this introduce a user-facing change?
Yes. Cedar policies for a VirtualMCPServer can authorize all tools originating from one backend with
resource in Backend::"<backend-id>", without listing individual tools or depending on advertised tool names.Implementation plan
Approved implementation plan
vmcp.Tool.BackendID, which is attached by the aggregation layer, as the trusted backend identity.Backend::<BackendID>as a resource parent UID. Directresource in Backend::"..."evaluation uses the Tool's parent set and does not require a request-time Backend entity.MCP::<vMCP-name>parent to preserve current policies.entities_jsonBackend entities only when policies need Backend attributes or transitive parent hierarchies.permitandforbidbehavior.Special notes for reviewers
entities_jsonremain available for attributes and transitive hierarchy traversal.MCPparent remains.permitnor a backend-scopedforbidmatches a composite.forbiddoes not prevent its internal workflow steps from reaching that backend because those steps do not enter admission as separate top-level tool calls. Backend restrictions should therefore be expressed through permit conditions, with composites authorized separately by theirToolentity.