From f38b03cdb8b30a19eeeb91cc3812f25855646bc3 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 13 Oct 2026 19:33:09 +0200 Subject: [PATCH] Host power-off: refuse while another guest or VM runs; hold the heavy gate; fail closed (R-998) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security review of 3403c87: (1) any guest's controller could switch off a shared host — demo-hp runs a scratch guest and the Tester 1 VM. Now refused (other_guests_running) when /proc shows any other lxc-start -n N or kvm -id N; an unreadable /proc refuses. (2) the checks ran once, so a backup could start before the power-off: the heavy-operation gate is now taken on acceptance and kept (released only if the power-off command fails). (3) an unwired gate or task reader refused nothing; now refuses. Each red-proofed. --- internal/localapi/hostpoweroff.go | 100 ++++++++++++++++++++++--- internal/localapi/hostpoweroff_test.go | 69 ++++++++++++++++- 2 files changed, 155 insertions(+), 14 deletions(-) diff --git a/internal/localapi/hostpoweroff.go b/internal/localapi/hostpoweroff.go index 31a92a5..c28a5c4 100644 --- a/internal/localapi/hostpoweroff.go +++ b/internal/localapi/hostpoweroff.go @@ -4,6 +4,8 @@ import ( "context" "errors" "net/http" + "os" + "path/filepath" "strconv" "strings" "time" @@ -20,8 +22,15 @@ import ( // - refused in the first 10 minutes after boot (no power-off loop); // - at most ONE accepted request per boot (poweroff.State, persisted, keyed by the boot); // - refused while a backup, restore test, OS update step, controller update, drive format, NAS verify -// or escrow ceremony runs, or a vzdump of the guest is in flight — each named in the 409. +// or escrow ceremony runs, or a vzdump of the guest is in flight — each named in the 409; +// - refused while ANY OTHER guest or VM runs on the host (security review 2026-10-13): one guest's +// controller must not switch off another household's guest, a scratch guest or a test VM (demo-hp +// carries the Tester 1 VM). Read from /proc (`lxc-start -n N`, `kvm -id N`), fail closed; +// - the heavy-operation gate is TAKEN on acceptance and never given back (the box is going down), so no +// backup, restore test or OS step can start between the checks and the power-off (security review). +// Released only if the power-off command itself fails. // +// Every check fails CLOSED: an unwired gate, an unreadable task list or /proc refuses. // On acceptance the record is written FIRST, the hub is given a report carrying it (bounded wait), and // only then: guest shutdown (time-limited), host power-off. Pinned by hostpoweroff_test.go. @@ -37,6 +46,7 @@ type HostPowerOffConfig struct { Power HostPowerer ReportNow func(ctx context.Context) bool // push a report carrying the record; true = the hub accepted it Uptime func() (time.Duration, bool) // time since host boot; false = unknown (refuse) + Running func() ([]int, error) // vmids of every running guest and VM on the host; default /proc MinUptime time.Duration // default 10 min GuestStop time.Duration // pct shutdown --timeout; default 120 s ReportMax time.Duration // longest wait for the hub; default 60 s @@ -54,6 +64,7 @@ const ( PowerOffDrive = "drive_operation_running" PowerOffEscrow = "escrow_ceremony_running" PowerOffBusy = "host_busy" + PowerOffOtherGuests = "other_guests_running" PowerOffUnknown = "state_unknown" ) @@ -71,6 +82,9 @@ func (s *Server) SetHostPowerOff(c HostPowerOffConfig) { if c.Now == nil { c.Now = time.Now } + if c.Running == nil { + c.Running = func() ([]int, error) { return runningGuestsFromProc("/proc") } + } s.powerOff = &c } @@ -101,8 +115,15 @@ func (s *Server) handleHostPowerOff(w http.ResponseWriter, r *http.Request, vmid writeStatus(w, http.StatusConflict, false, map[string]any{"reason": code, "detail": detail}, "host power-off refused: "+code) return } + // Take the heavy-operation gate for good: nothing heavy may start between these checks and the power-off. + release, busy, free := s.inFlight.TryAcquire("host-poweroff") + if !free { + writeStatus(w, http.StatusConflict, false, map[string]any{"reason": PowerOffBusy, "detail": busy}, "host power-off refused: "+PowerOffBusy) + return + } rec, err := c.State.Mark(c.Now(), vmid) if err != nil { + release() code := PowerOffUnknown if errors.Is(err, poweroff.ErrAlreadyThisBoot) { code = PowerOffAlreadyAsked @@ -117,7 +138,7 @@ func (s *Server) handleHostPowerOff(w http.ResponseWriter, r *http.Request, vmid if base == nil { base = context.Background() } - go s.runHostPowerOff(base, vmid) + go s.runHostPowerOff(base, vmid, release) writeStatus(w, http.StatusAccepted, true, map[string]any{ "vmid": vmid, "powering_off": true, "requested_at": rec.RequestedAt.Format(time.RFC3339), }, "") @@ -126,7 +147,7 @@ func (s *Server) handleHostPowerOff(w http.ResponseWriter, r *http.Request, vmid // runHostPowerOff: hub first (bounded), then the guest (time-limited), then the host. A failed guest // shutdown does not stop the power-off — the household asked for it, and Proxmox's own shutdown stops // any guest still running (pve-guests) before the host goes down. -func (s *Server) runHostPowerOff(base context.Context, vmid int) { +func (s *Server) runHostPowerOff(base context.Context, vmid int, release func()) { c := s.powerOff if c.ReportNow != nil { ctx, cancel := context.WithTimeout(base, c.ReportMax) @@ -148,7 +169,8 @@ func (s *Server) runHostPowerOff(base context.Context, vmid int) { ctx, cancel = context.WithTimeout(base, 60*time.Second) defer cancel() if err := c.Power.PowerOffHost(ctx); err != nil { - s.logger.Error("host-poweroff: host power-off command failed", "err", err) + s.logger.Error("host-poweroff: host power-off command failed — heavy-operation gate released", "err", err) + release() } } @@ -168,6 +190,22 @@ func (s *Server) powerOffBlocker(ctx context.Context, vmid int) (string, string) if up < c.MinUptime { return PowerOffTooSoon, "host up " + up.Round(time.Second).String() + ", minimum " + c.MinUptime.String() } + if s.inFlight == nil || s.staleLock == nil { + return PowerOffUnknown, "the heavy-operation gate or the Proxmox task reader is not wired" + } + running, err := c.Running() + if err != nil { + return PowerOffUnknown, "cannot list the running guests: " + err.Error() + } + var others []string + for _, v := range running { + if v != vmid { + others = append(others, strconv.Itoa(v)) + } + } + if len(others) > 0 { + return PowerOffOtherGuests, "other guests or VMs running: " + strings.Join(others, ", ") + } if busy := s.inFlight.Busy(); busy != "" { switch { case strings.HasPrefix(busy, "backup:"): @@ -213,14 +251,52 @@ func (s *Server) powerOffBlocker(ctx context.Context, vmid int) (string, string) if esc { return PowerOffEscrow, "escrow ceremony running" } - if s.staleLock != nil { - running, err := s.staleLock.BackupRunning(ctx, vmid) - if err != nil { - return PowerOffUnknown, "cannot list running Proxmox tasks: " + err.Error() - } - if running { - return PowerOffBackup, "vzdump running for guest " + strconv.Itoa(vmid) - } + vz, err := s.staleLock.BackupRunning(ctx, vmid) + if err != nil { + return PowerOffUnknown, "cannot list running Proxmox tasks: " + err.Error() + } + if vz { + return PowerOffBackup, "vzdump running for guest " + strconv.Itoa(vmid) } return "", "" } + +// runningGuestsFromProc lists the vmids of running containers (`lxc-start … -n N`) and VMs (`kvm -id N`) from +// the process table — readable by the non-root agent, unlike /run/qemu-server (measured on demo-hp 2026-10-13). +// An unreadable /proc is an error (fail closed); an unreadable single process is skipped (it exited). +func runningGuestsFromProc(procDir string) ([]int, error) { + ents, err := os.ReadDir(procDir) + if err != nil { + return nil, err + } + seen := map[int]bool{} + var out []int + for _, e := range ents { + if _, err := strconv.Atoi(e.Name()); err != nil { + continue + } + raw, err := os.ReadFile(filepath.Join(procDir, e.Name(), "cmdline")) + if err != nil || len(raw) == 0 { + continue + } + args := strings.Split(strings.TrimRight(string(raw), "\x00"), "\x00") + flag := "" + switch filepath.Base(args[0]) { + case "lxc-start": + flag = "-n" + case "kvm", "qemu-system-x86_64": + flag = "-id" + default: + continue + } + for i := 1; i+1 < len(args); i++ { + if args[i] == flag { + if v, err := strconv.Atoi(args[i+1]); err == nil && !seen[v] { + seen[v] = true + out = append(out, v) + } + } + } + } + return out, nil +} diff --git a/internal/localapi/hostpoweroff_test.go b/internal/localapi/hostpoweroff_test.go index ed6c0c0..f2c8f93 100644 --- a/internal/localapi/hostpoweroff_test.go +++ b/internal/localapi/hostpoweroff_test.go @@ -3,8 +3,12 @@ package localapi import ( "context" "encoding/json" + "errors" "net/http" + "os" "path/filepath" + "sort" + "strings" "sync" "testing" "time" @@ -62,6 +66,7 @@ func newPowerRig(t *testing.T, uptime time.Duration) *powerRig { t.Helper() srv := newTestServerS(t, &fakeGuests{}, &fakeBackups{}, &fakeStore{}, nil) srv.inFlight = &backup.InFlight{} + srv.staleLock = fakeVzdump{} p := &fakePower{} st := poweroff.New(filepath.Join(t.TempDir(), "poweroff.json"), func() string { return "1700000000" }) var reports []string @@ -79,8 +84,9 @@ func newPowerRig(t *testing.T, uptime time.Duration) *powerRig { } return true }, - Uptime: func() (time.Duration, bool) { return uptime, true }, - Now: func() time.Time { return testNow }, + Uptime: func() (time.Duration, bool) { return uptime, true }, + Running: func() ([]int, error) { return []int{8200}, nil }, + Now: func() time.Time { return testNow }, }) return &powerRig{srv: srv, h: srv.Handler(), power: p, state: st, report: &reports} } @@ -118,6 +124,10 @@ func TestHostPowerOff_AcceptedOrder(t *testing.T) { if len(steps) != 2 || steps[0] != want[0] || steps[1] != want[1] { t.Fatalf("steps = %v, want %v", steps, want) } + // The heavy-operation gate stays taken: no backup can start between the checks and the power-off. + if _, busy, ok := r.srv.inFlight.TryAcquire("backup:primary"); ok || busy != "host-poweroff" { + t.Fatalf("after acceptance a backup could take the gate (ok=%v busy=%q)", ok, busy) + } if len(*r.report) != 1 || (*r.report)[0] != "report" { t.Fatalf("hub report = %v, want one report carrying the record", *r.report) } @@ -215,3 +225,58 @@ func TestHostPowerOff_ScopeAndConfig(t *testing.T) { t.Fatal("a refused request changed state") } } + +// Another guest or VM on the host refuses: one controller must not switch off someone else's guest (security +// review 2026-10-13; demo-hp runs a scratch guest and the Tester 1 VM). An unreadable process list refuses too. +func TestHostPowerOff_RefusedWhileOtherGuestsRun(t *testing.T) { + r := newPowerRig(t, time.Hour) + r.srv.powerOff.Running = func() ([]int, error) { return []int{8200, 9202, 341}, nil } + w := do(t, r.h, "POST", "/host/poweroff", "A", "") + if w.Code != http.StatusConflict || reasonOf(t, w.Body.Bytes()) != PowerOffOtherGuests { + t.Fatalf("got %d %s, want 409 %s", w.Code, w.Body.String(), PowerOffOtherGuests) + } + r.srv.powerOff.Running = func() ([]int, error) { return nil, errors.New("no /proc") } + w = do(t, r.h, "POST", "/host/poweroff", "A", "") + if w.Code != http.StatusConflict || reasonOf(t, w.Body.Bytes()) != PowerOffUnknown { + t.Fatalf("unreadable: got %d %s, want 409 %s", w.Code, w.Body.String(), PowerOffUnknown) + } + if len(r.power.got()) != 0 || r.state.Current() != nil { + t.Fatal("a refused request changed state") + } +} + +// Fail closed: an unwired vzdump reader refuses instead of skipping the check. +func TestHostPowerOff_FailsClosedWithoutTaskReader(t *testing.T) { + r := newPowerRig(t, time.Hour) + r.srv.staleLock = nil + w := do(t, r.h, "POST", "/host/poweroff", "A", "") + if w.Code != http.StatusConflict || reasonOf(t, w.Body.Bytes()) != PowerOffUnknown { + t.Fatalf("got %d %s, want 409 %s", w.Code, w.Body.String(), PowerOffUnknown) + } +} + +// The /proc reader finds containers and VMs by their real command lines (as measured on demo-hp). +func TestRunningGuestsFromProc(t *testing.T) { + dir := t.TempDir() + write := func(pid, cmd string) { + _ = os.MkdirAll(filepath.Join(dir, pid), 0o755) + _ = os.WriteFile(filepath.Join(dir, pid, "cmdline"), []byte(strings.ReplaceAll(cmd, " ", "\x00")+"\x00"), 0o644) + } + write("1927", "/usr/bin/lxc-start -F -n 9201") + write("1900", "/usr/bin/lxc-start -F -n 9202") + write("1823", "/usr/bin/kvm -id 341 -name night1004-tester1 -no-shutdown") + write("974", "/usr/libexec/lxc/lxc-monitord --daemon") + write("50", "/usr/bin/bash -c echo -n 5") + _ = os.MkdirAll(filepath.Join(dir, "self"), 0o755) + got, err := runningGuestsFromProc(dir) + if err != nil { + t.Fatal(err) + } + sort.Ints(got) + if len(got) != 3 || got[0] != 341 || got[1] != 9201 || got[2] != 9202 { + t.Fatalf("got %v, want [341 9201 9202]", got) + } + if _, err := runningGuestsFromProc(filepath.Join(dir, "missing")); err == nil { + t.Fatal("an unreadable /proc must be an error") + } +}