lekss361/gendesign

Fork 0

tradein/auth: усилить защиту логина от перебора после снятия basic_auth #2571

New issue

Closed

opened 2026-07-30 21:17:56 +00:00 by lekss361 · 3 comments

lekss361 commented

2026-07-30 21:17:56 +00:00

Owner

Найдено deep-review PR #2569 (cutover, эпик #2549). 🟡 Не блокер, но актуально сразу после снятия basic_auth.

Проблема

До cutover форма входа была прикрыта Caddy basic_auth — снаружи до неё было не достучаться. После снятия POST /trade-in/api/v1/auth/login становится единственным публичным эндпоинтом, доступным из интернета без всяких кредов.

Текущий лимит (_LOGIN_LIMITER): 5 попыток / 300с с ключом f"{len(username)}:{username}:{ip}" — то есть на пару (пользователь, IP). Глобального потолка на username нет, поэтому распределённый перебор (credential stuffing с множества адресов) получает по 5 попыток с каждого источника.

Хорошая новость: _client_ip (ratelimit.py:140-153) берёт правый hop из XFF, а за Caddy ровно один прокси-хоп — значит ключ не подделывается заголовком.

Что сделать (варианты)

Глобальный лимит попыток на username поверх per-IP (например, 20/час независимо от источника) с логированием в user_events.
Экспоненциальная задержка/блокировка учётки после N неудач подряд, снимаемая менеджером через «Команду» или по таймауту.
Опционально: CAPTCHA/PoW после превышения порога.

Учесть: блокировка по username — вектор DoS против конкретного пользователя, поэтому лучше замедление, а не жёсткая блокировка.

DoD

Тесты: распределённый перебор (много IP, один username) упирается в глобальный потолок; легитимный пользователь с опечаткой не блокируется надолго; события неудачных входов видны в аудите.

Найдено deep-review PR #2569 (cutover, эпик #2549). 🟡 Не блокер, но актуально сразу после снятия basic_auth. ## Проблема До cutover форма входа была прикрыта Caddy basic_auth — снаружи до неё было не достучаться. После снятия `POST /trade-in/api/v1/auth/login` становится единственным публичным эндпоинтом, доступным из интернета без всяких кредов. Текущий лимит (`_LOGIN_LIMITER`): 5 попыток / 300с с ключом `f"{len(username)}:{username}:{ip}"` — то есть **на пару (пользователь, IP)**. Глобального потолка на username нет, поэтому распределённый перебор (credential stuffing с множества адресов) получает по 5 попыток с каждого источника. Хорошая новость: `_client_ip` (`ratelimit.py:140-153`) берёт **правый** hop из XFF, а за Caddy ровно один прокси-хоп — значит ключ не подделывается заголовком. ## Что сделать (варианты) - Глобальный лимит попыток на username поверх per-IP (например, 20/час независимо от источника) с логированием в `user_events`. - Экспоненциальная задержка/блокировка учётки после N неудач подряд, снимаемая менеджером через «Команду» или по таймауту. - Опционально: CAPTCHA/PoW после превышения порога. Учесть: блокировка по username — вектор DoS против конкретного пользователя, поэтому лучше замедление, а не жёсткая блокировка. ## DoD Тесты: распределённый перебор (много IP, один username) упирается в глобальный потолок; легитимный пользователь с опечаткой не блокируется надолго; события неудачных входов видны в аудите.

bot-backend commented

2026-08-05 17:17:32 +00:00

Collaborator

Уточнение по срочности: это уже не «актуально после снятия basic_auth», это актуально сейчас.

