v0.113.0 — E-2a: guarded backup-target wrapper + POST /backup/target

The agent cannot create a PVE storage (Datastore.Allocate at /storage) or grant
an ACL (Permissions.Modify) -- it holds neither by design, and widening the role
would trade the whole blast-radius containment model for one feature. The
privileged half therefore lives in a new fenced shim behind a literal
FELHOM_BACKUPTARGET sudoers alias, following the mkfs/pbs-apply pattern.

The wrapper enforces the two laws E-1 paid for on live hardware so no caller can
forget them: F-1 the path must BE the drive's own mountpoint, F-2 is_mountpoint 1
is hardcoded rather than a caller flag. It refuses a root-device target, has NO
storage-removal path of any kind (the pbs-apply no-delete law, grep-assertable),
is idempotent for the same path, and REFUSES to repoint an existing id.

POST /backup/target drives it in a fixed order: create -> grant -> config.
Reversed, a config pointing at an ungranted storage 403s every backup on first
run -- exactly E-1 finding F-3. A failed grant leaves the config untouched.

It deliberately does NOT restart the agent: restarting with a backup in flight
cancels the wait and records a spurious tier failure for a backup that actually
succeeded (E-1 did this to a real felhom-pbs run). It returns restart_required
and the caller restarts behind its own immediate in-flight check.

Config rewrite preserves unknown keys verbatim and writes in place, since
/etc/felhom-agent is root-owned while agent.json is agent-owned 0600.

