Repository navigation
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
Open
Rolv-Apneseth
wants to merge
1
commit into
coreos:main
Choose a base branch
from
Rolv-Apneseth:varlink
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ mod packagesystem; | |
| mod secureboot; | ||
| mod sha512string; | ||
| mod util; | ||
| mod varlink; | ||
|
|
||
| use clap::crate_name; | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| use anyhow::{anyhow, Context}; | ||
| use std::{ | ||
| os::{ | ||
| fd::IntoRawFd, | ||
| unix::io::{FromRawFd, OwnedFd}, | ||
| }, | ||
| path::PathBuf, | ||
| }; | ||
|
|
||
| #[cfg(efi_arch)] | ||
| mod sync_esps; | ||
|
|
||
| const SOCKET_PATH: &str = "/run/bootupd/org.coreos.bootupd1"; | ||
|
|
||
| #[derive(Debug, Clone, zlink::ReplyError, zlink::introspect::ReplyError)] | ||
| #[zlink(interface = "org.coreos.bootupd1")] | ||
| enum BootupdVarlinkError { | ||
| Failed { message: String }, | ||
| // Unfortunately we can't just remove endpoints based on architecture, | ||
| // so this is a generic error to be returned when an endpoint is not | ||
| // implemented for the target arch. | ||
| MethodNotAvailableOnArch { message: String }, | ||
| } | ||
|
|
||
| impl BootupdVarlinkError { | ||
| fn new(message: String) -> Self { | ||
| Self::Failed { message } | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| fn not_available_on_arch() -> Self { | ||
| Self::MethodNotAvailableOnArch { | ||
| message: format!("This method is not available on {}", std::env::consts::ARCH), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| 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. | ||
| /// | ||
| /// 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> { | ||
| #[cfg(efi_arch)] | ||
| return sync_esps::_sync_fwupd_updates(partuuid, capsule_dir); | ||
| #[cfg(not(efi_arch))] | ||
| return Err(BootupdVarlinkError::not_available_on_arch()); | ||
| } | ||
| } | ||
|
|
||
| /// 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") | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| 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}, | ||
| path::Path, | ||
| }; | ||
|
|
||
| use super::BootupdVarlinkError; | ||
| use crate::{ | ||
| bootupd::list_dev_current_root, efi, freezethaw::fsfreeze_thaw_cycle, model::SavedState, | ||
| }; | ||
|
|
||
| /// 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(()) | ||
| } | ||
|
|
||
| /// See [`super::BootupdVarlinkService::sync_fwupd_updates`] | ||
| pub(super) fn _sync_fwupd_updates( | ||
| 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); | ||
| 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); | ||
| 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(()) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../../data/libtest.sh |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?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.
This looks corrupted
Also, I think we should always offer varlink, just limit the available methods on it at build time.
Isn't that backwards? Also why wouldn't we just have
bootupctl updatebe implemented by internally forking the varlink and talking to it?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.
Yeah looks like I pasted over the diff somewhere, my bad.
I'm not sure I understand why that'd be backwards if we want the update to run first, then remain listening on the varlink socket. The varlink is currently only used by
fwupd.By internally forking the varlink and talking to it, do you mean we would be going to more of a daemon-client architecture with a full varlink interface? If so, how do we see automatic updates on boot functioning, if we want just a single unit? Or do you just mean we fork it off while updating so we don't have to call it directly in the unit?
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.
Hmm...I think the socket activation though is actually kind of important, we don't want to be a persistent daemon sitting there using RAM for no reason.
Sorry I know this is quite complex and nuanced. I may be missing something here, but I think what I'm saying is:
.socketunit that activates a.servicebootloader-update.servicejust becomes a small shim that writes to the socketWDYT?
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.
Ok, I see. So still adding a socket + service. However, we want
bootupctl updateto just use varlink interface.Is the thinking that every
bootupctlcall goes through the interface? It's fine by me, just probably separate from this PR as that's a big refactor.Is making sure that the varlink interface is available on every arch, with only the method itself being arch-dependent, the only change left for this PR if that's the case?
Thanks for the replies, it is indeed quite complex.
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.
Yes I'm OK to do it as a followup.
SGTM