-
Notifications
You must be signed in to change notification settings - Fork 57
varlink: add varlink interface for syncing fwupd capsule updates across ESPs #1138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,243 @@ | ||
| use anyhow::{anyhow, Context}; | ||
| use cap_std::{ambient_authority, fs::Dir}; | ||
| use cap_std_ext::dirext::CapStdExtDirExt; | ||
| use log::info; | ||
| use std::{ | ||
| fs::create_dir_all, | ||
| io::{copy, Write}, | ||
| os::{ | ||
| fd::IntoRawFd, | ||
| unix::io::{FromRawFd, OwnedFd}, | ||
| }, | ||
| path::{Path, PathBuf}, | ||
| }; | ||
|
|
||
| use crate::{ | ||
| bootupd::list_dev_current_root, efi, freezethaw::fsfreeze_thaw_cycle, model::SavedState, | ||
| }; | ||
|
|
||
| const SOCKET_PATH: &str = "/run/bootupd/org.coreos.bootupd1"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above re the socket |
||
|
|
||
| /// Find the ESP device matching a given partition UUID | ||
| fn find_esp_by_partuuid<'a>( | ||
| devices: &'a [bootc_internal_blockdev::Device], | ||
| partuuid: &str, | ||
| ) -> Option<&'a bootc_internal_blockdev::Device> { | ||
| devices.iter().find(|d| { | ||
| d.partuuid | ||
| .as_deref() | ||
| .is_some_and(|u| u.eq_ignore_ascii_case(partuuid)) | ||
| }) | ||
| } | ||
|
|
||
| /// Validate that a given path is a valid capsule directory | ||
| fn validate_capsule_dir(capsule_dir: &Path) -> Result<(), BootupdVarlinkError> { | ||
| // Must be relative | ||
| if capsule_dir.is_absolute() { | ||
| return Err(BootupdVarlinkError::new( | ||
| "capsule_dir must be relative to the ESP".into(), | ||
| )); | ||
| } | ||
|
|
||
| // Must not contain path traversal components | ||
| if capsule_dir | ||
| .components() | ||
| .any(|c| c == std::path::Component::ParentDir) | ||
| { | ||
| return Err(BootupdVarlinkError::new( | ||
| "capsule_dir must not contain '..' components - path traversal not allowed".into(), | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, zlink::ReplyError, zlink::introspect::ReplyError)] | ||
| #[zlink(interface = "org.coreos.bootupd1")] | ||
| enum BootupdVarlinkError { | ||
| Failed { message: String }, | ||
| } | ||
|
|
||
| impl BootupdVarlinkError { | ||
| fn new(message: String) -> Self { | ||
| Self::Failed { message } | ||
| } | ||
| } | ||
|
|
||
| impl From<anyhow::Error> for BootupdVarlinkError { | ||
| fn from(err: anyhow::Error) -> Self { | ||
| log::error!("varlink call failed: {err:#}"); | ||
| Self::Failed { | ||
| message: format!("{err:#}"), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| struct BootupdVarlinkService; | ||
|
|
||
| #[zlink::service(interface = "org.coreos.bootupd1")] | ||
| impl BootupdVarlinkService { | ||
| /// Sync capsule update files from a "primary" ESP to all colocated ESPs. | ||
|
Rolv-Apneseth marked this conversation as resolved.
|
||
| /// | ||
| /// partuuid: GPT partition UUID of the ESP containing the source capsule files. | ||
| /// capsule_dir: Path to the directory containing the source capsule files, relative | ||
| /// to the ESP root (e.g. "EFI/fedora/fw") | ||
| #[allow(clippy::unused_async)] | ||
| async fn sync_fwupd_updates( | ||
| &mut self, | ||
| partuuid: &str, | ||
| capsule_dir: &str, | ||
| ) -> Result<(), BootupdVarlinkError> { | ||
| if partuuid.is_empty() { | ||
| return Err(BootupdVarlinkError::new( | ||
| "partuuid must not be empty".into(), | ||
| )); | ||
| } | ||
| if capsule_dir.is_empty() { | ||
| return Err(BootupdVarlinkError::new( | ||
| "capsule_dir must not be empty".into(), | ||
| )); | ||
| } | ||
|
|
||
| let capsule_dir = Path::new(capsule_dir); | ||
| validate_capsule_dir(capsule_dir)?; | ||
|
|
||
| let root_device = list_dev_current_root()?; | ||
| let esp_devices = root_device | ||
| .find_colocated_esps()? | ||
| .ok_or_else(|| anyhow!("could not find any co-located ESPs"))?; | ||
|
|
||
| // Find the source ESP (the one fwupd wrote to). | ||
| let primary_device = find_esp_by_partuuid(&esp_devices, partuuid).ok_or_else(|| { | ||
| BootupdVarlinkError::new(format!("No ESP found with partuuid {partuuid}")) | ||
| })?; | ||
|
|
||
| // Avoid running at the same time as bootloader updates | ||
| let sysroot = Dir::open_ambient_dir("/", ambient_authority()).context("opening sysroot")?; | ||
| let _lock = SavedState::acquire_write_lock("/".into(), sysroot)?; | ||
|
|
||
| // Mount primary ESP and find capsule updates dir | ||
| let primary_efi = efi::mount_esp(&primary_device.path()).with_context(|| { | ||
| format!( | ||
| "Creating temp ESP mount for primary ESP (partuuid: {partuuid}) at {:?}", | ||
| primary_device.path(), | ||
| ) | ||
| })?; | ||
| let primary_mount = primary_efi.dir.path().to_path_buf(); | ||
|
|
||
| let src_capsule_path = primary_mount.join(capsule_dir); | ||
|
Rolv-Apneseth marked this conversation as resolved.
|
||
| let src_dir = Dir::open_ambient_dir(&src_capsule_path, ambient_authority()) | ||
| .context("opening source capsule dir")?; | ||
|
|
||
| // Sync to every other co-located ESP. | ||
| let mut synced_count = 0; | ||
| for esp in esp_devices.iter() { | ||
| if esp | ||
| .partuuid | ||
| .as_ref() | ||
| .is_some_and(|u| u.eq_ignore_ascii_case(partuuid)) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| let secondary_efi = efi::mount_esp(&esp.path()).with_context(|| { | ||
| format!( | ||
| "Creating temp ESP mount for secondary ESP (partuuid: {}) at {:?}", | ||
| esp.partuuid.as_deref().unwrap_or("unknown"), | ||
| esp.path(), | ||
| ) | ||
| })?; | ||
| let dest_mount = secondary_efi.dir.path().to_path_buf(); | ||
|
|
||
| let dest_capsule_path = dest_mount.join(capsule_dir); | ||
| create_dir_all(&dest_capsule_path) | ||
| .with_context(|| format!("creating {dest_capsule_path:?}"))?; | ||
|
|
||
| let dest_dir = Dir::open_ambient_dir(&dest_capsule_path, ambient_authority()) | ||
| .context("opening destination capsule dir")?; | ||
|
|
||
| for entry in src_dir.entries().context("reading source capsule dir")? { | ||
| let entry = entry.context("reading dir entry")?; | ||
| let name = entry.file_name(); | ||
| dest_dir | ||
| .atomic_replace_with(&name, |dest_file| -> std::io::Result<()> { | ||
| let mut src_file = src_dir.open(&name)?; | ||
| copy(&mut src_file, dest_file)?; | ||
| dest_file.flush()?; | ||
| dest_file.get_ref().as_file().sync_data() | ||
| }) | ||
| .with_context(|| { | ||
| format!( | ||
| "syncing {name:?} from ESP {partuuid} to ESP {}", | ||
| esp.partuuid.as_deref().unwrap_or("unknown") | ||
| ) | ||
| })?; | ||
| } | ||
|
|
||
| fsfreeze_thaw_cycle( | ||
| dest_dir | ||
| .reopen_as_ownedfd() | ||
| .context("reopening dest dir as owned fd")?, | ||
| )?; | ||
| drop(dest_dir); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are already dropped on scope exit. Any reason why we need to be explicit?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason in particular, just making it clear we want it unmounted. Would you prefer without the (now 2) |
||
| drop(secondary_efi); | ||
|
|
||
| synced_count += 1; | ||
| } | ||
| info!("successfully synced {capsule_dir:?} from ESP {partuuid} to {synced_count} colocated ESP(s)"); | ||
|
|
||
| drop(src_dir); | ||
| drop(primary_efi); | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// Ensure the Unix socket can be created | ||
| fn get_socket() -> anyhow::Result<PathBuf> { | ||
| let socket_path = PathBuf::from(SOCKET_PATH); | ||
|
|
||
| // Ensure the parent directory exists. | ||
| if let Some(parent) = socket_path.parent() { | ||
| std::fs::create_dir_all(parent) | ||
| .with_context(|| format!("creating directory {}", parent.display()))?; | ||
| } | ||
|
|
||
| // Remove any stale socket from a previous run. | ||
| if let Err(e) = std::fs::remove_file(&socket_path) { | ||
| if e.kind() != std::io::ErrorKind::NotFound { | ||
| return Err(e) | ||
| .with_context(|| format!("removing stale socket {}", socket_path.display())); | ||
| } | ||
| } | ||
|
|
||
| Ok(socket_path) | ||
| } | ||
|
|
||
| pub fn run_varlink_service() -> anyhow::Result<()> { | ||
| tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()? | ||
| .block_on(async { | ||
| let listener = if std::env::var_os("LISTEN_FDS").is_some() { | ||
| // Socket-activated | ||
| let fd = libsystemd::activation::receive_descriptors(false) | ||
| .context("receiving socket-activated fds")? | ||
| .into_iter() | ||
| .next() | ||
| .ok_or_else(|| anyhow!("no fds received"))?; | ||
| // SAFETY: `into_raw_fd` transfers ownership from `FileDescriptor`, ensuring the | ||
| // fd is valid and not closed elsewhere. `from_raw_fd` takes exclusive ownership. | ||
| let owned_fd = unsafe { OwnedFd::from_raw_fd(fd.into_raw_fd()) }; | ||
| zlink::tokio::unix::Listener::try_from(owned_fd) | ||
| .context("creating listener from socket-activated fd")? | ||
| } else { | ||
| // Bind our own socket | ||
| let socket_path = get_socket()?; | ||
| zlink::tokio::unix::bind(socket_path)? | ||
| }; | ||
|
|
||
| let server = zlink::Server::new(listener, BootupdVarlinkService); | ||
| server.run().await.context("running varlink service") | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| [Unit] | ||
| Description=Bootupd Varlink Service | ||
| Documentation=https://github.com/coreos/bootupd | ||
| # Interface only implemented on EFI systems | ||
| ConditionPathExists=/sys/firmware/efi | ||
|
|
||
| [Service] | ||
| Type=simple | ||
| # It doesn't make sense to sync ESP updates in "Live" environments. | ||
| # https://github.com/coreos/fedora-coreos-tracker/issues/2136 | ||
| ExecCondition=/bin/bash -c '[[ ! $(findmnt -n -o FSTYPE /sysroot) =~ ^(erofs|squashfs)$ ]]' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above, if we changed bootloader-update.service then we could avoid copy-pasta this |
||
| ExecStart=/usr/libexec/bootupd varlink | ||
| # Keep this stuff in sync with SYSTEMD_ARGS_BOOTUPD in general | ||
| PrivateNetwork=yes | ||
| ProtectHome=yes | ||
| KillMode=mixed | ||
| MountFlags=slave | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| [Unit] | ||
| Description=Bootupd Varlink Socket | ||
| Documentation=https://github.com/coreos/bootupd | ||
| # Interface only implemented on EFI systems | ||
| ConditionPathExists=/sys/firmware/efi | ||
|
|
||
| [Socket] | ||
| ListenStream=/run/bootupd/org.coreos.bootupd1 | ||
| SocketMode=0600 | ||
|
|
||
| [Install] | ||
| WantedBy=sockets.target |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../../data/libtest.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be nice to have systemd units enforce locking.
So we have a single
bootupd.serviceand change bootloader-update.service to actually call into the varlink API too, which would activate that service in the same way.Alternatively, do we actually need a
.socketunit? What we did in e.g. varlink for https://github.com/bootc-dev/bcvk/ is that it remains a CLI that has an interface one forks that enables varlink.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current state should work without a socket unit actually, so just
bootupd varlinkwill run it's own daemon. I don't quite understand the suggestion for a singlebootupd.servicethough, would you mind clarifying? If we're gonna have a service anyway why not letsystemdhandle the socket? I don't have a strong opinion on this and initially leaned towards a simple service at first but the PR over onfwupdassumed it was socket-activated so I shifted that direction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look by the way @cgwalters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well if fwupd already merged support for calling into the socket, then clearly we should probably do that.
However! There's new support for "varlink registry" stuff in systemd, see systemd/systemd#40590 among others. We should probably participate in that if we do have a bound socket.
But what I think is actually a good bit more valuable is having a single systemd .service unit - if we only have one, then systemd enforces locking for it (while I wouldn't argue to drop our lockfile, having systemd do it is just better).
It means that one can more consistently check logs in one place. It means isolation and configuration only appear in one place, not two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just documentation, the only assumption in the code is that there's a varlink interface that's callable. I was just explaining why I went that direction.
Sure, don't see why not.
Just trying to understand, so I have some questions:
We wouldn't be renaming the current unit, right?
Do you imagine it still running the update on boot, and then starting the varlink listener? Would we keep the transient update service for
bootupctl, or would every command have to go through avarlinkinterface?Are we fine with requiring people who mask the service currently to manually start
bootupd varlinkwhen they need the varlink functionality?