111 lines
16 KiB
Markdown
111 lines
16 KiB
Markdown
# Отчёт Opus — КРАКО и план доработок shared-sqs (2026-10-05)
|
||
|
||
> Полный текст ответа Opus (без сокращений) на промпт
|
||
> `opus-review-prompt-2026-10-05.md`. Сохранён для истории и критического
|
||
> анализа.
|
||
|
||
---
|
||
|
||
# 1. КРАКО — критический разбор архитектуры
|
||
|
||
## Что сделано хорошо
|
||
|
||
- **Изоляция тенантов проста и надёжна в ядре.** Ключ очереди `{AccessKey}:{queueName}` (`tenant_helpers.go`) делает кросс-тенантный доступ через SQS физически невозможным: тенант берётся из контекста, ключ строится из его AccessKey. Простое и верное решение.
|
||
- **SigV4 реально проверяется** (вопреки формулировке «подпись не проверяется» — это про JWT, не про SQS). В `auth_middleware.go` полноценный canonical request, `subtle.ConstantTimeCompare`, проверка clock skew, поддержка presigned. Это сильная сторона.
|
||
- **Аккуратно закрыты классические гонки.** Атомарный receive под одним `Lock` (устранён TOCTOU «потери» сообщений, `receive_message.go`), double-check под Lock в `send_message.go`, пер-очередной Lock в `PeriodicTasks` вместо глобального на весь обход (`gosqs.go`).
|
||
- **OOM-защита по нескольким фронтам:** лимит тела 3 MB, `MaxHeaderBytes`, пер-очередной лимит сообщений, глобальный лимит тенантов.
|
||
- **Продуманный graceful shutdown и HTTP-таймауты** с обоснованием под long-poll (`goaws.go`).
|
||
- **IDOR-защита в UI** и урезанные права UI-тенанта (не может создавать/удалять тенантов, видит только себя) — `admin.go`.
|
||
- **Персистентность с write-sequence guard** (last-write-wins по ключу) и синхронным удалением, чтобы удалённые очереди/сообщения не «воскресали» из Redis.
|
||
|
||
## Главные слабости и риски (по коду)
|
||
|
||
### 🔴 Критичные (безопасность)
|
||
|
||
1. **SecretKey детерминированно выводится из email + захардкоженная соль в исходнике.**
|
||
`tenant_store.go`: `secretKey = SHA256(lower(email) + ":" + "shared-sqs:secret-key:v1")`, `accessKey = SHA256(email)[:12]`. Email — не секрет. Кто знает email уже залогинившегося пользователя (тенант существует) и соль (она в репозитории форка), тот вычисляет AccessKey+SecretKey и получает **полный доступ к чужим очередям напрямую через SQS-эндпоинт** (JWT там не нужен — только SigV4). Это нарушает базовый принцип: секрет обязан быть случайным, а не функцией публичного идентификатора.
|
||
|
||
2. **Идентичность тенанта берётся из НЕподписанного JWT (claim `email`), а из email выводятся креды.**
|
||
`jwt.go` подпись не проверяет; доверие целиком на `PingNubesAPI`, который принимает токен, если **хоть один** стенд вернул не 401/403 (`jwt.go`). Если шлюз при GET `/svc` не связывает жёстко токен ↔ email, появляется вектор подмены email → провиженинг/доступ под чужой личностью. Плюс кэш валидации даёт отозванному токену жить до 5–7 мин (`cache.go`).
|
||
|
||
### 🟠 Высокие (production-readiness)
|
||
|
||
3. **Один глобальный мьютекс `SyncQueues` на весь сервис.** Любой send/receive/tick берёт единственный `Lock` (`globals.go`). `receiveMessagesUnderLock` держит глобальный Lock на время O(N)-скана сообщений, а long-poll переобходит его каждые 100 мс. Это главный архитектурный боттлнек — ровно он давал всплески latency, упомянутые в комментариях. Не масштабируется по числу очередей/тенантов.
|
||
|
||
4. **Невозможна горизонтальная масштабируемость → один под = SPOF.** Память — источник правды, Redis — write-through-кэш одной реплики. Две реплики разъедутся (каждая со своей `map`, last-write-wins затирает). Значит сервис принципиально single-instance: нет HA, рестарт = простой + окно потери данных.
|
||
|
||
5. **Асинхронная запись в Redis = окно потери подтверждённых сообщений.** `SendMessage` отвечает клиенту 200, а `HSet` уходит в горутину позже (`redis.go`). Краш пода между append-в-память и async-записью → подтверждённое сообщение потеряно. Для очереди это нарушение durability (AWS SQS персистит до ack).
|
||
|
||
6. **`MessageRetentionPeriod` не реализован** (`// TODO - not used`, `models.go`). Сообщения не истекают по времени, растут до пер-очередного лимита, после чего новые `SendMessage` отклоняются `OverLimit`. Нет TTL-эвикции — скрытая деградация.
|
||
|
||
7. **Сообщения хранятся плоским слайсом `[]SqsMessage`.** Receive/Delete/visibility-tick — O(N) на операцию; при лимите 120k сообщений и глобальном локе это дорого и усугубляет п.3. Нет индексов по ReceiptHandle/состоянию.
|
||
|
||
### 🟡 Средние
|
||
|
||
8. **Пересчёт legacy-тенанта в `CreateFromJWT` сиротит очереди** (`tenant_store.go`): очереди остаются под старым AccessKey, новый тенант видит 0 очередей, данные зависают в памяти+Redis навсегда (утечка + «пропажа» данных для пользователя).
|
||
|
||
9. **Повсеместные O(total) скан-по-префиксу** под глобальным локом: `countTenantQueues`, `listTenantQueues`, `createTenantQueue` (лимит), `deleteTenant`, gauge-updater. Квадратичная тенденция при росте тенант×очередь.
|
||
|
||
10. **Admin-токен сравнивается небезопасно по времени** — `authHeader != expected` (`admin.go`), в отличие от UI-demo-токена (там `ConstantTimeCompare`). Timing-side-channel.
|
||
|
||
11. **Хардкод demo-кредов и публичного UI-bypass-токена в исходнике** (`seed.go`, `admin.go`). Сами же комментарии требуют удалить это перед продом. Включается env-флагами, но риск реальный.
|
||
|
||
12. **Два расходящихся пути записи/создания.** `admin.createTenantQueue`/`sendMessageToQueue` (`admin.go`) создают очередь и кладут сообщение в обход gosqs-логики: без FIFO-детекта, без проверки размера, без MD5/дедупликации, с другими дефолтами. Поведение UI ≠ поведение SQS API.
|
||
|
||
13. **Биллинг: горутина на каждую операцию + пул 5 коннектов** (`billing.go`). Под нагрузкой — навал горутин, ждущих коннект. Плюс `msg_count` всегда 1, даже для `SendMessageBatch` → неточный учёт.
|
||
|
||
14. **`/metrics` публичен** и отдаёт per-tenant данные (ID тенантов, число очередей/сообщений) — утечка бизнес-метрик наружу.
|
||
|
||
15. **Хрупкое определение протокола** — строго по `Content-Type == application/x-amz-json-1.0` (`router.go`); иные версии JSON-протокола не распознаются.
|
||
|
||
16. **Легаси-остатки GoAWS/SNS**: Topics/Subscriptions в `goaws.yaml`, SNS-поля в моделях, `config.go` кладёт yaml-очереди под голым именем `queue.Name` (без tenant-префикса → такие очереди недостижимы tenant-хендлерами; сейчас спасает `Queues: []`).
|
||
|
||
---
|
||
|
||
# 2. Приоритизированный план доработок
|
||
|
||
## P0 — блокеры продакшна (безопасность и durability)
|
||
|
||
- **P0.1. Убрать вывод SecretKey из email.** Файлы: `tenant_store.go` (`secretKeyFromEmail`/`accessKeyFromEmail`). Генерировать AccessKey/SecretKey через `crypto/rand` при первом провиженинге и хранить в Redis; идентичность по email держать отдельно от материала ключей. Детерминированным оставить максимум tenantID (не секрет).
|
||
- **P0.2. Усилить доверие к JWT.** Файлы: `jwt.go`. Либо проверять подпись по JWKS Nubes, либо брать email/ sub **из ответа валидирующего API** (а не из незаверенного payload), чтобы claim нельзя было подменить. Сузить «принять токен, если любой стенд не вернул 401/403» до явного 2xx от доверенного эндпоинта.
|
||
- **P0.3. Durability для `SendMessage`.** Файлы: `redis.go`, `send_message.go`. Сделать персист сообщения синхронным до ответа 200 (как уже сделано для delete), либо явно задокументировать «best-effort» семантику и снять претензию на SQS-совместимость по надёжности.
|
||
|
||
## P1 — архитектурная зрелость
|
||
|
||
- **P1.1. Разнести блокировку.** Файлы: `globals.go`, все gosqs-хендлеры. Перейти на per-queue mutex (map под коротким RLock + своя блокировка в `Queue`). Снимает главный боттлнек и O(N)-скан под глобальным локом.
|
||
- **P1.2. Индексировать сообщения.** Файлы: `models.go`. Хранить `map[receiptHandle]` / структуры готовности, чтобы receive/delete/visibility были не O(N).
|
||
- **P1.3. Реализовать `MessageRetentionPeriod`.** Файлы: `gosqs.go` (`processQueueTick`). Эвиктить просроченные сообщения по retention в периодической задаче + чистить Redis.
|
||
- **P1.4. Индекс тенанта → его очереди.** Файлы: `tenant_helpers.go`, `admin.go`. Держать `map[accessKey]set(queueName)` — убрать O(total) скан-по-префиксу в create/list/delete/gauge.
|
||
- **P1.5. Убрать demo-bypass из прод-сборки.** Файлы: `seed.go`, `admin.go`. Вынести за build-tag или удалить ветку `authenticateUIDemoToken` и фиксированные креды.
|
||
|
||
## P2 — надёжность, корректность, чистка
|
||
|
||
- **P2.1. BYOC Admin API** (уже TODO в `byoc-credentials.md`). Добавить `access_key/secret_key` в `createTenantRequest` + ветку на `CreateFixed` в `admin.go`. `generateTenantID` экспортировать из tenant-пакета.
|
||
- **P2.2. Чинить сиротство очередей при пересчёте legacy-тенанта** — `tenant_store.go`: либо мигрировать ключи очередей на новый AccessKey, либо запретить смену ключей.
|
||
- **P2.3. Constant-time сравнение admin-токена** — `admin.go`.
|
||
- **P2.4. Единый путь создания/отправки** — admin-хендлеры должны переиспользовать gosqs-логику (FIFO/размер/MD5/дедуп), а не дублировать с иными дефолтами.
|
||
- **P2.5. Биллинг:** батч-вставки через воркер/буфер вместо горутины-на-операцию; корректный `msg_count` для batch (`billing.go`, `router.go` actionHandler).
|
||
- **P2.6. Защитить/урезать `/metrics`** (auth или internal-only), чтобы не светить per-tenant данные.
|
||
- **P2.7. Чистка легаси:** убрать SNS Topics/Subscriptions из `goaws.yaml`, мёртвый путь загрузки yaml-очередей без tenant-префикса в `config.go`, SNS-поля в моделях, `deployments/**`.
|
||
- **P2.8. Redis reconnect/health** — сейчас после обрыва `Client` остаётся не-nil, записи молча падают в лог (`redis.go`).
|
||
|
||
---
|
||
|
||
# 3. Противоречия док ↔ код
|
||
|
||
1. **Идентичность JWT-тенанта.** `tenant-isolation.md` утверждает: тенант создаётся из claim **`sub`**, TenantID = `SHA256(sub) → "sless-{16hex}"`, индекс **`bySub`/`GetBySub`**. Код: идентичность по **`email`**, TenantID = `t-{16hex}` из email, индекс **`byEmail`/`GetByEmail`** (`tenant_store.go`). Доковое описание полностью устарело.
|
||
2. **Генерация SecretKey.** Док: «SecretKey = 32 random bytes». Код: для email-тенантов SecretKey **детерминированный** `SHA256(email+соль)` — не случайный.
|
||
3. **Версии (дрейф).** README deploy-примеры: `v0.1.25`; раздел «История» в README — до `v0.1.14`; `HANDOFF` — прод `v0.1.35`. Три разных значения.
|
||
4. **`NUBES_ENDPOINT`.** README: дефолт `https://deck-api.ngcloud.ru/api/v1`. Код: стенды `https://lk-api-gateway*.ngcloud.ru/api/v1/svc` (`jwt.go`). Разные хосты/пути.
|
||
5. **BYOC статус.** `byoc-credentials.md` честно помечает Admin API/UI как TODO — **совпадает** с кодом (в `createTenantRequest` только `Name`, `MaxQueues`). Это не противоречие, а подтверждение открытого вопроса №1.
|
||
6. **Метрики.** `billing-and-metrics.md`: пример `http://localhost:9090/metrics`, тогда как сервис слушает `4100` и отдаёт `/metrics` на основном порту (`router.go`). Также ссылка на `deployments/k8s/vmservicescrape.yaml`, помеченный в README как легаси «не использовать».
|
||
7. **Дерево в README** перечисляет `auth/ — AWS Signature V4 верификация`, но не упоминает JWT-путь, `billing/`, `metrics/`. Неполно относительно фактической структуры.
|
||
8. **`/metrics → PUBLIC`** в tenant-isolation.md соответствует коду, но сам факт публичности per-tenant метрик в доке не отмечен как риск.
|
||
|
||
---
|
||
|
||
> Заключительная реплика Opus: «Если нужно — могу превратить это в файл-отчёт в
|
||
> doc/thinking/ или углубиться в любой из пунктов (например, расписать
|
||
> PoC-эксплуатацию P0.1/P0.2 или конкретный дизайн per-queue-локов). Код я не
|
||
> трогал, как и требовалось.»
|