Re: [PATCH 7/7] Support updates via systemd-sysupdate

[Alyssa Ross <hi@alyssa.is>]

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

Demi Marie Obenour <demiobenour@gmail.com> writes:

> Include a new 'update' command to update the system.  This works as
> follows:
>
> 1. Take a global, system-wide lock.
> 2. Create a BTRFS subvolume for the sys.updates VM to write the updates.
> 3. Bind-mount this subvolume into the VM's shared directory.
> 4. Start sys.updates to get the updates.
> 5. Wait for the VM to shut down.
> 6. Take a BTRFS snapshot of the subvolume.
> 7. Call syncfs() to flush all of the data on the subvolume.

Why do we need to do this?

> 8. Inspect the contents of the subvolume.
>    Check that everything is a regular file and that the names are reasonable.
>    Check that SHA256SUMS and SHA256SUMS.gpg are present.
> 9. Call systemd-sysupdate to run the actual update.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  host/rootfs/Makefile                               |  8 ++-
>  host/rootfs/default.nix                            | 17 ++++---
>  host/rootfs/file-list.mk                           |  5 ++
>  host/rootfs/image/etc/fstab                        |  1 +
>  .../image/etc/sysupdate.d/50-verity.transfer       | 21 ++++++++
>  host/rootfs/image/etc/sysupdate.d/60-root.transfer | 21 ++++++++
>  .../image/etc/sysupdate.d/70-kernel.transfer       | 25 ++++++++++
>  host/rootfs/image/usr/bin/run-update               | 54 ++++++++++++++++++++

This doesn't seem to be used anywhere, and is an exact copy of
run-appimage aside from being non-executable.

>  host/rootfs/image/usr/bin/update                   | 56 +++++++++++++++++++++
>  host/rootfs/image/usr/bin/vm-start                 | 25 +++++++++-
>  host/rootfs/os-release.in                          | 13 +++++
>  host/rootfs/os-release.in.license                  |  2 +
>  host/rootfs/shell.nix                              |  2 +-
>  update-signing-keys.gpg                            |  1 +
>  update-signing-keys.gpg.license                    |  2 +
>  update-url                                         |  1 +
>  update-url.license                                 |  2 +
>  vm/app/sysupdate.d/50-verity.transfer              | 18 +++++++
>  vm/app/sysupdate.d/60-root.transfer                | 18 +++++++
>  vm/app/sysupdate.d/70-kernel.transfer              | 18 +++++++
>  vm/app/updates.nix                                 | 57 ++++++++++++++++++++++

This should be vm/app/updates/default.nix and vm/app/updates/sysupdate.d.

