212 lines
15 KiB
Markdown
212 lines
15 KiB
Markdown
# Ревью проекта awg_profiler
|
||
|
||
*Дата: 2026-07-18. Метод: сверка каждого утверждения README.md с кодом
|
||
(все 13 Go-файлов, оба Dockerfile, compose-файлы, entrypoint'ы, webui).*
|
||
|
||
Общий вердикт: **README точен, проект добротный** — атомарные записи,
|
||
продуманная схема статистики, аккуратное разделение CLI/web. Но при сверке
|
||
нашлись реальные расхождения и проблемы в самом коде.
|
||
|
||
## Точность README: подтверждено кодом
|
||
|
||
- Окно «онлайн» ≤ 150 с — совпадает (`onlineWindow = 150s`, stats.go:29),
|
||
обновление UI каждые 10 с (`setInterval(refreshAll, 10000)`), фоновый замер
|
||
раз в 20 с (`time.Tick(20s)`).
|
||
- Накопление статистики: дельты неотрицательны, сброс счётчика распознаётся,
|
||
atomic write (temp+rename), битый файл → `.bad` — всё как описано (stats.go).
|
||
- Секреты не отдаются в JSON API (`clientOut` без private/psk), Basic auth
|
||
через `subtle.ConstantTimeCompare`, `die()` в web-режиме превращается в
|
||
HTTP-ошибку — совпадает.
|
||
- `go vet` чистый, `go test ./...` — ok (9 тестов).
|
||
|
||
## Найденные проблемы
|
||
|
||
### 1. CSRF на мутирующих POST-эндпоинтах (средняя серьёзность)
|
||
|
||
Web API защищён только Basic auth, а браузер прикладывает эти credentials
|
||
автоматически. Cross-origin `<form>` POST с телом `text/plain` не требует
|
||
preflight, а `json.Decoder` в хендлерах не проверяет `Content-Type` — то есть
|
||
вредоносная страница может выполнить `POST /api/server/stop`,
|
||
`/api/clients` (create), `/api/server/restart` от имени залогиненного
|
||
админа. `DELETE` через форму невозможен, но enable/disable/stop — POST.
|
||
|
||
**Фикс:** проверка `Content-Type: application/json` или заголовка
|
||
`Origin`/`Sec-Fetch-Site` в обёртке `h()` (web.go:214) — ~5 строк.
|
||
|
||
### 2. Заявлен произвольный CIDR, реально поддерживается только /24
|
||
|
||
`init-server` спрашивает «VPN network CIDR» (default `10.0.0.0/24`), но:
|
||
|
||
- `serverIP()` жёстко берёт `x.y.z.1`;
|
||
- `getFirstClientIP` — `x.y.z.2`;
|
||
- `incrementIP` крутит только последний октет (умирает на .255 → максимум
|
||
~252 клиента);
|
||
- `awgConfHeader` пишет `Address = %s/24` **независимо от введённого
|
||
префикса** (awg.go:111).
|
||
|
||
Введи пользователь `10.0.0.0/16` — конфиг молча станет /24.
|
||
|
||
**Фикс:** либо валидация «только /24» на входе, либо честная поддержка
|
||
префикса.
|
||
|
||
### 3. QR-код содержит приватный ключ, но PNG создаётся с правами 0644
|
||
|
||
`<name>.conf` пишется с 0600, а `<name>.png` — вывод `qrencode` с дефолтным
|
||
umask (0644) в каталоге 0755. QR кодирует весь конфиг, включая `PrivateKey`
|
||
и `PresharedKey` — любой локальный пользователь хоста может его прочитать и
|
||
декодировать.
|
||
|
||
**Фикс:** `chmod 0600` после генерации и/или `0700` на `awg_clients/`.
|
||
|
||
### 4. Противоречие Alpine-веток
|
||
|
||
Комментарий в Dockerfile: «AmneziaWG is NOT packaged in Alpine's repos»
|
||
(потому tools собираются из исходников), но `deps.go` для Alpine-хоста
|
||
выполняет `apk add amneziawg-tools` — если пакета нет, `install-deps` на
|
||
голом Alpine просто упадёт. Одно из двух утверждений неверно.
|
||
|
||
**Фикс:** проверить наличие пакета в Alpine и привести к единому поведению
|
||
(либо собирать из исходников и на хосте, либо убрать комментарий).
|
||
|
||
### 5. docker-compose по умолчанию: web без auth на всех интерфейсах хоста
|
||
|
||
`ports: "8080:8080"` публикует UI наружу, а auth закомментирован. Приложение
|
||
печатает WARN, README предупреждает — но безопасный дефолт был бы
|
||
`"127.0.0.1:8080:8080"` с комментарием «поменяйте после включения auth».
|
||
|
||
### Мелочи
|
||
|
||
- `randMagic()`: диапазон получается [5, 2147483646] вместо заявленного
|
||
[5, 2147483647] (`n % 2147483642 + 5`), плюс небольшой modulo bias от
|
||
uint32. Косметика, но спека в комментарии не совпадает с кодом на единицу.
|
||
- `handleCreate`: если `awgPeerAdd` упадёт после `saveRegistry`, клиент
|
||
останется в реестре, а вызывающему вернётся 400 — частичное состояние без
|
||
отката.
|
||
- IP-адреса удалённых клиентов не переиспользуются (кроме последнего) — пул
|
||
«протекает» при churn'е.
|
||
- Тесты покрывают только чистые функции (util/config/stats-accumulate); ни
|
||
одного теста на HTTP-хендлеры или registry-операции, хотя они легко
|
||
тестируются с `AWG_PROFILER_DIR` во временный каталог (паттерн уже есть в
|
||
`TestStateRoundTrip`).
|
||
- Оценка «~20-30 МБ рантайм-слой» в README оптимистична: один бинарник
|
||
профилировщика — 10.5 МБ, плюс alpine+bash+iproute2+nftables; реально
|
||
ближе к 40-50 МБ. Стоит поправить или убрать цифру.
|
||
|
||
## Что сделано хорошо
|
||
|
||
- Раздельные мьютексы: `opLock` для WG-операций, `statsLock` для
|
||
статистики — фоновый замер не блокируется долгим install.
|
||
- `die()` → panic → recover в web-режиме: ошибка операции становится
|
||
HTTP-ответом, а не падением сервера.
|
||
- Валидация ответа IP-сервисов через `net.ParseIP` с лимитом чтения
|
||
(защита от HTML-ответов вместо адреса).
|
||
- Hot-add/remove пиров через `awg set` без рестарта интерфейса.
|
||
- `entrypoint` сеет state-флаг только при отсутствии файла — не затирает
|
||
данные на persistent volume.
|
||
- `.dockerignore` минимизирует build-контекст.
|
||
- Дизайн накопления статистики (баз-поинт на диске + неотрицательные
|
||
дельты) — корректное решение реальной проблемы userspace-рестартов.
|
||
|
||
## Рекомендуемый порядок исправлений
|
||
|
||
1. **№1 (CSRF)** и **№3 (права QR)** — безопасность.
|
||
2. **№2** — валидация /24.
|
||
3. **№4** — согласовать Alpine-ветки.
|
||
4. Остальное — по мере необходимости.
|
||
|
||
## Исправлено в ходе ревью
|
||
|
||
- `Dockerfile` (Alpine): отсутствовал `COPY webui_glass/ ./webui_glass/` —
|
||
`go build` падал на чистом чекауте из-за `//go:embed webui/* webui_glass/*`.
|
||
Строка добавлена, сборка проверена.
|
||
|
||
## Статус доработок (выполнены)
|
||
|
||
### 1. CSRF — исправлено
|
||
|
||
Добавлена функция `csrfSafe()` (web.go), вызывается из обёртки `h()`:
|
||
любой `POST`/`DELETE` без `Content-Type: application/json` отклоняется
|
||
`415 Unsupported Media Type`. HTML-форма физически не может выставить этот
|
||
заголовок (только `text/plain`, `application/x-www-form-urlencoded`,
|
||
`multipart/form-data`), поэтому blind cross-site form-POST больше не
|
||
проходит. `GET` не тронут (не мутирует состояние).
|
||
|
||
Клиентская часть (`webui/app.js`, `webui_glass/app.js`, идентичны —
|
||
обновлены оба) теперь всегда шлёт этот заголовок на `POST`/`DELETE`, даже
|
||
если тела нет (`server/start`, `enable`/`disable`, `stats/reset`,
|
||
`install-deps` и т.д. раньше отправлялись вовсе без `Content-Type`).
|
||
|
||
Тест: `TestCsrfSafe` (awg_profiler_test.go).
|
||
|
||
### 2. Валидация /24 — исправлено (вариант «запретить не-/24»)
|
||
|
||
Добавлена `requireSlash24()` (util.go): парсит CIDR через `net.ParseCIDR`,
|
||
требует IPv4 и ровно `/24`, требует совпадения введённого адреса с базовым
|
||
адресом подсети (иначе подсказывает правильный). Вызывается из
|
||
`initServer()` (CLI, server.go) и `initServerWeb()` (веб-мастер, webops.go)
|
||
сразу после чтения `Network`. Любой другой префикс теперь явно отклоняется
|
||
с понятным сообщением, а не молча превращается в /24.
|
||
|
||
Тест: `TestRequireSlash24`.
|
||
|
||
README дополнен пояснением, что принимается только `/24`.
|
||
|
||
### 3. Права QR-кода — исправлено
|
||
|
||
`createClientConfig()` (client.go) теперь делает `os.Chmod(pngPath, 0600)`
|
||
сразу после `qrencode`, той же логике, что уже применялась к `.conf`.
|
||
Дополнительно (по варианту «и/или 0700 на awg_clients/» из фикса) каталоги
|
||
`data/` и `awg_clients/` в `initStorage()` (registry.go) и в
|
||
`saveStatsLocked()` (stats.go) теперь создаются с правами `0700` вместо
|
||
`0755` — они хранят приватные ключи (`registry.json`, `.conf`/`.png`).
|
||
Существующие деплойменты, где каталоги уже созданы с 0755, не меняются
|
||
автоматически (`MkdirAll` не трогает права существующих директорий).
|
||
|
||
### 4. Противоречие Alpine-веток — исправлено
|
||
|
||
Проверено через веб-поиск: `amneziawg-tools` действительно отсутствует в
|
||
официальных apk-репозиториях Alpine — значит был неверен `deps.go`, а не
|
||
комментарий в Dockerfile. `installDeps()` для Alpine больше не делает
|
||
`apk add amneziawg-tools` (пакета нет — команда просто падала бы на живом
|
||
хосте); вместо этого новая функция `buildAmneziawgToolsFromSource()`
|
||
(deps.go) собирает `awg`/`awg-quick` из исходников тем же способом, что и
|
||
`tools-builder`-стадия в Dockerfile (`git clone` → `make -C src` →
|
||
`make -C src install PREFIX=/usr`). Требование к kernel-модулю (нужно
|
||
предоставить отдельно) осталось прежним и явно описано в README.
|
||
|
||
### 5. docker-compose без auth на всех интерфейсах — исправлено
|
||
|
||
`docker-compose.yml` и `docker-compose.mint.yml`: порт `8080` теперь
|
||
публикуется как `127.0.0.1:8080:8080` вместо `8080:8080` — веб-UI по
|
||
умолчанию доступен только с самой машины. Комментарий рядом объясняет, что
|
||
расширять до всех интерфейсов стоит только вместе с
|
||
`AWG_WEB_USER`/`AWG_WEB_PASS`. README (Quick Start, вариант A) обновлён:
|
||
объяснено, как открыть UI локально или через SSH-туннель, и что менять
|
||
перед тем, как открывать порт наружу.
|
||
|
||
### Мелочи — частично исправлены
|
||
|
||
- **`randMagic()` off-by-one — исправлено.** Диапазон теперь честные
|
||
`[5, 2147483647]` (`span = 2147483647-5+1`), а не `[5, 2147483646]`.
|
||
- **`handleCreate` частичное состояние — исправлено.** `awgPeerAdd()`
|
||
(awg.go) больше не вызывает `die()` при неудаче `awg set` — на этом этапе
|
||
клиент уже сохранён в реестре и добавлен в `<iface>.conf`
|
||
(`confAppendPeer` вызывается раньше), так что живой hot-add — best-effort:
|
||
при неудаче пишется `warn()` и создание клиента по-прежнему считается
|
||
успешным (применится на следующем restart/sync-config).
|
||
- **Тесты на HTTP-хендлеры/registry — частично.** Добавлены целевые тесты
|
||
на обе новые функции безопасности (`TestCsrfSafe`, `TestRequireSlash24`).
|
||
Полное покрытие HTTP-хендлеров (`handleCreate`, `handleDelete` и т.д.) в
|
||
эту доработку не входило — осталось как есть.
|
||
- **IP-адреса удалённых клиентов не переиспользуются — не тронуто
|
||
осознанно.** Изменение схемы выдачи IP — это поведенческое изменение с
|
||
риском разойтись с форматом реестра bash-версии; оставлено как
|
||
зафиксированный, но не блокирующий issue.
|
||
- **Оценка размера образа «~20-30 МБ» — исправлено.** Неподтверждённая
|
||
цифра убрана из README вместо того, чтобы гадать без реальной сборки
|
||
образа.
|
||
|
||
Все правки проверены: `gofmt -l .` чист, `go vet ./...` чист, `go build .`
|
||
успешен, `go test ./...` — 11/11 тестов проходят (добавлены `TestCsrfSafe`,
|
||
`TestRequireSlash24`).
|