Fix systemd-boot signing on sealed image test #1809
Conversation
gerblesh
force-pushed
the
sign-sdboot
branch
from
64da24a to
a20e12c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the systemd-boot signing issue for sealed images, which is a great step forward for secure boot testing. The introduction of a dedicated Dockerfile.sdboot for the signing process is a clean approach. My review includes a few suggestions to enhance the new Dockerfile's robustness and readability, and a note on a temporarily disabled lint check.
gerblesh
force-pushed
the
sign-sdboot
branch
2 times, most recently
from
6bd32cd to
4ffe2f6
Compare
|
added automatic key enrollment (with sdboot) in |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thanks so much for starting this! Basically let's get the buildsystem rework in to properly sign systemd-boot first, and then look at the key autoenrollment as a distinct second step.
I can take a look at the first one, starting from the work here if you prefer!
|
alright split out the code for autoenroll to just focus on signing systemd-boot in CI |
|
#1818 new PR for auto enrollment here |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thanks again for starting this! Is it OK if I try some force pushing to this PR and we co-author?
Yeah go for it, just was cleaning up the comments and extra mounts |
cgwalters
force-pushed
the
sign-sdboot
branch
3 times, most recently
from
c7859a7 to
9be3705
Compare
cgwalters
force-pushed
the
sign-sdboot
branch
3 times, most recently
from
51730ea to
f64f6d7
Compare
|
OK what this is blocking on is I think we're not detecting the OVMF firmware correctly in bcvk on Debian derivatives. Looking at that |
cgwalters
force-pushed
the
sign-sdboot
branch
4 times, most recently
from
1505773 to
65e28b7
Compare
This would really be fixed by having `boot container ukify`. Signed-off-by: Colin Walters <walters@verbum.org>
Signed-off-by: Gareth Widlansky <gareth.widlansky@proton.me> Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
sign-sdboot
branch
2 times, most recently
from
34bdaf6 to
92fa74b
Compare
crates/xtask/src/tmt.rs
Outdated
| println!(" Image: {}", image); | ||
| println!(" VM name: {}\n", vm_name); | ||
|
|
||
| // Detect if this is a sealed image and build firmware args accordingly |
crates/xtask/src/tmt.rs
Outdated
| format!("--secure-boot-keys={}", sb_keys_dir), | ||
| ] | ||
| } else { | ||
| if is_sealed { |
There was a problem hiding this comment.
This fallback doesn't make sense
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Sorry, those comments were a misfire from my non-sandboxed agent; I'm going to ensure that doesn't happen again. |
Main goal is to reduce signing logic duplication between the systemd-boot and UKI generation. However, this quickly snowballed into wanting to actually verify by providing a custom secure boot keys to bcvk that things worked. This depends on bootc-dev/bcvk#170 Now as part of that, I ran into what I think are bugs in pesign; this cuts things back over to using sbsign. I'll file a tracker for that separately. Finally as part of this, just remove the TMT example that builds a sealed image but doesn't actually verify it works - it's already drifted from what we do outside here. Ultimately what we need is to shift some of this into the Fedora examples and we just fetch it here anyways. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
sign-sdboot
branch
from
92fa74b to
17feb27
Compare
|
This one is ready to merge I think but as always a lot of potential followup here. Hum looks like the rawhide failures might be a nushell regression/semantic change, but not related. |
|
@jmarrero you're assigned reviewer looks like |
|
(Technically I can merge this since it's someone else's PR but I added quite a lot of code here so someone else should at least do a medium-level review) |
| - /tmt/tests/tests/test-25-download-only-upgrade | ||
|
|
||
| /plan-26-examples-build: | ||
| summary: Test bootc examples build scripts |
There was a problem hiding this comment.
cc @jeckersb sorry I should have called out explicitly here for you that I was dropping this.
The core problem is we had two sealing impls going on, one out of the main Dockerfile and one in here, and it was hard to juggle ensuring I'd applied the correct fixes to both of them.
There was also some incorrect cruft here (copying in initramfs services etc.)
I think per discussion let's aim to do #1498 (comment)
Crucially if we do that after #1858 merges it becomes a lot more clear that the sealing aspect of the build is separate from building bootc, so it should then be easier to ensure that the sealing logic can also be applied generically outside of building the code here, and then we migrate it to say https://gitlab.com/fedora/bootc/examples/ ?
This relates to https://gitlab.com/fedora/bootc/docs/-/merge_requests/143
3 participants
Fixes systemd-boot signing, before the systemd-boot binary was signed on the buildroot but not on the target image, resulting in an unbootable image with secure boot enabled and the proper keys enrolled. This PR fixes it by first signing the systemd-boot on image (assumes it is installed), copying it over to the final image, then computing the digest, and then finally signing and creating the UKI with a different multi stage build. Definitely a little jank and #1498 does look like a better solution in the long term, however this at least gets the image in a bootable state on secure boot and allows for testing the secure boot in VMs. Would be happy to take a stab at proper image building UX but I'm not sure if that already has work done or if y'all have a particular vision in mind for the build system