diff --git a/build/systemd/spinifex-daemon.service b/build/systemd/spinifex-daemon.service index 2bd789979..54767f701 100644 --- a/build/systemd/spinifex-daemon.service +++ b/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 diff --git a/build/systemd/spinifex-vpcd.service b/build/systemd/spinifex-vpcd.service index 0df264c4b..38bbc7f01 100644 --- a/build/systemd/spinifex-vpcd.service +++ b/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 diff --git a/scripts/setup-ovn.sh b/scripts/setup-ovn.sh index 31ae8cde4..1fa0ebe98 100755 --- a/scripts/setup-ovn.sh +++ b/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 .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 ` 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 < /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 +# .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 /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 --- diff --git a/spinifex/network/host/health.go b/spinifex/network/host/health.go index 8f09b583e..0756fc880 100644 --- a/spinifex/network/host/health.go +++ b/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)), "\"") +} diff --git a/spinifex/network/host/health_test.go b/spinifex/network/host/health_test.go new file mode 100644 index 000000000..f0c56f9c2 --- /dev/null +++ b/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) + } + } +} diff --git a/spinifex/network/host/imds_reconcile.go b/spinifex/network/host/imds_reconcile.go index 552dd0b3f..bed6ce501 100644 --- a/spinifex/network/host/imds_reconcile.go +++ b/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 } diff --git a/spinifex/network/host/imds_reconcile_test.go b/spinifex/network/host/imds_reconcile_test.go index d6c47a3fc..4fabc4651 100644 --- a/spinifex/network/host/imds_reconcile_test.go +++ b/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-"), 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-"), 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"]) } } diff --git a/spinifex/network/host/run.go b/spinifex/network/host/run.go index 489526000..9fbd7a336 100644 --- a/spinifex/network/host/run.go +++ b/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 diff --git a/spinifex/utils/sudo.go b/spinifex/utils/sudo.go index c72c26745..b534da051 100644 --- a/spinifex/utils/sudo.go +++ b/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/.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< 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 +// /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 "" +} diff --git a/spinifex/utils/sudo_test.go b/spinifex/utils/sudo_test.go index 30559b509..c50d99f17 100644 --- a/spinifex/utils/sudo_test.go +++ b/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 .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<.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< 0 {