Green gate: build + vet + test rc=0 (29 packages), run separately from this commit.
This commit is contained in:
2026-07-29 09:05:59 +02:00
parent 958e54f6a6
commit 58b598b697
7 changed files with 571 additions and 2 deletions
+212
View File
@@ -0,0 +1,212 @@
package localapi
import (
"context"
"encoding/json"
"fmt"
"net/http"
"os"
"path/filepath"
"strings"
)
// BackupTargetWrapperPath is the pinned sudoers vector (configs/felhom-backup-target-apply). The
// agent cannot create a PVE storage or grant an ACL itself — Datastore.Allocate at /storage and
// Permissions.Modify are deliberately outside its role — so the privileged half runs here.
const BackupTargetWrapperPath = "/usr/local/sbin/felhom-backup-target-apply"
// backupTargetRequest is POST /backup/target: move the PRIMARY whole-guest backup tier onto the
// drive mounted at Where, creating the storage if needed.
type backupTargetRequest struct {
VMID int `json:"vmid"`
Where string `json:"where"` // the drive's OWN host mountpoint (F-1)
ID string `json:"id,omitempty"` // storage id; default backupTargetStorageID
}
// backupTargetStorageID is the conventional id, matching what E-1 created by hand on both demo boxes.
// Keeping the name identical is what makes this endpoint IDEMPOTENT on an already-migrated box: the
// wrapper accepts an existing entry with the same path and changes nothing.
const backupTargetStorageID = "felhom-backup"
// handleSetBackupTarget performs the whole move as one ordered operation: create the storage, grant
// the agent access, repoint the primary tier in agent.json, and hand back what the caller must do to
// make it take effect.
//
// THE ORDER IS THE DESIGN, and each step is a precondition for the next:
//
// create → grant → config
//
// Reversed, a config pointing at a storage that does not exist would make the tier DEFER (harmless
// but silent), and a config pointing at an ungranted storage would make every backup 403 on its
// first run — which is exactly what E-1 hit when the grant was forgotten (finding F-3). Creating and
// granting BEFORE the config means the worst interruption leaves an unused storage, never a broken
// tier.
//
// IT DOES NOT RESTART THE AGENT. That is deliberate and it is the E-1 lesson encoded: the backup
// tiers are built once at daemon start, so the move needs a restart to take effect — but restarting
// while a backup or restore-test is in flight cancels the wait and records a SPURIOUS tier failure
// for a backup that actually succeeded (E-1 did exactly this to a felhom-pbs run). A restart that
// this handler fires itself could never be re-checked against in-flight work by the caller, so the
// response reports `restart_required` and the caller performs it behind its own immediate
// in-flight check.
func (s *Server) handleSetBackupTarget(w http.ResponseWriter, r *http.Request, vmid int) {
if s.privileged == nil {
writeErr(w, http.StatusServiceUnavailable, "privileged runner not configured on this host")
return
}
var req backupTargetRequest
if !decodeBody(w, r, &req) {
return
}
if !s.scopedFromBody(w, req.VMID, vmid, r.URL.Path) {
return
}
where := strings.TrimSpace(req.Where)
if where == "" {
writeErr(w, http.StatusBadRequest, "where (the drive's own mountpoint) is required")
return
}
id := strings.TrimSpace(req.ID)
if id == "" {
id = backupTargetStorageID
}
// AGENT-SIDE VALIDATION FIRST, from the agent's own storage view — never the caller's claim.
// The wrapper re-checks everything as root (it is the security boundary), but refusing here gives
// the customer a reason instead of a shell error, and keeps a bad request from reaching sudo at all.
if err := s.validateBackupTargetMount(r.Context(), where); err != nil {
s.logger.Warn("local-api: backup-target move refused", "vmid", vmid, "where", where, "err", err)
writeErr(w, http.StatusBadRequest, err.Error())
return
}
if _, errOut, err := s.privileged.Run(r.Context(), BackupTargetWrapperPath, "create", id, where); err != nil {
s.logger.Error("local-api: backup-target create failed", "id", id, "where", where, "err", err, "stderr", string(errOut))
writeErr(w, http.StatusBadGateway, "could not create the backup storage: "+wrapperReason(errOut, err))
return
}
if _, errOut, err := s.privileged.Run(r.Context(), BackupTargetWrapperPath, "grant", id); err != nil {
// The storage exists but the agent cannot write to it. Say so precisely: this is the exact
// state that produced E-1's "403 permission denied at /storage/felhom-backup" on first backup.
s.logger.Error("local-api: backup-target grant failed", "id", id, "err", err, "stderr", string(errOut))
writeErr(w, http.StatusBadGateway, "storage created but the access grant failed — backups would 403: "+wrapperReason(errOut, err))
return
}
if err := s.setConfiguredBackupTarget(id); err != nil {
s.logger.Error("local-api: backup-target config write failed", "id", id, "err", err)
writeErr(w, http.StatusInternalServerError, "storage is ready but the config could not be updated: "+err.Error())
return
}
s.logger.Info("local-api: backup target moved — RESTART REQUIRED for it to take effect",
"vmid", vmid, "target", id, "where", where)
writeOK(w, map[string]any{
"vmid": vmid, "target": id, "where": where,
// The caller must restart the agent BEHIND ITS OWN in-flight check — see the doc comment.
"restart_required": true,
})
}
// validateBackupTargetMount refuses a mount that cannot be a real backup target, from the agent's own
// storage view + mount table. Mirrors the wrapper's laws so the customer gets a reason, not a shell error.
func (s *Server) validateBackupTargetMount(ctx context.Context, where string) error {
if s.storage == nil {
return fmt.Errorf("storage view unavailable")
}
// It must currently BE a mountpoint (F-1/F-2). Resolved from the mount table, which is the same
// source the wrapper's `mountpoint -q` consults.
mounts, err := s.hostReader().Mounts()
if err != nil {
return fmt.Errorf("could not read the mount table")
}
var dev string
for _, m := range mounts {
if m.MountPoint == where {
dev = m.Device
break
}
}
if dev == "" {
return fmt.Errorf("%s is not a mountpoint — the backup target must be the drive's own mountpoint", where)
}
// Never the system disk: a target there protects against corruption only, never drive loss.
for _, m := range mounts {
if m.MountPoint == "/" && m.Device == dev {
return fmt.Errorf("%s is on the system disk — a backup target there cannot survive a drive failure", where)
}
}
return nil
}
// setConfiguredBackupTarget rewrites backup.local_backup_target in agent.json.
//
// Read-modify-write over map[string]json.RawMessage so UNKNOWN KEYS ARE PRESERVED VERBATIM — the
// same discipline as pbsdr.seedEscrowStorageID, and the property that made E-1's hand edit safe to
// begin with. A typed round-trip would silently drop any key this build does not know about.
//
// Written IN PLACE (O_TRUNC), not tmp+rename: /etc/felhom-agent is root-owned while agent.json is
// agent-owned 0600, so the non-root agent cannot rename into that directory. A recovery copy is
// parked first, so a torn write is recoverable.
func (s *Server) setConfiguredBackupTarget(id string) error {
path := s.configPath
if path == "" {
return fmt.Errorf("no config path known to this agent (env-only config)")
}
raw, err := os.ReadFile(path)
if err != nil {
return err
}
var doc map[string]json.RawMessage
if err := json.Unmarshal(raw, &doc); err != nil {
return fmt.Errorf("parse %s: %w", path, err)
}
var bk map[string]json.RawMessage
if cur, ok := doc["backup"]; ok {
if err := json.Unmarshal(cur, &bk); err != nil {
return fmt.Errorf("parse backup section: %w", err)
}
} else {
bk = map[string]json.RawMessage{}
}
idJSON, _ := json.Marshal(id)
bk["local_backup_target"] = idJSON
bkJSON, err := json.Marshal(bk)
if err != nil {
return err
}
doc["backup"] = bkJSON
out, err := json.MarshalIndent(doc, "", " ")
if err != nil {
return err
}
st, err := os.Stat(path)
if err != nil {
return err
}
if s.stateDir != "" {
if err := os.MkdirAll(s.stateDir, 0o700); err == nil {
_ = os.WriteFile(filepath.Join(s.stateDir, "agent.json.pre-backup-target"), raw, 0o600)
}
}
f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, st.Mode().Perm())
if err != nil {
return err
}
if _, err := f.Write(out); err != nil {
f.Close()
return err
}
return f.Close()
}
// wrapperReason surfaces the wrapper's own REFUSED line when it produced one — it explains WHY in
// terms the customer can act on ("not a mountpoint", "already exists at …") — falling back to the
// exec error only when stderr said nothing useful.
func wrapperReason(errOut []byte, err error) string {
for _, line := range strings.Split(string(errOut), "\n") {
if strings.Contains(line, "REFUSED:") {
return strings.TrimSpace(line)
}
}
return err.Error()
}
@@ -0,0 +1,174 @@
package localapi
import (
"context"
"encoding/json"
"io"
"log/slog"
"net/http"
"os"
"path/filepath"
"strings"
"testing"
"gitea.dooplex.hu/admin/felhom-agent/internal/storage"
)
// wrapperRunner captures the wrapper vector without ever running sudo. The wrapper IS the security
// boundary, so tests substitute it rather than bypassing it — what is asserted here is the ORDER and
// the ARGUMENTS the agent sends, which is the agent's half of the contract.
type wrapperRunner struct {
calls [][]string
failOn string // verb to fail, "" = all succeed
}
func (r *wrapperRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) {
r.calls = append(r.calls, append([]string{name}, args...))
if len(args) > 0 && args[0] == r.failOn {
return nil, []byte("felhom-backup-target-apply: REFUSED: synthetic " + r.failOn + " failure\n"),
io.ErrUnexpectedEOF
}
return nil, nil, nil
}
func (r *wrapperRunner) verbs() []string {
var out []string
for _, c := range r.calls {
if len(c) > 1 {
out = append(out, c[1])
}
}
return out
}
// moveServer builds a server with /mnt/data mounted on its own device and / on another, plus a
// throwaway agent.json the move can rewrite.
func moveServer(t *testing.T, run *wrapperRunner) (http.Handler, string) {
t.Helper()
dir := t.TempDir()
cfgPath := filepath.Join(dir, "agent.json")
// An UNKNOWN key is deliberately present: the rewrite must preserve it verbatim.
seed := `{"backup":{"local_backup_target":"local","local_backup_retention":3},"some_future_key":{"keep":"me"}}`
if err := os.WriteFile(cfgPath, []byte(seed), 0o600); err != nil {
t.Fatalf("seed config: %v", err)
}
hr := fakeHostReader{mounts: []storage.Mount{
{Device: "/dev/sda1", MountPoint: "/"},
{Device: "/dev/sdb1", MountPoint: "/mnt/data"},
}}
srv, err := NewServer(Options{
ListenAddr: "127.0.0.1:0",
Guests: &fakeGuests{}, Backups: &fakeBackups{}, Store: &fakeStore{}, Storage: fakeStorage{},
Tokens: staticTokens{"A": 8200}, HostReader: hr,
Privileged: run, ConfigPath: cfgPath, StateDir: dir,
Logger: slog.New(slog.NewTextHandler(io.Discard, nil)),
})
if err != nil {
t.Fatalf("new server: %v", err)
}
srv.baseCtx = context.Background()
return srv.Handler(), cfgPath
}
// THE ORDER IS THE CONTRACT: create → grant → config. Reversed, a config pointing at an ungranted
// storage makes every backup 403 on first run, which is precisely what E-1 hit (finding F-3).
func TestBackupTargetMoveOrdersCreateThenGrantThenConfig(t *testing.T) {
run := &wrapperRunner{}
h, cfgPath := moveServer(t, run)
rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data"}`)
if rr.Code != http.StatusOK {
t.Fatalf("move = %d, body %s", rr.Code, rr.Body.String())
}
got := strings.Join(run.verbs(), ",")
if got != "create,grant" {
t.Fatalf("wrapper verbs = %q, want create,grant (in that order)", got)
}
// The config must have been written only AFTER both wrapper calls succeeded.
raw, _ := os.ReadFile(cfgPath)
var doc map[string]json.RawMessage
if err := json.Unmarshal(raw, &doc); err != nil {
t.Fatalf("config unreadable after move: %v", err)
}
var bk map[string]any
_ = json.Unmarshal(doc["backup"], &bk)
if bk["local_backup_target"] != "felhom-backup" {
t.Errorf("local_backup_target = %v, want felhom-backup", bk["local_backup_target"])
}
// Unknown keys preserved verbatim — the property that made E-1's hand edit safe.
if _, ok := doc["some_future_key"]; !ok {
t.Error("the rewrite DROPPED an unknown top-level key — a typed round-trip would do this " +
"and silently discard config this build does not know about")
}
// Sibling keys inside `backup` survive too.
if bk["local_backup_retention"] == nil {
t.Error("the rewrite dropped local_backup_retention from the backup section")
}
}
// A FAILED GRANT MUST NOT LEAVE THE CONFIG POINTING AT THE NEW STORAGE. That state is exactly E-1's
// 403-on-every-backup: the tier looks configured and cannot write.
func TestBackupTargetMoveDoesNotRepointWhenTheGrantFails(t *testing.T) {
run := &wrapperRunner{failOn: "grant"}
h, cfgPath := moveServer(t, run)
rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data"}`)
if rr.Code == http.StatusOK {
t.Fatalf("move SUCCEEDED despite a failed grant (%d)", rr.Code)
}
if !strings.Contains(rr.Body.String(), "403") {
t.Errorf("the error should name the consequence (backups would 403); got %s", rr.Body.String())
}
raw, _ := os.ReadFile(cfgPath)
if strings.Contains(string(raw), "felhom-backup") {
t.Fatal("the config was repointed at a storage the agent cannot write to — every backup " +
"would 403 while the tier reported as configured")
}
}
// It must NOT restart the agent itself. Restarting with a backup in flight cancels the wait and
// records a spurious tier failure for a backup that actually succeeded — E-1 did exactly that to a
// felhom-pbs run. Only the caller can re-check in-flight work immediately before restarting.
func TestBackupTargetMoveReportsRestartRequiredRatherThanRestarting(t *testing.T) {
run := &wrapperRunner{}
h, _ := moveServer(t, run)
rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data"}`)
if !strings.Contains(rr.Body.String(), `"restart_required":true`) {
t.Errorf("response must tell the caller a restart is required; got %s", rr.Body.String())
}
for _, c := range run.calls {
joined := strings.Join(c, " ")
if strings.Contains(joined, "systemctl") || strings.Contains(joined, "restart") {
t.Fatalf("the handler restarted the agent itself: %q — the caller must do it behind its "+
"own in-flight check", joined)
}
}
}
// A path that is not a mountpoint is refused BEFORE sudo is reached (F-1): a subdirectory target
// reports disconnected forever, and an unmounted path silently retargets onto the system drive.
func TestBackupTargetMoveRefusesANonMountpoint(t *testing.T) {
run := &wrapperRunner{}
h, _ := moveServer(t, run)
rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/mnt/data/sub"}`)
if rr.Code == http.StatusOK {
t.Fatal("a non-mountpoint was accepted as the backup target")
}
if len(run.calls) != 0 {
t.Errorf("a refused request still reached the privileged wrapper: %v", run.calls)
}
}
// The system disk is refused: a target there protects against corruption only, never drive loss —
// which is the entire point of the move.
func TestBackupTargetMoveRefusesTheSystemDisk(t *testing.T) {
run := &wrapperRunner{}
h, _ := moveServer(t, run)
rr := do(t, h, http.MethodPost, "/backup/target", "A", `{"vmid":8200,"where":"/"}`)
if rr.Code == http.StatusOK {
t.Fatal("the system disk was accepted as the backup target")
}
if len(run.calls) != 0 {
t.Errorf("a refused request still reached the privileged wrapper: %v", run.calls)
}
}
+21
View File
@@ -34,6 +34,12 @@ type GuestAPI interface {
WaitTask(ctx context.Context, upid string, opts proxmox.WaitOptions) (proxmox.TaskStatus, error)
}
// PrivilegedRunner runs a fenced root wrapper. The seam exists so the backup-target move is testable
// without sudo: the wrapper IS the security boundary, so tests substitute it, never bypass it.
type PrivilegedRunner interface {
Run(ctx context.Context, name string, args ...string) (stdout, stderr []byte, err error)
}
// BackupService enqueues a vzdump/PBS backup of a guest. Satisfied by *backup.BackupRunner.
// BackupWithSnapshotHook (8B.2) invokes onSnapshot once mid-backup when the storage snapshot is
// taken (snapshot mode only) so the controller can resume its app early; in stop mode it is never
@@ -146,6 +152,14 @@ type Options struct {
// NetStorage is the privileged network-mount (NAS) surface (Part A1). OPTIONAL — when nil, the
// /netstorage endpoints report "not configured". Satisfied by *storage.SudoHostOps.
NetStorage NetworkStorageOps
// Privileged runs the fenced root wrappers (E-2a: felhom-backup-target-apply). OPTIONAL — when
// nil, POST /backup/target reports "not configured". Satisfied by *proxmox.ExecRunner.
Privileged PrivilegedRunner
// ConfigPath is agent.json, so the backup-target move can repoint the primary tier. "" (env-only
// config) → the move reports it cannot persist rather than pretending it did.
ConfigPath string
// StateDir is where a pre-write recovery copy of agent.json is parked. "" → no copy is parked.
StateDir string
// SmbCredsDir is where the agent writes the 0600 SMB credentials files (out-of-band). "" →
// /var/lib/felhom-agent/smb-creds.
SmbCredsDir string
@@ -340,6 +354,9 @@ type Server struct {
escrowDone <-chan struct{} // closes when the detached job finishes (tests wait on it)
// ceremonyRun executes the fixed-argv sudo self-invocation (tests inject canned JSON).
ceremonyRun ceremonyRunner
privileged PrivilegedRunner
configPath string
stateDir string
// escrowSudoCheck is the preflight's list-mode grant probe (`sudo -n -l -- <argv>`).
escrowSudoCheck func(ctx context.Context) error
// escrowLookPath resolves a binary on PATH for preflight (tests inject).
@@ -384,6 +401,9 @@ func NewServer(o Options) (*Server, error) {
guestAttach: o.GuestAttach,
mem: o.Memory,
netStorage: o.NetStorage,
privileged: o.Privileged,
configPath: o.ConfigPath,
stateDir: o.StateDir,
netMountRoot: storage.NetworkMountRoot,
smbCredsDir: o.SmbCredsDir,
escrowStagePath: o.EscrowStagePath,
@@ -442,6 +462,7 @@ func (s *Server) Handler() http.Handler {
mux.HandleFunc("POST /backup", s.withGuest(s.handleBackup))
mux.HandleFunc("GET /backup/due", s.withGuest(s.handleBackupDue))
mux.HandleFunc("GET /backup/tiers", s.withGuest(s.handleBackupTiers))
mux.HandleFunc("POST /backup/target", s.withGuest(s.handleSetBackupTarget))
mux.HandleFunc("GET /backup/status", s.withGuest(s.handleBackupStatus))
mux.HandleFunc("GET /restore-test/status", s.withGuest(s.handleRestoreTestStatus))
// Host metrics (slice 9): host-wide health + per-storage capacity for the customer's monitoring