On 11/28/25 08:47, Alyssa Ross wrote: > Demi Marie Obenour writes: > >> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile >> index a6d9f23e9f5277b7c79a53105eb2dfe1bab1451e..74ff64019560aae6387df0e1b3409bc174251bdb 100644 >> --- a/host/rootfs/Makefile >> +++ b/host/rootfs/Makefile >> @@ -10,6 +10,7 @@ include file-list.mk >> ROOT_FS = build >> >> DIRS = \ >> + boot \ >> dev \ >> etc/s6-linux-init/env \ >> etc/s6-linux-init/run-image/configs \ >> @@ -33,13 +34,15 @@ DIRS = \ >> etc/s6-linux-init/run-image/vm/by-id \ >> etc/s6-linux-init/run-image/vm/by-name \ >> ext \ >> + home \ >> proc \ >> run \ >> - sys >> + sys \ >> + tmp >> >> FIFOS = etc/s6-linux-init/run-image/service/s6-svscan-log/fifo >> >> -BUILD_FILES = build/etc/s6-rc >> +BUILD_FILES = build/etc/s6-rc build/etc/os-release build/etc/update-url >> >> # This rule produces three files but Make only (portably) >> # supports one output per rule. Instead of resorting to temporary >> @@ -59,12 +62,22 @@ $(ROOT_FS_IMAGE): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_ >> mkdir -p $(ROOT_FS) && \ >> { \ >> cat $(PACKAGES_FILE) ;\ >> + printf '%s\n%s\n' "$$UPDATE_SIGNING_KEY" /etc/systemd/import-pubring.gpg; \ > > Inconsistent use of shell variable instead of make macro. > >> for file in $(FILES) $(LINKS); do printf '%s\n%s\n' $$file "$${file#image/}"; done ;\ >> for file in $(BUILD_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\ >> printf 'build/empty\n%s\n' $(DIRS) ;\ >> printf 'build/fifo\n%s\n' $(FIFOS) ;\ >> } | ../../scripts/make-erofs.sh $@ >> >> +build/etc/update-url: >> + mkdir -p build/etc >> + # might have metacharacters, so avoid interpolation >> + printf %s\\n "$${UPDATE_URL:?'update URL empty or missing'}" > build/etc/update-url > > I'm learning so many shell parameter expansions I didn't know from you :) > >> diff --git a/host/rootfs/image/usr/bin/spectrum-update b/host/rootfs/image/usr/bin/spectrum-update >> new file mode 100755 >> index 0000000000000000000000000000000000000000..613b43570d0538fce20296ccb1de2a6364e0df55 >> --- /dev/null >> +++ b/host/rootfs/image/usr/bin/spectrum-update >> @@ -0,0 +1,92 @@ >> +#!/bin/execlineb -WS1 >> +# SPDX-License-Identifier: EUPL-1.2+ >> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour >> + >> +if { mkdir -p -m 0700 /run/updater } >> + >> +# Take a global lock to avoid races. >> +s6-setlock /run/update-lock >> + >> +foreground { redirfd -w 2 /dev/null rmdir -- $1 } >> +if { umask 0077 mkdir -p -- $1 } >> +cd $1 >> +foreground { >> + # If this exists already that is okay. >> + foreground { redirfd -w 2 /dev/null btrfs subvolume create -- shared } >> + >> + # Delete any stale temporary files. Delete any existing signature >> + # files. If the VM is still running (it should not be), the VM might >> + # have write access to the directory. However, updates-dir-check is >> + # safe against that. >> + if { updates-dir-check cleanup shared } >> + >> + if { >> + foreground { >> + # TODO: suppress only "subvolume does not exist" errors. >> + redirfd -w 2 /dev/null >> + btrfs subvolume delete snapshot >> + } >> + rm -f snapshot >> + } >> + >> + backtick -E update_vm_id { >> + backtick -E id_path { readlink /run/vm/by-name/sys.appvm-systemd-sysupdate } >> + basename -- $id_path >> + } >> + >> + # $fsdir is read-only to the guest, but read-write to the host. >> + # Directories bind-mounted into it are read-write to the guest. >> + # See etc/s6-linux-init/run-image/service/vhost-user-fs/template/run >> + # for details. >> + > > This still refers to a non-existent variable. > >> + # Set up /etc with what the VM needs. The VM will overlay this >> + # on its own /etc. >> + # >> + # In the future, this should use a bind mount instead of copying >> + # into a tmpfs. However, this would significantly complicate the >> + # cleanup code. Deleting fs/etc would require undoing the bind >> + # mounts instead of rm -rf. Once this code is in a separate mount >> + # namespace, the copies should be replaced by bind mounts. >> + if { >> + if { rm -rf -- /run/vm/by-id/${update_vm_id}/fs/etc } >> + umask 022 >> + if { mkdir -p -- /run/vm/by-id/${update_vm_id}/fs/updates /run/vm/by-id/${update_vm_id}/fs/etc/systemd } >> + if { cp -R -- /etc/vm-sysupdate.d /etc/update-url /run/vm/by-id/${update_vm_id}/fs/etc } >> + cp -- /etc/systemd/import-pubring.gpg /run/vm/by-id/${update_vm_id}/fs/etc/systemd >> + } >> + >> + # If the directory is already mounted, unmount it. This prevents a >> + # confusing error from mount. >> + foreground { redirfd -w 2 /dev/null umount -- /run/vm/by-id/${update_vm_id}/fs/updates } >> + >> + # Share the update directory with the VM. >> + if { mount --bind -- shared /run/vm/by-id/${update_vm_id}/fs/updates } >> + >> + # Start the update VM. >> + if { vm-start $update_vm_id } >> + >> + # Wait for the VM to exit. >> + # TODO: This is racy. If the update finishes before this code runs, >> + # the s6-svwait call will fail. >> + if { s6-svwait -D /run/service/vmm/instance/${update_vm_id} } >> + >> + # Remove the bind mount. >> + if { umount -- /run/vm/by-id/${update_vm_id}/fs/updates } >> + >> + # Ensure that the VM cannot change the directory >> + # while systemd-sysupdate is using it. >> + if { btrfs subvolume snapshot -- shared snapshot } >> + >> + # Validate the update directory. Delete any stale temporary files. >> + # Check that a signature file was downloaded. >> + if { updates-dir-check check snapshot } >> + >> + unshare --mount >> + if { mount --bind -o ro -- snapshot /run/updater } >> + >> + /usr/lib/systemd/systemd-sysupdate update > > Why not just make a readonly snapshot? > (btrfs subvolume snapshot -r) The checker will delete any temporary files it comes across, so it needs write access. A snapshot is much heavier than a bind mount and isn't automatically cleaned up. >> diff --git a/vm/app/systemd-sysupdate/default.nix b/vm/app/systemd-sysupdate/default.nix >> new file mode 100644 >> index 0000000000000000000000000000000000000000..69be0bab500ea2ea6cb3b6d71edbf1a3e7bddbba >> --- /dev/null >> +++ b/vm/app/systemd-sysupdate/default.nix >> @@ -0,0 +1,26 @@ >> +# SPDX-License-Identifier: MIT >> +# SPDX-FileCopyrightText: 2023 Alyssa Ross >> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour >> + >> +import ../../../lib/call-package.nix ( >> +{ callSpectrumPackage, curl, lib, src >> +, runCommand, systemd, writeScript >> +}: >> + >> +let >> + downloadUpdate = builtins.path { >> + name = "download-update"; >> + path = ./download-update; >> + }; > > builtins.path is overkill here surely, as opposed to just writing > ${./download-update} below? ${./download-update} includes the working directory in the Nix store hash, which means that renaming your source tree forces an unnecessary rebuild. builtins.path is the standard way to avoid this. >> +in >> + >> +callSpectrumPackage ../../make-vm.nix {} { >> + providers.net = [ "sys.netvm" ]; >> + type = "nix"; >> + run = writeScript "run-script" '' >> +#!/usr/bin/env -S execlineb -WS0 > > #!/bin/execlineb -WS0 would be fine — we know that'll exist in the VM. Will fix. >> diff --git a/vm/app/systemd-sysupdate/download-update b/vm/app/systemd-sysupdate/download-update >> new file mode 100755 >> index 0000000000000000000000000000000000000000..eada41c6c8ad5edcedd9f4d76b76492e0b8be826 >> --- /dev/null >> +++ b/vm/app/systemd-sysupdate/download-update >> @@ -0,0 +1,68 @@ >> +#!/usr/bin/env -S execlineb -WS0 >> +# SPDX-License-Identifier: EUPL-1.2+ >> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour >> +export LC_ALL C >> +export LANGUAGE C >> +if { mount -toverlay -olowerdir=/run/virtiofs/virtiofs0/etc:/etc -- overlay /etc } >> +backtick tmpdir { mktemp -d /tmp/sysupdate-XXXXXX } >> +# Not a useless use of cat: if there are NUL bytes in the URL >> +# busybox's awk might misbehave. >> +backtick update_url { cat /etc/update-url } >> +if { >> + backtick sed_rhs { >> + # Use awk to both validate the URL and to escape sed metacharacters. >> + # Reject URLs with control characters, query parameters, or fragments. >> + # They *cannot* work and so are rejected to produce better error messages. >> + # >> + # curl rejects control characters with "Malformed input to a URL function". >> + # Fragment specifiers ("#") and query parameters ("?") break concatenating >> + # /SHA256SUMS and /SHA256SUMS.sha256.asc onto the update URL. Also, it is >> + # simpler to reject update URLs that contain whitespace than to try to >> + # escape them. >> + # >> + # Backslash needs to be escaped once for systemd-sysupdate and again for sed. >> + # Ampersand needs to be escaped once for sed. >> + awk "BEGIN { >> + update_url = ENVIRON[\"update_url\"]; >> + if (update_url ~ /^[^\\001-\\040?#\\x7F]+$/) { >> + # Use & to avoid extra escaping (16 or 32 backslashes!) >> + # and a divergence between POSIX and GNU awk. >> + gsub(/\\\\/, \"&&&&\", update_url); >> + gsub(/&/, \"\\\\\\\\&\", update_url); >> + print update_url; >> + exit 0; >> + } else { >> + print ARGV[2] > \"/dev/stderr\"; >> + exit 100; >> + } >> + }" -- $3 >> + "Bad update URL from host: control characters, whitespace, query parameters, and fragment specifiers not allowed" >> + } >> + elglob -w -0 transfer_file_ /etc/vm-sysupdate.d/*.transfer >> + forx -E transfer_file { $transfer_file_ } >> + backtick target_basename { >> + basename -- $transfer_file >> + } >> + multisubstitute { >> + importas -iuS sed_rhs >> + importas -iuS target_basename >> + importas -iuS tmpdir >> + define sed_input $transfer_file >> + } > > You could avoid some serial substitution here if you wanted, by not > passing -E to forx: > > forx transfer_file { $transfer_file_ } > backtick target_basename { > importas -iuS transfer_file > basename -- $transfer_file > } > multisubstitute { > … > } I considered it and decided that the extra define in the multisubstitute was cheaper than the extra importas process. >> + redirfd -w 1 ${tmpdir}/${target_basename} >> + sed -E -- "s#@UPDATE_URL@#${sed_rhs}#g" $sed_input > > Using awk to escape stuff for sed seems a bit Rube Goldberg. Would it > make more sense to just do the replacement in the awk program? Actually > a lot of this might be nicer in awk than execline? Feel free to tell me > to leave it this way for now, though. I'd prefer to leave it this way for now. Maybe add a TODO to clean this up. >> +} >> +multisubstitute { >> + importas -iuS update_url >> + importas -iuS CURL_PATH >> + importas -iuS SYSTEMD_SYSUPDATE_PATH >> + importas -iuS tmpdir >> +} >> +if { $SYSTEMD_SYSUPDATE_PATH --definitions=${tmpdir} update } >> +# [ and ] are allowed in update URLs so that IPv6 addresses work, but >> +# they cause globbing in the curl command-line tool by default. Use --globoff >> +# to disable this feature. >> +if { $CURL_PATH -L --proto-redir =http,https --globoff >> + -o /run/virtiofs/virtiofs0/updates/SHA256SUMS -- ${update_url}/SHA256SUMS } >> +$CURL_PATH -L --proto-redir =http,https --globoff >> + -o /run/virtiofs/virtiofs0/updates/SHA256SUMS.sha256.asc -- ${update_url}/SHA256SUMS.sha256.asc > > Much easier to understand now. Thanks! You're welcome! -- Sincerely, Demi Marie Obenour (she/her/hers)