mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-04 02:31:27 +00:00
fix(key mgmt): a project may only be attached to keys of its own team
A project is created under exactly one team, and its budget and models are validated against that team's. Nothing checked that a key's team matched, so `POST /key/generate` with `team_id: team-a` and a `project_id` owned by `team-b` answered 200 and stored exactly that — a key belonging to team-a, recorded under team-b's project and validated against its models and budget. The same held with no team at all. It needs no proxy-admin rights: an admin of team-a who is a member of no other team can issue keys under another tenant's project with nothing but the project id, and that tenant sees the spend without having granted anything. `_check_project_key_limits` now takes the key's team and refuses a project owned by a different one, before the model and budget checks so the refusal reads as what it is. A project with no owning team is left alone — nobody owns it, so there is no boundary to cross. `/key/update` passes the key's stored team when the request does not carry one, and now runs the check whenever the project itself is set or changed, which a request that moves only the project previously skipped. Two existing cells needed the project's team passed explicitly: they measure the model allowlist, not tenancy, and would otherwise have been asserting model behaviour on a request the new gate refuses. A third builds an unowned project for the same reason. Fixes #41089
This commit is contained in:
parent
9e1ed40db3
commit
eeb13fffbd
3 changed files with 152 additions and 5 deletions
|
|
@ -1547,10 +1547,17 @@ async def _check_project_key_limits(
|
|||
data: GenerateKeyRequest | UpdateKeyRequest,
|
||||
prisma_client: PrismaClient,
|
||||
user_api_key_cache: UserApiKeyCache,
|
||||
key_team_id: str | None = None,
|
||||
) -> None:
|
||||
"""
|
||||
Validate that key's models and budget respect its project's limits.
|
||||
Validate that the key belongs to the project's team, and that its models
|
||||
and budget respect the project's limits.
|
||||
|
||||
- The project's owning team must be the key's team. A project is created
|
||||
under exactly one team and its budget and models are that team's, so a
|
||||
key on another team recorded under it charges a tenant that never granted
|
||||
anything — and issuing one needs no proxy-admin rights, only the project
|
||||
id (#41089). A project with no team belongs to nobody and is left alone.
|
||||
- Key models must be a subset of project models, except the all-team-models / all-proxy-models
|
||||
sentinels, which inherit a parent scope and are narrowed by the project at request time
|
||||
- Key max_budget must be <= project max_budget
|
||||
|
|
@ -1567,6 +1574,17 @@ async def _check_project_key_limits(
|
|||
detail={"error": f"Project not found, project_id={project_id}"},
|
||||
)
|
||||
|
||||
# Validate the project's team owns the key
|
||||
if project_obj.team_id is not None and project_obj.team_id != key_team_id:
|
||||
raise HTTPException(
|
||||
status_code=403,
|
||||
detail={
|
||||
"error": f"Project {project_id} belongs to team {project_obj.team_id}, "
|
||||
f"but the key belongs to {key_team_id if key_team_id is not None else 'no team'}. "
|
||||
"A project can only be attached to keys of the team that owns it."
|
||||
},
|
||||
)
|
||||
|
||||
# Validate key models are a subset of project models
|
||||
if data.models and len(project_obj.models) > 0:
|
||||
for m in data.models:
|
||||
|
|
@ -1922,6 +1940,7 @@ async def generate_key_fn(
|
|||
data=data,
|
||||
prisma_client=prisma_client,
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
key_team_id=data.team_id,
|
||||
)
|
||||
|
||||
return await _common_key_generation_helper(
|
||||
|
|
@ -2864,12 +2883,18 @@ async def _validate_update_key_data(
|
|||
_project_id_to_check: Final = (
|
||||
data.project_id if "project_id" in data.model_fields_set else existing_key_row.project_id
|
||||
)
|
||||
if _project_id_to_check is not None and (data.models is not None or data.max_budget is not None):
|
||||
# Also when the project itself is being set or changed: that is exactly when
|
||||
# the team that owns it has to be checked, and a request that moves only the
|
||||
# project carries neither models nor max_budget (#41089).
|
||||
if _project_id_to_check is not None and (
|
||||
"project_id" in data.model_fields_set or data.models is not None or data.max_budget is not None
|
||||
):
|
||||
await _check_project_key_limits(
|
||||
project_id=_project_id_to_check,
|
||||
data=data,
|
||||
prisma_client=checked_prisma_client,
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
key_team_id=(data.team_id if "team_id" in data.model_fields_set else existing_key_row.team_id),
|
||||
)
|
||||
|
||||
# When the caller asks to change the key's organization_id, require that
|
||||
|
|
|
|||
|
|
@ -17849,11 +17849,13 @@ async def test_regenerate_key_repoints_live_membership_not_the_key_row_it_read(
|
|||
) == ["attached-model"]
|
||||
|
||||
|
||||
async def _cache_with_project(project_id: str, project_models: list[str]) -> UserApiKeyCache:
|
||||
async def _cache_with_project(
|
||||
project_id: str, project_models: list[str], team_id: str | None = "team-lit-5823"
|
||||
) -> UserApiKeyCache:
|
||||
user_api_key_cache = UserApiKeyCache()
|
||||
await user_api_key_cache.async_set_cache(
|
||||
key=_project_cache_key(project_id),
|
||||
value=LiteLLM_ProjectTableCachedObj(project_id=project_id, team_id="team-lit-5823", models=project_models),
|
||||
value=LiteLLM_ProjectTableCachedObj(project_id=project_id, team_id=team_id, models=project_models),
|
||||
model_type=LiteLLM_ProjectTableCachedObj,
|
||||
)
|
||||
return user_api_key_cache
|
||||
|
|
@ -17871,6 +17873,7 @@ async def test_check_project_key_limits_accepts_inherited_model_sentinels(reques
|
|||
data=request_cls(key="sk-lit-5823", models=[sentinel]),
|
||||
prisma_client=MagicMock(),
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
key_team_id="team-lit-5823",
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -17889,6 +17892,7 @@ async def test_check_project_key_limits_still_rejects_real_model_outside_project
|
|||
data=request_cls(key="sk-lit-5823", models=key_models),
|
||||
prisma_client=MagicMock(),
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
key_team_id="team-lit-5823",
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == 400
|
||||
|
|
@ -18257,7 +18261,10 @@ async def test_project_detachment_preserves_omission_and_other_key_fields():
|
|||
@pytest.mark.asyncio
|
||||
async def test_project_detachment_uses_effective_project_for_validation(project_id: str | None):
|
||||
existing: Final = LiteLLM_VerificationToken(token="project-detach-token", project_id="project-orbit")
|
||||
cache: Final = await _cache_with_project("project-orbit", ["model-orbit"])
|
||||
# An unowned project: this cell is about WHICH project the validation uses,
|
||||
# so the ownership gate (#41089) must not be what it measures. Giving the
|
||||
# key a team instead would pull the whole team lookup into a MagicMock db.
|
||||
cache: Final = await _cache_with_project("project-orbit", ["model-orbit"], team_id=None)
|
||||
data: Final = UpdateKeyRequest(key=existing.token, project_id=project_id, models=["model-other"])
|
||||
if project_id is None:
|
||||
await _validate_update_key_data(
|
||||
|
|
|
|||
|
|
@ -0,0 +1,115 @@
|
|||
"""A project may only be attached to keys of the team that owns it (#41089).
|
||||
|
||||
A project is created under exactly one team, and its budget and models are that
|
||||
team's. Nothing checked that the key's team matched, so an admin of team-a — a
|
||||
member of no other team — could issue a key on their own team pointing at a
|
||||
team-b project, and team-b would see the spend under a project they never
|
||||
granted anything on. Only the project id was needed.
|
||||
|
||||
The negative controls are the point: a project with no owning team is left
|
||||
alone, and a key on the owning team still passes, or the check would be a wall
|
||||
rather than a boundary.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.models.project import LiteLLM_ProjectTable
|
||||
from litellm.proxy._types import GenerateKeyRequest, UpdateKeyRequest
|
||||
|
||||
|
||||
def _project(team_id, models=None, project_id="proj-1"):
|
||||
return LiteLLM_ProjectTable(
|
||||
project_id=project_id,
|
||||
team_id=team_id,
|
||||
models=models or [],
|
||||
)
|
||||
|
||||
|
||||
async def _check(project, data, key_team_id):
|
||||
"""Drive _check_project_key_limits with the project the store would return."""
|
||||
from litellm.proxy.management_endpoints import key_management_endpoints as kme
|
||||
|
||||
original = kme.get_project_object
|
||||
kme.get_project_object = AsyncMock(return_value=project)
|
||||
try:
|
||||
await kme._check_project_key_limits(
|
||||
project_id=project.project_id,
|
||||
data=data,
|
||||
prisma_client=MagicMock(),
|
||||
user_api_key_cache=MagicMock(),
|
||||
key_team_id=key_team_id,
|
||||
)
|
||||
finally:
|
||||
kme.get_project_object = original
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_key_may_not_point_at_another_teams_project():
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await _check(
|
||||
_project(team_id="team-b"),
|
||||
GenerateKeyRequest(team_id="team-a"),
|
||||
key_team_id="team-a",
|
||||
)
|
||||
assert exc.value.status_code == 403
|
||||
detail = str(exc.value.detail)
|
||||
assert "team-b" in detail and "team-a" in detail
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_key_with_no_team_may_not_point_at_a_teams_project():
|
||||
# The issue's step 5: no team at all still charges a team's project.
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await _check(
|
||||
_project(team_id="team-b"),
|
||||
GenerateKeyRequest(),
|
||||
key_team_id=None,
|
||||
)
|
||||
assert exc.value.status_code == 403
|
||||
assert "no team" in str(exc.value.detail)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_key_on_the_owning_team_is_accepted():
|
||||
await _check(
|
||||
_project(team_id="team-b"),
|
||||
GenerateKeyRequest(team_id="team-b"),
|
||||
key_team_id="team-b",
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_project_with_no_team_is_left_alone():
|
||||
# Nobody owns it, so there is no boundary to cross — rejecting here would
|
||||
# break every project created outside a team.
|
||||
await _check(_project(team_id=None), GenerateKeyRequest(team_id="team-a"), key_team_id="team-a")
|
||||
await _check(_project(team_id=None), GenerateKeyRequest(), key_team_id=None)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_the_ownership_check_runs_before_the_model_check():
|
||||
# A foreign project must be refused as foreign, not as "model not allowed":
|
||||
# the 400 would read as a configuration problem and hide the tenancy one.
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await _check(
|
||||
_project(team_id="team-b", models=["gpt-4o-mini"]),
|
||||
GenerateKeyRequest(team_id="team-a", models=["gpt-4o"]),
|
||||
key_team_id="team-a",
|
||||
)
|
||||
assert exc.value.status_code == 403
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_an_update_carries_the_keys_existing_team():
|
||||
# /key/update sends no team_id when only the project changes, so the check
|
||||
# has to use the key's stored team rather than treating it as absent.
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await _check(
|
||||
_project(team_id="team-b"),
|
||||
UpdateKeyRequest(key="sk-x", project_id="proj-1"),
|
||||
key_team_id="team-a",
|
||||
)
|
||||
assert exc.value.status_code == 403
|
||||
Loading…
Add table
Reference in a new issue