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.", + }, } } }