Okay, I suppose there aren't any firm blockers here either, but something is really going to have to be done about the script that runs in the VM ASAP, because I don't see it being maintainable with this many layers of indirection. Demi Marie Obenour <demiobenour@gmail.com> writes:
Include a new `spectrum-update` command to update the system. This tells the new sys.appvm-systemd-sysupdate VM to download the updates into a staging directory using systemd-sysupdate. The host then runs systemd-sysupdate to apply the updates itself.
sys.appvm-systemd-sysupdate uses host-provided information to fetch the update. This allows editing files on the host to change the update URL and signing key.
Updates require /boot to be mounted so that systemd-sysupdate can update the unified kernel image. They also require that /tmp is writable so
What does "They" mean? What writes to /tmp? systemd-sysupdate?
that they can store temporary files, so put a tmpfs there. Furthermore, there needs to be a directory for storing downloaded updates. Create /home so that users can mount their persistent data there. The directory the VM downloads updates into is *not* reset (wiped) before or after the update. This allows the VM to know if the system is already up to date. Otherwise, it would redownload the entire multi-gigabyte update image.
Updates are currently not compressed. This should be changed in the future, but it would add a small amount of additional complexity. In particular, the script generating the update directory would need to generate a SHA256SUMS containing the hash of both the compressed and uncompressed versions. More importantly, the VM must not be able to make the host use the compressed version. This would be a potential security risk because decompression happens before signature verification. GnuPG currently decompresses signatures, but in the future it will be replaced by Sequoia which does not.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- Changes since v3: - Move builtins.path from lib/config.nix to host/rootfs/default.nix. - Change config options from "update-url" to "updateUrl" and "update-signing-key" to "updateSigningKey".
Changes since v2:
- Generate the transfer files in the guest, not the host. - Do not use an environment file. - Reject URLs that cannot work. - Escape sed metacharacters. - Escape backslashes for systemd-sysupdate. - Do not validate update URLs at build time, only at runtime. - Reject only update URLs that cannot possibly work. - Set the partition UUIDs according to systemd's recommendation. - Do not rely on finding partitions by label. - Strip leading and trailing whitespace from the update URL. - Rename the update command from `update` to `spectrum-update`. - Delete the list of steps. Replace it with comments in the script.
The awk script in the VM rejects URLs that contain whitespace. This is because they can't work, and passing them to systemd-sysupdate would require figuring out how to escape them. Rejecting such bogus URLs is simpler than preventing them from being mangled. --- host/rootfs/Makefile | 19 ++++- host/rootfs/default.nix | 17 +++-- host/rootfs/file-list.mk | 7 ++ host/rootfs/image/etc/fstab | 1 + .../image/etc/sysupdate.d/50-verity.transfer | 20 ++++++ host/rootfs/image/etc/sysupdate.d/60-root.transfer | 20 ++++++ .../image/etc/sysupdate.d/70-kernel.transfer | 20 ++++++ .../image/etc/vm-sysupdate.d/50-verity.transfer | 18 +++++ .../image/etc/vm-sysupdate.d/60-root.transfer | 18 +++++ .../image/etc/vm-sysupdate.d/70-kernel.transfer | 18 +++++
I might be missing something obvious here, but why aren't these part of the VM image? In the patch body you mention editing files on the host, but that's not something you can do on the Spectrum host.
host/rootfs/image/usr/bin/spectrum-update | 83 ++++++++++++++++++++++ host/rootfs/os-release.in | 15 ++++
Putting this in the root is inconsistent with how we handle generated s6-rc files, which live under image/etc/s6-rc.
lib/config.default.nix | 2 + lib/config.nix | 3 +- lib/fake-update-signing-key.gpg | 3 + release/live/shell.nix | 3 +- vm/app/systemd-sysupdate/default.nix | 57 +++++++++++++++ vm/app/systemd-sysupdate/escape-url.awk | 31 ++++++++ .../systemd-sysupdate/populate-transfer-directory | 26 +++++++ 19 files changed, 372 insertions(+), 9 deletions(-)
diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile index d64bce115cc6c306956121b4bcd7271331ba1b7e..1abb18dbf84077af3dbd527a01c02f38c4608e58 100644 --- a/host/rootfs/Makefile +++ b/host/rootfs/Makefile @@ -9,6 +9,7 @@ include file-list.mk ROOT_FS_DIR = build
DIRS = \ + boot \ dev \ etc/s6-linux-init/env \ etc/s6-linux-init/run-image/configs \ @@ -32,13 +33,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 @@ -56,12 +59,22 @@ $(ROOT_FS): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) mkdir -p $(ROOT_FS_DIR) && \ { \ cat $(PACKAGES_FILE) ;\ + printf '%s\n%s\n' "$$UPDATE_SIGNING_KEY" /etc/systemd/import-pubring.gpg; \
$(UPDATE_SIGNING_KEY), for consistency.
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 ;\ + for file in $(BUILD_FILES) $(BUILD_NON_TARGET_FILES); do printf '%s\n%s\n' $$file $${file#build/}; done ;\
I don't see this used anywhere.
printf 'build/empty\n%s\n' $(DIRS) ;\ printf 'build/fifo\n%s\n' $(FIFOS) ;\ } | ../../scripts/make-erofs.sh $(ROOT_FS)
+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 + +build/etc/os-release: + mkdir -p build/etc + sed 's/@VERSION@/$(VERSION)/g' < os-release.in > build/etc/os-release + build/fifo: mkdir -p build mkfifo -m 0600 $@ diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix index cd61c78b1f1668e7bc9c84c638ff6e7d8b6de140..1ebaf11cd7e9d61444b6524de6053a0f3cfb82c8 100644 --- a/host/rootfs/default.nix +++ b/host/rootfs/default.nix @@ -5,6 +5,7 @@ import ../../lib/call-package.nix ( { callSpectrumPackage, spectrum-build-tools, src , pkgsMusl, pkgsStatic, linux_latest +, config }: pkgsStatic.callPackage (
@@ -13,6 +14,7 @@ pkgsStatic.callPackage ( , busybox, cloud-hypervisor, cryptsetup, dbus, execline, inkscape , iproute2, inotify-tools, jq, mdevd, s6, s6-linux-init, socat , util-linuxMinimal, virtiofsd, xorg, xdg-desktop-portal-spectrum-host +, btrfs-progs }:
let @@ -33,8 +35,8 @@ let foot = pkgsGui.foot.override { allowPgo = false; };
packages = [ - cloud-hypervisor cryptsetup dbus execline inotify-tools iproute2 - jq mdevd s6 s6-linux-init s6-rc socat spectrum-host-tools + btrfs-progs cloud-hypervisor cryptsetup dbus execline inotify-tools + iproute2 jq mdevd s6 s6-linux-init s6-rc socat spectrum-host-tools util-linuxMinimal virtiofsd xdg-desktop-portal-spectrum-host
(busybox.override { @@ -80,11 +82,13 @@ let appvm-firefox = callSpectrumPackage ../../vm/app/firefox.nix {}; appvm-foot = callSpectrumPackage ../../vm/app/foot.nix {}; appvm-gnome-text-editor = callSpectrumPackage ../../vm/app/gnome-text-editor.nix {}; + appvm-systemd-sysupdate = callSpectrumPackage ../../vm/app/systemd-sysupdate {}; };
packagesSysroot = runCommand "packages-sysroot" { depsBuildBuild = [ inkscape ]; nativeBuildInputs = [ xorg.lndir ]; + src = builtins.path { name = "os-release"; path = ./os-release.in; };
What does this do?
} '' mkdir -p $out/usr/bin $out/usr/share/dbus-1/services \ $out/usr/share/icons/hicolor/20x20/apps @@ -96,8 +100,7 @@ let done
# If systemd-pull is missing systemd-sysupdate will fail with a - # very confusing error message. If systemd-sysupdate doesn't work, - # users will not be able to receive an update that fixes the problem. + # very confusing error message. for i in sysupdate pull; do if ! cat -- "$out/usr/lib/systemd/systemd-$i" > /dev/null; then echo "link to systemd-$i didn't get installed" >&2
Stray?
diff --git a/host/rootfs/image/etc/vm-sysupdate.d/70-kernel.transfer b/host/rootfs/image/etc/vm-sysupdate.d/70-kernel.transfer new file mode 100644 index 0000000000000000000000000000000000000000..cb181239d71c5a6d0a5b3652d5534a23eda64183 --- /dev/null +++ b/host/rootfs/image/etc/vm-sysupdate.d/70-kernel.transfer @@ -0,0 +1,18 @@ +# SPDX-License-Identifier: CC0-1.0 +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> + +# Uses example code from systemd man pages which is under MIT-0 +# (no attribution required). +[Transfer] +Verify=yes + +[Source] +Type=url-file +Path=@UPDATE_URL@ +MatchPattern=Spectrum_@v.efi + +[Target] +Type=regular-file +Path=/run/virtiofs/virtiofs0/updates +MatchPattern=Spectrum_@v.efi +Mode=0644 diff --git a/host/rootfs/image/usr/bin/spectrum-update b/host/rootfs/image/usr/bin/spectrum-update new file mode 100755 index 0000000000000000000000000000000000000000..ad598b557ac1cc4e9b95ff65a53a68f04d3759ee --- /dev/null +++ b/host/rootfs/image/usr/bin/spectrum-update @@ -0,0 +1,83 @@ +#!/bin/execlineb -WS1 +# SPDX-License-Identifier: EUPL-1.2+ +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> + +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 { + # rm -f ensures that "snapshot" does not exist afterwards. + ifte { exit 0 } { rm -f snapshot } + # TODO: suppress only "subvolume does not exist" errors. + redirfd -w 2 /dev/null btrfs subvolume delete snapshot
Why have a redundant if-true case rather than something like this? if { foreground { 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.
There is no fsdir variable, and references to full paths like this are likely to go stale — in fact this one already has. Probably this would belong better in a dedicated documentation page about VM filesystems?
+ + # Set up /etc with what the VM needs. The VM will overlay this + # on its own /etc. + 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
These could all be bind mounts rather than copies into RAM.
+ } + + # 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 }
Hopefully at some point we can use mount namespaces or something to make this sort of thing impossible. (Probably easier once we use a transient VM for this.)
+ # 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. + if { s6-svwait -D /run/service/vmm/instance/${update_vm_id} }
Technically there's a race here. The whole update could finish in between these two lines, as unlikely as that is. Again avoidable by using a transient VM in future.
+ # 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 } + + # Perform the update in a separate mount namespace.
This comment doesn't add much — I can tell that the update is being performed in a separate mount namespace from the next three lines, which switch into a separate mount namespace and run the update.
diff --git a/lib/fake-update-signing-key.gpg b/lib/fake-update-signing-key.gpg new file mode 100644 index 0000000000000000000000000000000000000000..12e18f4c7c740e31692e1f1975282fa72ac1f2e3 --- /dev/null +++ b/lib/fake-update-signing-key.gpg @@ -0,0 +1,3 @@ +SPDX-License-Identifier: CC0-1.0 +SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> +NOT A VALID KEY - UPDATES WILL NOT WORK diff --git a/release/live/shell.nix b/release/live/shell.nix index ffaa9a571c662810348822a5952d479d251a25e5..b263eacc4d0324191e3c7737dd90d304e477e79b 100644 --- a/release/live/shell.nix +++ b/release/live/shell.nix @@ -2,7 +2,7 @@ # SPDX-FileCopyrightText: 2021-2024 Alyssa Ross <hi@alyssa.is>
import ../../lib/call-package.nix ( -{ callSpectrumPackage, stdenv, qemu_kvm }: +{ callSpectrumPackage, config, stdenv, qemu_kvm }:
let efi = callSpectrumPackage ../../host/efi.nix {}; @@ -17,6 +17,7 @@ in OVMF_CODE = "${qemu_kvm}/share/qemu/edk2-${stdenv.hostPlatform.qemuArch}-code.fd"; ROOT_FS_DIR = efi.rootfs; EFI_IMAGE = efi; + VERSION = config.version; }; } )) (_: {})
Is this supposed to be in a previous patch?
diff --git a/vm/app/systemd-sysupdate/default.nix b/vm/app/systemd-sysupdate/default.nix new file mode 100644 index 0000000000000000000000000000000000000000..04df283f09a1f1ece9197e275d562193af170982 --- /dev/null +++ b/vm/app/systemd-sysupdate/default.nix @@ -0,0 +1,57 @@ +# SPDX-License-Identifier: MIT +# SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> + +import ../../../lib/call-package.nix ( +{ callSpectrumPackage, curl, lib, src +, runCommand, systemd, writeScript +}: + +let + escape-url = builtins.path { + name = "escape-url"; + path = ./escape-url.awk; + }; + populate-transfer-directory = builtins.path { + name = "populate-transfer-directory"; + path = ./populate-transfer-directory; + }; +in + +callSpectrumPackage ../../make-vm.nix {} { + providers.net = [ "sys.netvm" ]; + type = "nix"; + run = writeScript "run-script" '' +#!/usr/bin/execlineb -P +export LC_ALL C +export LANGUAGE C +if { mount -toverlay -olowerdir=/run/virtiofs/virtiofs0/etc:/etc -- overlay /etc } +backtick tmpdir { mktemp -d /run/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 } +# Leading and trailing whitespace is almost certainly user error, +# but be friendly to the user (by stripping it) rather than failing.
I would rather fail. It's one thing to prevent valid URLs being silently mishandled, but I don't think our configuration is the right place for Postel's law, which is an invitation to more and more complexity over time to try to handle inputs that shouldn't have been valid in the first place.
+backtick update_url { + awk "BEGIN { + url = ENVIRON[\"update_url\"] + gsub(/^[[:space:]]+/, \"\", url) + gsub(/[[:space:]]+$/, \"\", url) + print url + }" +} +multisubstitute { + importas -iSu tmpdir + importas -iSu update_url +} +if { ${populate-transfer-directory} ${escape-url} /etc/vm-sysupdate.d ''${tmpdir} ''${update_url} }
These variables don't all need to be wrapped in braces, incurring the necessary corresponding Nix escaping.
+if { ${systemd}/lib/systemd/systemd-sysupdate --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. Only allow HTTP and HTTPS protocols on redirection.
"Only allow HTTP and HTTPS protocols on redirection" is redundant as a comment because it contains no information that isn't obvious from --proto-redir =http,https.
+if { ${curl}/bin/curl -L --proto-redir =http,https --globoff + -o /run/virtiofs/virtiofs0/updates/SHA256SUMS -- ''${update_url}/SHA256SUMS } +${curl}/bin/curl -L --proto-redir =http,https --globoff + -o /run/virtiofs/virtiofs0/updates/SHA256SUMS.sha256.asc -- ''${update_url}/SHA256SUMS.sha256.asc +''; +}) (_: {})
Okay, this is really pushing the limits of the make-vm.nix interface. Having to pass around the paths to different scripts was a warning sign that there should be a better way here. The simplest way to make this nicer would probably be to writeScript a very simple script that sets environment variables pointing to the store paths of the other components, and any other information we want to pass in from Nix, and then execs a script that lives in its own file.
diff --git a/vm/app/systemd-sysupdate/escape-url.awk b/vm/app/systemd-sysupdate/escape-url.awk new file mode 100644 index 0000000000000000000000000000000000000000..8edd816a20ceefa08ecc7f1bc2d1cfbe33fa8a89 --- /dev/null +++ b/vm/app/systemd-sysupdate/escape-url.awk @@ -0,0 +1,31 @@ +#!/usr/bin/awk -f +# SPDX-License-Identifier: EUPL-1.2+ +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> +BEGIN { + update_url = ARGV[1]; + # Check for a GNU awk misfeature + newline = "\n"; + # 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. + if (update_url ~ /^[^\001-\040?#\x7F]+$/) { + # Backslashes are special to systemd-sysupdate. + # Use \\\\& because without the & the result is + # not portable between GNU awk and non-GNU awk. + gsub(/\\/, "\\\\&", update_url); + # "&" and "\\" are special on the RHS of a sed substitution + # and must be escaped with another backslash. The delimiter + # ("#" in this case) and "\n" must also be escaped, but they + # were rejected above so don't bother. + gsub(/[&\\]/, "\\\\&", update_url); + printf "%s", update_url; + exit 0; + } else { + print "Bad update URL from host: control characters, whitespace, query parameters, and fragment specifiers not allowed" > "/dev/stderr"; + exit 100; + } +} diff --git a/vm/app/systemd-sysupdate/populate-transfer-directory b/vm/app/systemd-sysupdate/populate-transfer-directory new file mode 100755 index 0000000000000000000000000000000000000000..f8e515c1a69b5a6a292cc3a4d387d501f1c6a3fe --- /dev/null +++ b/vm/app/systemd-sysupdate/populate-transfer-directory @@ -0,0 +1,26 @@ +#!/usr/bin/env -S execlineb -WS4 +# SPDX-License-Identifier: EUPL-1.2+ +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> +# $1: awk script name +# $2: transfer directory +# $3: target directory +# $4: update URL +export LC_ALL C +export LANGUAGE C +backtick -N sed_rhs { + # Use awk to both validate the URL and to escape sed metacharacters. + awk -f $1 -- $4 +} +export tmpdir $3 +elglob -w -0 transfer_file_ ${2}/*.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 source $transfer_file +} +redirfd -w 1 ${tmpdir}/${target_basename} sed -E -- "s#@UPDATE_URL@#${sed_rhs}#g" $source
Running an awk script, passed in as a parameter, to generate a sed expression, is very, very challenging to understand. Can't we do this in Nix, and save the work at runtime anyway?