feat(mcp): layered MCP access policies — env default, team entitlement, key narrowing - #822
Conversation
…t, key narrowing
New etcd resource kind mcp_policies ({scope: env|team, scope_ref, mode:
none|selected|all, allow, deny, expires_at, enabled}) and a new optional
api_keys field mcp_access ({mode: inherit|restrict|deny, allow, deny}).
The /mcp endpoint now resolves the caller's effective tool ACL from the
key together with the environment-default and team policies:
- base grant = the key team's active policy, else the env default;
- inherit uses it unchanged, restrict intersects the key's own allow
patterns on top (narrow-only), deny grants nothing;
- deny patterns are a global union (env + team + key) and always win,
including over legacy keys;
- keys without an mcp_access block keep the exact legacy allowed_tools
allow side — policies never widen an unmigrated key;
- disabled/expired policies neither grant nor deny.
tools/list filtering and tools/call rejection share the one resolved
ACL, as before. Legacy allowed_tools semantics (null/empty = no access,
single-* globs, server__tool namespace) are unchanged.
New harness mock: a stateless Streamable HTTP MCP upstream built on the official TypeScript SDK (echo + reverse tools per server), interop- testing the gateway's ephemeral rmcp client against the reference implementation. Eight cases pin the layered-ACL contract end to end (real binary + etcd + two upstreams): legacy scoping, deny-overlay-without-widening on legacy keys, env-default inherit, team takeover with surviving env deny, team all-mode, restrict narrowing, deny mode, and watch-path propagation of policy edits and deletes.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds environment and team MCP policies, per-key access modes, snapshot propagation, effective ACL resolution, schema support, and unit/integration/E2E coverage for policy-driven tool access. ChangesMCP policy contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPProxy
participant ToolAcl
participant Snapshot
participant MCPGateway
Client->>MCPProxy: Send MCP request
MCPProxy->>Snapshot: Read key and MCP policies
MCPProxy->>ToolAcl: Resolve effective ACL
ToolAcl->>MCPGateway: Apply allow and deny layers
MCPGateway->>Client: Return tools or reject request
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/aisix-mcp/src/gateway.rs (1)
190-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
denymode discards the accumulated policy deny list.Functionally equivalent today (the empty allow layer already rejects everything), but it makes the branch depend on
permitsnever being reworked to short-circuit on deny. Reusing the already-builtdenykeeps the invariant local.♻️ Optional tightening
- McpAccessMode::Deny => Self { - allow: vec![AllowLayer::Patterns(Vec::new())], - deny: Vec::new(), - }, + McpAccessMode::Deny => Self { + allow: vec![AllowLayer::Patterns(Vec::new())], + deny, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-mcp/src/gateway.rs` around lines 190 - 194, Update the McpAccessMode::Deny branch in the access-policy construction to reuse the already accumulated deny list instead of initializing deny to an empty vector. Preserve the rejecting empty allow layer while retaining the existing deny entries locally.tests/e2e/src/cases/mcp-access-policy-e2e.test.ts (1)
345-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test mutates shared suite state and only passes because it runs last.
It rewrites
T1_POLICY_IDtomode: "none"and deletesENV_POLICY_ID, invalidating the steady state thatbeforeAllestablished for every other test in the file. Nothing restores it, so any test appended after this one — or a runner configured to shuffle within a file — breaks. Restoring both rows at the end (and re-waiting for propagation) makes the suite order-independent.♻️ Restore the seeded state after the mutation checks
const restored = await callTool(KEY_LEGACY_WILD, "beta__reverse", "hi"); expect(restored).toEqual({ ok: true, text: "ih" }); await expectList(KEY_INHERIT, []); + + // Restore the suite's steady state so this test carries no ordering + // dependency for anything appended after it. + await seed.update("mcp_policies", ENV_POLICY_ID, { + scope: "env", + mode: "selected", + allow: ["alpha__*"], + deny: ["beta__reverse"], + }); + await seed.update("mcp_policies", T1_POLICY_ID, { + scope: "team", + scope_ref: TEAM1, + mode: "selected", + allow: ["beta__*"], + }); + await waitConfigPropagation(async () => { + for (const [token, names] of EXPECTED) { + if (!(await listMatches(token, names))) return false; + } + return true; + }); });As per coding guidelines, "Avoid explicit dependencies between tests and hidden execution order assumptions".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/src/cases/mcp-access-policy-e2e.test.ts` around lines 345 - 374, Restore the shared policy state at the end of the “policy edits and deletes propagate through the watch path” test: update T1_POLICY_ID back to its beforeAll mode and recreate ENV_POLICY_ID with its original seeded values. Wait for configuration propagation and verify the restored key behavior before the test completes, so subsequent tests do not depend on execution order.Source: Coding guidelines
tests/e2e/src/harness/upstream-mcp.ts (2)
109-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwallowing the error makes upstream failures undebuggable.
A malformed body, SDK error, or transport failure becomes a bare 500 with no trace; the E2E assertion then fails on a tool-name mismatch with no root cause. Binding and logging the error costs nothing in a harness.
♻️ Log before responding
- } catch { - if (!res.headersSent) res.writeHead(500).end(); + } catch (err) { + console.error(`mcp upstream ${label}: request failed`, err); + if (!res.headersSent) res.writeHead(500).end(); }As per coding guidelines, "Every function return value must be checked for errors (if applicable); errors must be properly handled, not ignored or silently swallowed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/src/harness/upstream-mcp.ts` around lines 109 - 111, Update the catch block in the upstream request handler to bind the caught error and log its details before sending the 500 response. Preserve the existing !res.headersSent guard and response behavior while ensuring malformed bodies, SDK errors, and transport failures are no longer silently swallowed.Source: Coding guidelines
45-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winForce-close pooled sockets here
httpServer.close()waits for keep-alive connections, so this teardown can stall if the MCP client keeps a socket open. CallcloseAllConnections()afterclose()to make shutdown deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/src/harness/upstream-mcp.ts` around lines 45 - 49, Update the teardown callback in the upstream MCP harness to force-close pooled sockets after invoking httpServer.close(), using httpServer.closeAllConnections() so shutdown does not wait on keep-alive connections. Preserve the existing Promise resolution behavior and close callback flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-core/src/models/mcp_policy.rs`:
- Around line 113-135: Replace the doc comment immediately above McpAccessMode
with public API reference text describing the enum and its Inherit, Restrict,
and Deny variants; leave McpAccess’s existing comment unchanged. Regenerate the
affected resource schemas using the dump-schema command so the corrected enum
description appears in the generated API documentation.
---
Nitpick comments:
In `@crates/aisix-mcp/src/gateway.rs`:
- Around line 190-194: Update the McpAccessMode::Deny branch in the
access-policy construction to reuse the already accumulated deny list instead of
initializing deny to an empty vector. Preserve the rejecting empty allow layer
while retaining the existing deny entries locally.
In `@tests/e2e/src/cases/mcp-access-policy-e2e.test.ts`:
- Around line 345-374: Restore the shared policy state at the end of the “policy
edits and deletes propagate through the watch path” test: update T1_POLICY_ID
back to its beforeAll mode and recreate ENV_POLICY_ID with its original seeded
values. Wait for configuration propagation and verify the restored key behavior
before the test completes, so subsequent tests do not depend on execution order.
In `@tests/e2e/src/harness/upstream-mcp.ts`:
- Around line 109-111: Update the catch block in the upstream request handler to
bind the caught error and log its details before sending the 500 response.
Preserve the existing !res.headersSent guard and response behavior while
ensuring malformed bodies, SDK errors, and transport failures are no longer
silently swallowed.
- Around line 45-49: Update the teardown callback in the upstream MCP harness to
force-close pooled sockets after invoking httpServer.close(), using
httpServer.closeAllConnections() so shutdown does not wait on keep-alive
connections. Preserve the existing Promise resolution behavior and close
callback flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbd3e63e-beaf-427f-8b57-7140c8222692
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
crates/aisix-admin/src/openapi.rscrates/aisix-core/src/bin/dump-schema.rscrates/aisix-core/src/models/apikey.rscrates/aisix-core/src/models/mcp_policy.rscrates/aisix-core/src/models/mod.rscrates/aisix-core/src/models/schema.rscrates/aisix-core/src/models/snapshot.rscrates/aisix-etcd/src/loader.rscrates/aisix-etcd/src/supervisor.rscrates/aisix-mcp/Cargo.tomlcrates/aisix-mcp/src/gateway.rscrates/aisix-mcp/tests/gateway_aggregation.rscrates/aisix-proxy/src/mcp.rsschemas/resources/api_key.schema.jsonschemas/resources/mcp_policy.schema.jsontests/e2e/package.jsontests/e2e/src/cases/mcp-access-policy-e2e.test.tstests/e2e/src/harness/index.tstests/e2e/src/harness/upstream-mcp.ts
…r duplicate rows - is_active_at: is_none_or form (clippy::nonminimal_bool held the lint job red under -D warnings). - McpAccessMode / McpAccess / McpPolicyScope / McpPolicyMode doc comments rewritten as public API reference text: the mode enum no longer carries its parent block's description, and rustdoc link syntax no longer leaks into the generated schemas (regenerated). - resolve: deny patterns now union across every active applicable policy row, so a deny can never vanish by losing the duplicate-row tie-break; the grant side still picks the lowest id. - Pinned three edge cases: restrict with empty allow, selected with empty allow, and a scope_ref-less team row (grants nothing, never shadows the env default). Standalone resources-file support for mcp_policies is deferred to #823.
1 participant
Fixes api7/AISIX-Cloud#1034
Problem
MCP tool access is configured only on individual API keys (
allowed_tools). For a fleet of hundreds or thousands of caller keys that means per-key configuration, repeated edits whenever an MCP server or tool is added, and inevitable permission drift. The linked issue asks for team-level entitlements inherited by member keys, key-level narrowing that can never widen, and an environment-wide default with an explicit — never implicit — "all current and future tools" mode.What this adds
A layered MCP access policy, resolved per request into the same single tool ACL that already backs
tools/listfiltering andtools/callrejection:mcp_policies:{scope: env|team, scope_ref, mode: none|selected|all, allow[], deny[], expires_at?, enabled}. One env-default row per environment plus one row per team; the control plane is the writer and enforces uniqueness (the DP still resolves duplicates deterministically).allcovers current and future tools and is a deliberate mode, not a default.mcp_access:{mode: inherit|restrict|deny, allow[], deny[]}.inherituses the inherited grant unchanged,restrictintersects the key's own patterns on top (narrow-only),denygrants nothing.mcp_accesskeeps the exact legacyallowed_toolsallow side (no inheritance, null/empty = no access, single-*globs,server__toolnamespace — all unchanged and still pinned by the existing tests). Policies can only narrow an unmigrated key, never widen it, so upgrading cannot silently grant access.<expiry, same boundary as key expiry).Prior-art comparison
Surveyed how established gateways shape MCP/tool entitlements before landing on this design:
Where this lands: the same narrow-only inheritance direction as the proxy baseline, but with explicit modes at every level (
none|selected|all,inherit|restrict|deny) instead of overloaded empty-list semantics — required anyway because our legacy contract already assigns "empty = no access" — plus deny patterns and an environment-scoped default object, which the issue explicitly calls for (deny overrides, env-wide grants with preview). Theserver__*glob pattern language is unchanged from the existingallowed_toolscontract.Cross-plane pairing
Per the config-knob rule this DP change is half of the feature; the paired control-plane PR (env policy + team entitlement APIs, projection, migration preview/apply, effective-permissions view, dashboard) follows against
api7/AISIX-Cloud, plus a docs PR againstapi7/docs.Upgrade order note: the API-key document schema is closed (
deny_unknown_fields), so a key carrying the newmcp_accessfield is rejected (fail-closed, 401 for that key) by a pre-upgrade data plane. The control plane only writes the field once an operator configures it, and the standard release ships both planes together, so the exposure is the usual brief cross-plane window.Tests
aisix-mcpresolution-semantics matrix (12 cases: legacy parity, deny overlay on legacy keys, mode combinations, team takeover + surviving env deny, restrict narrowing, inactive policies, deterministic duplicate handling) plus an HTTP-level gateway case.aisix-proxyendpoint tests:denymode rejected at the ACL (before routing), env-policy deny overlaying a legacy wildcard key.tests/e2e/src/cases/mcp-access-policy-e2e.test.ts): real binary + etcd + two real MCP upstreams built on the official TypeScript SDK (interop for the ephemeral rmcp client), 8 cases covering every mode plus watch-path propagation of policy edits/deletes. New harness helperstartMcpUpstream— the suite's first MCP mock.schemas/resources/regenerated (newmcp_policy.schema.json, extendedapi_key.schema.json).Deliberately deferred
The resources-file (standalone) source does not yet accept a top-level
mcp_policies:collection, while file-mode api_keys already parsemcp_access— an inherit-mode key in a resources file resolves to zero MCP access until the policy kind lands there (fail-closed). Tracked as #823; until then the layered policy is a managed-mode surface.