From 63c85c509352861c5e1757a445bab54d62c2402d Mon Sep 17 00:00:00 2001 From: Harnoor Lal Date: Tue, 14 Apr 2026 21:20:16 -0700 Subject: [PATCH] fix(check): distinguish permission-denied from boot chain failure Running `atomic-rollback check` as non-root on Fedora hits permission-denied reading /boot/efi/EFI/fedora/grub.cfg (root-readable only on Fedora). Before this change, the tool reported "Boot chain has problems" and exited 1, conflating "we could not verify" with "we found problems." Scripts relying on exit codes could not tell apart the two. Two semantically distinct facts now have distinct representations: - grub.rs: new GrubContextError enum with PermissionDenied and Other variants. The io::ErrorKind from fs::read_to_string is preserved at the source instead of collapsing into a String. - check.rs: new BootStatus::Inaccessible { reason, hint } variant. verify_bootable and verify_snapshot_bootable map PermissionDenied to Inaccessible, Other to Fail. - main.rs Check: new match arm prints "Cannot verify boot chain: " with hint, exits 3. - kernel_hook.rs: treats Inaccessible as a warning (log, do not abort kernel-install). - rollback.rs: treats Inaccessible as Fail (abort rather than proceed with an unverified snapshot). - check.rs gate: treats Inaccessible as Fail (migration runs as root; reaching this branch is unexpected and should fail safe). Exit codes: - 0: Pass - 1: Fail (real boot-chain problem) - 2: Warn (valid with warnings) - 3: Inaccessible (could not verify; try sudo) Verified on Fedora 43 VM: as root exits 0, as non-root exits 3 with "permission denied reading /boot/efi/EFI/fedora/grub.cfg" and a "Run: sudo atomic-rollback check" hint. The "Boot chain has problems" message no longer fires for non-root. Closes #14. --- CHANGELOG.md | 4 +++ crates/atomic-rollback/src/check.rs | 34 +++++++++++++++++++---- crates/atomic-rollback/src/grub.rs | 30 ++++++++++++++++---- crates/atomic-rollback/src/kernel_hook.rs | 6 ++++ crates/atomic-rollback/src/main.rs | 5 ++++ crates/atomic-rollback/src/rollback.rs | 10 +++++++ crates/changelog-core/src/lib.rs | 9 ++++++ 7 files changed, 87 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5351ea..9ac89c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to atomic-rollback are documented here. ## [Unreleased] +### Fixed + +- `check` now distinguishes "cannot verify" from "boot chain has problems." Running as non-root on Fedora (where the ESP grub.cfg is root-readable only) previously reported a boot-chain failure with exit 1, misleading users about the actual system state. The tool now reports "cannot verify boot chain" with a prompt to run as root, and exits with code 3 to let scripts distinguish the case. Closes #14. + ## [0.4.0] - 2026-04-14 ### Added diff --git a/crates/atomic-rollback/src/check.rs b/crates/atomic-rollback/src/check.rs index b175711..9d01793 100644 --- a/crates/atomic-rollback/src/check.rs +++ b/crates/atomic-rollback/src/check.rs @@ -6,7 +6,7 @@ use std::fs; use std::path::Path; -use crate::grub::GrubContext; +use crate::grub::{GrubContext, GrubContextError}; use crate::parse; use crate::platform::FEDORA as P; use crate::tools; @@ -18,6 +18,11 @@ pub enum BootStatus { Pass, Warn, Fail(Vec), + /// The check could not verify boot chain state (e.g., non-root + /// invocation hits permission-denied on the ESP grub.cfg). + /// Distinct from `Fail` because the boot chain may be fine; we + /// simply did not have the access needed to evaluate it. + Inaccessible { reason: String, hint: String }, } fn evaluate_checks(checks: Vec<(&str, Vec)>) -> BootStatus { @@ -51,8 +56,12 @@ fn evaluate_checks(checks: Vec<(&str, Vec)>) -> BootStatus { pub fn verify_bootable(root: &Path) -> BootStatus { let grub = match GrubContext::from_system(root) { Ok(g) => g, - Err(e) => return BootStatus::Fail(vec![format!( - "Cannot determine how GRUB boots this system: {e}. \ + Err(GrubContextError::PermissionDenied { path }) => return BootStatus::Inaccessible { + reason: format!("permission denied reading {}", path.display()), + hint: "The ESP grub.cfg is root-readable only on Fedora. Run: sudo atomic-rollback check".into(), + }, + Err(GrubContextError::Other(msg)) => return BootStatus::Fail(vec![format!( + "Cannot determine how GRUB boots this system: {msg}. \ Ensure {}/grub.cfg exists and the root filesystem is mounted.", P.esp_dir )]), }; @@ -71,8 +80,12 @@ pub fn verify_bootable(root: &Path) -> BootStatus { pub fn verify_snapshot_bootable(root: &Path) -> BootStatus { let grub = match GrubContext::for_snapshot(root) { Ok(g) => g, - Err(e) => return BootStatus::Fail(vec![format!( - "Cannot determine boot configuration: {e}. \ + Err(GrubContextError::PermissionDenied { path }) => return BootStatus::Inaccessible { + reason: format!("permission denied reading {}", path.display()), + hint: "The ESP grub.cfg is root-readable only on Fedora. Run as root.".into(), + }, + Err(GrubContextError::Other(msg)) => return BootStatus::Fail(vec![format!( + "Cannot determine boot configuration: {msg}. \ Ensure {}/grub.cfg exists.", P.esp_dir )]), }; @@ -116,6 +129,17 @@ pub fn gate(step: &str, root: &Path, swap_artifact: Option<&str>) { } std::process::exit(1); } + BootStatus::Inaccessible { reason, hint } => { + // Migration runs as root; reaching this branch means something + // unexpected prevented verification. Fail safe (don't proceed). + println!("\n GATE {step}: FAIL"); + if let Some(old) = swap_artifact { + eprintln!(" The swap completed. Old file preserved at {old}."); + } + eprintln!(" Cannot verify boot chain: {reason}"); + eprintln!(" {hint}"); + std::process::exit(1); + } } } diff --git a/crates/atomic-rollback/src/grub.rs b/crates/atomic-rollback/src/grub.rs index 87ec632..d7f2c4e 100644 --- a/crates/atomic-rollback/src/grub.rs +++ b/crates/atomic-rollback/src/grub.rs @@ -4,11 +4,24 @@ //! btrfs_relative_path is set. use std::fs; +use std::io; use std::path::{Path, PathBuf}; use crate::platform::FEDORA as P; use crate::tools; +/// Errors from building a GrubContext. Distinguishes permission-denied +/// on the ESP grub.cfg read (non-root invocation) from other errors, +/// so the caller can surface "try sudo" vs a real boot-chain problem. +pub enum GrubContextError { + PermissionDenied { path: PathBuf }, + Other(String), +} + +impl From for GrubContextError { + fn from(s: String) -> Self { GrubContextError::Other(s) } +} + /// Captures how GRUB resolves paths on the target filesystem. /// Used by check.rs to verify that GRUB can find kernels and configs. pub struct GrubContext { @@ -20,14 +33,20 @@ pub struct GrubContext { _mount: Option, } +fn read_esp_cfg(path: &Path) -> Result { + fs::read_to_string(path).map_err(|e| match e.kind() { + io::ErrorKind::PermissionDenied => GrubContextError::PermissionDenied { path: path.to_path_buf() }, + _ => GrubContextError::Other(format!("cannot read ESP grub.cfg: {e}")), + }) +} + impl GrubContext { /// Builds a context from the live system's ESP grub.cfg. /// Reads the target UUID, determines filesystem type, and mounts /// the target if not already mounted. - pub fn from_system(root: &Path) -> Result { + pub fn from_system(root: &Path) -> Result { let esp_cfg = root.join(&P.esp_dir[1..]).join("grub.cfg"); - let content = fs::read_to_string(&esp_cfg) - .map_err(|e| format!("cannot read ESP grub.cfg: {e}"))?; + let content = read_esp_cfg(&esp_cfg)?; let stub = tools::parse_esp_stub(&content)?; let target_fstype = tools::blkid_fstype(&stub.boot_uuid) @@ -42,10 +61,9 @@ impl GrubContext { /// Builds a context for verifying a snapshot before rollback. /// Reads ESP config from the live system (ESP is vfat, not in the snapshot). /// Resolves GRUB paths against the snapshot mount, not the live root. - pub fn for_snapshot(snapshot_root: &Path) -> Result { + pub fn for_snapshot(snapshot_root: &Path) -> Result { let esp_cfg = Path::new(P.esp_dir).join("grub.cfg"); - let content = fs::read_to_string(esp_cfg) - .map_err(|e| format!("cannot read ESP grub.cfg: {e}"))?; + let content = read_esp_cfg(&esp_cfg)?; let stub = tools::parse_esp_stub(&content)?; let target_fstype = tools::blkid_fstype(&stub.boot_uuid) diff --git a/crates/atomic-rollback/src/kernel_hook.rs b/crates/atomic-rollback/src/kernel_hook.rs index b8b2aa3..2bb91f3 100644 --- a/crates/atomic-rollback/src/kernel_hook.rs +++ b/crates/atomic-rollback/src/kernel_hook.rs @@ -98,6 +98,12 @@ fn handle_add(kver: &str) -> Result<(), String> { // Don't return Err; that would abort kernel-install and leave worse state. Ok(()) } + check::BootStatus::Inaccessible { reason, hint } => { + // Kernel-install runs as root; reaching this branch is unusual. + // Log and continue; do not abort kernel-install. + eprintln!("atomic-rollback: WARNING after kernel {kver} install: cannot verify boot chain ({reason}). {hint}"); + Ok(()) + } } } diff --git a/crates/atomic-rollback/src/main.rs b/crates/atomic-rollback/src/main.rs index 00a14d9..76b1ced 100644 --- a/crates/atomic-rollback/src/main.rs +++ b/crates/atomic-rollback/src/main.rs @@ -173,6 +173,11 @@ fn main() { println!("\nBoot chain has problems."); std::process::exit(1); } + check::BootStatus::Inaccessible { reason, hint } => { + eprintln!("Cannot verify boot chain: {reason}"); + eprintln!("{hint}"); + std::process::exit(3); + } } } Command::Setup => { diff --git a/crates/atomic-rollback/src/rollback.rs b/crates/atomic-rollback/src/rollback.rs index 4f286c7..3ed3106 100644 --- a/crates/atomic-rollback/src/rollback.rs +++ b/crates/atomic-rollback/src/rollback.rs @@ -39,6 +39,16 @@ pub fn rollback(snapshot_name: &str) -> Result<(), String> { let _ = std::fs::remove_dir(toplevel); return Err("rollback aborted: boot chain invalid for snapshot".into()); } + check::BootStatus::Inaccessible { reason, hint } => { + // Cannot verify snapshot bootability; fail safe rather than + // proceed with an unchecked RENAME_EXCHANGE. + println!("\n Snapshot verification FAILED:"); + eprintln!(" cannot verify boot chain: {reason}"); + eprintln!(" {hint}"); + tools::umount(toplevel)?; + let _ = std::fs::remove_dir(toplevel); + return Err("rollback aborted: boot chain could not be verified for snapshot".into()); + } } check::print_rollback_scope(&fstab); diff --git a/crates/changelog-core/src/lib.rs b/crates/changelog-core/src/lib.rs index d825dcc..936331c 100644 --- a/crates/changelog-core/src/lib.rs +++ b/crates/changelog-core/src/lib.rs @@ -207,6 +207,9 @@ fragments! { // v0.4.0 (7 Added, 2 Changed, 1 Removed, 1 Fixed) AutomaticSnapshotsRollingTimestampNames, + + // Unreleased + CheckDistinguishesPermissionDeniedFromBootFailure, SnapshotRetention, SnapshotListThreeColumnTable, RollbackAndDeleteAcceptBtrfsIds, @@ -479,6 +482,12 @@ impl Fragment { version: VersionId::V0_4_0, section: Section::Fixed, text: "Systems upgrading from any 0.3.x release have their legacy `root.pre-update` snapshot renamed to its creation timestamp (in the same `%Y-%m-%d_%H-%M-%S` format as new automatic snapshots) on upgrade. The renamed snapshot joins the rolling history and becomes eligible for retention; before this release it did not match the auto-name format and was treated as a user-named snapshot, persisting indefinitely on upgraded systems. The btrfs subvolume ID is preserved across the rename, so rollback targets that referenced the numeric ID are unaffected.", }, + + // Unreleased + Self::CheckDistinguishesPermissionDeniedFromBootFailure => Status::Unreleased { + section: Section::Fixed, + text: "`check` now distinguishes \"cannot verify\" from \"boot chain has problems.\" Running as non-root on Fedora (where the ESP grub.cfg is root-readable only) previously reported a boot-chain failure with exit 1, misleading users about the actual system state. The tool now reports \"cannot verify boot chain\" with a prompt to run as root, and exits with code 3 to let scripts distinguish the case. Closes #14.", + }, } } }