mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-07 02:59:05 +00:00
fix(guardrails): Alice WonderFence post_call never borrows a sibling guardrail's credentials
The stash recovery fell back to any sibling alice_wonderfence instance's stash when the current instance had none. With two instances on one request, a strict instance (allow_request_metadata_override=False) could inherit a permissive sibling's caller-supplied request-body credentials, scanning under credentials it would itself reject. Remove the fallback and fail closed when this instance's own stash is absent. The stash is now stored under a per-guardrail attribute (_alice_wonderfence_resolved__<name>) rather than a shared dict keyed by name, so the isolation is structural: there is no sibling slot to read. This only affects multi-instance during_call-only configs (pre_call stashes each instance's own, and key/team credentials re-resolve in post_call without a stash), so common single-instance and pre_call configs are unchanged. Regression tests: a strict reader with a permissive writer sibling fails closed instead of borrowing, and recover_resolved returns None for a name that never stashed even when a sibling did. Both fail on the prior implementation.
This commit is contained in:
parent
0c0bba57c7
commit
c0b05b803a
3 changed files with 63 additions and 79 deletions
|
|
@ -20,8 +20,6 @@ gone — see ``stash_resolved`` for the full rationale.
|
|||
|
||||
from typing import TYPE_CHECKING, Literal, Optional, Tuple
|
||||
|
||||
from litellm._logging import verbose_proxy_logger
|
||||
|
||||
from .exceptions import WonderFenceMissingSecrets
|
||||
|
||||
if TYPE_CHECKING:
|
||||
|
|
@ -30,18 +28,22 @@ if TYPE_CHECKING:
|
|||
)
|
||||
|
||||
|
||||
logger = verbose_proxy_logger.getChild("alice_wonderfence")
|
||||
# Prefix for the per-guardrail attribute on the request-scoped logging_obj where
|
||||
# the resolved (api_key, app_id) is stashed so post_call can recover it. The
|
||||
# guardrail name is baked into the attribute so each instance has a physically
|
||||
# separate slot — one instance can never read another's credentials.
|
||||
#
|
||||
# These are private instance attributes, NOT model_call_details keys, because
|
||||
# model_call_details is forwarded verbatim as ``kwargs`` to every logging
|
||||
# callback / exporter (litellm_logging.py) — stashing the resolved WonderFence
|
||||
# api_key there would leak a tenant secret into logs. A private attribute on the
|
||||
# same object gives the identical cross-hook / cross-task visibility (see
|
||||
# stash_resolved) without entering the logged payload.
|
||||
_STASH_ATTR_PREFIX = "_alice_wonderfence_resolved__"
|
||||
|
||||
|
||||
# Attribute on the request-scoped logging_obj where the resolved
|
||||
# (api_key, app_id) is stashed so post_call can recover it. This is a private
|
||||
# instance attribute, NOT a model_call_details key, because model_call_details
|
||||
# is forwarded verbatim as ``kwargs`` to every logging callback / exporter
|
||||
# (litellm_logging.py) — stashing the resolved WonderFence api_key there would
|
||||
# leak a tenant secret into logs. A private attribute on the same object gives
|
||||
# the identical cross-hook / cross-task visibility (see stash_resolved) without
|
||||
# entering the logged payload.
|
||||
_STASH_ATTR = "_alice_wonderfence_resolved"
|
||||
def _stash_attr(guardrail_name: str) -> str:
|
||||
return _STASH_ATTR_PREFIX + guardrail_name
|
||||
|
||||
|
||||
def get_metadata(request_data: dict) -> dict:
|
||||
|
|
@ -175,58 +177,31 @@ def stash_resolved(
|
|||
and post_call), so mutations to it are visible regardless of task
|
||||
boundary.
|
||||
|
||||
Keyed by ``guardrail_name`` so multiple alice_wonderfence instances
|
||||
configured on the same proxy don't collide.
|
||||
The attribute is per-guardrail (see ``_stash_attr``) so multiple
|
||||
alice_wonderfence instances on the same request each get an isolated slot.
|
||||
"""
|
||||
if logging_obj is None:
|
||||
return
|
||||
container = getattr(logging_obj, _STASH_ATTR, None)
|
||||
if not isinstance(container, dict):
|
||||
container = {}
|
||||
setattr(logging_obj, _STASH_ATTR, container)
|
||||
container[guardrail_name] = (api_key, app_id)
|
||||
setattr(logging_obj, _stash_attr(guardrail_name), (api_key, app_id))
|
||||
|
||||
|
||||
def recover_resolved(
|
||||
logging_obj: Optional["LiteLLMLoggingObj"], guardrail_name: str
|
||||
) -> Optional[Tuple[str, str]]:
|
||||
"""Look up (api_key, app_id) stashed earlier in this request.
|
||||
"""Look up the (api_key, app_id) this guardrail stashed earlier in this
|
||||
request, or ``None``.
|
||||
|
||||
Prefer this instance's own stash. If absent, fall back to any sibling
|
||||
alice_wonderfence instance's stash on the same request.
|
||||
|
||||
Why the sibling fallback exists:
|
||||
LiteLLM serializes parallel during_call hooks through a single shared
|
||||
slot ``data["guardrail_to_apply"]`` (``proxy/utils.py:1483``). That
|
||||
slot is overwritten in a loop *before* any gather() task runs, so
|
||||
only the last-registered guardrail callback actually executes its
|
||||
during_call — the others see ``None`` and bail. Post_call, by
|
||||
contrast, iterates sequentially and *all* registered guardrails run.
|
||||
Net effect when a single request lists multiple alice_wonderfence
|
||||
guardrails (e.g. ``guardrails: ["wonderfence", "alice-wonderfence"]``
|
||||
against a config that defines both): only one writes a stash, but
|
||||
every one tries to read one in post_call. Since every
|
||||
alice_wonderfence instance resolves api_key / app_id from the same
|
||||
request-body / key / team metadata fields, sibling stashes carry
|
||||
equivalent values.
|
||||
Returns only this instance's own stash. It deliberately does NOT fall back
|
||||
to another alice_wonderfence instance's stash: a sibling may have resolved
|
||||
under a different policy (e.g. ``allow_request_metadata_override=True``,
|
||||
carrying caller-supplied request-body credentials) that a stricter instance
|
||||
must not inherit. When this instance has no own stash, the caller fails
|
||||
closed rather than borrowing.
|
||||
"""
|
||||
if logging_obj is None:
|
||||
return None
|
||||
container = getattr(logging_obj, _STASH_ATTR, None)
|
||||
if not isinstance(container, dict) or not container:
|
||||
return None
|
||||
own = container.get(guardrail_name)
|
||||
if own is not None:
|
||||
return own
|
||||
sibling_name, sibling_value = next(iter(container.items()))
|
||||
logger.warning(
|
||||
"Alice WonderFence: post_call recovering stash from sibling "
|
||||
"guardrail '%s' (own name '%s' not in stash). See recover_resolved "
|
||||
"docstring for why.",
|
||||
sibling_name,
|
||||
guardrail_name,
|
||||
)
|
||||
return sibling_value
|
||||
value = getattr(logging_obj, _stash_attr(guardrail_name), None)
|
||||
return value if isinstance(value, tuple) else None
|
||||
|
||||
|
||||
def resolve_credentials(
|
||||
|
|
|
|||
|
|
@ -310,22 +310,25 @@ def test_responses_route_admin_pin_beats_caller_metadata_api_key():
|
|||
# --------------- stash storage: secret must not leak to logged payload ---------------
|
||||
|
||||
|
||||
def test_logging_obj_allows_private_stash_attr_off_model_call_details(make_logging_obj):
|
||||
"""Guard: the real LiteLLMLoggingObj must accept a private attribute that is
|
||||
NOT part of model_call_details. Fails if Logging becomes slotted/pydantic or
|
||||
the stash is moved back into the logged dict."""
|
||||
obj = make_logging_obj()
|
||||
obj._alice_wonderfence_resolved = {"g": ("k", "a")}
|
||||
assert obj._alice_wonderfence_resolved == {"g": ("k", "a")}
|
||||
assert "_alice_wonderfence_resolved" not in obj.model_call_details
|
||||
|
||||
|
||||
def test_stash_round_trips_on_real_logging_obj(make_logging_obj):
|
||||
"""Guard: the real LiteLLMLoggingObj must accept the per-guardrail stash
|
||||
attribute. Fails if Logging becomes slotted/pydantic."""
|
||||
obj = make_logging_obj()
|
||||
stash_resolved(obj, "guard-1", "wf-key-abc", "app-1")
|
||||
assert recover_resolved(obj, "guard-1") == ("wf-key-abc", "app-1")
|
||||
|
||||
|
||||
def test_recover_does_not_borrow_a_sibling_guardrails_stash(make_logging_obj):
|
||||
"""Each guardrail's stash is isolated: a name that never stashed recovers
|
||||
None even when a sibling stashed on the same logging_obj. This is what makes
|
||||
a stricter instance fail closed instead of inheriting a permissive sibling's
|
||||
caller-supplied credentials."""
|
||||
obj = make_logging_obj()
|
||||
stash_resolved(obj, "writer", "writer-key", "writer-app")
|
||||
assert recover_resolved(obj, "reader") is None
|
||||
assert recover_resolved(obj, "writer") == ("writer-key", "writer-app")
|
||||
|
||||
|
||||
def test_stashed_api_key_not_present_in_model_call_details(make_logging_obj):
|
||||
"""Regression: model_call_details is forwarded verbatim as kwargs to logging
|
||||
callbacks/exporters, so a resolved tenant api_key stashed there leaks. The
|
||||
|
|
|
|||
|
|
@ -122,13 +122,16 @@ async def test_post_call_without_prior_stash_raises(make_guardrail, make_logging
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_post_call_recovers_via_sibling_stash(
|
||||
async def test_post_call_does_not_borrow_sibling_stash(
|
||||
make_guardrail, make_request_data, make_logging_obj
|
||||
):
|
||||
"""When two alice_wonderfence instances are listed in one request's
|
||||
``guardrails`` array, LiteLLM only invokes one's during_call — but every
|
||||
instance runs post_call. The instance whose during_call did NOT fire
|
||||
must recover the stash written by the sibling that did."""
|
||||
"""A stricter instance must NOT inherit a sibling's stashed credentials.
|
||||
|
||||
Exploit being closed: a permissive writer (allow_request_metadata_override
|
||||
=True) stashes caller-supplied request-body credentials; a stricter reader
|
||||
(allow_request_metadata_override=False) that has no own stash must fail
|
||||
closed in post_call rather than scan with the writer's caller-controlled
|
||||
creds."""
|
||||
g_writer, c_writer = make_guardrail(
|
||||
guardrail_name="writer",
|
||||
allow_request_metadata_override=True,
|
||||
|
|
@ -136,7 +139,7 @@ async def test_post_call_recovers_via_sibling_stash(
|
|||
g_writer._client_cache["default-api-key"] = c_writer
|
||||
g_reader, c_reader = make_guardrail(
|
||||
guardrail_name="reader",
|
||||
allow_request_metadata_override=True,
|
||||
allow_request_metadata_override=False,
|
||||
)
|
||||
g_reader._client_cache["default-api-key"] = c_reader
|
||||
for c in (c_writer, c_reader):
|
||||
|
|
@ -149,25 +152,28 @@ async def test_post_call_recovers_via_sibling_stash(
|
|||
|
||||
logging_obj = make_logging_obj()
|
||||
|
||||
# Only the writer's during_call fires (simulating LiteLLM's
|
||||
# data["guardrail_to_apply"] last-write-wins behavior).
|
||||
# Writer stashes caller-supplied request-body app_id (override allowed).
|
||||
await g_writer.apply_guardrail(
|
||||
inputs={"texts": ["hi"]},
|
||||
request_data=make_request_data(
|
||||
metadata={"alice_wonderfence_app_id": "shared-app"}
|
||||
metadata={"alice_wonderfence_app_id": "caller-supplied-app"}
|
||||
),
|
||||
input_type="request",
|
||||
logging_obj=logging_obj,
|
||||
)
|
||||
|
||||
# Reader's post_call: own name not in stash, must fall back to writer's.
|
||||
await g_reader.apply_guardrail(
|
||||
inputs={"texts": ["resp"]},
|
||||
request_data={"model": "gpt-4", "metadata": {}},
|
||||
input_type="response",
|
||||
logging_obj=logging_obj,
|
||||
)
|
||||
assert c_reader.evaluate_response.call_args.kwargs["app_id"] == "shared-app"
|
||||
# Reader's post_call: no own stash, request_data resolves nothing, and it
|
||||
# must NOT borrow the writer's stash -> fail closed.
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await g_reader.apply_guardrail(
|
||||
inputs={"texts": ["resp"]},
|
||||
request_data={"model": "gpt-4", "metadata": {}},
|
||||
input_type="response",
|
||||
logging_obj=logging_obj,
|
||||
)
|
||||
assert exc.value.status_code == 500
|
||||
assert "alice_wonderfence_app_id" in exc.value.detail["exception"]
|
||||
c_reader.evaluate_response.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue