mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-29 06:33:28 +00:00
fix(proxy): run PBKDF2 password hashing off the event loop during async login
PBKDF2-HMAC-SHA256 at 600k iterations (~240ms verify, ~480ms rehash-on-login) is CPU-bound; running it inline in the async login/change-password/user-update/ invitation/migration paths stalled the event loop for the duration of every password operation. Wrap each async-boundary call in asyncio.to_thread so the KDF runs on a worker thread (hashlib releases the GIL for pbkdf2_hmac). The sync helpers and hashing parameters are unchanged for non-async callers. Closes litereview finding r4268681493 on PR #42921.
This commit is contained in:
parent
ea077db62a
commit
5d6a42f454
8 changed files with 206 additions and 7 deletions
|
|
@ -5,6 +5,7 @@ This module contains the core login logic that can be reused across different
|
|||
login endpoints (e.g., /login and /v2/login).
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
import os
|
||||
import secrets
|
||||
from collections.abc import Mapping
|
||||
|
|
@ -114,9 +115,12 @@ async def _rehash_password_if_needed(user_id: str, password: str, stored: str) -
|
|||
|
||||
if prisma_client is not None:
|
||||
try:
|
||||
# PBKDF2 at 600k iterations is CPU-bound (~240ms); hash it off the
|
||||
# event loop so concurrent requests do not stall behind a rehash.
|
||||
rehashed: Final = await asyncio.to_thread(hash_password, password)
|
||||
await UserRepository(prisma_client).table.update_many(
|
||||
where={"user_id": user_id, "password": stored},
|
||||
data={"password": hash_password(password)},
|
||||
data={"password": rehashed},
|
||||
)
|
||||
except Exception as e: # noqa: BLE001 # a failed rehash must never surface into the login
|
||||
verbose_proxy_logger.warning("Login-time password rehash could not update user %s: %s", user_id, e)
|
||||
|
|
@ -415,7 +419,9 @@ async def _sign_in(
|
|||
code=401,
|
||||
)
|
||||
|
||||
if verify_password(password, _password):
|
||||
# PBKDF2 at 600k iterations is CPU-bound (~240ms); run the KDF on a
|
||||
# worker thread so the event loop keeps serving requests during login.
|
||||
if await asyncio.to_thread(verify_password, password, _password):
|
||||
await _rehash_password_if_needed(_user_row.user_id, password, _password)
|
||||
breached_now: Final = prisma_client is not None and await screen_login_password_for_breach(
|
||||
user_id=_user_row.user_id,
|
||||
|
|
|
|||
|
|
@ -200,7 +200,9 @@ async def _hash_password_in_dict(
|
|||
if not password_prevalidated:
|
||||
validate_password_policy(data["password"], general_settings)
|
||||
await validate_password_not_breached(data["password"], general_settings, hibp_client)
|
||||
data["password"] = hash_password(data["password"])
|
||||
# PBKDF2 at 600k iterations is CPU-bound (~240ms); hash off the event
|
||||
# loop so concurrent requests do not stall behind this write path.
|
||||
data["password"] = await asyncio.to_thread(hash_password, data["password"])
|
||||
data["password_reset_required"] = True
|
||||
data["last_breach_check_at"] = None
|
||||
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ request kwargs to OTEL spans, which would log plaintext passwords. The audit
|
|||
signal is emitted by hand below, with field names only, never values.
|
||||
"""
|
||||
|
||||
import asyncio
|
||||
from typing import TYPE_CHECKING, Annotated, Final
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException
|
||||
|
|
@ -129,7 +130,9 @@ async def change_password(
|
|||
),
|
||||
)
|
||||
|
||||
if not verify_password(data.current_password, stored_password):
|
||||
# PBKDF2 at 600k iterations is CPU-bound (~240ms); run the KDF on a worker
|
||||
# thread so the event loop keeps serving requests during the checks.
|
||||
if not await asyncio.to_thread(verify_password, data.current_password, stored_password):
|
||||
raise HTTPException(status_code=400, detail=_error_detail("Current password is incorrect."))
|
||||
|
||||
if data.new_password == data.current_password:
|
||||
|
|
@ -141,8 +144,9 @@ async def change_password(
|
|||
validate_password_policy(data.new_password, general_settings)
|
||||
await validate_password_not_breached(data.new_password, general_settings, hibp_client)
|
||||
|
||||
new_hash: Final = await asyncio.to_thread(hash_password, data.new_password)
|
||||
password_update: Final[prisma_types.LiteLLM_UserTableUpdateInput] = {
|
||||
"password": hash_password(data.new_password),
|
||||
"password": new_hash,
|
||||
"password_reset_required": False,
|
||||
"last_breach_check_at": None,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -17564,7 +17564,8 @@ async def claim_onboarding_link(data: InvitationClaim, request: Request):
|
|||
|
||||
validate_password_policy(data.password, general_settings)
|
||||
await validate_password_not_breached(data.password, general_settings)
|
||||
hashed_pw: Final = hash_password(data.password)
|
||||
# PBKDF2 at 600k iterations is CPU-bound (~240ms); hash off the event loop.
|
||||
hashed_pw: Final = await asyncio.to_thread(hash_password, data.password)
|
||||
current_time = litellm.utils.get_utc_datetime()
|
||||
async with prisma_client.db.tx() as tx:
|
||||
updated_count: Final = await tx.litellm_invitationlink.update_many(
|
||||
|
|
|
|||
|
|
@ -7242,9 +7242,11 @@ async def migrate_plaintext_passwords_async(prisma_client) -> str:
|
|||
return "No plaintext passwords found"
|
||||
|
||||
for user_id, plaintext_password in plaintext_users:
|
||||
# PBKDF2 at 600k iterations is CPU-bound (~240ms per row); hash off
|
||||
# the event loop so startup does not stall other async work.
|
||||
await UserRepository(prisma_client).table.update(
|
||||
where={"user_id": user_id},
|
||||
data={"password": hash_password(plaintext_password)},
|
||||
data={"password": await asyncio.to_thread(hash_password, plaintext_password)},
|
||||
)
|
||||
return f"Migrated {len(plaintext_users)} plaintext passwords to pbkdf2"
|
||||
|
||||
|
|
|
|||
|
|
@ -596,6 +596,102 @@ async def test_authenticate_user_database_login_with_non_ascii_password():
|
|||
assert result.user_email == user_email
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_login_password_kdf_runs_off_event_loop_thread(monkeypatch):
|
||||
"""PBKDF2 at 600k iterations is CPU-bound (~240ms per verify, plus a full
|
||||
rehash on legacy rows). If it runs on the event loop thread, every concurrent
|
||||
request on that worker stalls for the duration of a login. The async login
|
||||
path must execute the KDF on a worker thread instead."""
|
||||
import base64
|
||||
import threading
|
||||
|
||||
from litellm.proxy.auth import login_utils as login_module
|
||||
|
||||
def _scrypt_row(password: str) -> str:
|
||||
salt = b"\x01" * 16
|
||||
derived = hashlib.scrypt(password.encode(), salt=salt, n=16384, r=8, p=1, dklen=32)
|
||||
return "scrypt:" + base64.b64encode(salt + derived).decode()
|
||||
|
||||
user_email = "kdf-thread@example.com"
|
||||
password = "correct-password"
|
||||
# Legacy scrypt row: exercises both the verify and the transparent rehash path.
|
||||
stored_password = _scrypt_row(password)
|
||||
|
||||
mock_user = MagicMock()
|
||||
mock_user.user_id = "test-user-kdf"
|
||||
mock_user.user_email = user_email
|
||||
mock_user.password = stored_password
|
||||
mock_user.user_role = LitellmUserRoles.INTERNAL_USER
|
||||
|
||||
def mock_find_first(**kwargs):
|
||||
where = kwargs.get("where", {})
|
||||
user_email_filter = where.get("user_email", {})
|
||||
if str(user_email_filter.get("equals", "")).lower() == user_email.lower():
|
||||
return mock_user
|
||||
return None
|
||||
|
||||
mock_prisma_client = MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable.find_first = AsyncMock(side_effect=mock_find_first)
|
||||
|
||||
seen_threads: dict[str, int] = {}
|
||||
real_verify = login_module.verify_password
|
||||
real_hash = login_module.hash_password
|
||||
|
||||
def recording_verify(password: str, stored: str) -> bool:
|
||||
seen_threads.setdefault("verify", threading.get_ident())
|
||||
return real_verify(password, stored)
|
||||
|
||||
def recording_hash(password: str) -> str:
|
||||
seen_threads.setdefault("hash", threading.get_ident())
|
||||
return real_hash(password)
|
||||
|
||||
monkeypatch.setattr(login_module, "verify_password", recording_verify)
|
||||
monkeypatch.setattr(login_module, "hash_password", recording_hash)
|
||||
|
||||
mock_user_repository = MagicMock()
|
||||
mock_user_repository.table.find_first = AsyncMock(side_effect=mock_find_first)
|
||||
mock_user_repository.table.update_many = AsyncMock(return_value=None)
|
||||
monkeypatch.setattr(login_module, "UserRepository", lambda client: mock_user_repository)
|
||||
|
||||
with patch.dict(
|
||||
os.environ,
|
||||
{"DATABASE_URL": "postgresql://test:test@localhost/test"},
|
||||
):
|
||||
with (
|
||||
patch( # test-quality-ok: _rehash_password_if_needed reads the proxy_server global
|
||||
"litellm.proxy.proxy_server.prisma_client", mock_prisma_client
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy.auth.login_utils.generate_key_helper_fn",
|
||||
new_callable=AsyncMock,
|
||||
) as mock_generate_key,
|
||||
):
|
||||
mock_generate_key.return_value = {"token": "token-123"}
|
||||
|
||||
result = await authenticate_user(
|
||||
username=user_email,
|
||||
password=password,
|
||||
master_key="sk-1234",
|
||||
prisma_client=mock_prisma_client,
|
||||
throttle=_unlimited_throttle(),
|
||||
general_settings=_POLICY_NO_BREACH_CHECK,
|
||||
)
|
||||
|
||||
assert isinstance(result, LoginResult)
|
||||
assert result.user_id == "test-user-kdf"
|
||||
# The KDF calls made during the async login must not run on the thread
|
||||
# hosting the event loop.
|
||||
loop_thread: Final = threading.get_ident()
|
||||
assert seen_threads.get("verify") is not None, "login never called verify_password"
|
||||
assert seen_threads["verify"] != loop_thread
|
||||
assert seen_threads.get("hash") is not None, "login never rehashed the legacy row"
|
||||
assert seen_threads["hash"] != loop_thread
|
||||
# The rehash still wrote a pbkdf2 row (non-fatal path stays functional).
|
||||
rehash_kwargs = mock_user_repository.table.update_many.call_args.kwargs
|
||||
assert rehash_kwargs["where"] == {"user_id": "test-user-kdf", "password": stored_password}
|
||||
assert rehash_kwargs["data"]["password"].startswith("pbkdf2:sha256:")
|
||||
|
||||
|
||||
class TestEncodeUiSessionJwt:
|
||||
"""The UI session cookie must carry a bounded exp so it does not stay
|
||||
signature-valid until the master key rotates, and so the session-cookie readers
|
||||
|
|
|
|||
|
|
@ -4795,3 +4795,33 @@ async def test_user_update_without_password_revokes_nothing(_admin_prisma, mocke
|
|||
await _update_single_user_helper(user_request=user_request, user_api_key_dict=admin_caller)
|
||||
|
||||
revoke_mock.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_hash_password_in_dict_runs_kdf_off_event_loop_thread(monkeypatch):
|
||||
"""_hash_password_in_dict runs inside async request handlers (/user/update,
|
||||
invitation onboarding); its PBKDF2 hash is CPU-bound and must not run on the
|
||||
event loop thread."""
|
||||
import threading
|
||||
|
||||
from litellm.proxy.management_endpoints import internal_user_endpoints as endpoints_module
|
||||
from litellm.proxy.management_endpoints.internal_user_endpoints import _hash_password_in_dict
|
||||
|
||||
seen_thread: dict[str, int] = {}
|
||||
real_hash = endpoints_module.hash_password
|
||||
|
||||
def recording_hash(password: str) -> str:
|
||||
seen_thread.setdefault("hash", threading.get_ident())
|
||||
return real_hash(password)
|
||||
|
||||
monkeypatch.setattr(endpoints_module, "hash_password", recording_hash)
|
||||
|
||||
data: dict = {"user_id": "u1", "password": "some-new-password"}
|
||||
await _hash_password_in_dict(data, general_settings={}, password_prevalidated=True)
|
||||
|
||||
assert data["password"].startswith("pbkdf2:sha256:")
|
||||
assert data["password_reset_required"] is True
|
||||
assert data["last_breach_check_at"] is None
|
||||
loop_thread: Final = threading.get_ident()
|
||||
assert seen_thread.get("hash") is not None, "_hash_password_in_dict never hashed the password"
|
||||
assert seen_thread["hash"] != loop_thread
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ HIBP is served by an AsyncHTTPHandler wrapping an httpx.MockTransport that is
|
|||
injected straight into change_password; no test here touches the network.
|
||||
"""
|
||||
|
||||
import base64
|
||||
import hashlib
|
||||
from typing import Final
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
|
@ -32,6 +33,12 @@ def _make_user_row(password: str | None) -> MagicMock:
|
|||
return user
|
||||
|
||||
|
||||
def _legacy_scrypt_row(password: str) -> str:
|
||||
salt = b"\x01" * 16
|
||||
derived = hashlib.scrypt(password.encode(), salt=salt, n=16384, r=8, p=1, dklen=32)
|
||||
return "scrypt:" + base64.b64encode(salt + derived).decode()
|
||||
|
||||
|
||||
def _make_prisma(user: MagicMock | None) -> MagicMock:
|
||||
prisma = MagicMock()
|
||||
prisma.db.litellm_usertable.find_first = AsyncMock(return_value=user)
|
||||
|
|
@ -133,6 +140,57 @@ async def test_change_password_rejects_wrong_current_password():
|
|||
prisma.db.litellm_usertable.update.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_change_password_kdf_runs_off_event_loop_thread(monkeypatch):
|
||||
"""The change-password endpoint's verify/hash are CPU-bound PBKDF2 calls
|
||||
(~240ms each); they must run on a worker thread, not the event loop thread
|
||||
that serves the rest of the proxy."""
|
||||
import threading
|
||||
|
||||
from litellm.proxy._types import ChangePasswordRequest
|
||||
from litellm.proxy.management_endpoints import password_endpoints as endpoints_module
|
||||
|
||||
seen_threads: dict[str, int] = {}
|
||||
real_verify = endpoints_module.verify_password
|
||||
real_hash = endpoints_module.hash_password
|
||||
|
||||
def recording_verify(password: str, stored: str) -> bool:
|
||||
seen_threads.setdefault("verify", threading.get_ident())
|
||||
return real_verify(password, stored)
|
||||
|
||||
def recording_hash(password: str) -> str:
|
||||
seen_threads.setdefault("hash", threading.get_ident())
|
||||
return real_hash(password)
|
||||
|
||||
monkeypatch.setattr(endpoints_module, "verify_password", recording_verify)
|
||||
monkeypatch.setattr(endpoints_module, "hash_password", recording_hash)
|
||||
|
||||
prisma = _make_prisma(_make_user_row(_legacy_scrypt_row(CURRENT_PASSWORD)))
|
||||
|
||||
with (
|
||||
patch( # test-quality-ok: change_password reads proxy_server module globals; no injection seam
|
||||
"litellm.proxy.proxy_server.prisma_client", prisma
|
||||
),
|
||||
patch( # test-quality-ok: change_password reads proxy_server module globals; no injection seam
|
||||
"litellm.proxy.proxy_server.general_settings", _POLICY_NO_BREACH_CHECK
|
||||
),
|
||||
):
|
||||
response = await change_password(
|
||||
data=ChangePasswordRequest(current_password=CURRENT_PASSWORD, new_password=NEW_PASSWORD),
|
||||
user_api_key_dict=_caller(),
|
||||
hibp_client=_hibp_client_never_called(),
|
||||
)
|
||||
|
||||
assert response.user_id == "user-123"
|
||||
loop_thread: Final = threading.get_ident()
|
||||
assert seen_threads.get("verify") is not None, "change_password never called verify_password"
|
||||
assert seen_threads["verify"] != loop_thread
|
||||
assert seen_threads.get("hash") is not None, "change_password never hashed the new password"
|
||||
assert seen_threads["hash"] != loop_thread
|
||||
stored = prisma.db.litellm_usertable.update.call_args.kwargs["data"]["password"]
|
||||
assert verify_password(NEW_PASSWORD, stored)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_change_password_rejects_unchanged_password():
|
||||
from litellm.proxy._types import ChangePasswordRequest
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue