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 78.93% 78.93% -0.01%
==========================================
Files 782 785 +3
Lines 78065 78106 +41
==========================================
+ Hits 61620 61652 +32
- Misses 16440 16449 +9
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amirejaz
left a comment
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.
| if backendID != "" { | ||
| backendUID := cedar.NewEntityUID(EntityTypeBackend, cedar.String(backendID)) | ||
| resourceParents = append(resourceParents, backendUID) | ||
| entities[backendUID] = cedar.Entity{ | ||
| UID: backendUID, | ||
| Parents: cedar.NewEntityUIDSet(), | ||
| Attributes: cedar.NewRecord(cedar.RecordMap{}), | ||
| Tags: cedar.NewRecord(cedar.RecordMap{}), | ||
| } | ||
| } |
There was a problem hiding this comment.
blocker: This entity insert is dead code. cedar-go resolves the direct resource in Backend::"x" case on the Tool entity's own Parents set without ever fetching Backend::"x", and it skips this placeholder during transitive traversal because Parents.Len() == 0 (evalers.go:944-949).
It also contradicts the comment at entity.go:181-185, and the group-parent comment it derives from, which establish that this factory attaches parent UIDs and leaves parent entities to entities_json.
| if backendID != "" { | |
| backendUID := cedar.NewEntityUID(EntityTypeBackend, cedar.String(backendID)) | |
| resourceParents = append(resourceParents, backendUID) | |
| entities[backendUID] = cedar.Entity{ | |
| UID: backendUID, | |
| Parents: cedar.NewEntityUIDSet(), | |
| Attributes: cedar.NewRecord(cedar.RecordMap{}), | |
| Tags: cedar.NewRecord(cedar.RecordMap{}), | |
| } | |
| } | |
| if backendID != "" { | |
| resourceParents = append(resourceParents, cedar.NewEntityUID(EntityTypeBackend, cedar.String(backendID))) | |
| } |
This keeps both the direct case and the entities_json-backed transitive case working, and makes the carve-out at core.go:530-538 unnecessary.
There was a problem hiding this comment.
Thanks for tracing this through cedar-go. Fixed in c1504ce98: request-time Backend entity materialization is removed, and the authorization path now adds only Backend::<BackendID> to the Tool parent UID set. TestCreateEntitiesForRequest_BackendParent verifies both the parent relationship and the absence of a Backend entity in the request map.
| // A request materializes a minimal Backend entity so direct | ||
| // resource-in-Backend policies work without static configuration. | ||
| // Preserve a configured Backend with the same UID because it may | ||
| // carry attributes or parents for transitive backend hierarchies. | ||
| if k.Type == EntityTypeBackend { | ||
| if _, configured := mergedEntities[k]; configured { | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
blocker: This exception exists only to undo the collision the materialization at entity.go:207 introduces — remove that and this goes away with it.
As written it makes Backend the one entity type where configured beats request, inverting the merge contract documented three lines above, inside a generic Cedar primitive that shouldn't know vMCP concepts. It's also quietly fragile: correctness depends on this loop running strictly after the a.entities loop, with nothing stating that dependency.
| // A request materializes a minimal Backend entity so direct | |
| // resource-in-Backend policies work without static configuration. | |
| // Preserve a configured Backend with the same UID because it may | |
| // carry attributes or parents for transitive backend hierarchies. | |
| if k.Type == EntityTypeBackend { | |
| if _, configured := mergedEntities[k]; configured { | |
| continue | |
| } | |
| } |
There was a problem hiding this comment.
Agreed. With the placeholder removed, I also removed the Backend-specific merge carve-out from IsAuthorized, restoring the normal request-entity merge behavior. The configured transitive Backend -> BackendGroup case remains covered by TestAuthorizeWithJWTClaims_BackendHierarchyPreserved.
| _, backendExists := entities[backendUID] | ||
| assert.Equal(t, tt.wantBackendEntity, backendExists, | ||
| "Backend entity must be materialized for Cedar hierarchy traversal") |
There was a problem hiding this comment.
suggestion: Tied to the blocker above rather than independently wrong — but these assert an implementation detail rather than behaviour, and they're the only two failures when the dead code is removed.
| _, backendExists := entities[backendUID] | |
| assert.Equal(t, tt.wantBackendEntity, backendExists, | |
| "Backend entity must be materialized for Cedar hierarchy traversal") |
wantBackendEntity stays in use at :652, so the struct field can remain. Keep that assertion and wantParentCount at :649: those test what Cedar actually consumes, and they'd catch a genuine regression.
There was a problem hiding this comment.
Updated. The test no longer requires a materialized Backend entity. It retains the behavioral assertions for MCP/Backend parent counts and Backend parent membership, and now explicitly verifies that the request map contains only the principal, action, and resource entities.
| // createEntitiesForRequest adds the request's principal, action, and resource | ||
| // entities. A non-empty backendID also makes the resource a child of a | ||
| // materialized Backend entity so Cedar can traverse backend membership. | ||
| func (f *EntityFactory) createEntitiesForRequest( |
There was a problem hiding this comment.
suggestion: An exported and an unexported method differing by one capital letter and one parameter is easy to misread at a call site. Once the materialization is dropped the delta is a single append, so a variadic on the existing exported method collapses this back to one function — all four internal callers (core.go:1092,1131,1177,1217) pass nothing extra:
func (f *EntityFactory) CreateEntitiesForRequest(
principal, action, resource string,
claimsMap, attributes map[string]interface{},
groups []string,
serverName string,
extraResourceParents ...cedar.EntityUID,
) (cedar.EntityMap, error)There was a problem hiding this comment.
Applied this shape. CreateEntitiesForRequest is now the single method and accepts variadic extraResourceParents ...cedar.EntityUID. The Cedar tool authorization path constructs the Backend UID and passes it through that generic mechanism.
| // BackendID is the logical vMCP backend identifier. It MUST be sourced from the | ||
| // aggregated capability, never from client-supplied request data such as tool | ||
| // arguments or an advertised-name prefix. | ||
| type ResourceMetadata struct { |
There was a problem hiding this comment.
suggestion: This is now the second context channel carrying the same category of thing — trusted server-side facts about the resource under authorization — written back-to-back with WithToolAnnotations at admission.go:156-158 and read back-to-back at core.go:1025-1030.
Per the "avoid parallel types that drift" guidance in .claude/rules/go-style.md, a third such fact will have two plausible homes. Consider Annotations *ToolAnnotations as a field here, or adding BackendID where annotations already live.
Not blocking — the ctx approach itself is the right trade, since it avoids modifying the stable Authorizer interface for one implementation's concern.
There was a problem hiding this comment.
Consolidated the new vMCP path: ResourceMetadata now carries both BackendID and Annotations, so list and call admission write one metadata value. Existing annotation helpers remain compatible for current Cedar/HTTP PDP callers through a shared precedence helper. Tests cover legacy reads, unified reads, and conflicting values.
| for i := range tools { | ||
| tool := &tools[i] | ||
| toolCtx := ctx | ||
| toolCtx := authorizers.WithResourceMetadata(ctx, authorizers.ResourceMetadata{BackendID: tool.BackendID}) |
There was a problem hiding this comment.
nitpick: Metadata is injected unconditionally while annotations on the next line are gated on != nil. Harmless, but two adjacent lines following different rules invites a "which one is the bug?" moment later. Either gate on tool.BackendID != "", or add a few words saying the empty case is deliberate — it is, it fails closed for composites.
There was a problem hiding this comment.
Added explicit comments on both list and call paths explaining that an empty BackendID is deliberate for backendless composite tools. Backend identity and annotations are also now injected together through ResourceMetadata. TestCedarAdmission_BackendScopedPolicy covers the empty-Backend permit outcome and list/call parity.
|
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
|
3 participants
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.