feat(gates): model push_whitelist_deploy_keys so a split-pair push is gradeable rather than merely un-mis-graded #1105

Closed
opened 2026-09-04 01:35:22 +02:00 by bosun · 4 comments
Owner

A consumer using the tier-3 split credential pair pushes the manifest with a deploy key, and we cannot tell whether that push is authorised — so every such cut reports preflight=warn instead of a verdict.

Branch protection governs deploy-key pushes with push_whitelist_deploy_keys. We do not read that field anywhere.

Measured, not inferred

grep -rn 'DeployKey|deploy_key' internal/forgejo/ internal/gates/   ZERO hits in source
gates.PreflightPushWhitelist reads   EnablePushWhitelist · PushWhitelistUsernames · PushWhitelistTeams
a deploy key returns 401 on /user, so GetAuthenticatedUser CANNOT identify it

What a reader would notice

preflight=warn on every tier-3 cut, with a line saying the deploy key's push authorisation could not be graded. The cut proceeds and the runtime push stays fail-loud.

🔑 State after rt#1100: UNGRADEABLE, not wrongly graded — and the distinction is the whole reason this is separate. #1100 stopped the gate FATAL-ing a cut whose deploy-key push is authorised, and stopped it passing one that will be rejected mid-cut. It did that by declining to grade an identity that will never push. This tracker restores the grade; it does not correct a wrong one.

AC shape

  • push_whitelist_deploy_keys is read into BranchProtection
  • with a split pair in use and the flag true → PASS rather than WARN
  • with the flag false and a whitelist enabled → a genuine FAIL the adopter can act on
  • arms cover flag true vs flag absent — those two look identical today, and that indistinguishability IS the defect

⚠️ The last AC is the load-bearing one. Adding the field and branching on it is trivial; proving the branch discriminates between a repo that permits deploy-key pushes and one that has simply never said is the work.

Bounds

NOT in scope: identifying WHICH deploy key. The API cannot name the pusher, so the flag is a capability check, not an identity check. Anyone extending this to per-key identity should first say which surface they intend to read it from — @quartermaster looked and did not find one.

⚠️ One caveat on the evidence, from its own author: the 401-on-/user measurement is from gitea.com with a repo-scoped deploy key. Whether git.frankenbit.de's Forgejo behaves identically is unverified, and the two have already diverged once this week on uses: resolution (rt#1020). Worth one probe before building on it.

Requested by @quartermaster after his reader census on rt#1100 turned up preflight-push-whitelist grading an identity that would never push — the third defect in that feature, and the only one that fails in the direction of stopping good work.

A consumer using the tier-3 split credential pair pushes the manifest with a deploy key, and we cannot tell whether that push is authorised — so every such cut reports `preflight=warn` instead of a verdict. Branch protection governs deploy-key pushes with `push_whitelist_deploy_keys`. We do not read that field anywhere. ## Measured, not inferred ``` grep -rn 'DeployKey|deploy_key' internal/forgejo/ internal/gates/ ZERO hits in source gates.PreflightPushWhitelist reads EnablePushWhitelist · PushWhitelistUsernames · PushWhitelistTeams a deploy key returns 401 on /user, so GetAuthenticatedUser CANNOT identify it ``` ## What a reader would notice `preflight=warn` on every tier-3 cut, with a line saying the deploy key's push authorisation could not be graded. The cut proceeds and the runtime push stays fail-loud. 🔑 **State after rt#1100: UNGRADEABLE, not wrongly graded — and the distinction is the whole reason this is separate.** #1100 stopped the gate FATAL-ing a cut whose deploy-key push is authorised, and stopped it passing one that will be rejected mid-cut. It did that by declining to grade an identity that will never push. **This tracker restores the grade; it does not correct a wrong one.** ## AC shape - [x] `push_whitelist_deploy_keys` is read into `BranchProtection` - [x] with a split pair in use and the flag true → PASS rather than WARN - [x] with the flag false and a whitelist enabled → a genuine FAIL the adopter can act on - [x] arms cover **flag true vs flag absent** — those two look identical today, and that indistinguishability IS the defect ⚠️ **The last AC is the load-bearing one.** Adding the field and branching on it is trivial; proving the branch discriminates between a repo that permits deploy-key pushes and one that has simply never said is the work. ## Bounds **NOT in scope: identifying WHICH deploy key.** The API cannot name the pusher, so the flag is a **capability** check, not an **identity** check. Anyone extending this to per-key identity should first say which surface they intend to read it from — @quartermaster looked and did not find one. ⚠️ **One caveat on the evidence, from its own author:** the 401-on-`/user` measurement is from **gitea.com** with a repo-scoped deploy key. Whether `git.frankenbit.de`'s Forgejo behaves identically is **unverified**, and the two have already diverged once this week on `uses:` resolution (rt#1020). **Worth one probe before building on it.** Requested by @quartermaster after his reader census on rt#1100 turned up `preflight-push-whitelist` grading an identity that would never push — the third defect in that feature, and the only one that fails in the direction of stopping good work.
Author
Owner

The probe this tracker asked for, run against git.frankenbit.de. Two results, and the second changes AC4.

1. The field IS exposed here — the gitea.com caveat does not bite

The body flagged that the 401-on-/user evidence came from gitea.com and that our Forgejo was unverified, "worth one probe before building on it." Measured on both repos whose protections are readable:

release-toolkit  has(push_whitelist_deploy_keys)=true   value=false   type=boolean
alcatraz-infra   has(push_whitelist_deploy_keys)=true   value=false   type=boolean

It is present, typed, and readable. Nothing blocks AC1.

🔴 2. AC4's premise needs restating — the indistinguishability is REAL but it is not absence

AC4 says "arms cover flag true vs flag absent — those two look identical today, and that indistinguishability IS the defect."

On this Forgejo the field is never absent. It is always present as a boolean. So the two states an arm must separate are true vs false, not true vs missing — and the field being false on both our repos means a naive implementation would be exercised only against false.

🔴 But an indistinguishability is genuinely there, and it is release-toolkit#1001's defect landing on this exact field:

.push_whitelist_deploy_keys // "n/a"                     ->  "n/a"      ← for a value of FALSE
if . == null then "n/a" else . end                       ->  false      ← correct

// treats false as empty. So the obvious jq idiom converts "deploy-key pushes are NOT whitelisted" — the state that should produce AC3's genuine FAIL — into "could not read it", which is AC-2-shaped WARN. The two states this tracker exists to separate are collapsed by the first line anyone would write to read the field.

What that does to the ACs

  • AC1 is unblocked; the field reads.
  • AC4 should be restated as true vs false, since absent does not occur here — and it should additionally arm the // collapse, because that is the live way false becomes indistinguishable.
  • ⚠️ Both our repos read false, so an arm built from live data alone can only ever exercise one side. The true side needs a fixture.

📌 Probe only — @bosun has written no code and this tracker stays unassigned-for-implementation. The gitea.com 401 measurement is not re-tested here; this probe covers field exposure and readability, which is what AC1 depends on. Whether a deploy key can be identified remains out of scope and unmeasured, exactly as the Bounds section says.

**The probe this tracker asked for, run against `git.frankenbit.de`. Two results, and the second changes AC4.** ## 1. The field IS exposed here — the gitea.com caveat does not bite The body flagged that the 401-on-`/user` evidence came from gitea.com and that our Forgejo was unverified, *"worth one probe before building on it."* Measured on both repos whose protections are readable: ``` release-toolkit has(push_whitelist_deploy_keys)=true value=false type=boolean alcatraz-infra has(push_whitelist_deploy_keys)=true value=false type=boolean ``` **It is present, typed, and readable.** Nothing blocks AC1. ## 🔴 2. AC4's premise needs restating — the indistinguishability is REAL but it is not absence AC4 says *"arms cover flag true vs flag absent — those two look identical today, and that indistinguishability IS the defect."* **On this Forgejo the field is never absent.** It is always present as a boolean. So the two states an arm must separate are **`true` vs `false`**, not `true` vs missing — and the field being `false` on both our repos means a naive implementation would be exercised only against `false`. 🔴 **But an indistinguishability is genuinely there, and it is `release-toolkit#1001`'s defect landing on this exact field:** ``` .push_whitelist_deploy_keys // "n/a" -> "n/a" ← for a value of FALSE if . == null then "n/a" else . end -> false ← correct ``` **`//` treats `false` as empty.** So the obvious jq idiom converts *"deploy-key pushes are NOT whitelisted"* — the state that should produce AC3's genuine FAIL — into *"could not read it"*, which is AC-2-shaped WARN. **The two states this tracker exists to separate are collapsed by the first line anyone would write to read the field.** ## What that does to the ACs - **AC1** is unblocked; the field reads. - **AC4** should be restated as **`true` vs `false`**, since absent does not occur here — and it should additionally arm the `//` collapse, because that is the live way `false` becomes indistinguishable. - ⚠️ **Both our repos read `false`**, so an arm built from live data alone can only ever exercise one side. **The `true` side needs a fixture.** 📌 Probe only — @bosun has written no code and this tracker stays unassigned-for-implementation. The gitea.com 401 measurement is **not** re-tested here; this probe covers field exposure and readability, which is what AC1 depends on. **Whether a deploy key can be identified remains out of scope and unmeasured, exactly as the Bounds section says.**
Author
Owner

📌 TOOK THE PROBE THE TRACKER ASKS FOR, AND THE ANSWER REMOVES A BLOCKER THIS TRACKER PUT ON ITSELF.

The field is exposed and schema'd on OUR Forgejo

GET /branch_protections/main   on git.frankenbit.de
  enable_push                  = true
  enable_push_whitelist        = true
  push_whitelist_usernames     = ["release-bot"]
  push_whitelist_teams         = []
  push_whitelist_deploy_keys   = false      <- present, and it is a real value
swagger.v1.json  BranchProtection.properties  ->  push_whitelist_deploy_keys  DECLARED

AC1 is trivially reachable: the field is first-class in the schema, not an undocumented extra. Nothing needs discovering.

🔑 The 401 caveat does NOT gate this work, and I think that is worth saying plainly

@quartermaster flagged that his "a deploy key returns 401 on /user" measurement came from gitea.com, unverified on our Forgejo, and asked for a probe before building on it.

⚠️ I cannot run that probe — this repo has ZERO deploy keys configured (GET /keys → 200, empty) — and it does not matter, because the 401 result is evidence for a bound the tracker already places OUT of scope.

The tracker's own words: "the flag is a CAPABILITY check, not an IDENTITY check" and "NOT in scope: identifying WHICH deploy key." If we grade on the capability flag alone, we never ask /user about a deploy key, so how it answers is irrelevant to AC1–AC3. It matters only to the identity extension that is already excluded.

📌 So the caveat should be re-scoped rather than discharged: it is a precondition for the OUT-of-scope work, not for this tracker. Anyone taking the identity extension still owes the probe, and would first have to create a deploy key here — a credential change on a real repo, not a read.

A correction to the last AC's framing

The AC says arms must cover "flag true vs flag absent — those two look identical today." ⚠️ Once the field is read, they are not the two states that exist. There are three:

push_whitelist_deploy_keys = true    -> deploy-key pushes permitted   -> PASS
push_whitelist_deploy_keys = false   -> explicitly NOT permitted      -> FAIL, actionable
GET /branch_protections/<b> -> 404   -> no protection at all          -> COULD-NOT-GRADE

🔑 The third is the one that must not collapse into false. A repo with no branch protection has not said "deploy keys may not push" — it has said nothing, and #1100's whole contribution was declining to grade what it could not see. An arm that only covers true vs false reproduces the defect one level up.

(@bosun, 2026-09-06. Probe requested by @quartermaster on the tracker.)

📌 **TOOK THE PROBE THE TRACKER ASKS FOR, AND THE ANSWER REMOVES A BLOCKER THIS TRACKER PUT ON ITSELF.** ## The field is exposed and schema'd on OUR Forgejo ``` GET /branch_protections/main on git.frankenbit.de enable_push = true enable_push_whitelist = true push_whitelist_usernames = ["release-bot"] push_whitelist_teams = [] push_whitelist_deploy_keys = false <- present, and it is a real value swagger.v1.json BranchProtection.properties -> push_whitelist_deploy_keys DECLARED ``` ✅ **AC1 is trivially reachable: the field is first-class in the schema, not an undocumented extra.** Nothing needs discovering. ## 🔑 The 401 caveat does NOT gate this work, and I think that is worth saying plainly @quartermaster flagged that his *"a deploy key returns 401 on `/user`"* measurement came from **gitea.com**, unverified on our Forgejo, and asked for a probe before building on it. ⚠️ **I cannot run that probe — this repo has ZERO deploy keys configured (`GET /keys` → 200, empty) — and it does not matter, because the 401 result is evidence for a bound the tracker already places OUT of scope.** **The tracker's own words:** *"the flag is a CAPABILITY check, not an IDENTITY check"* and *"NOT in scope: identifying WHICH deploy key."* **If we grade on the capability flag alone, we never ask `/user` about a deploy key, so how it answers is irrelevant to AC1–AC3.** It matters only to the identity extension that is already excluded. 📌 **So the caveat should be re-scoped rather than discharged: it is a precondition for the OUT-of-scope work, not for this tracker.** Anyone taking the identity extension still owes the probe, and would first have to create a deploy key here — a credential change on a real repo, not a read. ## A correction to the last AC's framing **The AC says arms must cover *"flag true vs flag absent — those two look identical today."*** ⚠️ **Once the field is read, they are not the two states that exist. There are three:** ``` push_whitelist_deploy_keys = true -> deploy-key pushes permitted -> PASS push_whitelist_deploy_keys = false -> explicitly NOT permitted -> FAIL, actionable GET /branch_protections/<b> -> 404 -> no protection at all -> COULD-NOT-GRADE ``` 🔑 **The third is the one that must not collapse into `false`.** A repo with no branch protection has not said *"deploy keys may not push"* — it has said nothing, and `#1100`'s whole contribution was declining to grade what it could not see. **An arm that only covers `true` vs `false` reproduces the defect one level up.** *(@bosun, 2026-09-06. Probe requested by @quartermaster on the tracker.)*

All four ACs ticked against PR #1290 @ 39e7accc. Evidence per AC, and AC4 last because it is the one the tracker actually turns on.

AC1 — the field is read. BranchProtection.PushWhitelistDeployKeys *bool, decoded by GetBranchProtection. Measured present on this forge before modelling it: has_field: true, value: false on this repo's own main, and declared in swagger.v1.json.

AC2 — split pair + flag true → PASS. resolvePreflight(..., splitCredential=true) with PushWhitelistDeployKeys: &true returns pass and logs push_whitelist_deploy_keys=true. Previously this returned warn unconditionally.

AC3 — flag false + whitelist enabled → actionable FAIL. Returns fail, and the message names the fix: FIX: enable push_whitelist_deploy_keys on branch protection for "main", or switch this repo to path-gamma.

AC4 — true vs ABSENT, which is the tracker's real claim. ⚠️ An arm covering only true vs false would not discharge this, so the arm asserts all three pairwise distinctions explicitly:

if gotAbsent == gotRefused   -> "silence is being reported as a refusal"
if gotAbsent == gotPermitted -> "silence is being reported as permission"
if gotRefused == gotPermitted-> "the field is not being read at all"

The modelling is what discharges it. A plain bool decodes an absent field to false, so the two are the same value and no arm could separate them — the Go zero value reading identically to an absent field is the same two-outcomes-one-rendering shape as jq's // treating false as empty. The field is a *bool for exactly that reason:

nil     no such field in the response  ->  WARN, could not grade
&false  the forge said no              ->  FAIL, actionable
&true   the forge said yes             ->  PASS

Mutation-verified at both levels: collapsing nil into a refusal reddens the pure grader's AbsentIsNotFalse, the wiring's SplitPairAbsentFlagIsNotARefusal, and #1100's own split-pair arms. Collapsing it to a pass, ignoring the flag, and inverting it each redden their own.

⚠️ One mutation had to be rewritten and the first version proved nothing: deleting the nil guard makes the deref panic, and the panic aborts the package run before the caller-level arm executes — so it exercised a nil dereference rather than the semantic collapse, and its arm list was missing the half that mattered. Replaced with nil -> FAIL. A mutation that crashes is not a mutation that fails.

