mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-25 01:02:15 +00:00
fix(mcp): admit and forward Authorization for passthrough OAuth return
For pass-through MCP servers (auth_type=none with delegate_auth_to_upstream) the RFC 9728 cold-start flow sends the client back with only "Authorization: Bearer <upstream-token>" after upstream OAuth discovery. Previously this path 1) was rejected in process_mcp_request because the oauth2_headers fallback only covered auth_type=oauth2 targets, and 2) had the Authorization header stripped by _prepare_mcp_server_headers when no x-litellm-api-key was present, treating the upstream token as a potential LiteLLM key leak. - Extend the elif oauth2_headers fallback to also admit anonymously when every target is a pass-through server. - Pass user_api_key_auth into _prepare_mcp_server_headers so it can forward Authorization for pass-through servers when admission did not consume the bearer as a LiteLLM key (api_key is unset). Co-authored-by: Yassin Kortam <yassin@berri.ai>
This commit is contained in:
parent
0fada28153
commit
e9ff79c96a
4 changed files with 200 additions and 16 deletions
|
|
@ -252,21 +252,45 @@ class MCPRequestHandler:
|
|||
# than coercing (``int("None")`` would raise ValueError and
|
||||
# rewrite the auth error as a 500).
|
||||
status = e.status_code if isinstance(e, HTTPException) else e.code
|
||||
if status in (
|
||||
401,
|
||||
403,
|
||||
"401",
|
||||
"403",
|
||||
) and MCPRequestHandler._target_servers_use_oauth2(
|
||||
is_auth_error = status in (401, 403, "401", "403")
|
||||
client_ip = IPAddressUtils.get_mcp_client_ip(request)
|
||||
if is_auth_error and MCPRequestHandler._target_servers_use_oauth2(
|
||||
path=request.url.path,
|
||||
mcp_servers=mcp_servers,
|
||||
client_ip=IPAddressUtils.get_mcp_client_ip(request),
|
||||
client_ip=client_ip,
|
||||
):
|
||||
verbose_logger.debug(
|
||||
"MCP OAuth2: target server is OAuth2-mode, treating "
|
||||
"Authorization as upstream OAuth2 token passthrough"
|
||||
)
|
||||
validated_user_api_key_auth = UserAPIKeyAuth()
|
||||
elif is_auth_error:
|
||||
# Pass-through cold-start return: per RFC 9728 / MCP
|
||||
# Authorization spec the client completes upstream OAuth
|
||||
# discovery and returns with ``Authorization: Bearer
|
||||
# <upstream-token>``. For ``auth_type=none`` passthrough
|
||||
# servers that bearer is not a LiteLLM key (auth above
|
||||
# failed) but is meant to be forwarded upstream
|
||||
# unchanged. Fall back to anonymous admission so the
|
||||
# caller is not rejected for following the discovery
|
||||
# flow without also setting ``x-litellm-api-key``.
|
||||
mcp_servers_from_path = _parse_mcp_server_names_from_path(
|
||||
request.url.path
|
||||
)
|
||||
if (
|
||||
mcp_servers_from_path is not None
|
||||
and _is_mcp_passthrough_cold_start(
|
||||
mcp_servers_from_path, client_ip=client_ip
|
||||
)
|
||||
):
|
||||
verbose_logger.debug(
|
||||
"MCP pass-through return: target server is "
|
||||
"passthrough, treating Authorization as "
|
||||
"upstream OAuth token for delegated auth"
|
||||
)
|
||||
validated_user_api_key_auth = UserAPIKeyAuth()
|
||||
else:
|
||||
raise
|
||||
else:
|
||||
raise
|
||||
else:
|
||||
|
|
|
|||
|
|
@ -1148,6 +1148,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header: Optional[str],
|
||||
oauth2_headers: Optional[Dict[str, str]],
|
||||
raw_headers: Optional[Dict[str, str]],
|
||||
user_api_key_auth: Optional[UserAPIKeyAuth] = None,
|
||||
) -> Tuple[Optional[Union[Dict[str, str], str]], Optional[Dict[str, str]]]:
|
||||
"""Build auth and extra headers for a server."""
|
||||
server_auth_header: Optional[Union[Dict[str, str], str]] = None
|
||||
|
|
@ -1177,6 +1178,17 @@ if MCP_AVAILABLE:
|
|||
has_explicit_litellm_admission_header = (
|
||||
normalized_raw_headers.get("x-litellm-api-key") is not None
|
||||
)
|
||||
# Admission consumed ``Authorization`` as a LiteLLM key only when
|
||||
# auth produced a validated ``api_key`` AND the caller did not
|
||||
# supply ``x-litellm-api-key``. When admission was anonymous
|
||||
# (e.g. pass-through cold-start return per RFC 9728), the bearer
|
||||
# in ``Authorization`` is the upstream OAuth token and must be
|
||||
# forwarded — not stripped — for the delegated flow to work.
|
||||
admission_consumed_authorization_as_litellm_key = (
|
||||
user_api_key_auth is not None
|
||||
and bool(getattr(user_api_key_auth, "api_key", None))
|
||||
and not has_explicit_litellm_admission_header
|
||||
)
|
||||
|
||||
for header in server.extra_headers:
|
||||
if not isinstance(header, str):
|
||||
|
|
@ -1189,13 +1201,20 @@ if MCP_AVAILABLE:
|
|||
continue
|
||||
# Transparent OAuth pass-through: forward the caller's
|
||||
# Authorization header only when LiteLLM admission used
|
||||
# a different header (`x-litellm-api-key`). Without an
|
||||
# explicit admission header, `Authorization` may itself
|
||||
# be the LiteLLM key — strip it to avoid leaking the
|
||||
# gateway credential upstream.
|
||||
if (
|
||||
server.is_oauth_passthrough
|
||||
and not has_explicit_litellm_admission_header
|
||||
# a different header (`x-litellm-api-key`) or when
|
||||
# admission was anonymous (delegated to upstream).
|
||||
# Without that signal `Authorization` may itself be the
|
||||
# LiteLLM key — strip it to avoid leaking the gateway
|
||||
# credential upstream. The legacy ``user_api_key_auth
|
||||
# is None`` callers keep the conservative pre-PR
|
||||
# behavior of stripping when no explicit admission
|
||||
# header was supplied.
|
||||
if server.is_oauth_passthrough and (
|
||||
admission_consumed_authorization_as_litellm_key
|
||||
or (
|
||||
user_api_key_auth is None
|
||||
and not has_explicit_litellm_admission_header
|
||||
)
|
||||
):
|
||||
continue
|
||||
header_value = normalized_raw_headers.get(header.lower())
|
||||
|
|
@ -1391,6 +1410,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header=mcp_auth_header,
|
||||
oauth2_headers=oauth2_headers,
|
||||
raw_headers=raw_headers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
|
||||
# Prefer server-stored per-user OAuth when configured, so a stale
|
||||
|
|
@ -1565,6 +1585,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header=mcp_auth_header,
|
||||
oauth2_headers=oauth2_headers,
|
||||
raw_headers=raw_headers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
|
||||
try:
|
||||
|
|
@ -1622,6 +1643,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header=mcp_auth_header,
|
||||
oauth2_headers=oauth2_headers,
|
||||
raw_headers=raw_headers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
|
||||
try:
|
||||
|
|
@ -1677,6 +1699,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header=mcp_auth_header,
|
||||
oauth2_headers=oauth2_headers,
|
||||
raw_headers=raw_headers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
|
||||
try:
|
||||
|
|
@ -2512,6 +2535,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header=mcp_auth_header,
|
||||
oauth2_headers=oauth2_headers,
|
||||
raw_headers=raw_headers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
|
||||
return await global_mcp_server_manager.get_prompt_from_server(
|
||||
|
|
@ -2562,6 +2586,7 @@ if MCP_AVAILABLE:
|
|||
mcp_auth_header=mcp_auth_header,
|
||||
oauth2_headers=oauth2_headers,
|
||||
raw_headers=raw_headers,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
|
||||
return await global_mcp_server_manager.read_resource_from_server(
|
||||
|
|
|
|||
|
|
@ -1170,9 +1170,14 @@ class TestMCPOAuth2FallbackTargetGating:
|
|||
"""
|
||||
|
||||
@staticmethod
|
||||
def _make_server(auth_type):
|
||||
def _make_server(auth_type, is_oauth_passthrough=False):
|
||||
server = MagicMock()
|
||||
server.auth_type = auth_type
|
||||
# MagicMock would otherwise auto-create truthy stand-ins for any
|
||||
# attribute access (including ``is_oauth_passthrough``), which
|
||||
# would silently flip the passthrough fallback gate on. Pin the
|
||||
# boolean explicitly so non-passthrough fixtures stay non-passthrough.
|
||||
server.is_oauth_passthrough = is_oauth_passthrough
|
||||
return server
|
||||
|
||||
async def test_fallback_blocked_when_target_is_not_oauth2(self):
|
||||
|
|
@ -1274,6 +1279,48 @@ class TestMCPOAuth2FallbackTargetGating:
|
|||
(auth_result, *_rest) = await MCPRequestHandler.process_mcp_request(scope)
|
||||
assert isinstance(auth_result, UserAPIKeyAuth)
|
||||
|
||||
async def test_fallback_allowed_when_target_is_passthrough(self):
|
||||
"""
|
||||
Cold-start return per RFC 9728 / MCP Authorization spec: client
|
||||
discovered the upstream IdP via the gateway's protected-resource
|
||||
metadata, completed OAuth, and is returning with
|
||||
``Authorization: Bearer <upstream-token>``. The bearer is not a
|
||||
LiteLLM key but the target is a pass-through server, so admission
|
||||
falls back to anonymous and forwards the bearer upstream.
|
||||
"""
|
||||
from fastapi import HTTPException
|
||||
|
||||
from litellm.types.mcp import MCPAuth
|
||||
|
||||
scope = {
|
||||
"type": "http",
|
||||
"method": "POST",
|
||||
"path": "/mcp/passthrough_server",
|
||||
"headers": [(b"authorization", b"Bearer upstream-token-xyz")],
|
||||
}
|
||||
|
||||
async def mock_user_api_key_auth_fails(api_key, request):
|
||||
raise HTTPException(status_code=401, detail="Invalid API key")
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.auth.user_api_key_auth_mcp.user_api_key_auth",
|
||||
side_effect=mock_user_api_key_auth_fails,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager"
|
||||
) as mock_mgr,
|
||||
):
|
||||
mock_mgr.get_mcp_server_by_name.return_value = (
|
||||
TestMCPOAuth2FallbackTargetGating._make_server(
|
||||
auth_type=MCPAuth.none,
|
||||
is_oauth_passthrough=True,
|
||||
)
|
||||
)
|
||||
(auth_result, *_rest) = await MCPRequestHandler.process_mcp_request(scope)
|
||||
assert isinstance(auth_result, UserAPIKeyAuth)
|
||||
assert auth_result.api_key is None
|
||||
|
||||
async def test_fallback_blocked_when_client_ip_hides_oauth2_target(self):
|
||||
from fastapi import HTTPException
|
||||
|
||||
|
|
@ -1305,7 +1352,12 @@ class TestMCPOAuth2FallbackTargetGating:
|
|||
await MCPRequestHandler.process_mcp_request(scope)
|
||||
|
||||
assert exc_info.value.status_code == 401
|
||||
mock_mgr.get_mcp_server_by_name.assert_called_once_with(
|
||||
# Lookup may run twice — once for the oauth2-target fallback gate
|
||||
# and once for the passthrough-target fallback gate. Both must
|
||||
# resolve to ``None`` (hidden by client IP) so neither bypass
|
||||
# opens. Use ``assert_any_call`` to assert the IP-scoped lookup
|
||||
# happened without locking the count.
|
||||
mock_mgr.get_mcp_server_by_name.assert_any_call(
|
||||
"hidden_oauth2_server", client_ip="203.0.113.10"
|
||||
)
|
||||
|
||||
|
|
|
|||
|
|
@ -171,6 +171,87 @@ def test_prepare_mcp_server_headers_passthrough_strips_authorization_without_adm
|
|||
assert extra_headers == {"x-request-id": "req-789"}
|
||||
|
||||
|
||||
def test_prepare_mcp_server_headers_passthrough_forwards_authorization_for_anonymous_admission():
|
||||
"""Cold-start return per RFC 9728: client admits anonymously through
|
||||
the pass-through fallback in :meth:`MCPRequestHandler.process_mcp_request`
|
||||
(``user_api_key_auth.api_key is None``) and the ``Authorization`` bearer
|
||||
is the upstream OAuth token — it must be forwarded, not stripped."""
|
||||
try:
|
||||
from litellm.proxy._experimental.mcp_server.server import (
|
||||
_prepare_mcp_server_headers,
|
||||
)
|
||||
except ImportError:
|
||||
pytest.skip("MCP server not available")
|
||||
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
server = MCPServer(
|
||||
server_id="server-passthrough-anon-admission",
|
||||
name="server",
|
||||
transport=MCPTransport.http,
|
||||
auth_type=MCPAuth.none,
|
||||
extra_headers=["Authorization", "x-request-id"],
|
||||
delegate_auth_to_upstream=True,
|
||||
)
|
||||
|
||||
server_auth_header, extra_headers = _prepare_mcp_server_headers(
|
||||
server=server,
|
||||
mcp_server_auth_headers=None,
|
||||
mcp_auth_header=None,
|
||||
oauth2_headers=None,
|
||||
raw_headers={
|
||||
"authorization": "Bearer upstream-oauth-token",
|
||||
"x-request-id": "req-790",
|
||||
},
|
||||
user_api_key_auth=UserAPIKeyAuth(),
|
||||
)
|
||||
|
||||
assert server_auth_header is None
|
||||
assert extra_headers == {
|
||||
"Authorization": "Bearer upstream-oauth-token",
|
||||
"x-request-id": "req-790",
|
||||
}
|
||||
|
||||
|
||||
def test_prepare_mcp_server_headers_passthrough_strips_authorization_for_authenticated_admission():
|
||||
"""When admission validated ``Authorization`` as a LiteLLM key
|
||||
(``user_api_key_auth.api_key`` is set, no explicit ``x-litellm-api-key``),
|
||||
the bearer must still be stripped to avoid leaking the gateway key
|
||||
upstream."""
|
||||
try:
|
||||
from litellm.proxy._experimental.mcp_server.server import (
|
||||
_prepare_mcp_server_headers,
|
||||
)
|
||||
except ImportError:
|
||||
pytest.skip("MCP server not available")
|
||||
|
||||
from litellm.proxy._types import UserAPIKeyAuth
|
||||
|
||||
server = MCPServer(
|
||||
server_id="server-passthrough-authenticated",
|
||||
name="server",
|
||||
transport=MCPTransport.http,
|
||||
auth_type=MCPAuth.none,
|
||||
extra_headers=["Authorization", "x-request-id"],
|
||||
delegate_auth_to_upstream=True,
|
||||
)
|
||||
|
||||
server_auth_header, extra_headers = _prepare_mcp_server_headers(
|
||||
server=server,
|
||||
mcp_server_auth_headers=None,
|
||||
mcp_auth_header=None,
|
||||
oauth2_headers=None,
|
||||
raw_headers={
|
||||
"authorization": "Bearer sk-litellm-key",
|
||||
"x-request-id": "req-791",
|
||||
},
|
||||
user_api_key_auth=UserAPIKeyAuth(api_key="sk-litellm-key"),
|
||||
)
|
||||
|
||||
assert server_auth_header is None
|
||||
assert extra_headers == {"x-request-id": "req-791"}
|
||||
|
||||
|
||||
def test_prepare_mcp_server_headers_oauth2_m2m_omits_litellm_caller_authorization():
|
||||
"""M2M OAuth must not put caller Bearer (LiteLLM API key) into extra_headers (#23652)."""
|
||||
try:
|
||||
|
|
@ -547,6 +628,7 @@ async def test_mcp_get_prompt_success():
|
|||
mcp_auth_header=None,
|
||||
oauth2_headers=None,
|
||||
raw_headers=None,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
mock_manager.get_prompt_from_server.assert_awaited_once_with(
|
||||
server=server,
|
||||
|
|
@ -608,6 +690,7 @@ async def test_mcp_read_resource_success():
|
|||
mcp_auth_header=None,
|
||||
oauth2_headers=None,
|
||||
raw_headers=None,
|
||||
user_api_key_auth=user_api_key_auth,
|
||||
)
|
||||
mock_manager.read_resource_from_server.assert_awaited_once_with(
|
||||
server=server,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue