Re: [PATCH v2 6/8] Support updates via systemd-sysupdate

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 15638 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.appvm-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.

Not any more.

> 9. Call systemd-sysupdate to run the actual update.
>
> sys.appvm-updates uses host-provided information to fetch the update.
> This allows editing files on the host to change the update URL and
> signing key.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
>  host/rootfs/Makefile                               |  2 +
>  host/rootfs/default.nix                            | 28 ++++++-
>  host/rootfs/file-list.mk                           |  4 +
>  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 +++++
>  host/rootfs/image/usr/bin/update                   | 89 ++++++++++++++++++++++
>  host/rootfs/os-release.in                          | 13 ++++
>  host/rootfs/os-release.in.license                  |  2 +
>  host/rootfs/updatevm-url-env                       |  3 +
>  host/rootfs/vm-sysupdate.d/50-verity.transfer      | 18 +++++
>  host/rootfs/vm-sysupdate.d/60-root.transfer        | 18 +++++
>  host/rootfs/vm-sysupdate.d/70-kernel.transfer      | 18 +++++
>  lib/config.default.nix                             |  2 +
>  lib/config.nix                                     | 11 ++-
>  lib/fake-update-signing-key.gpg                    |  1 +
>  lib/fake-update-signing-key.gpg.license            |  2 +
>  release/live/default.nix                           |  4 +-
>  release/live/shell.nix                             |  3 +-
>  vm/app/updates.nix                                 | 37 +++++++++
>  21 files changed, 309 insertions(+), 7 deletions(-)

> diff --git a/host/rootfs/default.nix b/host/rootfs/default.nix
> index b574b8ddf5858867156507429a55b7f537e3c485..0a7638f8d78cf36592c2721d059bc867b04f233c 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
> @@ -36,6 +38,7 @@ let
>      cloud-hypervisor cryptsetup dbus execline inotify-tools iproute2
>      jq mdevd s6 s6-linux-init s6-rc socat spectrum-host-tools
>      virtiofsd xdg-desktop-portal-spectrum-host
> +    btrfs-progs

Let's keep this sorted.

> @@ -79,11 +82,24 @@ 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 {};

I think appvm-sysupdate or appvm-systemd-sysupdate would be clearer.

>    };
>  
>    packagesSysroot = runCommand "packages-sysroot" {
>      depsBuildBuild = [ inkscape ];
>      nativeBuildInputs = [ xorg.lndir ];
> +    env = {
> +      VERSION = config.version;
> +      UPDATE_URL = config.update-url;
> +    };
> +    src = fileset.toSource {
> +      root = ./.;
> +      fileset = fileset.intersection src (fileset.unions [
> +        ./vm-sysupdate.d
> +        ./os-release.in
> +        ./updatevm-url-env
> +      ]);
> +    };
>    } ''
>      mkdir -p $out/usr/bin $out/usr/share/dbus-1/services \
>        $out/usr/share/icons/hicolor/20x20/apps
> @@ -95,8 +111,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
> @@ -118,6 +133,14 @@ let
>      ln -st $out/usr/share/dbus-1/services \
>          ${pkgsGui.xdg-desktop-portal-gtk}/share/dbus-1/services/org.freedesktop.impl.portal.desktop.gtk.service
>  
> +    mkdir -p -- "$out/etc/updatevm/sysupdate.d"
> +    substitute "$src/os-release.in" "$out/etc/os-release" --subst-var VERSION
> +    for d in "$src/vm-sysupdate.d"/*.transfer; do
> +      result_file=''${d#"$src/vm-sysupdate.d/"}
> +      substitute "$d" "$out/etc/updatevm/sysupdate.d/$result_file" --subst-var UPDATE_URL
> +    done
> +    substitute "$src/updatevm-url-env" "$out/etc/updatevm/url-env" --subst-var UPDATE_URL
> +

I think it would make more sense to do these at the Make layer.  It
handles other generated files, so I don't see why it can't handle these
too, and then if I add something to os-release I don't have to rebuild
any Nix stuff.

> diff --git a/host/rootfs/image/etc/fstab b/host/rootfs/image/etc/fstab
> index 6a82ecc85090a37b13603b29f74ca6e554a28c33..78cec99f29dda993ad97048771097121a0e42622 100644
> --- a/host/rootfs/image/etc/fstab
> +++ b/host/rootfs/image/etc/fstab
> @@ -4,3 +4,4 @@ proc	/proc		proc	defaults		0	0
>  devpts	/dev/pts	devpts	defaults,gid=4,mode=620	0	0
>  tmpfs	/dev/shm	tmpfs	defaults		0	0
>  sysfs	/sys		sysfs	defaults		0	0
> +tmpfs	/tmp		tmpfs	defaults,mode=0700	0	0

Is this used?

> diff --git a/host/rootfs/image/usr/bin/update b/host/rootfs/image/usr/bin/update
> new file mode 100755
> index 0000000000000000000000000000000000000000..cbbf8ad8634a7771a0a5f7d6586ee88cdc0672a8
> --- /dev/null
> +++ b/host/rootfs/image/usr/bin/update
> @@ -0,0 +1,89 @@
> +#!/bin/execlineb -WS1
> +# 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.

Not any more.

> +# 9. Call systemd-sysupdate to run the actual update.
> +
> +if { mkdir -p -m 0700 /run/updater }
> +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 }
> +

Wouldn't it break if there's already stuff in it?

I'd do

	foreground { redirfd -w 2 /dev/null btrfs subvolume delete -- shared }
	if { btrfs subvolume create -- shared }

and then you know you've got an empty subvolume.

> +  # Snapshot directory may have files or directories with untrusted names.
> +  # Redirect its output to /dev/null to avoid printing them to the console.
> +  ifelse -n { redirfd -w 2 /dev/null rm -rf -- snapshot } {
> +    foreground { redirfd -w 2 echo "Cannot remove snapshot directory" }
> +    exit 1
> +  }

Why not btrfs subvolume delete?  It's faster and won't print names.

> +
> +  backtick -E update_vm_id_ {
> +    backtick -E id_path { readlink /run/vm/by-name/sys.appvm-updates }
> +    basename -- $id_path
> +  }
> +
> +  multisubstitute {
> +    define fsdir /run/vm/by-id/${update_vm_id_}/fs
> +    define update_vm_id ${update_vm_id_}

Why?

> +    define svcdir /run/service/vmm/instance/${update_vm_id_}

Can also use /run/vm/by-name/sys.appvm-updates/fs and
/run/vm/by-name/sys.appvm-updates/service if you prefer, although you
need to look up the ID for vm-start anyway currently.

> +  }
> +
> +  # $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.
> +
> +  # Set up /etc with what the VM needs.  The VM will overlay this
> +  # on its own /etc.
> +  if { rm -rf -- ${fsdir}/etc }
> +  if { umask 022 mkdir -p -- ${fsdir}/updates ${fsdir}/etc/systemd }
> +  if { cp -R -- /etc/updatevm/sysupdate.d /etc/updatevm/url-env ${fsdir}/etc }
> +  if { cp -- /etc/systemd/import-pubring.gpg ${fsdir}/etc/systemd }

Why copy rather than bind mount?

> +
> +  # If the directory is already mounted, unmount it.  This prevents a
> +  # confusing error from mount.
> +  foreground { redirfd -w 2 /dev/null umount -- ${fsdir}/updates }
> +
> +  # Share the update directory with the VM.
> +  if { mount --bind -- shared ${fsdir}/updates }
> +
> +  # Start the update VM.
> +  if { vm-start $update_vm_id }
> +
> +  # Wait for the VM to exit.
> +  if { s6-svwait -D ${svcdir} }
> +

It might be more robust to use a transient VM, like we use for
AppImages, so that nothing can restart it.  Transient VMs are still
developing though, so it's also fine to say we'll do it this way for now
and adapt it later.  This would also save all the filesystem resetting
you're needing to do here.

> diff --git a/host/rootfs/os-release.in.license b/host/rootfs/os-release.in.license
> new file mode 100644
> index 0000000000000000000000000000000000000000..c4a0586a407fe14c3e0855749a7524ac3871dda4
> --- /dev/null
> +++ b/host/rootfs/os-release.in.license
> @@ -0,0 +1,2 @@
> +SPDX-License-Identifier: CC0-1.0
> +SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>

os-release files can have comments, so no need for a separate license
file here.

> diff --git a/lib/config.nix b/lib/config.nix
> index 01bcfa2bb2d5c412e212f5a60d9032e89c8a7442..5b6b95013734202b7e2e01d5ffce313080658006 100644
> --- a/lib/config.nix
> +++ b/lib/config.nix
> @@ -1,5 +1,6 @@
> -# SPDX-FileCopyrightText: 2023 Alyssa Ross <hi@alyssa.is>
>  # SPDX-License-Identifier: MIT
> +# SPDX-FileCopyrightText: 2024 Alyssa Ross <hi@alyssa.is>
> +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>

Why have I changed from 2023 to 2024?

>  
>  let
>    customConfigPath = builtins.tryEval <spectrum-config>;
> @@ -17,5 +18,11 @@ let
>    callConfig = config: if builtins.typeOf config == "lambda" then config {
>      inherit default;
>    } else config;
> +  finalConfig = default // callConfig config;
>  in
> -  default // callConfig config;
> +  finalConfig // {
> +    update-signing-key = builtins.path {
> +      name = "signing-key";
> +      path = finalConfig.update-signing-key;
> +    };
> +  }

What does this do?

> diff --git a/lib/fake-update-signing-key.gpg b/lib/fake-update-signing-key.gpg
> new file mode 100644
> index 0000000000000000000000000000000000000000..b4c15467614ee15deef02af05f4c6554a1f7a013
> --- /dev/null
> +++ b/lib/fake-update-signing-key.gpg
> @@ -0,0 +1 @@
> +NOT A VALID KEY - UPDATES WILL NOT WORK
> diff --git a/lib/fake-update-signing-key.gpg.license b/lib/fake-update-signing-key.gpg.license
> new file mode 100644
> index 0000000000000000000000000000000000000000..c4a0586a407fe14c3e0855749a7524ac3871dda4
> --- /dev/null
> +++ b/lib/fake-update-signing-key.gpg.license
> @@ -0,0 +1,2 @@
> +SPDX-License-Identifier: CC0-1.0
> +SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com>

Given it's not a valid key anyway might as well just put this in the file.

> diff --git a/release/live/default.nix b/release/live/default.nix
> index dc649732ffa46a998a4a66360aa8ff7ef6bccae0..581420da9acf855d4b3d9ececc1ef406f742fd75 100644
> --- a/release/live/default.nix
> +++ b/release/live/default.nix
> @@ -7,7 +7,7 @@ import ../../lib/call-package.nix (
>  { callSpectrumPackage, spectrum-build-tools, rootfs, src
>  , lib, pkgsStatic, stdenvNoCC
>  , cryptsetup, dosfstools, jq, mtools, util-linux
> -, systemdUkify, version, efi
> +, systemdUkify, config, efi
>  }:
>  
>  let
> @@ -49,7 +49,7 @@ stdenv.mkDerivation {
>      SYSTEMD_BOOT_EFI = "${efi.systemd}/lib/systemd/boot/efi/systemd-boot${efiArch}.efi";
>      EFI_IMAGE = efi;
>      EFINAME = "BOOT${toUpper efiArch}.EFI";
> -    VERSION = version;
> +    VERSION = config.version;
>    };
>  
>    buildFlags = [ "dest=$(out)" ];

Maybe this should be squashed into an earlier patch?

> diff --git a/vm/app/updates.nix b/vm/app/updates.nix
> new file mode 100644
> index 0000000000000000000000000000000000000000..d2c1e5fcb35b37c7ed8a173f19b97894a36a7f0c
> --- /dev/null
> +++ b/vm/app/updates.nix
> @@ -0,0 +1,37 @@
> +# 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, config, curl, lib, src
> +, runCommand, systemd, writeScript
> +}:
> +
> +let
> +  update-url = config.update-url;
> +  mountpoint = "/run/virtiofs/virtiofs0";
> +  sysupdate-path = "${systemd}/lib/systemd/systemd-sysupdate";
> +  runner = writeScript "update-run-script"
> +    ''
> +    #!/usr/bin/execlineb -P
> +    if { mount -toverlay -olowerdir=${mountpoint}/etc:/etc -- overlay /etc }
> +    envfile ${mountpoint}/etc/url-env

Seems like overkill to use an envfile for a single URL?

> +    importas -i update_url UPDATE_URL
> +    if { ${sysupdate-path} update }
> +    if { ${curl}/bin/curl -L --proto =http,https
> +       -o ${mountpoint}/updates/SHA256SUMS.gpg ''${update_url}/SHA256SUMS.gpg }
> +    # systemd-sysupdate recently went from needing SHA256SUMS.gpg to SHA256SUMS.sha256.asc.
> +    # I (Demi) have no need if this is intentional or a bug.  I also have no idea if this
> +    # behavior will stay unchanged in the future.  Therefore, create both files and let
> +    # systemd-sysupdate ignore the one it isn't interested in.
> +    if { ln -f ${mountpoint}/updates/SHA256SUMS.gpg ${mountpoint}/updates/SHA256SUMS.sha256.asc }

Would be good to figure out why that happened.  If we add a comment like
this it's very unlikely to ever get cleaned up.

> +    ${curl}/bin/curl -L --proto =http,https
> +       -o ${mountpoint}/updates/SHA256SUMS ''${update_url}/SHA256SUMS
> +    '';
> +in
> +
> +callSpectrumPackage ../make-vm.nix {} {
> +  providers.net = [ "sys.netvm" ];
> +  type = "nix";
> +  run = "${runner}";

Might as well inline this.

> +}) (_: {})
>
> -- 
> 2.51.2

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