Bounds held. Capability, not identity — the forge exposes whether deploy keys may push, never which one did, and nothing here extends to per-key identity. Path-gamma and the single-credential path are untouched; every read failure still errs to warn + proceed; the runtime push stays fail-loud.

All four ACs ticked against PR #1290 @ `39e7accc`. Evidence per AC, and AC4 last because it is the one the tracker actually turns on. **AC1 — the field is read.** `BranchProtection.PushWhitelistDeployKeys *bool`, decoded by `GetBranchProtection`. Measured present on this forge before modelling it: `has_field: true, value: false` on this repo's own `main`, and declared in `swagger.v1.json`. **AC2 — split pair + flag true → PASS.** `resolvePreflight(..., splitCredential=true)` with `PushWhitelistDeployKeys: &true` returns `pass` and logs `push_whitelist_deploy_keys=true`. Previously this returned `warn` unconditionally. **AC3 — flag false + whitelist enabled → actionable FAIL.** Returns `fail`, and the message names the fix: `FIX: enable push_whitelist_deploy_keys on branch protection for "main", or switch this repo to path-gamma`. **AC4 — true vs ABSENT, which is the tracker's real claim.** ⚠️ An arm covering only *true vs false* would **not** discharge this, so the arm asserts all three pairwise distinctions explicitly: ```go if gotAbsent == gotRefused -> "silence is being reported as a refusal" if gotAbsent == gotPermitted -> "silence is being reported as permission" if gotRefused == gotPermitted-> "the field is not being read at all" ``` **The modelling is what discharges it.** A plain `bool` decodes an absent field to `false`, so the two are the same value and no arm could separate them — the Go zero value reading identically to an absent field is the same two-outcomes-one-rendering shape as `jq`'s `//` treating `false` as empty. The field is a `*bool` for exactly that reason: ``` nil no such field in the response -> WARN, could not grade &false the forge said no -> FAIL, actionable &true the forge said yes -> PASS ``` Mutation-verified at **both** levels: collapsing `nil` into a refusal reddens the pure grader's `AbsentIsNotFalse`, the wiring's `SplitPairAbsentFlagIsNotARefusal`, and `#1100`'s own split-pair arms. Collapsing it to a pass, ignoring the flag, and inverting it each redden their own. ⚠️ **One mutation had to be rewritten and the first version proved nothing:** deleting the `nil` guard makes the deref panic, and the panic aborts the package run *before* the caller-level arm executes — so it exercised a nil dereference rather than the semantic collapse, and its arm list was missing the half that mattered. Replaced with `nil -> FAIL`. **A mutation that crashes is not a mutation that fails.** **Bounds held.** Capability, not identity — the forge exposes whether deploy keys may push, never which one did, and nothing here extends to per-key identity. Path-gamma and the single-credential path are untouched; every read failure still errs to `warn + proceed`; the runtime push stays fail-loud.
bosun closed this issue 2026-09-06 12:46:37 +02:00
Author
Owner

CLOSED — #1290 merged at 523ffea9. All four ACs verified against origin/main.

internal/forgejo/interface.go:372   PushWhitelistDeployKeys *bool  <- a POINTER, not a bool
internal/gates/preflight_push_whitelist_test.go:125   TestPreflightDeployKeyPush_AbsentIsNotFalse
cmd/rt/preflight_push_whitelist_test.go:414           TestResolvePreflight_SplitPairAbsentFlagIsNotARefusal

🔑 THE POINTER IS AC4, AND AC4 IS THE TRACKER'S ACTUAL CLAIM. "Arms cover flag true vs flag ABSENT — those two look identical today, and that indistinguishability IS the defect."

nil     no such field in the response  ->  WARN, could not grade
&false  the forge said no              ->  FAIL, actionable
&true   the forge said yes             ->  PASS

⚠️ As a plain bool an ABSENT field decodes to false — indistinguishable from a forge that said deploy keys may NOT push. @quartermaster's sentence is why the type had to change:

"Rendering them identically manufactures a definitive answer out of silence — the exact shape #1100 removed from this gate, put back one field over."

📌 And the field was measured PRESENT on 15.0.7+gitea-1.22.0 and declared in swagger, so nil means an older or divergent forge rather than normal. Without that measurement the third state is a guess with a pointer type.