>  21 files changed, 356 insertions(+), 11 deletions(-)
>
> diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile
> index 4712d9063e9f2e3c9b8b7b4fb2a7e54d119c6840..2faa1e46c1a3bbbdf31baf1e972d9b4ecb389ae5 100644
> --- a/host/rootfs/Makefile
> +++ b/host/rootfs/Makefile
> @@ -10,6 +10,7 @@ include file-list.mk
>  dest = build/rootfs.erofs
>  
>  DIRS = \
> +	boot \
>  	dev \
>  	etc/s6-linux-init/env \
>  	etc/s6-linux-init/run-image/configs \
> @@ -47,11 +48,13 @@ DIRS = \
>  
>  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
>  
>  $(dest): ../../scripts/make-erofs.sh $(PACKAGES_FILE) $(FILES) $(BUILD_FILES) build/empty build/fifo file-list.mk
> +	set -euo pipefail; \
>  	{ \
>  	    cat $(PACKAGES_FILE) ;\
> +	    printf '%s\n%s\n' ../../update-signing-keys.gpg /etc/systemd/import-pubring.gpg; \

Probably should just be a normal file in image/etc.

>  	    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) ;\
> @@ -99,6 +102,9 @@ debug:
>  	    $(VMLINUX)
>  .PHONY: debug
>  
> +build/etc/os-release: os-release.in build/etc
> +	sed 's#@VERSION@#$(VERSION)#g' < os-release.in > $@
> +
>  run: build/live.img $(EXT_FS) build/rootfs.verity.roothash ../../lib/kcmdline-utils.mk
>  	@set -x && \
>  	ext="$$(mktemp build/spectrum-rootfs-extfs.XXXXXXXXXX.img)" && \
> diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix
> index cb39f0d77b6640198da3ab840a2c8ca7cc1c91a1..c412fe17f45bde79f1efa42cadb29cfd5fbc3991 100644
> --- a/host/rootfs/default.nix
> +++ b/host/rootfs/default.nix
> @@ -8,10 +8,10 @@ import ../../lib/call-package.nix (
>  }:
>  pkgsStatic.callPackage (
>  
> -{ busybox, cloud-hypervisor, cryptsetup, dbus, erofs-utils, execline
> -, inkscape, inotify-tools, iproute2, jq, lib, mdevd, nixos
> -, runCommand, s6, s6-linux-init, s6-rc, socat, spectrum-host-tools
> -, stdenvNoCC, util-linux, virtiofsd, writeClosure
> +{ btrfs-progs, bash, busybox, cloud-hypervisor, cryptsetup, dbus
> +, erofs-utils, execline, inkscape, inotify-tools, iproute2, jq, lib
> +, mdevd, nixos, runCommand, s6, s6-linux-init, s6-rc, socat
> +, spectrum-host-tools, stdenvNoCC, util-linux, virtiofsd, writeClosure
>  , xdg-desktop-portal-spectrum-host, xorg
>  }:
>  let
> @@ -40,8 +40,8 @@ let
>    no_pgo_foot = foot.override { allowPgo = false; };
>  
>    packages = [
> -    cloud-hypervisor crosvm cryptsetup dbus execline inotify-tools
> -    iproute2 jq mdevd s6 s6-linux-init s6-rc socat
> +    btrfs-progs cloud-hypervisor crosvm cryptsetup dbus execline
> +    inotify-tools iproute2 jq mdevd s6 s6-linux-init s6-rc socat
>      spectrum-host-tools virtiofsd xdg-desktop-portal-spectrum-host
>  
>      (busybox.override {
> @@ -90,6 +90,7 @@ 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-updates = callSpectrumPackage ../../vm/app/updates.nix {};
>    };
>  
>    packagesSysroot = runCommand "packages-sysroot" {
> @@ -135,13 +136,15 @@ stdenvNoCC.mkDerivation {
>        ../../lib/common.mk
>        ../../lib/kcmdline-utils.mk
>        ../../lib/verity.mk
> +      ../../lib/kcmdline-utils.mk

Duplicate.

>        ../../scripts/make-erofs.sh
>        ../../version
> +      ../../update-signing-keys.gpg
>      ]);
>    };
>    sourceRoot = "source/host/rootfs";
>  
> -  nativeBuildInputs = [ erofs-utils spectrum-build-tools s6-rc ];
> +  nativeBuildInputs = [ erofs-utils spectrum-build-tools s6-rc bash btrfs-progs ];

What are these used for at build time?

> diff --git a/host/rootfs/image/etc/sysupdate.d/50-verity.transfer b/host/rootfs/image/etc/sysupdate.d/50-verity.transfer
> new file mode 100644
> index 0000000000000000000000000000000000000000..07a698f3956e19f9f55efff52db51128c16a5b56
> --- /dev/null
> +++ b/host/rootfs/image/etc/sysupdate.d/50-verity.transfer
> @@ -0,0 +1,21 @@
> +# 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]
> +ProtectVersion=%A
> +Verify=yes

Defaults to yes, and is unlikely to change.

> +
> +[Source]
> +Type=url-file
> +Path=file:///run/updater

The docs for url-file say "referenced via a HTTP or HTTPS URL".  Should
we submit a PR to clarify file:// is also supported?  AIUI Lennart is
basically on board with our approach here.

> +MatchPattern=Spectrum_OS_@v.verity
> +
> +[Target]
> +Type=partition
> +Path=auto
> +MatchPattern=Spectrum_OS_@v.verity
> +MatchPartitionType=root-verity
> +PartitionFlags=0
> +ReadOnly=1
> diff --git a/host/rootfs/image/etc/sysupdate.d/70-kernel.transfer b/host/rootfs/image/etc/sysupdate.d/70-kernel.transfer
> new file mode 100644
> index 0000000000000000000000000000000000000000..6f75dfb04abf5ae911be3ae95318685321a86f5f
> --- /dev/null
> +++ b/host/rootfs/image/etc/sysupdate.d/70-kernel.transfer
> @@ -0,0 +1,25 @@
> +# 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]
> +ProtectVersion=%A
> +Verify=yes
> +
> +[Source]
> +Type=url-file
> +Path=file:///run/updater
> +MatchPattern=Spectrum_OS_@v.efi
> +
> +[Target]
> +Type=regular-file
> +Path=/EFI/Linux
> +PathRelativeTo=boot
> +MatchPattern=Spectrum_OS_@v+@l-@d.efi \
> +             Spectrum_OS_@v+@l.efi \
> +             Spectrum_OS_@v.efi
> +Mode=0644
> +TriesLeft=3
> +TriesDone=0

Boot counting would be cool, but maybe we should leave it out for now
and implement it all at once later?  (Or is this a complete
implementation?)

> diff --git a/host/rootfs/image/usr/bin/update b/host/rootfs/image/usr/bin/update
> new file mode 100755
> index 0000000000000000000000000000000000000000..8e147929cecbef5873cd02c946adf1355da444c6
> --- /dev/null
> +++ b/host/rootfs/image/usr/bin/update
> @@ -0,0 +1,56 @@
> +#!/bin/execlineb -WS1

Oh TIL execlineb -W.  We should probably set that in more places.

> +# SPDX-License-Identifier: EUPL-1.2+
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +# Steps:
> +#
> +# 1. Take a global, system-wide lock.
> +# 2. Create a BTRFS subvolume for the sys.updates VM to write the updates.
> +# 3. Bind-mount this subvolume into the VM's shared directory.
> +# 4. Start sys.updates to get the updates.
> +# 5. Wait for the VM to shut down.
> +# 6. Take a BTRFS snapshot of the subvolume.
> +# 7. Call syncfs() to flush all of the data on the subvolume.
> +# 8. Inspect the contents of the subvolume.
> +#    Check that everything is a regular file and that the names are reasonable.
> +#    Check that SHA256SUMS and SHA256SUMS.gpg are present.
> +# 9. Call systemd-sysupdate to run the actual update.
> +
> +if { mkdir -p -m 0700 /run/updater }
> +if {
> +  case $1 {
> +    /[0-9A-Za-z._/-]+ { true }
> +  }
> +  foreground { fdmove -c 1 2 echo 'Update directory path has forbidden characters or is not absolute' }
> +  exit 1
> +}

Why are we trying to protect against the user here?

> +execline-cd $1

Just cd is fine.

> +s6-setlock /run/update-lock
> +foreground {
> +  # This might fail with a "File exists" error, but that is fine.
> +  foreground { redirfd -w 2 /dev/null btrfs subvolume create -- shared }
> +  if { umask 0022 mkdir -p shared/etc/systemd shared/update-destination }
> +  # TODO: use a safe copy program that is not vulnerable to symlink attacks.
> +  # This should be okay as the directory has not been shared yet, but better
> +  # safe than sorry.  Also nosymfollow should be a mitigation, but still,
> +  # better safe than sorry.

If we think it's currently safe, I'd rather not have the comment.  I
don't like TODOs that end up sticking around forever.

> +  if { cp /etc/systemd/import-pubring.gpg shared/etc/systemd }

Might as well bind mount this and save the copy?
If we run in a namespace it'll be cleaned up when we exit.
(Maybe I'm over-enthusiastic about bind mounts…)

> +  if {

Unnecessary if I think?

> +    if {
> +      backtick -E update_vm_id {
> +        backtick -E id_path { readlink /run/vm/by-name/sys.appvm-updates }
> +        basename -- $id_path
> +      }
> +      vm-start $update_vm_id shared

Okay, so I guess run-update is unused and included by default?

> +    }
> +    if { btrfs subvolume snapshot -- shared private }

Can I suggest "snapshot" as the name?  And a comment about why we do it
would be nice.

> +    if { sync -- private }
> +    if { updates-dir-check private/update-destination }
> +    unshare --mount
> +    if { mount --bind -o ro -- private/update-destination /run/updater }
> +    /usr/lib/systemd/systemd-sysupdate update
> +  }
> +}
> +importas -i sysupdate_exit_status "?"

No need to quote ? in execline.

> diff --git a/host/rootfs/image/usr/bin/vm-start b/host/rootfs/image/usr/bin/vm-start
> index 67480e5215d8a8260ce3f03c67f71ba8f210c291..8ae8d94203345c4f3e8b6e46de0d139fda6c11d6 100755
> --- a/host/rootfs/image/usr/bin/vm-start
> +++ b/host/rootfs/image/usr/bin/vm-start
> @@ -1,6 +1,24 @@
> -#!/bin/execlineb -S1
> +#!/bin/execlineb -Ws0
>  # SPDX-License-Identifier: EUPL-1.2+
>  # SPDX-FileCopyrightText: 2022-2023, 2025 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>
> +
> +if {
> +  case "${#}" {
> +    1 { true }
> +    2 {
> +      multisubstitute {
> +        define sourcedir ${2}
> +        define fsdir /run/vm/by-id/${1}/fs/user
> +      }
> +      if { mkdir -p ${fsdir} }
> +      foreground { redirfd -w 2 /dev/null umount ${fsdir} }
> +      mount --bind -- ${sourcedir} ${fsdir}
> +    }
> +  }
> +  foreground { fdmove -c 1 2 echo "Bad number of arguments ${#} (expected 1 or 3)" }
> +  exit 100
> +}
>  
>  foreground { s6-rc -bu change vm-env }
>  
> @@ -20,4 +38,7 @@ foreground {
>    redirfd -w 2 /dev/null
>    s6-svwait -U /run/service/vmm/instance/${1}
>  }
> -ch-remote --api-socket /run/vm/by-id/${1}/vmm boot
> +if { ch-remote --api-socket /run/vm/by-id/${1}/vmm boot }
> +case "${#}" {
> +  2 { s6-svwait -D /run/service/vmm/instance/${1} }
> +}

Hmm.  I don't think this belongs in vm-start.  You should be able to
interact with the fs directory before starting the VM, outside of
vm-start.

> diff --git a/host/rootfs/os-release.in b/host/rootfs/os-release.in
> new file mode 100644
> index 0000000000000000000000000000000000000000..8a167a39366dedc6ff9024efdb98383ec84618ec
> --- /dev/null
> +++ b/host/rootfs/os-release.in
> @@ -0,0 +1,13 @@
> +NAME="Spectrum OS"
> +ID="spectrum"

The os-release(5) examples don't quote where unnecessary so let's follow
that convention.

> +PRETTY_NAME="Spectrum OS @VERSION@"
> +VERSION=@VERSION@
> +VERSION_ID=@VERSION@
> +IMAGE_ID=spectrum_os_@VERSION@

Reading the documentation it doesn't look like a version is supposed to
be included here, and that's what IMAGE_VERSION is for?

> +IMAGE_VERSION=@VERSION@
> +RELEASE_TYPE=development
> +HOME_URL="https://www.spectrum-os.org"

No www is canonical.

> diff --git a/host/rootfs/shell.nix b/host/rootfs/shell.nix
> index bd234e90ee19bdfa6591d29c518cb0dc393b01c8..b5ed566062da03944567dd610c88e1a58523e303 100644
> --- a/host/rootfs/shell.nix
> +++ b/host/rootfs/shell.nix
> @@ -3,7 +3,7 @@
>  # SPDX-FileCopyrightText: 2022 Unikie
>  
>  import ../../lib/call-package.nix (
> -{ callSpectrumPackage, rootfs, pkgsStatic, srcOnly, stdenv
> +{ callSpectrumPackage, rootfs, srcOnly, stdenv
>  , btrfs-progs, cryptsetup, jq, netcat, qemu_kvm, reuse, util-linux
>  }:
>  

Good catch but doesn't belong in this change.

> diff --git a/vm/app/sysupdate.d/50-verity.transfer b/vm/app/sysupdate.d/50-verity.transfer
> new file mode 100644
> index 0000000000000000000000000000000000000000..e437860426b8a651ca20ee7bddff1a9b3cf39507
> --- /dev/null
> +++ b/vm/app/sysupdate.d/50-verity.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_OS_@v.verity
> +
> +[Target]
> +Type=regular-file
> +Path=/run/virtiofs/virtiofs0/user/update-destination

What does "user" mean here?

> diff --git a/vm/app/updates.nix b/vm/app/updates.nix
> new file mode 100644
> index 0000000000000000000000000000000000000000..30beca30e578b5c869eaedf2fd7e8913bf616a0c
> --- /dev/null
> +++ b/vm/app/updates.nix
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: MIT
> +# SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is>
> +
> +import ../../lib/call-package.nix (
> +{ callSpectrumPackage, lib, pkgsMusl, pkgsStatic, src, writeScript, systemd }:
> +
> +pkgsMusl.callPackage (
> +{ stdenvNoCC, curl }:

App VMs are Glibc, so no need for this callPackage.

> +
> +pkgsStatic.callPackage (
> +{ execline, runCommand }:

This looks vestigial.

> +
> +let
> +  raw_update_url = builtins.readFile ../../update-url;
> +  update-url =
> +    if builtins.match "^https?://([[:alnum:]:./?=~-]|%[[:xdigit:]]{2})+/\n$" raw_update_url == null then
> +      builtins.abort "Bad update URL"
> +    else
> +      builtins.substring 0 (builtins.stringLength raw_update_url - 1) raw_update_url;

Let's just inline the URL here.  We definitely do not need validations
in Nix code.

> +  sysupdate-d = stdenvNoCC.mkDerivation {
> +    name = "spectrum-systemd-transfer-files";
> +    src = ./.;
> +    installPhase =
> +      ''
> +      mkdir -- "$out"
> +      (
> +        cd -- "$src" &&
> +        for i in sysupdate.d/*.transfer; do
> +          s=''${i#sysupdate.d/} &&
> +          sed 's,@UPDATE_URL@,${update-url},g' < "$i" > "$out/$s" || exit
> +        done
> +        printf %s\\n '${update-url}' > "$out/update-url"
> +      ) || exit
> +      '';
> +  };

We have pkgs.substitute and friends for this.

> +  l = lib.escapeShellArgs;
> +  mountpoint = "/run/virtiofs/virtiofs0/user";
> +  sysupdate-path = "${systemd}/lib/systemd/systemd-sysupdate";
> +  runner = writeScript "update-run-script" (
> +    "#!/bin/sh --\n" +
> +    builtins.concatStringsSep " && \\\n" [
> +      (l ["mount" "-toverlay" "-olowerdir=${mountpoint}/etc:/etc" "--" "overlay" "/etc"])
> +      (l [sysupdate-path "--definitions=${sysupdate-d}" "update"])
> +      (l ["${curl}/bin/curl" "-L" "--proto" "=http,https"
> +          "-o" "${mountpoint}/update-destination/SHA256SUMS.gpg"
> +          "--" "${update-url}SHA256SUMS.gpg"])
> +      (l ["${curl}/bin/curl" "-L" "--proto" "=http,https"
> +          "-o" "${mountpoint}/update-destination/SHA256SUMS"
> +          "--" "${update-url}/SHA256SUMS"])

Why is this a shell script?

> +    ]);
> +in
> +
> +callSpectrumPackage ../make-vm.nix {} {
> +  providers.net = [ "sys.netvm" ];
> +  type = "nix";
> +  run = "${runner}";
> +}) {}) {}) (_: {})
>
> -- 
> 2.51.2

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