Проверил боевой конфиг Caddy в контейнере (не репозиторий, а то, что реально работает): секция /trade-in/* вынесена выше импорта авторизации — как и задумано в #2558, у трейд-ина своя форма входа поверх RBAC, а legacy-basic_auth гейтит только Site Finder. То есть POST /trade-in/api/v1/auth/login уже сейчас доступен из интернета без единого креда, и описанная в issue дыра открыта: распределённый перебор получает по 5 попыток с каждого адреса, глобального потолка на имя пользователя нет.

Работу начал. Дизайн выбран по твоему же предупреждению из issue — замедление, а не блокировка: жёсткая блокировка по имени пользователя даёт злоумышленнику способ выключить чужую учётку, что хуже проблемы, которую лечит.

Отдельный пункт, который стоит держать в голове при ревью: замедление само по себе может стать оракулом существования учётки. Если задержка применяется только к существующим пользователям, злоумышленник узнаёт валидные логины по времени ответа, не зная ни одного пароля. Явно поставил это требованием.

Уточнение по срочности: **это уже не «актуально после снятия basic_auth», это актуально сейчас.** Проверил боевой конфиг Caddy в контейнере (не репозиторий, а то, что реально работает): секция `/trade-in/*` вынесена **выше** импорта авторизации — как и задумано в #2558, у трейд-ина своя форма входа поверх RBAC, а legacy-basic_auth гейтит только Site Finder. То есть `POST /trade-in/api/v1/auth/login` **уже сейчас** доступен из интернета без единого креда, и описанная в issue дыра открыта: распределённый перебор получает по 5 попыток с каждого адреса, глобального потолка на имя пользователя нет. Работу начал. Дизайн выбран по твоему же предупреждению из issue — **замедление, а не блокировка**: жёсткая блокировка по имени пользователя даёт злоумышленнику способ выключить чужую учётку, что хуже проблемы, которую лечит. Отдельный пункт, который стоит держать в голове при ревью: замедление само по себе может стать оракулом существования учётки. Если задержка применяется только к существующим пользователям, злоумышленник узнаёт валидные логины по времени ответа, не зная ни одного пароля. Явно поставил это требованием.

Light1YT referenced this issue from a commit

2026-08-05 17:38:33 +00:00

feat(tradein/auth): глобальный потолок попыток входа на имя пользователя (#2571)

bot-backend referenced this issue

2026-08-05 17:40:59 +00:00

feat(tradein/auth): глобальный потолок попыток входа на имя пользователя (#2571) #2663

bot-backend commented

2026-08-05 17:41:20 +00:00

Collaborator

Working on this in PR #2663.

Кратко, что сделано: глобальный счётчик неудач на ИМЯ (без IP в ключе, дефолт 20/час) поверх существующего per-IP лимита; превышение порога растит задержку ответа (удвоение от 1с до потолка 8с), а не блокирует учётку — блокировка по имени была бы вектором DoS против конкретного человека. Порог/окно/потолок — в настройках.

Оракул существования учётки решён структурно: все ветки отказа по кредам сведены в один хвост, счётчик ведётся по ПРИСЛАННОМУ имени без проверки в реестре, поэтому несуществующее имя тормозит так же, как живое.

Честно про потолок защиты: задержка — это латентность ответа, а не потолок пропускной способности. Атакующий с сотнями одновременных соединений отспит их параллельно. Настоящий потолок темпа требует сериализации попыток на имя, а она возвращает тот самый DoS против владельца имени, от которого issue сознательно уходит. Подробности и два возможных follow-up — в описании PR.

Working on this in PR #2663. Кратко, что сделано: глобальный счётчик неудач на ИМЯ (без IP в ключе, дефолт 20/час) поверх существующего per-IP лимита; превышение порога растит задержку ответа (удвоение от 1с до потолка 8с), а не блокирует учётку — блокировка по имени была бы вектором DoS против конкретного человека. Порог/окно/потолок — в настройках. Оракул существования учётки решён структурно: все ветки отказа по кредам сведены в один хвост, счётчик ведётся по ПРИСЛАННОМУ имени без проверки в реестре, поэтому несуществующее имя тормозит так же, как живое. Честно про потолок защиты: задержка — это латентность ответа, а не потолок пропускной способности. Атакующий с сотнями одновременных соединений отспит их параллельно. Настоящий потолок темпа требует сериализации попыток на имя, а она возвращает тот самый DoS против владельца имени, от которого issue сознательно уходит. Подробности и два возможных follow-up — в описании PR.

bot-backend referenced this issue

2026-08-05 17:47:40 +00:00

[HIGH] tradein/auth: bcrypt блокирует единственный event loop — поток логинов кладёт весь API, и он же сейчас единственный настоящий ограничитель темпа #2665

Light1YT referenced this issue from a commit

2026-08-05 18:14:21 +00:00

fix(tradein/auth): не ронять и не занимать пул на замедлении входа (#2571)

bot-backend referenced this issue from a commit

2026-08-05 18:31:29 +00:00

Merge pull request 'feat(tradein/auth): глобальный потолок попыток входа на имя пользователя (#2571)' (#2663) from feat/2571-login-throttle into main

bot-backend commented

2026-08-05 18:37:34 +00:00

Collaborator

Сделано — PR #2663 смержен и проверен на проде живым запросом.

Прод

Код в образе: глобальный счётчик на имя, защита от переполнения, закрытие сессии перед задержкой, ограничение длины имени — все маркеры на месте.

Живая проба (один запрос с заведомо несуществующим именем, перебор не устраивал):

HTTP 401, время ответа 0.333 с

event_type   | username              | payload
login_failed | zzz-nonexistent-probe | {"throttle_delay_s": 0.0, "username_fails_in_window": 1}

Третья часть аудита появилась там, где её раньше не было: распределённый перебор перестаёт выглядеть россыпью одиночных неудач.

Обрати внимание на 0.333 секунды. Имя не существует, но ответ занял ровно столько же, сколько занял бы реальный пароль — то есть сравнение с фиктивным хешем действительно выполняется, и утечки существования учётки по времени нет. Это как раз то, что легко сломать неаккуратной правкой. throttle_delay_s: 0.0 тоже правильно: одна неудача ниже порога, замедления быть не должно.

Что нашло ревью и что было исправлено до мержа

Первая версия добавляла на публичный вход два новых способа положить сервис.

Переполнение отключало защиту саму себя. Расчёт задержки считал двойку в степени числа неудач без ограничения показателя, а min() вычисляет оба аргумента до сравнения. При 1045 неудачах по одному имени за час — это 0.29 запроса в секунду, то есть триста адресов по пять попыток даже не задевают лимит по адресу — арифметика переполнялась, и с этой попытки каждая следующая отдавала 500 вместо 401 мгновенно, без задержки и без записи в аудит. Обе ценности правки исчезали ровно тогда, когда атака реальна.

Задержка держала соединение из пула базы. Сон выполнялся внутри области жизни зависимости, а это та же сессия, что у основного движка, и запрос к пользователю уже открыл транзакцию. Порядка пятнадцати одновременно спящих попыток выбирали пул целиком — после этого падал любой эндпоинт. Мы отвергли жёсткую блокировку как отказ в обслуживании против владельца имени и чуть не привезли отказ против всех сразу.

Оба исправлены, оба покрыты тестами, которые проверяют не факт вызова, а порядок (между закрытием сессии и концом ответа лежит вся задержка).

Порог проверен по живым данным

Он был выбран умозрительно, поэтому измерил: 55 событий неудачного входа за всё время, 35 пар «пользователь × час», максимум 6 неудач за час у одного имени, p95 = 5, случаев выше порога ноль. Живого пользователя замедление не поймает. Оговорка: выборка маленькая, продукт до публичного запуска — когда пойдёт трафик, распределение стоит пересмотреть.

Честная оценка того, что получилось

Это аудит-сигнал плюс трение, а не потолок темпа. Счётчик инкрементируется до задержки, ничто не сериализует попытки по имени, а asyncio.sleep отпускает событийный цикл — значит атакующий с сотнями соединений отспит их параллельно. Потолок равен числу его соединений делённому на задержку, то есть выбирается им, а не нами. Я в переписке называл это «глобальным потолком» — формулировка была сильнее реальности.

Настоящий потолок упирается в #2665: сегодня темп ограничивает случайность — синхронный bcrypt блокирует событийный цикл и тем сериализует попытки, ценой того, что поток логинов кладёт весь API. Чинить это надо одним заходом с настоящим потолком, иначе вынос bcrypt из цикла просто ускорит перебор.

Семафор на имя рассматривали и отвергли: он даёт настоящий потолок, но создаёт очередь, где легитимный владелец имени ждёт за спинами атакующих — тот же отказ в обслуживании против человека, просто в форме «вход не открывается» вместо «вход отключён».

Сделано — PR #2663 смержен и проверен на проде живым запросом. ## Прод Код в образе: глобальный счётчик на имя, защита от переполнения, закрытие сессии перед задержкой, ограничение длины имени — все маркеры на месте. Живая проба (**один** запрос с заведомо несуществующим именем, перебор не устраивал): ``` HTTP 401, время ответа 0.333 с ``` ``` event_type | username | payload login_failed | zzz-nonexistent-probe | {"throttle_delay_s": 0.0, "username_fails_in_window": 1} ``` Третья часть аудита появилась там, где её раньше не было: распределённый перебор перестаёт выглядеть россыпью одиночных неудач. **Обрати внимание на 0.333 секунды.** Имя не существует, но ответ занял ровно столько же, сколько занял бы реальный пароль — то есть сравнение с фиктивным хешем действительно выполняется, и **утечки существования учётки по времени нет**. Это как раз то, что легко сломать неаккуратной правкой. `throttle_delay_s: 0.0` тоже правильно: одна неудача ниже порога, замедления быть не должно. ## Что нашло ревью и что было исправлено до мержа Первая версия добавляла на публичный вход **два новых способа положить сервис**. **Переполнение отключало защиту саму себя.** Расчёт задержки считал двойку в степени числа неудач без ограничения показателя, а `min()` вычисляет оба аргумента до сравнения. При 1045 неудачах по одному имени за час — это 0.29 запроса в секунду, то есть триста адресов по пять попыток даже не задевают лимит по адресу — арифметика переполнялась, и с этой попытки каждая следующая отдавала 500 вместо 401 **мгновенно, без задержки и без записи в аудит**. Обе ценности правки исчезали ровно тогда, когда атака реальна. **Задержка держала соединение из пула базы.** Сон выполнялся внутри области жизни зависимости, а это та же сессия, что у основного движка, и запрос к пользователю уже открыл транзакцию. Порядка пятнадцати одновременно спящих попыток выбирали пул целиком — после этого падал **любой** эндпоинт. Мы отвергли жёсткую блокировку как отказ в обслуживании против владельца имени и чуть не привезли отказ против всех сразу. Оба исправлены, оба покрыты тестами, которые проверяют не факт вызова, а **порядок** (между закрытием сессии и концом ответа лежит вся задержка). ## Порог проверен по живым данным Он был выбран умозрительно, поэтому измерил: 55 событий неудачного входа за всё время, 35 пар «пользователь × час», максимум **6** неудач за час у одного имени, p95 = 5, случаев выше порога **ноль**. Живого пользователя замедление не поймает. Оговорка: выборка маленькая, продукт до публичного запуска — когда пойдёт трафик, распределение стоит пересмотреть. ## Честная оценка того, что получилось **Это аудит-сигнал плюс трение, а не потолок темпа.** Счётчик инкрементируется до задержки, ничто не сериализует попытки по имени, а `asyncio.sleep` отпускает событийный цикл — значит атакующий с сотнями соединений отспит их параллельно. Потолок равен числу его соединений делённому на задержку, то есть выбирается им, а не нами. Я в переписке называл это «глобальным потолком» — формулировка была сильнее реальности. Настоящий потолок упирается в #2665: сегодня темп ограничивает **случайность** — синхронный bcrypt блокирует событийный цикл и тем сериализует попытки, ценой того, что поток логинов кладёт весь API. Чинить это надо одним заходом с настоящим потолком, иначе вынос bcrypt из цикла просто ускорит перебор. Семафор на имя рассматривали и отвергли: он даёт настоящий потолок, но создаёт очередь, где легитимный владелец имени ждёт за спинами атакующих — тот же отказ в обслуживании против человека, просто в форме «вход не открывается» вместо «вход отключён».

bot-backend closed this issue

2026-08-05 18:37:40 +00:00

Light1YT referenced this issue from a commit

2026-08-06 08:16:24 +00:00

fix(tradein/auth): bcrypt вне событийного цикла + настоящий потолок темпа логинов

bot-backend referenced this issue

2026-08-06 08:16:53 +00:00

fix(tradein/auth): bcrypt вне событийного цикла + настоящий потолок темпа логинов (#2665) #2712

bot-backend referenced this issue

2026-08-06 08:53:54 +00:00

[HIGH] tradein/auth: потолок сверок бьёт и по своим — один IP на 5 запросах в секунду закрывает вход всем, бессрочно #2714