F-LEAK: record all three attempts; the ACL fix is consumed by use, the fenced destroy is the fix

This commit is contained in:
2026-07-28 11:32:45 +02:00
parent cba48293f1
commit 5ffdcecb2d
2 changed files with 50 additions and 19 deletions
@@ -389,29 +389,60 @@ against the **587 s** the finding's original incident needed a human. The lock-d
live during the replay. Scenario B proven on the same box — an `onboot:0` guest was left stopped
across the whole window.
### F-LEAK — FIXED (host-install v1.21.0, 2026-07-28) — and the first fix was WRONG
`FelhomAgentGuest` is now granted at each `/vms/990000``/vms/990009` — the restore-test's scratch
band. The cause is structural rather than a missing privilege in the role: the role is granted at
`/pool/felhom`, and **a guest only joins that pool when its restore completes**, so a *failed*
restore-test leaves a guest that exists, is in no pool, and is outside the token's reach.
### F-LEAK — FIXED (agent v0.110.0 + host-install v1.21.0, 2026-07-28) — after TWO refuted attempts
**The first attempt was refuted live and is recorded because it looked right.** Agent v0.107.0 shipped
a teardown fallback that adopted the stranded guest into the pool and retried. It fired exactly as
designed and PVE refused it: `PUT /pools/felhom -> HTTP 500: permission denied at /vms/990000 (missing
privilege ...)`. `PUT /pools/{pool}` **also** requires `VM.Allocate` on the VM being added — pool
membership cannot bootstrap its own authority. That code was removed in agent v0.108.0.
**Three attempts, two refuted by live test.** Recorded in full because both wrong answers looked right
and one of them shipped.
**Why the grant is not a widening**, proven live at the same seam the defect lives in — and note that
PVE checks **permission before existence**, so these 403s are genuine refusals, not artifacts:
The cause is structural rather than a missing privilege in the role: `FelhomAgentGuest` is granted at
`/pool/felhom`, and **a guest joins that pool only when its restore completes**, so a *failed*
restore-test leaves a guest that exists, is in no pool, and is out of the token's reach entirely.
| `DELETE /nodes/<node>/lxc/<id>` with the agent's own token | result |
**Attempt 1 — adopt the stranded guest into the pool, then retry** (agent v0.107.0, shipped). Fired
exactly as designed and PVE refused it: `PUT /pools/felhom -> HTTP 500: permission denied at /vms/990000
(missing privilege ...)`. `PUT /pools/{pool}` **also** requires `VM.Allocate` on the VM being added —
**pool membership cannot bootstrap its own authority.** Removed in v0.108.0.
**Attempt 2 — grant the role per-path at `/vms/990000..990009`** (host-install v1.21.0). It *works*, and
the live A/B proved it: grant removed → `403 (/vms/990000, VM.Allocate)`, guest still present; grant
restored → `200 UPID:...vzdestroy`, guest gone. **But it is durable for exactly one use per slot.** The
post-destroy check showed `/vms/990000` with **0 grants**, and PVE's own source says why: the destroy
path calls `AccessControl::remove_vm_access($vmid)` (`API2/LXC.pm:906`), which deletes every ACL at
`/vms/<vmid>` (`AccessControl.pm:1898`). **The grant is consumed by the operation it authorises** — after
ten teardowns the band is ungranted and the defect returns. A per-VM ACL is the wrong tool; the pool
grant is durable only because it lives on a *pool* path, which `remove_vm_access` does not touch.
**Attempt 3 — route the destroy through the authority that already performs the create** (agent
v0.110.0). `Privileged.DestroyScratchLXC` is the **fourth** root-fenced exception, and it is fenced
harder than the other three, in three places none of which is consumed by use:
| layer | enforcement |
|---|---|
| `990000` (stranded, pool-less, restored scratch) | **destroyed** — the defect's exact case |
| `990010` (one past the band) | **403** `Permission check failed (/vms/990010, VM.Allocate)` |
| `100` (arbitrary non-pool guest) | **403** same |
| **sudoers** | `/usr/sbin/pct destroy 99000[0-9] --purge` — sudo matches the vmid **literally** |
| **`DestroyScratchLXC`** | re-checks the band; refuses an unconfigured or inverted band rather than defaulting |
| **`teardownScratch`** | acts only on this journal entry's own scratch provenance |
Granting at `/vms` was rejected: it would authorise destroying every guest on the box, including a
co-tenant's.
The API destroy is still tried **first** and stays the normal path; the v1.21.0 band ACLs stay
provisioned so the common case needs no privileged call at all. This is the fallback that makes teardown
deterministic instead of once-per-slot.
**The fence, proven live by asking sudo rather than executing** (executing the negative case against a
live guest is precisely what must not happen):
| command | sudo verdict |
|---|---|
| `pct destroy 990000 / 990005 / 990009 --purge` | **PERMITTED** |
| `pct destroy 9201 --purge` (the live customer guest) | **REFUSED** |
| `pct destroy 9100 / 9999 / 1 --purge` | **REFUSED** |
| `pct destroy 990010 --purge` (one past the band) | **REFUSED** |
| `pct start 990000` / `pct set 990000 -memory 512` | **REFUSED** — the grant is this one op, not this vmid |
Also proven at the API seam, and worth keeping because it establishes the *shape* of the refusal: a
`DELETE` of the non-existent `990010` returned **403**, not "does not exist" — PVE evaluates
**permission before existence**, so these are genuine authorization refusals rather than artifacts.
Granting `VM.Allocate` at `/vms` was rejected throughout: it would authorise destroying every guest on
the box, including a co-tenant's.
### F-OBS — FIXED (controller v0.180.0, 2026-07-28)
`deadapp-check` now emits a summary line at **INFO** every 20th scan (10 min at its 30 s cadence),