Очередность валидации при запросах клиента #39
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Наблюдение из #37:
Надо подправить реализацию, чтобы соответствовала документации - сначала проверяем подпись, потом hash.
@owner привет! Разобралась с #39 — и тут всплыла деталь, из-за которой хочу свериться с тобой до того, как трогать код проверки подписи (область чувствительная, auth +
Kind/Security).Что подтвердила в коде
Порядок проверок в gateway собран как цепочка декораторов в
gateway/internal/grpcapi/server.go:129-148. Читается изнутри наружу, и сейчас он такой:То есть да, hash проверяется раньше подписи — ровно как ты и заметил в наблюдении из #37. Декораторы независимы (оба только читают из контекста envelope и сессию, ничего друг другу не кладут), так что технически поменять их местами — это перестановка двух строк.
Но всплыло другое: расходится не «дока против кода», а одна дока против трёх источников
docs/ARCHITECTURE.md§15 (строки 762-763): сначала подпись, потом hash.gateway/README.md(строки 582-583): сначала hash, потом подпись.gateway/docs/flows.md(строки 41-42): сначала hash, потом подпись.server.go): сначала hash, потом подпись.Получается, код + README + flows.md согласованы между собой (hash раньше), и только ARCHITECTURE.md §15 говорит обратное. В issue это выглядело как «доку приняли за истину, код отстал», но по факту «отстала» как раз ARCHITECTURE.md — против неё три согласованных источника. Поэтому не хочу молча переписывать auth-цепочку: давай ты осознанно выберешь, что тут истина.
Вариант A — двигаем код под ARCHITECTURE.md (то, что ты просил в issue)
Меняю порядок на «подпись → hash» в коде и заодно правлю README + flows.md, чтобы они тоже говорили «подпись → hash» (иначе они начнут противоречить уже исправленному коду).
payload_hash(signature.go:66), так что hash имеет смысл проверять уже после того, как подпись подтвердила его подлинность. ARCHITECTURE.md — наш заявленный источник истины по модели безопасности.Вариант B — двигаем доку под код
Код не трогаю вообще, просто правлю ARCHITECTURE.md §15 на «hash → подпись», чтобы она совпала с кодом + README + flows.
Важный побочный эффект варианта A — семантика ошибки
Поскольку подпись накрывает
payload_hash, подделка поляpayload_hashломает и подпись. Значит при порядке «подпись первой» на таком запросе сработает сначала проверка подписи →UNAUTHENTICATED "invalid request signature", а неINVALID_ARGUMENT "payload_hash does not match payload_bytes", как сейчас. На безопасность это не влияет (как ты и сказал — оба отказа происходят до обработки payload), но это наблюдаемое изменение кода и текста ошибки для подделанных запросов.Чтобы сохранить осмысленное покрытие hash-гейта, перепишу 4 unit-теста в
payload_hash_integration_test.goтак, чтобы подпись оставалась валидной, а hash-проверка всё равно падала (для mismatch — валидная подпись над hash H, ноpayload_bytes, чей реальный хэш ≠ H; для invalid-length — подписываем заведомо некорректный по длине hash). Интеграционныйintegration/gateway_edge_test.goэто уже переживёт — он допускает оба кода ошибки.Моя рекомендация — вариант A
Порядок «подпись → hash» объективно правильнее, и ARCHITECTURE.md как security-источник истины должен задавать тон, а README и flows надо подтянуть к нему. Но раз это auth и
Kind/Security, без твоего «да» не двигаюсь.Что выбираешь — A или B?
Working log (issue-agent) — click to expand
State: in-review
Updated: 2026-05-24T00:47Z
Branch / PR:
feature/issue-39-verification-order→ PR #41 (intodevelopment), open, mergeable, CI green.Confirmed:
server.go:129-148are independent single-delegate wrappers; swap is mechanical. Signature is computed overpayload_hash(signature.go:60-67), so signature-first is cryptographically sound.server.go:66+session.go:56comments stay true. No further doc edits needed.Changes shipped:
server.go:newSignatureVerifyingServicenow wrapsnewPayloadHashVerifyingService(signature first).payload_hash.go/signature.go: decorator doc comments updated.gateway/README.md(steps 5↔6) +gateway/docs/flows.md(mermaid): signature→hash.payload_hash_integration_test.go: 4 tests re-sign over the tampered hash so they still trip the hash gate.payload_hash+ signature over original hash → UNAUTHENTICATED instead of INVALID_ARGUMENT (security unchanged). IntegrationTestGatewayEdge_PayloadHashMismatchunaffected (signs over the overridden hash, accepts both codes).Checks: Local gofmt/vet/
go test ./gateway/.../integration-vet all green. CI on PR #41: go-unit ✓, integration ✓ (runs 331 + 332; branch-push run 330 ✓).Open question: —
Decisions: Gated on the auth/Kind/Security blast radius and waited for the owner's A/B pick before touching code. Used
Refs #39(notCloses) so the merge does not auto-close — the owner closes the issue.Next: Owner-driven — review and merge PR #41 (human merge only; never merge here). If the owner comments with changes, resume: re-read thread, set
claude/working, adjust on the branch, re-push, re-watch CI. If the owner says to close, close #39 and clean up the branch/PR.Конечно, вариант А :) Спасибо!
developer referenced this issue2026-05-24 00:42:37 +00:00
@owner готово, вариант А уехал в PR #41 :)
Что сделала
Поменяла порядок в auth-цепочке так, чтобы подпись проверялась раньше
payload_hash— теперь код, README, flows.md и ARCHITECTURE.md §15 говорят одно и то же (подпись → hash).gateway/internal/grpcapi/server.go— поменяла местами два декоратора:newSignatureVerifyingServiceтеперь оборачиваетnewPayloadHashVerifyingService, то есть подпись идёт первой. Это буквально перестановка двух строк — декораторы независимы (оба лишь читают envelope/сессию из контекста, ничего друг другу не кладут).payload_hash.go/signature.go— поправила doc-комментарии под новый порядок.gateway/README.md(шаги 5↔6) иgateway/docs/flows.md(mermaid) — подтянула к signature→hash.payload_hash_integration_test.go— переписала 4 unit-теста: теперь они переподписывают запрос поверх испорченного hash, чтобы подпись оставалась валидной и срабатывал именно hash-гейт (иначе после перестановки первым падал бы signature-гейт с UNAUTHENTICATED).Побочный эффект (как и обсуждали)
Запрос с подделанным
payload_hash, но подписью над исходным hash, теперь отлетает на проверке подписи →UNAUTHENTICATED "invalid request signature", а неINVALID_ARGUMENT. На безопасность это не влияет — оба отказа происходят до обработки payload.ARCHITECTURE.md§15 трогать не пришлось — он уже говорил signature→hash, я просто привела к нему три остальных источника.FUNCTIONAL.mdпорядок не фиксирует, так что и русское зеркало не тронуто.Проверки (всё зелёное)
gofmt,go vet ./gateway/internal/grpcapi/,go test -count=1 ./gateway/...,go vet ./integration/....Мержить не буду — PR ждёт твоего ревью. Issue оставляю открытой, закроешь сама, когда всё устроит. Спасибо!