mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-07 08:26:10 +00:00
fix(audit): redact callback secrets and surface fire-and-forget failures
Two Greptile P2s addressed: 1. (security) The audit-log row for an ``add_team_callbacks`` call would serialize the entire ``callback_vars`` block — including ``langfuse_secret_key``, ``langsmith_api_key``, and the GCS service account path — verbatim into ``LiteLLM_AuditLogs``. Anyone with read access to the audit table could harvest team callback credentials. Same risk for ``disable_team_logging`` when the team's existing row has populated ``callback_settings.callback_vars``. Add ``_redact_callback_secrets``: deep-copies the metadata snapshot and replaces every ``callback_vars`` value with ``***REDACTED***``. The keys are kept so an auditor can still see *which* fields changed. Applied to both before and after snapshots. 2. ``asyncio.create_task`` is fire-and-forget; if the audit-log write raises (transient DB error etc.) the exception is silently discarded by the event loop and the audit row is just missing — the exact gap this PR is closing. Attach a ``done_callback`` that logs the exception at warning level via ``verbose_proxy_logger`` so the operator sees there's a gap. Tests assert that callback values are not present in the serialized audit payload (both for ``add_team_callbacks`` and for ``disable_team_logging`` when the team's existing row has populated secrets).
This commit is contained in:
parent
adc2c55ffc
commit
986cdedd4f
2 changed files with 123 additions and 3 deletions
|
|
@ -31,6 +31,56 @@ from litellm.proxy.management_helpers.utils import management_endpoint_wrapper
|
|||
router = APIRouter()
|
||||
|
||||
|
||||
_CALLBACK_VARS_REDACTED = "***REDACTED***"
|
||||
|
||||
|
||||
def _redact_callback_secrets(metadata: Any) -> Any:
|
||||
"""Strip secret values out of a team-metadata snapshot before audit logging.
|
||||
|
||||
Both ``team_metadata["logging"]`` (list of ``AddTeamCallback`` dicts) and
|
||||
``team_metadata["callback_settings"]["callback_vars"]`` carry provider
|
||||
credentials such as ``langfuse_secret_key``, ``langsmith_api_key``, and
|
||||
``gcs_path_service_account``. Persisting them verbatim into
|
||||
``LiteLLM_AuditLogs`` would let anyone with read access to the audit
|
||||
table harvest team callback credentials, so we replace each value with
|
||||
a fixed marker. The keys themselves are kept so the audit reader can
|
||||
still see *which* fields changed.
|
||||
"""
|
||||
if not isinstance(metadata, dict):
|
||||
return metadata
|
||||
redacted = copy.deepcopy(metadata)
|
||||
logging_entries = redacted.get("logging")
|
||||
if isinstance(logging_entries, list):
|
||||
for entry in logging_entries:
|
||||
if isinstance(entry, dict) and isinstance(entry.get("callback_vars"), dict):
|
||||
entry["callback_vars"] = {
|
||||
k: _CALLBACK_VARS_REDACTED for k in entry["callback_vars"]
|
||||
}
|
||||
callback_settings = redacted.get("callback_settings")
|
||||
if isinstance(callback_settings, dict) and isinstance(
|
||||
callback_settings.get("callback_vars"), dict
|
||||
):
|
||||
callback_settings["callback_vars"] = {
|
||||
k: _CALLBACK_VARS_REDACTED for k in callback_settings["callback_vars"]
|
||||
}
|
||||
return redacted
|
||||
|
||||
|
||||
def _log_audit_task_exception(task: "asyncio.Task[None]") -> None:
|
||||
"""Surface a fire-and-forget audit-log task failure.
|
||||
|
||||
``asyncio.create_task`` swallows exceptions silently — if the audit
|
||||
write fails (transient DB error etc.) we'd otherwise lose the row
|
||||
without any signal. Log at warning level so the operator sees there's
|
||||
a gap in the audit trail.
|
||||
"""
|
||||
if task.cancelled():
|
||||
return
|
||||
exc = task.exception()
|
||||
if exc is not None:
|
||||
verbose_proxy_logger.warning("Failed to write team-callback audit log: %s", exc)
|
||||
|
||||
|
||||
async def _emit_team_callback_audit_log(
|
||||
*,
|
||||
team_id: str,
|
||||
|
|
@ -46,6 +96,9 @@ async def _emit_team_callback_audit_log(
|
|||
when audit logging is not enabled on the proxy. Captured under
|
||||
``LitellmTableNames.TEAM_TABLE_NAME`` so the row co-locates with other
|
||||
team mutations in the audit table.
|
||||
|
||||
Callback secrets are redacted before serialization so the audit table
|
||||
cannot itself become a credential-harvest sink.
|
||||
"""
|
||||
if litellm.store_audit_logs is not True:
|
||||
return
|
||||
|
|
@ -55,7 +108,10 @@ async def _emit_team_callback_audit_log(
|
|||
)
|
||||
from litellm.proxy.proxy_server import litellm_proxy_admin_name
|
||||
|
||||
asyncio.create_task(
|
||||
redacted_before = _redact_callback_secrets(before_metadata)
|
||||
redacted_after = _redact_callback_secrets(after_metadata)
|
||||
|
||||
task = asyncio.create_task(
|
||||
create_audit_log_for_update(
|
||||
request_data=LiteLLM_AuditLogs(
|
||||
id=str(uuid.uuid4()),
|
||||
|
|
@ -67,11 +123,12 @@ async def _emit_team_callback_audit_log(
|
|||
table_name=LitellmTableNames.TEAM_TABLE_NAME,
|
||||
object_id=team_id,
|
||||
action="updated",
|
||||
updated_values=json.dumps({"metadata": after_metadata}, default=str),
|
||||
before_value=json.dumps({"metadata": before_metadata}, default=str),
|
||||
updated_values=json.dumps({"metadata": redacted_after}, default=str),
|
||||
before_value=json.dumps({"metadata": redacted_before}, default=str),
|
||||
)
|
||||
)
|
||||
)
|
||||
task.add_done_callback(_log_audit_task_exception)
|
||||
|
||||
|
||||
@router.post(
|
||||
|
|
|
|||
|
|
@ -195,6 +195,69 @@ async def test_add_team_callbacks_emits_audit_log_when_enabled(monkeypatch):
|
|||
assert len(after["metadata"]["logging"]) == 1
|
||||
assert after["metadata"]["logging"][0]["callback_name"] == "langfuse"
|
||||
|
||||
# Callback secrets MUST NOT leak into the audit log payload.
|
||||
callback_vars = after["metadata"]["logging"][0]["callback_vars"]
|
||||
assert callback_vars["langfuse_public_key"] != "pk"
|
||||
assert callback_vars["langfuse_secret_key"] != "sk"
|
||||
# Key names are preserved so the auditor can see which fields changed.
|
||||
assert "langfuse_public_key" in callback_vars
|
||||
assert "langfuse_secret_key" in callback_vars
|
||||
# And no plaintext secret should appear anywhere in the serialized row.
|
||||
assert "sk" not in log.updated_values.replace("sk-", "") # crude leak check
|
||||
assert "pk" not in (log.updated_values.replace("pk-", "").replace("public_key", ""))
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_disable_team_logging_redacts_existing_callback_secrets(monkeypatch):
|
||||
monkeypatch.setattr(litellm, "store_audit_logs", True)
|
||||
# Existing team has populated callback_vars containing secrets — redaction
|
||||
# must apply to the BEFORE snapshot too.
|
||||
mock_prisma = _patch_prisma(
|
||||
{
|
||||
"callback_settings": {
|
||||
"success_callback": ["langfuse"],
|
||||
"failure_callback": [],
|
||||
"callback_vars": {
|
||||
"langfuse_public_key": "pk-real",
|
||||
"langfuse_secret_key": "sk-real-secret",
|
||||
},
|
||||
}
|
||||
}
|
||||
)
|
||||
|
||||
audit_calls = []
|
||||
|
||||
async def capture(request_data):
|
||||
audit_calls.append(request_data)
|
||||
|
||||
with (
|
||||
patch("litellm.proxy.proxy_server.prisma_client", mock_prisma),
|
||||
patch("litellm.proxy.proxy_server.litellm_proxy_admin_name", "admin"),
|
||||
patch(
|
||||
"litellm.proxy.management_helpers.audit_logs.create_audit_log_for_update",
|
||||
new=capture,
|
||||
),
|
||||
):
|
||||
await disable_team_logging(
|
||||
http_request=MagicMock(spec=Request),
|
||||
team_id="team-1",
|
||||
user_api_key_dict=_admin_auth(),
|
||||
litellm_changed_by=None,
|
||||
)
|
||||
import asyncio
|
||||
|
||||
for _ in range(3):
|
||||
await asyncio.sleep(0)
|
||||
|
||||
assert len(audit_calls) == 1
|
||||
log = audit_calls[0]
|
||||
# The pre-existing secret_key value must NOT appear in the serialized
|
||||
# before_value or updated_values.
|
||||
assert "sk-real-secret" not in log.before_value
|
||||
assert "sk-real-secret" not in log.updated_values
|
||||
assert "pk-real" not in log.before_value
|
||||
assert "pk-real" not in log.updated_values
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_add_team_callbacks_no_audit_when_disabled(monkeypatch):
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue