From 24eeb15b7073cabec32801ca698e15d299e5ae82 Mon Sep 17 00:00:00 2001 From: Brad House Date: Tue, 18 Aug 2026 14:25:48 +0000 Subject: [PATCH] ci: harden the kvm_x86_64 build/boot workflow and boot harness Follow-up fixes to the CI added in 1a24708f, found in review by @mshych on PR #1126. Grouped here because they all fix already-merged code rather than anything that PR adds. Workflow: - The cross-toolchain cache key omitted inputs that change the toolchain. xtools.make derives XTOOLS_VERSION from ONIE_ARCH and LINUX_RELEASE, which live in machine/kvm_x86_64/machine.make and kernel-download.make -- neither of which was hashed. A kernel bump therefore kept HITTING the key while the build/x-tools/ directory name changed underneath it: the restored toolchain was the wrong one, "make xtools" rebuilt from scratch, and the save step (gated on a cache miss) skipped it, so every later run paid the same rebuild with no signal saying why. Hash those inputs, and add a guard that warns when a cache hit still had to build a toolchain -- the symptom of a key that is missing an input. - push: and pull_request: were both unfiltered, running the whole pipeline twice for every push to a branch with an open PR. The concurrency group cannot collapse them because the two events carry different refs. Add a small "gate" job that skips the push run when an open PR in the same repository already covers the commit. This is a query rather than the simpler branch filter on push: because a branch filter also removes CI from every topic branch in a fork, which is where this workflow does most of its pre-submit work. A fork's pull requests are opened against the upstream repository, so they raise no pull_request event in the fork, and workflow_dispatch is unavailable there unless the workflow is also on the fork's default branch -- a filtered branch would be left with no CI at all. The query has no such blind spot: in a fork it finds no pull request and the push run proceeds. - Drop --privileged from both docker run invocations. The Dockerfile ends with USER build, so the build is unprivileged and cannot use any capability --privileged grants; the image build uses fakeroot and mtools, not loop mounts. - Add a workflow-level "permissions: contents: read" and per-job timeout-minutes. Nothing here writes to the repository, and a wedged build would otherwise burn the 6-hour default. Dockerfile: - groupadd -g $GID failed when the host GID already exists in debian:11, which the low GID range is full of (30 dip, 50 staff, 100 users). A developer whose primary group lands there -- common with central/NFS accounts, and the norm on macOS -- could not build the image at all, while CI never saw it because the runner's GID happens to be free. Reuse the group when the GID is taken, and chown by numeric id since the "build" group then does not exist. - The /sbin:/usr/sbin PATH addition never took effect. It was appended to ~/.bashrc, but the workflow runs "bash -lc" and Debian's default .bashrc returns early for non-interactive shells. The root cause is a level deeper: /etc/profile *replaces* PATH for non-root users with one that omits both sbin dirs, so ENV alone would be undone under bash -l. Set both ENV and an /etc/profile.d snippet, which is sourced after /etc/profile. emulation/ci-boot-test.sh: - The "ONIE userspace" milestone matched strings GRUB itself echoes from grub-iso.cfg ("ONIE: Rescue Mode ...", "Version :"), so it duplicated the "GRUB reached" check and proved nothing about userspace. Assert "Info: BIOS mode:" from init-arch instead, which GRUB cannot produce. - secureboot mode never checked that Secure Boot was actually enforced: if the varstore failed to enrol the guest booted with SB off and the "must boot" assertion passed for the wrong reason. Assert the guest's own "Info: Secure Boot: Active." (read from the SecureBoot EFI variable by init-arch), and assert its absence in the relaxed run. - A failed virt-fw-vars was silently ignored: it was piped through "| sed | grep ... || true", which dropped its exit status twice over. Move enrolment into a helper that fails the run. - A QEMU that never started was indistinguishable from a firmware that refused the image -- both left an empty serial log -- so the negative control could score a broken harness as a successful rejection. Keep QEMU's stderr and hard-fail when it exits immediately. Also capture the OVMF debug console when the device is available, as a second evidence channel on debug OVMF builds. - The documented accel=kvm:tcg fallback did not work for secureboot at -smp 2: without KVM the boot dies in rcu_sched stalls and burns the whole timeout, where -smp 1 boots in ~31s. Use one vCPU when /dev/kvm is not usable. CI has /dev/kvm and is unaffected. Signed-off-by: Brad House --- .github/onie-build/Dockerfile | 33 +++++- .github/workflows/build-onie.yml | 153 ++++++++++++++++++++++++- emulation/ci-boot-test.sh | 190 ++++++++++++++++++++++++++----- 3 files changed, 338 insertions(+), 38 deletions(-) diff --git a/.github/onie-build/Dockerfile b/.github/onie-build/Dockerfile index 500e8aedf..3fc4420b1 100644 --- a/.github/onie-build/Dockerfile +++ b/.github/onie-build/Dockerfile @@ -87,14 +87,35 @@ RUN sed -i -e 's/# en_US.UTF-8 UTF-8/en_US.UTF-8 UTF-8/' /etc/locale.gen && \ # (https://github.com/moby/moby/issues/5419#issuecomment-41478290) ARG UID=1000 ARG GID=1000 -RUN groupadd -g $GID build && \ - useradd -l -m -u $UID -g $GID -s /bin/bash build && \ - chown -R build:build /onie +# Reuse the host's primary group when that GID is already taken in the +# image. Debian fills the low GID range with system groups (30 dip, 50 +# staff, 100 users), so insisting on creating our own group makes the image +# unbuildable for anyone whose primary group lands there -- common with +# central/NFS accounts, and the norm on macOS hosts (GID 20 = dialout here). +# When the group is reused, "build" no longer exists as a group name, so +# chown by numeric id. +RUN if getent group "$GID" >/dev/null; then \ + echo "GID $GID is $(getent group "$GID" | cut -d: -f1); reusing it"; \ + else \ + groupadd -g "$GID" build; \ + fi && \ + useradd -l -m -u "$UID" -g "$GID" -s /bin/bash build && \ + chown -R "$UID:$GID" /onie -USER build +# /sbin and /usr/sbin hold tools the build invokes. Set this in two places +# so it holds however the image is invoked: +# ENV -- non-login shells (docker run , bash -c) +# profile.d -- login shells. Debian's /etc/profile *replaces* PATH for +# non-root users with one that omits both sbin dirs, and it +# does so before sourcing profile.d, so the snippet wins. +# ~/.bashrc, which this used to use, works for neither: Debian's default +# .bashrc returns early when the shell is not interactive, so under the +# workflow's "bash -lc" the PATH line was never reached. +ENV PATH="/sbin:/usr/sbin:${PATH}" +RUN printf '%s\n' 'export PATH="/sbin:/usr/sbin:$PATH"' \ + > /etc/profile.d/00-onie-sbin.sh -# /sbin and /usr/sbin hold tools the build invokes. -RUN echo 'export PATH="/sbin:/usr/sbin:$PATH"' >> ~/.bashrc +USER build # The build runs git commands; give it a default identity. RUN git config --global user.email "build@example.com" && \ diff --git a/.github/workflows/build-onie.yml b/.github/workflows/build-onie.yml index f5b94c5e7..3e25f3d39 100644 --- a/.github/workflows/build-onie.yml +++ b/.github/workflows/build-onie.yml @@ -10,21 +10,96 @@ name: Build ONIE (kvm_x86_64) # confirm the change still builds, and the kvm_x86_64 image depends on far # more than the build files (installer/, rootconf/, patches/, ...), so path # filtering would risk skipping validation on build-affecting changes. +# +# A push to a branch that already has an open pull request IN THIS repository +# fires both events and ran the whole pipeline twice; the concurrency group +# below cannot collapse them, because the two events carry different refs +# (refs/heads/ vs refs/pull//merge). The "gate" job below drops the +# duplicate push run. See the comment there for why this is a query rather +# than a branch filter on "push:". on: workflow_dispatch: push: pull_request: -# Cancel an in-progress run when a newer commit is pushed to the same ref, -# so stacked pushes don't pile up concurrent builds. +# Cancel an in-progress run when a newer commit is pushed to the same ref, so +# stacked pushes don't pile up concurrent builds. Runs on the long-lived +# branches are never cancelled -- those results are the integration signal. concurrency: group: ${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + cancel-in-progress: >- + ${{ github.ref != 'refs/heads/master' + && github.ref != 'refs/heads/onie-modernization-2026' }} + +# Nothing here writes to the repository; without this the workflow inherits +# the repository default token scope, which is read/write in many repos. +permissions: + contents: read jobs: + # Skip a push-triggered run when an open pull request in this repository + # already covers the commit -- the pull_request run is the one that reports + # on the PR, so the push run is pure duplicate cost. + # + # Deliberately a query rather than restricting "push:" to the long-lived + # branches. A branch filter also removes CI from every topic branch in a + # FORK, and that is where this workflow does most of its pre-submit work: a + # fork's pull requests are opened against the upstream repository, so they + # raise no pull_request event in the fork, and workflow_dispatch is not + # available there either unless the workflow is also on the fork's default + # branch. A branch filter would leave such a branch with no CI at all. The + # query has no such blind spot: in a fork it simply finds no pull request and + # the push run proceeds. + # + # Only "build" needs to depend on this -- the other jobs chain from it, and a + # skipped job skips its dependents. + gate: + name: Check for a duplicate run + runs-on: ubuntu-latest + timeout-minutes: 5 + # Job-level permissions replace the workflow-level block rather than adding + # to it; this job reads pull requests and checks out nothing. + permissions: + pull-requests: read + outputs: + run: ${{ steps.check.outputs.run }} + steps: + - name: Decide whether this run is needed + id: check + # Everything from the event context is passed via env rather than + # interpolated into the script body, so a branch name can never be + # parsed as shell. + env: + GH_TOKEN: ${{ github.token }} + EVENT: ${{ github.event_name }} + REPO: ${{ github.repository }} + OWNER: ${{ github.repository_owner }} + BRANCH: ${{ github.ref_name }} + run: | + if [ "$EVENT" != push ]; then + echo "$EVENT is not a push; running." + echo "run=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + open="$(gh api --method GET "repos/$REPO/pulls" \ + -f state=open -f head="$OWNER:$BRANCH" --jq 'length')" + if [ "${open:-0}" -gt 0 ]; then + echo "An open pull request in $REPO already covers $BRANCH;" \ + "its pull_request run reports on it. Skipping the push run." + echo "run=false" >> "$GITHUB_OUTPUT" + else + echo "No open pull request in $REPO for $BRANCH; running." + echo "run=true" >> "$GITHUB_OUTPUT" + fi + build: name: Build kvm_x86_64 + needs: gate + if: needs.gate.outputs.run == 'true' runs-on: ubuntu-latest + # A cold build (toolchain + full image) is roughly 45-60 min; cap it so a + # wedged build fails here instead of burning the 6-hour GitHub default. + timeout-minutes: 180 steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -71,6 +146,17 @@ jobs: # environment the toolchain is built in. No loose restore-keys: any # input change forces a clean rebuild rather than restoring a # mismatched toolchain. + # + # kernel-download.make and the arch/machine makefiles are hashed because + # xtools.make derives + # XTOOLS_VERSION = $(ONIE_ARCH)-g$(GCC_VERSION)-lnx$(LINUX_RELEASE)-... + # (and seds CT_LINUX_VERSION into the generated toolchain .config), while + # LINUX_RELEASE lives in kernel-download.make and ONIE_ARCH in + # machine/kvm_x86_64/machine.make. With those omitted a kernel bump kept + # HITTING this key while the build/x-tools/ directory name + # changed underneath it: the restored toolchain was the wrong one, "make + # xtools" rebuilt from scratch, and the save step -- gated on a cache miss + # -- skipped it, so every later run paid the same rebuild forever. - name: Restore cross-toolchain cache id: xtools-cache uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 @@ -82,7 +168,17 @@ jobs: build/x-tools/*/stamp build/x-tools/*/install build/x-tools/*/build/.config - key: onie-xtools-kvm_x86_64-${{ hashFiles('build-config/make/xtools.make', 'build-config/make/crosstool-ng.make', 'build-config/make/compiler.make', 'build-config/conf/crosstool/**', 'patches/crosstool-NG/**', '.github/onie-build/Dockerfile') }} + key: >- + onie-xtools-kvm_x86_64-${{ hashFiles( + 'build-config/make/xtools.make', + 'build-config/make/crosstool-ng.make', + 'build-config/make/compiler.make', + 'build-config/make/kernel-download.make', + 'build-config/arch/x86_64.make', + 'machine/kvm_x86_64/machine.make', + 'build-config/conf/crosstool/**', + 'patches/crosstool-NG/**', + '.github/onie-build/Dockerfile') }} # ONIE drives its build with stamp files compared by mtime. A fresh # checkout gives every repo source file a current mtime, which is newer @@ -122,15 +218,53 @@ jobs: build/x-tools/*/build/.config \ -type f -exec touch -t "$ts" {} + + # Record which toolchains the cache restored, so the check after the + # build can tell whether "make xtools" had to build one the cache did not + # contain -- the symptom of a key that is still missing an input. + - name: Record restored toolchain set + id: xtools-before + if: steps.xtools-cache.outputs.cache-hit == 'true' + run: | + echo "dirs=$(ls -d build/x-tools/*/install 2>/dev/null | sort | tr '\n' ' ')" \ + >> "$GITHUB_OUTPUT" + + # No --privileged: the Dockerfile ends with "USER build", so the build + # runs unprivileged and cannot use any capability --privileged grants. + # It was ineffective as well as an unnecessary escalation -- the + # documented flow (contrib/build-env) builds unprivileged with fakeroot, + # and the image build uses mtools/fakeroot rather than loop mounts. - name: Build cross toolchain run: | - docker run --rm --privileged \ + docker run --rm \ -v "${PWD}:/onie" \ onie-build-env \ bash -lc 'cd build-config && make -j"$(nproc)" \ MACHINE=kvm_x86_64 \ xtools' + # A cache hit that nonetheless had to build a new toolchain means the key + # did not cover something that changed XTOOLS_VERSION. The save step + # cannot rescue that run -- the primary key already exists and GitHub + # will not overwrite it -- so warn instead of rebuilding silently forever. + - name: Check whether the cached toolchain was actually reused + if: steps.xtools-cache.outputs.cache-hit == 'true' + # Passed via env rather than interpolated into the script body, so the + # step output can never be parsed as shell. + env: + RESTORED_DIRS: ${{ steps.xtools-before.outputs.dirs }} + run: | + now="$(ls -d build/x-tools/*/install 2>/dev/null | sort | tr '\n' ' ')" + if [ "$now" != "$RESTORED_DIRS" ]; then + echo "::warning title=Stale cross-toolchain cache key::" \ + "Cache key hit but 'make xtools' built a toolchain the cache" \ + "did not contain (restored: [$RESTORED_DIRS], now: [$now])." \ + "XTOOLS_VERSION changed without changing the cache key, so this" \ + "toolchain cannot be saved and every run will rebuild it." \ + "Add the changed input to the 'Restore cross-toolchain cache' key." + else + echo "Cached toolchain reused as-is: $now" + fi + # Save only on a cache miss, and only after the toolchain build above # succeeded (default if: success()), so a broken toolchain is never # cached. Running before the full build means a later-stage failure @@ -148,9 +282,10 @@ jobs: build/x-tools/*/build/.config key: ${{ steps.xtools-cache.outputs.cache-primary-key }} + # No --privileged, as above. - name: Build ONIE run: | - docker run --rm --privileged \ + docker run --rm \ -v "${PWD}:/onie" \ onie-build-env \ bash -lc 'cd build-config && \ @@ -204,6 +339,9 @@ jobs: name: Boot test kvm_x86_64 needs: build runs-on: ubuntu-latest + # Three boots at up to 300s each plus package install and artifact + # download; this catches a QEMU that outlives the harness's own TIMEOUT. + timeout-minutes: 45 steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -268,6 +406,9 @@ jobs: # time, keeping CI resource use low. needs: boot-test runs-on: ubuntu-latest + # Three boot stages at up to 600s each, plus the embed and install work in + # between. + timeout-minutes: 60 steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 diff --git a/emulation/ci-boot-test.sh b/emulation/ci-boot-test.sh index c247e4a58..a4148b6f6 100755 --- a/emulation/ci-boot-test.sh +++ b/emulation/ci-boot-test.sh @@ -20,13 +20,20 @@ # enrolled. shim still chainloads grub->kernel, so it validates # "does it boot" independent of the SB signature chain. Boots on # the default 'pc' machine (ONIE's onie-vm.sh reference machine). +# Also asserts the guest does NOT report secure boot as active. # secureboot Secure-Boot-ENFORCED: enrolls the demonstration PK/KEK/db from # KEYS_DIR into an OVMF varstore (virt-fw-vars), boots the secboot # OVMF firmware on 'q35,smm=on' with the flash secure flag, and # asserts ONIE boots -- i.e. the shim/grub/kernel signing chain -# verifies under enforcement. Also runs a NEGATIVE control with -# the db omitted, which MUST be rejected (proves SB is enforcing, -# not merely permissive). +# verifies under enforcement. Enforcement itself is asserted from +# inside the guest ("Info: Secure Boot: Active."), so a varstore +# that failed to enroll cannot pass merely by booting unverified. +# Also runs a NEGATIVE control with the db omitted, which MUST be +# rejected (proves SB is enforcing, not merely permissive). +# +# Each boot writes three logs beside : the serial console itself, +# -firmware.log (the OVMF debug console, empty unless the host has a debug OVMF +# build) and -qemu-stderr.log. # # Usage: ci-boot-test.sh [timeout_secs] # Env: SERIAL_LOG= serial log path (default ./onie-boot-serial.log) @@ -53,6 +60,21 @@ BOOT_MODE="${BOOT_MODE:-relaxed}" READY_BOOT='Please press Enter to activate this console|discover: (ONIE|Rescue)|Starting ONIE Service Discovery' READY_NEG='Security Violation|Access Denied|verification failed|GNU GRUB|ONIE: (OS Install|Rescue) Mode' +# A milestone only ONIE userspace can produce. The strings this check used to +# match -- "ONIE: Rescue Mode ..." and "Version :" -- are echoed by GRUB +# itself, from the menuentry and onie_entry_end in +# build-config/recovery/grub-iso.cfg, so both appear before the kernel is even +# loaded: the check duplicated "GRUB reached" and proved nothing about +# userspace. "Info: BIOS mode:" is printed unconditionally during sysinit by +# rootconf/grub-arch/sysroot-lib-onie/init-arch and cannot come from GRUB. +USERSPACE_UP='Info: BIOS mode:' + +# The same init-arch reports the firmware's Secure Boot state, read from the +# SecureBoot EFI variable via efivar. Asserting it in the guest is direct +# evidence the firmware is enforcing, rather than inferring enforcement from +# the negative control's silence. +SB_ACTIVE='Info: Secure Boot: Active\.' + [ -r "$ISO" ] || { echo "ERROR: ISO not readable: $ISO" >&2; exit 2; } # A fixed owner GUID for the enrolled demo keys -- deterministic on purpose so @@ -79,6 +101,34 @@ done WORK="$(mktemp -d)" trap 'rm -rf "$WORK"' EXIT +# vCPU count: two with hardware acceleration, one on TCG. Pure emulation +# starves the guest badly enough that an SB-enforced boot with two vCPUs never +# reaches userspace -- it dies in "rcu: INFO: rcu_sched detected stalls" and +# burns the whole timeout. With identical firmware, varstore and ISO the same +# boot completes in ~31s at -smp 1. CI has /dev/kvm and still gets two; this +# only affects the accel=kvm:tcg fallback used by local runs. +if [ -w /dev/kvm ]; then SMP=2; else SMP=1; fi + +# Firmware (OVMF debug console) log that pairs with a given serial log. +fw_log_for() { echo "${1%.log}-firmware.log"; } + +# Capture the OVMF debug console as a second evidence channel for why the +# firmware stopped. A release OVMF -- what Ubuntu ships, so what CI uses -- +# emits nothing here, and its rejection line does reach ttyS0 anyway +# ("BdsDxe: failed to load Boot0001 ...: Access Denied"), so this is +# belt-and-braces for a debug OVMF build rather than the only place a rejection +# can appear. 0x402 is the I/O port edk2 debug builds log to. Probe for the +# device so this stays strictly additive: on a QEMU without it we lose the +# extra channel instead of failing every boot. Bounded, with stdin closed, so +# a QEMU that fails to exit cannot wedge the harness before any boot starts. +if timeout 10 qemu-system-x86_64 -device help /dev/null \ + | grep -q '"isa-debugcon"'; then + HAVE_DEBUGCON=yes +else + HAVE_DEBUGCON=no + echo " NOTE QEMU has no usable isa-debugcon; firmware console not captured" +fi + # --- boot QEMU headless, serial(ttyS0) -> file, -no-reboot so a panic doesn't # loop. Polls the serial log and stops QEMU as soon as the boot reaches a # terminal state (the , e.g. the console prompt) or panics -- @@ -88,16 +138,41 @@ trap 'rm -rf "$WORK"' EXIT # Args: [extra qemu args...] run_qemu() { local ready_re="$1" code="$2" vars="$3" log="$4"; shift 4 - : >"$log" + local fw_log qerr + fw_log="$(fw_log_for "$log")"; qerr="${log%.log}-qemu-stderr.log" + : >"$log"; : >"$fw_log"; : >"$qerr" + local -a DEBUGCON_ARGS=() + [ "$HAVE_DEBUGCON" = yes ] && \ + DEBUGCON_ARGS=(-debugcon "file:$fw_log" -global isa-debugcon.iobase=0x402) + # Keep QEMU's stderr rather than discarding it, so a QEMU that never ran can + # be told apart from a firmware that refused the image -- previously both + # left an empty serial log, which is what let a broken harness score as a + # successful secure-boot rejection. It also surfaces "Could not access KVM + # kernel module ... falling back to tcg", otherwise invisible. qemu-system-x86_64 \ - -m 2048 -smp 2 \ + -m 2048 -smp "$SMP" \ -drive if=pflash,format=raw,readonly=on,file="$code" \ -drive if=pflash,format=raw,file="$vars" \ -cdrom "$ISO" -boot d \ -netdev user,id=onienet -device virtio-net,netdev=onienet \ -display none -serial "file:$log" -no-reboot \ - "$@" >/dev/null 2>&1 & - local qpid=$! t=0 + "${DEBUGCON_ARGS[@]}" \ + "$@" >/dev/null 2>"$qerr" & + local qpid=$! t=0 rc=0 + # A QEMU that dies this early never ran the firmware: bad arguments, a + # malformed varstore, a missing device model. That is a harness error, not + # a verdict about the image, so fail loudly instead of letting a silent + # serial log be scored as a pass (relaxed) or a rejection (negative + # control). Any early exit counts, including a successful one: rc=0 still + # means no boot happened. + sleep 2 + if ! kill -0 "$qpid" 2>/dev/null ; then + wait "$qpid" 2>/dev/null + rc=$? + echo "ERROR: QEMU exited immediately (rc=$rc) -- harness error, not a boot result" >&2 + sed 's/^/ /' "$qerr" >&2 + exit 2 + fi while kill -0 "$qpid" 2>/dev/null; do if grep -Eaq "$ready_re" "$log" || grep -aq 'Kernel panic' "$log"; then break; fi [ "$t" -ge "$TIMEOUT" ] && break @@ -115,16 +190,38 @@ chk() { #