mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-21 00:21:49 +00:00
fix(proxy): refuse runtime writes to config-owned settings
A write into a settings store for a key the config file declares used to land in the runtime layer and then lose to the config on every read, so the caller saw success while nothing changed. It now raises ConfigOwnedKeyError, and the allowed-IP routes turn that into a 400 naming the key instead of reporting success on a list they never changed. Both allowed-IP routes now build a new list rather than mutating the one the config layer holds, and the os.environ resolver rebuilds the config it is given instead of writing back into it, so a reader can no longer corrupt the raw values the store keeps for provenance. The database reload leaves a config-owned key alone rather than writing a normalized copy back over it, which would now raise and abort the rest of the reconcile pass.
This commit is contained in:
Yuneng Jiang
parent
800b09ba41
commit
a987efca2c
6 changed files with 215 additions and 38 deletions
|
|
@ -17,6 +17,14 @@ from litellm.proxy.config_resolvers.settings_rules import (
|
|||
rule_for,
|
||||
)
|
||||
|
||||
|
||||
class ConfigOwnedKeyError(RuntimeError):
|
||||
def __init__(self, section: Section, key: str) -> None:
|
||||
super().__init__(f"{section}.{key} is set in the config file and cannot be changed at runtime")
|
||||
self.section: Final = section
|
||||
self.key: Final = key
|
||||
|
||||
|
||||
_EMPTY_VALUES: Final[Mapping[str, JsonValue]] = MappingProxyType({})
|
||||
_EMPTY_ROWS: Final[Mapping[DbRow, Mapping[str, JsonValue]]] = MappingProxyType({})
|
||||
|
||||
|
|
@ -72,8 +80,8 @@ class SettingsStore(MutableMapping[str, JsonValue]):
|
|||
return resolved.value
|
||||
|
||||
def __setitem__(self, key: str, value: JsonValue) -> None:
|
||||
if self.owned_by_config(key):
|
||||
return
|
||||
if self.owned_by_config(key) and value != self.get(key):
|
||||
raise ConfigOwnedKeyError(self._section, key)
|
||||
self._runtime_values = MappingProxyType({**self._runtime_values, key: value})
|
||||
self._deleted_runtime_keys = self._deleted_runtime_keys - frozenset((key,))
|
||||
|
||||
|
|
@ -81,7 +89,7 @@ class SettingsStore(MutableMapping[str, JsonValue]):
|
|||
if key not in self:
|
||||
raise KeyError(key)
|
||||
if self.owned_by_config(key):
|
||||
return
|
||||
raise ConfigOwnedKeyError(self._section, key)
|
||||
self._runtime_values = MappingProxyType(
|
||||
{key_: value for key_, value in self._runtime_values.items() if key_ != key}
|
||||
)
|
||||
|
|
|
|||
|
|
@ -5221,20 +5221,27 @@ class ProxyConfig:
|
|||
verbose_proxy_logger.warning("Maximum recursion depth (%s) reached while processing config.", max_depth)
|
||||
return config
|
||||
|
||||
for key, value in config.items():
|
||||
if isinstance(value, dict):
|
||||
config[key] = self._check_for_os_environ_vars(config=value, depth=depth + 1, max_depth=max_depth)
|
||||
elif isinstance(value, list):
|
||||
for item in value:
|
||||
if isinstance(item, dict):
|
||||
item = self._check_for_os_environ_vars(config=item, depth=depth + 1, max_depth=max_depth)
|
||||
# if the value is a string and starts with "os.environ/" - then it's an environment variable
|
||||
elif isinstance(value, str) and value.startswith("os.environ/"):
|
||||
resolved = get_secret(value)
|
||||
if resolved is None and secret_manager_would_be_consulted(value):
|
||||
verbose_proxy_logger.warning("%s is absent from the configured secret manager", value)
|
||||
config[key] = resolved
|
||||
return config
|
||||
return {
|
||||
key: self._resolved_config_value(value=value, depth=depth, max_depth=max_depth)
|
||||
for key, value in config.items()
|
||||
}
|
||||
|
||||
def _resolved_config_value(self, value: object, depth: int, max_depth: int) -> object:
|
||||
if isinstance(value, dict):
|
||||
return self._check_for_os_environ_vars(config=value, depth=depth + 1, max_depth=max_depth)
|
||||
if isinstance(value, list):
|
||||
return [
|
||||
self._check_for_os_environ_vars(config=item, depth=depth + 1, max_depth=max_depth)
|
||||
if isinstance(item, dict)
|
||||
else item
|
||||
for item in value
|
||||
]
|
||||
if isinstance(value, str) and value.startswith("os.environ/"):
|
||||
resolved: Final = get_secret(value)
|
||||
if resolved is None and secret_manager_would_be_consulted(value):
|
||||
verbose_proxy_logger.warning("%s is absent from the configured secret manager", value)
|
||||
return resolved
|
||||
return value
|
||||
|
||||
def _initialize_secret_manager_from_raw_config(
|
||||
self, config: Mapping[str, object], config_file_path: str | None
|
||||
|
|
@ -7321,7 +7328,9 @@ class ProxyConfig:
|
|||
"disable_auto_add_proxy_admin_to_teams",
|
||||
"apply_user_budget_to_team_keys",
|
||||
):
|
||||
if key in db_values and (value := self.settings.get(key)) is not None:
|
||||
if key not in db_values or self.settings.owned_by_config(key):
|
||||
continue
|
||||
if (value := self.settings.get(key)) is not None:
|
||||
self.settings[key] = coerce_bool(value)
|
||||
|
||||
async def _apply_cache_size_setting(
|
||||
|
|
@ -7331,21 +7340,24 @@ class ProxyConfig:
|
|||
) -> None:
|
||||
if "user_api_key_cache_max_size" not in db_values and not cache_size_was_db:
|
||||
return
|
||||
writable: Final = not self.settings.owned_by_config("user_api_key_cache_max_size")
|
||||
cache_value: Final = self.settings.get("user_api_key_cache_max_size")
|
||||
try:
|
||||
cache_max_size: Final = ConfigGeneralSettings.model_validate(
|
||||
MappingProxyType({"user_api_key_cache_max_size": cache_value})
|
||||
).user_api_key_cache_max_size
|
||||
except ValidationError:
|
||||
self.settings.pop("user_api_key_cache_max_size", None)
|
||||
if writable:
|
||||
self.settings.pop("user_api_key_cache_max_size", None)
|
||||
verbose_proxy_logger.warning(
|
||||
"Ignoring invalid general_settings.user_api_key_cache_max_size=%r from the DB", cache_value
|
||||
)
|
||||
return
|
||||
if cache_max_size is None:
|
||||
self.settings.pop("user_api_key_cache_max_size", None)
|
||||
else:
|
||||
self.settings["user_api_key_cache_max_size"] = cache_max_size
|
||||
if writable:
|
||||
if cache_max_size is None:
|
||||
self.settings.pop("user_api_key_cache_max_size", None)
|
||||
else:
|
||||
self.settings["user_api_key_cache_max_size"] = cache_max_size
|
||||
user_api_key_cache.update_in_memory_max_size(cache_max_size)
|
||||
|
||||
async def _apply_store_model_in_db_setting(self, db_values: Mapping[str, SettingsJsonValue]) -> None:
|
||||
|
|
@ -7357,7 +7369,8 @@ class ProxyConfig:
|
|||
return
|
||||
normalized: Final = coerce_bool(value)
|
||||
store_model_in_db = normalized if isinstance(normalized, bool) else bool(normalized)
|
||||
self.settings["store_model_in_db"] = store_model_in_db
|
||||
if not self.settings.owned_by_config("store_model_in_db"):
|
||||
self.settings["store_model_in_db"] = store_model_in_db
|
||||
|
||||
async def _apply_retention_settings(
|
||||
self,
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@ import asyncio
|
|||
import json
|
||||
import os
|
||||
from collections import Counter
|
||||
from collections.abc import Mapping, Sequence
|
||||
from collections.abc import Mapping, MutableMapping, Sequence
|
||||
from types import MappingProxyType
|
||||
from typing import (
|
||||
Final,
|
||||
|
|
@ -24,6 +24,7 @@ from litellm.litellm_core_utils.sensitive_data_masker import mask_sensitive_keys
|
|||
from litellm.proxy._experimental.mcp_server.tool_search import MCP_TOOL_SEARCH_SETTINGS_KEY
|
||||
from litellm.proxy._types import *
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
from litellm.proxy.config_resolvers.settings_store import ConfigOwnedKeyError
|
||||
from litellm.proxy.config_resolvers.sso import (
|
||||
SSO_FIELD_ENV_VARS,
|
||||
SSO_SECRET_FIELDS,
|
||||
|
|
@ -489,6 +490,23 @@ async def get_allowed_ips():
|
|||
return {"data": _allowed_ip}
|
||||
|
||||
|
||||
def _store_allowed_ips(general_settings: MutableMapping[str, object], allowed_ips: Sequence[str]) -> None:
|
||||
try:
|
||||
general_settings["allowed_ips"] = list(allowed_ips)
|
||||
except ConfigOwnedKeyError as owned:
|
||||
raise HTTPException(
|
||||
status_code=400,
|
||||
detail={
|
||||
"error": f"{owned.section} key '{owned.key}' is set in the config file and cannot be changed here",
|
||||
"keys": [owned.key],
|
||||
"section": owned.section,
|
||||
"resolution": (
|
||||
"edit the config file to change it, or remove it from the file to let the database own it"
|
||||
),
|
||||
},
|
||||
) from owned
|
||||
|
||||
|
||||
@router.post(
|
||||
"/add/allowed_ip",
|
||||
tags=["Budget & Spend Tracking"],
|
||||
|
|
@ -509,12 +527,10 @@ async def add_allowed_ip(
|
|||
if prisma_client is None:
|
||||
raise Exception("No DB Connected")
|
||||
|
||||
_allowed_ips: Final[list] = general_settings.get("allowed_ips", [])
|
||||
if ip_address.ip not in _allowed_ips:
|
||||
_allowed_ips.append(ip_address.ip)
|
||||
general_settings["allowed_ips"] = _allowed_ips
|
||||
else:
|
||||
_allowed_ips: Final[Sequence[str]] = general_settings.get("allowed_ips") or []
|
||||
if ip_address.ip in _allowed_ips:
|
||||
raise HTTPException(status_code=400, detail="IP address already exists")
|
||||
_store_allowed_ips(general_settings, (*_allowed_ips, ip_address.ip))
|
||||
|
||||
if store_model_in_db is not True:
|
||||
raise HTTPException(
|
||||
|
|
@ -568,12 +584,10 @@ async def delete_allowed_ip(
|
|||
proxy_config,
|
||||
)
|
||||
|
||||
_allowed_ips: Final[list] = general_settings.get("allowed_ips", [])
|
||||
if ip_address.ip in _allowed_ips:
|
||||
_allowed_ips.remove(ip_address.ip)
|
||||
general_settings["allowed_ips"] = _allowed_ips
|
||||
else:
|
||||
_allowed_ips: Final[Sequence[str]] = general_settings.get("allowed_ips") or []
|
||||
if ip_address.ip not in _allowed_ips:
|
||||
raise HTTPException(status_code=404, detail="IP address not found")
|
||||
_store_allowed_ips(general_settings, tuple(ip for ip in _allowed_ips if ip != ip_address.ip))
|
||||
|
||||
# Load existing config
|
||||
config: Final = await proxy_config.get_config()
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ from unittest.mock import patch
|
|||
import pytest
|
||||
|
||||
from litellm.proxy.config_resolvers.settings_rules import JsonValue
|
||||
from litellm.proxy.config_resolvers.settings_store import SettingsStore
|
||||
from litellm.proxy.config_resolvers.settings_store import ConfigOwnedKeyError, SettingsStore
|
||||
|
||||
|
||||
def test_settings_store_matches_plain_dict_mapping_operations() -> None:
|
||||
|
|
@ -162,13 +162,27 @@ def test_settings_store_refuses_a_runtime_write_to_a_config_owned_key() -> None:
|
|||
store: Final = SettingsStore("general_settings")
|
||||
store.load_yaml({"max_parallel_requests": 3})
|
||||
|
||||
store["max_parallel_requests"] = 11
|
||||
del store["max_parallel_requests"]
|
||||
with pytest.raises(ConfigOwnedKeyError) as write:
|
||||
store["max_parallel_requests"] = 11
|
||||
with pytest.raises(ConfigOwnedKeyError):
|
||||
del store["max_parallel_requests"]
|
||||
|
||||
assert "max_parallel_requests" in str(write.value)
|
||||
assert store["max_parallel_requests"] == 3
|
||||
assert store.source("max_parallel_requests") == "config"
|
||||
|
||||
|
||||
def test_settings_store_accepts_a_write_that_does_not_change_a_config_owned_value() -> None:
|
||||
store: Final = SettingsStore("general_settings")
|
||||
store.load_yaml({"master_key": "os.environ/MASTER_KEY"})
|
||||
store.apply_runtime_values({"master_key": "sk-resolved"})
|
||||
|
||||
store["master_key"] = "sk-resolved"
|
||||
|
||||
assert store["master_key"] == "sk-resolved"
|
||||
assert store.source("master_key") == "config"
|
||||
|
||||
|
||||
@pytest.mark.timeout(10)
|
||||
def test_settings_store_clear_removes_every_key_the_config_file_does_not_own() -> None:
|
||||
store: Final = SettingsStore("general_settings")
|
||||
|
|
@ -282,3 +296,12 @@ def test_settings_store_starts_with_an_unset_source() -> None:
|
|||
store: Final = SettingsStore("general_settings")
|
||||
|
||||
assert store.source("unknown") == "unset"
|
||||
|
||||
|
||||
def test_settings_store_still_accepts_a_write_to_a_key_the_config_does_not_own() -> None:
|
||||
store: Final = SettingsStore("general_settings")
|
||||
store.load_yaml({"allowed_ips": ["1.2.3.4"]})
|
||||
|
||||
store["max_parallel_requests"] = 7
|
||||
|
||||
assert store["max_parallel_requests"] == 7
|
||||
|
|
|
|||
|
|
@ -3311,6 +3311,76 @@ async def test_load_config_rejects_malformed_role_permissions(tmp_path):
|
|||
await ProxyConfig().load_config(router=MagicMock(), config_file_path=str(config_file))
|
||||
|
||||
|
||||
def test_os_environ_resolution_leaves_the_config_layer_holding_the_reference(monkeypatch):
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
monkeypatch.setenv("PROOF_NESTED_SECRET", "sk-nested-value")
|
||||
proxy_config: Final = ProxyConfig()
|
||||
config: Final = {
|
||||
"general_settings": {
|
||||
"master_key": "os.environ/PROOF_NESTED_SECRET",
|
||||
"coordination_redis": {"password": "os.environ/PROOF_NESTED_SECRET"},
|
||||
}
|
||||
}
|
||||
|
||||
proxy_config._load_yaml_settings_stores(config)
|
||||
resolved: Final = proxy_config._check_for_os_environ_vars(
|
||||
config=proxy_config._config_with_resolved_settings(config)
|
||||
)
|
||||
|
||||
assert resolved["general_settings"]["coordination_redis"]["password"] == "sk-nested-value"
|
||||
assert resolved["general_settings"]["master_key"] == "sk-nested-value"
|
||||
assert proxy_config.settings.config_value("master_key") == "os.environ/PROOF_NESTED_SECRET"
|
||||
assert proxy_config.settings.config_value("coordination_redis") == {
|
||||
"password": "os.environ/PROOF_NESTED_SECRET"
|
||||
}
|
||||
|
||||
|
||||
def test_os_environ_resolution_reaches_dicts_nested_in_a_list(monkeypatch):
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
monkeypatch.setenv("PROOF_LIST_SECRET", "sk-list-value")
|
||||
config: Final = {"model_list": [{"litellm_params": {"api_key": "os.environ/PROOF_LIST_SECRET"}}]}
|
||||
|
||||
resolved: Final = ProxyConfig()._check_for_os_environ_vars(config=config)
|
||||
|
||||
assert resolved["model_list"][0]["litellm_params"]["api_key"] == "sk-list-value"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("config_cache_size", ("not-a-number", "7"))
|
||||
@pytest.mark.asyncio
|
||||
async def test_db_reload_finishes_when_the_config_owns_a_setting_the_db_also_sets(monkeypatch, config_cache_size):
|
||||
import litellm
|
||||
from litellm.proxy import proxy_server as proxy_server_module
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
monkeypatch.setattr(litellm, "user_url_allowed_hosts", [], raising=False)
|
||||
monkeypatch.setattr(proxy_server_module, "user_api_key_cache", MagicMock(), raising=False)
|
||||
proxy_config: Final = ProxyConfig()
|
||||
proxy_config.settings.load_yaml(
|
||||
{
|
||||
"store_prompts_in_spend_logs": "os.environ/PROOF_FLAG",
|
||||
"store_model_in_db": "os.environ/PROOF_FLAG",
|
||||
"user_api_key_cache_max_size": config_cache_size,
|
||||
}
|
||||
)
|
||||
monkeypatch.setattr(proxy_server_module, "general_settings", proxy_config.settings, raising=False)
|
||||
|
||||
await proxy_config._update_general_settings(
|
||||
{
|
||||
"store_prompts_in_spend_logs": False,
|
||||
"store_model_in_db": False,
|
||||
"user_api_key_cache_max_size": 5,
|
||||
"user_url_allowed_hosts": ["proof.example.com"],
|
||||
}
|
||||
)
|
||||
|
||||
assert litellm.user_url_allowed_hosts == ["proof.example.com"]
|
||||
assert proxy_config.settings["store_prompts_in_spend_logs"] == "os.environ/PROOF_FLAG"
|
||||
assert proxy_config.settings["store_model_in_db"] == "os.environ/PROOF_FLAG"
|
||||
assert proxy_config.settings["user_api_key_cache_max_size"] == config_cache_size
|
||||
|
||||
|
||||
def test_max_ui_session_budget_default_is_one_dollar():
|
||||
"""LIT-4662: the dashboard session budget default is a product decision; the
|
||||
old 0.25 default locked admins out of auto router Test Connection and the
|
||||
|
|
|
|||
|
|
@ -2662,6 +2662,55 @@ def test_delete_allowed_ip_writes_deleted_audit_log(monkeypatch):
|
|||
app.dependency_overrides.pop(user_api_key_auth, None)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("route", ["/add/allowed_ip", "/delete/allowed_ip"])
|
||||
def test_allowed_ip_routes_refuse_a_config_owned_list_with_a_clear_400(route, monkeypatch):
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import litellm.proxy.proxy_server as proxy_server_module
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
from litellm.proxy.config_resolvers.settings_store import SettingsStore
|
||||
|
||||
store = SettingsStore("general_settings")
|
||||
store.load_yaml({"allowed_ips": ["203.0.113.77"]})
|
||||
saved = []
|
||||
|
||||
fake_prisma = MagicMock()
|
||||
fake_prisma.db.litellm_auditlog.create = AsyncMock()
|
||||
|
||||
async def _get_config():
|
||||
return {"general_settings": {"allowed_ips": ["203.0.113.77"]}}
|
||||
|
||||
async def _save_config(new_config=None):
|
||||
saved.append(new_config)
|
||||
return new_config
|
||||
|
||||
monkeypatch.setattr(proxy_server_module, "prisma_client", fake_prisma)
|
||||
monkeypatch.setattr(proxy_server_module, "store_model_in_db", True)
|
||||
monkeypatch.setattr(proxy_server_module, "general_settings", store)
|
||||
monkeypatch.setattr(proxy_server_module.proxy_config, "get_config", _get_config)
|
||||
monkeypatch.setattr(proxy_server_module.proxy_config, "save_config", _save_config)
|
||||
|
||||
async def _admin_auth():
|
||||
return UserAPIKeyAuth(
|
||||
user_id="config-admin",
|
||||
api_key="hashed-admin-key",
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN,
|
||||
)
|
||||
|
||||
app.dependency_overrides[user_api_key_auth] = _admin_auth
|
||||
try:
|
||||
ip = "198.51.100.9" if route == "/add/allowed_ip" else "203.0.113.77"
|
||||
resp = client.post(route, json={"ip": ip})
|
||||
|
||||
assert resp.status_code == 400, resp.text
|
||||
assert "allowed_ips" in resp.text
|
||||
assert list(store["allowed_ips"]) == ["203.0.113.77"]
|
||||
assert saved == []
|
||||
finally:
|
||||
app.dependency_overrides.pop(user_api_key_auth, None)
|
||||
|
||||
|
||||
def test_update_ui_theme_settings_writes_audit_log(mock_proxy_config, monkeypatch):
|
||||
"""Updating the UI theme must be audited under ui_theme_config."""
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue