diff --git a/.github/workflows/apt-packages.yml b/.github/workflows/apt-packages.yml new file mode 100644 index 000000000..3260e8f15 --- /dev/null +++ b/.github/workflows/apt-packages.yml @@ -0,0 +1,48 @@ +name: Apt Packages + +# Path-filtered like image-scripts.yml: this resolves setup.sh's runtime package +# list against each supported suite's real indices, so a renamed, dropped, or +# ambiguously virtual package fails here rather than on a user's machine. +on: + push: + branches: + - main + - dev + paths: + - scripts/setup.sh + - scripts/lint-apt-packages.sh + - .github/workflows/apt-packages.yml + pull_request: + branches: + - main + - dev + paths: + - scripts/setup.sh + - scripts/lint-apt-packages.sh + - .github/workflows/apt-packages.yml + workflow_dispatch: + +permissions: + contents: read + +jobs: + apt_packages: + name: Resolve apt list (${{ matrix.image }}) + runs-on: ubuntu-26.04 + + strategy: + fail-fast: false + matrix: + # The suites the nightly E2E matrix actually installs on. Ubuntu 22.04 is + # accepted by detect_os but has never been exercised and does not resolve. + image: + - ubuntu:24.04 + - ubuntu:26.04 + - debian:13 + + steps: + - name: Checkout Code + uses: actions/checkout@v7 + + - name: Resolve runtime package list + run: ./scripts/lint-apt-packages.sh ${{ matrix.image }} diff --git a/Makefile b/Makefile index c7a64a191..5b7cd7282 100644 --- a/Makefile +++ b/Makefile @@ -250,7 +250,7 @@ install-system: sudo apt-get update && sudo DEBIAN_FRONTEND=noninteractive apt-get install -y \ -o Dpkg::Options::="--force-confdef" \ -o Dpkg::Options::="--force-confold" \ - nbdkit nbdkit-plugin-dev pkg-config qemu-system-x86 qemu-system-arm qemu-utils qemu-kvm \ + nbdkit nbdkit-plugin-dev pkg-config qemu-system-x86 qemu-system-arm qemu-utils \ ovmf qemu-efi-aarch64 \ libvirt-daemon-system libvirt-clients libvirt-dev make gcc jq curl \ iproute2 netcat-openbsd openssh-client wget git unzip sudo xz-utils file \ diff --git a/docs/install/install-airgapped/README.md b/docs/install/install-airgapped/README.md index f54af4601..04ae03868 100644 --- a/docs/install/install-airgapped/README.md +++ b/docs/install/install-airgapped/README.md @@ -56,7 +56,7 @@ Pre-download the APT packages installed by the production setup. `apt install -- sudo apt update sudo apt install --download-only -y \ nbdkit nbdkit-plugin-dev pkg-config \ - qemu-system-x86 qemu-utils qemu-kvm \ + qemu-system-x86 qemu-utils \ ovmf qemu-efi-aarch64 \ libvirt-daemon-system libvirt-clients libvirt-dev \ ovn-central ovn-host openvswitch-switch \ diff --git a/scripts/lint-apt-packages.sh b/scripts/lint-apt-packages.sh new file mode 100755 index 000000000..638ae29ea --- /dev/null +++ b/scripts/lint-apt-packages.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +# Resolves setup.sh's runtime apt list against a suite's real indices with +# apt-get install -s. Catches a package that has been renamed, dropped, or made +# ambiguously virtual before it reaches a user's machine. +set -euo pipefail + +IMAGE="${1:?usage: lint-apt-packages.sh , e.g. ubuntu:26.04}" + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" + +# Source setup.sh for the list rather than restating it here, so the lint cannot +# drift from what the installer actually asks apt for. +INSTALL_SPINIFEX_LIB_ONLY=1 +export INSTALL_SPINIFEX_LIB_ONLY +# shellcheck source=/dev/null +. "${SCRIPT_DIR}/setup.sh" + +if [ -z "${APT_RUNTIME_PACKAGES:-}" ]; then + echo "setup.sh did not define APT_RUNTIME_PACKAGES" >&2 + exit 1 +fi + +# Both arch-specific qemu packages exist in the amd64 archive, so linting them +# together covers the aarch64 install path from an amd64 runner. +PACKAGES="qemu-system-x86 qemu-system-arm ${APT_RUNTIME_PACKAGES}" + +echo "Resolving ${IMAGE}..." +# shellcheck disable=SC2086 +docker run --rm "${IMAGE}" bash -c \ + "apt-get update -qq >/dev/null && apt-get install -s -y -qq ${PACKAGES//$'\n'/ } >/dev/null" + +echo "OK: every package resolves on ${IMAGE}" diff --git a/scripts/setup.sh b/scripts/setup.sh index 6fb69fa2f..83a250350 100755 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -24,6 +24,9 @@ set -e INSTALL_SPINIFEX_CHANNEL="${INSTALL_SPINIFEX_CHANNEL:-latest}" INSTALL_BASE_URL="${INSTALL_BASE_URL:-https://install.mulgadc.com}" +# Referenced by both the sudoers grant and the daemon, so the path is fixed here. +ENDPOINT_SYSCTL_HELPER="/usr/local/lib/spinifex/spinifex-set-endpoint-sysctl" + # --- Colors --- RED='\033[0;31m' GREEN='\033[0;32m' @@ -181,15 +184,76 @@ create_service_users() { } # --- Install scoped sudoers rules --- +# sudo-rs (Ubuntu's default sudo since 25.10) implements only a subset of +# Defaults and rejects an unknown one outright, so pam_session must be omitted +# there rather than emitted and ignored. +sudo_is_sudo_rs() { + sudo --version 2>/dev/null | head -1 | grep -qi 'sudo-rs' +} + +# The daemon only ever sets rp_filter and accept_local on an endpoint it just +# created. A fixed-verb helper keeps that grant expressible without a wildcard, +# which sudo-rs forbids inside command arguments. +install_endpoint_sysctl_helper() { + $SUDO install -d -m 0755 /usr/local/lib/spinifex + $SUDO tee "${ENDPOINT_SYSCTL_HELPER}" > /dev/null << 'HELPER' +#!/bin/sh +# Sets one of two per-endpoint sysctls for the asymmetric IMDS reply path. +# Interface, key and value are all constrained: this runs as root under a +# NOPASSWD grant, so it must never pass an arbitrary key through to sysctl. +set -eu + +if [ "$#" -ne 3 ]; then + echo "usage: $0 <0|1>" >&2 + exit 2 +fi + +iface=$1 +key=$2 +value=$3 + +# Reject anything that is not a plain interface name, so the key cannot be +# steered elsewhere in the sysctl tree with a "." or a "/". +case "${iface}" in + ''|*[!A-Za-z0-9_-]*) echo "invalid interface: ${iface}" >&2; exit 2 ;; +esac + +case "${key}" in + rp_filter|accept_local) ;; + *) echo "key not permitted: ${key}" >&2; exit 2 ;; +esac + +case "${value}" in + 0|1) ;; + *) echo "value not permitted: ${value}" >&2; exit 2 ;; +esac + +exec sysctl -qw "net.ipv4.conf.${iface}.${key}=${value}" +HELPER + $SUDO chown root:root "${ENDPOINT_SYSCTL_HELPER}" + $SUDO chmod 0755 "${ENDPOINT_SYSCTL_HELPER}" +} + install_sudoers() { stage "installing scoped sudoers rules" - $SUDO tee /etc/sudoers.d/spinifex-network > /dev/null << 'SUDOERS' + install_endpoint_sysctl_helper + + if sudo_is_sudo_rs; then + $SUDO tee /etc/sudoers.d/spinifex-network > /dev/null << 'SUDOERS' +# pam_session is omitted: this host runs sudo-rs, which does not implement it. +SUDOERS + else + $SUDO tee /etc/sudoers.d/spinifex-network > /dev/null << 'SUDOERS' # 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 +SUDOERS + fi + + $SUDO tee -a /etc/sudoers.d/spinifex-network > /dev/null << 'SUDOERS' # 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 @@ -208,11 +272,16 @@ Defaults:spinifex-daemon !pam_session # 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. +# per-tap IMDS datapath flows on br-imds; the sysctl helper 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 + + # Separate append so the prose above can stay in a quoted heredoc: it carries + # backticks that an expanding one would run as command substitution. + $SUDO tee -a /etc/sudoers.d/spinifex-network > /dev/null << SUDOERS +spinifex-daemon ALL=(root) NOPASSWD: ${ENDPOINT_SYSCTL_HELPER} SUDOERS $SUDO chmod 0440 /etc/sudoers.d/spinifex-network $SUDO visudo -cf /etc/sudoers.d/spinifex-network || fatal "Invalid sudoers syntax in spinifex-network" @@ -224,6 +293,18 @@ SUDOERS # scripts/iso-builder/build/packages.list in the mulga repo. When you add, # remove, or change a runtime package here, review that file too — drift means # the ISO and `curl | bash` paths install different software. +# +# Hoisted out of the apt-get call so scripts/lint-apt-packages.sh can resolve the +# same list against each supported suite. The arch-specific qemu package is +# added by install_apt_deps from detect_arch. +APT_RUNTIME_PACKAGES="nbdkit +qemu-utils gdisk ovmf qemu-efi-aarch64 less +libvirt-daemon-system libvirt-clients +pciutils +jq curl iproute2 netcat-openbsd wget unzip xz-utils file +ovn-central ovn-host openvswitch-switch openvswitch-ipsec strongswan-charon dhcpcd-base +chrony" + install_apt_deps() { stage "installing apt dependencies" if [ "${INSTALL_SPINIFEX_SKIP_APT}" = "1" ]; then @@ -232,14 +313,10 @@ install_apt_deps() { info "Installing system dependencies..." $SUDO apt-get update -qq + # Unquoted on purpose: both variables are whitespace-separated package lists. + # shellcheck disable=SC2086 DEBIAN_FRONTEND=noninteractive $SUDO apt-get install -y -qq \ - nbdkit \ - $QEMU_PACKAGES qemu-utils gdisk qemu-kvm ovmf qemu-efi-aarch64 less \ - libvirt-daemon-system libvirt-clients \ - pciutils \ - jq curl iproute2 netcat-openbsd wget unzip xz-utils file \ - ovn-central ovn-host openvswitch-switch openvswitch-ipsec strongswan-charon dhcpcd-base \ - chrony \ + $QEMU_PACKAGES $APT_RUNTIME_PACKAGES \ > /dev/null info "System dependencies installed" @@ -879,4 +956,8 @@ main() { fi } -main "$@" +# Guarded so the package lint can source this file for APT_RUNTIME_PACKAGES +# without running the installer. +if [ "${INSTALL_SPINIFEX_LIB_ONLY:-0}" != "1" ]; then + main "$@" +fi diff --git a/spinifex/network/host/imds_attach_test.go b/spinifex/network/host/imds_attach_test.go index 452ff521b..612e5f698 100644 --- a/spinifex/network/host/imds_attach_test.go +++ b/spinifex/network/host/imds_attach_test.go @@ -59,7 +59,7 @@ func TestInstallIMDSDatapath(t *testing.T) { s := newStubRunner() s.expect("ovs-vsctl", nil, nil) s.expect("ip", nil, nil) - s.expect("sysctl", nil, nil) + s.expect(utils.EndpointSysctlHelper, nil, nil) s.expect("ovs-ofctl", nil, nil) d := imdsDatapathSpec("eni-0abc1234", "02:00:00:00:01:05", "subnet-0fedcba9") @@ -146,7 +146,7 @@ func TestInstallIMDSDatapath_ServingFailureWrapped(t *testing.T) { s := newStubRunner() s.expect("ovs-vsctl", nil, nil) s.expect("ip", nil, nil) - s.expect("sysctl", nil, nil) + s.expect(utils.EndpointSysctlHelper, nil, nil) s.expect("ovs-ofctl del-flows", nil, nil) // Forward flows (priority=100) are connectivity and must succeed; the demux/egress // flows (priority=200) are serving — fail those to land in the serving stage. diff --git a/spinifex/network/host/imds_datapath.go b/spinifex/network/host/imds_datapath.go index ddfd3a16f..463e7cabd 100644 --- a/spinifex/network/host/imds_datapath.go +++ b/spinifex/network/host/imds_datapath.go @@ -204,9 +204,12 @@ func ensureIMDSEndpoint(ctx context.Context, r Runner, d IMDSTapDatapath) error return setEndpointSysctl(ctx, r, d.Endpoint, "accept_local", "1") } +// setEndpointSysctl goes through the helper rather than sysctl itself: the +// daemon's grant has to name a fixed command, since sudo-rs rejects the +// wildcard the old `sysctl -qw net.ipv4.conf.*` rule relied on. func setEndpointSysctl(ctx context.Context, r Runner, endpoint, suffix, val string) error { key := "net.ipv4.conf." + endpoint + "." + suffix - if _, err := r.Run(ctx, "sysctl", "-qw", key+"="+val); err != nil { + if _, err := r.Run(ctx, utils.EndpointSysctlHelper, endpoint, suffix, val); err != nil { return fmt.Errorf("set %s=%s: %w", key, val, err) } return nil diff --git a/spinifex/network/host/imds_datapath_test.go b/spinifex/network/host/imds_datapath_test.go index 310facc40..d8e3b49b6 100644 --- a/spinifex/network/host/imds_datapath_test.go +++ b/spinifex/network/host/imds_datapath_test.go @@ -5,6 +5,8 @@ import ( "errors" "strings" "testing" + + "github.com/mulgadc/spinifex/spinifex/utils" ) func testDatapath() IMDSTapDatapath { @@ -114,7 +116,7 @@ func TestInstallTapDatapath(t *testing.T) { s := newStubRunner() s.expect("ovs-vsctl", nil, nil) s.expect("ip", nil, nil) - s.expect("sysctl", nil, nil) + s.expect(utils.EndpointSysctlHelper, nil, nil) s.expect("ovs-ofctl", nil, nil) d := testDatapath() @@ -130,8 +132,8 @@ func TestInstallTapDatapath(t *testing.T) { "ip link set " + d.Endpoint + " up", "ip addr replace " + imdsMetaAddr + "/32 dev " + d.Endpoint, "ip addr replace " + imdsDNSAddr + "/32 dev " + d.Endpoint, - "sysctl -qw net.ipv4.conf." + d.Endpoint + ".rp_filter=0", - "sysctl -qw net.ipv4.conf." + d.Endpoint + ".accept_local=1", + utils.EndpointSysctlHelper + " " + d.Endpoint + " rp_filter 0", + utils.EndpointSysctlHelper + " " + d.Endpoint + " accept_local 1", // Flows are not cleared here: installIMDSDatapath clears the shared cookie // once up front so this install does not wipe the patch's forward flows. // Ingress demux (gateway dst MAC -> endpoint MAC), one per captured addr. @@ -161,7 +163,7 @@ func TestInstallTapDatapathReattachIsIdempotent(t *testing.T) { s := newStubRunner() s.expect("ovs-vsctl", nil, nil) s.expect("ip", nil, nil) - s.expect("sysctl", nil, nil) + s.expect(utils.EndpointSysctlHelper, nil, nil) s.expect("ovs-ofctl", nil, nil) d := testDatapath() diff --git a/spinifex/utils/sudo.go b/spinifex/utils/sudo.go index b534da051..ba1642f47 100644 --- a/spinifex/utils/sudo.go +++ b/spinifex/utils/sudo.go @@ -33,6 +33,11 @@ var socketClients = map[string]bool{ "systemctl": true, } +// EndpointSysctlHelper is the fixed-verb wrapper setup.sh installs. sudo-rs +// (Ubuntu's default sudo since 25.10) forbids a wildcard inside a command +// argument, so the daemon's per-endpoint sysctl grant names this instead. +const EndpointSysctlHelper = "/usr/local/lib/spinifex/spinifex-set-endpoint-sysctl" + // Capability bit numbers from linux/capability.h. const ( capNetAdmin = 12 @@ -48,6 +53,10 @@ func capForTool(name string, args []string) (uint, bool) { return capNetAdmin, true case "arping": return capNetRaw, true + case EndpointSysctlHelper: + // The helper only ever writes net.ipv4.conf.., so a holder of + // CAP_NET_ADMIN runs it directly and never reaches the sudoers grant. + return capNetAdmin, 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. diff --git a/spinifex/utils/sudo_invariants_test.go b/spinifex/utils/sudo_invariants_test.go index 0703ea79b..c64ed1fd8 100644 --- a/spinifex/utils/sudo_invariants_test.go +++ b/spinifex/utils/sudo_invariants_test.go @@ -124,6 +124,35 @@ func TestRG10_VPCDHoldsNoSudoersGrant(t *testing.T) { } } +// TestSudoersGrantsCarryNoArgumentWildcard pins the sudo-rs constraint. Ubuntu +// has shipped sudo-rs as the default sudo since 25.10, and it rejects a `*` +// inside a command argument outright — visudo fails and setup.sh aborts. +func TestSudoersGrantsCarryNoArgumentWildcard(t *testing.T) { + root := repoRoot(t) + 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-daemon ALL=") { + continue + } + _, cmds, _ := strings.Cut(line, "NOPASSWD:") + for spec := range strings.SplitSeq(cmds, ",") { + fields := strings.Fields(spec) + // A standalone final `*` is the one form sudo-rs accepts, so drop it + // and treat any wildcard left inside an argument as the failure. + if n := len(fields); n > 0 && fields[n-1] == "*" { + fields = fields[:n-1] + } + if strings.Contains(strings.Join(fields, " "), "*") { + t.Fatalf("scripts/setup.sh:%d grants a wildcard inside a command argument:\n %s\n"+ + "sudo-rs rejects this; name a fixed-verb helper instead (see EndpointSysctlHelper).", i+1, line) + } + } + } +} + // ambientLine returns the unit's AmbientCapabilities= value, or "". func ambientLine(unit string) string { for line := range strings.SplitSeq(unit, "\n") { diff --git a/spinifex/utils/sudo_test.go b/spinifex/utils/sudo_test.go index c50d99f17..9d06c9064 100644 --- a/spinifex/utils/sudo_test.go +++ b/spinifex/utils/sudo_test.go @@ -89,6 +89,7 @@ func TestAmbientCapabilitiesReplaceSudo(t *testing.T) { {"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"}, + {EndpointSysctlHelper, "ime-12345678", "rp_filter", "0"}, } for _, argv := range unescalated { if NeedsPrivilege(argv[0], argv[1:]...) { @@ -124,6 +125,7 @@ func TestDaemonAmbientSetStillEscalatesIP(t *testing.T) { for _, argv := range [][]string{ {"ip", "tuntap", "add", "tap0", "mode", "tap"}, {"sysctl", "-qw", "net.ipv4.conf.tap0.rp_filter=0"}, + {EndpointSysctlHelper, "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)