Re: [PATCH v5 11/13] Support updates via systemd-sysupdate

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 10903 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> 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 <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 {
> +    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)

> 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 <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
> +  downloadUpdate = builtins.path {
> +    name = "download-update";
> +    path = ./download-update;
> +  };

builtins.path is overkill here surely, as opposed to just writing
${./download-update} below?

> +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.

> 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 <demiobenour@gmail.com>
> +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 {
  …
}

> +  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.

> +}
> +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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]