mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-14 23:21:35 +00:00
fix(credentials): scope access validation and credential_info merge to logging destinations
Three collateral regressions surfaced while reviewing this PR, each narrowing feature-added behavior back toward base for non-destination paths. validate_credential_access now runs only for logging destinations, so a provider credential carrying an unrelated access key is no longer rejected with a 400. update_db_credential and _sync_in_memory_credential subfield-merge credential_info only for logging destinations; provider credentials keep base replace semantics. regenerate_key_fn no longer swallows the team-lookup 404, so regenerating a key whose team was deleted fails like base instead of silently proceeding against a dangling team The logging-destination merge preserves credential_type, description, and the untouched access subfields, which the Edit-access UI modal depends on since it patches only access. The gate keys off the credential_type tag alone (not a full parse of access) so a destination with a malformed access is still routed into validation and rejected. Follow-up LIT-5004 tracks the general /credentials PATCH replace-vs-merge decision for all credential types
This commit is contained in:
Yucheng Zhu
parent
f8500bf23e
commit
22b04d0f67
6 changed files with 229 additions and 21 deletions
|
|
@ -14,6 +14,7 @@ from litellm.litellm_core_utils.litellm_logging import _get_masked_values
|
|||
from litellm.proxy._types import CommonProxyErrors, UserAPIKeyAuth
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
from litellm.proxy.common_utils.encrypt_decrypt_utils import encrypt_value_helper
|
||||
from litellm.proxy.management_endpoints.logging_exporter_access import is_logging_credential
|
||||
from litellm.proxy.management_endpoints.logging_exporter_validation import (
|
||||
validate_credential_access,
|
||||
)
|
||||
|
|
@ -65,7 +66,8 @@ async def create_credential(
|
|||
"""
|
||||
from litellm.proxy.proxy_server import llm_router, prisma_client
|
||||
|
||||
validate_credential_access(credential.credential_info)
|
||||
if is_logging_credential(credential.credential_info):
|
||||
validate_credential_access(credential.credential_info)
|
||||
|
||||
try:
|
||||
if prisma_client is None:
|
||||
|
|
@ -285,9 +287,12 @@ def update_db_credential(
|
|||
merged_credential.credential_values.update(encrypted_params)
|
||||
|
||||
if encrypted_credential.credential_info:
|
||||
if merged_credential.credential_info is None:
|
||||
merged_credential.credential_info = {}
|
||||
_merge_credential_info(merged_credential.credential_info, encrypted_credential.credential_info)
|
||||
if is_logging_credential(db_credential.credential_info):
|
||||
if merged_credential.credential_info is None:
|
||||
merged_credential.credential_info = {}
|
||||
_merge_credential_info(merged_credential.credential_info, encrypted_credential.credential_info)
|
||||
else:
|
||||
merged_credential.credential_info = encrypted_credential.credential_info
|
||||
|
||||
return merged_credential
|
||||
|
||||
|
|
@ -344,8 +349,6 @@ async def update_credential(
|
|||
"""
|
||||
from litellm.proxy.proxy_server import prisma_client
|
||||
|
||||
validate_credential_access(credential.credential_info)
|
||||
|
||||
try:
|
||||
if prisma_client is None:
|
||||
raise HTTPException(
|
||||
|
|
@ -356,6 +359,8 @@ async def update_credential(
|
|||
db_credential = await credentials_repository.find_by_name(credential_name)
|
||||
if db_credential is None:
|
||||
raise HTTPException(status_code=404, detail="Credential not found in DB.")
|
||||
if is_logging_credential(db_credential.credential_info) or is_logging_credential(credential.credential_info):
|
||||
validate_credential_access(credential.credential_info)
|
||||
merged_credential = update_db_credential(db_credential, _patch_to_credential_item(credential, credential_name))
|
||||
credential_object_jsonified = jsonify_object(merged_credential.model_dump())
|
||||
await credentials_repository.update_by_name(
|
||||
|
|
@ -384,10 +389,11 @@ def _sync_in_memory_credential(
|
|||
"""Mirror the DB write into ``litellm.credential_list``.
|
||||
|
||||
Skips when the credential isn't resident in memory (e.g. created on
|
||||
another scaled instance, restored from DB on the next reload). The
|
||||
in-memory ``credential_info`` is merged subfield-by-subfield via
|
||||
``_merge_credential_info`` so a partial patch can't clobber stored
|
||||
``access`` subfields it didn't touch.
|
||||
another scaled instance, restored from DB on the next reload). For a
|
||||
logging destination the in-memory ``credential_info`` is merged
|
||||
subfield-by-subfield via ``_merge_credential_info`` so a partial patch
|
||||
can't clobber stored ``access`` subfields it didn't touch; a provider
|
||||
credential keeps the base replace semantics.
|
||||
"""
|
||||
existing_in_memory: CredentialItem | None = None
|
||||
for cred in litellm.credential_list:
|
||||
|
|
@ -402,7 +408,10 @@ def _sync_in_memory_credential(
|
|||
in_memory_values.update(patch.credential_values)
|
||||
in_memory_info = dict(existing_in_memory.credential_info or {})
|
||||
if patch.credential_info:
|
||||
_merge_credential_info(in_memory_info, patch.credential_info)
|
||||
if is_logging_credential(existing_in_memory.credential_info):
|
||||
_merge_credential_info(in_memory_info, patch.credential_info)
|
||||
else:
|
||||
in_memory_info = dict(patch.credential_info)
|
||||
updated_in_memory = CredentialItem(
|
||||
credential_name=merged.credential_name,
|
||||
credential_values=in_memory_values,
|
||||
|
|
|
|||
|
|
@ -4805,15 +4805,12 @@ async def regenerate_key_fn( # noqa: C901 # single endpoint handling many opti
|
|||
if data is not None and (data.access_group_ids or data.object_permission is not None):
|
||||
regenerate_team_table: LiteLLM_TeamTableCachedObj | None = None
|
||||
if _key_in_db.team_id is not None:
|
||||
try:
|
||||
regenerate_team_table = await get_team_object(
|
||||
team_id=_key_in_db.team_id,
|
||||
prisma_client=prisma_client,
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
check_db_only=True,
|
||||
)
|
||||
except HTTPException:
|
||||
regenerate_team_table = None
|
||||
regenerate_team_table = await get_team_object(
|
||||
team_id=_key_in_db.team_id,
|
||||
prisma_client=prisma_client,
|
||||
user_api_key_cache=user_api_key_cache,
|
||||
check_db_only=True,
|
||||
)
|
||||
_regen_is_proxy_admin = user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN.value
|
||||
TeamMemberPermissionChecks.enforce_member_can_assign_access_groups(
|
||||
user_api_key_dict=user_api_key_dict,
|
||||
|
|
|
|||
|
|
@ -10,12 +10,20 @@ scope is the given set of team ids and org ids. The resolver passes a
|
|||
one-element scope built with ``identity_scope``.
|
||||
"""
|
||||
|
||||
from pydantic import ValidationError
|
||||
from pydantic import BaseModel, ConfigDict, ValidationError
|
||||
|
||||
import litellm
|
||||
from litellm.models.credentials import CredentialAccess, CredentialInfo
|
||||
|
||||
|
||||
class _LoggingDestinationTag(BaseModel):
|
||||
"""Lenient read of just the ``credential_type`` tag, ignoring the rest of
|
||||
``credential_info`` (including a possibly-malformed ``access``)."""
|
||||
|
||||
model_config = ConfigDict(extra="ignore")
|
||||
credential_type: str | None = None
|
||||
|
||||
|
||||
def parse_credential_info(raw: object) -> CredentialInfo | None:
|
||||
"""Parse stored ``credential_info`` into the typed model, or ``None`` when it is
|
||||
absent or malformed.
|
||||
|
|
@ -32,6 +40,23 @@ def parse_credential_info(raw: object) -> CredentialInfo | None:
|
|||
return None
|
||||
|
||||
|
||||
def is_logging_credential(raw: object) -> bool:
|
||||
"""Whether ``credential_info`` is tagged as an admin-owned logging destination.
|
||||
|
||||
The ``access`` shape validation and the ``credential_info`` subfield merge are
|
||||
scoped to these; a provider credential is left on its base replace-and-accept path.
|
||||
|
||||
This keys off the ``credential_type`` tag alone and does not parse ``access``: a
|
||||
destination carrying a malformed ``access`` is still a logging destination, and the
|
||||
point of the gate is to route it into ``validate_credential_access`` so that bad
|
||||
``access`` is rejected rather than stored.
|
||||
"""
|
||||
try:
|
||||
return _LoggingDestinationTag.model_validate(raw).credential_type == "logging"
|
||||
except ValidationError:
|
||||
return False
|
||||
|
||||
|
||||
def identity_scope(team_id: str | None, org_id: str | None) -> tuple[frozenset[str], frozenset[str]]:
|
||||
"""A single request identity's scope as ``(team_ids, org_ids)`` for
|
||||
``access_grants``."""
|
||||
|
|
|
|||
|
|
@ -97,6 +97,80 @@ def test_update_db_credential_preserves_untouched_access_subfields():
|
|||
}
|
||||
|
||||
|
||||
# --- provider credentials keep base replace semantics (merge is logging-only) ---
|
||||
|
||||
def test_update_db_credential_replaces_info_for_provider_credential():
|
||||
"""The subfield merge is scoped to logging destinations. A provider credential
|
||||
patch replaces credential_info wholesale (base behavior): omitted keys drop, so a
|
||||
partial patch is a full replace, not a merge -- merging non-destination creds would
|
||||
silently resurrect stale provider metadata the caller meant to remove."""
|
||||
from litellm.proxy.credential_endpoints.endpoints import update_db_credential
|
||||
|
||||
db = CredentialItem(
|
||||
credential_name="openai",
|
||||
credential_values={},
|
||||
credential_info={"custom_llm_provider": "openai", "stale": "keepout"},
|
||||
)
|
||||
patch = CredentialItem(
|
||||
credential_name="openai",
|
||||
credential_values={},
|
||||
credential_info={"custom_llm_provider": "azure"},
|
||||
)
|
||||
|
||||
merged = update_db_credential(db, patch)
|
||||
|
||||
assert merged.credential_info == {"custom_llm_provider": "azure"}
|
||||
|
||||
|
||||
# --- access-shape validation is scoped to logging destinations ---------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_credential_validates_access_only_for_logging(monkeypatch):
|
||||
"""validate_credential_access runs for a logging destination but never for a
|
||||
provider credential. A provider cred carrying an unrelated `access` key must not be
|
||||
rejected by the destination access-shape validator (that would 400 a valid
|
||||
provider credential the validator was never meant to see)."""
|
||||
import litellm.proxy.proxy_server as proxy_server
|
||||
from litellm.types.utils import CreateCredentialItem
|
||||
|
||||
monkeypatch.setattr(proxy_server, "prisma_client", None, raising=False)
|
||||
|
||||
class _Validated(Exception):
|
||||
pass
|
||||
|
||||
def _spy(_info):
|
||||
raise _Validated()
|
||||
|
||||
monkeypatch.setattr(endpoints, "validate_credential_access", _spy)
|
||||
|
||||
async def _create(credential):
|
||||
return await endpoints.create_credential(
|
||||
request=MagicMock(),
|
||||
fastapi_response=MagicMock(),
|
||||
credential=credential,
|
||||
user_api_key_dict=_admin(),
|
||||
)
|
||||
|
||||
logging_cred = CreateCredentialItem(
|
||||
credential_name="dest",
|
||||
credential_values={"otel_endpoint": "http://collector:4318"},
|
||||
credential_info={"credential_type": "logging", "description": "generic", "access": {"global": True}},
|
||||
)
|
||||
with pytest.raises(_Validated):
|
||||
await _create(logging_cred)
|
||||
|
||||
provider_cred = CreateCredentialItem(
|
||||
credential_name="openai",
|
||||
credential_values={"api_key": "sk"},
|
||||
credential_info={"custom_llm_provider": "openai", "access": {"bogus": True}},
|
||||
)
|
||||
# Validator is skipped; the handler proceeds and fails on the (None) prisma client,
|
||||
# a 500 -- never the _Validated sentinel.
|
||||
with pytest.raises(Exception) as excinfo:
|
||||
await _create(provider_cred)
|
||||
assert not isinstance(excinfo.value, _Validated)
|
||||
|
||||
|
||||
# --- PATCH routing regression ------------------------------------------------
|
||||
|
||||
def test_patch_credentials_route_targets_update_credential():
|
||||
|
|
|
|||
|
|
@ -13162,6 +13162,87 @@ async def test_regenerate_applies_normalized_mcp_object_permission():
|
|||
assert regenerated_data.object_permission.mcp_servers == ["server-id"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_regenerate_propagates_team_not_found():
|
||||
"""Regenerating a key whose team no longer exists must surface the team-lookup
|
||||
404 rather than swallowing it and regenerating against a dangling team. The
|
||||
access_group_ids/object_permission gate depends on the resolved team, so a missing
|
||||
team must abort the regenerate (matching /key/generate and /key/update) instead of
|
||||
silently continuing with team_table=None."""
|
||||
from litellm.proxy._types import RegenerateKeyRequest
|
||||
from litellm.proxy.management_endpoints.key_management_endpoints import (
|
||||
regenerate_key_fn,
|
||||
)
|
||||
|
||||
data = RegenerateKeyRequest(key="sk-old", access_group_ids=["ag-1"])
|
||||
existing_key = LiteLLM_VerificationToken(
|
||||
token="abc123",
|
||||
user_id="user-1",
|
||||
models=["gpt-4"],
|
||||
team_id="dangling-team",
|
||||
max_budget=None,
|
||||
tags=None,
|
||||
)
|
||||
mock_prisma_client = AsyncMock()
|
||||
mock_repo = MagicMock()
|
||||
mock_repo.table.find_unique = AsyncMock(return_value=existing_key)
|
||||
execute_mock = AsyncMock(return_value=MagicMock())
|
||||
enforce_mock = MagicMock()
|
||||
|
||||
async def _raise_team_not_found(*args, **kwargs):
|
||||
raise HTTPException(status_code=404, detail={"error": "Team not found"})
|
||||
|
||||
with (
|
||||
patch("litellm.proxy.proxy_server.premium_user", True),
|
||||
patch("litellm.proxy.proxy_server.master_key", None),
|
||||
patch("litellm.proxy.proxy_server.prisma_client", mock_prisma_client),
|
||||
patch("litellm.proxy.proxy_server.proxy_logging_obj", MagicMock()),
|
||||
patch("litellm.proxy.proxy_server.user_api_key_cache", MagicMock()),
|
||||
patch("litellm.proxy.proxy_server.hash_token", lambda token: "hashed-old"),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints.VerificationTokenRepository",
|
||||
return_value=mock_repo,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints.TeamMemberPermissionChecks.can_team_member_execute_key_management_endpoint",
|
||||
new_callable=AsyncMock,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints.TeamMemberPermissionChecks.enforce_member_can_assign_access_groups",
|
||||
enforce_mock,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints.can_modify_verification_token",
|
||||
new_callable=AsyncMock,
|
||||
return_value=True,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints.get_team_object",
|
||||
_raise_team_not_found,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.management_endpoints.key_management_endpoints._execute_virtual_key_regeneration",
|
||||
execute_mock,
|
||||
),
|
||||
):
|
||||
with pytest.raises((HTTPException, ProxyException)) as exc_info:
|
||||
await regenerate_key_fn(
|
||||
key="sk-old",
|
||||
data=data,
|
||||
user_api_key_dict=UserAPIKeyAuth(
|
||||
user_role=LitellmUserRoles.PROXY_ADMIN.value,
|
||||
api_key="sk-admin",
|
||||
user_id="admin",
|
||||
),
|
||||
)
|
||||
|
||||
err = exc_info.value
|
||||
code = getattr(err, "status_code", None) or getattr(err, "code", None)
|
||||
assert str(code) == "404"
|
||||
enforce_mock.assert_not_called()
|
||||
execute_mock.assert_not_awaited()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Regression tests for GHSA-q775-qw9r-2r4g: budget escalation via key/generate
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -15,11 +15,33 @@ from litellm.models.credentials import CredentialAccess, CredentialInfo, Credent
|
|||
from litellm.proxy.management_endpoints.logging_exporter_access import (
|
||||
access_grants,
|
||||
identity_scope,
|
||||
is_logging_credential,
|
||||
parse_credential_info,
|
||||
resolved_logging_exporter_names,
|
||||
)
|
||||
|
||||
|
||||
# --- is_logging_credential: the access-validation + merge gate ---------------
|
||||
|
||||
|
||||
def test_is_logging_credential_true_for_logging_type():
|
||||
assert is_logging_credential({"credential_type": "logging", "description": "arize"}) is True
|
||||
|
||||
|
||||
def test_is_logging_credential_true_even_with_malformed_access():
|
||||
"""A destination carrying an invalid access shape is still a logging destination.
|
||||
The gate must route it into validate_credential_access (which rejects it), not skip
|
||||
validation because the strict access model can't parse it."""
|
||||
assert is_logging_credential({"credential_type": "logging", "access": {"nonsense_field": True}}) is True
|
||||
|
||||
|
||||
def test_is_logging_credential_false_for_provider_and_malformed():
|
||||
assert is_logging_credential({"custom_llm_provider": "openai"}) is False
|
||||
assert is_logging_credential({"custom_llm_provider": "openai", "access": {"global": True}}) is False
|
||||
assert is_logging_credential(None) is False
|
||||
assert is_logging_credential("nope") is False
|
||||
|
||||
|
||||
# --- parse_credential_info: fail closed on bad input -----------------------
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue