ci(deploy): harden production deploy auth and add approval gate - #179
ci(deploy): harden production deploy auth and add approval gate#179cefothe wants to merge 3 commits into
Conversation
Addresses midt-bg#89: tighten Cloudflare deploy credentials and add release governance to the production deploy path. - timeout-minutes: 10 on the deploy job (was the 360-min default), so a wedged wrangler or stalled network fails fast instead of holding a runner for hours; the required-reviewers wait does not count against it - ::add-mask:: the generated LOG_IP_KEY before it is used, belt-and- suspenders against accidental log leakage in a later edit - scripts/provision-environments.sh: codify the production Environment required-reviewers gate + v* deployment tag policy (reproducible and auditable, not a hidden UI click); idempotent, converges to exactly one v* tag policy and validates reviewer refs - deploy.yml: note that workflow_dispatch to production only works from a v* tag ref once the tag policy is provisioned - docs/deploy.md: make the production approval gate mandatory, document token expiry/quarterly rotation, the four-eyes caveat, and record that Cloudflare has no OIDC for Wrangler (so the mitigation is minimal-scope + rotation, not keyless auth)
5f65bae to
6ff9b56
Compare
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Силна стъпка — кодифицираният approval gate + възпроизводимите protection rules са точно каквото иска #89, а скриптът е чист (parameterized jq, валидация на login-ите преди interpolation, отказва празен reviewers[], prevent_self_review). Гейтът е вързан правилно (environment: на deploy job-а).
Едно за provisioning-а — не по кода, а по това кой държи ключовете за прод деплоя: нека required reviewers да са екип (REVIEWER_TEAMS="midt-bg/maintainers"), а не отделен потребител (нищо, че на практика днес това значи само Тодор).
- Реален four-eyes: както сам отбелязваш,
prevent_self_reviewпази само инициатора. Един-единствен reviewer концентрира одобрението на прод деплоя в едно лице — и може да заключи деплоя, ако този човек е и инициаторът. - Прод деплоят е по-добре да минава през група, а не през конкретен личен акаунт.
Скриптът вече го поддържа — само да не се provision-не с примерния единичен reviewer от README-то.
Address review on midt-bg#179: a lone required reviewer concentrates prod-deploy approval in one person and can lock the deploy if that person is also the initiator (prevent_self_review only guards the initiator). Lead the docs + script usage with REVIEWER_TEAMS=midt-bg/maintainers and add an explicit note not to provision production with the single-user example.
|
Благодаря! Точна забележка — обнових документацията и скрипта да водят с екип, не с личен акаунт (
Самата operational стъпка (подаване на екипа при изпълнение на скрипта след merge) остава ръчна по дизайн — не може да се enforce-не в YAML — но документацията вече го прави недвусмислено. |
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Бележката за reviewer-екип е адресирана (commit 5cd41d2) — скриптът и docs вече водят с REVIEWER_TEAMS="midt-bg/maintainers", единичният примерен reviewer отпадна, four-eyes е изричен. Реалното подаване на екипа при provision остава ръчна admin стъпка — коректно, не може да се enforce-не в YAML.
Останалото е чисто: approval gate-ът е вързан правилно (environment: на deploy job-а, v* таг спира на „Review deployments" преди CF auth), скриптът е injection-safe (parameterized jq, валидиран login преди interpolation, отказва празен reviewers[], prevent_self_review), LOG_IP_KEY е маскиран преди употреба, timeout е изричен, а обосновката за дълголетен token вместо OIDC е вярна към юни 2026.
Одобрявам (advisory; финалното одобрение/merge е на maintainer).
|
Прегледах подробно от страна на сигурност, интегритет на данните и OWASP — с фокус върху инжекции, работа с тайни и евентуален зловреден код. Промяната е чиста и подобрява сигурността. Какво е добре:
Дребни, незадължителни:
Благодаря, че го кодифицира вместо да остане като невидимо UI състояние. Заключение: Одобрявам — няма проблеми със сигурността, интегритета на данните или инжекции; двете бележки по-горе са незадължителна шлифовка. |
Address security review on midt-bg#179 (nit 2): $REPO was interpolated into the gh api paths without the regex guard applied to reviewer logins/teams. Add a symmetric owner/name check so every interpolated path segment is validated the same way — rejects extra '/', path traversal, and malformed refs. Low risk (admin-supplied) but keeps the guard surface uniform.
|
Благодаря за подробния security преглед! По двете бележки: 2 ( 1 (гейтът става реален едва след REVIEWER_TEAMS="midt-bg/maintainers" ./scripts/provision-environments.shи проверява Settings → Environments → production. Ще се погрижа това да се случи веднага след merge. Двете бележки са затворени — благодаря за approve-а. |
|
Всички бележки от прегледите са адресирани и затворени:
Няма отворени въпроси по сигурност, интегритет или инжекции. CI е зелено и PR-ът е готов за merge. Благодаря за прегледите! |
|
Прегледах PR #179 задълбочено — с фокус върху сигурност, интегритет на данните, инжекции, работа с тайни и евентуален зловреден/supply-chain код — и локално по скрипта, workflow-а и документацията. Съпоставих и с acceptance критериите на #89. Обхват на промяната
Сигурност и OWASP
Съответствие с #89 Незадължителни наблюдения (не блокират)
Промяната е чиста, тясно скоупната и подобрява сигурността на release веригата. Благодаря, че кодифицира protection rule-ите вместо да останат като невидимо UI състояние. Заключение: Одобрявам — няма проблеми със сигурността, интегритета на данните или инжекции; кодът е OWASP-съобразен, а трите наблюдения по-горе са незадължителна шлифовка. |
|
Ре-проверих новия връх
Approve-ът остава. Единствено: branch-ът е |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: „ci(deploy): harden production deploy auth and add approval gate“
Фаза 0 — Security-Critical сканиране: ✅ ЧИСТО
- Твърдо кодирани тайни: няма.
LOG_IP_KEYсе генерира по време на изпълнение сopenssl rand -hex 32и веднага се маскира — не се комитва нищо. - URL промени: единствената нова връзка е towards GitHub discussion (
workers-sdk#11434) в документацията — безобидна, whitelisted-домейн (github.com). - Зловредни шаблони: няма backdoors, code injection или обфускация.
gh apiизвикванията са конструирани върху валидиран вход. - Зависимости: няма нови пакети.
Заключение: не се блокира, продължавам с ревюто.
Обща оценка
Много добре обмислена и щателно документирана промяна. Целта (issue #89) — да се пренесе управлението на production деплоя от невидими UI кликове към възпроизводим, одитируем скрипт + approval gate — е постигната. Валидацията на вход в provision-environments.sh (path-traversal guard за REPO, потребители и екипи; проверка за брой reviewer-и ≤6; отказ при 0 разрешени reviewer-а) е образцова. Логиката за конвергенция на tag policy е идемпотентна и обработва пагинация, а LIST провалът е фатален — правилно решение.
Силни страни
echo "::add-mask::$key"е поставено на реда непосредствено след генерирането — коректно (маскирането не е ретроактивно в рамките на ред).timeout-minutes: 10вместо 360-мин default — разумен fail-fast.- Документацията ясно обяснява ограниченията на
prevent_self_reviewи защо е нужен екип/≥2 reviewer-а за реален four-eyes контрол — точно описание на поведението на GitHub (изисква се само 1 одобрение, но инициаторът не може да е одобряващият). - Обяснението „защо все още long-lived token, а не OIDC“ е добре обосновано.
Забележки (не-блокиращи)
- Прозорец на заключване при прекъсване (
provision-environments.sh). PUT-ът задаваcustom_branch_policies: true, а v* tag policy се създава чак с последващия POST. Ако скриптът прекъсне между двете стъпки,productionостава с включени custom policies, но без нито една валидна policy → GitHub блокира всички деплои към production, докато скриптът не бъде изпълнен отново. Коментарът „просто изпълнете отново“ смекчава, но си струва изрична бележка в лога/документацията, че до повторното изпълнение прод деплоите са спрени. - Reviewer-ите трябва да имат write/admin достъп до repo-то, иначе PUT-ът връща 422. Скриптът резолвва id-та, но не проверява правата — препоръчвам по-ясно съобщение при 422 или бележка в prerequisites.
- Остатъчен риск при
set -x. Коментарът околоadd-maskпази бъдещи редакции, но ако някой добавиset -xв стъпката, xtrace на реда за генериране на ключа ще изтече стойността преди маскирането да влезе в сила. Струва си изрична забрана заset -x/ACTIONS_STEP_DEBUGв тази стъпка.
Оценка спрямо quality gates
- Security (agent-level): 1.0/1.0 — валидация на вход, липса на injection, коректно маскиране.
- Code Quality: 2.0/2.0 — последователен стил, ясни коментари, реуз на shell идиоми.
- Documentation: 2.0/2.0 — изчерпателна, точна, с примери и предупреждения.
- Performance: 2.0/2.0 — без регресии; timeout подобрява поведението.
- Tests: 1.0/3.0 — ⚠ Няма автоматизирани проверки (напр.
shellcheckв CI, bats тестове за валидациите на вход). За CI/infra скрипт е разбираемо, но строгите gates изискват покритие; поне добавяне наshellcheckкъм pipeline-а би било лесна печалба.
Композитна оценка: ~9.0/10 — сваля я единствено липсата на автоматизирано валидиране/тестове за новия скрипт.
Препоръка: COMMENT
Промяната е солидна, сигурна и готова за merge по същество. Не давам пълно APPROVE само защото gate-овете изискват автоматизирано тестово покритие/shellcheck за новия provision-environments.sh и защото прозорецът на заключване (забележка 1) заслужава изрична документация. Нито едно от тези не е блокиращо за функционалността.
| # runs in single-digit minutes, so a job past 10 min is hung (stalled network I/O, wedged | ||
| # wrangler) — fail fast rather than burn a 6-hour slot. The required-reviewers wait on | ||
| # `production` does NOT count against this; the timer starts only once the job hits a runner. | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
👍 Разумен fail-fast лимит вместо 360-мин default. Коментарът коректно уточнява, че изчакването за одобрение на production не влиза в таймера (той тръгва чак щом job-ът хване runner). Единствен риск: ако легитимен деплой някога стане бавен (напр. голям build), 10 мин може да го убие — стойността е разумна, но следете дали не е твърде агресивна при растеж на проекта.
| # Register the value as a secret with the runner BEFORE it is used anywhere else, so an | ||
| # accidental echo / set -x trace in a later edit can never leak it. Masking is not | ||
| # retroactive within a line, hence add-mask on the line right after generation. | ||
| echo "::add-mask::$key" |
There was a problem hiding this comment.
Коректно: add-mask е на реда непосредствено след генерирането, така че стойността се регистрира като secret преди първата ѝ употреба. Остатъчен риск: ако някой добави set -x (или включи ACTIONS_STEP_DEBUG) в тази стъпка, xtrace на реда key="$(openssl rand -hex 32)" ще изведе ключа преди маскирането да влезе в сила. Обмислете изрична бележка/забрана за xtrace в тази стъпка, за да е защитата пълна.
| 2. **Deployment tag policy `v*`** — GitHub сам отказва прод деплой, ако ref-ът не е release таг, дори | ||
| ако `detect` логиката в [deploy.yml](../.github/workflows/deploy.yml) някога сгреши (defense in | ||
| depth). Job-ът има и изричен `timeout-minutes: 10` вместо 360-мин default. | ||
|
|
There was a problem hiding this comment.
Обяснението на four-eyes е точно. Едно уточнение си струва да се добави: GitHub изисква само едно одобрение, дори когато са изброени няколко reviewer-а — така че REVIEWER_USERS="alice,bob" НЕ изисква и двамата да одобрят, а гарантира four-eyes само в комбинация с prevent_self_review (инициаторът не може да одобри). За екип това означава, че екипът трябва да има ≥2 членове с write достъп — иначе gate-ът може да се заключи. Текстът го подсказва, но изричното „изисква се 1 одобрение“ би премахнало двусмислие.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
add-mask placement-ът е коректен (::add-mask::$key веднага след генерирането; ключът отива към wrangler по stdin, не в args — set -x не може да го изтече), а provision guard-овете (regex валидация на repo/logins/slugs, отказ да PUT-не празни reviewers) са издържани.
Дребно (defense-in-depth): detect job-ът интерполира ${{ inputs.environment }} / ${{ github.event_name }} / ${{ github.ref }} директно в run:. На практика не инжектират (choice input-ите са server-validated, ref-овете без кавички), но по-чисто е през env: + $VAR. И: production protection rule-ът не съществува докато provision-environments.sh не се пусне след merge — минимизирай прозореца merge→provision, че v* tag дотогава деплойва без approval (PR-ът го документира).
3 participants
Резюме
Подсилва production deploy пътя с гейт за човешко одобрение и затяга хигиената на CI job-овете. Адресира #89.
v*таг сам по себе си вече не пуска прод автоматично. Deploy job-ът спира на „Review deployments“ преди да стигне runner (т.е. преди Cloudflare автентикацията изобщо да се изпълни). Кодифицирано в нов идемпотентен скрипт —scripts/provision-environments.sh— така че protection rule-ите на средата (required reviewers,prevent_self_review,v*deployment tag policy) са възпроизводими и одитируеми вместо невидими кликове в UI-я.LOG_IP_KEYсе регистрира с::add-mask::веднага след генерирането му, преди първата му употреба, така че по-късен случаенset -x/echo не може да го изтече.deployjob-ът вече се ограничава до 10 мин вместо 360-мин default на GitHub, така че заклещен wrangler или блокирала мрежа се провалят бързо. Изчакването за одобрение не влиза в таймера (той тръгва чак щом job-ът хване runner).docs/deploy.mdописва approval gate-а, изтичането на token-а + тримесечната ротация, four-eyes уговорката, и защо все още се ползват дълголетни token-и (а не OIDC) — Cloudflare няма OIDC/workload-identity federation за Wrangler към юни 2026 (workers-sdk#11434).Файлове
scripts/provision-environments.shproductionrequired reviewers +v*tag policy.github/workflows/deploy.ymltimeout-minutes: 10,::add-mask::заLOG_IP_KEY, бележка заworkflow_dispatchtag policydocs/deploy.mdКакво трябва да се направи ръчно
Protection rule-ите на средата живеят в repo Settings, не в YAML — затова не се прилагат автоматично при merge. След merge изпълнете провизиониращия скрипт веднъж с admin права:
Изисквания:
ghCLI, автентикиран с admin права върху repo-то, иjq. Проверка след това: Settings → Environments →production.Acceptance (#89)
prevent_self_review).timeout-minutes: 10).