Harden input handling and filter non-global DNS addresses

X-API-Key compared as bytes (401 instead of 500 on non-ASCII), strict parsing of the diff "since" parameter (400 instead of 500), FQDN resolution keeps only global addresses (allow_non_global_ips to opt out). Adds the code review report and the plan/summary for this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-21 09:42:51 +03:00
1 parent 02f7b49e12
commit 46d56654d8
@@ -0,0 +1,39 @@
+# План: безопасность ввода и данных (п. 2-4 ревью)
+
+Источник: `docs/review-2026-09-21.md`, находки 2, 3 и 4.
+
+## Цель
+Некорректный ввод не приводит к ответу 500, а адреса, которые не должны попадать в правила файрвола (loopback, частные, `0.0.0.0`), не попадают в выдачу.
+
+## Дизайн
+
+### Находка 2: заголовок `X-API-Key`
+- Сравнение токенов выполняется по байтам (`encode("utf-8")` обеих сторон): `secrets.compare_digest` для `str` принимает только ASCII и падает на других символах.
+- Результат: любой неверный ключ, в том числе нелатинский, даёт `401`; поведение с верным ключом и без токена в окружении (`503`) не меняется.
+
+### Находка 3: параметр `since` (`GET /addresses/diff`)
+- Курсором считается строка из ASCII-цифр длиной до 30 символов (`isascii() and isdigit()`); более длинная или с юникод-цифрами трактуется как некорректное значение.
+- Разбор времени защищён от `OverflowError` (крайние даты с поясом) вместе с `ValueError`.
+- Все некорректные значения дают `400` с прежним текстом; `410` остаётся для корректных, но вышедших за журнал значений (в том числе очень больших курсоров до 30 цифр).
+
+### Находка 4: фильтрация DNS-адресов
+- `FQDNCollector.resolve_fqdn` оставляет только глобальные адреса (`ipaddress.ip_address(...).is_global`; зона IPv6 `%if` отбрасывается перед разбором). Отфильтрованные значения пишутся в лог (WARNING, с перечнем).
+- Если после фильтрации адресов не осталось, поведение как при ошибке DNS: предупреждение, TTL не применяется, данные не удаляются.
+- Обходной путь для внутренних имён: ключ `allow_non_global_ips` в `config.json` (по умолчанию `false`; `true` отключает фильтр).
+- Ранее собранные неглобальные адреса не удаляются сразу: они истекают по `ttl_days` (для немедленной очистки - `DELETE /fqdns/<имя>?purge=true` и повторное добавление).
+
+## Изменения
+1. `api_server.py`: сравнение токена по байтам; `parse_since` (ограничение длины, ASCII-цифры, перехват `OverflowError`).
+2. `cidr_collector.py`: фильтр в `resolve_fqdn`, чтение `allow_non_global_ips`.
+3. `README.md`: ограничения `since` (`400`), фильтрация адресов и ключ `allow_non_global_ips`, ключ в описании `config.json`.
+4. `docs/review-2026-09-21.md`: статусы находок; итоги в `docs/summary-input-hardening.md`.
+5. Тесты (2, всего 22): API (нелатинский ключ даёт 401; `since` с юникод-цифрой, длинным числом и крайней датой даёт 400); сборщик (смесь глобальных, частных, loopback и зональных адресов фильтруется, с `allow_non_global_ips` остаётся всё, если ничего не осталось - пустой результат).
+
+## Не входит
+Находка 1 (пустая выдача после порчи без копий) - отдельное решение по поведению; находки 5-10 - отдельные доработки.
+
+## Проверка
+Тесты в контейнере; вручную в контейнерном окружении повторить запросы из ревью (`since=²`, длинное число, `0001-01-01T00:00:00+05:00`, нелатинский `X-API-Key`), для DNS - сбор на копии с подставленными ответами `getaddrinfo`.
+
+## Откат
+Изменения локальны (две функции и одно чтение конфигурации): возврат к предыдущему коммиту; схема данных не затрагивается.
@@ -0,0 +1,48 @@
+# Ревью кодовой базы (2026-09-21)
+
+**Метод:** структура по графу знаний (405 узлов; код: 7 модулей, 1382 строки), затем чтение кода. Часть находок подтверждена запуском (отмечено). Состояние на момент ревью: коммит `02f7b49`, 20 тестов.
+
+## Структура по графу
+- **Связанность модулей:** `api_server` ↔ `db` (13 связей), `api_server` ↔ `cidr_collector` (11), `cidr_collector` ↔ `collector_daemon` (8). Зависимости направлены в основном одним способом, слои читаемы.
+- **Хабы:** `api_server.py` (посредничество 0,38), `db.py` (0,23), `cidr_collector.py` (0,20).
+- **Циклическая зависимость:** `cidr_collector` импортирует `db`, а `db` берёт пути и настройки из `cidr_collector` отложенным импортом внутри функций.
+- **Мёртвый код не найден.** Узлы кода со степенью не более 1 - обработчики, регистрируемые декораторами, и функции через словарь диспетчеризации (`_frr`, `_mikrotik`): ложные срабатывания.
+
+## Находки
+
+| # | Серьёзность | Находка | Где | Статус |
+|---|---|---|---|---|
+| 1 | **Высокая** | **Пустой список после порчи базы без копий (подтверждено запуском).** Первый запрос даёт 503, затем создаётся новая пустая база, и `/addresses` отвечает `200 []`. Если демон жив, `/health` показывает `ok` при `counts = 0`. Потребители, забирающие список по расписанию, могут стереть свои списки. | `db.py:83-112` | Ждёт решения |
+| 2 | Средняя | **500 вместо 401 при нелатинском `X-API-Key` (подтверждено).** `secrets.compare_digest` для `str` работает только с ASCII. Обхода авторизации нет. | `api_server.py:105` | Исправлено (`summary-input-hardening.md`) |
+| 3 | Средняя | **500 при некорректном `since` (подтверждено):** `since=²` (`str.isdigit()` истинно для символов юникода, а `int()` их не принимает), число длиннее 4300 цифр (лимит `int`), крайние даты с поясом (`0001-01-01T00:00:00+05:00`, `9999-12-31T23:59:59-05:00`: `OverflowError`). | `api_server.py:152` | Исправлено (`summary-input-hardening.md`) |
+| 4 | Средняя | **DNS-адреса без фильтрации (подтверждено).** В списки попадают любые ответы: `127.0.0.1`, `10.x`, `0.0.0.0`. Оставлять нужно только глобальные адреса. | `cidr_collector.py:176-179` | Исправлено (`summary-input-hardening.md`) |
+| 5 | Низкая | `/addresses` открывает три соединения и три разных снимка (курсор, ASN, FQDN): части ответа могут относиться к разным моментам, соединения тратят время на PRAGMA и проверку схемы. Достаточно одной сессии с одной транзакцией. | `api_server.py:133-141` | Не начато |
+| 6 | Низкая | `/health` без авторизации отдаёт внутренние пути и текст ошибки (`last_restore`). | `api_server.py:214-226` | Не начато |
+| 7 | Низкая | `run_job` меняет `job_state` вне `_state_lock`, хотя `write_status` читает под блокировкой (гонка безвредна благодаря GIL). | `collector_daemon.py:72` | Не начато |
+| 8 | Низкая | `Dockerfile` копирует модули явным списком: новый модуль не попадёт в образ, ошибка проявится при запуске (healthcheck поймает, но поздно). | `Dockerfile:12` | Не начато |
+| 9 | Низкая | Запросы к RIPEstat без повторов и без параметра `sourceapp`. | `cidr_collector.py:119` | Не начато |
+| 10 | Низкая | `get_changes`: по запросу на каждое значение при проверке наличия; на десятках тысяч изменений станет заметно. | `db.py` | Не начато |
+
+## Архитектура и сопровождение
+- Цикл `db` ↔ `cidr_collector` обходится отложенным импортом; чище вынести пути и значения по умолчанию в `settings.py`.
+- `cidr_collector.py` совмещает четыре роли: помощники конфигурации, файловый обмен с демоном, два сборщика, CLI (связность сообщества 0,09).
+- `api_server.py` растёт: схемы, проверки и все эндпоинты в одном модуле (326 строк, наблюдение 11 анализа).
+
+## Что сделано хорошо
+- Токен проверяется безопасным сравнением и отказывает по умолчанию (503 без токена).
+- Запись атомарна везде (JSON через `os.replace`, SQLite в транзакциях, копии через `.tmp`).
+- Сетевые запросы выполняются до транзакции, блокировка записи короткая.
+- Ошибки хранилища сведены к одному типу (`StorageError`).
+- Порча данных не приводит к потере: карантин и копии.
+
+## Пробелы тестов
+Нет проверок задания `backup` в демоне, поля `last_restore` в `/health` и граничных значений `X-API-Key` и `since`.
+
+## Ограничения ревью
+- Граф не заменяет чтение кода: 14 связей типа INFERRED между модулями по отдельности не проверялись.
+- Сборка MikroTik и FRR на реальном ПО не проверялась (риск 4 анализа).
+
+## Рекомендуемый порядок
+1. Доработка «Безопасность ввода и данных»: п. 2, 3, 4 и тесты (`plan-input-hardening.md`).
+2. Отдельное решение по п. 1: после порчи без копий отвечать 503 до первого успешного сбора или запретить пустую выдачу при только что созданной базе.
+3. Пункты 5-10 объединить с доработкой наблюдаемости (п. 2 плана из анализа).
@@ -0,0 +1,18 @@
+# Итоги: безопасность ввода и данных (п. 2-4 ревью)
+
+План: `docs/plan-input-hardening.md`. Источник: `docs/review-2026-09-21.md`.
+
+## Сделано
+- **`X-API-Key` (находка 2):** токены сравниваются по байтам (`api_server.py`), нелатинский ключ даёт `401` вместо 500. Поведение с верным ключом и без токена в окружении (`503`) прежнее.
+- **`since` (находка 3):** курсор - только ASCII-цифры длиной до 30 символов (`MAX_CURSOR_DIGITS`); разбор времени защищён от `OverflowError`. Юникод-цифры, слишком длинные числа и крайние даты с поясом дают `400`; корректный, но выходящий за журнал курсор (до 30 цифр) по-прежнему `410`.
+- **DNS-адреса (находка 4):** `FQDNCollector.resolve_fqdn` оставляет только глобальные адреса (`is_global`), зона IPv6 (`%eth0`) отбрасывается, отфильтрованное пишется в лог (WARNING). Если глобальных адресов нет, результат пуст: сбор ведёт себя как при ошибке DNS (TTL не применяется, данные не удаляются). Ключ `allow_non_global_ips` в `config.json` (по умолчанию `false`) отключает фильтр для внутренних имён.
+- **README:** ограничения `since`, ключ `allow_non_global_ips`, логика сборщика. В отчёте ревью находки 2-4 отмечены исправленными.
+- **Тесты:** 2 новых (нелатинский ключ и некорректный `since`; фильтр адресов с разрешением и без). Всего 22, в контейнере 22 passed.
+
+## Проверка
+Повторены запросы из ревью в контейнерном окружении: `X-API-Key` с `é` -> 401; `since` = `²`, число из 5000 цифр, `0001-01-01T00:00:00+05:00`, `9999-12-31T23:59:59-05:00` -> 400 (раньше 500); `since` = 30 девяток -> 410; корректное время с поясом -> 200. Реальный `getaddrinfo` для `localhost` возвращает пустой список, в логе `ignoring non-global addresses: 127.0.0.1, ::1`.
+
+## Замечания
+- Фильтр меняет поведение для существующих установок только при наличии в данных неглобальных адресов; уже собранные такие адреса не удаляются сразу, а истекают по `ttl_days` (для немедленной очистки: `DELETE /fqdns/<имя>?purge=true` и добавить имя заново).
+- `is_global` отсекает и адреса общего назначения, например `100.64.0.0/10` (CGNAT) и документационные диапазоны; для внутренних имён используйте `allow_non_global_ips`.
+- Не вошло: находка 1 (пустая выдача после порчи без копий) ждёт решения по поведению; находки 5-10 - отдельные доработки.