fix: dropping the box role revokes through box, not behind its back #55
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:bootstrap
scope:coolify
scope:db
scope:docs
scope:drill
scope:installer
scope:labels
scope:platform
scope:runner
scope:users
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/rig#55
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/incus-revoke-via-box"
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?
The bug
rig users applyconverges membership inrig-admin rig incusexactly.For
rig-adminandriga baregpasswd -dis the whole story — they arerig's groups. For
incusit is not: box'ssetup-hostcreates that group,box granthands it out, andbox revoketakes it back doing strictly morethan removing a membership.
box revokesays out loud what removing the membership does not do:Supplementary groups are fixed at login. rig logged
removed <user> from incusand moved on — so an operator who dropped someone from the usersfile and watched apply report success believed their VM access was gone, and
was wrong for as long as that user held a session. A leftover tmux keeps
the Incus socket. Nothing in the output suggested otherwise.
Two sites had it:
commands/users-apply.sh:223(per-user convergence) and:285(the dropped/renamed-user sweep).The fix
Issue option 1 — box takes its own group back, so the group keeps one owner.
Both removal paths route
incusthrough a singledrop_incushelper.--purge. It deletes the user's boxes, images and project.Destroying someone's running machines is not a convergence step; an edit to
a users file must not do it.
box revoke <user> --purgestays a deliberateadmin act. Proven at runtime on every path box is reached on, including the
retry-ish ones — a fallback must never escalate to the destructive verb.
box's installer): a revoke that returns 0 with the membership still
standing has not closed the socket. The effective state is checked, and rig
falls back to removing the group itself.
fallback.
bug being fixed, not the
gpasswdcall. Wherepgrepis absent rig cannottell whether the user holds sessions, so it warns anyway: an unnecessary
warning costs one command, a missing one costs a wrong belief about who can
reach the daemon.
incusgroup needs no new guard.id -nGcannot report a groupthat does not exist, so the existing
in_grouptest at both call sites isalready false on a
host=nobox or one wherebox setup-hostnever ran —nothing to revoke, nothing said, apply moves on. This is stated in the code
rather than left to be rediscovered.
README.mdand--helpupdated to match.Verification
shellcheck -xover the full CI sweep (bin/* **/*.sh): 22 files,clean.
bash test/cli.sh: 344 passed, 0 failed (325 onmain— 19 new).bash test/release.sh: 41 passed, 0 failed.The new coverage exercises the real function, it does not argue about it.
users-apply.shexecutes when sourced (and dies at the root check), sodrop_incusis lifted out of the real file verbatim — column-0drop_incus() {through column-0}— and driven against stublog/warn/in_groupand a PATH replaced by a stub directory holding onlybox,gpasswdandpgrep(replaced, not prefixed, so "box absent" meansabsent even on a host that really has box). Four scenarios: box revokes
successfully; box exits 0 leaving the membership standing; box exits non-zero;
box not on the host. Plus a fifth run with no
pgrepat all.Two guards keep it from passing vacuously: the extraction is asserted
non-empty and whole first, and the
gpasswdstub reports on stderrbecause the real call is
gpasswd -d … >/dev/null— a stub speaking on stdoutwould be silenced by the code under test.
Sensitivity checked by mutation: reverting
drop_incusto the old baregpasswd -d+logturns 10 of the new assertions red.Decisions the issue did not settle
box revokeshould do. Letting it propagate would abortapply mid-convergence under
set -e, leaving users half-converged over ahost-extra tool. Ignoring it would leave the group in place. It falls back
to
gpasswd -d+ the warning: access ends either way, and the operator istold box did not do its job.
the repo's own "don't trust exit codes" discipline, and the membership is
the thing that closes the socket.
pgrepis absent. box only warns when it finds liveprocesses. rig mirrors that, but fails toward saying it when it cannot
tell.
drop_incusstayed inusers-apply.shrather than moving tocommands/lib/, which is parsing-only and would have to gainlog,warnand
in_groupto host it. The test lifts the function instead.bootstrap.shalready runs box's installer. Its lines self-identify (
group:,sessions:,WARNING:,revoked:), and reshaping another tool's warningis how the wording drifts.
Closes #50
🤖 Generated with Claude Code
Verdict: approve.
Re-verified from scratch at head 65964a6. This closes #50 the way the issue asked (option 1), and the failure-mode coverage is the strongest part of the PR.
What I checked
Both direct paths are gone. The per-user convergence (
commands/users-apply.sh:280) and the dropped-user sweep (commands/users-apply.sh:352) now routeincusthrough the singledrop_incushelper; the baregpasswd -dremains only forrig-admin/rig, which are rig's own groups. Grepped the whole tree: no other site touches theincusgroup directly, and nothing anywhere touchesincus-admin(it stays a non-role, per the README). The test attest/cli.sh:761pins the count at exactly 2 call sites.Box absent or failing is loud, and the state is reconciled, not lost.
drop_incus(commands/users-apply.sh:220-244) covers the full matrix: box succeeds (bare revoke, effective-state check, box's own warning passes through); box exits 0 with the membership standing (caught by the! in_groupre-check — the #12 "don't trust exit codes" discipline, correctly applied since the membership is what closes the socket); box exits non-zero (warn +gpasswd -dfallback); box not installed (same fallback). Every fallback path carries the session warning itself, which is the actual bug being fixed — the old code's silence, not itsgpasswd. There is no path where the role is dropped in rig's model while the group quietly survives.--purgeis provably unreachable. Asserted both by shape (test/cli.sh:770) and at runtime on all three box-reached paths (test/cli.sh:846-848) — a fallback never escalates to the destructive verb. Right call: convergence removes access, never running machines.Idempotence. Both call sites gate on
in_group, so a never-granted or already-revoked user is a clean no-op, and the absent-group case (host=no, orbox setup-hostnever ran) needs no new guard becauseid -nGcannot report a nonexistent group — the code comment atcommands/users-apply.sh:216-219states this rather than leaving it to be rediscovered. The sweep's transition-only warning semantics are preserved.Tests are real, not vacuous. The suite lifts
drop_incusverbatim from the live file (guarded against an empty lift attest/cli.sh:782), replaces PATH entirely so "absent" means absent even on a box-bearing host, and the stubgpasswdspeaks on stderr because the real call is>/dev/null-silenced — both anti-vacuity guards hold up under inspection. Ran locally:test/cli.sh344 passed, 0 failed (19 new),test/release.sh41 passed, 0 failed,shellcheck -xclean across the sweep. All match the PR's claims.Non-blocking observations
Wording in one corner (
commands/users-apply.sh:231): ifbox revokeexits non-zero but did remove the membership (e.g., dies after the group edit), rig warns "'box revoke' did not remove the incus group — removing it directly", then removes nothing (thein_groupguard at :233 is false) and logs no removal at all. State and session warning are both correct; only the warn's claim is wrong in that corner. Something like "failed or left the membership standing — reconciling directly" would be accurate on both sub-paths. Fine to leave.Series note: the grant side (
usermod -aGatcommands/users-apply.sh:274) still bypassesbox grant— that is #53's scope, and #53 is open againstmaintouching this same file. Whichever of #53/#55 merges second will need a rebase; when reconciling,drop_incus's pattern (effective-state check, no destructive escalation, fallback that speaks) is the one the grant path should mirror.The lift-by-
sedextraction couples the test todrop_incusstaying at column 0 inusers-apply.sh. The non-empty/whole guard means a future reshaping fails loudly rather than vacuously, which is the right failure direction — just be aware the coupling exists.— automated review by
claude-bot-andresmgsl· heavy-duty-review-botVerdict: Approve — I agree with this as-is.
Why
box/ full offboarding routesincusthroughbox revoke(no--purge) with effective-state check after revoke (exit code not trusted).drop_incus. Stub-driven tests cover ok/hollow/fail/absent. CI green.No blockers.
Verdict: I agree with everything and have no additional feedback. Both
incusremoval paths now delegate to barebox revoke, verify effective membership state instead of trusting its exit code, and fall back without ever escalating to--purge. The fallback preserves the session warning that motivated the fix, and the current CI checks are green.@danmt — handoff summary.
What this fixes
A live bug, independent of the other PRs in this batch.
rig users applyconvergedincusmembership with a baregpasswd -dat two sites — the per-user arm and the dropped-user sweep. rig ownsrig-adminandrig; box ownsincus, andbox revokedoes strictly more with it: it warns that supplementary groups are fixed at login, so a session the user already holds keeps the Incus socket until it dies, and it namesloginctl terminate-useras the remedy.apply logged "removed from incus" and moved on. An operator who dropped someone from the users file and watched apply succeed believed their VM access was gone — and was wrong for as long as that user held a session. The silence was the bug, not the
gpasswd. Closes #50.Review history
One round, no fixes needed. 3/3 approved at head
65964a6— claude-bot, codex-bot, grok-bot. claude-bot re-verified from scratch and called the failure-mode coverage the strongest part of the PR.Verification
test/cli.sh344 passed / 0 failed (baseline 325 — 19 new checks)test/release.sh41 passed / 0 failedshellcheck -xover the CI sweep, 22 files: cleancheck,scope,db-integration,reconcileall greendrop_incusto the old baregpasswd -dturns 10 of the new assertions red — the tests fail against the bug they describeHow it works
Both removal sites route through one
drop_incushelper: call barebox revoke→ verify the membership actually went rather than trusting the exit code (the repo's own #12 discipline; the membership is what closes the socket) → on hollow success, failure, or box absent, fall back togpasswd -dcarrying the session warning itself.--purgeis never passed on any path, asserted both by shape and at runtime — convergence removes access, never running machines.Test quality worth noting
The suite lifts
drop_incusverbatim out of the live file and drives it against stubs with PATH fully replaced, so "box absent" means absent even on a box-bearing host. Two anti-vacuity guards were live bugs during development: the extraction is asserted non-empty and whole before use, and thegpasswdstub speaks on stderr because the real call is>/dev/null-silenced — a stdout stub would have made every fallback assertion pass for free.Flagged for your judgment
One wording corner claude-bot caught (
users-apply.sh:231): ifbox revokeexits non-zero but did remove the membership — dying after the group edit — rig warns "'box revoke' did not remove the incus group — removing it directly", then removes nothing, because the guard below is correctly false. State and session warning are both right; only the warning's claim is wrong in that one sub-path. "failed or left the membership standing — reconciling directly" would be accurate on both. I left it rather than restart an approved round for a wording fix — say the word and I'll push it.A coupling to be aware of: the test's
sedlift ties itself todrop_incusstaying at column 0. The non-empty/whole guard means a future reshaping fails loudly rather than vacuously, which is the right failure direction — but the coupling exists.Merge order matters
#53 touches this same file — it owns the add arm, this PR owns the removal arms, deliberately non-overlapping. Whichever lands second needs a rebase; both also touch CHANGELOG's
## Unreleased. claude-bot's suggestion when reconciling:drop_incus's pattern — effective-state check, no destructive escalation, fallback that speaks — is the one the grant path should mirror.🤖 Generated with Claude Code
Rebased onto
mainafter #60 merged — and the changelog needed hand-placing@danmt — sweep after #60. This branch is rebased onto
main;mergeable: true.The changelog needed more than conflict resolution, and the reason is worth knowing before you merge the rest of this batch.
maincut 0.1.0 at 18:03:45Z. #60 merged at 19:02:12Z — an hour later. Its entry had been written under## Unreleased, but by merge time that heading had been stamped to## 0.1.0, so git merged the entry cleanly, with no conflict, into the notes of an already-published release. Same thing happened to this branch on rebase.Every PR in this batch predates the release, so every one of them has this. It does not announce itself — #54's rebase produced zero conflicts and still put a BREAKING entry inside 0.1.0's notes.
Resolved here by rebuilding the section structure: a fresh
## Unreleasedholding this PR's entry and #60's (moved back out of 0.1.0, where it does not belong — it shipped after), with## 0.1.0left holding only what it actually released.Scope of the damage, checked rather than assumed: the published GitHub release body for 0.1.0 is clean — it was generated at release time, before #60 merged, so it never contained the stray entry. Only the
CHANGELOG.mdfile onmainwas wrong, and merging any PR from this batch corrects it.Filed as heavy-duty/rig#66 — the release flow has no guard against this, and it will recur on every release.
Verification at the rebased head
shellcheck -xover the CI sweep — cleantest/release.sh— 61 / 0Content is otherwise unchanged from what the bots approved; the delta is the rebase plus changelog placement.