From 46d56654d8d161884fcfe71da081d880bcd40c75 Mon Sep 17 00:00:00 2001 From: ayurishchev Date: Mon, 21 Sep 2026 09:42:51 +0300 Subject: [PATCH] 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 --- README.md | 5 ++-- api_server.py | 16 +++++++---- cidr_collector.py | 21 +++++++++++++-- docs/plan-input-hardening.md | 39 +++++++++++++++++++++++++++ docs/review-2026-09-21.md | 48 +++++++++++++++++++++++++++++++++ docs/summary-input-hardening.md | 18 +++++++++++++ tests/test_core.py | 19 +++++++++++++ tests/test_sources_api.py | 11 ++++++++ 8 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 docs/plan-input-hardening.md create mode 100644 docs/review-2026-09-21.md create mode 100644 docs/summary-input-hardening.md diff --git a/README.md b/README.md index 7c25486..2b51b18 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,7 @@ The system consists of **two independent processes**: the collector daemon (`col } ``` `ttl_days` - how long an address is kept after it was last seen (default `90`, `0` = keep forever). + Optional `allow_non_global_ips` (default `false`) - by default only public addresses resolved from FQDNs are stored; loopback, private, link-local and unspecified addresses (`127.0.0.1`, `10.x`, `0.0.0.0`, ...) are ignored and logged. Set `true` for internal names. Optional `backup_keep` - how many database backups to keep (default `7`); the backup cron is `schedule.backup` (default `30 4 * * *`), see section 9. Optional `changes_retention_days` - how long the change journal for `/addresses/diff` is kept (default `30`, `0` = forever). @@ -337,7 +338,7 @@ curl "http://localhost:8000/addresses/diff?since=0" # {"since":"0","now":"2026-09-21T04:09:44Z","cursor":42,"added":["3.0.0.0/24"],"removed":["2.0.0.0/24"]} curl "http://localhost:8000/addresses/diff?since=42" # next sync: use the returned cursor ``` -- `since` is a **cursor** from the previous answer (recommended: exact, independent of clocks) or an ISO 8601 time (no time zone = UTC; write `+` in a URL as `%2B`). Time is compared with millisecond precision, borders are inclusive, so an entry may be reported twice - repeating it is harmless. +- `since` is a **cursor** from the previous answer (recommended: exact, independent of clocks) or an ISO 8601 time (no time zone = UTC; write `+` in a URL as `%2B`). Time is compared with millisecond precision, borders are inclusive, so an entry may be reported twice - repeating it is harmless. A cursor is at most 30 ASCII digits; anything else that is not an ISO 8601 time (including out-of-range dates) is `400`. - The result is the net effect: an address added and removed (or the other way) within the interval is not reported; an address that another source still holds is not reported as removed. Both CIDRs and IPs of FQDNs count. Output is JSON only, no aggregation. - The first sync: take the full `/addresses` list and the cursor from its response header `X-Changes-Cursor` (read before the data), then call `/addresses/diff?since=` regularly. - `400` - invalid `since`; `410 Gone` - `since` is older than the journal (see `changes_retention_days`) or the cursor does not belong to this database: fetch the full `/addresses` list and continue from the new `cursor`. @@ -423,7 +424,7 @@ When the collector runs (whether manually or via schedule): 1. **Instantiation**: Creates a new instance of `CIDRCollector` or `FQDNCollector`. This forces a fresh read of `config.json`, ensuring any added ASNs/FQDNs are immediately processed. 2. **Fetching**: * **ASN**: Queries RIPE NCC API (`stat.ripe.net`). - * **FQDN**: Uses Python's `socket.getaddrinfo` to resolve A and AAAA records. + * **FQDN**: Uses Python's `socket.getaddrinfo` to resolve A and AAAA records. Non-public addresses are dropped (see `allow_non_global_ips`); if nothing is left the run is treated like a DNS failure (nothing is deleted). 3. **Merge in one transaction**: the fetched addresses are merged into the SQLite table `addresses` (`db.py`) in a single write transaction, so readers (the API) never see a half-updated state. 4. **Accumulation with TTL**: Each address has `first_seen`/`last_seen`. * New addresses are inserted; already seen ones get `last_seen` refreshed. diff --git a/api_server.py b/api_server.py index fc7e11c..eac2f0a 100644 --- a/api_server.py +++ b/api_server.py @@ -23,6 +23,7 @@ logging.basicConfig(level=logging.INFO, format="%(asctime)s %(levelname)s %(mess logger = logging.getLogger(__name__) TOKEN_ENV = "RIPE_API_TOKEN" +MAX_CURSOR_DIGITS = 30 # курсор длиннее не бывает: такое значение - ошибка запроса app = FastAPI(title="RIPE CIDR/FQDN API") @@ -102,7 +103,8 @@ def verify_token(x_api_key: Optional[str] = Header(None)): if not expected: # Fail closed: без заданного токена управление отключено raise HTTPException(status_code=503, detail=f"{TOKEN_ENV} is not configured; write access disabled.") - if not x_api_key or not secrets.compare_digest(x_api_key, expected): + # Сравнение по байтам: compare_digest для str принимает только ASCII и падает на других символах + if not x_api_key or not secrets.compare_digest(x_api_key.encode(), expected.encode()): raise HTTPException(status_code=401, detail="Invalid or missing X-API-Key.") @@ -149,14 +151,18 @@ def get_addresses( def parse_since(raw: str): """since: целый курсор или время ISO 8601 (без пояса - UTC). Возвращает (курсор, время UTC в формате журнала).""" - if raw.isdigit(): + invalid = HTTPException(status_code=400, detail="since must be a cursor (integer) or an ISO 8601 time") + # Только ASCII-цифры и ограниченная длина: str.isdigit() принимает юникод-цифры, а int() ограничен по длине + if raw.isascii() and raw.isdigit(): + if len(raw) > MAX_CURSOR_DIGITS: + raise invalid return int(raw), None try: # «+» в адресной строке приходит пробелом; «Z» понимаем явно moment = datetime.fromisoformat(raw.strip().replace(" ", "+").replace("Z", "+00:00")) - except ValueError: - raise HTTPException(status_code=400, detail="since must be a cursor (integer) or an ISO 8601 time") - moment = moment.replace(tzinfo=timezone.utc) if moment.tzinfo is None else moment.astimezone(timezone.utc) + moment = moment.replace(tzinfo=timezone.utc) if moment.tzinfo is None else moment.astimezone(timezone.utc) + except (ValueError, OverflowError): # OverflowError: крайние даты с часовым поясом + raise invalid return None, moment.strftime("%Y-%m-%dT%H:%M:%S.") + f"{moment.microsecond // 1000:03d}Z" diff --git a/cidr_collector.py b/cidr_collector.py index 753bdd7..9b35a87 100644 --- a/cidr_collector.py +++ b/cidr_collector.py @@ -1,5 +1,6 @@ import requests import datetime +import ipaddress import os import argparse import logging @@ -96,6 +97,14 @@ def pop_collection_requests(): return [t for t in types if t in COLLECT_TYPES] +def _is_global(address): + """Публичный адрес (не loopback, не частный, не 0.0.0.0, не link-local и т. п.).""" + try: + return ipaddress.ip_address(address).is_global + except ValueError: + return False + + class CIDRCollector: def __init__(self): self.config = load_full_config() @@ -161,6 +170,8 @@ class FQDNCollector: self.config = load_full_config() self.fqdns = self.config.get("fqdns", []) self.ttl_days = self.config.get("ttl_days", DEFAULT_TTL_DAYS) + # По умолчанию в список попадают только глобальные адреса; true - для внутренних имён + self.allow_non_global_ips = bool(self.config.get("allow_non_global_ips", False)) def add_fqdn(self, fqdn): added, self.fqdns = add_to_config_list("fqdns", fqdn) @@ -177,11 +188,17 @@ class FQDNCollector: try: # Family 0 - получаем и IPv4 (A), и IPv6 (AAAA) results = socket.getaddrinfo(fqdn, None) - # result[4] - sockaddr, для IP-протоколов индекс 0 - строка с адресом - return list({result[4][0] for result in results}) + # result[4] - sockaddr, для IP-протоколов индекс 0 - строка с адресом; зона IPv6 (%eth0) не нужна + addresses = {result[4][0].split("%")[0] for result in results} except socket.gaierror as e: logger.error("Error resolving %s: %s", fqdn, e) return [] + if self.allow_non_global_ips: + return list(addresses) + kept = {a for a in addresses if _is_global(a)} + if kept != addresses: + logger.warning("%s: ignoring non-global addresses: %s", fqdn, ", ".join(sorted(addresses - kept))) + return list(kept) def run_collection(self): logger.info("Starting FQDN IP collection...") diff --git a/docs/plan-input-hardening.md b/docs/plan-input-hardening.md new file mode 100644 index 0000000..efcc733 --- /dev/null +++ b/docs/plan-input-hardening.md @@ -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`. + +## Откат +Изменения локальны (две функции и одно чтение конфигурации): возврат к предыдущему коммиту; схема данных не затрагивается. diff --git a/docs/review-2026-09-21.md b/docs/review-2026-09-21.md new file mode 100644 index 0000000..0601c26 --- /dev/null +++ b/docs/review-2026-09-21.md @@ -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 плана из анализа). diff --git a/docs/summary-input-hardening.md b/docs/summary-input-hardening.md new file mode 100644 index 0000000..542e3e4 --- /dev/null +++ b/docs/summary-input-hardening.md @@ -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 - отдельные доработки. diff --git a/tests/test_core.py b/tests/test_core.py index 359cb28..12a2388 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -78,3 +78,22 @@ def test_post_schedule_auth(files, monkeypatch): assert client.post("/schedule", json=body, headers={"X-API-Key": "secret"}).status_code == 200 assert load_json(cc.CONFIG_FILE, {})["schedule"]["asn"] == "*/5 * * * *" assert client.get("/addresses").status_code == 200 + + +def test_fqdn_keeps_only_global_addresses(files, monkeypatch): + answers = [(2, 1, 6, "", (address, 0)) for address in + ("93.184.216.34", "127.0.0.1", "10.0.0.5", "0.0.0.0", "fe80::1%eth0", "2606:2800:220:1::1")] + monkeypatch.setattr(cc.socket, "getaddrinfo", lambda *args, **kwargs: answers) + + (files / "config.json").write_text('{"fqdns": ["x.example"]}') + assert sorted(cc.FQDNCollector().resolve_fqdn("x.example")) == ["2606:2800:220:1::1", "93.184.216.34"] + + # Внутренние имена: фильтр отключается, зона IPv6 отбрасывается + (files / "config.json").write_text('{"fqdns": ["x.example"], "allow_non_global_ips": true}') + everything = cc.FQDNCollector().resolve_fqdn("x.example") + assert {"127.0.0.1", "10.0.0.5", "fe80::1"} <= set(everything) and len(everything) == 6 + + # Глобальных адресов нет - результат пуст, как при ошибке DNS (сбор ничего не удаляет) + monkeypatch.setattr(cc.socket, "getaddrinfo", lambda *args, **kwargs: [answers[1]]) + (files / "config.json").write_text('{"fqdns": ["x.example"]}') + assert cc.FQDNCollector().resolve_fqdn("x.example") == [] diff --git a/tests/test_sources_api.py b/tests/test_sources_api.py index 1986b08..a714769 100644 --- a/tests/test_sources_api.py +++ b/tests/test_sources_api.py @@ -95,3 +95,14 @@ def test_addresses_diff(env): assert env.get("/addresses/diff", params={"since": body["cursor"] + 1}).status_code == 410 assert env.get("/addresses/diff", params={"since": "2000-01-01T00:00:00Z"}).status_code == 410 assert env.get("/addresses/diff").status_code == 422 + + +def test_input_hardening(env): + # Нелатинский ключ - 401 (раньше compare_digest падал: 500) + assert env.post("/collect", headers={"X-API-Key": "é".encode("latin-1")}).status_code == 401 + + # Некорректный since - 400: юникод-цифра, слишком длинное число, крайние даты с поясом (раньше 500) + for bad in ("²", "1" * 5000, "1" * 31, "0001-01-01T00:00:00+05:00", "9999-12-31T23:59:59-05:00"): + assert env.get("/addresses/diff", params={"since": bad}).status_code == 400, bad + # Корректный, но выходящий за журнал курсор - 410, а не 400 + assert env.get("/addresses/diff", params={"since": "9" * 30}).status_code == 410