Authorize MCP Skills extension requests - #6512
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6512 +/- ##
==========================================
- Coverage 78.92% 78.90% -0.03%
==========================================
Files 781 781
Lines 77881 77969 +88
==========================================
+ Hits 61467 61518 +51
- Misses 16409 16446 +37
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
There was a problem hiding this comment.
Reviewed with parallel MCP protocol, security, Go quality, and docs sync passes — all clean. One optional nit: authorizeGet in pkg/authz/authorizers/cedar/core.go has a dead case MCPFeatureTool, MCPFeatureResource: fallthrough above default that returns the same error either way; not blocking.
Fail-closed behavior for missing/duplicate/malformed skill URIs verified end-to-end, and the skills/get/skills/list shapes match the SEP-2640 reference spec.
247beb4 to
f3280e9
Compare
jhrozek
left a comment
There was a problem hiding this comment.
Re-checked after the update — the one nit from my earlier review (dead fallthrough case in authorizeGet) is still present but non-blocking. Fail-closed behavior and skills/get, skills/list shapes look good.
Summary
skills/getto CedarAction::"get_skill"/Skill::<exact URI>and filterskills/listentries by that same permission.resources/readauthorization.Type of change
Test plan
task test) — attempted withTMPDIR="$PWD/.tmp" task test; the full race suite failed in unrelated existing tests:pkg/client.TestDetectProjectRoot,pkg/server/discovery.TestCheckHealth_UnixSocket_Success, and the 10-minute timeout inpkg/transport.TestBridge_ToolsListChanged_TriggersReSync.task test-e2e)TMPDIR="$PWD/.tmp" task lint-fix)skills/get,skills/listresponse filtering, malformed/duplicate URI rejection, and SSE error substitution; automated regressions cover each path.Changes
pkg/mcp,pkg/authzget_skillpolicy mapping.docs/authz.mdDoes this introduce a user-facing change?
Yes. Operators can now write
get_skillpolicies for direct-proxied MCP servers that negotiate SEP-2640. Askills/listresponse exposes only the skills the caller may get; reading its files still requires the existing resource-read permission.Implementation plan
Approved implementation plan
skillauthorization feature and parseskills/getby its literal URI.skills/getto an exact CedarSkillentity and admission-check invalid/ambiguous identifiers before dispatch.skills/listbut filter every entry through the sameget_skillpermission; fail closed on unsafe response shapes.Deferred: vMCP aggregation/routing, content-integrity verification, consent records, host materialization, dynamic Skills, and
resources/directory/read.Special notes for reviewers
This deliberately implements only the direct-proxy authorization seam. ToolHive does not advertise, fabricate, or rewrite the Skills extension; client and backend capability negotiation remains end-to-end. The full unit suite currently has unrelated failures noted above.