lekss361/gendesign

Fork 0

tradein/auth: хвост после #2712 — насыщение без следа в аудите и с логом, выселяющим прочие логи; гейт после выборки из БД; правило только в коммент… #2715

New issue

Closed

opened 2026-08-06 08:54:30 +00:00 by bot-backend · 2 comments

bot-backend commented

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

Collaborator

Хвост глубокого ревью PR #2712 (issue #2665). Ничего блокирующего, но каждый пункт — из тех, что обнаруживаются только в момент, когда они уже мешают.

1. Атака не оставляет следа в аудите, а единственный след выселяет все остальные логи

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

Но тогда весь инцидент виден только в logger.warning, а он:

пишется на каждый отклонённый запрос, без ограничения темпа;
делит с остальным бэкендом бюджет json-file max-size 20m, max-file 3 (проверено docker inspect tradein-backend).

При флуде в сотни запросов в секунду 60 МБ прокручиваются за минуты и выселяют все прочие логи backend ровно во время атаки — то есть в момент, когда они нужнее всего.

Предлагается: агрегировать (не чаще раза в секунду, с числом отклонённых за окно) либо одно событие login_verify_saturated на окно. Второе ещё и даёт срабатывание по уже существующему каналу — правда, только после того, как будет задан получатель (#2673).

2. Гейт насыщения стоит ПОСЛЕ похода в базу

app/api/v1/auth.py:254 — SELECT по имени, :263 — гейт. Каждый заведомо отклоняемый запрос всё равно берёт соединение из пула и делает выборку.

Два следствия:

Побочный канал по времени. Под насыщением ответ возвращается примерно за 1 мс, и единственная работа в нём — выборка по имени. Выравнивателя (bcrypt, 275 мс) на этом пути нет, значит разница «строка найдена / не найдена» ничем не замаскирована. Практически не эксплуатируется — сетевой джиттер на порядки больше, — но модульный docstring auth.py:8-23 целиком построен на том, что такой разницы не бывает. Либо закрыть, либо оговорить.
Лишний чекаут пула и лишняя выборка ровно тогда, когда система уже перегружена.

Правится дёшево и без нарушения инварианта «одна точка выноса = одна точка учёта»: дешёвый предчек над SELECT, авторитетная проверка остаётся внутри verify_password_bounded.

3. Правило живёт только в комментарии, и соседний файл его уже нарушает

hash_password — тот же bcrypt cost 12, те же ~275 мс — остался синхронным внутри async def: app/api/v1/team.py:444 (create_employee) и :568 (update_employee). Поверхности для атаки там нет (ручки аутентифицированные и редкие), поэтому в PR #2712 это сознательно не трогали.

Но правило в docstring password.py:9 («из async def зови ТОЛЬКО verify_password_bounded») о хешировании молчит — и формально соседний файл его нарушает. Это надо либо оговорить в тексте, либо сделать проверяемым.

Сделать проверяемым дешевле, и в репозитории уже есть прецедент такого теста (backend/tests/sql/test_auth_sql_migrations.py): grep-guard «verify_password( не встречается в app/ вне app/core/password.py». Без него синхронная функция остаётся публичной и импортируемой, и asyncio.to_thread(verify_password, ...) в будущем коде даст вынос вообще без учёта слотов.

4. Утечка слота при закрытом событийном цикле — ловушка для тестов, не для прода

app/core/password.py:167: except RuntimeError глотает отказ call_soon_threadsafe. Если цикл закрыт, пока работа в пуле, слот не освобождается никогда.

На проде недостижимо — цикл закрывается только со смертью процесса (docker top показал один uvicorn на контейнер). Но pytest-asyncio в режиме asyncio_mode=auto даёт по циклу на тест, а счётчик занятых слотов — глобал модуля. Один такой тест навсегда уменьшает бюджет всем последующим: при max_inflight=4 четыре штуки дают вечный 429 и совершенно непонятную красноту где-то дальше по файлу.

Просится autouse-fixture с проверкой «слотов занято 0» в teardown у всех тестов, трогающих вход.

Связано: #2665, #2712, #2714, #2673.

Хвост глубокого ревью PR #2712 (issue #2665). Ничего блокирующего, но каждый пункт — из тех, что обнаруживаются только в момент, когда они уже мешают. ## 1. Атака не оставляет следа в аудите, а единственный след выселяет все остальные логи За весь флуд в `user_events` попало **одно** событие, и то от постороннего запроса. Событие на пути отказа при насыщении не пишется сознательно — «это не попытка входа», и тратить бюджет неудач тут действительно нельзя, иначе насыщением можно заблокировать чужую учётную запись. Но тогда весь инцидент виден только в `logger.warning`, а он: - пишется на **каждый** отклонённый запрос, без ограничения темпа; - делит с остальным бэкендом бюджет `json-file max-size 20m, max-file 3` (проверено `docker inspect tradein-backend`). При флуде в сотни запросов в секунду 60 МБ прокручиваются за минуты и **выселяют все прочие логи backend ровно во время атаки** — то есть в момент, когда они нужнее всего. Предлагается: агрегировать (не чаще раза в секунду, с числом отклонённых за окно) либо одно событие `login_verify_saturated` на окно. Второе ещё и даёт срабатывание по уже существующему каналу — правда, только после того, как будет задан получатель (#2673). ## 2. Гейт насыщения стоит ПОСЛЕ похода в базу `app/api/v1/auth.py:254` — SELECT по имени, `:263` — гейт. Каждый заведомо отклоняемый запрос всё равно берёт соединение из пула и делает выборку. Два следствия: - **Побочный канал по времени.** Под насыщением ответ возвращается примерно за 1 мс, и единственная работа в нём — выборка по имени. Выравнивателя (bcrypt, 275 мс) на этом пути нет, значит разница «строка найдена / не найдена» ничем не замаскирована. Практически не эксплуатируется — сетевой джиттер на порядки больше, — но модульный docstring `auth.py:8-23` целиком построен на том, что такой разницы не бывает. Либо закрыть, либо оговорить. - **Лишний чекаут пула и лишняя выборка ровно тогда, когда система уже перегружена.** Правится дёшево и без нарушения инварианта «одна точка выноса = одна точка учёта»: дешёвый предчек над SELECT, авторитетная проверка остаётся внутри `verify_password_bounded`. ## 3. Правило живёт только в комментарии, и соседний файл его уже нарушает `hash_password` — тот же bcrypt cost 12, те же ~275 мс — остался синхронным внутри `async def`: `app/api/v1/team.py:444` (`create_employee`) и `:568` (`update_employee`). Поверхности для атаки там нет (ручки аутентифицированные и редкие), поэтому в PR #2712 это сознательно не трогали. Но правило в docstring `password.py:9` («из `async def` зови ТОЛЬКО `verify_password_bounded`») о хешировании молчит — и формально соседний файл его нарушает. Это надо либо оговорить в тексте, либо сделать проверяемым. **Сделать проверяемым дешевле**, и в репозитории уже есть прецедент такого теста (`backend/tests/sql/test_auth_sql_migrations.py`): grep-guard «`verify_password(` не встречается в `app/` вне `app/core/password.py`». Без него синхронная функция остаётся публичной и импортируемой, и `asyncio.to_thread(verify_password, ...)` в будущем коде даст вынос вообще без учёта слотов. ## 4. Утечка слота при закрытом событийном цикле — ловушка для тестов, не для прода `app/core/password.py:167`: `except RuntimeError` глотает отказ `call_soon_threadsafe`. Если цикл закрыт, пока работа в пуле, слот не освобождается никогда. На проде недостижимо — цикл закрывается только со смертью процесса (`docker top` показал один uvicorn на контейнер). Но `pytest-asyncio` в режиме `asyncio_mode=auto` даёт **по циклу на тест**, а счётчик занятых слотов — глобал модуля. Один такой тест навсегда уменьшает бюджет всем последующим: при `max_inflight=4` четыре штуки дают вечный 429 и совершенно непонятную красноту где-то дальше по файлу. Просится autouse-fixture с проверкой «слотов занято 0» в teardown у всех тестов, трогающих вход. Связано: #2665, #2712, #2714, #2673.

Light1YT referenced this issue from a commit

2026-08-06 12:38:09 +00:00

fix(tradein/auth): отказ по насыщению — до выборки из БД и с агрегированным следом (#2715)

bot-backend referenced this issue

2026-08-06 12:38:42 +00:00

fix(tradein/auth): отказ по насыщению — до выборки из БД и с агрегированным следом (#2715) #2734

Light1YT referenced this issue from a commit

2026-08-06 12:40:00 +00:00

test(tradein/auth): правило про синхронную сверку — сторожем, а не комментарием (#2715)

bot-backend referenced this issue

2026-08-06 12:40:17 +00:00

test(tradein/auth): правило про синхронную сверку — сторожем, а не комментарием (#2715) #2735

Light1YT referenced this issue from a commit

2026-08-06 12:46:54 +00:00

fix(tradein/auth): агрегированный отказ пишется на ERROR — иначе канала нет (#2715)

bot-backend commented

2026-08-06 12:51:43 +00:00

Author

Collaborator

Работаю над этим в PR #2734 (пункты 1-2) и PR #2735 (пункт 3). Оба зелёные, не мержу — ждут глубокого ревью.

п.1 — на окно 1с одна запись в лог (уровень ERROR, чтобы становиться событием GlitchTip) и одно событие login_verify_saturated в user_events, обе с числом отказов с прошлой записи. Замер флудом 100×1с: было 1634 строки на 1634 отказа, стало 1. Получателей у GlitchTip по-прежнему нет (#2673) — доставку проверить не на чем, и это записано в docstring, а не подразумевается.
п.2 — verify_slots_saturated(key) вызывается до get_user_by_username; авторитетная проверка осталась внутри verify_password_bounded и зовёт эту же функцию. Выборок из реестра на отклонённом пути: было 1634, стало 0.
п.3 — сторож по AST (не grep: verify_password поминается в комментариях, а форму to_thread(verify_password, …) без скобок grep не ловит вовсе). Детектор проверен на себе.
п.4 — правки не нужно: autouse-фикстура из #2717 этот путь покрывает. Пробником проверено — тест, отдающий освобождение слота в закрытый цикл, падает в teardown с указанием виновника, а следующий тест видит чистое состояние.

Попутно опровергнута одна предпосылка issue: миграция для нового event_type не требуется — user_events.event_type это свободный text без CHECK (проверено на проде).

Работаю над этим в PR #2734 (пункты 1-2) и PR #2735 (пункт 3). Оба зелёные, **не мержу** — ждут глубокого ревью. - **п.1** — на окно 1с одна запись в лог (уровень ERROR, чтобы становиться событием GlitchTip) и одно событие `login_verify_saturated` в `user_events`, обе с числом отказов с прошлой записи. Замер флудом 100×1с: было 1634 строки на 1634 отказа, стало 1. Получателей у GlitchTip по-прежнему нет (#2673) — доставку проверить не на чем, и это записано в docstring, а не подразумевается. - **п.2** — `verify_slots_saturated(key)` вызывается до `get_user_by_username`; авторитетная проверка осталась внутри `verify_password_bounded` и зовёт эту же функцию. Выборок из реестра на отклонённом пути: было 1634, стало 0. - **п.3** — сторож по AST (не grep: `verify_password` поминается в комментариях, а форму `to_thread(verify_password, …)` без скобок grep не ловит вовсе). Детектор проверен на себе. - **п.4** — правки не нужно: autouse-фикстура из #2717 этот путь покрывает. Пробником проверено — тест, отдающий освобождение слота в закрытый цикл, падает в teardown с указанием виновника, а следующий тест видит чистое состояние. Попутно опровергнута одна предпосылка issue: миграция для нового `event_type` не требуется — `user_events.event_type` это свободный `text` без CHECK (проверено на проде).

Light1YT referenced this issue from a commit

2026-08-06 14:20:23 +00:00

test(tradein/auth): сторож не должен зеленеть от съехавшей области (#2715)

Light1YT referenced this issue from a commit

2026-08-06 14:22:46 +00:00

fix(tradein/auth): хвост счётчика датируется честно, безымянное событие — не аккаунт (#2715)

bot-backend referenced this issue from a commit

2026-08-06 14:27:29 +00:00

fix(tradein/auth): отказ по насыщению — до выборки из БД и с агрегированным следом (#2715) (#2734)

bot-backend referenced this issue from a commit

2026-08-06 14:35:35 +00:00

test(tradein/auth): правило про синхронную сверку — сторожем, а не комментарием (#2715) (#2735)

bot-backend referenced this issue

2026-08-06 21:19:46 +00:00

fix(tradein/devops): логи в journald — переживают пересоздание контейнера (#2741) #2758

bot-backend commented

2026-08-07 09:10:09 +00:00

Author

Collaborator

ЗАКРЫТО — все четыре пункта, проверка на проде 2026-08-07 09:0x UTC

п.1 — след в аудите есть, и он ЖИВОЙ, а не только в тесте

Не пришлось верить замеру флудом: событие уже сработало на проде.

user_events, единственная строка login_verify_saturated:
  id 3372 · 2026-08-06 14:34:33 UTC · ip 127.0.0.1 · POST /api/v1/auth/login
  payload {"rejected": 1, "since_prev_s": null}

since_prev_s = null — первый отказ отчитан сразу, а не в конце окна, ровно как записано в docstring. Событий за сутки: api_request 978, login_failed 352, login_verify_saturated 1. То есть насыщение больше не невидимо, и след пережил и ротацию логов, и сегодняшние пересоздания контейнера.

Опровергнутая предпосылка задачи подтвердилась замером: user_events вообще не несёт CHECK-ограничений (pg_constraint → только PRIMARY KEY), миграция под новый event_type была не нужна.

п.2 — гейт стоит ДО выборки

Живой tradein-backend, app/api/v1/auth.py:

385:    if verify_slots_saturated(ip):
388:    user = get_user_by_username(db, body.username)

Отклонённый запрос больше не берёт соединение из пула и не делает выборку по имени — значит и побочный канал по времени, на отсутствии которого построен модульный docstring, закрыт.

п.3 — сторож проверен НА СЛОМ, а не на наличие

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

Скопировал app/ целиком, положил внутрь файл-нарушитель с формой, которую grep 'verify_password(' не поймал бы вовсе:

return await asyncio.to_thread(verify_password, plain, hashed)

Результат: test_sync_verify_password_is_called_from_one_place_only покраснел, назвав файл — ['app/api/v1/_violator.py']. На чистом дереве он зелёный. То есть сторож различает эти два состояния, а не подтверждает сам себя.

Отдельно ценно, что тест держит исполняемыми и свои слепые зоны (getattr-доступ, прямой bcrypt.checkpw), и проверяет, что область сканирования не съехала (_OWNER.exists()) — без этого rglob по несуществующему каталогу дал бы вечно зелёного сторожа на пустоте.

п.4 — не требовал правки

Autouse-фикстура _no_leaked_password_verify_slots (tests/conftest.py:26) пришла с #2717 и покрывает утечку слота при закрытом цикле. Ноль здесь имеет причину «уже починено выше по течению», а не «неприменимо».

PR: #2734 (п.1-2), #2735 (п.3).

## ЗАКРЫТО — все четыре пункта, проверка на проде 2026-08-07 09:0x UTC ### п.1 — след в аудите есть, и он ЖИВОЙ, а не только в тесте Не пришлось верить замеру флудом: событие уже сработало на проде. ``` user_events, единственная строка login_verify_saturated: id 3372 · 2026-08-06 14:34:33 UTC · ip 127.0.0.1 · POST /api/v1/auth/login payload {"rejected": 1, "since_prev_s": null} ``` `since_prev_s = null` — первый отказ отчитан сразу, а не в конце окна, ровно как записано в docstring. Событий за сутки: `api_request` 978, `login_failed` 352, `login_verify_saturated` 1. То есть насыщение больше не невидимо, и след пережил и ротацию логов, и сегодняшние пересоздания контейнера. Опровергнутая предпосылка задачи подтвердилась замером: `user_events` вообще не несёт CHECK-ограничений (`pg_constraint` → только PRIMARY KEY), миграция под новый `event_type` была не нужна. ### п.2 — гейт стоит ДО выборки Живой `tradein-backend`, `app/api/v1/auth.py`: ``` 385: if verify_slots_saturated(ip): 388: user = get_user_by_username(db, body.username) ``` Отклонённый запрос больше не берёт соединение из пула и не делает выборку по имени — значит и побочный канал по времени, на отсутствии которого построен модульный docstring, закрыт. ### п.3 — сторож проверен НА СЛОМ, а не на наличие Это главное, что я обязан был проверить: сторож в этом репозитории уже ловили на тавтологии. Здесь её нет — проверил сам, независимо от авторского самотеста. Скопировал `app/` целиком, положил внутрь файл-нарушитель с формой, которую `grep 'verify_password('` не поймал бы вовсе: ```python return await asyncio.to_thread(verify_password, plain, hashed) ``` Результат: `test_sync_verify_password_is_called_from_one_place_only` **покраснел**, назвав файл — `['app/api/v1/_violator.py']`. На чистом дереве он зелёный. То есть сторож различает эти два состояния, а не подтверждает сам себя. Отдельно ценно, что тест держит исполняемыми и свои слепые зоны (`getattr`-доступ, прямой `bcrypt.checkpw`), и проверяет, что область сканирования не съехала (`_OWNER.exists()`) — без этого `rglob` по несуществующему каталогу дал бы вечно зелёного сторожа на пустоте. ### п.4 — не требовал правки Autouse-фикстура `_no_leaked_password_verify_slots` (`tests/conftest.py:26`) пришла с #2717 и покрывает утечку слота при закрытом цикле. Ноль здесь имеет причину «уже починено выше по течению», а не «неприменимо». PR: #2734 (п.1-2), #2735 (п.3).

bot-backend closed this issue

2026-08-07 09:10:26 +00:00