mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-09 22:31:41 +00:00
fix(auth): screen bulk-update passwords concurrently before any db write
/user/bulk_update awaited a separate HIBP lookup for each user in the batch, so a degraded-slow HIBP (5s timeout per lookup) could stretch a 500-user batch to ~2500s and time out the request after some updates had already persisted. validate_passwords_bulk dedupes the batch's passwords, strength-checks first, then fires every needed HIBP lookup concurrently, bounding the worst case at one 5s timeout window. bulk_update_processed_users now screens the whole batch before the serial update loop, so a rejected password fails only its own entry and validation failures precede any persistence.
This commit is contained in:
parent
2f8980a417
commit
d4d82c151b
5 changed files with 390 additions and 384 deletions
|
|
@ -2005,4 +2005,4 @@ BATCH_ENQUEUED_TOKEN_LIMIT_METADATA_KEY: Final = "batch_enqueued_token_limit"
|
|||
EMPTY_MAPPING: Final = MappingProxyType({})
|
||||
|
||||
# API endpoint for breached password k-anonymity search
|
||||
HIBP_RANGE_API_BASE: Final = "https://api.pwnedpasswords.com/range"
|
||||
HIBP_RANGE_API_BASE: Final = "https://api.pwnedpasswords.com/range"
|
||||
|
|
|
|||
|
|
@ -11,9 +11,11 @@ of the password's SHA-1 hash ever leave the proxy, and the check fails open
|
|||
(allows the password) when HIBP is unreachable.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import hashlib
|
||||
from collections.abc import Mapping
|
||||
from collections.abc import Mapping, Sequence
|
||||
from dataclasses import dataclass
|
||||
from types import MappingProxyType
|
||||
from typing import Final
|
||||
|
||||
from litellm._logging import verbose_proxy_logger
|
||||
|
|
@ -136,6 +138,33 @@ async def _is_password_breached(password: str, client: AsyncHTTPHandler) -> bool
|
|||
return breached
|
||||
|
||||
|
||||
def is_breach_check_enabled(general_settings: Mapping[str, object]) -> bool:
|
||||
return general_settings.get("password_policy_check_breached_passwords", True) is not False
|
||||
|
||||
|
||||
async def is_password_breached(
|
||||
password: str,
|
||||
general_settings: Mapping[str, object],
|
||||
client: AsyncHTTPHandler | None = None,
|
||||
) -> bool:
|
||||
"""False when the check is disabled, the password is absent from the HIBP
|
||||
corpus, or HIBP is unreachable (fail open)."""
|
||||
if not is_breach_check_enabled(general_settings):
|
||||
return False
|
||||
return await _is_password_breached(password, client if client is not None else _hibp_client())
|
||||
|
||||
|
||||
def breached_password_error() -> ProxyException:
|
||||
return ProxyException(
|
||||
message=(
|
||||
"This password appears in known data breaches and cannot be used. Please choose a different password."
|
||||
),
|
||||
type=ProxyErrorTypes.validation_error,
|
||||
param="password",
|
||||
code=400,
|
||||
)
|
||||
|
||||
|
||||
async def validate_password_not_breached(
|
||||
password: str,
|
||||
general_settings: Mapping[str, object],
|
||||
|
|
@ -144,16 +173,42 @@ async def validate_password_not_breached(
|
|||
"""Raise ``ProxyException`` (400) if ``password`` appears in a known data breach.
|
||||
|
||||
Fails open: an unreachable or misbehaving HIBP allows the password."""
|
||||
check_enabled: Final = general_settings.get("password_policy_check_breached_passwords", True) is not False
|
||||
if not check_enabled:
|
||||
if not await is_password_breached(password, general_settings, client):
|
||||
return
|
||||
if not await _is_password_breached(password, client if client is not None else _hibp_client()):
|
||||
return
|
||||
raise ProxyException(
|
||||
message=(
|
||||
"This password appears in known data breaches and cannot be used. Please choose a different password."
|
||||
),
|
||||
type=ProxyErrorTypes.validation_error,
|
||||
param="password",
|
||||
code=400,
|
||||
raise breached_password_error()
|
||||
|
||||
|
||||
def _strength_verdict(password: str, general_settings: Mapping[str, object]) -> ProxyException | None:
|
||||
try:
|
||||
validate_password_policy(password, general_settings)
|
||||
except ProxyException as e:
|
||||
return e
|
||||
return None
|
||||
|
||||
|
||||
async def validate_passwords_bulk(
|
||||
passwords: Sequence[str],
|
||||
general_settings: Mapping[str, object],
|
||||
client: AsyncHTTPHandler | None = None,
|
||||
) -> Mapping[str, ProxyException | None]:
|
||||
"""Per-unique-password policy verdicts for a batch: the ProxyException to
|
||||
surface, or None when the password is acceptable.
|
||||
|
||||
Deduplicates first, then issues every needed HIBP lookup concurrently, so a
|
||||
batch caller pays one HIBP timeout window in the worst case instead of one
|
||||
per password (each lookup still fails open independently)."""
|
||||
unique_passwords: Final = tuple(dict.fromkeys(passwords))
|
||||
strength_verdicts: Final[Mapping[str, ProxyException | None]] = MappingProxyType(
|
||||
{password: _strength_verdict(password, general_settings) for password in unique_passwords}
|
||||
)
|
||||
to_screen: Final = tuple(password for password in unique_passwords if strength_verdicts[password] is None)
|
||||
breached_flags: Final = await asyncio.gather(
|
||||
*(is_password_breached(password, general_settings, client) for password in to_screen)
|
||||
)
|
||||
breached_passwords: Final = frozenset(password for password, breached in zip(to_screen, breached_flags) if breached)
|
||||
return MappingProxyType(
|
||||
{
|
||||
password: breached_password_error() if password in breached_passwords else strength_verdicts[password]
|
||||
for password in unique_passwords
|
||||
}
|
||||
)
|
||||
|
|
|
|||
|
|
@ -26,9 +26,14 @@ from fastapi import APIRouter, Depends, Header, HTTPException, Request, status
|
|||
import litellm
|
||||
from litellm._logging import verbose_proxy_logger
|
||||
from litellm._uuid import uuid
|
||||
from litellm.llms.custom_httpx.http_handler import AsyncHTTPHandler
|
||||
from litellm.proxy._types import *
|
||||
from litellm.proxy.auth.auth_checks import get_team_object, get_user_object
|
||||
from litellm.proxy.auth.password_policy import validate_password_not_breached, validate_password_policy
|
||||
from litellm.proxy.auth.password_policy import (
|
||||
validate_password_not_breached,
|
||||
validate_password_policy,
|
||||
validate_passwords_bulk,
|
||||
)
|
||||
from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
||||
from litellm.proxy.common_utils.user_api_key_cache import (
|
||||
object_permission_cache_key,
|
||||
|
|
@ -157,11 +162,17 @@ def _team_membership_table(
|
|||
return team_membership_table
|
||||
|
||||
|
||||
async def _hash_password_in_dict(data: dict, general_settings: Mapping[str, object]) -> None:
|
||||
"""Validate and hash password field in-place if present."""
|
||||
async def _hash_password_in_dict(
|
||||
data: dict, general_settings: Mapping[str, object], password_prevalidated: bool = False
|
||||
) -> None:
|
||||
"""Validate and hash password field in-place if present.
|
||||
|
||||
``password_prevalidated`` skips the policy checks for callers that already
|
||||
validated the password (the bulk path screens its whole batch upfront)."""
|
||||
if "password" in data and data["password"] is not None:
|
||||
validate_password_policy(data["password"], general_settings)
|
||||
await validate_password_not_breached(data["password"], general_settings)
|
||||
if not password_prevalidated:
|
||||
validate_password_policy(data["password"], general_settings)
|
||||
await validate_password_not_breached(data["password"], general_settings)
|
||||
data["password"] = hash_password(data["password"])
|
||||
|
||||
|
||||
|
|
@ -1416,6 +1427,7 @@ async def _update_single_user_helper(
|
|||
user_request: UpdateUserRequest,
|
||||
user_api_key_dict: UserAPIKeyAuth,
|
||||
litellm_changed_by: str | None = None,
|
||||
password_prevalidated: bool = False,
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Helper function to update a single user.
|
||||
|
|
@ -1438,7 +1450,7 @@ async def _update_single_user_helper(
|
|||
|
||||
data_json: Final[dict] = user_request.model_dump(exclude_unset=True)
|
||||
non_default_values = _update_internal_user_params(data_json=data_json, data=user_request)
|
||||
await _hash_password_in_dict(non_default_values, general_settings)
|
||||
await _hash_password_in_dict(non_default_values, general_settings, password_prevalidated=password_prevalidated)
|
||||
|
||||
existing_user_row: BaseModel | None = None
|
||||
if user_request.user_id:
|
||||
|
|
@ -1682,19 +1694,38 @@ async def bulk_update_processed_users(
|
|||
users_to_update: list[UpdateUserRequest],
|
||||
user_api_key_dict: UserAPIKeyAuth,
|
||||
litellm_changed_by: str | None = None,
|
||||
hibp_client: AsyncHTTPHandler | None = None,
|
||||
) -> BulkUpdateUserResponse:
|
||||
from litellm.proxy.proxy_server import general_settings
|
||||
|
||||
results: Final[list[UserUpdateResult]] = []
|
||||
successful_updates = 0
|
||||
failed_updates = 0
|
||||
|
||||
# Screen the batch's passwords upfront and concurrently: done per-user
|
||||
# inside the loop below, each HIBP lookup would be awaited serially and a
|
||||
# degraded-slow HIBP could stretch a full batch to minutes, timing out the
|
||||
# request after some updates already persisted.
|
||||
password_verdicts: Final = await validate_passwords_bulk(
|
||||
tuple(u.password for u in users_to_update if u.password is not None),
|
||||
general_settings,
|
||||
client=hibp_client,
|
||||
)
|
||||
|
||||
# Process each user update independently
|
||||
try:
|
||||
for user_request in users_to_update:
|
||||
try:
|
||||
if (
|
||||
user_request.password is not None
|
||||
and (password_error := password_verdicts.get(user_request.password)) is not None
|
||||
):
|
||||
raise password_error
|
||||
response = await _update_single_user_helper(
|
||||
user_request=user_request,
|
||||
user_api_key_dict=user_api_key_dict,
|
||||
litellm_changed_by=litellm_changed_by,
|
||||
password_prevalidated=True,
|
||||
)
|
||||
# Record success
|
||||
results.append(
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ The breach-check (HIBP) tests inject a real AsyncHTTPHandler wrapping an
|
|||
httpx.MockTransport, so no network is touched and nothing is monkeypatched.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import hashlib
|
||||
|
||||
import httpx
|
||||
|
|
@ -21,6 +22,7 @@ from litellm.proxy.auth.password_policy import (
|
|||
get_password_policy,
|
||||
validate_password_not_breached,
|
||||
validate_password_policy,
|
||||
validate_passwords_bulk,
|
||||
)
|
||||
|
||||
STRONG_PASSWORD = "Str0ng!Passw0rd"
|
||||
|
|
@ -268,3 +270,76 @@ async def test_breach_check_fails_open_on_malformed_response_body():
|
|||
client=_client_returning(f"{_sha1_upper('password12345')[5:]}:not-a-number"),
|
||||
)
|
||||
assert result is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validate_passwords_bulk_screens_concurrently():
|
||||
"""All HIBP lookups for a batch must be in flight at once: each handler
|
||||
call stalls until every expected request has arrived, and a handler that
|
||||
gives up waiting reports the password as breached. Serial awaiting (the
|
||||
old per-user behavior) leaves each earlier request waiting forever for the
|
||||
later ones, so every verdict comes back as a breach and the test fails."""
|
||||
passwords = ("Uniqu3!Passw0rd-a", "Uniqu3!Passw0rd-b", "Uniqu3!Passw0rd-c")
|
||||
suffix_by_prefix = {_sha1_upper(p)[:5]: _sha1_upper(p)[5:] for p in passwords}
|
||||
all_arrived = asyncio.Event()
|
||||
arrivals: list[str] = []
|
||||
|
||||
async def handler(request: httpx.Request) -> httpx.Response:
|
||||
arrivals.append(request.url.path)
|
||||
if len(arrivals) == len(passwords):
|
||||
all_arrived.set()
|
||||
try:
|
||||
await asyncio.wait_for(all_arrived.wait(), timeout=5)
|
||||
except TimeoutError:
|
||||
return httpx.Response(200, text=f"{suffix_by_prefix[request.url.path.rsplit('/', 1)[-1]]}:1")
|
||||
return httpx.Response(200, text="0000000000000000000000000000000000A:1")
|
||||
|
||||
verdicts = await validate_passwords_bulk(passwords, {}, client=_client_with_transport(handler))
|
||||
assert set(arrivals) == {f"/range/{prefix}" for prefix in suffix_by_prefix}
|
||||
assert all(verdicts[p] is None for p in passwords)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validate_passwords_bulk_deduplicates_lookups():
|
||||
"""500 users sharing one password must cost exactly one HIBP lookup."""
|
||||
password = "Sh@red-Passw0rd!"
|
||||
request_count = 0
|
||||
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
nonlocal request_count
|
||||
request_count += 1
|
||||
return httpx.Response(200, text="0000000000000000000000000000000000A:1")
|
||||
|
||||
verdicts = await validate_passwords_bulk((password,) * 500, {}, client=_client_with_transport(handler))
|
||||
assert request_count == 1
|
||||
assert verdicts == {password: None}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validate_passwords_bulk_mixed_verdicts():
|
||||
"""Weak passwords are rejected without an HIBP lookup; breached ones get
|
||||
the breach error; acceptable ones map to None."""
|
||||
breached = "Br3ached!Passw0rd"
|
||||
clean = "Cl3an!!Passw0rd42"
|
||||
weak = "short1!"
|
||||
breached_sha1 = _sha1_upper(breached)
|
||||
looked_up_prefixes: list[str] = []
|
||||
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
looked_up_prefixes.append(request.url.path.rsplit("/", 1)[-1])
|
||||
if request.url.path == f"/range/{breached_sha1[:5]}":
|
||||
return httpx.Response(200, text=f"{breached_sha1[5:]}:99")
|
||||
return httpx.Response(200, text="0000000000000000000000000000000000A:1")
|
||||
|
||||
verdicts = await validate_passwords_bulk((breached, clean, weak), {}, client=_client_with_transport(handler))
|
||||
assert _sha1_upper(weak)[:5] not in looked_up_prefixes
|
||||
assert verdicts[clean] is None
|
||||
assert "data breaches" in verdicts[breached].message
|
||||
assert verdicts[breached].code == "400"
|
||||
assert "12 characters" in verdicts[weak].message
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_validate_passwords_bulk_empty_batch_makes_no_lookups():
|
||||
verdicts = await validate_passwords_bulk((), {}, client=_client_never_called())
|
||||
assert verdicts == {}
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load diff
Loading…
Add table
Reference in a new issue