From 0fabc158969778caf9c86a9a722f631a7c227e04 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Sun, 26 Jul 2026 16:49:40 +0200 Subject: [PATCH] =?UTF-8?q?v0.101.0=20=E2=80=94=20R-82:=20a=20leaked=20res?= =?UTF-8?q?tore-test=20scratch=20can=20no=20longer=20auto-start?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CORRECTION: I earlier reported that the restore-test would boot a scratch guest with the live guest's MAC/static island IP/hostname and break the control plane. That was WRONG — RunRestoreTest step 2 link-downs EVERY interface (withLinkDown, unit-tested) before the guest is ever started. The design already handled it. The real, narrower hazard: a restore that fails BEFORE step 2 (what the v0.100.0 wait bug caused) leaves a scratch holding the SOURCE guest's config verbatim, including onboot:1. If teardown also fails (403 missing VM.Allocate — PVE associates the pool only at restore completion), a host reboot would start that leaked clone alongside the original with NICs up. - proxmox.RestoreLXCOptions.ConfigOverrides: guest-config params applied AT RESTORE TIME. - The restore-test passes onboot=0 — at restore time, not after, because 'after' is exactly the path that leaks. NOT changed: the link-down step (already correct, the primary defence); the agent's Proxmox privileges (widening VM.Allocate to /vms would remove the accidental guard that stopped a destructive mid-restore teardown). restore_test_cadence_seconds was set to -1 on demo-felhom under the mistaken reading; re-enabled. Red-proof observed; full suite green (29 packages). --- CHANGELOG.md | 37 ++++++++++++++++++++++++++ internal/proxmox/mutate.go | 13 +++++++++ internal/reconcile/restoretest.go | 6 +++++ internal/reconcile/restoretest_test.go | 24 +++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f512a87..5acf31f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,40 @@ +## v0.101.0 — R-82: a leaked restore-test scratch can no longer auto-start (2026-07-26) + +**Correction first, because it matters more than the fix.** I reported earlier in this arc that the +restore-test would boot a scratch guest carrying the live guest's MAC, static island IP and +hostname, and so would break the controller→agent link. **That was WRONG.** `RunRestoreTest` step 2 +link-downs **every** interface (`withLinkDown`, unit-tested) BEFORE the guest is ever started, so on +the normal path there is no L2/IP conflict. The design already handled it. + +What is real is narrower. A restore that fails **before** step 2 — exactly what the v0.100.0 wait +bug caused — leaves a scratch guest holding the SOURCE guest's config verbatim, including +`onboot: 1`. If teardown then also fails (it did: `403 … missing privilege VM.Allocate`, because PVE +associates the pool only at restore COMPLETION), the leaked guest survives and **a host reboot would +start it** alongside the original, NICs up, same MAC, same `169.254.253.2/30`. + +So the hazard needed three things to line up, and it did, once, on demo-felhom. + +### Changed +- **`proxmox.RestoreLXCOptions.ConfigOverrides`** — arbitrary guest-config params applied AT RESTORE + TIME, for settings that must hold from the instant the guest exists. +- **The restore-test passes `onboot=0`.** At restore time, not after: "after" is precisely the path + that leaks. A leaked scratch is now inert across a host reboot even with its NICs still up. + +### NOT changed +- The link-down step. It was already correct and is the primary defence; this is depth behind it. +- The agent's Proxmox privileges. It still cannot tear down a scratch until the restore completes. + Widening `VM.Allocate` to `/vms` would remove the accidental guard that stopped a destructive + mid-restore teardown — the wrong trade. With the v0.100.0 timeout fix the teardown no longer fires + mid-restore. + +### Operational +`restore_test_cadence_seconds` was set to `-1` on demo-felhom as a stopgap under the mistaken +reading above. **Re-enabled** — the scheduled restore-test is safe and always was. + +### Tests +`TestRestoreTest_RestoreSetsOnbootZero`; red-proof observed (removing the override yields +`got "" (map[string]string(nil))`). Full suite green (29 packages). + ## v0.100.0 — R-82: the restore tier comes from the ARCHIVE, not the configured target (2026-07-26) **Found by the first real PBS restore round-trip (2026-07-26), not by review.** Restoring diff --git a/internal/proxmox/mutate.go b/internal/proxmox/mutate.go index 5888dca..d05d6a1 100644 --- a/internal/proxmox/mutate.go +++ b/internal/proxmox/mutate.go @@ -47,6 +47,16 @@ type RestoreLXCOptions struct { // 'mpN' to bind mount is only possible for root"); replacing it with a throwaway volume needs // no root and the boot-verify doesn't need the drive's data. MountOverrides map[string]string + // ConfigOverrides sets arbitrary guest-config params AT RESTORE TIME (they take precedence over + // the archive's own values), for settings that must hold from the instant the guest exists — + // before any post-restore SetConfig could run. + // + // The restore-test uses it for `onboot=0`. A restore that fails BEFORE the post-restore config + // step leaves a scratch guest carrying the SOURCE guest's config verbatim, including + // `onboot: 1` — so a leaked scratch would auto-start on the next host reboot, with the source's + // MAC, static island IP and hostname. Observed live 2026-07-26. The normal path link-downs every + // NIC before boot, so this is defence in depth for the ABNORMAL path, where the leak happens. + ConfigOverrides map[string]string } // RestoreLXC restores an LXC from a vzdump/PBS archive via POST /nodes/{node}/lxc @@ -71,6 +81,9 @@ func (c *Client) RestoreLXC(ctx context.Context, opts RestoreLXCOptions) (string for k, val := range opts.MountOverrides { v.Set(k, val) // e.g. mp0 -> "local-lvm:1,mp=/data,backup=0" (overrides the archive's mp0) } + for k, val := range opts.ConfigOverrides { + v.Set(k, val) // e.g. onboot -> "0" (a leaked scratch must never auto-start) + } return c.dataString(ctx, http.MethodPost, "/nodes/"+c.node+"/lxc", v) } diff --git a/internal/reconcile/restoretest.go b/internal/reconcile/restoretest.go index a7af2a2..c22befb 100644 --- a/internal/reconcile/restoretest.go +++ b/internal/reconcile/restoretest.go @@ -231,6 +231,12 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS // Pool=DefaultPool so the scratch guest is created INTO the felhom pool — else a pool-scoped // token 403s on the scratch guest's config/start/destroy (SPIKE residual #2). VMID: vmid, Archive: spec.Archive, Storage: spec.RestoreStorage, MountOverrides: mountOverrides, Pool: DefaultPool, + // onboot=0 from the instant the guest exists. Step 2 below link-downs every NIC before the + // guest is ever started, so the NORMAL path cannot conflict with the live source. This + // covers the ABNORMAL path: a restore that fails before step 2 (e.g. the wait expiring) can + // leave a scratch carrying the source's `onboot: 1` plus its MAC/static island IP/hostname — + // which a host reboot would then start alongside the original. Observed live 2026-07-26. + ConfigOverrides: map[string]string{"onboot": "0"}, }) if err != nil { if pveAlreadyExists(err) { diff --git a/internal/reconcile/restoretest_test.go b/internal/reconcile/restoretest_test.go index 1bde068..35c0359 100644 --- a/internal/reconcile/restoretest_test.go +++ b/internal/reconcile/restoretest_test.go @@ -667,3 +667,27 @@ func TestRunRestoreTest_RefusalsPropagate(t *testing.T) { t.Fatalf("never restore a partial guest to verify it: %+v", apiBind.restores) } } + +// A leaked scratch guest must never AUTO-START. The normal path link-downs every NIC before boot +// (TestRestoreTest… above), so the source can never be conflicted with on the happy path. This +// covers the abnormal one: a restore that fails BEFORE the link-down step leaves a scratch carrying +// the SOURCE guest's config verbatim — including `onboot: 1`, its MAC, its static island IP and its +// hostname. Observed live 2026-07-26, when a wait-timeout left exactly such a guest on demo-felhom. +// onboot=0 is therefore set AT RESTORE TIME, not after: after is too late for the path that leaks. +func TestRestoreTest_RestoreSetsOnbootZero(t *testing.T) { + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{990000: scratchCfg()}} + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + _ = e.RunRestoreTest(context.Background(), RestoreTestSpec{ + Archive: "local:backup/x.tar.zst", RestoreStorage: "local-lvm", + ScratchMin: 990000, ScratchMax: 990009, SourceTier: "local", + }) + if len(api.restores) != 1 { + t.Fatalf("want one restore, got %+v", api.restores) + } + if got := api.restores[0].ConfigOverrides["onboot"]; got != "0" { + t.Fatalf("the restore MUST set onboot=0 so a leaked scratch cannot auto-start; got %q (%#v)", + got, api.restores[0].ConfigOverrides) + } +}