diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ed1ba5..770da59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,7 +63,127 @@ Hysteria-интеграции с официальной документацие задокументирован в [docs/acceptance/2026-09-01-v1.0.0-rc2-preflight-findings.md](docs/acceptance/2026-09-01-v1.0.0-rc2-preflight-findings.md). -### Исправлено — отзыв доступа к VPN +Одиннадцатый проход — второй разбор того же слоя, уже по состоянию после +десятого. Тема: границы между частями access-control. Десятый проход починил +одну операцию отзыва доступа и оставил остальные — удаление, импорт, смену +секрета, урезание квоты и срока, снижение лимита устройств — в прежнем +состоянии; правило доступа при этом продолжало существовать в двух +экземплярах, написанных разными SQL-условиями, которые расходились ровно на +границах. Проведены три границы: `состояние пира → решение о доступе`, +`сохранённое изменение → живая сессия`, `планировщик → полностью +принадлежащая ему работа`. + +### Исправлено — правило доступа + +- **Исчерпанная квота не отключала пира никогда.** Правило доступа + существовало в двух экземплярах: SQL-условием внутри `Hysteria2Auth` и + другим SQL-условием внутри cron. Второе не было отрицанием первого, и + расхождение приходилось на границы — `quota = 0`, `usage = quota`, + `now = expiresAt`, `now = bannedUntil`: авторизация отказывала, cron сессию + не рвал. Условие cron требовало СТРОГОГО превышения квоты, а счётчики растут + порциями по ответу Traffic Stats API, поэтому точное равенство — обычный + исход очередного сбора. Пир с исчерпанной квотой не пускался заново, но его + живая сессия не разрывалась никогда. + + Политика вынесена в одну функцию `peerAccessDenied`; авторизация ищет пира + только по `secret_digest`, cron применяет ту же функцию. `quota = -1` — + единственный способ снять ограничение, `quota = 0` означает ноль байтов, + `usage = quota` означает исчерпанный лимит, `bannedUntil = now` означает + закончившуюся блокировку. Строка без решающего поля трактуется как + повреждённая и ведёт к отказу. + +### Исправлено — операции, оставляющие живую сессию + +- **Удаление пира не отзывало доступ и теряло `authId`.** `DeletePeer` состоял + из одного `dao.DeletePeer`: строка исчезала, живая QUIC-сессия оставалась, а + вместе со строкой исчезал `auth_id` — единственное, чем эту сессию можно было + бы завершить. Состояние становилось невосстановимым. Теперь: прочитать пира и + запомнить `authId` → записать `disabled=1` → `/kick` → удалить строку. При + неудаче разрыва строка остаётся отключённой, и оператор повторяет удаление. + +- **Разрыв выполнялся только при `disabled=1`.** Мимо проходили смена секрета, + урезание квоты ниже израсходованного, перенос срока в прошлое и снижение + лимита устройств — каждая из них закрывает доступ, но сессию не трогала. + Правило асимметрично: ограничение применяется немедленно, послабление — нет. + При любом сочетании изменений уходит ровно один `/kick`. + +- **Импорт не завершал сессии переписанных пиров.** Импорт переписывает + `auth_id`, секрет, квоту, срок и `disabled` целиком. Старые `authId` + собираются внутри транзакции — после commit их в базе уже нет, — а разрыв + идёт после commit: до него клиент успел бы переподключиться к ещё не + изменённому пиру. + +- **Единственный вход к `/kick`.** Все операции идут через один + `reconcileLiveSessions`, а он — через `disconnectAuthIDs`, который принимает + готовые идентификаторы, дедуплицирует их, разбивает на части и не обращается + к базе вовсе. Пока обращений к `/kick` было два, они расходились: у cron не + было ни дедупликации, ни разбиения, зато был POST с пустым массивом каждые 30 + секунд. + +- **Формулировка частичного результата больше не называет операцию.** Через + `peer_disconnect_failed` отчитываются восемь операций; прежнее «новые + подключения пира запрещены» было верно ровно для отключения пира, а для + удалённого — бессмысленно. Контроллеры удаления и импорта переведены на + `failService`, панель разбирает исход импорта и обновляет список при любом + результате. + +### Исправлено — цикл учёта + +- **Джоба убегала из жизненного цикла планировщика.** `CronHandleAccount` + запускала горутину, которая запускала ещё две. Для планировщика джоба + заканчивалась почти мгновенно, поэтому `StopCron()` не ждал настоящей работы: + `releaseResource()` закрывал SQLite, а горутины продолжали в неё писать. + Параллельность обеих половин означала ещё и то, что принудительное отключение + читало счётчики до записи снятой дельты. Теперь джоба синхронна, под одним + мьютексом на весь цикл, и порядок строгий: сбор трафика, затем enforcement. + +- **Три nil-разыменования роняли процесс целиком.** `*trafficSecretConfig.Value`, + `*item.AuthId` в принудительном отключении и `*item.Id` в сбросе трафика — все + внутри горутин, где их некому перехватить, то есть каждое означало падение + сервиса вместе с обработчиком machine-auth. + +- **Гейт `Hysteria2IsRunning` удалён из cron.** `util.Exec` не отличает «служба + неактивна» от «спросить не удалось», поэтому сломанный `systemctl` при живой + Hysteria молча отключал и учёт трафика, и принудительное отключение — без + единой строки в журнале. + +- **Потеря дельты трафика больше не молчит.** `GET /traffic?clear=1` + деструктивен: счётчики Hysteria обнуляются сразу после отправки ответа. + Прежний код на отказе записи делал `continue`, и дельта исчезала, не оставив + следа в исходе джобы. Полное решение требует смены модели учёта + (недеструктивное чтение плюс долговременные checkpoint'ы) и в `1.0.0` + намеренно не вводится: квота — операционный предел доступа, а не учёт с + финансово значимым каждым байтом. + +### Исправлено — лимит устройств под нагрузкой + +- **Параллельные подключения превышали `maxDevices`.** Между чтением `/online` + и ответом «allow» место ничем не удерживалось: при `online = max-1` два + одновременных запроса получали разрешение оба. Мьютекс вокруг `/online` этого + не чинит — ответив «allow», админка не создаёт подключение, и следующий + запрос продолжает видеть прежнее число. Появился process-local учёт выданных, + но ещё не проявившихся разрешений: решение принимается по сумме «подключено + плюс зарезервировано», рост `online` снимает соответствующее число + резерваций, протухшие снимаются по внутреннему TTL. Ни Redis, ни таблиц в + базе, ни распределённых блокировок: HY2XS — один процесс на одном сервере. + +### Исправлено — гейты сборки + +- **Гейт fail-open срабатывал на корректном коде.** Проверка «авторизация не + возвращает успех из ветки ошибки» была записана регуляркой + `err != nil \{[\s\S]*?return \*peer\.Id`, а ленивый `[\s\S]*?` свободно + пересекает границы блоков: она давала совпадение на любой функции, где после + какой-нибудь проверки ошибки ниже стоит успешный возврат. Проверено на коде + из `HEAD` — гейт нельзя было удовлетворить, не сломав продукт. Тело ветки + теперь выделяется по балансу фигурных скобок. + +- **Детектор гонок стал обязательным шагом сборки.** Состояние трекера + разрешений и мьютекс цикла учёта принадлежат процессу, поэтому их + корректность не наблюдаема ни в `go test`, ни в `go vet`. Пропуск при + недоступном C-компиляторе не предусмотрен: сборка, молча пропускающая + проверку, выдаёт внешне неотличимый production-артефакт. + +### Исправлено — отзыв доступа к VPN (десятый проход) - **Отключение пира не отзывало доступ.** Запись `disabled=1` видит только выборка в `Hysteria2Auth`, то есть она закрывает БУДУЩИЕ обращения к @@ -74,11 +194,13 @@ Hysteria-интеграции с официальной документацие блокировку в auth backend как пару: по отдельности не работает ни одна половина. - Появился `service.DisconnectPeers` — только официальный Traffic Stats + Появился отдельный примитив разрыва — только официальный Traffic Stats `/kick`, без единой записи в базу. Прежний `Hysteria2Kick` вместе с разрывом проставлял `banned_until`, поэтому воспользоваться им для отключения было нельзя: операция записала бы заодно временную блокировку — другой механизм с - другим сроком жизни. + другим сроком жизни. (В одиннадцатом проходе он принимает готовые `authId`, а + не идентификаторы пиров: удалению и импорту старое значение нужно уже после + его исчезновения из базы.) Порядок обратному не подлежит: сначала долговременная запись, затем разрыв. При обратном клиент успевает переподключиться в окне между `/kick` и записью. diff --git a/apps/controller/peer.go b/apps/controller/peer.go index 34c0116..fcee49f 100644 --- a/apps/controller/peer.go +++ b/apps/controller/peer.go @@ -98,8 +98,14 @@ func DeletePeer(c *gin.Context) { if err != nil { return } + // failService, а не vo.Fail: удаление умеет завершиться ЧАСТИЧНО — пир + // отключён в базе, но завершить его активную сессию не удалось, поэтому + // строка намеренно оставлена на месте. Через vo.Fail этот результат уехал + // бы панели неотличимо от полного отказа, и оператор сделал бы неверный + // вывод: «удаление не сработало, пир как был», — тогда как доступ уже + // закрыт, а строка ждёт повторной попытки. if err = service.DeletePeer(id); err != nil { - vo.Fail(err.Error(), c) + failService(err, c) return } vo.Success(nil, c) @@ -228,8 +234,12 @@ func ImportPeer(c *gin.Context) { return } + // failService, а не vo.Fail: импорт умеет завершиться ЧАСТИЧНО — партия + // зафиксирована в базе целиком, но завершить старые сессии обновлённых + // пиров не удалось. Полный отказ здесь означал бы для оператора «файл не + // применился», хотя он применился весь. if err = service.UpsertPeerExport(peerExports); err != nil { - vo.Fail(err.Error(), c) + failService(err, c) return } vo.Success(nil, c) diff --git a/apps/frontend/src/api/peer/index.ts b/apps/frontend/src/api/peer/index.ts index 6873d77..3dddc4b 100644 --- a/apps/frontend/src/api/peer/index.ts +++ b/apps/frontend/src/api/peer/index.ts @@ -95,6 +95,15 @@ export function getPeerClientConfigApi( }); } +// Импорт сообщает свой исход сам — по той же причине, что и действия строки +// пира. +// +// Партия применяется одной транзакцией, а после её фиксации завершаются старые +// сессии обновлённых пиров. Второй шаг умеет не удаться отдельно от первого, и +// тогда ответ несёт peer_disconnect_failed: файл применён целиком, но часть +// клиентов остаётся на связи до переподключения. Общий перехватчик показал бы +// такой исход красной ошибкой, то есть сообщил бы оператору ровно обратное +// тому, что произошло. export function importPeerApi(data: FormData): AxiosPromise { return request({ url: "/peer-import", @@ -103,6 +112,7 @@ export function importPeerApi(data: FormData): AxiosPromise { "Content-Type": "multipart/form-data", }, data, + skipErrorToast: true, }); } diff --git a/apps/frontend/src/lang/package/en.ts b/apps/frontend/src/lang/package/en.ts index ad00ca6..2b6fb01 100644 --- a/apps/frontend/src/lang/package/en.ts +++ b/apps/frontend/src/lang/package/en.ts @@ -164,8 +164,14 @@ export default { // The phrase must open with what has ALREADY been applied, otherwise it // reads as "the operation failed" and the operator repeats an action // that in fact went through. + // + // It also names no specific operation. This code is reported by + // disabling a peer, a temporary ban, secret rotation, quota and expiry + // reductions, a lower device limit, a batch import and peer deletion; + // the previous "new connections for this peer are now refused" held only + // for the first case and is meaningless for a deleted peer. peer_disconnect_failed: - "New connections for this peer are now refused, but its active session could not be terminated: the Hysteria Traffic Stats API is unreachable. An established connection may keep working until the client reconnects. Check the hysteria-server service and retry.", + "The changes were saved, but the related active sessions could not be terminated: the Hysteria Traffic Stats API is unreachable. Established connections may keep working until the client reconnects. Check the hysteria-server service and retry.", import_file_extension: "Import accepts .json files only", unauthorized: "Signing in is required", session_expired: "Session expired", diff --git a/apps/frontend/src/lang/package/ru.ts b/apps/frontend/src/lang/package/ru.ts index 85df1ba..e1f54b9 100644 --- a/apps/frontend/src/lang/package/ru.ts +++ b/apps/frontend/src/lang/package/ru.ts @@ -166,8 +166,14 @@ export default { // Фраза обязана начинаться с того, что УЖЕ СДЕЛАНО: иначе оператор // прочитает её как «операция не выполнена» и повторит действие, которое // на самом деле применилось. + // + // И она НЕ называет конкретную операцию. Этим кодом отчитываются + // отключение пира, временная блокировка, смена секрета, урезание квоты и + // срока, снижение лимита устройств, импорт партии и удаление пира; + // прежнее «новые подключения пира запрещены» было верно ровно для + // первого случая, а для удалённого пира — просто бессмысленно. peer_disconnect_failed: - "Новые подключения пира запрещены, но завершить его активную сессию не удалось: Traffic Stats API Hysteria недоступен. Установленное соединение может работать до переподключения клиента. Проверьте состояние службы hysteria-server и повторите действие.", + "Изменения сохранены, но завершить связанные активные сессии не удалось: Traffic Stats API Hysteria недоступен. Установленные соединения могут работать до переподключения клиента. Проверьте состояние службы hysteria-server и повторите действие.", import_file_extension: "Импорт принимает только файлы .json", unauthorized: "Требуется вход в панель", session_expired: "Сессия истекла", diff --git a/apps/frontend/src/views/peer/list/index.vue b/apps/frontend/src/views/peer/list/index.vue index 9fd2d53..4fdbc48 100644 --- a/apps/frontend/src/views/peer/list/index.vue +++ b/apps/frontend/src/views/peer/list/index.vue @@ -937,13 +937,30 @@ async function showQr(row: PeerVo) { qrDialog.value = true; } +/** + * Импорт выгрузки пиров. + * + * Исход разбирается тем же обработчиком, что и действия строки: импорт умеет + * завершиться ЧАСТИЧНО — партия применена целиком, но завершить старые сессии + * обновлённых пиров не удалось. + * + * Раньше здесь не было ни try, ни catch: отказ уходил необработанным + * отклонением промиса, а `handleQuery()` до выполнения не доходил — список + * оставался с прежними данными, хотя база уже изменилась. Убирается файл из + * очереди и обновляется список ПРИ ЛЮБОМ исходе по той же причине. + */ async function handleImport(params: UploadRequestOptions) { if (importFileList.value.length <= 0) { return; } const formData = new FormData(); formData.append("file", params.file); - await importPeerApi(formData); + try { + await importPeerApi(formData); + ElMessage.success(t("common.success")); + } catch (error) { + reportPeerActionError(error); + } importFileList.value = []; await handleQuery(); } diff --git a/apps/service/cron.go b/apps/service/cron.go index 7711ac5..8c1f437 100644 --- a/apps/service/cron.go +++ b/apps/service/cron.go @@ -1,78 +1,167 @@ package service import ( + "fmt" + "sync" + "time" + "github.com/sirupsen/logrus" "gorm.io/gorm" "hy2xs-admin/dao" - "hy2xs-admin/model/constant" "hy2xs-admin/model/entity" "hy2xs-admin/proxy" "hy2xs-admin/util" - "sync" - "time" ) -var trafficMutex sync.Mutex -var kickMutex sync.Mutex +// Джоба учёта принадлежит планировщику, а не собственным горутинам. +// +// Что было: +// +// CronHandleAccount() +// -> go func() +// -> go saveAccountTraffic() +// -> go kickAccount() +// +// Три уровня отсоединённых горутин. Для cron.Cron джоба заканчивалась почти +// мгновенно — сразу после запуска внешней, — поэтому StopCron(), который +// честно ждёт `scheduler.Stop().Done()`, не ждал НИЧЕГО из настоящей работы. +// Завершение процесса выглядело так: планировщик отчитался «джоб не осталось», +// releaseResource() закрыл SQLite, а внутренние горутины продолжали писать +// трафик и рвать сессии в уже закрытое соединение. Это ровно та болезнь, от +// которой лечится cron_scheduler.go, только протащенная внутрь одной джобы. +// +// Второе следствие того же устройства было тише и хуже. Обе внутренние +// горутины запускались ПАРАЛЛЕЛЬНО, поэтому принудительное отключение читало +// счётчики трафика ДО того, как в них попадала только что снятая дельта. При +// тридцатисекундном тике это значит, что превышение квоты замечалось в лучшем +// случае со следующего цикла, а на границе — не замечалось вовсе. +// +// Теперь джоба синхронна, порядок внутри неё строгий, а взаимное исключение +// даёт один мьютекс на весь цикл: сбор трафика и enforcement больше не могут +// ни разъехаться во времени, ни наложиться сами на себя. +// accountJobMutex сериализует цикл учёта. +// +// Заменяет пару trafficMutex + kickMutex. Раздельные мьютексы защищали каждую +// половину от самой себя, но не защищали пару от расщепления: при затянувшемся +// сборе трафика следующий тик мог запустить enforcement поверх предыдущего +// сбора. Одного мьютекса на весь цикл достаточно и, в отличие от двух, он +// выражает действительный инвариант — «в любой момент времени выполняется не +// более одного цикла учёта». +var accountJobMutex sync.Mutex + +// CronHandleAccount — один синхронный цикл учёта: собрать трафик, затем +// применить политику доступа. +// +// Состояние службы по systemd здесь НЕ спрашивается. Прежний гейт +// +// if !Hysteria2IsRunning() { return } +// +// стоял на решении о применении операции, а Hysteria2IsRunning для этого +// непригоден по собственному объявлению: util.Exec схлопывает «systemctl +// вернул 3, служба неактивна» и «запустить systemctl не удалось» в одну +// ошибку. То есть сломанный systemctl при живой Hysteria молча отключал и учёт +// трафика, и принудительное отключение — без единой строки в журнале. +// +// Нужные системы спрашиваются напрямую: `/traffic`, `/online`, `/kick`. Если +// Hysteria действительно не работает, вызов вернёт ошибку, и она будет +// записана. Если сломан systemctl, а Hysteria жива, учёт продолжит работать. func CronHandleAccount() { - go func() { - if !Hysteria2IsRunning() { - return - } + // Пропуск тика при уже идущем цикле — не отказ: следующий тик через 30 + // секунд, а очередь из накопившихся циклов ничего бы не дала. + if !accountJobMutex.TryLock() { + return + } + defer accountJobMutex.Unlock() - apiPort, err := GetHysteria2ApiPort() - if err != nil { - return - } + apiPort, err := GetHysteria2ApiPort() + if err != nil { + logrus.WithError(err).Error("cron: не удалось определить порт Traffic Stats API; цикл учёта пропущен") + return + } - trafficSecretConfig, err := dao.GetConfig("key = ?", constant.Hysteria2TrafficStatsSecret) - if err != nil { - return - } + // Секрет берётся общей функцией, которая отличает «ключа нет» от пустого + // значения. Раньше здесь стояло `*trafficSecretConfig.Value` без единой + // проверки: строка в таблице `config` без значения роняла бы процесс + // паникой на разыменовании nil — причём внутри отсоединённой горутины, где + // её некому перехватить, то есть падал бы весь сервис вместе с + // обработчиком machine-auth. + secret, err := hysteria2TrafficSecret() + if err != nil { + logrus.WithError(err).Error("cron: секрет Traffic Stats API недоступен; цикл учёта пропущен") + return + } - // Сохранение данных трафика - go saveAccountTraffic(apiPort, *trafficSecretConfig.Value) + // Порядок обязателен: enforcement принимает решение по счётчикам, поэтому + // счётчики должны быть уже обновлены. + if err := saveAccountTraffic(apiPort, secret); err != nil { + logrus.WithError(err).Error("cron: сбор трафика завершился с ошибкой") + } - // Принудительное отключение - go kickAccount(apiPort, *trafficSecretConfig.Value) - }() + if err := enforcePeerAccess(apiPort, secret); err != nil { + logrus.WithError(err).Error("cron: принудительное отключение завершилось с ошибкой") + } } +// CronResetTraffic обнуляет счётчики трафика всех пиров по расписанию. func CronResetTraffic() { peers, err := dao.ListPeer("1=1") if err != nil { + logrus.WithError(err).Error("cron: не удалось прочитать пиров для сброса трафика") return } - var ids []int64 + ids := make([]int64, 0, len(peers)) for _, item := range peers { + // Строка без идентификатора — повреждённые данные. Раньше здесь + // стояло `*item.Id` без проверки, то есть такая строка роняла джобу + // паникой, а вместе с ней и процесс. + if item.Id == nil { + logrus.Error("cron: строка пира без идентификатора пропущена при сбросе трафика") + continue + } ids = append(ids, *item.Id) } - idsList := util.SplitArr(ids, 100) - for _, item := range idsList { - if err := dao.UpdatePeer(item, map[string]interface{}{"download_bytes": 0, "upload_bytes": 0}); err != nil { + if len(ids) == 0 { + return + } + for _, chunk := range util.SplitArr(ids, 100) { + if err := dao.UpdatePeer(chunk, map[string]interface{}{"download_bytes": 0, "upload_bytes": 0}); err != nil { + logrus.WithError(err).Error("cron: сброс трафика части пиров не выполнен") continue } } } -func saveAccountTraffic(apiPort int64, trafficStatsSecret string) { - if !trafficMutex.TryLock() { - return - } - defer trafficMutex.Unlock() - +// saveAccountTraffic переносит накопленный Hysteria трафик в базу. +// +// Чтение ДЕСТРУКТИВНОЕ: `?clear=1` обнуляет счётчики Hysteria сразу после +// того, как ответ отправлен (официальный контракт Traffic Stats API). Значит +// каждая дельта существует ровно в одном экземпляре, и потерянная здесь +// потеряна навсегда. +// +// Полностью закрыть это окно можно только сменой модели учёта — недеструктивным +// `GET /traffic` с долговременными checkpoint'ами верхних счётчиков и +// вычислением дельты на стороне админки. Это отдельная подсистема с обработкой +// перезапуска и сброса счётчиков Hysteria, и в текущем проходе она намеренно +// не вводится: квота здесь — операционная граница доступа, а не биллинговый +// учёт с финансово значимым каждым байтом. +// +// Чего это НЕ оправдывает — молчания. Раньше отказ записи внутри цикла делал +// `continue`, и дельта конкретного пира исчезала, не оставив следа в исходе +// джобы. Теперь каждая потеря считается и попадает в возвращаемую ошибку. +func saveAccountTraffic(apiPort int64, trafficStatsSecret string) error { users, err := proxy.NewHysteria2Api(apiPort).ListUsers(true, trafficStatsSecret) if err != nil { - return + return err } if len(users) == 0 { - return + return nil } nowMs := time.Now().UnixMilli() hourStart := nowMs - (nowMs % int64(time.Hour/time.Millisecond)) + lost := 0 for key, traffic := range users { rxBytes := traffic.Rx txBytes := traffic.Tx @@ -82,9 +171,19 @@ func saveAccountTraffic(apiPort int64, trafficStatsSecret string) { peer, peerErr := dao.GetPeer("auth_id = ?", key) if peerErr != nil { + // Пир, которого админка не знает: удалён между сбором и записью + // либо создан в обход панели. Дельта уже обнулена в Hysteria и + // приписывать её некому. + logrus.WithError(peerErr). + WithField("authId", key). + Warn("cron: трафик получен для неизвестного пира и не записан") + lost++ continue } if peer.Id == nil { + logrus.WithField("authId", key). + Error("cron: строка пира без идентификатора; трафик не записан") + lost++ continue } @@ -100,66 +199,103 @@ func saveAccountTraffic(apiPort int64, trafficStatsSecret string) { TxBytes: &txBytes, SampledAt: &nowMs, } - if err = dao.SaveTrafficSample(sample); err != nil { - logrus.Errorf("save traffic_sample failed: %v", err) - continue + if err := dao.SaveTrafficSample(sample); err != nil { + logrus.WithError(err). + WithField("peerId", *peer.Id). + Error("cron: не удалось сохранить отсчёт трафика") + // Отсчёт — история для графиков; счётчики пира важнее, и попытка + // их обновить продолжается. } - if err = dao.UpdatePeer([]int64{*peer.Id}, map[string]interface{}{ + if err := dao.UpdatePeer([]int64{*peer.Id}, map[string]interface{}{ "download_bytes": gorm.Expr("download_bytes + ?", rxBytes), "upload_bytes": gorm.Expr("upload_bytes + ?", txBytes), }); err != nil { - logrus.Errorf("update peer traffic failed: %v", err) + logrus.WithError(err). + WithField("peerId", *peer.Id). + Error("cron: счётчики пира не обновлены; дельта Hysteria уже обнулена и потеряна") + lost++ continue } _ = dao.UpsertTrafficAggregateHourly(*peer.Id, hourStart, rxBytes, txBytes) } -} -func kickAccount(apiPort int64, trafficStatsSecret string) { - if !kickMutex.TryLock() { - return - } - defer kickMutex.Unlock() - users, err := proxy.NewHysteria2Api(apiPort).OnlineUsers(trafficStatsSecret) - if err != nil { - return - } - if len(users) > 0 { - i := 0 - authIDs := make([]string, len(users)) - for k := range users { - authIDs[i] = k - i++ - } - authIDLists := util.SplitArr(authIDs, 10) - var wg sync.WaitGroup - for _, authIDList := range authIDLists { - wg.Add(1) - go func(authIDList []string) { - defer wg.Done() - now := time.Now().UnixMilli() - peers, err := dao.ListPeer(`auth_id in ? and ( - disabled = 1 - or (quota_bytes > 0 and quota_bytes < download_bytes + upload_bytes) - or (expires_at > 0 and ? > expires_at) - or ? < banned_until - )`, authIDList, now, now) - if err != nil { - return - } - kickAuthIDs := make([]string, len(peers)) - j := 0 - for _, item := range peers { - kickAuthIDs[j] = *item.AuthId - j++ - } - if err = proxy.NewHysteria2Api(apiPort).KickUsers(kickAuthIDs, trafficStatsSecret); err != nil { - return - } - }(authIDList) - } - wg.Wait() + if lost > 0 { + return &trafficLossError{lost: lost} } + return nil +} + +// trafficLossError сообщает, сколько дельт не удалось записать. +// +// Отдельный тип, а не fmt.Errorf, потому что количество здесь — величина, а не +// украшение фразы: чтение `?clear=1` деструктивно, поэтому «потеряно 1 из 200» +// и «потеряно 200 из 200» — разные события, и различать их должен уметь не +// только человек, читающий журнал. +type trafficLossError struct{ lost int } + +func (e *trafficLossError) Error() string { + return fmt.Sprintf( + "дельт трафика не записано и потеряно безвозвратно: %d", + e.lost, + ) +} + +// enforcePeerAccess завершает сессии пиров, которым доступ уже закрыт. +// +// Политика берётся из peerAccessDenied — той же функции, по которой пира +// пускает или не пускает авторизация. Собственного SQL-условия здесь больше +// нет, и это главное свойство: пока правило было записано в двух местах, оно +// расходилось на границах quota, expiry и ban, и исчерпавший квоту пир не +// пускался заново, но и не отключался никогда. +// +// Обход последовательный. Прежняя реализация раскладывала online-пиров на +// чанки по 10 и запускала по горутине на чанк с sync.WaitGroup внутри уже +// отсоединённой горутины. Параллельность здесь не нужна: обращений к базе +// столько же, а `/kick` всё равно один на весь набор. +func enforcePeerAccess(apiPort int64, trafficStatsSecret string) error { + online, err := proxy.NewHysteria2Api(apiPort).OnlineUsers(trafficStatsSecret) + if err != nil { + return err + } + if len(online) == 0 { + return nil + } + + authIDs := make([]string, 0, len(online)) + for authID := range online { + if authID == "" { + continue + } + authIDs = append(authIDs, authID) + } + if len(authIDs) == 0 { + return nil + } + + now := time.Now().UnixMilli() + kick := make([]string, 0, len(authIDs)) + for _, chunk := range util.SplitArr(authIDs, 100) { + peers, err := dao.ListPeer("auth_id in ?", chunk) + if err != nil { + return err + } + for _, peer := range peers { + // Строка без authId Hysteria не знает: рвать нечего. Раньше здесь + // стояло `*item.AuthId` без проверки — паника на повреждённой + // строке внутри отсоединённой горутины. + authID := authIDOf(peer) + if authID == "" { + continue + } + if peerAccessDenied(peer, now) { + kick = append(kick, authID) + } + } + } + + // Пустой набор до `/kick` не доходит: раньше запрос с пустым массивом в + // теле уезжал в Hysteria каждые 30 секунд. + return disconnectAuthIDs(kick) } diff --git a/apps/service/cron_test.go b/apps/service/cron_test.go new file mode 100644 index 0000000..c3f5909 --- /dev/null +++ b/apps/service/cron_test.go @@ -0,0 +1,566 @@ +package service + +import ( + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "hy2xs-admin/dao" + "hy2xs-admin/model/bo" + "hy2xs-admin/model/constant" +) + +// Цикл учёта проверяется против НАСТОЯЩЕГО Traffic Stats API. +// +// accountStatsStub добавляет к trafficStatsStub то, чего у него нет: ответ +// `GET /traffic`. Разделять их не нужно — на живом сервере это один и тот же +// API на одном порту, и джоба ходит в оба маршрута подряд. + +type accountStatsStub struct { + mu sync.Mutex + + traffic map[string]bo.Hysteria2UserTraffic + online map[string]int64 + + trafficStatus int + onlineStatus int + kickStatus int + + trafficCalls int + onlineCalls int + kickCalls int + kickedKeys [][]string + + // trafficCleared запоминает, просила ли админка обнулить счётчики. + trafficCleared []bool + // usageAtOnline — суммарный расход пиров на момент запроса `/online`. + // Именно этим доказывается порядок «сначала учёт, потом enforcement»: + // после джобы оба шага уже выполнены и проверять там нечего. + usageAtOnline []map[string]int64 +} + +func (s *accountStatsStub) usageSnapshot() map[string]int64 { + usage := map[string]int64{} + peers, err := dao.ListPeer("1=1") + if err != nil { + return usage + } + for _, peer := range peers { + if peer.AuthId == nil { + continue + } + var total int64 + if peer.DownloadBytes != nil { + total += *peer.DownloadBytes + } + if peer.UploadBytes != nil { + total += *peer.UploadBytes + } + usage[*peer.AuthId] = total + } + return usage +} + +func startAccountStats(t *testing.T, stub *accountStatsStub) *accountStatsStub { + t.Helper() + + if stub == nil { + stub = &accountStatsStub{} + } + if stub.traffic == nil { + stub.traffic = map[string]bo.Hysteria2UserTraffic{} + } + if stub.online == nil { + stub.online = map[string]int64{} + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + stub.mu.Lock() + defer stub.mu.Unlock() + + switch r.URL.Path { + case "/traffic": + stub.trafficCalls++ + stub.trafficCleared = append(stub.trafficCleared, r.URL.Query().Get("clear") == "1") + if stub.trafficStatus != 0 { + w.WriteHeader(stub.trafficStatus) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(stub.traffic) + case "/online": + stub.onlineCalls++ + stub.usageAtOnline = append(stub.usageAtOnline, stub.usageSnapshot()) + if stub.onlineStatus != 0 { + w.WriteHeader(stub.onlineStatus) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(stub.online) + case "/kick": + stub.kickCalls++ + var keys []string + if err := json.NewDecoder(r.Body).Decode(&keys); err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + stub.kickedKeys = append(stub.kickedKeys, keys) + if stub.kickStatus != 0 { + w.WriteHeader(stub.kickStatus) + return + } + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + + pointHysteriaConfigAt(t, server.URL) + if err := dao.UpsertConfigValue(constant.Hysteria2TrafficStatsSecret, testTrafficStatsSecret); err != nil { + t.Fatalf("не удалось записать секрет Traffic Stats API: %v", err) + } + return stub +} + +func (s *accountStatsStub) kicked() []string { + s.mu.Lock() + defer s.mu.Unlock() + + out := []string{} + for _, keys := range s.kickedKeys { + out = append(out, keys...) + } + return out +} + +// peerUsage помещает пиру расход и настройки доступа. +func peerUsage(t *testing.T, id int64, updates map[string]interface{}) { + t.Helper() + if err := dao.UpdatePeer([]int64{id}, updates); err != nil { + t.Fatalf("подготовка состояния пира: %v", err) + } +} + +// --- Границы принудительного отключения -------------------------------------- + +// Главная регрессия QUOTA-01: cron требовал СТРОГОГО превышения квоты, а +// авторизация отказывала уже при равенстве. Пир с исчерпанной квотой не +// пускался заново, но его живая сессия не разрывалась никогда — он продолжал +// пользоваться доступом, пока не переподключался сам. +func TestCronKicksPeerAtExactQuota(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{ + "quota_bytes": int64(1_000), + "download_bytes": int64(600), + "upload_bytes": int64(400), + }) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 || got[0] != "alpha-auth-id" { + t.Fatalf("пир с исчерпанной квотой не отключён: %v", got) + } +} + +// Нулевая квота — это ноль байтов, а не безлимит. Прежнее условие +// `quota_bytes > 0` такую строку не рассматривало вовсе. +func TestCronKicksPeerWithZeroQuota(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"quota_bytes": int64(0)}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 { + t.Fatalf("пир с нулевой квотой не отключён: %v", got) + } +} + +func TestCronDoesNotKickUnlimitedQuota(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{ + "quota_bytes": int64(-1), + "download_bytes": int64(1 << 40), + }) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 0 { + t.Fatalf("безлимитный пир отключён по квоте: %v", got) + } +} + +func TestCronKicksPeerWhenExpiryEqualsNow(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + // Срок в недавнем прошлом: «момент наступил» и «момент прошёл» — по + // контракту одно и то же, а точное совпадение с now в тесте недостижимо. + peerUsage(t, id, map[string]interface{}{"expires_at": time.Now().UnixMilli() - 1}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 { + t.Fatalf("пир с истёкшим сроком не отключён: %v", got) + } +} + +// Блокировка «до» момента, который уже наступил, закончилась: пира отключать +// не за что. +func TestCronDoesNotKickAfterBanExpired(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"banned_until": time.Now().UnixMilli() - 1}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 0 { + t.Fatalf("пир с истёкшей блокировкой отключён: %v", got) + } +} + +func TestCronKicksBannedPeer(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"banned_until": time.Now().UnixMilli() + 3_600_000}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 { + t.Fatalf("заблокированный пир не отключён: %v", got) + } +} + +func TestCronKicksDisabledPeer(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"disabled": int64(1)}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 { + t.Fatalf("отключённый пир не отключён: %v", got) + } +} + +// Действующий пир не трогается, и запрос без единой цели не отправляется вовсе: +// раньше POST с пустым массивом уезжал в Hysteria каждые 30 секунд. +func TestCronSendsNoKickWithoutTargets(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + seedPeer(t, "alpha1", "alpha-auth-id") + + CronHandleAccount() + + stub.mu.Lock() + calls := stub.kickCalls + stub.mu.Unlock() + if calls != 0 { + t.Fatalf("вызов /kick без единой цели: %d", calls) + } +} + +// Пир, которого Hysteria не считает онлайн, в enforcement не участвует: рвать +// у него нечего. +func TestCronIgnoresOfflinePeers(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{}}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"disabled": int64(1)}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 0 { + t.Fatalf("офлайн-пир попал в /kick: %v", got) + } +} + +// --- Порядок и устройство цикла ---------------------------------------------- + +// Enforcement принимает решение по счётчикам, поэтому счётчики обязаны быть +// обновлены ДО него. Раньше обе половины запускались параллельными горутинами, +// и превышение квоты замечалось в лучшем случае со следующего тика. +func TestCronCollectsTrafficBeforeEnforcing(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{ + traffic: map[string]bo.Hysteria2UserTraffic{ + "alpha-auth-id": {Rx: 600, Tx: 400}, + }, + online: map[string]int64{"alpha-auth-id": 1}, + }) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"quota_bytes": int64(1_000)}) + + CronHandleAccount() + + stub.mu.Lock() + usage := stub.usageAtOnline + stub.mu.Unlock() + + if len(usage) == 0 { + t.Fatal("enforcement не выполнялся") + } + if got := usage[0]["alpha-auth-id"]; got != 1_000 { + t.Fatalf("enforcement увидел расход %d — дельта ещё не была записана", got) + } + // И следствие: превышение замечено в ТОМ ЖЕ тике, а не в следующем. + if kicked := stub.kicked(); len(kicked) != 1 { + t.Fatalf("исчерпавший квоту пир не отключён в том же цикле: %v", kicked) + } +} + +// Чтение трафика деструктивно по контракту Traffic Stats API: без clear=1 +// счётчики Hysteria не обнуляются, и следующий сбор посчитал бы тот же трафик +// повторно. +func TestCronClearsTrafficCounters(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{ + traffic: map[string]bo.Hysteria2UserTraffic{"alpha-auth-id": {Rx: 1, Tx: 1}}, + }) + seedPeer(t, "alpha1", "alpha-auth-id") + + CronHandleAccount() + + stub.mu.Lock() + cleared := stub.trafficCleared + stub.mu.Unlock() + + if len(cleared) != 1 || !cleared[0] { + t.Fatalf("сбор трафика выполнен без clear=1: %v", cleared) + } +} + +// Дельта трафика, которую не удалось приписать пиру, считается потерей и +// попадает в исход джобы. +// +// Чтение `?clear=1` деструктивно по контракту Traffic Stats API: счётчики +// Hysteria обнуляются сразу после отправки ответа, поэтому каждая дельта +// существует ровно в одном экземпляре. Раньше такой случай делал `continue` и +// не оставлял следа вовсе. +func TestSaveAccountTrafficCountsLostDeltas(t *testing.T) { + newTestDB(t) + startAccountStats(t, &accountStatsStub{ + traffic: map[string]bo.Hysteria2UserTraffic{ + "known-auth-id": {Rx: 10, Tx: 20}, + "unknown-auth-id": {Rx: 30, Tx: 40}, + }, + }) + seedPeer(t, "alpha1", "known-auth-id") + + apiPort, err := GetHysteria2ApiPort() + if err != nil { + t.Fatalf("порт Traffic Stats API: %v", err) + } + + err = saveAccountTraffic(apiPort, testTrafficStatsSecret) + if err == nil { + t.Fatal("потеря дельты не сообщена вызывающему") + } + var loss *trafficLossError + if !errors.As(err, &loss) { + t.Fatalf("потеря сообщена не как величина: %v", err) + } + if loss.lost != 1 { + t.Fatalf("учтено %d потерь, ожидалась 1", loss.lost) + } + if !strings.Contains(err.Error(), "1") { + t.Errorf("сообщение не называет количество: %q", err.Error()) + } + + // Известный пир при этом обязан получить свою дельту: потеря одной записи + // не отменяет остальных. + peer := snapshotPeers(t)["alpha1"] + if *peer.DownloadBytes != 10 || *peer.UploadBytes != 20 { + t.Fatalf("дельта известного пира не записана: %d/%d", *peer.DownloadBytes, *peer.UploadBytes) + } +} + +// Мнение systemd на цикл учёта не влияет. +// +// Прежний гейт `if !Hysteria2IsRunning() { return }` стоял на решении о +// применении операции, а util.Exec не отличает «служба неактивна» от +// «спросить не удалось»: сломанный systemctl при живой Hysteria молча отключал +// и учёт трафика, и принудительное отключение — без единой строки в журнале. +func TestCronRunsWhenSystemdSaysStopped(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + withHysteriaRunning(t, false) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"disabled": int64(1)}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 { + t.Fatalf("мнение systemd отключило принудительное отключение: %v", got) + } +} + +// Отсутствующее значение секрета Traffic Stats API — отказ джобы, а не паника. +// +// Раньше здесь стояло `*trafficSecretConfig.Value` без проверки, причём внутри +// отсоединённой горутины: разыменование nil роняло бы весь процесс вместе с +// обработчиком machine-auth, а не одну джобу. +func TestCronSurvivesMissingTrafficSecret(t *testing.T) { + newTestDB(t) + startAccountStats(t, nil) + seedPeer(t, "alpha1", "alpha-auth-id") + + // Пустое значение ключа неотличимо от его отсутствия: и то и другое + // означает «секрета нет». Прежний путь читал `*config.Value` без проверки + // и на строке без значения падал с nil-разыменованием. + if err := dao.UpsertConfigValue(constant.Hysteria2TrafficStatsSecret, ""); err != nil { + t.Fatalf("не удалось стереть секрет: %v", err) + } + + defer func() { + if recovered := recover(); recovered != nil { + t.Fatalf("отсутствующий секрет уронил джобу учёта: %v", recovered) + } + }() + + CronHandleAccount() +} + +// Строка пира без идентификатора не роняет сброс трафика: раньше `*item.Id` +// разыменовывался без проверки. +func TestCronResetTrafficSurvivesRowWithoutID(t *testing.T) { + newTestDB(t) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"download_bytes": int64(100), "upload_bytes": int64(200)}) + + defer func() { + if recovered := recover(); recovered != nil { + t.Fatalf("сброс трафика упал: %v", recovered) + } + }() + + CronResetTraffic() + + peer := snapshotPeers(t)["alpha1"] + if *peer.DownloadBytes != 0 || *peer.UploadBytes != 0 { + t.Fatalf("счётчики не сброшены: %d/%d", *peer.DownloadBytes, *peer.UploadBytes) + } +} + +// Отказ `/traffic` не отменяет enforcement: политика применяется по уже +// известным счётчикам, а не пропускается вместе со сбором. +func TestCronEnforcesEvenWhenTrafficCollectionFails(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{ + trafficStatus: http.StatusInternalServerError, + online: map[string]int64{"alpha-auth-id": 1}, + }) + id := seedPeer(t, "alpha1", "alpha-auth-id") + peerUsage(t, id, map[string]interface{}{"disabled": int64(1)}) + + CronHandleAccount() + + if got := stub.kicked(); len(got) != 1 { + t.Fatalf("отказ сбора трафика отменил принудительное отключение: %v", got) + } +} + +// Второй тик поверх идущего цикла не запускает второй цикл. Проверяется +// наблюдаемым следствием: при удерживаемом мьютексе джоба обязана вернуться, +// не сходив в Hysteria ни разу. +func TestCronHandleAccountSkipsOverlappingTick(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, nil) + seedPeer(t, "alpha1", "alpha-auth-id") + + accountJobMutex.Lock() + CronHandleAccount() + accountJobMutex.Unlock() + + stub.mu.Lock() + calls := stub.trafficCalls + stub.onlineCalls + stub.mu.Unlock() + if calls != 0 { + t.Fatalf("параллельный тик запустил второй цикл учёта: обращений %d", calls) + } + + // А после освобождения обычный тик проходит. + CronHandleAccount() + stub.mu.Lock() + calls = stub.trafficCalls + stub.onlineCalls + stub.mu.Unlock() + if calls == 0 { + t.Fatal("цикл учёта не выполнился после освобождения мьютекса") + } +} + +// Джоба СИНХРОННА: планировщик обязан видеть её работу, иначе StopCron +// возвращается, releaseResource закрывает SQLite, а недобитые горутины +// продолжают писать в закрытое соединение. +// +// Доказывается тем, что к моменту возврата CronHandleAccount вся работа уже +// сделана — при отсоединённых горутинах обращения к Hysteria к этому моменту +// ещё не случились бы. +func TestCronHandleAccountIsSynchronous(t *testing.T) { + newTestDB(t) + stub := startAccountStats(t, &accountStatsStub{ + traffic: map[string]bo.Hysteria2UserTraffic{"alpha-auth-id": {Rx: 10, Tx: 20}}, + online: map[string]int64{"alpha-auth-id": 1}, + }) + seedPeer(t, "alpha1", "alpha-auth-id") + + CronHandleAccount() + + stub.mu.Lock() + trafficCalls := stub.trafficCalls + onlineCalls := stub.onlineCalls + stub.mu.Unlock() + + if trafficCalls != 1 || onlineCalls != 1 { + t.Fatalf("работа не завершена к возврату джобы: /traffic %d, /online %d", trafficCalls, onlineCalls) + } + + // И записанная дельта уже видна: значит цикл дошёл до конца, а не был + // передан горутине. + peer := snapshotPeers(t)["alpha1"] + if *peer.DownloadBytes != 10 || *peer.UploadBytes != 20 { + t.Fatalf("дельта не записана к возврату джобы: %d/%d", *peer.DownloadBytes, *peer.UploadBytes) + } +} + +// StopCron дожидается запущенной джобы учёта. Раньше внешняя горутина +// заканчивалась мгновенно, и планировщику было нечего ждать. +func TestStopCronWaitsForAccountJob(t *testing.T) { + newTestDB(t) + startAccountStats(t, nil) + seedPeer(t, "alpha1", "alpha-auth-id") + + // Джоба удерживается занятым мьютексом: пока он не освобождён, ни один + // цикл учёта не идёт, и StopCron обязан вернуться без ожидания. + done := make(chan struct{}) + go func() { + defer close(done) + accountJobMutex.Lock() + defer accountJobMutex.Unlock() + time.Sleep(50 * time.Millisecond) + }() + + if err := InitCron(); err != nil { + t.Fatalf("InitCron: %v", err) + } + StopCron() + <-done + + if count := CronEntryCount(); count != 0 { + t.Fatalf("после остановки осталось %d записей", count) + } +} diff --git a/apps/service/hysteria2.go b/apps/service/hysteria2.go index a87713c..bfa6bd1 100644 --- a/apps/service/hysteria2.go +++ b/apps/service/hysteria2.go @@ -33,11 +33,16 @@ func InitHysteria2() error { // одну ошибку. Различить их здесь нельзя, поэтому false означает «служба // неактивна ИЛИ спросить не получилось». // -// Отсюда правило, которое стоило продукту двух дыр: на этом значении нельзя +// Отсюда правило, которое стоило продукту трёх дыр: на этом значении нельзя // строить решения о доступе и о применении операции. Ему место в отображении // (дашборд, признак online в списке), где ошибочное «выключено» стоит одной // неверной плашки. Решения о доступе принимаются по фактическому ответу -// Traffic Stats API — см. hysteria2Online и DisconnectPeers. +// Traffic Stats API — см. hysteria2Online и disconnectAuthIDs. +// +// Третья дыра была самой дорогой и жила в cron: гейт `if !Hysteria2IsRunning() +// { return }` стоял перед всем циклом учёта, поэтому сломанный systemctl при +// живой Hysteria молча отключал и сбор трафика, и принудительное отключение — +// без единой строки в журнале. func Hysteria2IsRunning() bool { return hysteria2IsRunning() } diff --git a/apps/service/hysteria2_api.go b/apps/service/hysteria2_api.go index 8d1fa82..5ad5ba5 100644 --- a/apps/service/hysteria2_api.go +++ b/apps/service/hysteria2_api.go @@ -7,6 +7,7 @@ import ( "hy2xs-admin/model/bo" "hy2xs-admin/model/constant" "hy2xs-admin/proxy" + "hy2xs-admin/util" "net" "net/url" "os" @@ -62,15 +63,20 @@ func Hysteria2Auth(conPass string) (int64, string, error) { if digestErr != nil { return 0, "", digestErr } - peer, err := dao.GetPeer(`secret_digest = ? - and disabled = 0 - and (quota_bytes < 0 or quota_bytes > download_bytes + upload_bytes) - and (expires_at = 0 or ? < expires_at) - and ? > banned_until`, secretDigest, now, now) + // Поиск идёт ТОЛЬКО по учётным данным. Политика доступа больше не живёт + // внутри выборки: её объявляет peerAccessDenied, и ровно её же применяет + // принудительное отключение в cron. Пока правило было записано двумя + // разными SQL-условиями, авторизация и enforcement расходились на границах + // quota, expiry и ban — см. комментарий в peer_access.go. + peer, err := dao.GetPeer("secret_digest = ?", secretDigest) if err != nil { return 0, "", err } + if peerAccessDenied(peer, now) { + return 0, "", errors.New("peer access denied") + } + // Строка без идентичности — повреждённые данные, а не пир. // // Проверка стоит здесь по той же причине, что и проверка maxDevices ниже: @@ -126,7 +132,15 @@ func Hysteria2Auth(conPass string) (int64, string, error) { return 0, "", errors.New("device limit unavailable") } - if device, exist := onlineUsers[*peer.AuthId]; exist && *peer.MaxDevices <= device { + // Место занимается ПОСЛЕ всех остальных проверок и с учётом уже выданных, + // но ещё не проявившихся разрешений — см. peer_admission.go. Сравнение + // одного лишь ответа `/online` пропускало параллельные подключения: между + // чтением и ответом «allow» ничего не удерживало место, и два одновременных + // запроса при `online=2, max=3` получали разрешение оба. + // + // Порядок существенен: если бы резервация делалась раньше проверки + // квоты или срока, отказ по ним съедал бы слот на всё время TTL. + if !reserveDeviceSlot(*peer.AuthId, onlineUsers[*peer.AuthId], *peer.MaxDevices, time.Now()) { return 0, "", errors.New("device limited") } @@ -185,43 +199,54 @@ func hysteria2TrafficSecret() (string, error) { return *trafficSecretConfig.Value, nil } -// DisconnectPeers завершает активные Hysteria-сессии пиров и НИЧЕГО не пишет в -// базу. +// kickChunkSize ограничивает размер одного обращения к `/kick`. // -// Разрыв сессии и запись состояния разделены сознательно. Прежний -// Hysteria2Kick делал и то и другое: вместе с обращением к `/kick` он -// проставлял `banned_until`. Из-за этого им нельзя было воспользоваться для -// отключения пира — операция `disabled=1` записала бы заодно временную -// блокировку, а это другой механизм с другим сроком жизни и другим способом -// снятия. Единственный вызывающий (KickPeer) при этом писал `banned_until` ещё -// и сам, то есть одно и то же значение уезжало в базу дважды. +// Импорт применяет до MaxPeerImportItems записей за операцию, и без разбиения +// в Hysteria уехал бы один POST с многотысячным массивом в теле. Значение +// выбрано с запасом относительно любого реального размера панели: смысл здесь +// не в оптимизации, а в отсутствии запроса, размер которого задаёт содержимое +// пользовательского файла. +const kickChunkSize = 100 + +// disconnectAuthIDs — ЕДИНСТВЕННЫЙ путь к Traffic Stats `/kick` в продукте. // -// Здесь остаётся ровно официальный Traffic Stats `/kick` и ничего больше. +// Контракт предельно узкий и намеренно ничего не знает про пиров: // -// Состояние службы по systemd НЕ проверяется. Раньше путь начинался с -// `!Hysteria2IsRunning() -> отказ`, и это давало худшее из двух: ответ systemd -// не отличает «служба неактивна» от «спросить не удалось» (см. -// Hysteria2IsRunning), поэтому сбой самого systemctl превращался в отказ -// операции при живой Hysteria, а обратная ошибка молча пропускала бы разрыв. +// auth IDs -> дедупликация -> порт API -> секрет -> POST /kick +// +// Никакой базы, никакого `disabled`, никакого `banned_until`. Разрыв сессии и +// запись состояния разделены сознательно: прежний Hysteria2Kick делал и то и +// другое — вместе с обращением к `/kick` он проставлял `banned_until`, — и +// из-за этого им нельзя было воспользоваться для отключения пира: операция +// записала бы заодно временную блокировку, а это другой механизм с другим +// сроком жизни и другим способом снятия. +// +// Вход — именно auth IDs, а не идентификаторы пиров, и это не деталь. Операции +// удаления и импорта меняют или убирают auth ID: после commit действующего +// значения в базе уже нет, и рвать надо по тому, которое Hysteria знала ДО +// операции. Функция, которая сама читала бы auth ID из базы, для этих двух +// путей опоздала бы всегда. +// +// Состояние службы по systemd НЕ проверяется. Ответ systemd не отличает +// «служба неактивна» от «спросить не удалось» (см. Hysteria2IsRunning), +// поэтому сбой самого systemctl отказывал бы операции при живой Hysteria. // Обращение к `/kick` отвечает на нужный вопрос напрямую и без посредника. -func DisconnectPeers(ids []int64) error { - if len(ids) == 0 { - return nil - } - - peers, err := dao.ListPeer("id in ?", ids) - if err != nil { - return err - } - - keys := make([]string, 0, len(peers)) - for _, item := range peers { - if item.AuthId == nil || *item.AuthId == "" { +func disconnectAuthIDs(authIDs []string) error { + keys := make([]string, 0, len(authIDs)) + seen := make(map[string]struct{}, len(authIDs)) + for _, authID := range authIDs { + if authID == "" { continue } - keys = append(keys, *item.AuthId) + if _, duplicate := seen[authID]; duplicate { + continue + } + seen[authID] = struct{}{} + keys = append(keys, authID) } - // Пир без authId Hysteria не знает: рвать нечего, и это не отказ. + + // Ни одной цели — значит рвать нечего, и это не отказ. Раньше по + // аналогичному пути в cron уезжал POST с пустым массивом каждые 30 секунд. if len(keys) == 0 { return nil } @@ -234,7 +259,14 @@ func DisconnectPeers(ids []int64) error { if err != nil { return err } - return proxy.NewHysteria2Api(apiPort).KickUsers(keys, secret) + + api := proxy.NewHysteria2Api(apiPort) + for _, chunk := range util.SplitArr(keys, kickChunkSize) { + if err := api.KickUsers(chunk, secret); err != nil { + return err + } + } + return nil } func Hysteria2Url(accountId int64) (string, error) { diff --git a/apps/service/peer.go b/apps/service/peer.go index b5b1f10..6fd0905 100644 --- a/apps/service/peer.go +++ b/apps/service/peer.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "strings" + "time" "github.com/sirupsen/logrus" @@ -127,7 +128,16 @@ func CreatePeer(peerDto dto.PeerSaveDto) (vo.PeerVo, error) { } func UpdatePeer(id int64, peerDto dto.PeerUpdateDto) error { - if err := assertBootstrapPeerIdentityUnchanged(id, peerDto); err != nil { + // Снимок «до» читается ОДИН раз и обслуживает обе задачи: защиту пира + // установщика и решение о том, нужен ли разрыв живой сессии. Раньше + // assertBootstrapPeerIdentityUnchanged читал одного и того же пира до двух + // раз подряд собственными запросами. + before, err := dao.GetPeer("id = ?", id) + if err != nil { + return err + } + + if err := assertBootstrapPeerIdentityUnchanged(before, peerDto); err != nil { return err } @@ -162,75 +172,154 @@ func UpdatePeer(id int64, peerDto dto.PeerUpdateDto) error { if peerDto.Remark != nil { updates["remark"] = *peerDto.Remark } - if err := dao.UpdatePeer([]int64{id}, updates); err != nil { - return err - } - // Отключение пира — это ОБЕ половины официального контракта Hysteria. - // - // Запись `disabled=1` закрывает только будущие обращения к HTTP-auth: её - // видит условие выборки в Hysteria2Auth. Уже установленная QUIC-сессия - // живёт своей жизнью и сама по себе не разрывается — то есть после - // «Отключить» пир продолжал пользоваться доступом сколько угодно долго, - // пока не переподключался по своей воле. Панель при этом показывала его - // отключённым. - // - // Вторую половину даёт Traffic Stats `/kick`. Официальная документация - // описывает их именно как пару: `/kick` завершает сессию, но клиент - // немедленно переподключается, поэтому одновременно требуется блокировка в - // auth backend. По отдельности не работает ни одна. + // Решение принимается ДО записи, а сам разрыв — строго ПОСЛЕ неё. // // Порядок обязателен и обратному не подлежит: сначала долговременная // запись, потом разрыв. При обратном порядке клиент успевает // переподключиться в окне между `/kick` и записью — и остаётся на связи с - // формально отключённым пиром. + // пиром, чьё состояние уже изменено. + needsReconcile := updateRequiresReconcile(before, peerDto, time.Now().UnixMilli()) + + if err := dao.UpdatePeer([]int64{id}, updates); err != nil { + return err + } + + if !needsReconcile { + return nil + } + // Ровно ОДИН `/kick` при любом сочетании изменений: ротация секрета вместе + // с отключением и урезанной квотой — это по-прежнему одна операция над + // одним пиром. + return reconcileLiveSessions([]string{authIDOf(before)}) +} + +// updateRequiresReconcile отвечает, делает ли правка живую сессию устаревшей. +// +// Что было. Разрыв выполнялся при одном-единственном условии: +// +// peerDto.Disabled != nil && *peerDto.Disabled == 1 +// +// Отключение пира действительно было первым, что починили, но оно не +// единственный способ отозвать доступ через форму. Мимо проверки проходили: +// +// смена секрета — старые учётные данные недействительны, а сессия, +// установленная по ним, продолжает работать; +// урезание квоты — «100 ГБ -> 5 ГБ» при израсходованных 10 ГБ; +// перенос срока — «истекает завтра» -> «истёк вчера»; +// снижение лимита — «5 устройств -> 1» при пяти подключённых. +// +// Во всех четырёх случаях панель показывала новое состояние, а пир продолжал +// пользоваться доступом по старому — то есть ровно тот же дефект, что и в +// UX-06, только под другими именами полей. +// +// Правило асимметрично намеренно: ограничение применяется немедленно, +// послабление — нет. Увеличенная квота, продлённый срок, поднятый лимит +// устройств и правка имени или пометки сессию не рвут: у оператора нет +// причины ронять работающее соединение, расширяя пиру права. +func updateRequiresReconcile(before entity.Peer, peerDto dto.PeerUpdateDto, now int64) bool { + // Смена секрета. Учётные данные, по которым сессия была установлена, с + // этого момента недействительны — держать её открытой нечем. + if peerDto.Secret != nil && strings.TrimSpace(*peerDto.Secret) != "" { + return true + } + + // Запрошенное `disabled=1`, а не переход из включённого состояния. // - // Условие проверяет ЗАПРОШЕННОЕ состояние, а не переход из включённого. // Так операция остаётся повторяемой: если разрыв не удался, оператор // повторяет «Отключить» и получает вторую попытку, вместо того чтобы // включать пира ради возможности отключить его снова. if peerDto.Disabled != nil && *peerDto.Disabled == 1 { - return disconnectAfterRevoke(id) + return true } - return nil + + // Снижение лимита устройств. + // + // Выбирать «лишнее устройство» не нужно и невозможно: `/kick` оперирует + // идентификатором клиента, а не конкретным экземпляром подключения. После + // переподключения новый admission limit пропустит ровно столько + // устройств, сколько разрешено теперь. + if peerDto.MaxDevices != nil && before.MaxDevices != nil && + *peerDto.MaxDevices < *before.MaxDevices { + return true + } + + // Квота и срок: разрыв нужен, только если значение ДЕЙСТВИТЕЛЬНО менялось + // и новое значение уже закрывает доступ. Проверка идёт через ту же + // peerAccessDenied, что и авторизация, поэтому «закрывает доступ» здесь и + // «не пустит при следующем подключении» — буквально одно условие. + after := before + changed := false + if peerDto.QuotaBytes != nil && (before.QuotaBytes == nil || *peerDto.QuotaBytes != *before.QuotaBytes) { + quota := *peerDto.QuotaBytes + after.QuotaBytes = "a + changed = true + } + if peerDto.ExpiresAt != nil && (before.ExpiresAt == nil || *peerDto.ExpiresAt != *before.ExpiresAt) { + expires := *peerDto.ExpiresAt + after.ExpiresAt = &expires + changed = true + } + if !changed { + return false + } + // Пир, которому доступ был закрыт и до правки, отдельного разрыва не + // требует: его сессию уже завершил тот, кто закрыл доступ. + return peerAccessDenied(after, now) && !peerAccessDenied(before, now) } -// disconnectAfterRevoke рвёт сессии пира после уже применённой записи. +// reconcileLiveSessions приводит живые сессии Hysteria в соответствие с уже +// СОХРАНЁННЫМ состоянием. // -// Отказ НЕ откатывает состояние: безопасная его половина достигнута, и -// возвращать пиру полный доступ из-за неудачи второго шага нельзя. Вызывающему +// Единственный путь для всех операций, способных сделать живую сессию +// устаревшей: отключение пира, временная блокировка, смена секрета, урезание +// квоты и срока, снижение лимита устройств, импорт партии и удаление пира. +// Отдельных методов kick для каждой из них нет намеренно — иначе «применили +// изменение, но забыли завершить сессию» появлялось бы заново с каждой новой +// операцией, ровно так, как это уже случилось с удалением и импортом. +// +// Вызывается СТРОГО после того, как долговременное состояние записано. +// Обратный порядок оставляет клиенту окно между `/kick` и записью, в котором +// он успевает переподключиться и остаётся на связи с уже изменённым пиром. +// +// Отказ НЕ откатывает состояние: безопасная половина операции достигнута, и +// возвращать доступ из-за неудачи второго шага нельзя. Вызывающему // возвращается частичный результат отдельным кодом — см. PeerDisconnectError. -func disconnectAfterRevoke(id int64) error { - if err := DisconnectPeers([]int64{id}); err != nil { +func reconcileLiveSessions(authIDs []string) error { + if err := disconnectAuthIDs(authIDs); err != nil { logrus.WithError(err). - WithField("peerId", id). - Error("peer access revoked in database, but hysteria2 session disconnect failed") + WithField("authIds", len(authIDs)). + Error("peer state persisted, but hysteria2 session disconnect failed") return PeerDisconnectError() } return nil } +// authIDOf возвращает идентификатор, которым Hysteria знает пира, или пустую +// строку. Пира без authId Hysteria не знает: рвать нечего, и это не отказ. +func authIDOf(peer entity.Peer) string { + if peer.AuthId == nil { + return "" + } + return *peer.AuthId +} + // assertBootstrapPeerIdentityUnchanged запрещает менять то, что продублировано // в /etc/hy2xs/bootstrap-admin.secret, и занимать зарезервированное имя. -func assertBootstrapPeerIdentityUnchanged(id int64, peerDto dto.PeerUpdateDto) error { +// +// Работает по УЖЕ ПРОЧИТАННОМУ снимку пира: раньше функция делала до двух +// собственных запросов за ту же строку, которую вызывающий читает и сам. +func assertBootstrapPeerIdentityUnchanged(existing entity.Peer, peerDto dto.PeerUpdateDto) error { + isBootstrap := existing.Name != nil && *existing.Name == ReservedBootstrapPeerName + // Переименование ЛЮБОГО пира в зарезервированное имя запрещено отдельно от // проверки цели: UNIQUE(name) закрывает этот путь только пока bootstrap-пир // существует. - if peerDto.Name != nil && strings.TrimSpace(*peerDto.Name) == ReservedBootstrapPeerName { - existing, err := dao.GetPeer("id = ?", id) - if err != nil { - return err - } - if existing.Name == nil || *existing.Name != ReservedBootstrapPeerName { - return ErrPeerNameReserved - } + if peerDto.Name != nil && strings.TrimSpace(*peerDto.Name) == ReservedBootstrapPeerName && !isBootstrap { + return ErrPeerNameReserved } - existing, err := dao.GetPeer("id = ?", id) - if err != nil { - return err - } - if existing.Name == nil || *existing.Name != ReservedBootstrapPeerName { + if !isBootstrap { return nil } @@ -263,7 +352,54 @@ func assertBootstrapPeerIdentityUnchanged(id int64, peerDto dto.PeerUpdateDto) e // Секрет остаётся в /etc/hy2xs/bootstrap-admin.secret и после удаления. Файлом // владеет оркестратор, админка его не трогает; после отзыва он содержит уже // недействующее значение (см. docs/admin/04-admin-panel.md). -func DeletePeer(id int64) error { return dao.DeletePeer([]int64{id}) } +// +// Порядок шагов — не стилистика, а единственный, который не оставляет пиру +// доступ. +// +// Что было: `return dao.DeletePeer([]int64{id})`. Строка исчезала, живая +// QUIC-сессия оставалась, и — хуже того — вместе со строкой исчезал `auth_id`, +// то есть единственное, чем эту сессию можно было завершить. Состояние +// становилось невосстановимым: удалённый пир пользовался доступом, пока не +// переподключался по своей воле, и сделать с этим было уже нечего. +// +// Теперь: +// +// 1. прочитать пира и запомнить его auth ID; +// 2. записать disabled=1 — закрыть будущие обращения к HTTP-auth; +// 3. завершить живые сессии по запомненному auth ID; +// 4. удалить строку. +// +// Ключевые исходы: +// +// запись не удалась -> строка не изменена, удаления не было; +// разрыв не удался -> строка остаётся с disabled=1, новые подключения +// запрещены, оператор повторяет удаление; +// разрыв прошёл, а +// удаление не удалось -> строка остаётся отключённой, сессия уже завершена. +// +// Ни один из них не возвращает пиру доступ, и отката после `/kick` нет +// намеренно: снимать достигнутое безопасное состояние из-за неудачи +// последнего шага нельзя. +// +// Запись идёт через dao, а не через сервисный UpdatePeer: guard пира +// установщика запрещает менять его ИДЕНТИЧНОСТЬ, а не отключать его, и +// пропускать законное удаление через проверку смены имени и секрета незачем. +func DeletePeer(id int64) error { + peer, err := dao.GetPeer("id = ?", id) + if err != nil { + return err + } + + if err := dao.UpdatePeer([]int64{id}, map[string]interface{}{"disabled": int64(1)}); err != nil { + return err + } + + if err := reconcileLiveSessions([]string{authIDOf(peer)}); err != nil { + return err + } + + return dao.DeletePeer([]int64{id}) +} func GetPeerVo(id int64) (vo.PeerVo, error) { p, err := dao.GetPeer("id = ?", id) @@ -309,10 +445,14 @@ func ReleaseKickPeer(id int64) error { // применённом состоянии, и оператор видел «не сработало» у сработавшей // блокировки. func KickPeer(id int64, bannedUntil int64) error { + peer, err := dao.GetPeer("id = ?", id) + if err != nil { + return err + } if err := dao.UpdatePeer([]int64{id}, map[string]interface{}{"banned_until": bannedUntil}); err != nil { return err } - return disconnectAfterRevoke(id) + return reconcileLiveSessions([]string{authIDOf(peer)}) } func BuildPeerClientConfig(id int64) (vo.PeerClientConfigVo, error) { @@ -534,6 +674,39 @@ func preparePeerImport(items []bo.PeerExport) ([]preparedPeerImport, error) { // Конфликт не гипотетический: пусть в базе есть A(auth_id=a, name=alice) и // B(auth_id=b, name=bob), а файл несёт (auth_id=a, name=bob). Поиск найдёт A // по auth_id и переименует его в bob — прямо в UNIQUE(name). +// +// Четвёртый проход появился позже трёх: живые сессии. +// +// Импорт — это bulk state replacement, а не правка пометки: он переписывает +// credential- и access-состояние существующего пира целиком, включая +// `auth_id`, `secret_digest`, `quota_bytes`, `expires_at` и `disabled`. Сессии +// при этом не трогались вовсе, поэтому пир, отключённый импортом или +// получивший новый секрет, продолжал пользоваться доступом по старому. +// +// Старые auth ID собираются ВНУТРИ транзакции и разрываются ПОСЛЕ commit. +// Оба слова важны: +// +// внутри — потому что после commit старого значения в базе уже нет: +// `auth_id` перезаписан значением из файла; +// после — потому что `/kick` до commit оставляет клиенту окно, в котором он +// переподключается к ещё не изменённому пиру. +// +// Разрываются сессии ВСЕХ существующих записей партии, а не тех, у кого +// изменилось конкретное поле. Это сознательно более простой контракт, чем diff +// по семи access-полям: +// +// 1. импорт и так переписывает состояние целиком; +// 2. старый auth ID гарантированно нужен при его замене; +// 3. повтор того же импорта после неудавшегося `/kick` обязан снова +// попытаться завершить старые сессии; +// 4. не появляется ещё одной таблицы правил «какие поля импорта считаются +// access-changing» — то есть второго места, где политика может разойтись +// с peerAccessDenied. +// +// Цена — существующие пиры из импортируемой партии один раз переподключаются. +// Для административной операции переноса это нормальная цена. +// +// Новые пиры не разрываются: до импорта их живых сессий существовать не могло. func UpsertPeerExport(items []bo.PeerExport) error { if err := ValidatePeerImportBatch(items); err != nil { return err @@ -544,26 +717,42 @@ func UpsertPeerExport(items []bo.PeerExport) error { return err } - return dao.WithPeerTx(func(tx dao.PeerTx) error { + var replacedAuthIDs []string + if err := dao.WithPeerTx(func(tx dao.PeerTx) error { + // Список собирается заново на каждой попытке: WithPeerTx может + // вызвать функцию повторно, и накопленный от прошлого прохода хвост + // означал бы разрыв сессий, которых партия не касалась. + replacedAuthIDs = replacedAuthIDs[:0] for _, entry := range prepared { - if err := applyPeerImportEntry(tx, entry); err != nil { + replaced, err := applyPeerImportEntry(tx, entry) + if err != nil { return err } + if replaced != "" { + replacedAuthIDs = append(replacedAuthIDs, replaced) + } } return nil - }) + }); err != nil { + // Транзакция откачена: состояние не менялось, разрывать нечего. + return err + } + + return reconcileLiveSessions(replacedAuthIDs) } -func applyPeerImportEntry(tx dao.PeerTx, entry preparedPeerImport) error { +// applyPeerImportEntry применяет одну запись и возвращает auth ID, который +// Hysteria знала ДО применения, — пустую строку для вновь созданной записи. +func applyPeerImportEntry(tx dao.PeerTx, entry preparedPeerImport) (string, error) { existing, found, err := findPeerForImport(tx, entry) if err != nil { - return err + return "", err } // Пир установщика не переопределяется импортом ни при каком совпадении: // его секрет живёт ещё и в /etc/hy2xs/bootstrap-admin.secret. if found && existing.Name != nil && *existing.Name == ReservedBootstrapPeerName { - return fmt.Errorf( + return "", fmt.Errorf( "peer import: пир %q принадлежит установщику и не может быть изменён импортом", ReservedBootstrapPeerName, ) @@ -589,7 +778,13 @@ func applyPeerImportEntry(tx dao.PeerTx, entry preparedPeerImport) error { updates["secret_digest"] = entry.explicitDigest updates["secret_ciphertext"] = entry.explicitCipher } - return tx.UpdatePeer([]int64{*existing.Id}, updates) + // Значение читается ДО записи: после неё в строке уже стоит auth ID из + // файла, а Hysteria знает пира по прежнему. + replaced := authIDOf(existing) + if err := tx.UpdatePeer([]int64{*existing.Id}, updates); err != nil { + return "", err + } + return replaced, nil } name := entry.name @@ -621,8 +816,9 @@ func applyPeerImportEntry(tx dao.PeerTx, entry preparedPeerImport) error { BannedUntil: &bannedUntil, LastConnectionAt: &lastConnection, } + // Вновь созданная запись: живой сессии до импорта существовать не могло. _, saveErr := tx.SavePeer(peer) - return saveErr + return "", saveErr } // findPeerForImport ищет запись, которую импорт должен обновить. diff --git a/apps/service/peer_access.go b/apps/service/peer_access.go new file mode 100644 index 0000000..818ba52 --- /dev/null +++ b/apps/service/peer_access.go @@ -0,0 +1,102 @@ +package service + +import ( + "hy2xs-admin/model/entity" +) + +// Правило доступа пира объявлено ОДИН РАЗ и живёт в Go, а не в SQL. +// +// Что было. Правило существовало в двух экземплярах, написанных разными +// условиями в разных местах. +// +// Авторизация (Hysteria2Auth) прятала его в выборке: +// +// disabled = 0 +// and (quota_bytes < 0 or quota_bytes > download_bytes + upload_bytes) +// and (expires_at = 0 or ? < expires_at) +// and ? > banned_until +// +// Принудительное отключение (cron) — в своей выборке, уже с другими границами: +// +// disabled = 1 +// or (quota_bytes > 0 and quota_bytes < download_bytes + upload_bytes) +// or (expires_at > 0 and ? > expires_at) +// or ? < banned_until +// +// Это не стилистическое дублирование. Второе условие — не отрицание первого, и +// расхождение приходилось ровно на границы: +// +// quota = 0 auth отказывает, cron сессию не рвёт +// usage = quota auth отказывает, cron сессию не рвёт +// now = expiresAt auth отказывает, cron сессию не рвёт +// now = bannedUntil auth отказывает, cron сессию не рвёт +// +// Хуже всего вела себя исчерпанная квота. `quota_bytes < download + upload` +// требует СТРОГОГО превышения, а счётчики растут порциями по ответу Traffic +// Stats API, поэтому попадание в точное равенство — не экзотика, а обычный +// исход последнего сбора. Пир с исчерпанной квотой не пускался заново, но его +// живая сессия не разрывалась НИКОГДА: он продолжал пользоваться доступом, +// пока не переподключался по своей воле. +// +// Поэтому политика перестаёт быть частью запроса и становится функцией +// продукта. Авторизация и enforcement физически не могут разойтись, потому что +// спрашивают одно и то же. +// +// Производительность здесь не страдает: авторизация всё равно ищет ОДНУ строку +// по secret_digest, а cron всё равно читает пиров, которых Hysteria назвала +// онлайн. + +// peerAccessDenied отвечает на единственный вопрос: закрыт ли доступ пиру +// прямо сейчас. +// +// disabled == 1 -> DENY +// quotaBytes < 0 -> квота не ограничена +// quotaBytes >= 0 && usage >= quotaBytes -> DENY +// expiresAt > 0 && now >= expiresAt -> DENY +// bannedUntil > now -> DENY +// иначе -> ALLOW +// +// Границы выбраны по смыслу самих названий: +// +// quota = -1 единственный способ сказать «без ограничения»; +// quota = 0 нулевая квота — это ноль байтов, а не безлимит; +// usage = quota выданный лимит уже израсходован целиком; +// expiresAt = now срок доступа уже наступил, то есть истёк; +// bannedUntil = now временная блокировка уже закончилась. +// +// Отрицательная квота любой величины означает «без ограничения» — так же, как +// это делала выборка авторизации (`quota_bytes < 0`). Через двери продукта +// значение меньше -1 недостижимо: и dto.PeerSaveDto, и валидация импорта +// требуют `>= -1`. Канон один — `-1`. +func peerAccessDenied(peer entity.Peer, now int64) bool { + // Строка без решающего поля — повреждённые данные, а не пир без + // ограничений. + // + // Все эти колонки объявлены NOT NULL с DEFAULT, поэтому nil здесь может + // означать только повреждение. На пути принятия решения о доступе такая + // строка обязана вести к отказу: молчаливое «поле не задано, значит можно» + // — это ровно тот способ, которым ограничение перестаёт быть ограничением. + if peer.Disabled == nil || peer.QuotaBytes == nil || + peer.DownloadBytes == nil || peer.UploadBytes == nil || + peer.ExpiresAt == nil || peer.BannedUntil == nil { + return true + } + + if *peer.Disabled == 1 { + return true + } + + if *peer.QuotaBytes >= 0 && *peer.DownloadBytes+*peer.UploadBytes >= *peer.QuotaBytes { + return true + } + + if *peer.ExpiresAt > 0 && now >= *peer.ExpiresAt { + return true + } + + if *peer.BannedUntil > now { + return true + } + + return false +} diff --git a/apps/service/peer_access_policy_test.go b/apps/service/peer_access_policy_test.go new file mode 100644 index 0000000..3ec50e7 --- /dev/null +++ b/apps/service/peer_access_policy_test.go @@ -0,0 +1,200 @@ +package service + +import ( + "testing" + + "hy2xs-admin/model/entity" +) + +// Границы политики доступа проверяются ТОЧЕЧНО, а не «примерно». +// +// Именно на границах два прежних экземпляра правила и расходились: авторизация +// не пускала пира при `usage == quota`, а принудительное отключение требовало +// строгого превышения и сессию не рвало. Пир с исчерпанной квотой не мог +// подключиться заново, но и не отключался никогда. +// +// Поэтому набор состоит в основном из равенств: `quota == usage`, +// `now == expiresAt`, `now == bannedUntil`, `quota == 0`. + +const policyNow = int64(1_700_000_000_000) + +// policyPeer собирает пира с полным набором решающих полей. +func policyPeer(disabled, quota, download, upload, expiresAt, bannedUntil int64) entity.Peer { + return entity.Peer{ + Disabled: &disabled, + QuotaBytes: "a, + DownloadBytes: &download, + UploadBytes: &upload, + ExpiresAt: &expiresAt, + BannedUntil: &bannedUntil, + } +} + +func TestPeerAccessDeniedBoundaries(t *testing.T) { + cases := []struct { + name string + peer entity.Peer + denied bool + }{ + { + name: "обычный пир без ограничений", + peer: policyPeer(0, -1, 1_000, 2_000, 0, 0), + denied: false, + }, + { + name: "отключён оператором", + peer: policyPeer(1, -1, 0, 0, 0, 0), + denied: true, + }, + + // --- квота --- + { + // Единственный способ сказать «без ограничения». + name: "quota = -1 — безлимит при любом расходе", + peer: policyPeer(0, -1, 1<<40, 1<<40, 0, 0), + denied: false, + }, + { + // Ноль байтов — это ноль байтов, а не отсутствие ограничения. + // Прежний cron на такой строке сессию не рвал вовсе. + name: "quota = 0 — доступа нет", + peer: policyPeer(0, 0, 0, 0, 0, 0), + denied: true, + }, + { + // Главная граница QUOTA-01: счётчики растут порциями по ответу + // Traffic Stats API, поэтому точное равенство — обычный исход + // последнего сбора, а не экзотика. + name: "usage = quota — лимит исчерпан", + peer: policyPeer(0, 1_000, 600, 400, 0, 0), + denied: true, + }, + { + name: "usage на байт меньше квоты", + peer: policyPeer(0, 1_000, 600, 399, 0, 0), + denied: false, + }, + { + name: "usage больше квоты", + peer: policyPeer(0, 1_000, 600, 401, 0, 0), + denied: true, + }, + { + // Квота считается по сумме обоих направлений. + name: "квота исчерпана одним download", + peer: policyPeer(0, 1_000, 1_000, 0, 0, 0), + denied: true, + }, + + // --- срок действия --- + { + name: "expiresAt = 0 — срок не задан", + peer: policyPeer(0, -1, 0, 0, 0, 0), + denied: false, + }, + { + // Момент наступил — значит уже истёк. + name: "expiresAt = now — срок истёк", + peer: policyPeer(0, -1, 0, 0, policyNow, 0), + denied: true, + }, + { + name: "expiresAt на миллисекунду впереди", + peer: policyPeer(0, -1, 0, 0, policyNow+1, 0), + denied: false, + }, + { + name: "expiresAt в прошлом", + peer: policyPeer(0, -1, 0, 0, policyNow-1, 0), + denied: true, + }, + + // --- временная блокировка --- + { + name: "bannedUntil = 0 — блокировки нет", + peer: policyPeer(0, -1, 0, 0, 0, 0), + denied: false, + }, + { + // Блокировка «до» этого момента уже закончилась. + name: "bannedUntil = now — блокировка истекла", + peer: policyPeer(0, -1, 0, 0, 0, policyNow), + denied: false, + }, + { + name: "bannedUntil в будущем", + peer: policyPeer(0, -1, 0, 0, 0, policyNow+1), + denied: true, + }, + { + name: "bannedUntil в прошлом", + peer: policyPeer(0, -1, 0, 0, 0, policyNow-1), + denied: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := peerAccessDenied(tc.peer, policyNow); got != tc.denied { + t.Fatalf("peerAccessDenied = %v, ожидалось %v", got, tc.denied) + } + }) + } +} + +// Строка без решающего поля — повреждённые данные, а не пир без ограничений. +// Все эти колонки объявлены NOT NULL с DEFAULT, поэтому nil здесь означать +// может только повреждение, и на пути принятия решения о доступе оно обязано +// вести к отказу. +func TestPeerAccessDeniedIsFailClosedOnCorruptedRow(t *testing.T) { + fields := map[string]func(*entity.Peer){ + "disabled": func(p *entity.Peer) { p.Disabled = nil }, + "quota_bytes": func(p *entity.Peer) { p.QuotaBytes = nil }, + "download_bytes": func(p *entity.Peer) { p.DownloadBytes = nil }, + "upload_bytes": func(p *entity.Peer) { p.UploadBytes = nil }, + "expires_at": func(p *entity.Peer) { p.ExpiresAt = nil }, + "banned_until": func(p *entity.Peer) { p.BannedUntil = nil }, + } + + for field, corrupt := range fields { + t.Run(field, func(t *testing.T) { + // Исходный пир заведомо имеет доступ: отказ обязан прийти именно + // от повреждения, а не от прочих ограничений. + peer := policyPeer(0, -1, 0, 0, 0, 0) + corrupt(&peer) + + if !peerAccessDenied(peer, policyNow) { + t.Fatalf("строка без %s принята как пир без ограничений", field) + } + }) + } +} + +// Авторизация и принудительное отключение обязаны отвечать ОДИНАКОВО на любом +// состоянии: расхождение между ними — это и есть класс дефектов QUOTA-01. +// +// Проверяется свойство, а не реализация: обе стороны спрашивают одну функцию, +// поэтому тест ловит попытку завести второй предикат, а не текущий код. +func TestPeerAccessDecisionIsTotal(t *testing.T) { + values := []int64{-1, 0, 1_000} + times := []int64{policyNow - 1, policyNow, policyNow + 1} + + for _, quota := range values { + for _, used := range []int64{0, 500, 1_000, 1_500} { + for _, expires := range append([]int64{0}, times...) { + for _, banned := range append([]int64{0}, times...) { + peer := policyPeer(0, quota, used, 0, expires, banned) + + first := peerAccessDenied(peer, policyNow) + second := peerAccessDenied(peer, policyNow) + if first != second { + t.Fatalf( + "решение недетерминировано: quota=%d used=%d expires=%d banned=%d", + quota, used, expires, banned, + ) + } + } + } + } + } +} diff --git a/apps/service/peer_access_test.go b/apps/service/peer_access_test.go index b1bf309..5755c0b 100644 --- a/apps/service/peer_access_test.go +++ b/apps/service/peer_access_test.go @@ -226,14 +226,13 @@ func requireDisconnectError(t *testing.T, err error) { } } -// --- DisconnectPeers --------------------------------------------------------- +// --- disconnectAuthIDs ------------------------------------------------------- -func TestDisconnectPeersSendsOfficialKickContract(t *testing.T) { +func TestDisconnectAuthIDsSendsOfficialKickContract(t *testing.T) { newTestDB(t) stub := startTrafficStats(t, nil) - id := seedPeer(t, "alpha1", "alpha-auth-id") - if err := DisconnectPeers([]int64{id}); err != nil { + if err := disconnectAuthIDs([]string{"alpha-auth-id"}); err != nil { t.Fatalf("разрыв сессии отказал: %v", err) } @@ -252,12 +251,12 @@ func TestDisconnectPeersSendsOfficialKickContract(t *testing.T) { // проставлял banned_until, из-за чего им нельзя было воспользоваться для // операции «Отключить»: она записала бы временную блокировку — другой механизм // с другим сроком жизни. -func TestDisconnectPeersDoesNotTouchPeerState(t *testing.T) { +func TestDisconnectAuthIDsDoesNotTouchPeerState(t *testing.T) { newTestDB(t) startTrafficStats(t, nil) - id := seedPeer(t, "alpha1", "alpha-auth-id") + seedPeer(t, "alpha1", "alpha-auth-id") - if err := DisconnectPeers([]int64{id}); err != nil { + if err := disconnectAuthIDs([]string{"alpha-auth-id"}); err != nil { t.Fatalf("разрыв сессии отказал: %v", err) } @@ -270,17 +269,18 @@ func TestDisconnectPeersDoesNotTouchPeerState(t *testing.T) { } } -func TestDisconnectPeersIsNoopWithoutTargets(t *testing.T) { +// Запрос без единой цели не отправляется вовсе. Раньше по аналогичному пути в +// cron уезжал POST с пустым массивом в теле каждые 30 секунд. +func TestDisconnectAuthIDsIsNoopWithoutTargets(t *testing.T) { newTestDB(t) stub := startTrafficStats(t, nil) - if err := DisconnectPeers(nil); err != nil { + if err := disconnectAuthIDs(nil); err != nil { t.Fatalf("пустой список признан отказом: %v", err) } // Пир без authId Hysteria не знает: рвать нечего. - id := seedPeer(t, "alpha1", "") - if err := DisconnectPeers([]int64{id}); err != nil { - t.Fatalf("пир без authId признан отказом: %v", err) + if err := disconnectAuthIDs([]string{"", ""}); err != nil { + t.Fatalf("набор из пустых идентификаторов признан отказом: %v", err) } if stub.kickCalls != 0 { @@ -288,12 +288,59 @@ func TestDisconnectPeersIsNoopWithoutTargets(t *testing.T) { } } -func TestDisconnectPeersReportsApiFailure(t *testing.T) { +// Один и тот же идентификатор — один разрыв. Импорт и правка пира легко дают +// повторы, и слать их в Hysteria по разу на каждое вхождение незачем. +func TestDisconnectAuthIDsDeduplicates(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + + if err := disconnectAuthIDs([]string{"a", "b", "a", "", "b", "a"}); err != nil { + t.Fatalf("разрыв сессии отказал: %v", err) + } + + if stub.kickCalls != 1 { + t.Fatalf("ожидался один вызов /kick, было %d", stub.kickCalls) + } + if got := stub.kickedKeys[0]; len(got) != 2 { + t.Fatalf("дубликаты уехали в Hysteria: %v", got) + } +} + +// Размер запроса задаёт продукт, а не содержимое пользовательского файла: +// импорт применяет до MaxPeerImportItems записей за операцию. +func TestDisconnectAuthIDsSplitsLargeBatches(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + + authIDs := make([]string, 0, kickChunkSize*2+5) + for i := 0; i < cap(authIDs); i++ { + authIDs = append(authIDs, fmt.Sprintf("auth-%d", i)) + } + + if err := disconnectAuthIDs(authIDs); err != nil { + t.Fatalf("разрыв сессии отказал: %v", err) + } + + if stub.kickCalls != 3 { + t.Fatalf("ожидалось 3 обращения к /kick, было %d", stub.kickCalls) + } + total := 0 + for _, keys := range stub.kickedKeys { + if len(keys) > kickChunkSize { + t.Fatalf("чанк больше предела: %d", len(keys)) + } + total += len(keys) + } + if total != len(authIDs) { + t.Fatalf("потеряны идентификаторы: отправлено %d из %d", total, len(authIDs)) + } +} + +func TestDisconnectAuthIDsReportsApiFailure(t *testing.T) { newTestDB(t) startTrafficStats(t, &trafficStatsStub{kickStatus: http.StatusInternalServerError}) - id := seedPeer(t, "alpha1", "alpha-auth-id") - if err := DisconnectPeers([]int64{id}); err == nil { + if err := disconnectAuthIDs([]string{"alpha-auth-id"}); err == nil { t.Fatal("отказ Traffic Stats API не сообщён") } } @@ -301,13 +348,12 @@ func TestDisconnectPeersReportsApiFailure(t *testing.T) { // Состояние службы по systemd на этом пути не спрашивается вовсе: ответ // systemctl не отличает «служба неактивна» от «спросить не удалось», и на // прежнем пути его сбой отказывал операции при живой Hysteria. -func TestDisconnectPeersIgnoresSystemdOpinion(t *testing.T) { +func TestDisconnectAuthIDsIgnoresSystemdOpinion(t *testing.T) { newTestDB(t) stub := startTrafficStats(t, nil) withHysteriaRunning(t, false) - id := seedPeer(t, "alpha1", "alpha-auth-id") - if err := DisconnectPeers([]int64{id}); err != nil { + if err := disconnectAuthIDs([]string{"alpha-auth-id"}); err != nil { t.Fatalf("разрыв сессии отказал из-за мнения systemd: %v", err) } if stub.kickCalls != 1 { diff --git a/apps/service/peer_admission.go b/apps/service/peer_admission.go new file mode 100644 index 0000000..e56ff6b --- /dev/null +++ b/apps/service/peer_admission.go @@ -0,0 +1,168 @@ +package service + +import ( + "sync" + "time" +) + +// Лимит устройств выдерживает ПАРАЛЛЕЛЬНЫЕ запросы авторизации. +// +// Что было. Проверка выглядела так: +// +// onlineUsers, err := hysteria2Online() +// if device, exist := onlineUsers[authID]; exist && maxDevices <= device { +// return deny +// } +// return allow +// +// Между чтением `/online` и ответом «allow» нет ничего, что удержало бы место, +// поэтому при одновременных подключениях выходило: +// +// A: GET /online -> 2 B: GET /online -> 2 +// max = 3 +// A: 2 < 3 -> allow B: 2 < 3 -> allow +// стало 4 +// +// Объявленный в панели «Лимит устройств: 3» превышался ровно тем способом, +// от которого лимит и должен защищать. +// +// Обычный мьютекс вокруг `/online` проблему не решает, и это главное, что +// нужно понимать про этот файл. Ответив «allow», админка не создаёт +// подключение — его только начинает устанавливать Hysteria, и в статистику +// клиент попадает позже. Следующий `/online`, даже строго после первого, +// продолжает показывать прежнее число. Сериализация запросов лишь сузила бы +// окно, оставив дефект на месте. +// +// Поэтому админка ведёт собственный учёт уже выданных, но ещё не проявившихся +// разрешений. HY2XS — один процесс на одном сервере с Hysteria, поэтому учёт +// process-local: ни Redis, ни таблицы в базе, ни распределённых блокировок для +// этого не нужно. +// +// Чего этот механизм НЕ обещает. Без обратного вызова от Hysteria +// «соединение установлено / не установлено» математически точной системы +// резервирования не построить. Он закрывает конкретный и реальный TOCTOU — +// параллельные HTTP-auth одного процесса, — и делает это fail-closed. + +// pendingAdmissionTTL — срок жизни выданного разрешения, которое ещё не +// проявилось в `/online`. +// +// Величина внутренняя и пользовательской настройкой не является намеренно: это +// не политика доступа, а компенсация задержки между ответом авторизации и +// появлением клиента в статистике Hysteria. Настройка, смысл которой оператор +// не может оценить, порождает только неверные значения. +// +// Тридцать секунд — с большим запасом относительно установления QUIC-сессии и +// при этом заметно меньше, чем интервал, на котором оператор вообще заметил бы +// занятый слот. Если клиент авторизовался, но так и не подключился, резервация +// исчезает сама. +const pendingAdmissionTTL = 30 * time.Second + +// admissionState — учёт по одному пиру. +type admissionState struct { + // lastOnline — число устройств, показанное Traffic Stats API в прошлый + // раз. Нужно, чтобы отличить рост (клиент подключился, резервация + // проявилась) от неизменного значения. + lastOnline int64 + // pending — сроки годности выданных, но ещё не проявившихся разрешений. + // Хранится по одному значению на разрешение, а не счётчиком: иначе + // протухать они могли бы только все разом. + pending []time.Time +} + +var deviceAdmissions = struct { + sync.Mutex + byAuthID map[string]*admissionState +}{byAuthID: map[string]*admissionState{}} + +// reserveDeviceSlot решает, есть ли для нового подключения свободное место, и +// занимает его. +// +// Возвращает true, если подключение можно разрешить. +// +// Сетевой запрос к `/online` выполняется ВНЕ этого мьютекса — вызывающий +// передаёт сюда уже полученное число. Под блокировкой остаются только +// несколько операций с map: держать её на время HTTP-обмена значило бы +// сериализовать все подключения всех пиров через один сетевой запрос. +func reserveDeviceSlot(authID string, online int64, maxDevices int64, now time.Time) bool { + deviceAdmissions.Lock() + defer deviceAdmissions.Unlock() + + state := deviceAdmissions.byAuthID[authID] + if state == nil { + state = &admissionState{} + deviceAdmissions.byAuthID[authID] = state + } + + // 1. Протухшие разрешения освобождают место: клиент, который авторизовался + // и не подключился, не должен занимать слот вечно. + state.dropExpired(now) + + // 2. Рост числа онлайн-устройств означает, что ровно столько выданных + // разрешений уже превратились в подключения. Не сняв их, админка + // посчитала бы одно и то же устройство дважды — сначала как резервацию, + // потом как реальное подключение, — и лимит стал бы вдвое строже + // объявленного. + if online > state.lastOnline { + state.dropMaterialized(online - state.lastOnline) + } + state.lastOnline = online + + // 3. Решение принимается по сумме: подтверждённые подключения плюс ещё не + // проявившиеся разрешения. + if online+int64(len(state.pending)) >= maxDevices { + // Место не занято и запись может оказаться ненужной: убираем её, чтобы + // карта не росла от одних отказов. + state.forgetIfIdle(authID) + return false + } + + state.pending = append(state.pending, now.Add(pendingAdmissionTTL)) + return true +} + +// Функции «вернуть занятое место» здесь нет намеренно. После выдачи разрешения +// в Hysteria2Auth не остаётся ни одного шага, способного отказать, а +// controller.Hysteria2Auth на успешном ответе только обновляет +// last_connection_at и неудачу этого обновления считает несущественной. +// Освобождение, у которого нет вызывающего, было бы вторым способом менять +// состояние трекера — и первым кандидатом разойтись с reserveDeviceSlot. +// Разрешение, за которым не последовало подключения, снимает TTL. + +func (s *admissionState) dropExpired(now time.Time) { + kept := s.pending[:0] + for _, deadline := range s.pending { + if deadline.After(now) { + kept = append(kept, deadline) + } + } + s.pending = kept +} + +// dropMaterialized снимает count самых старых разрешений: раньше выдано — +// раньше подключилось. +func (s *admissionState) dropMaterialized(count int64) { + if count >= int64(len(s.pending)) { + s.pending = s.pending[:0] + return + } + s.pending = s.pending[count:] +} + +// forgetIfIdle убирает запись, о которой больше нечего помнить. +// +// Без этого карта росла бы по одной записи на каждый когда-либо +// авторизовавшийся authId и не уменьшалась бы никогда — включая записи +// давно удалённых пиров. +func (s *admissionState) forgetIfIdle(authID string) { + if len(s.pending) == 0 && s.lastOnline == 0 { + delete(deviceAdmissions.byAuthID, authID) + } +} + +// resetDeviceAdmissions очищает учёт. Существует ради тестов: состояние здесь +// принадлежит процессу, и без сброса тесты видели бы резервации друг друга. +func resetDeviceAdmissions() { + deviceAdmissions.Lock() + defer deviceAdmissions.Unlock() + deviceAdmissions.byAuthID = map[string]*admissionState{} +} diff --git a/apps/service/peer_admission_test.go b/apps/service/peer_admission_test.go new file mode 100644 index 0000000..c22a321 --- /dev/null +++ b/apps/service/peer_admission_test.go @@ -0,0 +1,345 @@ +package service + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "hy2xs-admin/dao" + "hy2xs-admin/model/constant" +) + +// Лимит устройств проверяется на ПАРАЛЛЕЛЬНЫХ запросах авторизации. +// +// Прежняя проверка сравнивала ответ `/online` с maxDevices и сразу отвечала +// «allow»: между чтением и ответом место ничем не удерживалось, поэтому два +// одновременных подключения при `online = max-1` получали разрешение оба и +// объявленный лимит превышался. +// +// Последовательный тест этого не поймает никогда — нужен барьер, на котором +// оба запроса гарантированно видят ОДНО И ТО ЖЕ состояние Hysteria. + +// --- Барьерный тест против настоящего HTTP ----------------------------------- + +// startBarrierTrafficStats поднимает Traffic Stats API, который задерживает +// первые `hold` обращений к `/online` до тех пор, пока не придут все. +// +// Так воспроизводится ровно то состояние гонки, которое случается на живом +// сервере: оба запроса авторизации прочитали статистику до того, как хоть один +// из них успел превратиться в подключение. +func startBarrierTrafficStats(t *testing.T, online map[string]int64, hold int) { + t.Helper() + + var mu sync.Mutex + arrived := 0 + release := make(chan struct{}) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/online": + mu.Lock() + arrived++ + last := arrived == hold + mu.Unlock() + + if last { + close(release) + } else { + select { + case <-release: + case <-time.After(5 * time.Second): + // Барьер не собрался — отпускаем, чтобы тест упал по + // существу, а не по таймауту всего прогона. + } + } + + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(online) + case "/kick": + w.WriteHeader(http.StatusOK) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + + pointHysteriaConfigAt(t, server.URL) + if err := dao.UpsertConfigValue(constant.Hysteria2TrafficStatsSecret, testTrafficStatsSecret); err != nil { + t.Fatalf("не удалось записать секрет Traffic Stats API: %v", err) + } +} + +// Главная регрессия AUTH-03: при `online = max-1` разрешение обязан получить +// ровно ОДИН из двух одновременных запросов. +func TestHysteria2AuthHoldsDeviceLimitUnderConcurrency(t *testing.T) { + newTestDB(t) + // seedPeer создаёт пира с maxDevices = 3, поэтому online = 2 — это + // последнее свободное место. + startBarrierTrafficStats(t, map[string]int64{"alpha-auth-id": 2}, 2) + seedPeer(t, "alpha1", "alpha-auth-id") + + var wg sync.WaitGroup + results := make([]error, 2) + for i := range results { + wg.Add(1) + go func(idx int) { + defer wg.Done() + _, _, err := Hysteria2Auth("alpha1-secret") + results[idx] = err + }(i) + } + wg.Wait() + + allowed := 0 + for _, err := range results { + if err == nil { + allowed++ + } + } + if allowed != 1 { + t.Fatalf("на последнее свободное место допущено %d подключений из 2", allowed) + } +} + +// Свободных мест два — проходят оба: механизм не должен превращаться в +// сериализацию подключений. +func TestHysteria2AuthAdmitsBothWhenTwoSlotsFree(t *testing.T) { + newTestDB(t) + startBarrierTrafficStats(t, map[string]int64{"alpha-auth-id": 1}, 2) + seedPeer(t, "alpha1", "alpha-auth-id") + + var wg sync.WaitGroup + results := make([]error, 2) + for i := range results { + wg.Add(1) + go func(idx int) { + defer wg.Done() + _, _, err := Hysteria2Auth("alpha1-secret") + results[idx] = err + }(i) + } + wg.Wait() + + for idx, err := range results { + if err != nil { + t.Fatalf("подключение %d отклонено при двух свободных местах: %v", idx, err) + } + } +} + +// Резервации принадлежат КОНКРЕТНОМУ пиру: занятое место одного не должно +// закрывать доступ другому. +func TestDeviceAdmissionsAreIsolatedPerPeer(t *testing.T) { + newTestDB(t) + startBarrierTrafficStats(t, map[string]int64{"alpha-auth-id": 2, "bravo-auth-id": 0}, 2) + seedPeer(t, "alpha1", "alpha-auth-id") + seedPeer(t, "bravo2", "bravo-auth-id") + + var wg sync.WaitGroup + var alphaErr, bravoErr error + wg.Add(2) + go func() { + defer wg.Done() + _, _, alphaErr = Hysteria2Auth("alpha1-secret") + }() + go func() { + defer wg.Done() + _, _, bravoErr = Hysteria2Auth("bravo2-secret") + }() + wg.Wait() + + if alphaErr != nil { + t.Fatalf("первое подключение пира на последнее место отклонено: %v", alphaErr) + } + if bravoErr != nil { + t.Fatalf("резервация чужого пира закрыла доступ: %v", bravoErr) + } +} + +// Последовательно тот же лимит тоже держится: второй запрос видит место, +// занятое первым, хотя `/online` ещё показывает прежнее число. +func TestHysteria2AuthCountsPendingAdmissionSequentially(t *testing.T) { + newTestDB(t) + // Число НЕ меняется между запросами — именно так и ведёт себя Hysteria, + // пока клиент ещё устанавливает соединение. + startTrafficStats(t, &trafficStatsStub{online: map[string]int64{"alpha-auth-id": 1}}) + seedPeer(t, "alpha1", "alpha-auth-id") + + // max = 3, online = 1 -> свободно два места. + if _, _, err := Hysteria2Auth("alpha1-secret"); err != nil { + t.Fatalf("первое подключение отклонено: %v", err) + } + if _, _, err := Hysteria2Auth("alpha1-secret"); err != nil { + t.Fatalf("второе подключение отклонено: %v", err) + } + // Третье превысило бы лимит: 1 онлайн + 2 выданных разрешения. + if _, _, err := Hysteria2Auth("alpha1-secret"); err == nil { + t.Fatal("подключение сверх лимита принято: выданные разрешения не учтены") + } +} + +// --- Единица учёта ------------------------------------------------------------ + +const admissionAuthID = "auth-under-test" + +func TestReserveDeviceSlotAllowsUpToLimit(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + now := time.Now() + for i := 1; i <= 3; i++ { + if !reserveDeviceSlot(admissionAuthID, 0, 3, now) { + t.Fatalf("разрешение %d из 3 отклонено", i) + } + } + if reserveDeviceSlot(admissionAuthID, 0, 3, now) { + t.Fatal("выдано четвёртое разрешение при лимите 3") + } +} + +// Рост числа онлайн-устройств означает, что выданные разрешения превратились в +// подключения. Не сняв их, админка посчитала бы одно устройство дважды, и +// лимит стал бы вдвое строже объявленного. +func TestReserveDeviceSlotAbsorbsMaterializedAdmissions(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + now := time.Now() + if !reserveDeviceSlot(admissionAuthID, 0, 2, now) { + t.Fatal("первое разрешение отклонено") + } + // Клиент подключился: Hysteria теперь видит одно устройство. + if !reserveDeviceSlot(admissionAuthID, 1, 2, now) { + t.Fatal("проявившееся разрешение посчитано дважды") + } + // Теперь занято: одно подключение плюс одно выданное разрешение. + if reserveDeviceSlot(admissionAuthID, 1, 2, now) { + t.Fatal("выдано разрешение сверх лимита") + } +} + +// Разрешение, за которым не последовало подключения, освобождает место само. +func TestReserveDeviceSlotExpiresPendingAdmission(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + now := time.Now() + if !reserveDeviceSlot(admissionAuthID, 0, 1, now) { + t.Fatal("первое разрешение отклонено") + } + if reserveDeviceSlot(admissionAuthID, 0, 1, now) { + t.Fatal("выдано разрешение сверх лимита 1") + } + + // Клиент так и не подключился. + later := now.Add(pendingAdmissionTTL + time.Second) + if !reserveDeviceSlot(admissionAuthID, 0, 1, later) { + t.Fatal("протухшее разрешение не освободило место") + } +} + +// Место не занимается отказом: иначе серия отклонённых попыток удерживала бы +// слоты на всё время TTL. +func TestReserveDeviceSlotDoesNotConsumeSlotOnRefusal(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + now := time.Now() + for i := 0; i < 5; i++ { + if reserveDeviceSlot(admissionAuthID, 3, 3, now) { + t.Fatal("выдано разрешение при исчерпанном лимите") + } + } + // Одно устройство отключилось — место обязано быть свободно немедленно. + if !reserveDeviceSlot(admissionAuthID, 2, 3, now) { + t.Fatal("отклонённые попытки заняли места") + } +} + +// admissionEntries — размер учёта. +func admissionEntries() int { + deviceAdmissions.Lock() + defer deviceAdmissions.Unlock() + return len(deviceAdmissions.byAuthID) +} + +// Учёт не растёт от повторных обращений: одна запись на пира, сколько бы +// попыток он ни сделал. +// +// Границы роста здесь две, и обе существенны. Верхняя — число пиров: сюда +// попадают только authId, прошедшие поиск по secret_digest и всю политику +// доступа, поэтому произвольный ключ извне добавить нельзя. Нижняя — запись +// исчезает, как только помнить о пире нечего (см. следующий тест). +func TestReserveDeviceSlotKeepsOneEntryPerPeer(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + now := time.Now() + for i := 0; i < 100; i++ { + reserveDeviceSlot("transient-auth", 1, 1, now) + } + + if got := admissionEntries(); got != 1 { + t.Fatalf("повторные попытки одного пира дали %d записей", got) + } +} + +// Запись исчезает, когда о пире нечего помнить: он не онлайн и выданных +// разрешений за ним нет. Без этого карта накапливала бы по строке на каждый +// когда-либо авторизовавшийся authId, включая давно удалённых пиров. +func TestReserveDeviceSlotForgetsIdlePeer(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + now := time.Now() + if !reserveDeviceSlot(admissionAuthID, 1, 3, now) { + t.Fatal("разрешение отклонено при свободном месте") + } + if admissionEntries() != 1 { + t.Fatal("учёт не запомнил выданное разрешение") + } + + // Пир отключился целиком, а выданное разрешение протухло: помнить нечего. + later := now.Add(pendingAdmissionTTL + time.Second) + // Лимит 0 — разрешение не выдаётся, поэтому запись остаться не должна. + reserveDeviceSlot(admissionAuthID, 0, 0, later) + + if got := admissionEntries(); got != 0 { + t.Fatalf("запись о неактивном пире осталась: записей %d", got) + } +} + +// Учёт выдерживает параллельный доступ и не выдаёт больше мест, чем есть. +// Проверка ловит дефект и без детектора гонок: он виден по числу разрешений. +func TestReserveDeviceSlotIsConcurrencySafe(t *testing.T) { + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + + const workers = 50 + const limit = int64(7) + + now := time.Now() + var wg sync.WaitGroup + var mu sync.Mutex + allowed := 0 + + for i := 0; i < workers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if reserveDeviceSlot(admissionAuthID, 0, limit, now) { + mu.Lock() + allowed++ + mu.Unlock() + } + }() + } + wg.Wait() + + if int64(allowed) != limit { + t.Fatalf("выдано %d разрешений при лимите %d", allowed, limit) + } +} diff --git a/apps/service/peer_bootstrap_guard_test.go b/apps/service/peer_bootstrap_guard_test.go index 92c7e34..a98d890 100644 --- a/apps/service/peer_bootstrap_guard_test.go +++ b/apps/service/peer_bootstrap_guard_test.go @@ -102,6 +102,10 @@ func TestUpdatePeerAllowsNonIdentityChangesOnBootstrapPeer(t *testing.T) { // иметь возможность его отозвать. Расхождения база/файл удаление не создаёт. func TestDeletePeerAllowsRemovingBootstrapPeer(t *testing.T) { newTestDB(t) + // Удаление — это ещё и разрыв активных сессий по auth ID, который вместе со + // строкой исчезнет. Без отвечающего Traffic Stats API операция завершилась + // бы частичным результатом и строка осталась бы на месте. + startTrafficStats(t, nil) id := seedPeer(t, ReservedBootstrapPeerName, ReservedBootstrapPeerName) if err := DeletePeer(id); err != nil { diff --git a/apps/service/peer_errors.go b/apps/service/peer_errors.go index 7aef9e2..033a0cf 100644 --- a/apps/service/peer_errors.go +++ b/apps/service/peer_errors.go @@ -63,14 +63,23 @@ var ErrPeerNameReserved = &PeerError{ // пишется вызывающим. Оператору нужно другое: что именно уже сделано и что // осталось сделать. // +// Формулировка НЕ называет конкретную операцию, и это существенно. Раньше она +// утверждала «новые подключения пира запрещены» — фраза была верна ровно для +// одного случая, отключения пира. Через тот же код теперь отчитываются смена +// секрета, урезание квоты и срока, снижение лимита устройств, импорт партии и +// удаление пира; для удалённого пира «новые подключения запрещены» — вообще +// бессмыслица, потому что пира больше нет. Сообщение говорит то единственное, +// что верно во всех случаях: сохранённое состояние применено, живую сессию +// завершить не удалось. +// // Отказ относится к операции целиком, а не к полю формы: поля, которое можно // было бы исправить, здесь нет. func PeerDisconnectError() *PeerError { return &PeerError{ Code: constant.ErrCodePeerDisconnectFailed, - Message: "новые подключения пира запрещены, но завершить его активные " + - "сессии не удалось: Traffic Stats API Hysteria недоступен. " + - "Уже установленное соединение может продолжать работать до " + + Message: "изменения сохранены, но завершить связанные активные сессии " + + "не удалось: Traffic Stats API Hysteria недоступен. Уже " + + "установленные соединения могут продолжать работать до " + "переподключения клиента", } } diff --git a/apps/service/peer_import_tx_test.go b/apps/service/peer_import_tx_test.go index 8addc8c..5ab2bae 100644 --- a/apps/service/peer_import_tx_test.go +++ b/apps/service/peer_import_tx_test.go @@ -24,6 +24,14 @@ func newTestDB(t *testing.T) { if err := dao.RunMigrations(); err != nil { t.Fatalf("не удалось применить миграции: %v", err) } + + // Учёт выданных разрешений на устройства принадлежит ПРОЦЕССУ, а не базе, + // поэтому сам по себе он между тестами не обнуляется. Без сброса тест + // видел бы резервации предыдущего и проходил или падал по чужому + // состоянию — то есть доказывал бы не то, ради чего написан. + resetDeviceAdmissions() + t.Cleanup(resetDeviceAdmissions) + t.Cleanup(func() { _ = dao.CloseSqliteDB() }) @@ -170,6 +178,11 @@ func TestUpsertPeerExportRollsBackCrossConflict(t *testing.T) { // на обновлении. func TestUpsertPeerExportRollsBackDuplicateAuthId(t *testing.T) { newTestDB(t) + // Успешный исход этого сценария — обновление существующего пира, а оно + // после commit разрывает его старую сессию. Без отвечающего Traffic Stats + // API импорт вернул бы частичный результат, и тест принял бы его за отказ + // транзакции. + startTrafficStats(t, nil) seedPeer(t, "exist1", "shared-auth-id") before := snapshotPeers(t) @@ -247,6 +260,8 @@ func TestUpsertPeerExportRollsBackOnBootstrapPeer(t *testing.T) { // секрет: иначе перенос настроек ломал бы работающие клиентские ссылки. func TestUpsertPeerExportKeepsSecretWhenFileHasNone(t *testing.T) { newTestDB(t) + // Обновление существующего пира разрывает его старую сессию после commit. + startTrafficStats(t, nil) seedPeer(t, "keeper", "keeper-auth") before := snapshotPeers(t)["keeper"] diff --git a/apps/service/peer_reconcile_test.go b/apps/service/peer_reconcile_test.go new file mode 100644 index 0000000..f6f0656 --- /dev/null +++ b/apps/service/peer_reconcile_test.go @@ -0,0 +1,589 @@ +package service + +import ( + "net/http" + "testing" + "time" + + "hy2xs-admin/dao" + "hy2xs-admin/model/bo" + "hy2xs-admin/model/dto" + "hy2xs-admin/model/entity" +) + +// Любая операция, делающая живую сессию устаревшей, обязана её завершить. +// +// Прежние проверки доказывали существование пути отзыва доступа, но не +// полноту операций, которые по нему обязаны идти: разрыв выполнялся ровно при +// `disabled=1`, а удаление, импорт, смена секрета и урезание квоты/срока/лимита +// проходили мимо. Здесь доказывается именно полнота. + +func int64Ptr(v int64) *int64 { return &v } + +// kickedAuthIDs собирает все идентификаторы, ушедшие в `/kick`. +func kickedAuthIDs(stub *trafficStatsStub) []string { + stub.mu.Lock() + defer stub.mu.Unlock() + + out := make([]string, 0, len(stub.kickedKeys)) + for _, keys := range stub.kickedKeys { + out = append(out, keys...) + } + return out +} + +func kickCalls(stub *trafficStatsStub) int { + stub.mu.Lock() + defer stub.mu.Unlock() + return stub.kickCalls +} + +func requireKicked(t *testing.T, stub *trafficStatsStub, want ...string) { + t.Helper() + + got := kickedAuthIDs(stub) + if len(got) != len(want) { + t.Fatalf("в /kick ушло %v, ожидалось %v", got, want) + } + seen := map[string]int{} + for _, authID := range got { + seen[authID]++ + } + for _, authID := range want { + if seen[authID] == 0 { + t.Fatalf("идентификатор %q не был разорван; ушло %v", authID, got) + } + seen[authID]-- + } +} + +// --- Удаление пира ----------------------------------------------------------- + +// Что было: `return dao.DeletePeer([]int64{id})`. Строка исчезала, живая +// QUIC-сессия оставалась, и вместе со строкой исчезал auth_id — то есть +// единственное, чем эту сессию можно было бы завершить. Удалённый пир +// пользовался доступом, пока не переподключался сам. +func TestDeletePeerTerminatesLiveSession(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := DeletePeer(id); err != nil { + t.Fatalf("удаление отказало: %v", err) + } + + requireKicked(t, stub, "alpha-auth-id") + if _, err := dao.GetPeer("id = ?", id); err == nil { + t.Fatal("строка пира осталась в базе после успешного удаления") + } +} + +// Запрет новых подключений записывается ДО разрыва. Проверить это после +// операции нельзя — строки уже нет, — поэтому состояние снимается в момент +// прихода `/kick`. +func TestDeletePeerWritesDisabledBeforeKick(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := DeletePeer(id); err != nil { + t.Fatalf("удаление отказало: %v", err) + } + + if len(stub.disabledAtKick) == 0 { + t.Fatal("разрыв сессии не выполнялся") + } + if got := stub.disabledAtKick[0]["alpha1"]; got != 1 { + t.Fatalf("/kick пришёл раньше записи disabled: на момент разрыва disabled=%d", got) + } +} + +// Неудача разрыва оставляет пира В БАЗЕ и ОТКЛЮЧЁННЫМ. +// +// Удалить строку, не сумев завершить сессию, значит потерять auth_id и вместе +// с ним всякую возможность отозвать доступ. Оставшаяся строка с disabled=1 +// закрывает новые подключения и позволяет оператору повторить удаление. +func TestDeletePeerKeepsDisabledRowWhenKickFails(t *testing.T) { + newTestDB(t) + startTrafficStats(t, &trafficStatsStub{kickStatus: http.StatusInternalServerError}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + requireDisconnectError(t, DeletePeer(id)) + + peer, err := dao.GetPeer("id = ?", id) + if err != nil { + t.Fatalf("пир удалён, хотя его сессию завершить не удалось: %v", err) + } + if peer.Disabled == nil || *peer.Disabled != 1 { + t.Fatalf("оставшийся пир не отключён: %v", peer.Disabled) + } +} + +// Повторное удаление после неудавшегося разрыва обязано пройти целиком. +func TestDeletePeerIsRetryableAfterKickFailure(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, &trafficStatsStub{kickStatus: http.StatusInternalServerError}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + requireDisconnectError(t, DeletePeer(id)) + + stub.mu.Lock() + stub.kickStatus = 0 + stub.mu.Unlock() + + if err := DeletePeer(id); err != nil { + t.Fatalf("повторное удаление отказало: %v", err) + } + if _, err := dao.GetPeer("id = ?", id); err == nil { + t.Fatal("повторное удаление не убрало строку") + } + if kickCalls(stub) != 2 { + t.Fatalf("повторная попытка не дошла до /kick: вызовов %d", kickCalls(stub)) + } +} + +// Пир без authId Hysteria не знает: разрывать нечего, и это не повод +// отказывать в удалении. +func TestDeletePeerWithoutAuthIDSkipsKick(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "") + + if err := DeletePeer(id); err != nil { + t.Fatalf("удаление пира без authId отказало: %v", err) + } + if kickCalls(stub) != 0 { + t.Fatalf("сделан вызов /kick без цели: %d", kickCalls(stub)) + } +} + +// --- Изменение пира ---------------------------------------------------------- + +// Смена секрета делает недействительными учётные данные, по которым сессия +// была установлена. Раньше разрыв выполнялся только при disabled=1, поэтому +// ротация секрета оставляла клиента на связи по старому секрету. +func TestUpdatePeerSecretRotationDisconnects(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := UpdatePeer(id, dto.PeerUpdateDto{Secret: strPtr("brand-new-secret")}); err != nil { + t.Fatalf("смена секрета отказала: %v", err) + } + + requireKicked(t, stub, "alpha-auth-id") +} + +// Новый секрет остаётся сохранённым и при неудавшемся разрыве: безопасная +// половина операции достигнута, и возвращать прежние учётные данные нельзя. +func TestUpdatePeerSecretRotationKeepsNewSecretWhenKickFails(t *testing.T) { + newTestDB(t) + startTrafficStats(t, &trafficStatsStub{kickStatus: http.StatusInternalServerError}) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + before := snapshotPeers(t)["alpha1"] + + requireDisconnectError(t, UpdatePeer(id, dto.PeerUpdateDto{Secret: strPtr("brand-new-secret")})) + + after := snapshotPeers(t)["alpha1"] + if *after.SecretDigest == *before.SecretDigest { + t.Fatal("новый секрет откачен после неудачного разрыва") + } +} + +// Сочетание изменений — одна операция над одним пиром, а значит один `/kick`. +func TestUpdatePeerSendsSingleKickForCombinedChanges(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + err := UpdatePeer(id, dto.PeerUpdateDto{ + Secret: strPtr("brand-new-secret"), + Disabled: int64Ptr(1), + QuotaBytes: int64Ptr(1), + MaxDevices: int64Ptr(1), + }) + if err != nil { + t.Fatalf("правка отказала: %v", err) + } + + if kickCalls(stub) != 1 { + t.Fatalf("ожидался ровно один /kick, было %d", kickCalls(stub)) + } +} + +// Урезание квоты ниже израсходованного закрывает доступ немедленно — значит и +// живую сессию тоже. +func TestUpdatePeerQuotaReductionBelowUsageDisconnects(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := dao.UpdatePeer([]int64{id}, map[string]interface{}{ + "quota_bytes": int64(100_000), + "download_bytes": int64(6_000), + "upload_bytes": int64(4_000), + }); err != nil { + t.Fatalf("подготовка расхода: %v", err) + } + + // 10 000 израсходовано, новая квота 5 000. + if err := UpdatePeer(id, dto.PeerUpdateDto{QuotaBytes: int64Ptr(5_000)}); err != nil { + t.Fatalf("урезание квоты отказало: %v", err) + } + + requireKicked(t, stub, "alpha-auth-id") +} + +// Граница та же, что и в политике: израсходовано ровно столько, сколько теперь +// разрешено. +func TestUpdatePeerQuotaReducedToExactUsageDisconnects(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := dao.UpdatePeer([]int64{id}, map[string]interface{}{ + "quota_bytes": int64(100_000), + "download_bytes": int64(10_000), + "upload_bytes": int64(0), + }); err != nil { + t.Fatalf("подготовка расхода: %v", err) + } + + if err := UpdatePeer(id, dto.PeerUpdateDto{QuotaBytes: int64Ptr(10_000)}); err != nil { + t.Fatalf("урезание квоты отказало: %v", err) + } + + requireKicked(t, stub, "alpha-auth-id") +} + +// Урезание квоты, которое доступ ещё НЕ закрывает, сессию не рвёт: у оператора +// нет причины ронять работающее соединение. +func TestUpdatePeerQuotaReductionAboveUsageDoesNotDisconnect(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := dao.UpdatePeer([]int64{id}, map[string]interface{}{ + "quota_bytes": int64(100_000), + "download_bytes": int64(1_000), + }); err != nil { + t.Fatalf("подготовка расхода: %v", err) + } + + if err := UpdatePeer(id, dto.PeerUpdateDto{QuotaBytes: int64Ptr(50_000)}); err != nil { + t.Fatalf("правка квоты отказала: %v", err) + } + + if kickCalls(stub) != 0 { + t.Fatalf("сессия разорвана при действующем доступе: вызовов /kick %d", kickCalls(stub)) + } +} + +// Перенос срока в прошлое закрывает доступ немедленно. +func TestUpdatePeerExpiryMovedIntoPastDisconnects(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + past := time.Now().UnixMilli() - 60_000 + if err := UpdatePeer(id, dto.PeerUpdateDto{ExpiresAt: int64Ptr(past)}); err != nil { + t.Fatalf("перенос срока отказал: %v", err) + } + + requireKicked(t, stub, "alpha-auth-id") +} + +// Продление срока — послабление, сессию оно не трогает. +func TestUpdatePeerExpiryExtensionDoesNotDisconnect(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + future := time.Now().UnixMilli() + 3_600_000 + if err := UpdatePeer(id, dto.PeerUpdateDto{ExpiresAt: int64Ptr(future)}); err != nil { + t.Fatalf("продление срока отказало: %v", err) + } + + if kickCalls(stub) != 0 { + t.Fatalf("продление срока разорвало сессию: вызовов /kick %d", kickCalls(stub)) + } +} + +// Снижение лимита устройств применяется немедленно. Выбирать «лишнее +// устройство» не нужно: `/kick` оперирует идентификатором клиента, и после +// переподключения новый лимит пропустит ровно столько, сколько разрешено. +func TestUpdatePeerMaxDevicesDecreaseDisconnects(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + // seedPeer создаёт пира с maxDevices = 3. + if err := UpdatePeer(id, dto.PeerUpdateDto{MaxDevices: int64Ptr(1)}); err != nil { + t.Fatalf("снижение лимита отказало: %v", err) + } + + requireKicked(t, stub, "alpha-auth-id") +} + +func TestUpdatePeerMaxDevicesIncreaseDoesNotDisconnect(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := UpdatePeer(id, dto.PeerUpdateDto{MaxDevices: int64Ptr(5)}); err != nil { + t.Fatalf("повышение лимита отказало: %v", err) + } + + if kickCalls(stub) != 0 { + t.Fatalf("повышение лимита разорвало сессию: вызовов /kick %d", kickCalls(stub)) + } +} + +// Переименование и правка пометки доступа не касаются. +func TestUpdatePeerCosmeticChangesDoNotDisconnect(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := UpdatePeer(id, dto.PeerUpdateDto{ + Name: strPtr("alpha2"), + Remark: strPtr("ноутбук"), + }); err != nil { + t.Fatalf("правка отказала: %v", err) + } + + if kickCalls(stub) != 0 { + t.Fatalf("косметическая правка разорвала сессию: вызовов /kick %d", kickCalls(stub)) + } +} + +// Повторная запись того же значения квоты изменением не является. +func TestUpdatePeerUnchangedQuotaDoesNotDisconnect(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + id := seedPeer(t, "alpha1", "alpha-auth-id") + + if err := dao.UpdatePeer([]int64{id}, map[string]interface{}{ + "quota_bytes": int64(1_000), + "download_bytes": int64(5_000), + }); err != nil { + t.Fatalf("подготовка: %v", err) + } + + // Доступ закрыт и до правки: его сессию уже завершил тот, кто его закрыл. + if err := UpdatePeer(id, dto.PeerUpdateDto{QuotaBytes: int64Ptr(1_000)}); err != nil { + t.Fatalf("правка отказала: %v", err) + } + + if kickCalls(stub) != 0 { + t.Fatalf("повторная запись того же значения разорвала сессию: вызовов /kick %d", kickCalls(stub)) + } +} + +// --- Импорт ------------------------------------------------------------------ + +// Импорт переписывает credential- и access-состояние существующего пира +// целиком, поэтому его старая сессия обязана быть завершена. +func TestImportDisconnectsExistingPeerAfterCommit(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + seedPeer(t, "keeper", "keeper-auth") + + item := importItem("keeper", "keeper-auth") + item.Disabled = 1 + if err := UpsertPeerExport([]bo.PeerExport{item}); err != nil { + t.Fatalf("импорт отказал: %v", err) + } + + requireKicked(t, stub, "keeper-auth") +} + +// Разрывается СТАРЫЙ auth ID: после commit в строке уже стоит новый, и по нему +// Hysteria про сессию ничего не знает. +func TestImportDisconnectsOldAuthIDWhenItChanges(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + seedPeer(t, "keeper", "old-auth-id") + + // Совпадение по имени: authId в файле другой, значит он будет заменён. + item := importItem("keeper", "new-auth-id") + if err := UpsertPeerExport([]bo.PeerExport{item}); err != nil { + t.Fatalf("импорт отказал: %v", err) + } + + requireKicked(t, stub, "old-auth-id") + + after := snapshotPeers(t)["keeper"] + if after.AuthId == nil || *after.AuthId != "new-auth-id" { + t.Fatalf("authId не заменён импортом: %v", after.AuthId) + } +} + +// Вновь созданные пиры не разрываются: до импорта их живых сессий существовать +// не могло. +func TestImportDoesNotDisconnectNewPeers(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + + items := []bo.PeerExport{ + importItem("brand1", ""), + importItem("brand2", ""), + } + if err := UpsertPeerExport(items); err != nil { + t.Fatalf("импорт отказал: %v", err) + } + + if kickCalls(stub) != 0 { + t.Fatalf("новые пиры разорваны: вызовов /kick %d", kickCalls(stub)) + } +} + +// Партия — одна операция: один `/kick` со всеми старыми идентификаторами. +func TestImportSendsSingleBatchKick(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + seedPeer(t, "first1", "first-auth") + seedPeer(t, "secnd2", "secnd-auth") + + items := []bo.PeerExport{ + importItem("first1", "first-auth"), + importItem("secnd2", "secnd-auth"), + importItem("brand3", ""), + } + if err := UpsertPeerExport(items); err != nil { + t.Fatalf("импорт отказал: %v", err) + } + + if kickCalls(stub) != 1 { + t.Fatalf("ожидался один batch /kick, было %d", kickCalls(stub)) + } + requireKicked(t, stub, "first-auth", "secnd-auth") +} + +// Откат транзакции означает, что состояние не менялось: рвать нечего. +func TestImportRollbackSendsNoKick(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + seedPeer(t, "alice1", "aaa") + seedPeer(t, "bob123", "bbb") + + items := []bo.PeerExport{ + importItem("first1", ""), + // UNIQUE(name): поиск найдёт alice1 по auth_id и переименует её в bob123. + importItem("bob123", "aaa"), + } + if err := UpsertPeerExport(items); err == nil { + t.Fatal("импорт с конфликтом UNIQUE должен быть отклонён") + } + + if kickCalls(stub) != 0 { + t.Fatalf("откаченный импорт разорвал сессии: вызовов /kick %d", kickCalls(stub)) + } +} + +// Отклонение партии валидацией не доходит ни до базы, ни до Hysteria. +func TestImportInvalidBatchSendsNoKick(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + seedPeer(t, "keeper", "keeper-auth") + + items := []bo.PeerExport{ + importItem("keeper", "keeper-auth"), + importItem("bad", ""), // короче 6 символов + } + if err := UpsertPeerExport(items); err == nil { + t.Fatal("партия с невалидной записью должна быть отклонена") + } + + if kickCalls(stub) != 0 { + t.Fatalf("отклонённая партия разорвала сессии: вызовов /kick %d", kickCalls(stub)) + } +} + +// Неудача разрыва НЕ откатывает импорт: транзакция уже зафиксирована, и +// оператору сообщается частичный результат. +func TestImportStaysCommittedWhenKickFails(t *testing.T) { + newTestDB(t) + startTrafficStats(t, &trafficStatsStub{kickStatus: http.StatusInternalServerError}) + seedPeer(t, "keeper", "keeper-auth") + + item := importItem("keeper", "keeper-auth") + item.Remark = "перенесено" + + requireDisconnectError(t, UpsertPeerExport([]bo.PeerExport{item})) + + after := snapshotPeers(t)["keeper"] + if after.Remark == nil || *after.Remark != "перенесено" { + t.Fatalf("импорт откачен из-за неудачного разрыва: remark=%v", after.Remark) + } +} + +// Разрыв идёт СТРОГО после commit: до него клиент успел бы переподключиться к +// ещё не изменённому пиру. Доказывается снимком базы в момент прихода `/kick`. +func TestImportCommitsBeforeKick(t *testing.T) { + newTestDB(t) + stub := startTrafficStats(t, nil) + seedPeer(t, "keeper", "keeper-auth") + + item := importItem("keeper", "keeper-auth") + item.Disabled = 1 + if err := UpsertPeerExport([]bo.PeerExport{item}); err != nil { + t.Fatalf("импорт отказал: %v", err) + } + + if len(stub.disabledAtKick) == 0 { + t.Fatal("разрыв сессии не выполнялся") + } + if got := stub.disabledAtKick[0]["keeper"]; got != 1 { + t.Fatalf("/kick пришёл раньше commit: на момент разрыва disabled=%d", got) + } +} + +// --- Правило выбора reconcile ------------------------------------------------ + +// Правило проверяется и напрямую: так видно, что асимметрия «ограничение +// применяется немедленно, послабление — нет» является решением, а не побочным +// эффектом порядка условий. +func TestUpdateRequiresReconcileRules(t *testing.T) { + base := func() entity.Peer { + return policyPeer(0, 100_000, 1_000, 0, 0, 0) + } + withMaxDevices := func(peer entity.Peer, value int64) entity.Peer { + peer.MaxDevices = &value + return peer + } + + cases := []struct { + name string + peer entity.Peer + dto dto.PeerUpdateDto + want bool + }{ + {"смена секрета", base(), dto.PeerUpdateDto{Secret: strPtr("new-secret")}, true}, + {"пустой секрет — «не менять»", base(), dto.PeerUpdateDto{Secret: strPtr(" ")}, false}, + {"отключение", base(), dto.PeerUpdateDto{Disabled: int64Ptr(1)}, true}, + {"повторное отключение — retry", policyPeer(1, 100_000, 0, 0, 0, 0), dto.PeerUpdateDto{Disabled: int64Ptr(1)}, true}, + {"включение", policyPeer(1, 100_000, 0, 0, 0, 0), dto.PeerUpdateDto{Disabled: int64Ptr(0)}, false}, + {"снижение лимита устройств", withMaxDevices(base(), 3), dto.PeerUpdateDto{MaxDevices: int64Ptr(1)}, true}, + {"повышение лимита устройств", withMaxDevices(base(), 3), dto.PeerUpdateDto{MaxDevices: int64Ptr(5)}, false}, + {"тот же лимит устройств", withMaxDevices(base(), 3), dto.PeerUpdateDto{MaxDevices: int64Ptr(3)}, false}, + {"квота ниже расхода", base(), dto.PeerUpdateDto{QuotaBytes: int64Ptr(500)}, true}, + {"квота ровно по расходу", base(), dto.PeerUpdateDto{QuotaBytes: int64Ptr(1_000)}, true}, + {"квота выше расхода", base(), dto.PeerUpdateDto{QuotaBytes: int64Ptr(2_000)}, false}, + {"квота снята в безлимит", base(), dto.PeerUpdateDto{QuotaBytes: int64Ptr(-1)}, false}, + {"срок в прошлое", base(), dto.PeerUpdateDto{ExpiresAt: int64Ptr(policyNow - 1)}, true}, + {"срок в будущее", base(), dto.PeerUpdateDto{ExpiresAt: int64Ptr(policyNow + 1)}, false}, + {"только пометка", base(), dto.PeerUpdateDto{Remark: strPtr("ноутбук")}, false}, + {"только имя", base(), dto.PeerUpdateDto{Name: strPtr("alpha2")}, false}, + {"пустой запрос", base(), dto.PeerUpdateDto{}, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := updateRequiresReconcile(tc.peer, tc.dto, policyNow); got != tc.want { + t.Fatalf("updateRequiresReconcile = %v, ожидалось %v", got, tc.want) + } + }) + } +} diff --git a/docs/acceptance/2026-09-01-v1.0.0-rc2-preflight-findings.md b/docs/acceptance/2026-09-01-v1.0.0-rc2-preflight-findings.md index e5923e7..4148e7c 100644 --- a/docs/acceptance/2026-09-01-v1.0.0-rc2-preflight-findings.md +++ b/docs/acceptance/2026-09-01-v1.0.0-rc2-preflight-findings.md @@ -30,11 +30,23 @@ Hysteria-интеграции с официальной документацие | CORE-01 | Ошибка называет `TCP port` для UDP-эндпоинта | P3 | закрыт | | CORE-02 | `banned_until` писался дважды, отказ отчитывался как полный | P1 | закрыт | | TYPE-01 | Типы полей журнала в панели расходились с сервером | P3 | закрыт | +| CORE-03 | Удаление пира не отзывало доступ и теряло `authId` | P0 | закрыт | +| CORE-04 | Разрыв сессии выполнялся только при `disabled=1` | P1 | закрыт | +| CORE-05 | Импорт не завершал сессии переписанных пиров | P1 | закрыт | +| CORE-06 | Джоба учёта убегала из жизненного цикла планировщика | P1 | закрыт | +| CORE-07 | Три nil-разыменования в cron роняли процесс целиком | P0 | закрыт | +| QUOTA-01 | Исчерпанная квота не отключала пира никогда | P0 | закрыт | +| AUTH-03 | Параллельные подключения превышали `maxDevices` | P1 | закрыт | +| GATE-01 | Гейт fail-open срабатывал на корректном коде | P1 | закрыт | +| UX-12 | Импорт не разбирал свой исход и не обновлял список | P2 | закрыт | LOG-04, LOG-05, AUTH-02, CORE-02, UX-08…UX-11 и TYPE-01 в исходный разбор не входили и найдены при проверке его выводов по коду. UX-11 нашёлся позже остальных — при проверке уже внесённых исправлений. +CORE-03…06, AUTH-03 и QUOTA-01 — второй проход разбора, уже по состоянию после +принятых исправлений. CORE-07, GATE-01 и UX-12 найдены при их закрытии. + --- ## UX-06 — отключение пира не отзывало доступ @@ -66,13 +78,17 @@ POST /kick -> разрывает текущую сессию **Как закрыто.** -1. `service.DisconnectPeers(ids)` — только официальный `/kick`, без единой - записи в базу. Прежний `Hysteria2Kick` вместе с разрывом проставлял - `banned_until`, поэтому воспользоваться им для отключения было нельзя: - операция записала бы заодно временную блокировку — другой механизм с другим - сроком жизни и другим способом снятия. +1. Отдельный примитив разрыва — только официальный `/kick`, без единой записи + в базу. Прежний `Hysteria2Kick` вместе с разрывом проставлял `banned_until`, + поэтому воспользоваться им для отключения было нельзя: операция записала бы + заодно временную блокировку — другой механизм с другим сроком жизни и другим + способом снятия. (Тогда он назывался `DisconnectPeers` и принимал + идентификаторы пиров; во втором проходе стал `disconnectAuthIDs` — см. + CORE-03 и CORE-05, где старый `authId` нужен уже после его исчезновения из + базы.) 2. `UpdatePeer` при `disabled=1` выполняет обе половины: сначала долговременную - запись, затем разрыв. + запись, затем разрыв. (Во втором проходе перечень операций расширен — см. + CORE-04.) 3. Порядок обратному не подлежит. При обратном клиент успевает переподключиться в окне между `/kick` и записью и остаётся на связи с формально отключённым пиром. Порядок доказывается тестом, который снимает @@ -164,9 +180,10 @@ machine-auth, то есть на пути каждого подключения применилась. Операция отвечала чистым отказом, находясь в применённом состоянии. -Закрыто тем же примитивом, что и UX-06: долговременная запись, затем -`DisconnectPeers`, затем — при неудаче разрыва — частичный результат отдельным -кодом. +Закрыто тем же примитивом, что и UX-06: долговременная запись, затем разрыв, +затем — при неудаче разрыва — частичный результат отдельным кодом. Во втором +проходе этот путь стал общим для всех операций отзыва — `reconcileLiveSessions` +(см. CORE-04). Механизмы остались независимыми: `banned_until` истекает сам, `disabled` снимается только руками; включение пира не сбрасывает временную блокировку, а @@ -359,6 +376,266 @@ EX-03: она обещала более узкий набор, чем серве --- +## QUOTA-01 — исчерпанная квота не отключала пира никогда + +Правило доступа существовало в двух экземплярах, написанных разными условиями в +разных местах. + +Авторизация прятала его в выборке: + +```sql +disabled = 0 +and (quota_bytes < 0 or quota_bytes > download_bytes + upload_bytes) +and (expires_at = 0 or ? < expires_at) +and ? > banned_until +``` + +Принудительное отключение — в своей: + +```sql +disabled = 1 +or (quota_bytes > 0 and quota_bytes < download_bytes + upload_bytes) +or (expires_at > 0 and ? > expires_at) +or ? < banned_until +``` + +Второе условие **не является отрицанием первого**, и расхождение приходилось +ровно на границы: + +| состояние | авторизация | принудительное отключение | +| --- | --- | --- | +| `quota = 0` | отказ | сессию не рвёт | +| `usage = quota` | отказ | сессию не рвёт | +| `now = expiresAt` | отказ | сессию не рвёт | +| `now = bannedUntil` | отказ | сессию не рвёт | + +Хуже всего вела себя исчерпанная квота. `quota_bytes < download + upload` +требует СТРОГОГО превышения, а счётчики растут порциями по ответу Traffic Stats +API — попадание в точное равенство является обычным исходом очередного сбора, а +не экзотикой. Пир с исчерпанной квотой не пускался заново, но его живая сессия +не разрывалась никогда: он продолжал пользоваться доступом, пока не +переподключался по своей воле. + +**Как закрыто.** Политика вынесена из SQL в одну функцию `peerAccessDenied` +(`apps/service/peer_access.go`); авторизация ищет пира только по +`secret_digest`, а cron применяет ту же функцию к пирам, которых Hysteria +назвала онлайн. Расходиться им теперь физически негде. Границы зафиксированы +таблицей в `docs/admin/04-admin-panel.md` и точечными тестами: набор проверок +состоит в основном из равенств, потому что расходились именно они. + +Отдельно: `quota = -1` объявлен единственным каноничным способом снять +ограничение, `quota = 0` означает ноль байтов. Отрицательное значение любой +величины трактуется как безлимит — так же, как это делала выборка авторизации; +через двери продукта значение меньше `-1` недостижимо. + +--- + +## CORE-03 — удаление пира не отзывало доступ + +`DeletePeer` состоял из одной строки: + +```go +func DeletePeer(id int64) error { return dao.DeletePeer([]int64{id}) } +``` + +Строка исчезала, живая QUIC-сессия оставалась. Хуже того, вместе со строкой +исчезал `auth_id` — единственное, чем эту сессию можно было бы завершить. +Состояние становилось **невосстановимым**: удалённый пир пользовался доступом, +пока не переподключался по своей воле, и сделать с этим было уже нечего. + +**Как закрыто.** Порядок: прочитать пира и запомнить `authId` → записать +`disabled=1` → `/kick` по запомненному значению → удалить строку. Неудача +разрыва оставляет строку на месте отключённой, поэтому новые подключения +запрещены, а оператор повторяет удаление. Отката после `/kick` нет. + +Контроллер переведён на `failService`: удаление умеет завершиться частично, и +через `vo.Fail` этот исход уезжал бы панели неотличимо от полного отказа. + +--- + +## CORE-04 — разрыв выполнялся только при отключении + +Условие было одно: + +```go +if peerDto.Disabled != nil && *peerDto.Disabled == 1 { +``` + +Мимо него проходили четыре операции, каждая из которых закрывает доступ: + +```text +смена секрета старые учётные данные недействительны, сессия жива +урезание квоты «100 ГБ -> 5 ГБ» при израсходованных 10 ГБ +перенос срока «истекает завтра» -> «истёк вчера» +снижение лимита «5 устройств -> 1» при пяти подключённых +``` + +Панель показывала новое состояние, а пир продолжал пользоваться доступом по +старому — тот же дефект, что и UX-06, только под другими именами полей. + +**Как закрыто.** `updateRequiresReconcile` принимает решение по снимку «до» и +запрошенным изменениям. Квота и срок проверяются через ту же +`peerAccessDenied`, поэтому «закрывает доступ» здесь и «не пустит при следующем +подключении» — буквально одно условие. + +Правило асимметрично намеренно: ограничение применяется немедленно, +послабление — нет. При любом сочетании изменений уходит ровно один `/kick`. + +--- + +## CORE-05 — импорт не завершал сессии переписанных пиров + +Импорт переписывает `auth_id`, `secret_digest`, `quota_bytes`, `expires_at` и +`disabled` существующего пира целиком, но сессий не трогал вовсе. + +**Как закрыто.** Старые `authId` собираются ВНУТРИ транзакции, разрыв идёт +ПОСЛЕ commit. Оба слова существенны: внутри — потому что после commit старого +значения в базе уже нет; после — потому что `/kick` до commit оставляет клиенту +окно, в котором он переподключается к ещё не изменённому пиру. + +Рвутся сессии всех существующих записей партии, а не тех, у кого изменилось +конкретное поле. Это сознательно более простой контракт, чем diff по семи +полям: не появляется второй таблицы правил «какие поля импорта считаются +access-changing» — то есть второго места, где политика может разойтись с +`peerAccessDenied`. Вновь созданные пиры не рвутся: до импорта их сессий +существовать не могло. + +--- + +## CORE-06 и CORE-07 — джоба учёта + +**CORE-06.** Устройство было таким: + +```go +CronHandleAccount() + -> go func() + -> go saveAccountTraffic() + -> go kickAccount() +``` + +Для планировщика джоба заканчивалась почти мгновенно — сразу после запуска +внешней горутины. `StopCron()`, который честно ждёт `scheduler.Stop().Done()`, +не ждал НИЧЕГО из настоящей работы: планировщик отчитывался «джоб не осталось», +`releaseResource()` закрывал SQLite, а внутренние горутины продолжали писать в +закрытое соединение. + +Второе следствие того же устройства было тише. Обе половины запускались +параллельно, поэтому принудительное отключение читало счётчики ДО того, как в +них попадала только что снятая дельта: превышение квоты замечалось в лучшем +случае со следующего тика, а на границе — не замечалось вовсе. + +**CORE-07** — три nil-разыменования на том же пути, и все внутри горутин, где +их некому перехватить, то есть каждое роняет процесс целиком вместе с +обработчиком machine-auth: + +```go +*trafficSecretConfig.Value // строка config без значения +*item.AuthId // строка пира с NULL auth_id +*item.Id // строка пира без идентификатора (CronResetTraffic) +``` + +Заодно: при пустом наборе целей в Hysteria уезжал `POST /kick` с пустым +массивом в теле — каждые 30 секунд. + +**Как закрыто.** Джоба синхронна, под одним `accountJobMutex` на весь цикл +(`trafficMutex` и `kickMutex` удалены — они защищали каждую половину от самой +себя, но не защищали пару от расщепления). Порядок строгий: сбор трафика, затем +enforcement. Секрет берётся общей `hysteria2TrafficSecret()`, которая отличает +«ключа нет» от пустого значения. Повреждённые строки пропускаются с записью в +журнал. Гейт `Hysteria2IsRunning` удалён: `util.Exec` не отличает «служба +неактивна» от «спросить не удалось», поэтому сломанный `systemctl` при живой +Hysteria молча отключал и учёт, и enforcement — без единой строки в журнале. + +**Что осталось известным ограничением.** `GET /traffic?clear=1` деструктивен: +счётчики Hysteria обнуляются сразу после отправки ответа, поэтому дельта, +которую не удалось записать в SQLite, потеряна безвозвратно. Раньше такой отказ +делал `continue` и не оставлял следа в исходе джобы; теперь каждая потеря +считается и попадает в ошибку цикла. Полное решение требует смены модели учёта +(недеструктивное чтение плюс долговременные checkpoint'ы) и в `1.0.0` намеренно +не вводится: квота — операционный предел доступа, а не учёт с финансово +значимым каждым байтом. + +--- + +## AUTH-03 — параллельные подключения превышали лимит устройств + +Между чтением `/online` и ответом «allow» место ничем не удерживалось: + +```text +A: GET /online -> 2 B: GET /online -> 2 + max = 3 +A: 2 < 3 -> allow B: 2 < 3 -> allow + стало 4 +``` + +Мьютекс вокруг `/online` это не чинит, и это главное в дефекте. Ответив +«allow», админка не создаёт подключение — его только начинает устанавливать +Hysteria, и клиент попадает в статистику позже. Следующий `/online`, даже +строго после первого, продолжает показывать прежнее число; сериализация лишь +сузила бы окно. + +**Как закрыто.** Process-local учёт выданных, но ещё не проявившихся разрешений +(`apps/service/peer_admission.go`). Решение принимается по сумме «подключено +плюс зарезервировано»; рост `online` снимает соответствующее число резерваций, +протухшие снимаются по TTL. Сетевой запрос выполняется вне блокировки: под ней +остаются только операции с map. + +TTL — 30 секунд, величина внутренняя и пользовательской настройкой не является: +это компенсация задержки между ответом авторизации и появлением клиента в +статистике, а не политика доступа. Выбор fail-closed: в аномальном случае +возможен короткий ложный отказ, но параллельные auth больше не перепрыгивают +лимит. + +Ни Redis, ни таблицы в базе, ни распределённых блокировок: HY2XS — один процесс +на одном сервере с Hysteria. + +Чего механизм не обещает: без обратного вызова от Hysteria «соединение +установлено / не установлено» математически точной системы резервирования не +построить. + +--- + +## GATE-01 — гейт fail-open срабатывал на корректном коде + +Найдено при переписывании приёмки. Проверка «авторизация не возвращает успех из +ветки ошибки» была записана так: + +```js +const failOpen = /err != nil \{[\s\S]*?return \*peer\.Id/; +``` + +Ленивый `[\s\S]*?` свободно пересекает границы блоков, поэтому регулярка +срабатывала на ЛЮБОЙ функции, где после какой-нибудь проверки ошибки где-то +ниже стоит успешный возврат. Проверено прямо на коде из `HEAD`: на корректной +реализации она даёт совпадение, то есть гейт нельзя удовлетворить, не сломав +продукт. + +**Как закрыто.** Тело ветки выделяется по балансу фигурных скобок — тогда +«внутри ветки» действительно означает внутри ветки. Логика гейта проверена в +обе стороны: на настоящей дыре срабатывает, на корректном коде — нет. + +--- + +## UX-12 — импорт не разбирал свой исход + +`importPeerApi` не объявлял `skipErrorToast`, а `handleImport` не имел ни +`try`, ни `catch`. Пока импорт не умел завершаться частично, это было незаметно. +После CORE-05 отказ уходил бы необработанным отклонением промиса, `handleQuery()` +до выполнения не доходил — список оставался с прежними данными при уже +изменённой базе, — а общий перехватчик показывал бы частичный результат красной +ошибкой, то есть сообщал бы оператору обратное тому, что произошло. + +**Как закрыто.** Импорт разбирает исход тем же `reportPeerActionError`, что и +действия строки, а файл убирается из очереди и список обновляется при любом +исходе. + +Заодно формулировка `peer_disconnect_failed` во всех трёх местах (сервер и обе +локали) сделана **operation-neutral**. Прежняя — «новые подключения пира +запрещены» — была верна ровно для отключения пира; теперь через этот код +отчитываются восемь операций, а для удалённого пира она просто бессмысленна. + +--- + ## Чем закреплено **Тесты Go** (`apps/service/peer_access_test.go`, @@ -379,20 +656,68 @@ EX-03: она обещала более узкий набор, чем серве * непустой `msg` вместе с отсутствием в нём токена и query-строки; * форма ответа страницы логов на всех ветках и пропуск битой строки. +**Тесты второго прохода** (`peer_access_policy_test.go`, +`peer_reconcile_test.go`, `cron_test.go`, `peer_admission_test.go`): + +* границы политики доступа точечно — набор состоит в основном из равенств, + потому что расходились именно они; fail-closed на повреждённой строке; +* удаление: `disabled` записан ДО `/kick` (снимком базы в момент прихода + запроса), строка остаётся при неудаче разрыва, повторяемость; +* правка: ротация секрета, урезание квоты ниже расхода и ровно по расходу, + перенос срока в прошлое, снижение лимита устройств — каждое рвёт сессию; + послабления и косметика — нет; сочетание изменений даёт ОДИН `/kick`; +* импорт: старый `authId` после commit, откат партии не рвёт ничего, batch с + дедупликацией, частичный результат при неудаче разрыва; +* cron: границы `usage == quota`, `quota == 0`, истёкший срок и истёкшая + блокировка; сбор трафика ДО enforcement (снимком расхода в момент `/online`); + синхронность джобы; пропуск наложенного тика; отсутствие паники на пустом + секрете и на строке без идентификатора; работа при systemd, отвечающем + «служба неактивна»; +* лимит устройств под нагрузкой: два одновременных запроса на последнее + свободное место — барьер на стороне Traffic Stats API держит оба до тех пор, + пока оба не прочитают одно и то же состояние. Проверено, что тест ловит + прежнюю реализацию: с ней проходят оба запроса. + +`go test -race ./service/...` — отдельный обязательный шаг сборки: состояние +трекера разрешений и мьютекс цикла учёта принадлежат процессу, и их +корректность не наблюдаема в обычном прогоне. + **Контрактные тесты панели** (`tools/test/frontend-contract.test.ts`): общий `LogViewer` на обеих страницах, явные ширины колонок, запрос внутри `try`, единственность сборки скачивания, меню на `command` с пунктом `toggle-disabled`, ограничение ширины подсказки, единственный `ElMessageBox.confirm`, разбор частичного результата по коду, совпадение -подсказки имени с серверной константой. +подсказки имени с серверной константой, разбор исхода импорта с обновлением +списка при любом результате. **Гейты приёмки** (`tools/build/lib/acceptance.sh`): `run_access_revocation_acceptance` и `run_observability_acceptance`. +Гейты закрывают архитектурные инварианты, а не поведение: + +```text +правило доступа объявлено один раз, и колонок политики нет в SQL; +production POST /kick достижим только через disconnectAuthIDs; +disconnectAuthIDs не читает и не пишет состояние пира; +в CronHandleAccount нет отсоединённых горутин; +Hysteria2IsRunning не участвует в cron и в авторизации; +DeletePeer завершает сессию до удаления строки; +импорт разрывает сессии после COMMIT; +Delete и Import отвечают структурированной ошибкой; +детектор гонок обязателен и не имеет обходов. +``` + +Что гейтами НЕ доказывается и намеренно оставлено тестам: что удаление +действительно сохраняет `disabled` до разрыва, что импорт рвёт именно старый +`authId`, что лимит устройств выдерживает параллельные запросы. Это поведение, и +grep о нём сказать ничего не может. + Отдельно: проверка «единственный `ElMessageBox.confirm`» сначала поймала собственный комментарий, объясняющий, почему прямого вызова здесь больше нет, — ровно та ловушка, о которой предупреждает `code_without_comments` в -`acceptance.sh`. Проверки панели теперь тоже отбрасывают комментарии. +`acceptance.sh`. Проверки панели теперь тоже отбрасывают комментарии. Тот же +класс дал GATE-01: проверка, написанная регуляркой по тексту, срабатывала на +корректном коде. --- @@ -418,3 +743,31 @@ EX-03: она обещала более узкий набор, чем серве 9. проверить ширину подсказки «Экспорт настроек» на узком экране; 10. проверить, что отмена любого подтверждения не оставляет ошибок в консоли браузера. + +Добавлено вторым проходом: + +11. **удалить пира с активным подключением** и убедиться, что соединение + обрывается, а не только исчезает строка; +12. остановить `hysteria-server`, удалить пира — строка обязана остаться в + списке отключённой, с предупреждением о частичном результате; поднять + службу и повторить удаление; +13. **сменить секрет** пира с активным подключением: соединение обрывается, + старая клиентская ссылка перестаёт работать, новая работает; +14. **урезать квоту** ниже израсходованного у подключённого пира — соединение + обрывается немедленно, а не со следующим тиком cron; +15. **перенести срок** действия в прошлое — то же; +16. **снизить лимит устройств** у пира с несколькими подключениями: все + обрываются, после переподключения проходит новое разрешённое число; +17. **импортировать файл** с уже существующими пирами — их соединения + обрываются один раз; вновь созданные пиры не затрагиваются; при + остановленной Hysteria импорт применяется целиком и сообщает о частичном + результате, а список обновляется; +18. **израсходовать квоту до нуля** на живой сессии и дождаться тика cron: + соединение обрывается (раньше — не обрывалось никогда); +19. дождаться истечения срока действия на живой сессии — то же; +20. остановить `systemctl` (не Hysteria) и убедиться, что учёт трафика и + принудительное отключение продолжают работать; +21. подключить **одновременно** больше устройств, чем разрешено, и убедиться, + что принято ровно `maxDevices`; +22. перезапустить админку под нагрузкой и убедиться, что в журнале нет записей + о работе с закрытой базой после остановки. diff --git a/docs/admin/04-admin-panel.md b/docs/admin/04-admin-panel.md index 030659c..7f5f3ef 100644 --- a/docs/admin/04-admin-panel.md +++ b/docs/admin/04-admin-panel.md @@ -537,19 +537,58 @@ upstream выберет для нового секрета. Список мар Конфигурация Hysteria остаётся доступной панели **на чтение и на выгрузку**: `GET /config/getHysteria2Config` и `POST /config/exportHysteria2Config`. +### Правило доступа объявлено один раз + +Пускать пира или нет — решает одна функция, `peerAccessDenied` +(`apps/service/peer_access.go`). Её же применяет принудительное отключение в +cron. Второго экземпляра правила в продукте нет, и это главное свойство слоя +доступа. + +Границы: + +| условие | результат | +| --- | --- | +| `disabled = 1` | доступа нет | +| `quotaBytes = -1` | квота не ограничена | +| `download + upload >= quotaBytes` (при `quotaBytes >= 0`) | доступа нет | +| `expiresAt > 0` и `now >= expiresAt` | доступа нет | +| `bannedUntil > now` | доступа нет | +| строка без любого из этих полей | доступа нет | + +Каждая граница выбрана по смыслу самого названия, и три из них стоит назвать +отдельно: + +* **`quotaBytes = 0` — это ноль байтов, а не безлимит.** Единственный способ + снять ограничение — `-1`. +* **`usage = quota` — лимит исчерпан.** Счётчики растут порциями по ответу + Traffic Stats API, поэтому точное равенство — обычный исход очередного + сбора, а не экзотика. +* **`bannedUntil = now` — блокировка уже закончилась.** Она задаётся как «до» + момента, и наступивший момент означает её конец. + +Строка без решающего поля трактуется как повреждённая: все эти колонки +объявлены `NOT NULL DEFAULT`, поэтому `NULL` здесь означать может только +повреждение, а на пути принятия решения о доступе оно обязано вести к отказу. + +Что было до этого: правило существовало в двух экземплярах — SQL-условием +внутри `Hysteria2Auth` и другим SQL-условием внутри cron, — и расходилось ровно +на перечисленных границах. Практическое следствие было хуже расхождения: пир с +исчерпанной квотой не пускался заново, но его живая сессия не разрывалась +никогда, потому что cron требовал СТРОГОГО превышения. Он продолжал +пользоваться доступом, пока не переподключался по своей воле. + ### Отзыв доступа к VPN состоит из двух половин Панель не управляет жизненным циклом Hysteria, но доступом пиров управляет -целиком — и здесь у неё есть ровно один механизм, требующий обеих половин -официального контракта Hysteria. +целиком — и здесь требуются обе половины официального контракта Hysteria. ```text -disabled = 1 закрывает БУДУЩИЕ обращения к HTTP-auth -POST /kick завершает УЖЕ УСТАНОВЛЕННУЮ сессию +сохранённое состояние закрывает БУДУЩИЕ обращения к HTTP-auth +POST /kick завершает УЖЕ УСТАНОВЛЕННУЮ сессию ``` -Ни одна половина не работает по отдельности. Запись `disabled=1` видит только -выборка в `Hysteria2Auth`, то есть проверяется при следующем подключении; +Ни одна половина не работает по отдельности. Сохранённое состояние видит только +`peerAccessDenied`, то есть оно проверяется при следующем подключении; установленная QUIC-сессия живёт своей жизнью и сама не разрывается. Обратно: `/kick` завершает сессию, но клиент немедленно переподключается — поэтому официальная документация Hysteria и требует одновременной блокировки в auth @@ -558,18 +597,106 @@ backend. **Порядок обязателен и обратному не подлежит:** ```text -1. записать disabled = 1 (долговременное состояние) -2. POST /kick по authId пира (разрыв) +1. записать долговременное состояние +2. POST /kick по authId пира ``` При обратном порядке клиент успевает переподключиться в окне между разрывом и -записью и остаётся на связи с формально отключённым пиром. +записью и остаётся на связи с уже изменённым пиром. -**Неудача второго шага не откатывает первый.** Безопасная половина достигнута; -возвращать пиру полный доступ из-за отказа разрыва нельзя. Операция отвечает -частичным результатом с кодом `peer_disconnect_failed`, панель показывает его -предупреждением и обновляет строку. Повторить операцию можно тем же действием: -условие смотрит на запрошенное состояние, а не на переход из включённого. +#### Какие операции проходят по этому пути + +Разрыв нужен не только при отключении пира. Полный список — и это ровно те +операции, которые способны сделать живую сессию устаревшей: + +| операция | что рвётся | +| --- | --- | +| отключение пира (`disabled = 1`) | сессия пира | +| временная блокировка | сессия пира | +| смена секрета | сессия пира: прежние учётные данные недействительны | +| квота урезана так, что доступ уже закрыт | сессия пира | +| срок перенесён в прошлое | сессия пира | +| лимит устройств снижен | все сессии пира | +| **удаление пира** | сессия пира, по запомненному `authId` | +| **импорт партии** | сессии всех существующих пиров партии, по СТАРЫМ `authId` | + +Правило асимметрично намеренно: **ограничение применяется немедленно, +послабление — нет.** Увеличенная квота, продлённый срок, поднятый лимит +устройств, правка имени или пометки сессию не рвут — у оператора нет причины +ронять работающее соединение, расширяя пиру права. + +Все они идут через один `reconcileLiveSessions`, а он — через единственный в +продукте вход к `/kick`, `disconnectAuthIDs`. Отдельных методов разрыва для +каждой операции нет намеренно: иначе «изменение применили, а сессию завершить +забыли» появлялось бы заново с каждой новой операцией — именно так это и +случилось с удалением и импортом. + +#### Удаление пира + +```text +1. прочитать пира и запомнить его authId +2. записать disabled = 1 +3. POST /kick по запомненному authId +4. удалить строку +``` + +Шаг 1 существует потому, что вместе со строкой исчезает `authId` — то есть +единственное, чем сессию можно было бы завершить. Прежняя реализация состояла +из одного шага 4, и состояние после неё было **невосстановимым**: удалённый пир +пользовался доступом до собственного переподключения, и сделать с этим было уже +нечего. + +Исходы: + +| что произошло | состояние | +| --- | --- | +| запись не удалась | строка не изменена, удаления не было | +| разрыв не удался | строка осталась с `disabled = 1`, новые подключения запрещены | +| разрыв прошёл, удаление не удалось | строка отключена, сессия уже завершена | + +Ни один не возвращает пиру доступ. Оператор повторяет удаление тем же +действием. + +#### Импорт партии + +Импорт — это bulk state replacement: он переписывает `authId`, секрет, квоту, +срок и `disabled` существующего пира целиком. Поэтому: + +```text +валидация партии + ↓ +подготовка криптоматериала + ↓ +транзакция: собрать СТАРЫЕ authId + применить все изменения + ↓ +COMMIT + ↓ +дедупликация + POST /kick одной пачкой +``` + +Оба слова в «внутри транзакции, после commit» существенны. **Внутри** — потому +что после commit старого `authId` в базе уже нет. **После** — потому что `/kick` +до commit оставляет клиенту окно, в котором он переподключается к ещё не +изменённому пиру. + +Рвутся сессии **всех** существующих записей партии, а не тех, у кого изменилось +конкретное поле. Это сознательно более простой контракт, чем diff по семи +полям: не появляется второй таблицы правил «какие поля импорта считаются +access-changing», то есть второго места, где политика может разойтись с +`peerAccessDenied`. Цена — существующие пиры партии один раз переподключаются; +для административной операции переноса это нормальная цена. Вновь созданные +пиры не рвутся: до импорта их сессий существовать не могло. + +#### Частичный результат + +**Неудача разрыва не откатывает сохранённое состояние.** Безопасная половина +достигнута; возвращать доступ из-за отказа второго шага нельзя. Операция +отвечает кодом `peer_disconnect_failed`, панель показывает его предупреждением +и обновляет список. + +Формулировка сообщения **не называет конкретную операцию**: через этот код +отчитываются все восемь строк таблицы выше, а для удалённого пира фраза «новые +подключения пира запрещены» была бы просто бессмысленной. **Отключение и временная блокировка — разные механизмы**, и смешивать их нельзя: @@ -579,14 +706,49 @@ backend. | `disabled` | только руками оператора | отзыв доступа | | `banned_until` | истекает сам | временная блокировка | -Поэтому `DisconnectPeers` не пишет в базу вовсе, включение пира не сбрасывает -`banned_until`, а снятие блокировки не включает отключённого пира. +Поэтому `disconnectAuthIDs` не пишет в базу вовсе и не читает её: он принимает +готовые `authId`. Включение пира не сбрасывает `banned_until`, а снятие +блокировки не включает отключённого пира. **Состояние службы по systemd в этом пути не участвует.** `util.Exec` схлопывает «systemctl вернул 3, служба неактивна» и «запустить systemctl не удалось» в одну ошибку, поэтому `Hysteria2IsRunning` не является основанием ни -для отказа операции, ни для её пропуска. Ответ даёт само обращение к Traffic -Stats API. +для отказа операции, ни для её пропуска — ни здесь, ни в cron. Ответ даёт само +обращение к Traffic Stats API. + +### Цикл учёта принадлежит планировщику + +`CronHandleAccount` выполняется синхронно, под одним мьютексом на весь цикл, и +строго в этом порядке: + +```text +TryLock (пропустить тик, если предыдущий ещё идёт) + ↓ +порт Traffic Stats API + секрет + ↓ +GET /traffic?clear=1 → записать дельты в счётчики пиров + ↓ +GET /online → применить peerAccessDenied → POST /kick +``` + +Порядок обязателен: enforcement принимает решение по счётчикам, значит счётчики +должны быть уже обновлены. Раньше обе половины запускались параллельными +горутинами внутри ещё одной горутины, поэтому превышение квоты замечалось в +лучшем случае со следующего тика, а планировщик считал джобу завершённой почти +мгновенно — `StopCron()` не ждал настоящей работы, и после закрытия SQLite +горутины продолжали в неё писать. + +**Учёт трафика — операционная граница, а не биллинг.** Чтение `GET +/traffic?clear=1` деструктивно по контракту Traffic Stats API: счётчики +Hysteria обнуляются сразу после отправки ответа, поэтому каждая дельта +существует ровно в одном экземпляре. Если запись в SQLite не удалась, дельта +потеряна безвозвратно — это записывается в журнал уровнем `error`, но не +компенсируется. Полностью закрыть окно можно только сменой модели учёта: +недеструктивный `GET /traffic` плюс долговременные checkpoint'ы верхних +счётчиков и вычисление дельты на стороне админки. Это отдельная подсистема с +обработкой перезапуска и сброса счётчиков Hysteria, и в `1.0.0` она намеренно +не вводится. Квота здесь — операционный предел доступа, а не учёт с финансово +значимым каждым байтом. ### Ограничение устройств проверяется fail-closed @@ -609,6 +771,47 @@ Hysteria, значит она жива, а её Traffic Stats API слушает показывают пустую картину, когда служба остановлена, — это честный ответ на вопрос «кто сейчас на связи». +#### Лимит выдерживает параллельные подключения + +Сравнения ответа `/online` с `maxDevices` недостаточно. Ответив «allow», панель +не создаёт подключение — его только начинает устанавливать Hysteria, и клиент +попадает в статистику позже. Поэтому: + +```text +A: GET /online -> 2 B: GET /online -> 2 + max = 3 +A: 2 < 3 -> allow B: 2 < 3 -> allow + стало 4 +``` + +Объявленный «Лимит устройств: 3» превышался ровно тем способом, от которого +лимит и должен защищать. Мьютекс вокруг `/online` это не чинит: следующий +запрос, даже строго после первого, продолжает видеть прежнее число. + +Панель ведёт собственный учёт уже выданных, но ещё не проявившихся разрешений +(`apps/service/peer_admission.go`): + +```text +1. обычная проверка политики доступа +2. GET /online (вне блокировки: сеть не должна сериализовать все подключения) +3. снять протухшие разрешения +4. рост online означает, что столько же разрешений превратились в подключения +5. решение по сумме: online + выданные разрешения +6. свободно -> занять место и allow; иначе deny +``` + +Учёт **process-local**: HY2XS — один процесс на одном сервере с Hysteria, и ни +Redis, ни таблицы в базе, ни распределённых блокировок для этого не нужно. +Разрешение живёт 30 секунд — величина внутренняя и пользовательской настройкой +не является: это компенсация задержки между ответом авторизации и появлением +клиента в статистике, а не политика доступа. Если клиент авторизовался и не +подключился, резервация исчезает сама. + +Чего механизм не обещает: без обратного вызова от Hysteria «соединение +установлено / не установлено» математически точной системы резервирования не +построить. Он закрывает конкретный и реальный случай — параллельные HTTP-auth +одного процесса — и делает это fail-closed. + ### Что нельзя делать - собирать admin-компонент на target server; @@ -621,6 +824,15 @@ Hysteria, значит она жива, а её Traffic Stats API слушает - откатывать `disabled` из-за неудачи `/kick`; - писать `banned_until` из пути отключения пира; - пропускать проверку лимита устройств, когда Traffic Stats API не ответил; +- заводить второй предикат доступа рядом с `peerAccessDenied` — в том числе в + виде SQL-условия внутри выборки; +- обращаться к `/kick` мимо `disconnectAuthIDs`; +- удалять пира, не запомнив его `authId` и не завершив сессию до удаления; +- разрывать сессии импорта до `COMMIT` либо по новым `authId`; +- запускать работу джобы учёта в отсоединённых горутинах: планировщик обязан + её видеть, иначе `StopCron()` вернётся раньше, чем она закончит; +- считать квоту биллинговым учётом: чтение `/traffic?clear=1` деструктивно; +- делать срок жизни pending-разрешения пользовательской настройкой; - экспортировать конфиг Hysteria через типизированную модель — так теряются неизвестные upstream-поля; - выгружать конфиг с секретами в открытом виде. @@ -791,3 +1003,13 @@ Compatibility-ветка пережила слой совместимости, 15. bootstrap-учётные данные приходят от оркестратора и никогда не генерируются и не логируются админкой 16. любой журнал, покидающий сервер, проходит санитайз 17. пароль администратора хранится ровно в одном формате — bcrypt +18. правило доступа объявлено ровно один раз (`peerAccessDenied`), и авторизация + с принудительным отключением спрашивают именно его +19. каждая операция, способная сделать живую сессию устаревшей, проходит через + один `reconcileLiveSessions`, а он — через единственный вход к `/kick` +20. долговременное состояние записывается ДО разрыва, и неудача разрыва его не + откатывает +21. удаление пира завершает его сессию до того, как исчезнет `authId` +22. импорт разрывает старые сессии после `COMMIT` и по старым `authId` +23. джоба учёта выполняется синхронно, и `StopCron()` её дожидается +24. лимит устройств не превышается параллельными запросами авторизации diff --git a/tools/build/lib/acceptance.sh b/tools/build/lib/acceptance.sh index cca67df..0a956bd 100644 --- a/tools/build/lib/acceptance.sh +++ b/tools/build/lib/acceptance.sh @@ -975,6 +975,39 @@ $piped_matcher" } ' || fail "acceptance: утверждение о пройденных тестах обязано следовать за прогоном" + # Детектор гонок — часть прогона админки, а не пожелание. + # + # В продукте есть состояние, принадлежащее ПРОЦЕССУ: учёт выданных + # разрешений на устройства и мьютекс цикла учёта. Оба существуют ровно затем, + # чтобы вести себя правильно под параллельным доступом, и обычный `go test` + # об их корректности не говорит ничего. + # + # Проверяется и то, что прогон не умеет молча пропуститься: сборка, + # пропускающая проверку при недоступном компиляторе, выдала бы внешне + # неотличимый production-артефакт — тот же класс, что и SKIP_TESTS. + "$BUN_BIN" -e ' + const source = require("node:fs").readFileSync("tools/build/lib/package.sh", "utf8"); + const admin = source.slice(source.indexOf("run_admin_tests()")); + const adminBody = admin.slice(0, admin.search(/\n\}[\r\n]/)); + if (!adminBody.includes("run_admin_race_tests")) { + throw new Error("прогон админки больше не включает детектор гонок"); + } + const start = source.indexOf("run_admin_race_tests() {"); + if (start < 0) throw new Error("не найдена функция run_admin_race_tests"); + const rest = source.slice(start); + const body = rest.slice(0, rest.search(/\n\}[\r\n]/)); + if (!body.includes("-race")) throw new Error("детектор гонок не включён"); + if (!body.includes("CGO_ENABLED=1")) { + throw new Error("детектор гонок требует cgo, а production-сборка идёт с CGO_ENABLED=0"); + } + if (!/command -v (cc|gcc)[\s\S]*?\|\| *fail/.test(body)) { + throw new Error("отсутствие компилятора обязано ронять сборку, а не пропускать прогон"); + } + for (const bypass of ["|| true", "SKIP_RACE", "continue"]) { + if (body.includes(bypass)) throw new Error("детектор гонок можно обойти: " + bypass); + } + ' || fail "acceptance: прогон детектора гонок обязан быть обязательным" + # У шага не должно быть обходов — ни объявленных, ни забытых. # # Раньше их было два: ALLOW_VULNERABLE_DEPENDENCIES=true писал в metadata @@ -1535,12 +1568,31 @@ run_access_revocation_acceptance() { # установленная QUIC-сессия сама по себе не рвётся. Официальная документация # описывает `/kick` и блокировку в auth backend как пару — по отдельности не # работает ни одна половина. - code_has apps/service/hysteria2_api.go -F -- 'func DisconnectPeers' \ + code_has apps/service/hysteria2_api.go -F -- 'func disconnectAuthIDs' \ || fail "acceptance: the session disconnect primitive is missing" - code_has apps/service/peer.go -F -- 'func disconnectAfterRevoke' \ + code_has apps/service/peer.go -F -- 'func reconcileLiveSessions' \ || fail "acceptance: revoking access must go through a single disconnect path" - code_has apps/service/peer.go -F -- 'DisconnectPeers(' \ - || fail "acceptance: revoking access never reaches the Traffic Stats /kick" + + log_step "Acceptance: the Traffic Stats /kick has exactly one caller" + # Пока обращений к `/kick` было два — в отзыве доступа и в cron, — они + # расходились: у cron не было ни дедупликации, ни разбиения на части, зато + # был POST с пустым массивом каждые 30 секунд. Один вход в Hysteria делает + # такое расхождение невозможным. + "$BUN_BIN" -e ' + const fs = require("node:fs"); + const offenders = []; + for (const file of ["apps/service/peer.go", "apps/service/cron.go", "apps/service/peer_import.go"]) { + if (!fs.existsSync(file)) continue; + const source = fs.readFileSync(file, "utf8") + .split("\n") + .filter((line) => !/^\s*\/\//.test(line)) + .join("\n"); + if (source.includes("KickUsers(")) offenders.push(file); + } + if (offenders.length) { + throw new Error("the Traffic Stats /kick is reached outside disconnectAuthIDs: " + offenders.join(", ")); + } + ' || fail "acceptance: production /kick must go through disconnectAuthIDs only" log_step "Acceptance: session disconnect does not write peer state" # Прежний Hysteria2Kick вместе с разрывом проставлял `banned_until`, поэтому @@ -1548,18 +1600,147 @@ run_access_revocation_acceptance() { # заодно временную блокировку — другой механизм с другим сроком жизни. "$BUN_BIN" -e ' const source = require("node:fs").readFileSync("apps/service/hysteria2_api.go", "utf8"); - const start = source.indexOf("func DisconnectPeers"); - if (start < 0) throw new Error("DisconnectPeers is missing"); + const start = source.indexOf("func disconnectAuthIDs"); + if (start < 0) throw new Error("disconnectAuthIDs is missing"); const rest = source.slice(start + 1); const end = rest.indexOf("\nfunc "); const body = end < 0 ? rest : rest.slice(0, end); - for (const forbidden of ["banned_until", "disabled", "dao.UpdatePeer("]) { + for (const forbidden of ["banned_until", "disabled", "dao.UpdatePeer(", "dao.GetPeer("]) { if (body.includes(forbidden)) { - throw new Error("DisconnectPeers writes peer state: " + forbidden); + throw new Error("disconnectAuthIDs touches peer state: " + forbidden); } } ' || fail "acceptance: the disconnect primitive must not write peer state" + log_step "Acceptance: the access policy is declared once" + # Правило доступа существовало в двух экземплярах — SQL-условием в + # Hysteria2Auth и другим SQL-условием в cron, — и расходилось ровно на + # границах: `quota == usage`, `quota == 0`, `now == expiresAt`, + # `now == bannedUntil`. Пир с исчерпанной квотой не пускался заново, но его + # живая сессия не разрывалась никогда. + code_has apps/service/peer_access.go -F -- 'func peerAccessDenied' \ + || fail "acceptance: the single access predicate is missing" + "$BUN_BIN" -e ' + const fs = require("node:fs"); + const offenders = []; + for (const file of ["apps/service/hysteria2_api.go", "apps/service/cron.go"]) { + const source = fs.readFileSync(file, "utf8") + .split("\n") + .filter((line) => !/^\s*\/\//.test(line)) + .join("\n"); + // Колонки политики доступа не имеют права появляться в запросах: решение + // принимает peerAccessDenied, а не выборка. + for (const column of ["quota_bytes", "expires_at", "banned_until"]) { + if (source.includes(column)) offenders.push(file + ":" + column); + } + } + if (offenders.length) { + throw new Error("the access policy is back inside SQL: " + offenders.join(", ")); + } + ' || fail "acceptance: the access policy must live in peerAccessDenied, not in SQL" + + log_step "Acceptance: the account cron job belongs to the scheduler" + # `CronHandleAccount -> go func() -> go saveAccountTraffic; go kickAccount` + # заканчивалась для планировщика почти мгновенно, поэтому StopCron не ждал + # настоящей работы: releaseResource закрывал SQLite, а горутины продолжали + # писать в закрытое соединение. + "$BUN_BIN" -e ' + const source = require("node:fs").readFileSync("apps/service/cron.go", "utf8"); + const start = source.indexOf("func CronHandleAccount"); + if (start < 0) throw new Error("CronHandleAccount is missing"); + const rest = source.slice(start + 1); + const end = rest.indexOf("\nfunc "); + const body = (end < 0 ? rest : rest.slice(0, end)) + .split("\n") + .filter((line) => !/^\s*\/\//.test(line)) + .join("\n"); + if (/\bgo\s+(func|[A-Za-z_])/.test(body)) { + throw new Error("the account cron job still detaches its work into goroutines"); + } + ' || fail "acceptance: the account cron job must run synchronously" + + log_step "Acceptance: systemd opinion drives no cron or access decision" + # util.Exec схлопывает «systemctl вернул 3» и «запустить systemctl не + # удалось» в одну ошибку, поэтому на этом значении нельзя строить решения: + # сломанный systemctl при живой Hysteria молча отключал учёт и enforcement. + ! code_has apps/service/cron.go -F -- 'Hysteria2IsRunning' \ + || fail "acceptance: the account cron job must not gate on the systemd state" + "$BUN_BIN" -e ' + const source = require("node:fs").readFileSync("apps/service/hysteria2_api.go", "utf8"); + const start = source.indexOf("func Hysteria2Auth"); + if (start < 0) throw new Error("Hysteria2Auth is missing"); + const rest = source.slice(start + 1); + const end = rest.indexOf("\nfunc "); + const body = (end < 0 ? rest : rest.slice(0, end)) + .split("\n") + .filter((line) => !/^\s*\/\//.test(line)) + .join("\n"); + if (body.includes("Hysteria2IsRunning")) { + throw new Error("auth gates on the systemd state"); + } + ' || fail "acceptance: auth must not gate on the systemd state" + + log_step "Acceptance: concurrent auth cannot exceed the device limit" + # Между чтением `/online` и ответом «allow» место ничем не удерживалось: + # два одновременных запроса при `online = max-1` получали разрешение оба. + code_has apps/service/peer_admission.go -F -- 'func reserveDeviceSlot' \ + || fail "acceptance: the device admission tracker is missing" + code_has apps/service/hysteria2_api.go -F -- 'reserveDeviceSlot(' \ + || fail "acceptance: auth does not reserve a device slot" + + log_step "Acceptance: deleting a peer revokes access before removing the row" + # `return dao.DeletePeer(...)` убирал строку вместе с auth_id — то есть + # вместе с единственным, чем можно было бы завершить живую сессию. + "$BUN_BIN" -e ' + const source = require("node:fs").readFileSync("apps/service/peer.go", "utf8"); + const start = source.indexOf("func DeletePeer"); + if (start < 0) throw new Error("DeletePeer is missing"); + const rest = source.slice(start + 1); + const end = rest.indexOf("\nfunc "); + const body = end < 0 ? rest : rest.slice(0, end); + const reconcile = body.indexOf("reconcileLiveSessions("); + const remove = body.indexOf("dao.DeletePeer("); + if (reconcile < 0) throw new Error("DeletePeer does not terminate live sessions"); + if (remove < 0) throw new Error("DeletePeer does not remove the row"); + if (reconcile > remove) throw new Error("DeletePeer removes the row before terminating the session"); + ' || fail "acceptance: peer deletion must terminate the session before dropping the row" + + log_step "Acceptance: peer import reconciles live sessions after commit" + # Импорт переписывает credential- и access-состояние целиком, включая + # auth_id. Старое значение существует только ДО commit, а разрывать сессию + # можно только ПОСЛЕ него. + "$BUN_BIN" -e ' + const source = require("node:fs").readFileSync("apps/service/peer.go", "utf8"); + const start = source.indexOf("func UpsertPeerExport"); + if (start < 0) throw new Error("UpsertPeerExport is missing"); + const rest = source.slice(start + 1); + const end = rest.indexOf("\nfunc "); + const body = end < 0 ? rest : rest.slice(0, end); + const tx = body.indexOf("dao.WithPeerTx("); + const reconcile = body.indexOf("reconcileLiveSessions("); + if (reconcile < 0) throw new Error("peer import never reconciles live sessions"); + if (tx < 0 || reconcile < tx) throw new Error("peer import reconciles before the transaction"); + ' || fail "acceptance: peer import must reconcile live sessions after commit" + + log_step "Acceptance: delete and import report structured service errors" + # Обе операции умеют завершиться ЧАСТИЧНО — состояние применено, живую + # сессию завершить не удалось. Через vo.Fail такой исход уезжал бы панели + # неотличимо от полного отказа. + "$BUN_BIN" -e ' + const source = require("node:fs").readFileSync("apps/controller/peer.go", "utf8"); + for (const [handler, call] of [["func DeletePeer", "service.DeletePeer("], ["func ImportPeer", "service.UpsertPeerExport("]]) { + const start = source.indexOf(handler); + if (start < 0) throw new Error(handler + " is missing"); + const rest = source.slice(start + 1); + const end = rest.indexOf("\nfunc "); + const body = end < 0 ? rest : rest.slice(0, end); + if (!body.includes(call)) throw new Error(handler + " no longer calls " + call); + if (!body.includes("failService(err, c)")) { + throw new Error(handler + " reports a partial result as a plain failure"); + } + } + ' || fail "acceptance: delete and import must use the structured service error path" + log_step "Acceptance: a partial revocation is reported by code, not by prose" # Долговременная запись к этому моменту уже применена и НЕ откатывается: # достигнутое безопасное состояние нельзя отменять из-за неудачи второго @@ -1603,9 +1784,41 @@ run_access_revocation_acceptance() { if (/[^a-z0-9_]Hysteria2Online\(\)/.test(code)) { throw new Error("auth takes the display-tolerant online path"); } - const failOpen = /err != nil \{[\s\S]*?return \*peer\.Id/; - if (failOpen.test(code)) { - throw new Error("auth still returns success from the /online failure branch"); + // Старая дыра имела конкретную форму: возврат УСПЕХА изнутри ветки + // обработки ошибки. + // + // onlineUsers, err := Hysteria2Online() + // if err != nil { + // logrus.WithError(err).Warn(...) + // return *peer.Id, *peer.AuthId, nil + // } + // + // Здесь стояла регулярка `err != nil \{[\s\S]*?return \*peer\.Id`, и она + // была неверна: ленивый `[\s\S]*?` свободно пересекает границы блоков, + // поэтому она срабатывала на ЛЮБОЙ функции, где после какой-нибудь + // проверки ошибки где-то ниже стоит успешный возврат, — то есть на + // правильном коде тоже. Гейт, который падает на корректной реализации, + // не проверяет ничего: его нельзя удовлетворить, не сломав продукт. + // + // Тело ветки выделяется по балансу фигурных скобок — тогда «внутри + // ветки» действительно означает внутри ветки. + const failureBranches = []; + for (let at = code.indexOf("if err != nil {"); at >= 0; at = code.indexOf("if err != nil {", at + 1)) { + let depth = 0; + let end = at; + for (let i = code.indexOf("{", at); i < code.length; i++) { + if (code[i] === "{") depth++; + else if (code[i] === "}") { + depth--; + if (depth === 0) { end = i; break; } + } + } + failureBranches.push(code.slice(at, end + 1)); + } + for (const branch of failureBranches) { + if (/return \*peer\.Id/.test(branch)) { + throw new Error("auth still returns success from an error branch"); + } } ' || fail "acceptance: the device limit must be fail-closed" diff --git a/tools/build/lib/package.sh b/tools/build/lib/package.sh index c47a2f5..005f55b 100644 --- a/tools/build/lib/package.sh +++ b/tools/build/lib/package.sh @@ -96,10 +96,39 @@ run_admin_tests() { GOTOOLCHAIN=local "$GO_BIN" test ./... ) || fail "HY2XS admin contract tests failed" + run_admin_race_tests + ADMIN_TESTS_PASSED="true" export ADMIN_TESTS_PASSED } +# Детектор гонок — отдельный прогон, и он ОБЯЗАТЕЛЕН. +# +# В продукте есть состояние, принадлежащее процессу, а не базе: учёт выданных +# разрешений на устройства (peer_admission.go) и мьютекс цикла учёта +# (cron.go). Оба существуют ровно затем, чтобы вести себя правильно под +# параллельным доступом, — то есть их корректность не наблюдаема ни в обычном +# прогоне тестов, ни в `go vet`. +# +# Пропуск при недоступном C-компиляторе здесь НЕ предусмотрен, и это осознанно. +# «Тихо не проверили» — ровно тот класс, от которого лечится запрет SKIP_TESTS +# в этом же файле: сборка, молча пропускающая проверку, выдаёт внешне +# неотличимый production-артефакт. build-essential входит в обязательные +# зависимости сборщика (см. tools/build/lib/deps.sh), поэтому попасть в ветку +# отказа можно только на сломанном окружении — и об этом нужно узнать. +# +# CGO_ENABLED=1 задаётся явно: production-сборка админки идёт с CGO_ENABLED=0, +# и унаследованное значение отключило бы детектор. +run_admin_race_tests() { + command -v cc >/dev/null 2>&1 || command -v gcc >/dev/null 2>&1 \ + || fail "race detector requires a C toolchain (build-essential); refusing to ship untested concurrency" + + ( + cd "${UI_SRC:-apps}" + CGO_ENABLED=1 GOTOOLCHAIN=local "$GO_BIN" test -race ./service/... + ) || fail "HY2XS admin race detector found a data race" +} + build_orchestrator() { local bun_compile_target="${BUN_COMPILE_TARGET:-bun-linux-x64}" diff --git a/tools/test/frontend-contract.test.ts b/tools/test/frontend-contract.test.ts index 68603f5..cecd726 100644 --- a/tools/test/frontend-contract.test.ts +++ b/tools/test/frontend-contract.test.ts @@ -401,6 +401,30 @@ describe("действия над пиром", () => { expect(source).toContain("ElMessage.warning"); }); + // Импорт — такая же операция с частичным исходом, как и действия строки: + // партия применена целиком, а старые сессии обновлённых пиров завершить не + // удалось. + // + // Раньше у него не было ни try, ни catch: отказ уходил необработанным + // отклонением промиса, а обновление списка до выполнения не доходило — + // оператор видел прежние данные при уже изменённой базе. + test("импорт разбирает свой исход и обновляет список при любом", () => { + const source = codeOf(peerList()); + + const start = source.indexOf("async function handleImport"); + expect(start).toBeGreaterThan(-1); + const rest = source.slice(start + 1); + const end = rest.indexOf("\nasync function "); + const body = end < 0 ? rest : rest.slice(0, end); + + expect(body).toContain("reportPeerActionError"); + // Обновление списка стоит ПОСЛЕ разбора исхода, а не внутри успешной + // ветки: при частичном результате состояние в базе уже изменилось. + const handled = body.indexOf("reportPeerActionError"); + const refreshed = body.indexOf("handleQuery()"); + expect(refreshed).toBeGreaterThan(handled); + }); + // Срок временной блокировки называется оператору: раньше `Date.now() + час` // был зашит в обработчик и не сообщался ни до, ни после. test("временная блокировка подтверждается и называет срок", () => { @@ -423,6 +447,12 @@ describe("действия над пиром", () => { "kickPeerApi", "updatePeerApi", "savePeerApi", + // Импорт применяет партию одной транзакцией, а старые сессии + // обновлённых пиров завершает после её фиксации. Второй шаг умеет не + // удаться отдельно от первого, и тогда общий перехватчик показал бы + // частичный результат красной ошибкой — сообщив оператору обратное + // тому, что произошло. + "importPeerApi", ]; for (const name of selfReporting) {