fix: group-scope the OVS/OVN sockets and retire the vpcd sudoers grants (#609)

* fix: group-scope the OVS/OVN sockets and stop sudoing read-only probes

The OVS DB socket and both sets of ctl sockets were chmod 0666 by setup-ovn.sh,
with a systemd override re-applying it on every openvswitch-switch restart. Write
access to db.sock is full control of the local datapath, so this handed the
datapath to any account on the box. Both service users run with Group=spinifex,
so the access is now 0660 root:spinifex, /var/run/ovn is 0750, and a shared
helper re-applies it from ExecStartPost on openvswitch-switch and ovn-controller
alike — the latter is new, and matters because the ovn-controller ctl socket name
embeds the pid and is recreated on every start.

That also fixes the health probe reporting ovn-controller as not_running on a
healthy node. It shelled out to `sudo ovn-appctl`, which only spinifex-vpcd is
granted; as spinifex-daemon sudo demanded a password and the swallowed error read
as OVN being down. With the ctl socket group-owned the probe needs no privilege,
so health.go moves to a non-escalating Runner rather than the alternative of
granting the daemon a root-equivalent ovn-appctl rule to read a status.

ListIMDSTaps ran `list-ports` plus one `get` per patch port on vpcd's 15s tick,
so 1+N sudo invocations every tick, each one logging a pam_limits warning about
the core rlimit the unit deliberately pins to 0. It now issues a single JSON
query per pass, and the sudoers drop-in disables the PAM session for both service
users, which also leaves sudo children inheriting the unit's rlimits.

An absent iface-id on a patch port is now skipped with a warning instead of
aborting the pass: it is the only record of the full ENI, so such a port cannot
join the live set either way, and failing the pass stalled reconcile for every
healthy port behind it. A transient query failure still aborts.

* fix: group-own the OVS/OVN pidfiles too, and cover the ipsec ctl socket

Group-owning the ctl sockets was not enough for `ovn-appctl -t ovn-controller`:
it resolves the pid-named socket through the pidfile, and OVS opens that
O_RDWR to test the fcntl liveness lock, so a group-readable pidfile still fails
the probe with EACCES. The pidfiles now get the same 0660 root:spinifex, which
also tightens them from the 0644 they ship with.

openvswitch-ipsec joins the override list because setup-ovn.sh restarts it
after the sweep runs, leaving its ctl socket as the one file still at the
shipped mode.

Verified on the dev node: every socket and pidfile is 0660 root:spinifex and
survives a restart of all three units, an unprivileged user is refused, and
/health reports ovn-controller ok where the sudo probe read not_running.

* refactor: stop escalating the OVS/OVN socket clients

Now that setup-ovn.sh group-owns the control sockets, ovs-vsctl, ovs-appctl,
ovn-nbctl, ovn-sbctl, ovn-appctl and systemctl reach their daemons as the
service user. utils.NeedsPrivilege carries that policy and both command
builders consult it, so ip, iptables, dhcpcd, sysctl and arping still escalate
while the socket clients no longer do. Their sudoers grants are removed: each
took unrestricted args and all of them accept --log-file=PATH, so every one was
a root-equivalent grant handed out to run a query.

ovs-ofctl keeps its grant. It talks to a per-bridge <bridge>.mgmt socket that
ovs-vswitchd creates when the bridge appears, including bridges spinifex
creates at runtime, so the provisioning sweep cannot group-own it up front.
The NB and SB database sockets join the sweep for ovn-nbctl and ovn-sbctl.

vpcd carried its own sudoCommand copy that escalated unconditionally, bypassing
the policy entirely. It survived only because the grants existed and broke the
OVN flows-ready barrier the moment they were removed, so it now delegates to
utils.SudoCommand. TestRG10_SudoOnlyThroughThePolicy fails the build if another
package reintroduces a local copy.

Verified on the dev node: an instance launches, IMDS is served, the flows-ready
barrier completes, and no sudo denial appears anywhere in the journal.

* refactor: drop every spinifex-vpcd sudoers grant for ambient capabilities

vpcd held NOPASSWD grants for ip, iptables, sysctl and arping. The ip grant
alone was root-equivalent: sudoers cannot constrain its arguments, so
`sudo ip netns exec <ns> /bin/sh` was a root shell for the service account.

The unit already grants CAP_NET_ADMIN and CAP_NET_RAW ambiently, and the kernel
passes an ambient set to each child on exec, so those tools work as the service
user with no escalation at all. Verified on a live node: all sixteen ip
subcommands vpcd issues, plus iptables reads and writes, sysctl -w on the
net.ipv4.neigh tree, and arping, all succeed under exactly the unit's ambient
set. spinifex-vpcd now has no sudoers rules.

NeedsPrivilege had to become capability-aware to do this. spx is one binary
shared by both service units, and the daemon holds CAP_SYS_ADMIN and
CAP_DAC_OVERRIDE but no CAP_NET_ADMIN, so a static tool list cannot express
"vpcd runs ip directly, the daemon escalates it". The decision now reads CapAmb
from /proc/self/status once and matches it against the capability each
invocation needs. sysctl is resolved per key, since CAP_NET_ADMIN covers the
net.* trees and nothing else.

The dhcpcd grants go too: neither daemon invokes it. Only setup-ovn.sh, which
runs as the provisioning user, and the in-guest ecs-agent.

TestRG10_VPCDHoldsNoSudoersGrant pins both halves together, so a future grant
cannot be re-added without also explaining why the capability stopped working.

* chore: restore routeTree.gen.ts swept in by mistake

* fix: probe ovn-controller with ovn-appctl, not ovs-appctl

checkOVNController's first probe passed a bare -t ovn-controller to ovs-appctl,
which resolves it against OVS_RUNDIR — /var/run/openvswitch. ovn-controller's
socket and pidfile are in /var/run/ovn, so the probe could never succeed and
logged "ovn-controller.pid: open: No such file or directory" on every check.

Not a functional break: the next probe globs the full /var/run/ovn path and
succeeds, so the function still returned nil. It cost an exec per check and
made a healthy node look like it was failing an OVN liveness probe.

ovn-appctl defaults OVS_RUNDIR to /var/run/ovn, so the common case now matches
first. The glob and systemctl fallbacks stay for older layouts.

build/systemd/spinifex-daemon.service

@@ -25,7 +25,8 @@ Environment=SPINIFEX_WAL_DIR=/var/lib/spinifex/spinifex/
 # Pin to host tmpfs: QEMU pidfiles/QMP sockets must survive PrivateTmp namespace churn (DDIL)
 Environment=XDG_RUNTIME_DIR=/run/spinifex
 
-# RG-9: daemon tier — privileged by necessity (GPU vfio). NoNewPrivileges omitted (RG-10: sudo ip/ovs-vsctl/dhcpcd)
+# RG-9: daemon tier — privileged by necessity (GPU vfio). No CAP_NET_ADMIN here,
+# so unlike vpcd it still escalates ip/sysctl and NoNewPrivileges stays omitted.
 AmbientCapabilities=CAP_SYS_ADMIN CAP_DAC_OVERRIDE
 ProtectSystem=full
 PrivateDevices=no

build/systemd/spinifex-vpcd.service

@@ -20,7 +20,11 @@ Environment=SPINIFEX_BASE_DIR=/var/lib/spinifex/vpcd/
 
 # RG-9: network tier. Per-tap IMDS removed the in-process setns, so vpcd no
 # longer enters a per-VPC netns: CAP_SYS_ADMIN is dropped and the filesystem
-# sandbox is restored. NoNewPrivileges stays off (RG-10: sudo ip/ovs-vsctl/dhcpcd).
+# sandbox is restored.
+#
+# RG-10: these are ambient so ip/iptables/sysctl/arping inherit them on exec and
+# vpcd holds no sudoers rules at all. NoNewPrivileges stays off pending the ADR
+# update that retires the last escalation path.
 AmbientCapabilities=CAP_NET_ADMIN CAP_NET_RAW CAP_NET_BIND_SERVICE CAP_SETUID CAP_SETGID
 CapabilityBoundingSet=CAP_NET_ADMIN CAP_NET_RAW CAP_NET_BIND_SERVICE CAP_SETUID CAP_SETGID
 NoNewPrivileges=no

scripts/setup-ovn.sh

@@ -941,40 +941,109 @@ else
     echo "  WARNING: geneve module not available (tunnels may not work)"
 fi
 
-# --- Step 8: Grant non-root access to OVS/OVN ---
+# --- Step 8: Grant the spinifex service users access to OVS/OVN ---
 echo ""
-echo "Step 8: Configuring non-root access..."
+echo "Step 8: Configuring service-user access..."
 
-# Open OVS DB socket so non-root processes can use ovs-vsctl
-OVS_SOCK="/var/run/openvswitch/db.sock"
-if [ -S "$OVS_SOCK" ]; then
-    sudo chmod 0666 "$OVS_SOCK"
-    echo "  OVS DB socket: opened ($OVS_SOCK)"
+# Group-scoped, never world. spinifex-daemon and spinifex-vpcd both run with
+# Group=spinifex, so 0660 root:spinifex is exactly the reach they need. A
+# world-writable db.sock hands the local datapath — bridges, ports, ovn-remote —
+# to any account on the box, and a world-writable ctl socket lets any account
+# run `ovn-appctl exit`.
+PERMS_HELPER="/usr/local/lib/spinifex/ovs-socket-perms.sh"
+sudo mkdir -p "$(dirname "$PERMS_HELPER")"
+sudo tee "$PERMS_HELPER" >/dev/null <<'HELPER'
+#!/bin/sh
+# Group-own the OVS/OVN control sockets to `spinifex` so the service users reach
+# them without sudo. Driven from ExecStartPost on both openvswitch-switch and
+# ovn-controller: each recreates its own sockets on start, and the ovn-controller
+# ctl socket name embeds the pid, so it is a new file every time.
+#
+# $1 is an optional glob for the caller's own socket, waited on before the sweep.
+set -eu
+GROUP=spinifex
+WAIT_FOR="${1:-}"
+
+if ! getent group "$GROUP" >/dev/null 2>&1; then
+    exit 0
 fi
 
-# Open OVN runtime directory and ctl sockets for ovs-appctl access
-if [ -d "/var/run/ovn" ]; then
-    sudo chmod 0755 /var/run/ovn
-    sudo chmod 0666 /var/run/ovn/*.ctl 2>/dev/null || true
-    echo "  OVN ctl sockets: opened (/var/run/ovn/)"
-fi
-if [ -d "/var/run/openvswitch" ]; then
-    sudo chmod 0666 /var/run/openvswitch/*.ctl 2>/dev/null || true
+# Unquoted so the caller's pattern globs. A caller passing nothing sweeps at once.
+wait_target_present() {
+    if [ -z "$WAIT_FOR" ]; then
+        return 0
+    fi
+    for s in $WAIT_FOR; do
+        if [ -S "$s" ]; then
+            return 0
+        fi
+    done
+    return 1
+}
+
+# ExecStartPost can outrun socket creation, so poll briefly rather than miss the
+# socket and leave the health probe unable to read connection-status.
+i=0
+while [ "$i" -lt 25 ]; do
+    if wait_target_present; then
+        break
+    fi
+    i=$((i + 1))
+    sleep 0.2
+done
+
+# The dir must be traversable by the group before the sockets inside it matter.
+# Only the group changes; the owner keeps full access whoever it is.
+if [ -d /var/run/ovn ]; then
+    chgrp "$GROUP" /var/run/ovn 2>/dev/null || true
+    chmod 0750 /var/run/ovn 2>/dev/null || true
 fi
 
-# Create persistent systemd override so permissions survive OVS restarts
-OVERRIDE_DIR="/etc/systemd/system/openvswitch-switch.service.d"
-if [ ! -f "$OVERRIDE_DIR/spinifex-perms.conf" ]; then
+# ovn??_db.sock are the NB/SB databases themselves, which ovn-nbctl and
+# ovn-sbctl connect to. Bridge <name>.mgmt sockets are deliberately NOT here:
+# ovs-vswitchd creates one whenever a bridge appears, including bridges spinifex
+# creates at runtime, so ovs-ofctl keeps its sudo grant.
+for s in /var/run/openvswitch/db.sock /var/run/openvswitch/*.ctl \
+    /var/run/ovn/*.ctl /var/run/ovn/ovnnb_db.sock /var/run/ovn/ovnsb_db.sock; do
+    if [ -S "$s" ]; then
+        chgrp "$GROUP" "$s" 2>/dev/null || true
+        chmod 0660 "$s" 2>/dev/null || true
+    fi
+done
+
+# Group-WRITE on the pidfiles, not just read: `ovn-appctl -t <daemon>` resolves
+# the pid-named ctl socket through the pidfile, and OVS opens it O_RDWR to test
+# the fcntl liveness lock. Read-only fails the probe with EACCES. This also
+# tightens them from the 0644 they ship with.
+for p in /var/run/openvswitch/*.pid /var/run/ovn/*.pid; do
+    if [ -f "$p" ]; then
+        chgrp "$GROUP" "$p" 2>/dev/null || true
+        chmod 0660 "$p" 2>/dev/null || true
+    fi
+done
+HELPER
+sudo chmod 0755 "$PERMS_HELPER"
+sudo "$PERMS_HELPER"
+echo "  OVS/OVN sockets: 0660 root:spinifex (group-scoped, no world access)"
+
+# Persist across restarts of both daemons. Rewritten unconditionally: an existing
+# file is the old 0666 override, and skipping would leave that exposure in place.
+# openvswitch-ipsec is included because Step 10 restarts it after this sweep,
+# so its ctl socket would otherwise be the one file left at the shipped mode.
+for unit in openvswitch-switch:/var/run/openvswitch/db.sock \
+    "ovn-controller:/var/run/ovn/*.ctl" \
+    "openvswitch-ipsec:/var/run/openvswitch/ovs-monitor-ipsec.*.ctl"; do
+    UNIT="${unit%%:*}"
+    WAIT_GLOB="${unit#*:}"
+    OVERRIDE_DIR="/etc/systemd/system/${UNIT}.service.d"
     sudo mkdir -p "$OVERRIDE_DIR"
-    sudo tee "$OVERRIDE_DIR/spinifex-perms.conf" >/dev/null <<'OVERRIDE'
+    sudo tee "$OVERRIDE_DIR/spinifex-perms.conf" >/dev/null <<OVERRIDE
 [Service]
-ExecStartPost=/bin/chmod 0666 /var/run/openvswitch/db.sock
+ExecStartPost=$PERMS_HELPER "$WAIT_GLOB"
 OVERRIDE
-    sudo systemctl daemon-reload
-    echo "  systemd override: created (db.sock permissions persist across restarts)"
-else
-    echo "  systemd override: already exists"
-fi
+    echo "  systemd override: ${UNIT}.service.d/spinifex-perms.conf"
+done
+sudo systemctl daemon-reload
 
 # Sudoers rules for spinifex-daemon and spinifex-vpcd are managed by setup.sh
 # (install_sudoers). Skip writing here to avoid conflicts.

scripts/setup.sh

@@ -184,31 +184,39 @@ create_service_users() {
 install_sudoers() {
     stage "installing scoped sudoers rules"
     $SUDO tee /etc/sudoers.d/spinifex-network > /dev/null << 'SUDOERS'
-# Spinifex daemon: tap devices, OVS bridge management, and DHCP for external IPs
-# ovs-ofctl installs the per-tap IMDS datapath flows on br-imds; sysctl sets the
-# per-endpoint rp_filter/accept_local the asymmetric reply path needs.
-spinifex-daemon ALL=(root) NOPASSWD: /sbin/ip, /usr/sbin/ip
-spinifex-daemon ALL=(root) NOPASSWD: /usr/bin/ovs-vsctl, /usr/bin/ovs-appctl, /usr/bin/ovs-ofctl
-spinifex-daemon ALL=(root) NOPASSWD: /usr/sbin/sysctl -qw net.ipv4.conf.*
-spinifex-daemon ALL=(root) NOPASSWD: /usr/sbin/dhcpcd
-spinifex-daemon ALL=(root) NOPASSWD: /usr/bin/systemctl is-active openvswitch-ipsec.service
-spinifex-daemon ALL=(root) NOPASSWD: /usr/bin/ovn-nbctl set NB_Global . ipsec=true
+# No PAM session for the service users. Every sudo call otherwise logs
+# "pam_limits: Could not set limit for 'core'": the units set LimitCORE=0 and
+# their CapabilityBoundingSet omits CAP_SYS_RESOURCE, so pam_limits cannot apply
+# the limits.conf default and warns. It also means sudo children inherit the
+# unit's rlimits rather than the system defaults, which is the RG-9 intent.
+Defaults:spinifex-daemon !pam_session
 
-# Spinifex VPC daemon: OVN and OVS read/write, OVN controller status check and DHCP
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/dhcpcd
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/bin/ovs-vsctl, /usr/bin/ovs-appctl
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/bin/ovn-nbctl, /usr/bin/ovn-sbctl, /usr/bin/ovn-appctl
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/bin/systemctl is-active --quiet ovn-controller
-spinifex-vpcd ALL=(root) NOPASSWD: /sbin/ip, /usr/sbin/ip
-# Routed-NAT mode: transit masquerade + per-EIP FORWARD accepts, proxy-ARP
-# delay tune, and gratuitous ARP announcements for host-delivered EIPs.
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/iptables, /sbin/iptables
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/sysctl -w net.ipv4.neigh.*
-spinifex-vpcd ALL=(root) NOPASSWD: /usr/sbin/arping, /usr/bin/arping
+# The OVS/OVN client tools are deliberately absent. They do all their work over
+# control sockets that setup-ovn.sh group-owns to `spinifex`, so they run as the
+# service user. Granting them would be worse than pointless: a NOPASSWD rule for
+# them takes unrestricted args, and ovs-vsctl/ovn-nbctl/ovn-appctl all accept
+# --log-file=PATH, which writes a root-owned file wherever the caller points it.
+#
+# ovs-ofctl is the exception that keeps its grant: it talks to a per-bridge
+# <bridge>.mgmt socket created by ovs-vswitchd when the bridge appears, including
+# bridges spinifex creates at runtime, so it cannot be group-owned up front.
+
+# spinifex-vpcd has no rules at all. Its ip/iptables/sysctl/arping work is done
+# under the CAP_NET_ADMIN and CAP_NET_RAW its unit grants ambiently, which the
+# kernel passes to each child on exec. Those grants were also root-equivalent:
+# `sudo ip netns exec <ns> /bin/sh` is a root shell.
+
+# Spinifex daemon: tap devices and OpenFlow rules. Its unit holds no
+# CAP_NET_ADMIN, so unlike vpcd it cannot drop these. ovs-ofctl installs the
+# per-tap IMDS datapath flows on br-imds; sysctl sets the per-endpoint
+# rp_filter/accept_local the asymmetric reply path needs.
+spinifex-daemon ALL=(root) NOPASSWD: /sbin/ip, /usr/sbin/ip
+spinifex-daemon ALL=(root) NOPASSWD: /usr/bin/ovs-ofctl
+spinifex-daemon ALL=(root) NOPASSWD: /usr/sbin/sysctl -qw net.ipv4.conf.*
 SUDOERS
     $SUDO chmod 0440 /etc/sudoers.d/spinifex-network
     $SUDO visudo -cf /etc/sudoers.d/spinifex-network || fatal "Invalid sudoers syntax in spinifex-network"
-    info "Scoped sudoers rules installed for spinifex-daemon and spinifex-vpcd"
+    info "Scoped sudoers rules installed for spinifex-daemon (spinifex-vpcd needs none)"
 }
 
 # --- Install apt dependencies ---

spinifex/network/host/health.go

@@ -1,9 +1,8 @@
 package host
 
 import (
+	"context"
 	"strings"
-
-	"github.com/mulgadc/spinifex/spinifex/utils"
 )
 
 // OVNHealth reports the readiness of OVN networking on this compute node.
@@ -17,9 +16,18 @@ type OVNHealth struct {
 
 // HealthStatus probes local OVS/OVN state to determine network readiness.
 func HealthStatus() OVNHealth {
+	return healthStatus(context.Background(), NewExecRunner())
+}
+
+// healthStatus is HealthStatus over an injected Runner. Every probe here is
+// read-only and runs unprivileged: ovs-vsctl and ovn-appctl are socket clients
+// (utils.NeedsPrivilege), and db.sock plus the ovn-controller ctl socket are
+// group-owned by `spinifex`. Escalating instead would mean granting the daemon a
+// root-equivalent ovn-appctl rule to read a status.
+func healthStatus(ctx context.Context, r Runner) OVNHealth {
 	status := OVNHealth{}
 
-	if err := utils.SudoCommand("ovs-vsctl", "br-exists", "br-int").Run(); err == nil {
+	if _, err := r.Run(ctx, "ovs-vsctl", "br-exists", "br-int"); err == nil {
 		status.BrIntExists = true
 	}
 
@@ -33,19 +41,24 @@ func HealthStatus() OVNHealth {
 	// true only when the process is up AND synced with the SB RAFT cluster, so a
 	// stale-SB wedge correctly surfaces as not_running instead of hiding behind a
 	// process-alive check.
-	if out, err := utils.SudoCommand("ovn-appctl", "-t", "ovn-controller", "connection-status").Output(); err == nil {
+	if out, err := r.Run(ctx, "ovn-appctl", "-t", "ovn-controller", "connection-status"); err == nil {
 		status.OVNControllerUp = strings.TrimSpace(string(out)) == "connected"
 	}
 
-	if out, err := utils.SudoCommand("ovs-vsctl", "get", "Open_vSwitch", ".", "external_ids:system-id").CombinedOutput(); err == nil {
-		status.ChassisID = strings.Trim(strings.TrimSpace(string(out)), "\"")
-	}
-	if out, err := utils.SudoCommand("ovs-vsctl", "get", "Open_vSwitch", ".", "external_ids:ovn-encap-ip").CombinedOutput(); err == nil {
-		status.EncapIP = strings.Trim(strings.TrimSpace(string(out)), "\"")
-	}
-	if out, err := utils.SudoCommand("ovs-vsctl", "get", "Open_vSwitch", ".", "external_ids:ovn-remote").CombinedOutput(); err == nil {
-		status.OVNRemote = strings.Trim(strings.TrimSpace(string(out)), "\"")
-	}
+	status.ChassisID = ovsGlobalExternalID(ctx, r, "system-id")
+	status.EncapIP = ovsGlobalExternalID(ctx, r, "ovn-encap-ip")
+	status.OVNRemote = ovsGlobalExternalID(ctx, r, "ovn-remote")
 
 	return status
 }
+
+// ovsGlobalExternalID reads one Open_vSwitch external_ids key, returning "" when
+// it is unset or unreadable — the field is omitted from the report rather than
+// carrying an error string into it.
+func ovsGlobalExternalID(ctx context.Context, r Runner, key string) string {
+	out, err := r.Run(ctx, "ovs-vsctl", "get", "Open_vSwitch", ".", "external_ids:"+key)
+	if err != nil {
+		return ""
+	}
+	return strings.Trim(strings.TrimSpace(string(out)), "\"")
+}

spinifex/network/host/health_test.go

@@ -0,0 +1,118 @@
+package host
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/mulgadc/spinifex/spinifex/utils"
+)
+
+const (
+	brExistsCmd     = "ovs-vsctl br-exists br-int"
+	connStatusCmd   = "ovn-appctl -t ovn-controller connection-status"
+	systemIDCmd     = "ovs-vsctl get Open_vSwitch . external_ids:system-id"
+	encapIPCmd      = "ovs-vsctl get Open_vSwitch . external_ids:ovn-encap-ip"
+	ovnRemoteCmd    = "ovs-vsctl get Open_vSwitch . external_ids:ovn-remote"
+	healthyChassis  = "hydrogen"
+	healthyEncapIP  = "192.168.1.13"
+	healthyOVNRemte = "tcp:192.168.1.13:6642"
+)
+
+// stubHealthyOVN primes every probe with the output of a node where OVN is up.
+func stubHealthyOVN(s *stubRunner) {
+	s.expect(brExistsCmd, nil, nil)
+	s.expect(connStatusCmd, []byte("connected\n"), nil)
+	s.expect(systemIDCmd, []byte("\""+healthyChassis+"\"\n"), nil)
+	s.expect(encapIPCmd, []byte("\""+healthyEncapIP+"\"\n"), nil)
+	s.expect(ovnRemoteCmd, []byte("\""+healthyOVNRemte+"\"\n"), nil)
+}
+
+func TestHealthStatus_HealthyNode(t *testing.T) {
+	s := newStubRunner()
+	stubHealthyOVN(s)
+
+	got := healthStatus(context.Background(), s)
+
+	if !got.BrIntExists {
+		t.Error("BrIntExists = false, want true")
+	}
+	if !got.OVNControllerUp {
+		t.Error("OVNControllerUp = false, want true")
+	}
+	if got.ChassisID != healthyChassis {
+		t.Errorf("ChassisID = %q, want %q", got.ChassisID, healthyChassis)
+	}
+	if got.EncapIP != healthyEncapIP {
+		t.Errorf("EncapIP = %q, want %q", got.EncapIP, healthyEncapIP)
+	}
+	if got.OVNRemote != healthyOVNRemte {
+		t.Errorf("OVNRemote = %q, want %q", got.OVNRemote, healthyOVNRemte)
+	}
+}
+
+// No probe may escalate. The false-negative this replaced came from probing
+// ovn-appctl through sudo as spinifex-daemon, which holds no ovn-appctl grant:
+// sudo demanded a password, the error was swallowed, and a healthy node reported
+// ovn-controller as not_running. The sockets are group-owned instead, so a sudo
+// prefix reappearing here would resurrect that bug.
+func TestHealthStatus_ProbesDoNotEscalate(t *testing.T) {
+	s := newStubRunner()
+	stubHealthyOVN(s)
+
+	healthStatus(context.Background(), s)
+
+	if len(s.calls) == 0 {
+		t.Fatal("healthStatus issued no probes")
+	}
+	for _, c := range s.calls {
+		if strings.HasPrefix(c, "sudo ") {
+			t.Errorf("probe %q escalates; OVS/OVN read probes must run unprivileged", c)
+		}
+	}
+}
+
+// ovn-controller up but not synced with the SB cluster is not readiness: a
+// stale-SB wedge must surface as down rather than hide behind process liveness.
+func TestHealthStatus_UnsyncedControllerIsDown(t *testing.T) {
+	s := newStubRunner()
+	stubHealthyOVN(s)
+	s.expect(connStatusCmd, []byte("not connected\n"), nil)
+
+	if healthStatus(context.Background(), s).OVNControllerUp {
+		t.Error("OVNControllerUp = true for a controller that is not connected to the SB cluster")
+	}
+}
+
+// A probe that cannot run reports its field as unset, never as healthy.
+func TestHealthStatus_FailedProbesReportUnset(t *testing.T) {
+	s := newStubRunner()
+	s.expect(brExistsCmd, nil, errors.New("no such bridge"))
+	s.expect(connStatusCmd, nil, errors.New("no ovn-controller socket"))
+	s.expect(systemIDCmd, nil, errors.New("ovsdb unreachable"))
+	s.expect(encapIPCmd, nil, errors.New("ovsdb unreachable"))
+	s.expect(ovnRemoteCmd, nil, errors.New("ovsdb unreachable"))
+
+	got := healthStatus(context.Background(), s)
+
+	if got != (OVNHealth{}) {
+		t.Errorf("healthStatus = %+v on a node where every probe failed, want the zero value", got)
+	}
+}
+
+// Every tool healthStatus probes must be one the escalation policy classifies as
+// unprivileged, or the probe would need a sudoers grant to read a status.
+func TestHealthStatus_ProbesAreSocketClients(t *testing.T) {
+	s := newStubRunner()
+	stubHealthyOVN(s)
+
+	healthStatus(context.Background(), s)
+
+	for _, c := range s.calls {
+		tool := strings.Fields(c)[0]
+		if utils.NeedsPrivilege(tool) {
+			t.Errorf("probe tool %q is classified as needing privilege; health must only use socket clients", tool)
+		}
+	}
+}

spinifex/network/host/imds_reconcile.go

@@ -2,6 +2,7 @@ package host
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"log/slog"
 	"strings"
@@ -21,45 +22,120 @@ type IMDSTapEndpoint struct {
 
 // ListIMDSTaps enumerates the local primary-ENI IMDS datapaths from live OVS state,
 // the source vpcd reconciles its responders against. The br-int patch ports ("imi-*")
-// carry the OVN iface-id, the only place the full ENI survives on the chassis. A
-// malformed iface-id is skipped; an unreadable one aborts the pass so a transient
-// read error can't drop a live tap and make reconcile stop its healthy responder.
+// carry the OVN iface-id, the only place the full ENI survives on the chassis.
+//
+// One query serves the whole pass: this runs on vpcd's 15s tick, and a per-port
+// query was 1+N sudo invocations every tick. The imi- prefix is unambiguous (the
+// br-imds patch end is "imp-", the endpoint "ime-"), so filtering every Interface
+// on the prefix is equivalent to enumerating br-int and filtering. A failed query
+// aborts the pass so a transient OVS error can't drop a live tap and make reconcile
+// stop its healthy responder.
 func ListIMDSTaps(ctx context.Context, r Runner) ([]IMDSTapEndpoint, error) {
-	out, err := r.Run(ctx, "ovs-vsctl", "list-ports", "br-int")
+	out, err := r.Run(ctx, "ovs-vsctl", "--format=json", "--columns=name,external_ids", "list", "Interface")
 	if err != nil {
-		return nil, fmt.Errorf("list br-int ports: %w", err)
+		return nil, fmt.Errorf("list OVS interfaces: %w", err)
+	}
+	rows, err := parseOVSInterfaceRows(out)
+	if err != nil {
+		return nil, err
 	}
 	var taps []IMDSTapEndpoint
-	for port := range strings.FieldsSeq(string(out)) {
-		if !strings.HasPrefix(port, imdsIntPatchPrefix) {
+	for _, row := range rows {
+		if !strings.HasPrefix(row.name, imdsIntPatchPrefix) {
 			continue
 		}
-		eniID, err := imdsPatchENI(ctx, r, port)
-		if err != nil {
-			// Abort rather than drop a live tap: a transient read error would make
-			// reconcile treat its healthy responder as stale and stop it.
-			return nil, fmt.Errorf("read iface-id for %s: %w", port, err)
-		}
-		if eniID == "" {
-			slog.Warn("IMDS: skipping IMDS patch port with unexpected iface-id", "port", port)
+		// Skip rather than abort: the iface-id is the only record of the full ENI, so
+		// a port missing it can never be a member of the live set either way, and one
+		// structurally broken port must not stall every reconcile pass.
+		ifaceID, ok := row.externalIDs["iface-id"]
+		if !ok || !strings.HasPrefix(ifaceID, ovsIfaceIDPrefix) {
+			slog.Warn("IMDS: skipping IMDS patch port with unexpected iface-id", "port", row.name, "iface_id", ifaceID)
 			continue
 		}
+		eniID := strings.TrimPrefix(ifaceID, ovsIfaceIDPrefix)
 		taps = append(taps, IMDSTapEndpoint{ENIID: eniID, Endpoint: IMDSEndpointName(eniID)})
 	}
 	return taps, nil
 }
 
-// imdsPatchENI reads a br-int IMDS patch port's iface-id and recovers the full
-// ENI. Returns "" (not an error) when the iface-id is missing the expected
-// "port-" prefix, so the caller skips rather than aborts.
-func imdsPatchENI(ctx context.Context, r Runner, port string) (string, error) {
-	out, err := r.Run(ctx, "ovs-vsctl", "get", "Interface", port, "external_ids:iface-id")
-	if err != nil {
-		return "", err
-	}
-	ifaceID := strings.Trim(strings.TrimSpace(string(out)), `"`)
-	if !strings.HasPrefix(ifaceID, ovsIfaceIDPrefix) {
-		return "", nil
-	}
-	return strings.TrimPrefix(ifaceID, ovsIfaceIDPrefix), nil
+// ovsInterfaceRow is one interface's name and external_ids from an ovs-vsctl list.
+type ovsInterfaceRow struct {
+	name        string
+	externalIDs map[string]string
+}
+
+// ovsListResult is the `ovs-vsctl --format=json list` envelope. Cells stay raw
+// because a column's shape follows its schema type: a scalar is a bare JSON
+// value, a map is the two-element array ["map", [[k, v], ...]].
+type ovsListResult struct {
+	Headings []string            `json:"headings"`
+	Data     [][]json.RawMessage `json:"data"`
+}
+
+// parseOVSInterfaceRows decodes `ovs-vsctl --format=json --columns=name,external_ids
+// list Interface`. Column positions come from the headings rather than being
+// assumed to follow the --columns order.
+func parseOVSInterfaceRows(out []byte) ([]ovsInterfaceRow, error) {
+	var result ovsListResult
+	if err := json.Unmarshal(out, &result); err != nil {
+		return nil, fmt.Errorf("parse ovs-vsctl JSON: %w", err)
+	}
+	nameCol, extCol := -1, -1
+	for i, h := range result.Headings {
+		switch h {
+		case "name":
+			nameCol = i
+		case "external_ids":
+			extCol = i
+		}
+	}
+	if nameCol < 0 || extCol < 0 {
+		return nil, fmt.Errorf("ovs-vsctl JSON missing name/external_ids columns, got %v", result.Headings)
+	}
+	rows := make([]ovsInterfaceRow, 0, len(result.Data))
+	for _, cells := range result.Data {
+		if nameCol >= len(cells) || extCol >= len(cells) {
+			return nil, fmt.Errorf("ovs-vsctl JSON row has %d cells, want at least %d", len(cells), max(nameCol, extCol)+1)
+		}
+		var name string
+		if err := json.Unmarshal(cells[nameCol], &name); err != nil {
+			return nil, fmt.Errorf("parse interface name: %w", err)
+		}
+		ids, err := parseOVSMap(cells[extCol])
+		if err != nil {
+			return nil, fmt.Errorf("parse external_ids for %s: %w", name, err)
+		}
+		rows = append(rows, ovsInterfaceRow{name: name, externalIDs: ids})
+	}
+	return rows, nil
+}
+
+// parseOVSMap decodes an OVSDB map cell, serialised as ["map", [[key, value], ...]].
+func parseOVSMap(cell json.RawMessage) (map[string]string, error) {
+	var wrapper []json.RawMessage
+	if err := json.Unmarshal(cell, &wrapper); err != nil {
+		return nil, err
+	}
+	if len(wrapper) != 2 {
+		return nil, fmt.Errorf("expected a 2-element map cell, got %d elements", len(wrapper))
+	}
+	var kind string
+	if err := json.Unmarshal(wrapper[0], &kind); err != nil {
+		return nil, err
+	}
+	if kind != "map" {
+		return nil, fmt.Errorf("expected a map cell, got %q", kind)
+	}
+	var pairs [][]string
+	if err := json.Unmarshal(wrapper[1], &pairs); err != nil {
+		return nil, err
+	}
+	ids := make(map[string]string, len(pairs))
+	for _, p := range pairs {
+		if len(p) != 2 {
+			return nil, fmt.Errorf("expected a key/value pair, got %d elements", len(p))
+		}
+		ids[p[0]] = p[1]
+	}
+	return ids, nil
 }

spinifex/network/host/imds_reconcile_test.go

@@ -2,6 +2,7 @@ package host
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"strings"
 	"testing"
@@ -9,24 +10,58 @@ import (
 	"github.com/mulgadc/spinifex/spinifex/vm"
 )
 
-// ListIMDSTaps must enumerate only the IMDS patch ports (imi-*) on br-int,
-// recover the *full* ENI from each port's iface-id ("port-<eniID>"), and pair it
-// with the br-imds endpoint name — ignoring guest taps and the br-imds-end patch.
+// listInterfacesCmd is the single query ListIMDSTaps issues per pass.
+const listInterfacesCmd = "ovs-vsctl --format=json --columns=name,external_ids list Interface"
+
+// ovsListJSON renders the `ovs-vsctl --format=json list` envelope for a set of
+// interfaces, so the tests exercise the real decode path rather than a hand-typed
+// blob. A nil external_ids renders as OVSDB's empty map.
+func ovsListJSON(t *testing.T, ifaces []struct {
+	name string
+	ids  map[string]string
+},
+) []byte {
+	t.Helper()
+	data := make([]any, 0, len(ifaces))
+	for _, iface := range ifaces {
+		pairs := make([][]string, 0, len(iface.ids))
+		for k, v := range iface.ids {
+			pairs = append(pairs, []string{k, v})
+		}
+		data = append(data, []any{iface.name, []any{"map", pairs}})
+	}
+	out, err := json.Marshal(map[string]any{
+		"headings": []string{"name", "external_ids"},
+		"data":     data,
+	})
+	if err != nil {
+		t.Fatalf("marshal ovs-vsctl JSON: %v", err)
+	}
+	return out
+}
+
+// ListIMDSTaps must enumerate only the IMDS patch ports (imi-*), recover the
+// *full* ENI from each port's iface-id ("port-<eniID>"), and pair it with the
+// br-imds endpoint name — ignoring guest taps, the br-imds-end patch and the
+// endpoint port, all of which are in the same unfiltered interface list.
 func TestListIMDSTaps_RecoversENIFromPatchIfaceID(t *testing.T) {
 	const (
 		eniA = "eni-0aaa1111deadbeef"
 		eniB = "eni-0bbb2222cafef00d"
 	)
 	s := newStubRunner()
-	// br-int carries the two IMDS patch ports plus a guest tap and the br-imds-end
-	// patch name (which lives on br-imds, not br-int, but guards the imi- filter).
-	s.expect("ovs-vsctl list-ports br-int",
-		[]byte(IMDSIntPatchPort(eniA)+"\ntap0deadbeef\n"+IMDSIntPatchPort(eniB)+"\n"+IMDSPatchPort(eniA)+"\n"), nil)
-	// ovs-vsctl quotes the iface-id value and appends a newline.
-	s.expect("ovs-vsctl get Interface "+IMDSIntPatchPort(eniA)+" external_ids:iface-id",
-		[]byte("\""+vm.OVSIfaceID(eniA)+"\"\n"), nil)
-	s.expect("ovs-vsctl get Interface "+IMDSIntPatchPort(eniB)+" external_ids:iface-id",
-		[]byte("\""+vm.OVSIfaceID(eniB)+"\"\n"), nil)
+	s.expect(listInterfacesCmd, ovsListJSON(t, []struct {
+		name string
+		ids  map[string]string
+	}{
+		{name: IMDSIntPatchPort(eniA), ids: map[string]string{"iface-id": vm.OVSIfaceID(eniA), "attached-mac": "52:54:00:aa:bb:cc"}},
+		{name: "tap0deadbeef"},
+		{name: IMDSIntPatchPort(eniB), ids: map[string]string{"iface-id": vm.OVSIfaceID(eniB)}},
+		// The br-imds patch end and the endpoint carry neighbouring prefixes; both
+		// must fall outside the imi- filter.
+		{name: IMDSPatchPort(eniA)},
+		{name: IMDSEndpointName(eniA)},
+	}), nil)
 
 	taps, err := ListIMDSTaps(context.Background(), s)
 	if err != nil {
@@ -49,30 +84,56 @@ func TestListIMDSTaps_RecoversENIFromPatchIfaceID(t *testing.T) {
 			t.Errorf("endpoint for %s = %q, want %q", eni, got[eni], ep)
 		}
 	}
-	// The guest tap and the br-imds-end patch must never be queried.
-	if s.called("ovs-vsctl get Interface tap0deadbeef") {
-		t.Error("guest tap must not be queried for an iface-id")
+}
+
+// The pass costs exactly one OVS invocation however many taps are live. It runs
+// on vpcd's 15s tick, and the per-port query it replaced was 1+N sudo calls a
+// tick — the bulk of the service journal.
+func TestListIMDSTaps_IssuesOneQueryPerPass(t *testing.T) {
+	ifaces := make([]struct {
+		name string
+		ids  map[string]string
+	}, 0, 4)
+	for _, eni := range []string{"eni-01", "eni-02", "eni-03", "eni-04"} {
+		ifaces = append(ifaces, struct {
+			name string
+			ids  map[string]string
+		}{name: IMDSIntPatchPort(eni), ids: map[string]string{"iface-id": vm.OVSIfaceID(eni)}})
 	}
-	if s.called("ovs-vsctl get Interface " + IMDSPatchPort(eniA)) {
-		t.Error("br-imds-end patch must not be queried for an iface-id")
+	s := newStubRunner()
+	s.expect(listInterfacesCmd, ovsListJSON(t, ifaces), nil)
+
+	taps, err := ListIMDSTaps(context.Background(), s)
+	if err != nil {
+		t.Fatalf("ListIMDSTaps: %v", err)
+	}
+	if len(taps) != 4 {
+		t.Fatalf("ListIMDSTaps returned %d taps, want 4", len(taps))
+	}
+	if n := len(s.calls); n != 1 {
+		t.Fatalf("ListIMDSTaps issued %d commands, want exactly 1: %v", n, s.calls)
 	}
 }
 
-// A patch port whose iface-id is unexpected (missing the "port-" prefix) is
-// skipped, not fatal: one malformed port must not stall serving for the rest.
+// A patch port whose iface-id is unexpected — malformed (missing the "port-"
+// prefix) or absent entirely — is skipped, not fatal. The iface-id is the only
+// record of the full ENI, so such a port cannot join the live set either way, and
+// aborting would stall every pass for the healthy ports behind it.
 func TestListIMDSTaps_SkipsUnexpectedIfaceID(t *testing.T) {
 	const (
-		eniOK    = "eni-0aaa1111deadbeef"
-		eniWrong = "eni-0ccc3333beeff00d"
+		eniOK      = "eni-0aaa1111deadbeef"
+		eniWrong   = "eni-0ccc3333beeff00d"
+		eniMissing = "eni-0ddd4444f00dcafe"
 	)
 	s := newStubRunner()
-	s.expect("ovs-vsctl list-ports br-int",
-		[]byte(IMDSIntPatchPort(eniOK)+"\n"+IMDSIntPatchPort(eniWrong)+"\n"), nil)
-	s.expect("ovs-vsctl get Interface "+IMDSIntPatchPort(eniOK)+" external_ids:iface-id",
-		[]byte("\""+vm.OVSIfaceID(eniOK)+"\"\n"), nil)
-	// Missing the "port-" prefix — recovered ENI would be wrong, so it is dropped.
-	s.expect("ovs-vsctl get Interface "+IMDSIntPatchPort(eniWrong)+" external_ids:iface-id",
-		[]byte("\"bogus\"\n"), nil)
+	s.expect(listInterfacesCmd, ovsListJSON(t, []struct {
+		name string
+		ids  map[string]string
+	}{
+		{name: IMDSIntPatchPort(eniOK), ids: map[string]string{"iface-id": vm.OVSIfaceID(eniOK)}},
+		{name: IMDSIntPatchPort(eniWrong), ids: map[string]string{"iface-id": "bogus"}},
+		{name: IMDSIntPatchPort(eniMissing), ids: map[string]string{"attached-mac": "52:54:00:aa:bb:cc"}},
+	}), nil)
 
 	taps, err := ListIMDSTaps(context.Background(), s)
 	if err != nil {
@@ -83,28 +144,6 @@ func TestListIMDSTaps_SkipsUnexpectedIfaceID(t *testing.T) {
 	}
 }
 
-// A patch port whose iface-id cannot be READ aborts the reconcile pass rather
-// than silently dropping the tap. Dropping it would leave the ENI out of the live
-// set, so reconcile would treat the guest's healthy responder as stale and stop
-// it — flapping IMDS on a transient OVS read error. The caller retries next tick.
-func TestListIMDSTaps_UnreadableIfaceIDAbortsPass(t *testing.T) {
-	const (
-		eniOK  = "eni-0aaa1111deadbeef"
-		eniErr = "eni-0bbb2222cafef00d"
-	)
-	s := newStubRunner()
-	s.expect("ovs-vsctl list-ports br-int",
-		[]byte(IMDSIntPatchPort(eniOK)+"\n"+IMDSIntPatchPort(eniErr)+"\n"), nil)
-	s.expect("ovs-vsctl get Interface "+IMDSIntPatchPort(eniOK)+" external_ids:iface-id",
-		[]byte("\""+vm.OVSIfaceID(eniOK)+"\"\n"), nil)
-	s.expect("ovs-vsctl get Interface "+IMDSIntPatchPort(eniErr)+" external_ids:iface-id",
-		nil, errors.New("ovs-vsctl: connection refused"))
-
-	if _, err := ListIMDSTaps(context.Background(), s); err == nil {
-		t.Fatal("expected error when a patch port's iface-id cannot be read; a silent drop would stop the tap's healthy responder")
-	}
-}
-
 // TestOVSIfaceIDPrefixMatchesVM keeps the "port-" prefix ListIMDSTaps strips to
 // recover the full ENI in sync with the prefix vm.OVSIfaceID / topology.Port
 // prepend. host inlines the constant (it cannot import the value without an
@@ -118,12 +157,42 @@ func TestOVSIfaceIDPrefixMatchesVM(t *testing.T) {
 	}
 }
 
-// A failure listing br-int ports is fatal to one reconcile pass (the caller
-// retries on the next tick), not silently empty.
-func TestListIMDSTaps_ListPortsErrorPropagates(t *testing.T) {
+// A failed query is fatal to one reconcile pass (the caller retries on the next
+// tick), never silently empty. Returning no taps would make reconcile treat every
+// guest's healthy responder as stale and stop it on a transient OVS error.
+func TestListIMDSTaps_QueryErrorPropagates(t *testing.T) {
 	s := newStubRunner()
-	s.expect("ovs-vsctl list-ports br-int", nil, errors.New("ovs down"))
+	s.expect(listInterfacesCmd, nil, errors.New("ovs down"))
 	if _, err := ListIMDSTaps(context.Background(), s); err == nil {
-		t.Fatal("expected error when list-ports fails")
+		t.Fatal("expected error when the interface query fails; a silent empty result would stop every healthy responder")
+	}
+}
+
+// Unparseable output is an error too, for the same reason: it must not decode to
+// an empty live set.
+func TestListIMDSTaps_MalformedJSONPropagates(t *testing.T) {
+	s := newStubRunner()
+	s.expect(listInterfacesCmd, []byte("not json"), nil)
+	if _, err := ListIMDSTaps(context.Background(), s); err == nil {
+		t.Fatal("expected error on unparseable ovs-vsctl output")
+	}
+}
+
+// Column positions are read from the headings, not assumed to follow the
+// --columns order, so a reordering by ovs-vsctl cannot silently swap the fields.
+func TestParseOVSInterfaceRows_HonoursHeadingOrder(t *testing.T) {
+	out := []byte(`{"headings":["external_ids","name"],"data":[[["map",[["iface-id","port-eni-1"]]],"imi-abc"]]}`)
+	rows, err := parseOVSInterfaceRows(out)
+	if err != nil {
+		t.Fatalf("parseOVSInterfaceRows: %v", err)
+	}
+	if len(rows) != 1 {
+		t.Fatalf("got %d rows, want 1", len(rows))
+	}
+	if rows[0].name != "imi-abc" {
+		t.Errorf("name = %q, want imi-abc", rows[0].name)
+	}
+	if rows[0].externalIDs["iface-id"] != "port-eni-1" {
+		t.Errorf("iface-id = %q, want port-eni-1", rows[0].externalIDs["iface-id"])
 	}
 }

spinifex/network/host/run.go

@@ -10,9 +10,13 @@ import (
 	"os/exec"
 	"path/filepath"
 	"strings"
+
+	"github.com/mulgadc/spinifex/spinifex/utils"
 )
 
-// execRunner runs the command directly when uid 0, otherwise via sudo.
+// execRunner escalates only the commands that need it — see utils.NeedsPrivilege.
+// The OVS/OVN socket clients reach their daemons over the group-owned control
+// sockets, and ip/iptables/arping run under the unit's ambient capabilities.
 type execRunner struct{}
 
 var _ Runner = execRunner{}
@@ -22,10 +26,10 @@ func NewExecRunner() Runner { return execRunner{} }
 
 func (execRunner) Run(ctx context.Context, name string, args ...string) ([]byte, error) {
 	var cmd *exec.Cmd
-	if os.Getuid() == 0 {
-		cmd = exec.CommandContext(ctx, name, args...)
-	} else {
+	if utils.NeedsPrivilege(name, args...) {
 		cmd = exec.CommandContext(ctx, "sudo", append([]string{name}, args...)...)
+	} else {
+		cmd = exec.CommandContext(ctx, name, args...)
 	}
 	// Capture stdout and stderr separately so stderr is not folded into parsed
 	// output: under a restrictive CapabilityBoundingSet (no CAP_AUDIT_WRITE) sudo

spinifex/utils/sudo.go

@@ -4,19 +4,119 @@ import (
 	"context"
 	"os"
 	"os/exec"
+	"strconv"
+	"strings"
+	"sync"
 )
 
+// socketClients are the OVS/OVN client tools that do all their work over a
+// control socket, not through privileged syscalls. setup-ovn.sh group-owns
+// those sockets to `spinifex` (0660), so the service users reach them as
+// themselves and escalating buys nothing.
+//
+// Escalating actively costs: a sudoers rule for these takes unrestricted args,
+// and every one of them accepts --log-file=PATH, which writes a root-owned file
+// wherever the caller points it. A NOPASSWD grant for any of them is therefore a
+// root-equivalent grant handed out to read a status.
+//
+// ovs-ofctl is deliberately absent: it talks to a per-bridge
+// /var/run/openvswitch/<bridge>.mgmt socket created by ovs-vswitchd when the
+// bridge appears — including bridges spinifex creates at runtime, long after
+// the provisioning sweep — so those sockets cannot be group-owned up front.
+var socketClients = map[string]bool{
+	"ovs-vsctl":  true,
+	"ovs-appctl": true,
+	"ovn-nbctl":  true,
+	"ovn-sbctl":  true,
+	"ovn-appctl": true,
+	// systemctl is-active is a read of the system bus, allowed unprivileged.
+	"systemctl": true,
+}
+
+// Capability bit numbers from linux/capability.h.
+const (
+	capNetAdmin = 12
+	capNetRaw   = 13
+)
+
+// capForTool returns the capability that lets this invocation run unescalated.
+// The bool is false when no capability substitutes for root, in which case the
+// caller must go through sudo.
+func capForTool(name string, args []string) (uint, bool) {
+	switch name {
+	case "ip", "iptables", "ip6tables":
+		return capNetAdmin, true
+	case "arping":
+		return capNetRaw, true
+	case "sysctl":
+		// Only the net.* trees are governed by CAP_NET_ADMIN in the caller's
+		// network namespace; every other key is a root-only write.
+		for _, a := range args {
+			if strings.HasPrefix(a, "-") {
+				continue
+			}
+			return capNetAdmin, strings.HasPrefix(a, "net.")
+		}
+	}
+	return 0, false
+}
+
+// ambientCaps reports the process's ambient capability set, read once. Tests
+// override it.
+var ambientCaps = sync.OnceValue(readAmbientCaps)
+
+// readAmbientCaps parses CapAmb from /proc/self/status. The ambient set — not
+// the effective one — is what an exec'd child inherits, so it is what decides
+// whether `ip` will hold CAP_NET_ADMIN when we run it as ourselves.
+func readAmbientCaps() uint64 {
+	body, err := os.ReadFile("/proc/self/status")
+	if err != nil {
+		return 0
+	}
+	for line := range strings.SplitSeq(string(body), "\n") {
+		hex, ok := strings.CutPrefix(line, "CapAmb:")
+		if !ok {
+			continue
+		}
+		caps, err := strconv.ParseUint(strings.TrimSpace(hex), 16, 64)
+		if err != nil {
+			return 0
+		}
+		return caps
+	}
+	return 0
+}
+
+// NeedsPrivilege reports whether a command has to be escalated. False for the
+// OVS/OVN socket clients, for anything already running as root, and for tools
+// covered by a capability this process holds ambiently.
+func NeedsPrivilege(name string, args ...string) bool {
+	if os.Getuid() == 0 {
+		return false
+	}
+	if socketClients[name] {
+		return false
+	}
+	// spx is one binary shared by units with different ambient sets: vpcd holds
+	// CAP_NET_ADMIN/CAP_NET_RAW and runs these directly, the daemon does not and
+	// still needs its sudoers grant. So this is decided at runtime, not by tool.
+	if bit, ok := capForTool(name, args); ok && ambientCaps()&(1<<bit) != 0 {
+		return false
+	}
+	return true
+}
+
 // sudoCommand is the private runtime implementation; use SetSudoCommandForTest in tests.
 var sudoCommand = func(name string, args ...string) *exec.Cmd {
-	if os.Getuid() == 0 {
+	if !NeedsPrivilege(name, args...) {
 		return exec.Command(name, args...)
 	}
 	return exec.Command("sudo", append([]string{name}, args...)...)
 }
 
-// SudoCommand wraps exec.Command with sudo when running as non-root.
-// OVS/OVN/ip require CAP_NET_ADMIN; production daemons run as root, but
-// dev environments may not.
+// SudoCommand wraps exec.Command with sudo when the command genuinely needs it.
+// The OVS/OVN socket clients never do, and neither does a tool whose capability
+// this process already holds.
 func SudoCommand(name string, args ...string) *exec.Cmd {
 	return sudoCommand(name, args...)
 }
@@ -24,7 +124,7 @@ func SudoCommand(name string, args ...string) *exec.Cmd {
 // SudoCommandContext is SudoCommand bound to a context so a wedged subprocess is
 // killed when the context is cancelled or its deadline elapses.
 func SudoCommandContext(ctx context.Context, name string, args ...string) *exec.Cmd {
-	if os.Getuid() == 0 {
+	if !NeedsPrivilege(name, args...) {
 		return exec.CommandContext(ctx, name, args...)
 	}
 	return exec.CommandContext(ctx, "sudo", append([]string{name}, args...)...)

spinifex/utils/sudo_invariants_test.go

@@ -0,0 +1,135 @@
+package utils
+
+import (
+	"os"
+	"path/filepath"
+	"runtime"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// policyOwners are the only files allowed to build a sudo invocation. Everything
+// else must route through utils.SudoCommand / host.NewExecRunner so the
+// escalation policy in NeedsPrivilege applies.
+var policyOwners = map[string]bool{
+	filepath.Join("spinifex", "utils", "sudo.go"):          true,
+	filepath.Join("spinifex", "network", "host", "run.go"): true,
+}
+
+// repoRoot walks up from this test file to the directory holding go.mod.
+func repoRoot(t *testing.T) string {
+	t.Helper()
+	_, self, _, ok := runtime.Caller(0)
+	if !ok {
+		t.Fatal("runtime.Caller failed")
+	}
+	dir := filepath.Dir(self)
+	for range 8 {
+		if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
+			return dir
+		}
+		dir = filepath.Dir(dir)
+	}
+	t.Fatalf("go.mod not found above %s", self)
+	return ""
+}
+
+// TestRG10_SudoOnlyThroughThePolicy fails when a package builds its own sudo
+// invocation. vpcd carried such a copy: every OVS/OVN call it made was escalated
+// unconditionally, so it kept working only because the sudoers grants existed,
+// and it broke the OVN flows-ready barrier the moment they were removed. A local
+// copy also silently re-widens the sudoers surface a reviewer thinks is gone.
+func TestRG10_SudoOnlyThroughThePolicy(t *testing.T) {
+	root := repoRoot(t)
+	var offenders []string
+
+	err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if d.IsDir() {
+			switch d.Name() {
+			case ".git", "vendor", "node_modules", "testdata":
+				return filepath.SkipDir
+			// The e2e harness is not a daemon: it runs as the developer or CI
+			// user, who holds full sudo, and shells out to inspect a live node.
+			// The policy governs what the service users escalate.
+			case "tests":
+				return filepath.SkipDir
+			}
+			return nil
+		}
+		if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") {
+			return nil
+		}
+		rel, relErr := filepath.Rel(root, path)
+		if relErr != nil {
+			return relErr
+		}
+		if policyOwners[rel] {
+			return nil
+		}
+		body, readErr := os.ReadFile(path)
+		if readErr != nil {
+			return readErr
+		}
+		for i, line := range strings.Split(string(body), "\n") {
+			if strings.Contains(line, `exec.Command(`) && strings.Contains(line, `"sudo"`) {
+				offenders = append(offenders, rel+":"+strconv.Itoa(i+1))
+			}
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("walk %s: %v", root, err)
+	}
+
+	if len(offenders) > 0 {
+		t.Fatalf("RG-10: these build sudo invocations directly, bypassing utils.NeedsPrivilege:\n  %s\n"+
+			"Use utils.SudoCommand or host.NewExecRunner so the OVS/OVN socket clients stay unescalated.",
+			strings.Join(offenders, "\n  "))
+	}
+}
+
+// TestRG10_VPCDHoldsNoSudoersGrant pins the two halves that have to move
+// together: vpcd's unit grants CAP_NET_ADMIN and CAP_NET_RAW ambiently, and in
+// exchange it gets no sudoers rules. Re-adding a rule reopens the hole the caps
+// closed — every candidate takes unrestricted args, and `sudo ip netns exec
+// <ns> /bin/sh` is a root shell.
+func TestRG10_VPCDHoldsNoSudoersGrant(t *testing.T) {
+	root := repoRoot(t)
+
+	unit, err := os.ReadFile(filepath.Join(root, "build", "systemd", "spinifex-vpcd.service"))
+	if err != nil {
+		t.Fatalf("read vpcd unit: %v", err)
+	}
+	ambient := ambientLine(string(unit))
+	for _, capability := range []string{"CAP_NET_ADMIN", "CAP_NET_RAW"} {
+		if !strings.Contains(ambient, capability) {
+			t.Fatalf("RG-10: spinifex-vpcd.service must grant %s ambiently; without it "+
+				"ip/iptables/arping fail as the service user and the sudoers rules cannot stay removed", capability)
+		}
+	}
+
+	setup, err := os.ReadFile(filepath.Join(root, "scripts", "setup.sh"))
+	if err != nil {
+		t.Fatalf("read setup.sh: %v", err)
+	}
+	for i, line := range strings.Split(string(setup), "\n") {
+		if strings.HasPrefix(line, "spinifex-vpcd ALL=") {
+			t.Fatalf("RG-10: scripts/setup.sh:%d grants spinifex-vpcd a sudoers rule:\n  %s\n"+
+				"vpcd runs these under its ambient capabilities; add the capability to the unit instead.", i+1, line)
+		}
+	}
+}
+
+// ambientLine returns the unit's AmbientCapabilities= value, or "".
+func ambientLine(unit string) string {
+	for line := range strings.SplitSeq(unit, "\n") {
+		if v, ok := strings.CutPrefix(line, "AmbientCapabilities="); ok {
+			return v
+		}
+	}
+	return ""
+}

spinifex/utils/sudo_test.go

@@ -5,23 +5,128 @@ import (
 	"testing"
 )
 
-func TestSudoCommand_NonRoot(t *testing.T) {
-	cmd := SudoCommand("ovs-vsctl", "br-exists", "br-int")
+// withAmbientCaps pins the ambient capability set for one test, so the result
+// does not depend on how the test binary itself was launched.
+func withAmbientCaps(t *testing.T, caps uint64) {
+	t.Helper()
+	orig := ambientCaps
+	ambientCaps = func() uint64 { return caps }
+	t.Cleanup(func() { ambientCaps = orig })
+}
+
+// A tool that needs real privilege is wrapped in sudo when we are not root, and
+// invoked directly when we are.
+func TestSudoCommand_PrivilegedTool(t *testing.T) {
+	withAmbientCaps(t, 0)
+	cmd := SudoCommand("ip", "link", "show")
 	args := cmd.Args
 
 	if os.Getuid() == 0 {
-		if args[0] != "ovs-vsctl" {
-			t.Errorf("as root, expected args[0]='ovs-vsctl', got %q", args[0])
+		if args[0] != "ip" {
+			t.Errorf("as root, expected args[0]='ip', got %q", args[0])
 		}
 		return
 	}
 	if args[0] != "sudo" {
 		t.Errorf("as non-root, expected args[0]='sudo', got %q", args[0])
 	}
-	if args[1] != "ovs-vsctl" {
-		t.Errorf("as non-root, expected args[1]='ovs-vsctl', got %q", args[1])
+	if args[1] != "ip" {
+		t.Errorf("as non-root, expected args[1]='ip', got %q", args[1])
 	}
 	if len(args) != 4 {
-		t.Errorf("expected 4 args [sudo ovs-vsctl br-exists br-int], got %d: %v", len(args), args)
+		t.Errorf("expected 4 args [sudo ip link show], got %d: %v", len(args), args)
+	}
+}
+
+// The OVS/OVN socket clients must never be escalated. Each accepts
+// --log-file=PATH, so a NOPASSWD sudoers rule for one — which necessarily takes
+// unrestricted args — writes a root-owned file wherever the caller points it.
+// They reach their daemons over the group-owned control sockets instead.
+func TestSocketClientsAreNotEscalated(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("running as root: nothing is escalated, so the policy is not exercised")
+	}
+	for _, tool := range []string{"ovs-vsctl", "ovs-appctl", "ovn-nbctl", "ovn-sbctl", "ovn-appctl", "systemctl"} {
+		if NeedsPrivilege(tool) {
+			t.Errorf("%s is escalated; it talks to a group-owned socket, and a sudoers grant for it would be root-equivalent", tool)
+		}
+		if args := SudoCommand(tool, "--version").Args; len(args) > 0 && args[0] == "sudo" {
+			t.Errorf("SudoCommand(%s) built a sudo invocation: %v", tool, args)
+		}
+	}
+}
+
+// The tools that genuinely need privilege keep it. ovs-ofctl is in this list on
+// purpose: it talks to a per-bridge <bridge>.mgmt socket that ovs-vswitchd
+// creates when the bridge appears — including bridges spinifex creates at
+// runtime — so those cannot be group-owned by the provisioning sweep.
+func TestPrivilegedToolsStillEscalate(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("running as root: nothing is escalated, so the policy is not exercised")
+	}
+	withAmbientCaps(t, 0)
+	for _, tool := range []string{"ip", "iptables", "dhcpcd", "sysctl", "arping", "ovs-ofctl"} {
+		if !NeedsPrivilege(tool, "net.ipv4.neigh.x=1") {
+			t.Errorf("%s is not escalated, but it needs root or an ambient capability", tool)
+		}
+		if args := SudoCommand(tool, "--version").Args; len(args) == 0 || args[0] != "sudo" {
+			t.Errorf("SudoCommand(%s) did not build a sudo invocation: %v", tool, args)
+		}
+	}
+}
+
+// vpcd holds CAP_NET_ADMIN and CAP_NET_RAW ambiently, so the kernel hands them
+// to each child on exec and these tools work as the service user. This is what
+// lets spinifex-vpcd carry no sudoers rules at all.
+func TestAmbientCapabilitiesReplaceSudo(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("running as root: nothing is escalated, so the policy is not exercised")
+	}
+	withAmbientCaps(t, 1<<capNetAdmin|1<<capNetRaw)
+
+	unescalated := [][]string{
+		{"ip", "addr", "replace", "10.0.0.1/24", "dev", "eth0"},
+		{"iptables", "-A", "FORWARD", "-j", "ACCEPT"},
+		{"arping", "-U", "-c", "2", "-I", "br-wan", "10.0.0.1"},
+		{"sysctl", "-w", "net.ipv4.neigh.br-wan.proxy_delay=0"},
+	}
+	for _, argv := range unescalated {
+		if NeedsPrivilege(argv[0], argv[1:]...) {
+			t.Errorf("%v is escalated despite the ambient capability", argv)
+		}
+	}
+
+	// The capability is per-tool, and for sysctl per-key: CAP_NET_ADMIN governs
+	// the net.* trees only, and nothing makes ovs-ofctl's root-owned
+	// <bridge>.mgmt socket readable.
+	escalated := [][]string{
+		{"sysctl", "-w", "vm.swappiness=10"},
+		{"ovs-ofctl", "dump-flows", "br-int"},
+		{"dhcpcd", "-q", "br-wan"},
+	}
+	for _, argv := range escalated {
+		if !NeedsPrivilege(argv[0], argv[1:]...) {
+			t.Errorf("%v is not escalated, but CAP_NET_ADMIN/CAP_NET_RAW do not cover it", argv)
+		}
+	}
+}
+
+// The daemon runs the same spx binary as vpcd but its unit grants no
+// CAP_NET_ADMIN, so the decision has to be made from the live capability set
+// rather than from the tool name.
+func TestDaemonAmbientSetStillEscalatesIP(t *testing.T) {
+	if os.Getuid() == 0 {
+		t.Skip("running as root: nothing is escalated, so the policy is not exercised")
+	}
+	const capSysAdmin, capDACOverride = 21, 1
+	withAmbientCaps(t, 1<<capSysAdmin|1<<capDACOverride)
+
+	for _, argv := range [][]string{
+		{"ip", "tuntap", "add", "tap0", "mode", "tap"},
+		{"sysctl", "-qw", "net.ipv4.conf.tap0.rp_filter=0"},
+	} {
+		if !NeedsPrivilege(argv[0], argv[1:]...) {
+			t.Errorf("%v is not escalated, but the daemon holds no CAP_NET_ADMIN", argv)
+		}
 	}
 }

spinifex/vpcd/vpcd.go

@@ -65,13 +65,11 @@ var waitForFlowsHV = func() error {
 	return nil
 }
 
-// sudoCommand wraps exec.Command with sudo when not root; OVS/OVN commands require elevated privileges.
-func sudoCommand(name string, args ...string) *exec.Cmd {
-	if os.Getuid() == 0 {
-		return exec.Command(name, args...)
-	}
-	return exec.Command("sudo", append([]string{name}, args...)...)
-}
+// sudoCommand is utils.SudoCommand, which escalates only what genuinely needs
+// it. Every caller here is an OVS/OVN socket client, so none of them escalate;
+// a local copy that always sudoed silently bypassed that policy and broke the
+// flows-ready barrier once the grants were removed.
+var sudoCommand = utils.SudoCommand
 
 var serviceName = "vpcd"
 
@@ -203,9 +201,11 @@ var checkBrInt = func() error {
 	return nil
 }
 
-// checkOVNController verifies ovn-controller is running. Tries legacy socket path, then OVN 22.03+ path, then systemctl.
+// checkOVNController verifies ovn-controller is running. ovn-appctl resolves a
+// bare target against /var/run/ovn where the socket and pidfile live; ovs-appctl
+// looks in /var/run/openvswitch and only ever logged a missing pidfile.
 var checkOVNController = func() error {
-	if sudoCommand("ovs-appctl", "-t", "ovn-controller", "version").Run() == nil {
+	if sudoCommand("ovn-appctl", "-t", "ovn-controller", "version").Run() == nil {
 		return nil
 	}
 	if matches, _ := filepath.Glob("/var/run/ovn/ovn-controller.*.ctl"); len(matches) > 0 {