Properly handle fwupd update capsules, take 2

[lilyinstarlight]

This is a mostly verbatim copy of #113 (credit to @dasJ for most all of this) that has been updated to use NixOS/nixpkgs#220555 (which has hit nixos-unstable now). It has been successfully tested with a BIOS update (with secureboot enforcing) on a 12th-gen Framework

Also we probably should update the lockfile in this repo first (or let Renovate bot handle it), since this will probably fail eval without that PR

[blitz]

Thanks for bringing this back from the dead! :)

Can you please run nix flake update --commit-lock-file? Otherwise, as you already say, this doesn't evaluate. You can use nix -L flake check to see whether everything works. Unfortunately, Hercules CI didn't run on this PR. :(

I've enabled lock file maintenance for Renovate, but it will take some time before the bot comes by again.

[lilyinstarlight]

Can you please run nix flake update --commit-lock-file? Otherwise, as you already say, this doesn't evaluate. You can use nix -L flake check to see whether everything works. Unfortunately, Hercules CI didn't run on this PR. :(

It looks like https://github.com/RaitoBezarius/nixpkgs/tree/simplified-qemu-boot-disks (cc @RaitoBezarius) needs to be rebased since the checks actually use that branch rather than nixos-unstable-small

Edit: I'll go ahead and convert to draft until this is done

[RaitoBezarius]

Ugh, I need to get this merged. I will do it, apologies for the inconveniences.

[lilyinstarlight]

Ugh, I need to get this merged. I will do it, apologies for the inconveniences.

No worries! I appreciate your work on this :)

NixOS/nixpkgs#221155 just got merged and we'll want that too, so it'll be a bit before that hits nixos-unstable and then we'll need to update the flake inputs

[lilyinstarlight]

NixOS/nixpkgs#221155 has made it to nixos-unstable

@RaitoBezarius What is the best option for using an updated nixpkgs input, given the tests are running from that one branch in your fork? (e.g. should that one branch just be rebased?)

[RaitoBezarius]

I will rebase that branch and push for the merge of NixOS/nixpkgs#207039 so we won't need that branch anymore. I will take care of this tomorrow morning CEST. So we can get this merged soon and unblock everyone on that. Le mer. 15 mars 2023 à 21:21, Lily Foster ***@***.***> a écrit :

…

NixOS/nixpkgs#221155 <NixOS/nixpkgs#221155> has made it to nixos-unstable @RaitoBezarius <https://github.com/RaitoBezarius> What is the best option for using an updated nixpkgs input, given the tests are running from that one branch in your fork? (e.g. should that one branch just be rebased?) — Reply to this email directly, view it on GitHub <#131 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACMZRFYP6VPMOHBGV2YA4LW4IQF7ANCNFSM6AAAAAAVZZCXTI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lilyinstarlight]

I've updated the branch for the nixpkgs-test input in the flake to NixOS/nixpkgs#207039 and also pinned the crane input to the last good revision because it is currently broken with latest HEAD (ref: ipetkov/crane#262)

The flake checks all pass and this otherwise should be good now!

[lilyinstarlight]

@dasJ, just curious is serviceConfig.RuntimeDirectory needed?

This seems to clash because the built-in unit already sets RuntimeDirectory to motd.d and so systemd appends them both separated by a :, which fwupd doesn't understand how to process multiple runtime dirs (see here) and so assumes /run/fwupd-efi:/run/motd.d is literal and makes the motd file in that suspicious-looking directory /run/fwupd-efi:/run/motd.d/85-fwupd

[lilyinstarlight]

Ah I think it was added initially to make sure that directory exists. Since it makes more problems than it solves, I've removed it and added a mkdir -p /run/fwupd-efi to the pre-start script

Let me know if my analysis there is wrong @dasJ or if this is appropriate

[dasJ]

Yeah that works as well but I hate the approach of doing mkdir/chmod/chown in prestart. That's why I did it this way. We could also set PrivateTmp and just write to /tmp instead?

[lilyinstarlight]

I like /run better than /tmp (since it's supposed to be available for the entire lifetime of the daemon and isn't really temporary), but I'll try to think of options that don't use mkdir if you want

[lilyinstarlight]

Yeah the more I think about it, having mkdir is a bit unfortunate compared to having systemd manage the lifetime of the dir

That means the best option to keep /run though is probably to either manually set env var RUNTIME_DIRECTORY (which I'm not actually sure will even work with systemd) since fwupd can't otherwise handle that or creating another dummy unit that is WantedBy (to weakly start with), PartOf (to stop with), and Before fwupd or something with that RuntimeDirectory set and runs the signing itself

[lilyinstarlight]

Here's a diff for that latter option, for comparison:

diff --git a/nix/modules/lanzaboote.nix b/nix/modules/lanzaboote.nix
index 02f8f3f..f0e18b7 100644
--- a/nix/modules/lanzaboote.nix
+++ b/nix/modules/lanzaboote.nix
@@ -78,10 +78,23 @@ in
     systemd.services.fwupd = lib.mkIf config.services.fwupd.enable {
       # Tell fwupd to load its efi files from /run
       environment.FWUPD_EFIAPPDIR = "/run/fwupd-efi";
+    };
+
+    systemd.services.fwupd-efi = lib.mkIf config.services.fwupd.enable {
+      description = "Sign fwupd EFI app";
+      # Exist with the lifetime of the fwupd service
+      wantedBy = [ "fwupd.service" ];
+      partOf = [ "fwupd.service" ];
+      before = [ "fwupd.service" ];
+      # Create runtime directory for signed efi app
+      serviceConfig = {
+        Type = "oneshot";
+        RemainAfterExit = true;
+        RuntimeDirectory = "fwupd-efi";
+      };
       # Place the fwupd efi files in /run and sign them
-      preStart = ''
-        mkdir -p /run/fwupd-efi
-        cp ${config.services.fwupd.package.fwupd-efi}/libexec/fwupd/efi/fwupd*.efi /run/fwupd-efi/
+      script = ''
+        ln -s ${config.services.fwupd.package.fwupd-efi}/libexec/fwupd/efi/fwupd*.efi /run/fwupd-efi/
         ${pkgs.sbsigntool}/bin/sbsign --key '${cfg.privateKeyFile}' --cert '${cfg.publicKeyFile}' /run/fwupd-efi/fwupd*.efi
       '';
     };

[lilyinstarlight]

I've edited the above diff to actually work as intended. Thoughts on using it @dasJ?

Alternatively we could do the PrivateTmp hack (which I'm really not sure I like the idea of) or we can just use StateDirectory if we don't care if the signed file persists

Edit: Or we can leave the mkdir if we really need -- I'm not sure what would be the "cleanest" option here

[lilyinstarlight]

I've rebased on latest master to get the flake lockfile update in #138 and removed all other flake updates from this PR except for the nixpkgs-test input update (since it's pointing to a new branch now)

All flake checks still pass locally for me

[RaitoBezarius]

This sounds sane to me; untested yet with our flake check.

[RaitoBezarius]

Ran the tests, everything seems fine. We should probably add a test suite for this at some point, unsure of how exactly to test it though.