mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(litellm): don't forward a caller key the target cannot accept (#2883)
Split out of #2852 at review request: that PR is bounded upstream calls plus measured hot-path costs, and this is an authentication/routing change that belongs on its own scope. #2852 now carries only the timeout work. ## The bug A routing extension can rewrite the model across families mid-request (`claude-opus-5` → `gpt-5-mini`). The caller's key does not travel with that rewrite, so the proxy forwards `sk-ant-...` to OpenAI and earns a guaranteed 401. Downstream that is indistinguishable from *"the cheap model failed the task"* — it scores as a quality regression against the router, not as a bug. Dropping the `api_key` kwarg instead lets litellm fall back to the target provider's own env credential, which is the only key that can work. ## Why this cut is different from the one that was rejected The first version returned `not provider.startswith("anthropic")`, so **any** non-`sk-ant-` credential was dropped against an Anthropic-class target — a plain Bearer token against an Anthropic-compatible or custom gateway lost its key and fell back to an env credential that may not exist. That direction is the dangerous one. A false refusal breaks a deployment that was working; a missed refusal just leaves today's 401. So this refuses on **positive evidence only**: | credential | target | forwarded? | |---|---|---| | `sk-ant-…` | `openai` / `azure` / `gemini` | **no** — cannot possibly authenticate | | `sk-ant-…` | anthropic | yes | | `sk-ant-…` | unrecognised / unclassifiable model | yes — pass-through | | anything else | anything | yes — pass-through, unchanged | `sk-ant-` is Anthropic's documented vendor-specific prefix, which is what makes it classifiable. `sk-` is not: a dozen vendors mint that shape. Everything the string cannot settle keeps main's behaviour. The reject list is explicit rather than inverted (`not anthropic`) because an unrecognised provider is usually a compatible or self-hosted gateway. Marked in the code as a hand-kept tuple with the registry-lookup upgrade path noted. Bedrock / Vertex / SageMaker are unaffected — all four dispatch sites already skip credential forwarding for them entirely (env-based auth). ## Verification `tests/test_litellm_caller_key.py`, 12 cases — the refusal, the Anthropic target, the unknown provider, `get_llm_provider` raising, and each unclassifiable credential shape asserted against **both** target families. Those last ones fail against the rejected version. Applied at all four dispatch sites (Anthropic non-stream/stream, OpenAI non-stream/stream). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
f624d3a00a
commit
2f2950a626
2 changed files with 140 additions and 16 deletions
|
|
@ -447,6 +447,47 @@ def _upstream_timeout() -> float:
|
||||||
return v if v > 0 else DEFAULT_UPSTREAM_TIMEOUT
|
return v if v > 0 else DEFAULT_UPSTREAM_TIMEOUT
|
||||||
|
|
||||||
|
|
||||||
|
# Providers that cannot possibly accept an Anthropic `sk-ant-` credential.
|
||||||
|
#
|
||||||
|
# Explicit, rather than the inverse "anything not Anthropic": an unrecognised
|
||||||
|
# provider is usually a compatible or self-hosted gateway, and guessing wrong
|
||||||
|
# there drops a key that WAS working. Bedrock/Vertex are absent because the
|
||||||
|
# dispatch sites already skip them entirely (env-based auth).
|
||||||
|
#
|
||||||
|
# ponytail: a hand-kept tuple; grow it as targets are confirmed. A registry
|
||||||
|
# lookup would be the upgrade if this ever outgrows a handful of entries.
|
||||||
|
_REJECTS_ANTHROPIC_KEY = ("openai", "azure", "gemini")
|
||||||
|
|
||||||
|
|
||||||
|
def _caller_key_travels_to(model: str, key: str) -> bool:
|
||||||
|
"""Can this inbound credential authenticate the provider we are about to call?
|
||||||
|
|
||||||
|
The caller authenticates to the PROXY. A routing extension may then rewrite
|
||||||
|
the model across families mid-request (claude-opus-5 -> gpt-5-mini), and the
|
||||||
|
caller's key does not travel with that rewrite: we forward `sk-ant-...` to
|
||||||
|
OpenAI and earn a guaranteed 401, which reads downstream as "the cheap model
|
||||||
|
failed the task" rather than as the routing bug it is.
|
||||||
|
|
||||||
|
Only an unambiguous mismatch is refused. `sk-ant-` is Anthropic's documented
|
||||||
|
vendor-specific prefix, so it cannot authenticate one of the providers above.
|
||||||
|
Every other credential -- a plain Bearer token, an OpenAI-style `sk-` that a
|
||||||
|
dozen vendors also mint, anything aimed at a compatible or custom gateway --
|
||||||
|
is unclassifiable from the string alone and keeps the pass-through.
|
||||||
|
|
||||||
|
Returning False drops the api_key kwarg, so litellm falls back to the target
|
||||||
|
provider's own env credential: the only key that can work.
|
||||||
|
"""
|
||||||
|
if not key.startswith("sk-ant-"):
|
||||||
|
return True
|
||||||
|
try:
|
||||||
|
from litellm import get_llm_provider
|
||||||
|
|
||||||
|
provider = (get_llm_provider(model)[1] or "").lower()
|
||||||
|
except Exception: # noqa: BLE001 - unclassifiable model, keep pass-through
|
||||||
|
return True
|
||||||
|
return provider not in _REJECTS_ANTHROPIC_KEY
|
||||||
|
|
||||||
|
|
||||||
def get_provider_config(provider: str) -> ProviderConfig:
|
def get_provider_config(provider: str) -> ProviderConfig:
|
||||||
"""Get provider config, with fallback for unknown providers."""
|
"""Get provider config, with fallback for unknown providers."""
|
||||||
if provider in PROVIDER_REGISTRY:
|
if provider in PROVIDER_REGISTRY:
|
||||||
|
|
@ -951,10 +992,14 @@ class LiteLLMBackend(Backend):
|
||||||
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
||||||
if self.provider not in _env_auth_providers:
|
if self.provider not in _env_auth_providers:
|
||||||
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
||||||
if auth_header.startswith("Bearer "):
|
_caller_key = (
|
||||||
kwargs["api_key"] = auth_header[7:]
|
auth_header[7:]
|
||||||
elif headers.get("x-api-key"):
|
if auth_header.startswith("Bearer ")
|
||||||
kwargs["api_key"] = headers["x-api-key"]
|
else headers.get("x-api-key", "")
|
||||||
|
)
|
||||||
|
# Only forward it if it can actually authenticate the TARGET.
|
||||||
|
if _caller_key and _caller_key_travels_to(litellm_model, _caller_key):
|
||||||
|
kwargs["api_key"] = _caller_key
|
||||||
|
|
||||||
logger.debug(f"LiteLLM request: model={litellm_model}")
|
logger.debug(f"LiteLLM request: model={litellm_model}")
|
||||||
|
|
||||||
|
|
@ -1059,10 +1104,14 @@ class LiteLLMBackend(Backend):
|
||||||
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
||||||
if self.provider not in _env_auth_providers:
|
if self.provider not in _env_auth_providers:
|
||||||
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
||||||
if auth_header.startswith("Bearer "):
|
_caller_key = (
|
||||||
kwargs["api_key"] = auth_header[7:]
|
auth_header[7:]
|
||||||
elif headers.get("x-api-key"):
|
if auth_header.startswith("Bearer ")
|
||||||
kwargs["api_key"] = headers["x-api-key"]
|
else headers.get("x-api-key", "")
|
||||||
|
)
|
||||||
|
# Only forward it if it can actually authenticate the TARGET.
|
||||||
|
if _caller_key and _caller_key_travels_to(litellm_model, _caller_key):
|
||||||
|
kwargs["api_key"] = _caller_key
|
||||||
|
|
||||||
msg_id = f"msg_{uuid.uuid4().hex[:24]}"
|
msg_id = f"msg_{uuid.uuid4().hex[:24]}"
|
||||||
|
|
||||||
|
|
@ -1315,10 +1364,14 @@ class LiteLLMBackend(Backend):
|
||||||
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
||||||
if self.provider not in _env_auth_providers:
|
if self.provider not in _env_auth_providers:
|
||||||
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
||||||
if auth_header.startswith("Bearer "):
|
_caller_key = (
|
||||||
kwargs["api_key"] = auth_header[7:]
|
auth_header[7:]
|
||||||
elif headers.get("x-api-key"):
|
if auth_header.startswith("Bearer ")
|
||||||
kwargs["api_key"] = headers["x-api-key"]
|
else headers.get("x-api-key", "")
|
||||||
|
)
|
||||||
|
# Only forward it if it can actually authenticate the TARGET.
|
||||||
|
if _caller_key and _caller_key_travels_to(litellm_model, _caller_key):
|
||||||
|
kwargs["api_key"] = _caller_key
|
||||||
|
|
||||||
logger.debug(f"LiteLLM OpenAI request: model={litellm_model}")
|
logger.debug(f"LiteLLM OpenAI request: model={litellm_model}")
|
||||||
|
|
||||||
|
|
@ -1490,10 +1543,14 @@ class LiteLLMBackend(Backend):
|
||||||
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
_env_auth_providers = ("bedrock", "vertex_ai", "vertex_ai_beta", "sagemaker")
|
||||||
if self.provider not in _env_auth_providers:
|
if self.provider not in _env_auth_providers:
|
||||||
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
auth_header = headers.get("authorization", headers.get("Authorization", ""))
|
||||||
if auth_header.startswith("Bearer "):
|
_caller_key = (
|
||||||
kwargs["api_key"] = auth_header[7:]
|
auth_header[7:]
|
||||||
elif headers.get("x-api-key"):
|
if auth_header.startswith("Bearer ")
|
||||||
kwargs["api_key"] = headers["x-api-key"]
|
else headers.get("x-api-key", "")
|
||||||
|
)
|
||||||
|
# Only forward it if it can actually authenticate the TARGET.
|
||||||
|
if _caller_key and _caller_key_travels_to(litellm_model, _caller_key):
|
||||||
|
kwargs["api_key"] = _caller_key
|
||||||
|
|
||||||
# Bounded, always: an upstream that never answers must not
|
# Bounded, always: an upstream that never answers must not
|
||||||
# block the caller forever. setdefault so an explicit value wins.
|
# block the caller forever. setdefault so an explicit value wins.
|
||||||
|
|
|
||||||
67
tests/test_litellm_caller_key.py
Normal file
67
tests/test_litellm_caller_key.py
Normal file
|
|
@ -0,0 +1,67 @@
|
||||||
|
"""A caller's key must not be dropped unless we are certain it cannot work.
|
||||||
|
|
||||||
|
The proxy forwards the inbound credential to the upstream provider. When a
|
||||||
|
routing extension rewrites the model across families mid-request, that key stops
|
||||||
|
matching the target and the 401 that follows is indistinguishable, downstream,
|
||||||
|
from "the cheap model failed the task".
|
||||||
|
|
||||||
|
Refusing to forward is the fix, but it is also the more dangerous direction: a
|
||||||
|
false positive silently strips a credential from a deployment that was working,
|
||||||
|
and litellm then falls back to an env key that may not exist. So the rule is
|
||||||
|
positive evidence only -- an unrecognised credential always travels.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from headroom.backends.litellm import _caller_key_travels_to
|
||||||
|
|
||||||
|
ANTHROPIC_KEY = "sk-ant-api03-abc123"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"model",
|
||||||
|
["gpt-5-mini", "gpt-4o", "azure/gpt-4", "gemini/gemini-2.0-flash"],
|
||||||
|
)
|
||||||
|
def test_anthropic_key_is_refused_for_a_provider_that_cannot_accept_it(model: str) -> None:
|
||||||
|
"""The bug this exists for: claude-* rewritten to a non-Anthropic target."""
|
||||||
|
pytest.importorskip("litellm")
|
||||||
|
assert _caller_key_travels_to(model, ANTHROPIC_KEY) is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"model",
|
||||||
|
["claude-opus-4-5-20251101", "anthropic/claude-sonnet-4-5-20250929"],
|
||||||
|
)
|
||||||
|
def test_anthropic_key_travels_to_anthropic(model: str) -> None:
|
||||||
|
pytest.importorskip("litellm")
|
||||||
|
assert _caller_key_travels_to(model, ANTHROPIC_KEY) is True
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize(
|
||||||
|
"key",
|
||||||
|
[
|
||||||
|
"sk-proj-openai-style", # a dozen vendors mint this shape
|
||||||
|
"Bearer-ish-opaque-token", # a plain gateway token
|
||||||
|
"hf_abc123",
|
||||||
|
"sk-ant", # near miss, not the prefix
|
||||||
|
"",
|
||||||
|
],
|
||||||
|
)
|
||||||
|
def test_only_the_anthropic_prefix_is_ever_classified(key: str) -> None:
|
||||||
|
"""Everything else is unclassifiable from the string, so it passes through.
|
||||||
|
|
||||||
|
This is the regression the review caught: the first version returned
|
||||||
|
`not provider.startswith("anthropic")`, which dropped every one of these
|
||||||
|
against an Anthropic-class target.
|
||||||
|
"""
|
||||||
|
assert _caller_key_travels_to("gpt-5-mini", key) is True
|
||||||
|
assert _caller_key_travels_to("claude-opus-4-5-20251101", key) is True
|
||||||
|
|
||||||
|
|
||||||
|
def test_unknown_provider_keeps_the_pass_through() -> None:
|
||||||
|
"""A compatible or self-hosted gateway we cannot classify must not lose its
|
||||||
|
key -- including when `get_llm_provider` raises on the model string."""
|
||||||
|
assert _caller_key_travels_to("some-self-hosted-thing", ANTHROPIC_KEY) is True
|
||||||
|
assert _caller_key_travels_to("", ANTHROPIC_KEY) is True
|
||||||
Loading…
Add table
Add a link
Reference in a new issue