diff --git a/litellm/proxy/health_endpoints/_health_endpoints.py b/litellm/proxy/health_endpoints/_health_endpoints.py index b94bee8d312..9b1fe29471b 100644 --- a/litellm/proxy/health_endpoints/_health_endpoints.py +++ b/litellm/proxy/health_endpoints/_health_endpoints.py @@ -83,40 +83,39 @@ _HEALTH_CREDENTIAL_FIELDS = ( "litellm_credential_name", "aws_secret_access_key", "aws_session_token", + "azure_ad_token", "vertex_credentials", ) -_HEALTH_DESTINATION_FIELDS = ( - "api_base", - "base_url", - "aws_bedrock_runtime_endpoint", - "aws_sts_endpoint", - "api_version", - "vertex_location", - "vertex_project", - "aws_region_name", -) # Caller-controlled endpoint URLs: overriding one re-points the request (and any -# credential) at a new host, so it is SSRF-validated. +# credential) at a new host, so it is SSRF-validated. Mirrors the endpoint-redirect +# subset of auth_utils._BANNED_REQUEST_BODY_PARAMS. _HEALTH_URL_FIELDS = ( "api_base", "base_url", "aws_bedrock_runtime_endpoint", "aws_sts_endpoint", + "sagemaker_base_url", + "s3_endpoint_url", + "deployment_url", ) def _assert_non_admin_destination_override_is_safe( - request_litellm_params: dict, user_api_key_dict: UserAPIKeyAuth + config_litellm_params: dict, + request_litellm_params: dict, + user_api_key_dict: UserAPIKeyAuth, ) -> None: """Confused-deputy / SSRF guard for /health/test_connection. When a non-admin overrides the connection target (api_base/base_url/provider - endpoint), the request must carry its own non-empty credential. Otherwise the - downstream provider falls back to a stored or environment secret (e.g. the - resolved deployment's key, or OPENAI_API_KEY) and sends it to the - caller-chosen address. The overridden URL is also SSRF-validated so it cannot - point at an internal host or cloud-metadata endpoint. Proxy admins are - unaffected. + endpoint), every credential the resolved deployment stores must be re-supplied + by the request (non-empty), and the request must carry a non-empty credential + at all. Otherwise the downstream provider sends a stored credential, or falls + back to an environment secret (e.g. OPENAI_API_KEY), to the caller-chosen + address — including the case where the caller supplies an unrelated credential + field to leave the stored one riding along. The overridden URL is also + SSRF-validated so it cannot point at an internal or cloud-metadata host. + Proxy admins are unaffected. """ from litellm.litellm_core_utils.url_utils import SSRFError, validate_url from litellm.proxy.common_utils.resource_ownership import is_proxy_admin @@ -125,6 +124,14 @@ def _assert_non_admin_destination_override_is_safe( return if not any(request_litellm_params.get(field) for field in _HEALTH_URL_FIELDS): return + for field in _HEALTH_CREDENTIAL_FIELDS: + if config_litellm_params.get(field) and not request_litellm_params.get(field): + raise HTTPException( + status_code=400, + detail={ + "error": f"Re-provide {field} when overriding the connection target; a stored credential cannot be sent to a caller-chosen endpoint." + }, + ) if not any( request_litellm_params.get(field) for field in _HEALTH_CREDENTIAL_FIELDS ): @@ -1926,6 +1933,7 @@ async def test_model_connection( # noqa: PLR0915 # A non-admin overriding the destination must bring their own credential # and a non-internal URL, so no stored/environment secret is sent out. _assert_non_admin_destination_override_is_safe( + config_litellm_params=config_litellm_params, request_litellm_params=request_litellm_params, user_api_key_dict=user_api_key_dict, ) diff --git a/litellm/proxy/management_endpoints/model_management_endpoints.py b/litellm/proxy/management_endpoints/model_management_endpoints.py index 3ce666804da..f1eab257253 100644 --- a/litellm/proxy/management_endpoints/model_management_endpoints.py +++ b/litellm/proxy/management_endpoints/model_management_endpoints.py @@ -36,6 +36,7 @@ from litellm.proxy._types import ( TeamModelDeleteRequest, UserAPIKeyAuth, ) +from litellm.litellm_core_utils.litellm_logging import _CUSTOM_PRICING_KEYS from litellm.litellm_core_utils.url_utils import SSRFError, validate_url from litellm.proxy.auth.user_api_key_auth import user_api_key_auth from litellm.proxy.common_utils.encrypt_decrypt_utils import ( @@ -75,14 +76,20 @@ _CREDENTIAL_LITELLM_PARAMS = ( "litellm_credential_name", "aws_secret_access_key", "aws_session_token", + "azure_ad_token", "vertex_credentials", ) -# Caller-controlled endpoint URLs that must be SSRF-validated on write. +# Caller-controlled endpoint URLs that must be SSRF-validated on write. Mirrors +# the endpoint-redirect subset of auth_utils._BANNED_REQUEST_BODY_PARAMS so a +# stored config can't reach an internal host that the request-time guard blocks. _URL_LITELLM_PARAMS = ( "api_base", "base_url", "aws_bedrock_runtime_endpoint", "aws_sts_endpoint", + "sagemaker_base_url", + "s3_endpoint_url", + "deployment_url", ) # Destination fields whose change must drop an inherited credential (URLs above # plus the provider selector, which isn't a URL so isn't SSRF-validated). @@ -135,18 +142,31 @@ def _assert_credential_resupplied_on_destination_change( db_plaintext: "Callable[[str], object]", ) -> None: """Changing a model's destination (api_base/base_url/provider endpoint) must - not send an inherited credential to the new endpoint. Clearing the credential - is NOT sufficient: an empty/absent key falls back to the proxy's environment - secret (e.g. OPENAI_API_KEY) and would still be sent there. So require the - caller to re-supply, with a non-empty value, every credential the model - already had; otherwise reject. A non-empty re-supplied value is the caller's - own credential going to their own endpoint, which is allowed.""" + not send a credential to the new endpoint that the caller did not themselves + provide. Two things are required. First, the patch must carry a non-empty + credential at all: even a model stored without one falls back to the proxy's + environment secret (e.g. OPENAI_API_KEY) at call time, which would then be + sent to the new endpoint. Second, every credential the model already had must + be re-supplied non-empty, so an inherited secret is never kept alongside an + unrelated new one. A non-empty re-supplied value is the caller's own + credential going to their own endpoint, which is allowed.""" destination_changed = any( field in patch_plaintext and patch_plaintext[field] != db_plaintext(field) for field in _DESTINATION_LITELLM_PARAMS ) if not destination_changed: return + if not any(patch_plaintext.get(field) for field in _CREDENTIAL_LITELLM_PARAMS): + raise ProxyException( + message=( + "Provide a non-empty credential (e.g. api_key) when changing the " + "model endpoint; otherwise the proxy's environment credential " + "would be used at the new destination." + ), + type=ProxyErrorTypes.validation_error.value, + code=status.HTTP_400_BAD_REQUEST, + param="api_key", + ) for field in _CREDENTIAL_LITELLM_PARAMS: if db_plaintext(field) and not patch_plaintext.get(field): raise ProxyException( @@ -162,10 +182,15 @@ def _assert_credential_resupplied_on_destination_change( def _is_pricing_field(field: str) -> bool: - """Custom per-token / per-second / per-pixel / per-request (and tiered) cost - overrides all follow this naming, so match by convention rather than an - enumerated subset that drifts out of date.""" - return "cost_per" in field or field.endswith("_cost") + """A field that influences cost/spend accounting, so it is proxy-admin-only. + + The authoritative boundary is litellm's own custom-pricing key set (the same + fields the cost calculator reads from a deployment), so this cannot drift + behind the calculator. base_model redirects the cost-lookup model, and the + "cost" substring is a belt-and-suspenders catch for any future field that + follows the naming convention before it lands in the type. + """ + return field in _CUSTOM_PRICING_KEYS or field == "base_model" or "cost" in field def _contains_env_reference(value: object) -> bool: diff --git a/tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py b/tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py index 915719f82fe..ea82ec51743 100644 --- a/tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py +++ b/tests/test_litellm/proxy/health_endpoints/test_health_endpoints.py @@ -1967,27 +1967,35 @@ def test_non_admin_destination_override_guard(): non_admin = MagicMock(user_role="internal_user") admin = MagicMock(user_role=LitellmUserRoles.PROXY_ADMIN) + def guard(config, request, user): + _assert_non_admin_destination_override_is_safe(config, request, user) + # Non-admin overrides api_base without supplying a credential -> rejected # (a stored or environment key would otherwise be sent there). with pytest.raises(HTTPException): - _assert_non_admin_destination_override_is_safe( - {"api_base": "https://attacker.example"}, non_admin + guard({}, {"api_base": "https://attacker.example"}, non_admin) + # Stored credential not re-supplied (caller sends an UNRELATED field) -> + # rejected; the stored api_key must not ride along to the new endpoint. + with pytest.raises(HTTPException): + guard( + {"api_key": "sk-victim", "api_base": "https://victim"}, + {"api_base": "https://attacker.example", "aws_session_token": "x"}, + non_admin, ) with patch.object(litellm, "user_url_validation", False): # Non-admin overrides WITH their own credential -> allowed. - _assert_non_admin_destination_override_is_safe( - {"api_base": "https://attacker.example", "api_key": "sk-own"}, non_admin + guard( + {}, {"api_base": "https://attacker.example", "api_key": "sk-own"}, non_admin ) # No destination override -> allowed. - _assert_non_admin_destination_override_is_safe({"model": "gpt-4o"}, non_admin) + guard({"api_key": "sk-victim"}, {"model": "gpt-4o"}, non_admin) # Proxy admin is exempt even when overriding without a credential. - _assert_non_admin_destination_override_is_safe( - {"api_base": "https://attacker.example"}, admin - ) + guard({"api_key": "sk-victim"}, {"api_base": "https://attacker.example"}, admin) # Non-admin override to an internal/metadata IP -> SSRF-rejected. with patch.object(litellm, "user_url_validation", True): with pytest.raises(HTTPException): - _assert_non_admin_destination_override_is_safe( + guard( + {}, { "api_base": "http://169.254.169.254/latest/meta-data/", "api_key": "sk-own", @@ -2072,3 +2080,52 @@ async def test_test_connection_model_name_path_authorizes_against_resolved_owner ) assert captured.get("team_id") == "team-OWNER" + + +@pytest.mark.asyncio +async def test_test_connection_rejects_partial_credential_resupply_on_override(): + """A non-admin overriding api_base while supplying only an UNRELATED credential + field must not let the resolved deployment's stored api_key ride along to the + new endpoint.""" + from fastapi import HTTPException + + from litellm.types.router import Deployment, LiteLLM_Params + + victim = Deployment( + model_name="victim", + litellm_params=LiteLLM_Params( + model="openai/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.side_effect = lambda model_id: ( + victim if model_id == "victim-id" else None + ) + mock_auth = AsyncMock(return_value=True) + 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) + with pytest.raises(HTTPException) as exc_info: + await health_test_model_connection( + request=MagicMock(), + mode="chat", + litellm_params={ + "model": "openai/gpt-4o", + "api_base": "https://attacker.example/v1", + "aws_session_token": "x", # unrelated; api_key NOT re-supplied + }, + model_info={"id": "victim-id"}, + user_api_key_dict=MagicMock( + user_id="attacker", token="t", user_role="internal_user" + ), + ) + assert exc_info.value.status_code == 400 + + mock_ahealth.assert_not_called() # stored sk-victim never went out diff --git a/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py index 68ab1e969d4..d00d52c26ad 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py @@ -1996,10 +1996,22 @@ class TestModelMgmtAuthzHardening: db = {"api_base": "https://real.example.com", "api_key": "sk-stored"} self._assert_resupply({"api_base": "https://real.example.com"}, db) - def test_keyless_model_destination_change_is_allowed(self): - # A model that never carried a credential has nothing to leak. + def test_keyless_model_destination_change_requires_credential(self): + from litellm.proxy._types import ProxyException + + # Even a model stored without a credential falls back to the proxy env + # key (e.g. OPENAI_API_KEY) at call time, so repointing it without + # supplying a credential must be rejected. db = {"api_base": "https://real.example.com"} - self._assert_resupply({"api_base": "https://attacker.example.com"}, db) + with pytest.raises(ProxyException) as e: + self._assert_resupply({"api_base": "https://attacker.example.com"}, db) + assert str(e.value.code) == "400" + + def test_keyless_model_destination_change_with_credential_is_allowed(self): + db = {"api_base": "https://real.example.com"} + self._assert_resupply( + {"api_base": "https://attacker.example.com", "api_key": "sk-own"}, db + ) def test_validate_model_url_params_blocks_internal_ip(self): import litellm @@ -2018,6 +2030,10 @@ class TestModelMgmtAuthzHardening: _validate_model_url_params( {"aws_bedrock_runtime_endpoint": "http://169.254.169.254/"} ) + with pytest.raises(ProxyException): + _validate_model_url_params( + {"sagemaker_base_url": "http://169.254.169.254/"} + ) # Honors the opt-out toggle (internal endpoints / Ollama). with ( patch.object(litellm, "user_url_validation", False), @@ -2466,3 +2482,65 @@ class TestModelMgmtAuthzHardening: db, ) assert e.value.param == "api_key" + + # --- Veria sweep follow-ups: authoritative pricing set, base_model, extra URLs --- + + def test_spend_affecting_fields_are_proxy_admin_only(self): + from litellm.proxy._types import ProxyException + from litellm.proxy.management_endpoints.model_management_endpoints import ( + _assert_privileged_model_fields_authorized, + _is_pricing_field, + ) + from litellm.types.router import ModelInfo, updateLiteLLMParams + + # Gate uses litellm's authoritative custom-pricing key set (not a subset), + # plus base_model which redirects the cost-lookup model. + for f in ( + "tiered_pricing", + "cache_read_input_token_cost_flex", + "cache_creation_input_token_cost_above_1hr", + "base_model", + "input_cost_per_second", + ): + assert _is_pricing_field(f), f + for f in ("api_key", "api_base", "model", "custom_llm_provider"): + assert not _is_pricing_field(f), f + + # A non-admin cannot zero a cache cost field outside the old subset... + with pytest.raises(ProxyException) as e: + _assert_privileged_model_fields_authorized( + litellm_params=updateLiteLLMParams( + cache_read_input_token_cost_flex=0.0 + ), + model_info=None, + user_api_key_dict=self._non_admin(), + ) + assert str(e.value.code) == "403" + # ...nor repoint cost lookup via model_info.base_model. + with pytest.raises(ProxyException) as e: + _assert_privileged_model_fields_authorized( + litellm_params=None, + model_info=ModelInfo(base_model="gpt-3.5-turbo"), + user_api_key_dict=self._non_admin(), + ) + assert str(e.value.code) == "403" + # Proxy admin may set them. + _assert_privileged_model_fields_authorized( + litellm_params=updateLiteLLMParams(cache_read_input_token_cost_flex=0.0), + model_info=ModelInfo(base_model="gpt-3.5-turbo"), + user_api_key_dict=self._admin(), + ) + + def test_sagemaker_base_url_change_requires_credential_resupply(self): + from litellm.proxy._types import ProxyException + + # sagemaker_base_url retargets outbound SageMaker traffic, so changing it + # without re-supplying the stored AWS credential must be rejected. + db = { + "sagemaker_base_url": "https://real.example", + "aws_secret_access_key": "stored", + } + with pytest.raises(ProxyException): + self._assert_resupply( + {"sagemaker_base_url": "https://attacker.example"}, db + )