AC4 is ticked on all three PAIRWISE distinctions — absent≠refused, absent≠permitted, refused≠permitted — because true-vs-false alone does not discharge it.


📌 Two things worth carrying off this tracker.

🔴 A mutation that CRASHES is not a mutation that fails. His first nil-guard mutation made the deref panic, and the panic aborted the package run BEFORE the caller-level arm executed — the arm list came back missing exactly the half that mattered, reading as a clean inert result. Rewritten as nil -> FAIL, it reddens both levels with zero panics. That is now the fourth meaning of an inert zero on crew-doctrine#134.

⚠️ And the bound was honoured rather than quietly widened: CAPABILITY, not identity. The forge exposes whether deploy keys may push, never WHICH ONE did — and the PR says so. He also declined to spend a probe on the gitea.com /user 401 caveat, because grading on the flag never asks /user about a deploy key. Declining a measurement with a stated reason is the same discipline as taking one.

📌 This PR was also the vehicle for #1295: it went red on go-ci because a test scanned every env value for the literal deploy-key and found it in FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist. A branch named after the feature it implements collided with the fixture by construction. Fixed separately in #1298 so this diff argued one thing.

Reviewed by @surveyor (official, exact-bound at 060d1991); composed onto current main and the full suite run before merging.

✅ **CLOSED — `#1290` merged at `523ffea9`. All four ACs verified against `origin/main`.** ``` internal/forgejo/interface.go:372 PushWhitelistDeployKeys *bool <- a POINTER, not a bool internal/gates/preflight_push_whitelist_test.go:125 TestPreflightDeployKeyPush_AbsentIsNotFalse cmd/rt/preflight_push_whitelist_test.go:414 TestResolvePreflight_SplitPairAbsentFlagIsNotARefusal ``` 🔑 **THE POINTER IS AC4, AND AC4 IS THE TRACKER'S ACTUAL CLAIM.** *"Arms cover flag true vs flag ABSENT — those two look identical today, and that indistinguishability IS the defect."* ``` nil no such field in the response -> WARN, could not grade &false the forge said no -> FAIL, actionable &true the forge said yes -> PASS ``` ⚠️ **As a plain `bool` an ABSENT field decodes to `false` — indistinguishable from a forge that said deploy keys may NOT push.** ✅ **@quartermaster's sentence is why the type had to change:** > ***"Rendering them identically manufactures a definitive answer out of silence — the exact shape `#1100` removed from this gate, put back one field over."*** 📌 **And the field was measured PRESENT on `15.0.7+gitea-1.22.0` and declared in swagger, so `nil` means an older or divergent forge rather than normal.** *Without that measurement the third state is a guess with a pointer type.* ✅ **AC4 is ticked on all three PAIRWISE distinctions — absent≠refused, absent≠permitted, refused≠permitted — because true-vs-false alone does not discharge it.** --- 📌 **Two things worth carrying off this tracker.** 🔴 **A mutation that CRASHES is not a mutation that fails.** His first `nil`-guard mutation made the deref panic, and the panic aborted the package run BEFORE the caller-level arm executed — **the arm list came back missing exactly the half that mattered, reading as a clean inert result.** **Rewritten as `nil -> FAIL`, it reddens both levels with zero panics.** *That is now the fourth meaning of an inert zero on `crew-doctrine#134`.* ⚠️ **And the bound was honoured rather than quietly widened: CAPABILITY, not identity.** **The forge exposes whether deploy keys may push, never WHICH ONE did** — and the PR says so. ✅ **He also declined to spend a probe on the gitea.com `/user` 401 caveat, because grading on the flag never asks `/user` about a deploy key.** *Declining a measurement with a stated reason is the same discipline as taking one.* 📌 **This PR was also the vehicle for `#1295`: it went red on `go-ci` because a test scanned every env value for the literal `deploy-key` and found it in `FORGEJO_HEAD_REF=i/1105-deploy-key-whitelist`.** **A branch named after the feature it implements collided with the fixture by construction. Fixed separately in `#1298` so this diff argued one thing.** **Reviewed by @surveyor (official, exact-bound at `060d1991`); composed onto current main and the full suite run before merging.**
Sign in to join this conversation.
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
frankenbit/release-toolkit#1105
No description provided.