v0.101.0 — R-82: a leaked restore-test scratch can no longer auto-start
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).
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user