mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-30 01:52:18 +00:00
chore(proxy): clear inherited credentials per-field on destination change and authorize model-name health checks against the resolved owner
Two residual gaps. The destination-change credential clear was all-or-nothing: supplying any one credential field (e.g. aws_secret_access_key) preserved every other stored secret, so a patch could redirect api_base while keeping the inherited api_key. It now drops each credential the patch did not itself supply. And /health/test_connection only authorized against the resolved deployment when it was looked up by id; resolving by model_name left the auth running against the caller-supplied model_info, so it now derives the owner from the matched deployment on that path too. Adds regression tests for both.
This commit is contained in:
parent
f9cd054445
commit
dbb6306fdf
4 changed files with 79 additions and 9 deletions
|
|
@ -1881,6 +1881,11 @@ async def test_model_connection( # noqa: PLR0915
|
|||
config_litellm_params = dict(
|
||||
deployments[0].get("litellm_params", {})
|
||||
)
|
||||
# Authorize against the matched deployment's owner, not
|
||||
# the caller-supplied model_info (same as the id path).
|
||||
resolved_model_info = dict(
|
||||
deployments[0].get("model_info", {}) or {}
|
||||
)
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.debug(
|
||||
f"Could not find model {model_name} in router: {e}. "
|
||||
|
|
|
|||
|
|
@ -135,19 +135,19 @@ def _strip_credentials_on_destination_change(
|
|||
patch_plaintext: dict,
|
||||
db_plaintext: "Callable[[str], object]",
|
||||
) -> None:
|
||||
"""If the patch changes a destination field (api_base/base_url/custom_llm_provider)
|
||||
without supplying a fresh credential, drop the inherited secret(s) from the
|
||||
merged params so a stored credential is never silently re-pointed at a new
|
||||
(possibly attacker-controlled) endpoint."""
|
||||
"""When a destination field (api_base/base_url/provider endpoint) changes,
|
||||
drop every INHERITED credential — one the patch did not itself supply — so a
|
||||
stored secret is never silently re-pointed at a new (possibly attacker-
|
||||
controlled) endpoint. Cleared per field: supplying one credential must not
|
||||
preserve the others."""
|
||||
destination_changed = any(
|
||||
field in patch_plaintext and patch_plaintext[field] != db_plaintext(field)
|
||||
for field in _DESTINATION_LITELLM_PARAMS
|
||||
)
|
||||
supplied_new_credential = any(
|
||||
field in patch_plaintext for field in _CREDENTIAL_LITELLM_PARAMS
|
||||
)
|
||||
if destination_changed and not supplied_new_credential:
|
||||
for field in _CREDENTIAL_LITELLM_PARAMS:
|
||||
if not destination_changed:
|
||||
return
|
||||
for field in _CREDENTIAL_LITELLM_PARAMS:
|
||||
if field not in patch_plaintext:
|
||||
merged_litellm_params.pop(field, None)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -1987,3 +1987,45 @@ def test_reject_inherited_credential_redirect_helper():
|
|||
config_litellm_params={"api_key": "sk-x"},
|
||||
request_litellm_params={"model": "gpt-4o"},
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_test_connection_model_name_path_authorizes_against_resolved_owner():
|
||||
"""When the deployment is resolved by model_name (no id), auth must still run
|
||||
against the resolved deployment's owner, not the caller-supplied model_info."""
|
||||
victim_deployment = {
|
||||
"model_name": "victim-model",
|
||||
"litellm_params": {
|
||||
"model": "azure/gpt-4o",
|
||||
"api_key": "sk-victim",
|
||||
"api_base": "https://victim.example/v1",
|
||||
},
|
||||
"model_info": {"id": "victim-id", "team_id": "team-OWNER"},
|
||||
}
|
||||
mock_router = MagicMock()
|
||||
mock_router.get_deployment.return_value = None # no id resolution
|
||||
mock_router.get_model_list.return_value = [victim_deployment]
|
||||
|
||||
captured = {}
|
||||
|
||||
async def _capture(*, model_params, **kwargs):
|
||||
captured["team_id"] = model_params.model_info.team_id
|
||||
return True
|
||||
|
||||
mock_auth = AsyncMock(side_effect=_capture)
|
||||
mock_ahealth = AsyncMock(return_value={"status": "healthy"})
|
||||
|
||||
with contextlib.ExitStack() as stack:
|
||||
for p in _health_test_connection_patches(
|
||||
MagicMock(), mock_router, mock_auth, mock_ahealth
|
||||
):
|
||||
stack.enter_context(p)
|
||||
await health_test_model_connection(
|
||||
request=MagicMock(),
|
||||
mode="chat",
|
||||
litellm_params={"model": "victim-model"}, # resolved by name, no id
|
||||
model_info={"team_id": "team-ATTACKER"},
|
||||
user_api_key_dict=MagicMock(user_id="attacker", token="t"),
|
||||
)
|
||||
|
||||
assert captured.get("team_id") == "team-OWNER"
|
||||
|
|
|
|||
|
|
@ -2480,3 +2480,26 @@ class TestModelMgmtAuthzHardening:
|
|||
model_info=None,
|
||||
user_api_key_dict=self._admin(),
|
||||
)
|
||||
|
||||
def test_destination_change_clears_inherited_credential_per_field(self):
|
||||
from litellm.proxy.management_endpoints.model_management_endpoints import (
|
||||
_strip_credentials_on_destination_change,
|
||||
)
|
||||
|
||||
# Patch redirects api_base AND supplies an UNRELATED credential
|
||||
# (aws_secret_access_key); the inherited api_key must still be dropped.
|
||||
merged = {
|
||||
"api_key": "sk-inherited",
|
||||
"aws_secret_access_key": "supplied",
|
||||
"api_base": "https://attacker.example",
|
||||
}
|
||||
patch_plaintext = {
|
||||
"api_base": "https://attacker.example",
|
||||
"aws_secret_access_key": "supplied",
|
||||
}
|
||||
db = {"api_base": "https://real.example", "api_key": "sk-inherited"}
|
||||
_strip_credentials_on_destination_change(
|
||||
merged, patch_plaintext, lambda f: db.get(f)
|
||||
)
|
||||
assert "api_key" not in merged # inherited, not re-supplied -> dropped
|
||||
assert merged.get("aws_secret_access_key") == "supplied" # supplied -> kept
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue