From 5d6a42f454dde5234bc1100d8f7a60f67bf41b08 Mon Sep 17 00:00:00 2001 From: yucheng-berri <293926326+yucheng-berri@users.noreply.github.com> Date: Wed, 14 Oct 2026 02:12:20 -0700 Subject: [PATCH] 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. --- litellm/proxy/auth/login_utils.py | 10 +- .../internal_user_endpoints.py | 4 +- .../password_endpoints.py | 8 +- litellm/proxy/proxy_server.py | 3 +- litellm/proxy/utils.py | 4 +- tests/unit/proxy/auth/test_login_utils.py | 96 +++++++++++++++++++ .../test_internal_user_endpoints.py | 30 ++++++ .../test_password_endpoints.py | 58 +++++++++++ 8 files changed, 206 insertions(+), 7 deletions(-) diff --git a/litellm/proxy/auth/login_utils.py b/litellm/proxy/auth/login_utils.py index ef0732221df..2311390ecf8 100644 --- a/litellm/proxy/auth/login_utils.py +++ b/litellm/proxy/auth/login_utils.py @@ -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, diff --git a/litellm/proxy/management_endpoints/internal_user_endpoints.py b/litellm/proxy/management_endpoints/internal_user_endpoints.py index d0b3a08bc77..2087ceee2b8 100644 --- a/litellm/proxy/management_endpoints/internal_user_endpoints.py +++ b/litellm/proxy/management_endpoints/internal_user_endpoints.py @@ -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 diff --git a/litellm/proxy/management_endpoints/password_endpoints.py b/litellm/proxy/management_endpoints/password_endpoints.py index 16f3e7dfcfb..4b0b35b2f08 100644 --- a/litellm/proxy/management_endpoints/password_endpoints.py +++ b/litellm/proxy/management_endpoints/password_endpoints.py @@ -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, } diff --git a/litellm/proxy/proxy_server.py b/litellm/proxy/proxy_server.py index 2e5d4c5aeaa..5ab73146825 100644 --- a/litellm/proxy/proxy_server.py +++ b/litellm/proxy/proxy_server.py @@ -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( diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 0e49428a656..a5b14c6fc6a 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -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" diff --git a/tests/unit/proxy/auth/test_login_utils.py b/tests/unit/proxy/auth/test_login_utils.py index ffafaedbabf..56a1cfe9d4b 100644 --- a/tests/unit/proxy/auth/test_login_utils.py +++ b/tests/unit/proxy/auth/test_login_utils.py @@ -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 diff --git a/tests/unit/proxy/management_endpoints/test_internal_user_endpoints.py b/tests/unit/proxy/management_endpoints/test_internal_user_endpoints.py index 799fe59147c..9adca657c5f 100644 --- a/tests/unit/proxy/management_endpoints/test_internal_user_endpoints.py +++ b/tests/unit/proxy/management_endpoints/test_internal_user_endpoints.py @@ -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 diff --git a/tests/unit/proxy/management_endpoints/test_password_endpoints.py b/tests/unit/proxy/management_endpoints/test_password_endpoints.py index e3bfb12e6ab..e6733c33bb1 100644 --- a/tests/unit/proxy/management_endpoints/test_password_endpoints.py +++ b/tests/unit/proxy/management_endpoints/test_password_endpoints.py @@ -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