From 69c8b20b783e021d7b8bae29db3e6ff35cda462d Mon Sep 17 00:00:00 2001 From: tomnewton-mulga Date: Wed, 15 Apr 2026 10:58:45 +1000 Subject: [PATCH] fix: harden ISO installer error handling and safety checks - partUUID: error on empty blkid output instead of writing UUID= to fstab - installBootloader: fail if both EFI and BIOS targets fail; track efiErr - machine-id fallback: write at mode 0600 so systemd can commit on first boot - Run: defer unmount so partitions are always cleaned up on failure - copyRootfs: verify critical paths exist after rsync before continuing - resolveNIC: skip virtual/bridge interfaces (docker0, virbr0, ovs-system) - EjectAndReboot: use syscall.Reboot instead of exec(reboot) which needs D-Bus - ui.Run: return error on TTY open failure instead of silent fallback - firstboot script: trap EXIT to always self-disable, preventing retry loops - enableUnit/enableBannerUnit: deduplicate into enableSystemdUnit; check Remove - runAdminBanner: promote service restart failure to slog.Error - parseNodeConf: log non-ENOENT read failures --- cmd/installer/autoinstall/autoinstall.go | 40 ++++++++++++-- cmd/installer/firstboot/firstboot.go | 31 ++++++----- cmd/installer/install/install.go | 68 +++++++++++++++++------- cmd/installer/ui/ui.go | 5 +- cmd/spinifex/cmd/admin.go | 16 +++--- 5 files changed, 114 insertions(+), 46 deletions(-) diff --git a/cmd/installer/autoinstall/autoinstall.go b/cmd/installer/autoinstall/autoinstall.go index ad12a4da0..649f55988 100644 --- a/cmd/installer/autoinstall/autoinstall.go +++ b/cmd/installer/autoinstall/autoinstall.go @@ -29,6 +29,7 @@ import ( "sort" "strconv" "strings" + "syscall" "time" "github.com/mulgadc/spinifex/cmd/installer/install" @@ -63,7 +64,11 @@ func EjectAndReboot() { fmt.Println("Remove the USB drive now if it was not ejected automatically.") fmt.Println("Rebooting in 10 seconds...") time.Sleep(10 * time.Second) - _ = exec.Command("reboot").Run() + // Use the kernel syscall directly — the live environment runs spinifex-init + // as PID 1 (not systemd), so exec("reboot") fails trying to reach D-Bus. + if err := syscall.Reboot(syscall.LINUX_REBOOT_CMD_RESTART); err != nil { + slog.Error("autoinstall: reboot syscall failed", "err", err) + } } func buildConfig() (*install.Config, error) { @@ -286,8 +291,17 @@ func nvmeDisk() (string, error) { } } +// virtualNICPrefixes lists interface name prefixes that identify non-physical +// interfaces (bridges, tunnels, container/OVS veth pairs, etc.). These are +// skipped when auto-selecting a NIC so we don't configure docker0 or +// ovs-system as the management interface on a machine that previously ran +// Docker or OVN. +var virtualNICPrefixes = []string{ + "docker", "veth", "virbr", "br-", "ovs-", "vxlan", "genev", "tun", "tap", +} + // resolveNIC returns the interface name to use. "auto" or empty picks the -// first physical (non-loopback) interface that is not exclude. +// first physical (non-loopback, non-virtual) interface that is not exclude. func resolveNIC(name, exclude string) (string, error) { if name != "" && name != "auto" { return name, nil @@ -300,10 +314,30 @@ func resolveNIC(name, exclude string) (string, error) { if iface.Flags&net.FlagLoopback != 0 { continue } + // Require broadcast capability — filters out point-to-point tunnels. + if iface.Flags&net.FlagBroadcast == 0 { + continue + } + // Require a MAC address — virtual/tunnel interfaces have none. + if len(iface.HardwareAddr) == 0 { + continue + } if iface.Name == exclude { continue } + // Skip known virtual interface prefixes. + virtual := false + for _, pfx := range virtualNICPrefixes { + if strings.HasPrefix(iface.Name, pfx) { + virtual = true + break + } + } + if virtual { + continue + } + slog.Info("autoinstall: NIC resolved", "mode", "auto", "selected", iface.Name) return iface.Name, nil } - return "", fmt.Errorf("no suitable NIC found") + return "", fmt.Errorf("no suitable physical NIC found (non-loopback, broadcast-capable, with MAC)") } diff --git a/cmd/installer/firstboot/firstboot.go b/cmd/installer/firstboot/firstboot.go index e9c9432a2..d5d13218c 100644 --- a/cmd/installer/firstboot/firstboot.go +++ b/cmd/installer/firstboot/firstboot.go @@ -73,6 +73,10 @@ func writeScript(root string, cfg Config) error { # Spinifex firstboot — runs once after ISO installation, then disables itself. set -euo pipefail +# Always disable this service on exit, even on failure, so a partial run +# does not cause an infinite retry loop on subsequent reboots. +trap 'systemctl disable spinifex-firstboot.service' EXIT + # Set hostname hostnamectl set-hostname %s @@ -137,9 +141,6 @@ ln -sf /etc/spinifex/master.key /var/lib/spinifex/awsgw/config/master.key # Start services systemctl start spinifex.target - -# Disable this service so it never runs again -systemctl disable spinifex-firstboot.service `, cfg.Hostname, setupOVN, clusterCmd) path := filepath.Join(root, "usr/local/bin/spinifex-firstboot.sh") @@ -168,18 +169,23 @@ WantedBy=multi-user.target return os.WriteFile(path, []byte(unit), 0o644) } -func enableUnit(root string) error { +func enableSystemdUnit(root, serviceName string) error { wantsDir := filepath.Join(root, "etc/systemd/system/multi-user.target.wants") if err := os.MkdirAll(wantsDir, 0o755); err != nil { return err } - link := filepath.Join(wantsDir, "spinifex-firstboot.service") - target := "/etc/systemd/system/spinifex-firstboot.service" - // Remove stale symlink if present. - _ = os.Remove(link) + link := filepath.Join(wantsDir, serviceName) + target := "/etc/systemd/system/" + serviceName + if err := os.Remove(link); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("remove stale symlink %s: %w", link, err) + } return os.Symlink(target, link) } +func enableUnit(root string) error { + return enableSystemdUnit(root, "spinifex-firstboot.service") +} + func buildClusterCmd(cfg Config) string { switch cfg.ClusterRole { case "join": @@ -214,14 +220,7 @@ WantedBy=multi-user.target } func enableBannerUnit(root string) error { - wantsDir := filepath.Join(root, "etc/systemd/system/multi-user.target.wants") - if err := os.MkdirAll(wantsDir, 0o755); err != nil { - return err - } - link := filepath.Join(wantsDir, "spinifex-banner.service") - target := "/etc/systemd/system/spinifex-banner.service" - _ = os.Remove(link) - return os.Symlink(target, link) + return enableSystemdUnit(root, "spinifex-banner.service") } // writeGettyDropIn holds the primary consoles (tty1 and ttyS0) until diff --git a/cmd/installer/install/install.go b/cmd/installer/install/install.go index 56196aaef..6057da0c4 100644 --- a/cmd/installer/install/install.go +++ b/cmd/installer/install/install.go @@ -40,6 +40,14 @@ func Run(cfg *Config) error { // The live environment may not have /sbin or /usr/sbin in PATH. Set it // explicitly so exec.Command's LookPath finds system binaries like grub-install. _ = os.Setenv("PATH", "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin") + + // Unmount unconditionally on exit so a failed step never leaves partitions + // mounted in the live environment, which would cause a retry to double-mount. + defer func() { + _ = run("umount", efiPart) + _ = run("umount", mountRoot) + }() + steps := []struct { name string fn func() error @@ -54,7 +62,6 @@ func Run(cfg *Config) error { {"write firstboot service", func() error { return firstboot.Write(mountRoot, cfg.toFirstbootConfig()) }}, {"install bootloader", func() error { return installBootloader(cfg.Disk) }}, {"install CA cert", func() error { return installCACert(cfg) }}, - {"unmount", unmount}, } for _, s := range steps { @@ -125,6 +132,20 @@ func copyRootfs() error { return err } + // Verify critical paths exist before proceeding. rsync exits 0 on ENOSPC + // for individual file writes on some filesystems, which would produce a + // partial rootfs that boots into a panic. + critical := []string{ + filepath.Join(mountRoot, "bin/bash"), + filepath.Join(mountRoot, "lib/systemd/systemd"), + filepath.Join(mountRoot, "usr/local/bin/spx"), + } + for _, p := range critical { + if _, err := os.Stat(p); err != nil { + return fmt.Errorf("copyRootfs: critical path missing after rsync (%s): %w", p, err) + } + } + // rsync skips excluded paths entirely — recreate the virtual filesystem // mount points that systemd expects to exist on the installed system. mountPoints := []struct { @@ -156,8 +177,13 @@ func installSpinifex(cfg *Config) error { machineIDPath := filepath.Join(mountRoot, "etc/machine-id") _ = os.Remove(machineIDPath) if err := run("chroot", mountRoot, "systemd-machine-id-setup"); err != nil { - // Fallback: write empty file so systemd generates one on first boot. - _ = os.WriteFile(machineIDPath, []byte(""), 0o444) + // Fallback: write empty file (mode 0600, writable) so systemd-machine-id-commit + // can persist the generated ID on first boot. Mode 0444 would prevent the + // commit and cause the ID to change on every reboot. + slog.Warn("installSpinifex: systemd-machine-id-setup failed, writing uninitialized marker", "err", err) + if writeErr := os.WriteFile(machineIDPath, []byte(""), 0o600); writeErr != nil { + return fmt.Errorf("write machine-id marker: %w", writeErr) + } } // Hostname. @@ -372,7 +398,9 @@ WantedBy=multi-user.target return err } link := filepath.Join(wantsDir, "spinifex-lan-bridge.service") - _ = os.Remove(link) + if err := os.Remove(link); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("remove stale symlink %s: %w", link, err) + } return os.Symlink("/etc/systemd/system/spinifex-lan-bridge.service", link) } @@ -383,23 +411,28 @@ func installBootloader(disk string) error { bootDir := filepath.Join(mountRoot, "boot") efiDir := filepath.Join(mountRoot, "boot", "efi") - if err := run("grub-install", + efiErr := run("grub-install", "--target=x86_64-efi", "--efi-directory="+efiDir, "--boot-directory="+bootDir, "--bootloader-id=spinifex", "--removable", "--recheck", - ); err != nil { - slog.Warn("installBootloader: EFI install failed (may not be EFI system)", "err", err) + ) + if efiErr != nil { + slog.Warn("installBootloader: EFI install failed", "err", efiErr) } - if err := run("grub-install", + if biosErr := run("grub-install", "--target=i386-pc", "--boot-directory="+bootDir, "--recheck", disk, - ); err != nil { - return err + ); biosErr != nil { + if efiErr != nil { + // Both targets failed — the system will not boot. + return fmt.Errorf("both bootloader targets failed (EFI: %v; BIOS: %w)", efiErr, biosErr) + } + return biosErr } // Configure serial console in the installed system's GRUB so the boot menu // and kernel output are visible over serial (ttyS0) as well as VGA. @@ -448,13 +481,6 @@ func installCACert(cfg *Config) error { return run("chroot", mountRoot, "update-ca-certificates") } -func unmount() error { - if err := run("umount", efiPart); err != nil { - return err - } - return run("umount", mountRoot) -} - func reboot() error { // sync filesystems before reboot so nothing is lost. _ = run("sync") @@ -501,9 +527,13 @@ func writeFstab(disk string) error { func partUUID(dev string) (string, error) { out, err := exec.Command("blkid", "-s", "UUID", "-o", "value", dev).Output() if err != nil { - return "", err + return "", fmt.Errorf("blkid %s: %w", dev, err) } - return strings.TrimSpace(string(out)), nil + uuid := strings.TrimSpace(string(out)) + if uuid == "" { + return "", fmt.Errorf("blkid returned no UUID for %s — partition may not have a filesystem yet", dev) + } + return uuid, nil } // partitionPaths returns the EFI and root partition device paths for a given diff --git a/cmd/installer/ui/ui.go b/cmd/installer/ui/ui.go index 4975fd233..133c23eb8 100644 --- a/cmd/installer/ui/ui.go +++ b/cmd/installer/ui/ui.go @@ -167,9 +167,10 @@ func Run(ttyPath string) (*install.Config, error) { if ttyPath != "" { tty, err := os.OpenFile(ttyPath, os.O_RDWR, 0) - if err == nil { - opts = append(opts, tea.WithInput(tty), tea.WithOutput(tty)) + if err != nil { + return nil, fmt.Errorf("open console %s: %w", ttyPath, err) } + opts = append(opts, tea.WithInput(tty), tea.WithOutput(tty)) } p := tea.NewProgram(m, opts...) diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index 7074c0356..a8aad6c31 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -2198,7 +2198,8 @@ func runAdminBanner(cmd *cobra.Command, _ []string) { restartCmd.Stdout = os.Stdout restartCmd.Stderr = os.Stderr if err := restartCmd.Run(); err != nil { - slog.Warn("Failed to restart spinifex.target after IP change", "err", err) + // Services are still bound to the old IP — operator must act. + slog.Error("Failed to restart spinifex.target after IP change — services may be unreachable on new IP", "err", err) } } @@ -2206,11 +2207,11 @@ func runAdminBanner(cmd *cobra.Command, _ []string) { +----------------------------------------------------+ | Spinifex — Mulga Defense Corporation | +----------------------------------------------------+ - | Node: %-40s| - | Login: %-40s| - | Dashboard: %-40s| - | API: %-40s| - | SSH: %-40s| + | Node: %-39s| + | Login: %-39s| + | Dashboard: %-39s| + | API: %-39s| + | SSH: %-39s| +----------------------------------------------------+ | AWS credentials: cat ~/.aws/credentials | +----------------------------------------------------+ @@ -2268,6 +2269,9 @@ func parseNodeConf(path string) map[string]string { result := make(map[string]string) data, err := os.ReadFile(path) if err != nil { + if !os.IsNotExist(err) { + slog.Warn("parseNodeConf: could not read node.conf", "path", path, "err", err) + } return result } for line := range strings.SplitSeq(string(data), "\n") {