feat(mcp/auth): additive key access-group grants + opt-in member assignment#29313
Conversation
A key whose unified access_group_ids grant a private MCP server was having that grant intersected against its team's MCP ceiling, so a key in a team scoped to other servers (or with no own scope) lost the granted server entirely. Resolve access_group_ids once as ungated additive grants and union them on top of the key/team ceiling instead of folding them into the key scope that gets intersected.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR makes two related changes to how unified access groups (
Confidence Score: 4/5The implementation is internally consistent and well-tested, but the shift from a lookup-time gate to a write-time gate changes the MCP access model in a way that depends on teams correctly configuring the new permission. The MCP resolution path now treats any access_group_ids attached to a key as an unconditional additive grant, dropping the group-ownership check that previously prevented a key from reaching servers the group admin never intended to expose. The compensating KEY_ACCESS_GROUP_ASSIGNMENT gate only fires at key creation and update for team keys; personal keys remain ungated on both the old and new paths. The tests cover the new ceiling/grant interaction thoroughly and the gate implementation is correct for team keys, but the changed threat model warrants careful review before merge. user_api_key_auth_mcp.py carries the most consequential change in _get_key_access_group_mcp_server_extras; key_management_endpoints.py is worth a close look to confirm enforcement covers all mutation paths for team keys.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Core MCP authorization reworked: _get_allowed_mcp_servers_for_key drops access-group resolution, and _get_key_access_group_mcp_server_extras is now the single ungated additive path. The removal of the assigned_team_ids/assigned_key_ids gate is the central security tradeoff flagged in a prior review thread. |
| litellm/proxy/management_helpers/team_member_permission_checks.py | Adds enforce_member_can_assign_access_groups, a default-deny gate for team-key access group assignment. Logic is correct: proxy admins and team admins bypass, non-members get 403, and personal (teamless) keys are explicitly out of scope. |
| litellm/proxy/management_endpoints/key_management_endpoints.py | The new enforcement is wired into _team_key_generation_check, _validate_update_key_data, and regenerate_key_fn. Personal (teamless) keys are ungated by design. |
| litellm/proxy/_types.py | Adds KEY_ACCESS_GROUP_ASSIGNMENT as a field-level permission token (not a real HTTP route) in management_routes so the UI can display it as a team-member checkbox. No actual handler exists for the URL, so no routing impact. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Several existing tests are renamed and their assertions inverted to match the new additive-grant semantics; a new parametrized regression test_access_group_grants_are_additive_over_ceiling is added covering the three critical ceiling/grant interaction scenarios. |
| ui/litellm-dashboard/src/components/team/permission_definitions.tsx | Two-line addition registering the human-readable label for the new /key/access_group_assignment permission checkbox. |
Reviews (3): Last reviewed commit: "fix(auth): gate access-group assignment ..." | Re-trigger Greptile
|
On the |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis PR extends MCP authentication to support additive access-group grants on API keys, along with an opt-in path for assigning access groups to members or teams. The touched authorization logic determines which MCP servers a key can access based on team and access-group configuration. One security issue remains open: access-group IDs on a key can be used without verifying that the group is actually assigned to the key or its team, allowing a non-admin with key configuration ability to reach MCP servers from another group. This is an authorization bypass with clear cross-group access impact, though it depends on the attacker being able to set access-group IDs. Two previously reported issues have already been addressed, so the remaining risk is narrowed but still significant. Open issues (1)
Fixed/addressed: 2 · PR risk: 7/10 |
The previous commit moved key.access_group_ids resolution out of the intersected key ceiling (_get_allowed_mcp_servers_for_key) and into the ungated additive grant path (_get_key_access_group_mcp_server_extras), unioned on top of the team ceiling. Five tests from #28890/#29195 still asserted the old gated / in-key-scope contract and failed: - _get_allowed_mcp_servers_for_key now returns the object_permission ceiling only and never resolves access_group_ids; two tests now assert the group resolver is not called from that path (with and without an object_permission present). - The extras path is ungated, so a group whose assigned_team_ids / assigned_key_ids exclude the caller still contributes its servers. - The end-to-end test asserts the grant surfaces via the extras path rather than the base key path. - Dropped test_key_access_group_ids_empty_returns_no_extras; the empty case is already covered by the extras family's no-groups test.
|
@greptileai re review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Non-admin team members could attach access_group_ids to keys they create or update, letting them self-grant resources (MCP servers/models) the team admin never intended. Add an opt-in KEY_ACCESS_GROUP_ASSIGNMENT team-member permission (default-deny) enforced at /key/generate and /key/update; proxy and team admins bypass. Surfaces automatically as a checkbox in the team Member Permissions UI.
ryan-crabbe-berri
changed the title
…itellm_mcp_key_access_group_additive_grants
RegenerateKeyRequest inherits access_group_ids and prepare_key_update_data persists it, so a non-admin key owner could self-grant access groups by regenerating. Apply the same opt-in member gate using the existing key's team.
|
@greptileai re review |
Add unit tests for enforce_member_can_assign_access_groups (deny without opt-in, allow with opt-in, and proxy-admin / team-admin / non-team-key bypasses) and for _get_key_access_group_mcp_server_extras (no-auth and no-resolved-servers return empty, resolved ids are expanded, errors degrade to no grants).
| valid_token=user_api_key_auth, | ||
| team_object=None, | ||
| resource_field="access_mcp_server_ids", | ||
| raw_server_ids = await _get_mcp_server_ids_from_access_groups( |
There was a problem hiding this comment.
High: MCP access-group authorization bypass
_get_mcp_server_ids_from_access_groups fetches resources for every ID on the key and does not check assigned_team_ids or assigned_key_ids before those servers are unioned over the team ceiling. A non-admin who can set access_group_ids on a personal key, or on a team key where the new field permission is enabled, can attach another group's ID and call MCP tools on that group's servers; keep the additive behavior only after verifying the group is assigned to the key or the key's team.
There was a problem hiding this comment.
this is the point, we are unioning the keys ag's mcp's with the team instead of keeping the team as the gate. this behavior falls in line with the current design of access groups being purely additive. it will be opt in to be able to allow internal users to attach access groups to the key, this will be a a team member permission you can set
ryan-crabbe-berri
merged commit 90b5104
into
litellm_internal_staging
…gnment (BerriAI#29313) * fix(mcp): make key.access_group_ids grants additive over team ceiling A key whose unified access_group_ids grant a private MCP server was having that grant intersected against its team's MCP ceiling, so a key in a team scoped to other servers (or with no own scope) lost the granted server entirely. Resolve access_group_ids once as ungated additive grants and union them on top of the key/team ceiling instead of folding them into the key scope that gets intersected. * test(mcp): align key access-group tests with additive-grant model The previous commit moved key.access_group_ids resolution out of the intersected key ceiling (_get_allowed_mcp_servers_for_key) and into the ungated additive grant path (_get_key_access_group_mcp_server_extras), unioned on top of the team ceiling. Five tests from BerriAI#28890/BerriAI#29195 still asserted the old gated / in-key-scope contract and failed: - _get_allowed_mcp_servers_for_key now returns the object_permission ceiling only and never resolves access_group_ids; two tests now assert the group resolver is not called from that path (with and without an object_permission present). - The extras path is ungated, so a group whose assigned_team_ids / assigned_key_ids exclude the caller still contributes its servers. - The end-to-end test asserts the grant surfaces via the extras path rather than the base key path. - Dropped test_key_access_group_ids_empty_returns_no_extras; the empty case is already covered by the extras family's no-groups test. * feat(auth): gate member access-group assignment on keys behind opt-in Non-admin team members could attach access_group_ids to keys they create or update, letting them self-grant resources (MCP servers/models) the team admin never intended. Add an opt-in KEY_ACCESS_GROUP_ASSIGNMENT team-member permission (default-deny) enforced at /key/generate and /key/update; proxy and team admins bypass. Surfaces automatically as a checkbox in the team Member Permissions UI. * fix(auth): gate access-group assignment on /key/regenerate too RegenerateKeyRequest inherits access_group_ids and prepare_key_update_data persists it, so a non-admin key owner could self-grant access groups by regenerating. Apply the same opt-in member gate using the existing key's team. * test(auth): cover member access-group gate and additive MCP grants Add unit tests for enforce_member_can_assign_access_groups (deny without opt-in, allow with opt-in, and proxy-admin / team-admin / non-team-key bypasses) and for _get_key_access_group_mcp_server_extras (no-auth and no-resolved-servers return empty, resolved ids are expanded, errors degrade to no grants).
2 participants
Summary
Two related changes to how unified access groups (
access_group_ids) interact with team scopes on virtual keys:Additive MCP grants (runtime). A virtual key whose
access_group_idsgrant a private MCP server now actually sees and can call that server, even when its team's MCP scope is limited to other servers (or the key has no scope of its own). Previously the grant was intersected against the key/team ceiling and silently dropped, so the key got an empty server list and a 403. The grant is resolved once as an ungated additive permission and unioned on top of the ceiling; attaching the access group to the key is itself the grant, with noassigned_team_ids/assigned_key_idsre-check on this path.Opt-in member assignment (authorization). Because attaching an access group is now a real grant, we gate who can attach one. A new default-deny
KEY_ACCESS_GROUP_ASSIGNMENT(/key/access_group_assignment) team-member permission controls whether non-admin members may setaccess_group_idson keys they create or update. Enforced at/key/generateand/key/update(403 when not opted in); proxy admins and team admins always bypass, and keys created without access groups are unaffected. It surfaces automatically as a checkbox in the team Member Permissions UI ("Member can assign access groups to virtual keys for this team").Together: MCP grants are additive, but a non-admin member can only self-grant via an access group if their team has explicitly enabled it.
Screenshots
before
after
Member Permissions checkbox (opt-in)
Test plan
Additive MCP grants
tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py::TestMCPRequestHandler::test_access_group_grants_are_additive_over_ceiling— parametrized regression covering grant-over-team-ceiling, grant-survives-empty-key/team-intersection, and no-grant-keeps-intersection (guards against additive leakage).{test}only), a fresh access group containing context7 (not assigned to any team or key), and a key in that team pointing at the group viaaccess_group_ids. Ran the same probe on the pre-fix commit and on this branch:Opt-in member assignment
tests/access_matrix/verify_member_ag_permission.py), 7/7 pass. Seeds a fresh team, a non-admin member, and an access group, then exercises generate + update with/without the permission:tests/proxy_behavior/management/test_team_permissions.py,test_key_update.py,test_team_permissions_bulk_update.py, andtest_litellm/.../test_team_member_permission_checks.py).tests/access_matrix/AG_PERMISSION_QA.md).