diff --git a/.forgejo/workflows/release-bakery.yml b/.forgejo/workflows/release-bakery.yml index a2373cb..2ac03bf 100644 --- a/.forgejo/workflows/release-bakery.yml +++ b/.forgejo/workflows/release-bakery.yml @@ -59,7 +59,13 @@ jobs: - name: regenerate index.json env: MINISIGN_SEC_KEY: ${{ secrets.BAKERY_MINISIGN_SEC_KEY_PATH }} - run: cd src && bash scripts/gen-index.sh + run: | + set -euo pipefail + if [ -z "${MINISIGN_SEC_KEY:-}" ]; then + echo "::error::BAKERY_MINISIGN_SEC_KEY_PATH secret not set — refusing to regenerate stable index.json unsigned (would leave a stale signature mismatched against fresh content and break bakery for everyone on the stable track)" + exit 1 + fi + cd src && bash scripts/gen-index.sh - name: upload to GitHub Release env: diff --git a/.forgejo/workflows/release-bread-theme.yml b/.forgejo/workflows/release-bread-theme.yml index a8eb8df..2892e14 100644 --- a/.forgejo/workflows/release-bread-theme.yml +++ b/.forgejo/workflows/release-bread-theme.yml @@ -57,7 +57,13 @@ jobs: - name: regenerate index.json env: MINISIGN_SEC_KEY: ${{ secrets.BAKERY_MINISIGN_SEC_KEY_PATH }} - run: cd src && bash scripts/gen-index.sh + run: | + set -euo pipefail + if [ -z "${MINISIGN_SEC_KEY:-}" ]; then + echo "::error::BAKERY_MINISIGN_SEC_KEY_PATH secret not set — refusing to regenerate stable index.json unsigned (would leave a stale signature mismatched against fresh content and break bakery for everyone on the stable track)" + exit 1 + fi + cd src && bash scripts/gen-index.sh - name: upload to GitHub Release env: diff --git a/Cargo.lock b/Cargo.lock index e99d7bd..b3391cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -107,9 +107,11 @@ name = "bakery" version = "0.3.1" dependencies = [ "anyhow", + "bread-utils", "chrono", "clap", "dirs", + "fs4", "hex", "minisign-verify", "semver", @@ -117,7 +119,6 @@ dependencies = [ "serde_json", "sha2", "tempfile", - "toml 0.8.23", "ureq", ] @@ -655,6 +656,16 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fs4" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f7e180ac76c23b45e767bd7ae9579bc0bb458618c4bc71835926e098e61d15f8" +dependencies = [ + "rustix 0.38.44", + "windows-sys 0.52.0", +] + [[package]] name = "futures-channel" version = "0.3.33" @@ -1325,6 +1336,12 @@ dependencies = [ "libc", ] +[[package]] +name = "linux-raw-sys" +version = "0.4.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d26c52dbd32dccf2d10cac7725f8eae5296885fb5703b261f7d0a0739ec807ab" + [[package]] name = "linux-raw-sys" version = "0.12.1" @@ -1815,6 +1832,19 @@ dependencies = [ "semver", ] +[[package]] +name = "rustix" +version = "0.38.44" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fdb5bc1ae2baa591800df16c9ca78619bf65c0488b41b96ccec5d11220d8c154" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys 0.4.15", + "windows-sys 0.52.0", +] + [[package]] name = "rustix" version = "1.1.4" @@ -1824,7 +1854,7 @@ dependencies = [ "bitflags", "errno", "libc", - "linux-raw-sys", + "linux-raw-sys 0.12.1", "windows-sys 0.61.2", ] @@ -2063,7 +2093,7 @@ dependencies = [ "fastrand", "getrandom 0.4.3", "once_cell", - "rustix", + "rustix 1.1.4", "windows-sys 0.61.2", ] diff --git a/bakery/Cargo.toml b/bakery/Cargo.toml index f65aedd..377a6fe 100644 --- a/bakery/Cargo.toml +++ b/bakery/Cargo.toml @@ -11,7 +11,6 @@ repository = "https://git.breadway.dev/Breadway/bread-ecosystem" anyhow = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } -toml = { workspace = true } dirs = { workspace = true } ureq = { workspace = true } sha2 = { workspace = true } @@ -20,6 +19,6 @@ clap = { workspace = true } chrono = { workspace = true } minisign-verify = { workspace = true } semver = { workspace = true } - -[dev-dependencies] +bread-utils = { path = "../bread-utils" } +fs4 = { version = "0.8", features = ["sync"] } tempfile = "3" diff --git a/bakery/src/download.rs b/bakery/src/download.rs index 3362bd0..0e53ef3 100644 --- a/bakery/src/download.rs +++ b/bakery/src/download.rs @@ -4,8 +4,10 @@ use std::path::Path; use crate::manifest::{fetch_binary, Binary}; -/// Download a binary to a temp path, verify its SHA-256, then atomically move -/// it into place. Bails before touching `dest` if the checksum fails. +/// Download a binary, verify its SHA-256, then atomically write it into +/// place (fsynced, temp-in-same-dir-with-unique-name then rename — see +/// `bread_utils::atomic`). Bails before touching `dest` if the checksum +/// fails. pub fn fetch_and_place(binary: &Binary, dest: &Path) -> Result<()> { println!(" downloading {}…", binary.name); let bytes = fetch_binary(&binary.dl_url, &binary.github_url) @@ -14,20 +16,8 @@ pub fn fetch_and_place(binary: &Binary, dest: &Path) -> Result<()> { verify_sha256(&bytes, &binary.sha256) .with_context(|| format!("checksum mismatch for {}", binary.name))?; - if let Some(dir) = dest.parent() { - std::fs::create_dir_all(dir)?; - } - - let tmp = dest.with_extension("tmp"); - std::fs::write(&tmp, &bytes).context("writing binary to tmp")?; - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o755))?; - } - - std::fs::rename(&tmp, dest).context("placing binary")?; + bread_utils::atomic::write_atomic_bytes(dest, &bytes, Some(0o755)) + .with_context(|| format!("placing binary at {}", dest.display()))?; println!(" installed {}", dest.display()); Ok(()) } @@ -38,6 +28,9 @@ pub fn fetch_and_place(binary: &Binary, dest: &Path) -> Result<()> { /// [`fetch_and_place`]), and config-example / systemd-unit downloads in /// `install.rs` — so all downloaded artifacts get the same integrity check. pub fn verify_sha256(bytes: &[u8], expected_hex: &str) -> Result<()> { + if expected_hex.is_empty() { + bail!("index entry has no sha256 recorded — refusing to trust an unverifiable download"); + } let mut hasher = Sha256::new(); hasher.update(bytes); let actual = hex::encode(hasher.finalize()); @@ -80,4 +73,14 @@ mod tests { let hash = sha256_hex(bytes); assert!(verify_sha256(bytes, &hash).is_ok()); } + + #[test] + fn verify_missing_sha256_gives_a_clear_error() { + // gen-index.sh emits an empty sha256 string when a .sha256 sidecar + // is missing — must not fall through to the generic mismatch + // message ("expected: \n actual: "), which is confusing about + // what actually went wrong. + let err = verify_sha256(b"anything", "").unwrap_err(); + assert!(err.to_string().contains("no sha256 recorded")); + } } diff --git a/bakery/src/install.rs b/bakery/src/install.rs index e6ab26e..858edca 100644 --- a/bakery/src/install.rs +++ b/bakery/src/install.rs @@ -1,19 +1,62 @@ -use anyhow::{Context, Result}; +use anyhow::{bail, Context, Result}; +use std::io::IsTerminal; use std::path::{Path, PathBuf}; use std::process::Command; use crate::download::{fetch_and_place, verify_sha256}; use crate::manifest::{fetch_binary, Package, Service}; use crate::state::{InstalledPackage, State}; +use crate::track::Track; -pub fn install_package(pkg: &Package, bin_dir: &Path) -> Result<()> { +/// Rejects a filename that isn't a safe single path component — no `/`, +/// `\`, empty, `.`, or `..`. `bin.name`/`svc.unit`/`cfg.example`/`pkg.name`/ +/// `license_file`/`desktop_file`/`data_archive` all come from the +/// minisign-verified index, so this is defense in depth (not exploitable +/// without a compromised signing key) rather than the primary guard — but +/// closing the path-traversal class is cheap enough to do anyway before any +/// of these are joined onto a fixed base directory. +fn ensure_safe_component(name: &str, what: &str) -> Result<()> { + if name.is_empty() || name == "." || name == ".." || name.contains('/') || name.contains('\\') { + bail!("refusing to install: {what} '{name}' is not a safe filename"); + } + Ok(()) +} + +/// Prompts `prompt [y/N] ` and returns the answer. `assume_yes` (the global +/// `--yes` flag) skips the prompt entirely; otherwise, a non-tty stdin +/// (CI, piped input) answers "no" rather than blocking on a read that will +/// never resolve. +fn confirm(prompt: &str, assume_yes: bool) -> bool { + if assume_yes { + return true; + } + if !std::io::stdin().is_terminal() { + return false; + } + use std::io::Write; + print!("{prompt} [y/N] "); + std::io::stdout().flush().ok(); + let mut buf = String::new(); + std::io::stdin().read_line(&mut buf).ok(); + matches!(buf.trim().to_lowercase().as_str(), "y" | "yes") +} + +pub fn install_package( + pkg: &Package, + bin_dir: &Path, + track: Track, + no_hooks: bool, + assume_yes: bool, +) -> Result<()> { + ensure_safe_component(&pkg.name, "package name")?; println!("installing {}@{}…", pkg.name, pkg.version); // 1. Download and verify all binaries. let mut binary_names = Vec::new(); for bin in &pkg.binaries { + ensure_safe_component(&bin.name, "binary name")?; let install_name = strip_arch_suffix(&bin.name); - let dest = bin_dir.join(&install_name); + let dest = bin_dir.join(install_name); fetch_and_place(bin, &dest)?; binary_names.push(install_name.to_string()); } @@ -45,46 +88,74 @@ pub fn install_package(pkg: &Package, bin_dir: &Path) -> Result<()> { service_names.push(svc.unit.clone()); } - // 7. Run post_install hooks. - for cmd in &pkg.post_install { - run_hook(cmd, &pkg.name)?; + // 7. Run post_install hooks — arbitrary `sh -c` on index-controlled + // strings, so this is gated behind --no-hooks and an interactive + // confirmation rather than running unconditionally. + if !pkg.post_install.is_empty() { + if no_hooks { + println!( + " note: skipped {} post_install hook(s) for {} (--no-hooks)", + pkg.post_install.len(), + pkg.name + ); + } else if confirm( + &format!( + " run {} post_install hook(s) for {}?", + pkg.post_install.len(), + pkg.name + ), + assume_yes, + ) { + for cmd in &pkg.post_install { + run_hook(cmd, &pkg.name)?; + } + } else { + println!(" skipped post_install hooks for {} (declined)", pkg.name); + } } - // 8. Record in state. - let mut state = State::load()?; - state.record(InstalledPackage { - name: pkg.name.clone(), - version: pkg.version.clone(), - binaries: binary_names, - services: service_names, - installed_at: chrono::Utc::now().to_rfc3339(), - }); - state.save()?; + // 8. Record in state, under an exclusive lock so a concurrent `bakery` + // invocation can't clobber this install's record with its own. + State::with_lock(|state| { + state.record(InstalledPackage { + name: pkg.name.clone(), + version: pkg.version.clone(), + binaries: binary_names, + services: service_names, + installed_at: chrono::Utc::now().to_rfc3339(), + track, + }); + Ok(()) + })?; println!(" {} installed successfully", pkg.name); warn_path_if_needed(bin_dir); Ok(()) } -pub fn remove_package(pkg_name: &str, bin_dir: &Path) -> Result<()> { - let mut state = State::load()?; - let installed = match state.remove(pkg_name) { +pub fn remove_package(pkg_name: &str, bin_dir: &Path, assume_yes: bool) -> Result<()> { + let installed = State::with_lock(|state| Ok(state.remove(pkg_name)))?; + let installed = match installed { Some(p) => p, None => { eprintln!("{pkg_name} is not installed"); return Ok(()); } }; - // Commit removal immediately — file cleanup below is best-effort. - state.save()?; + // State is already committed by with_lock above — everything from here + // is best-effort file cleanup, and must all run even if part of it fails. - // Remove binaries. + // Remove binaries. Collect failures instead of aborting on the first one + // so a stuck/permission-denied binary doesn't skip service removal and + // the config/data-preserved messages below. + let mut failures = Vec::new(); for bin in &installed.binaries { let path = bin_dir.join(bin); if path.exists() { - std::fs::remove_file(&path) - .with_context(|| format!("removing {}", path.display()))?; - println!(" removed {}", path.display()); + match std::fs::remove_file(&path) { + Ok(()) => println!(" removed {}", path.display()), + Err(e) => failures.push(format!("{}: {e}", path.display())), + } } } @@ -93,7 +164,7 @@ pub fn remove_package(pkg_name: &str, bin_dir: &Path) -> Result<()> { let service_dir = systemd_user_dir(); for unit in &installed.services { let unit_path = service_dir.join(unit); - if confirm_remove_unit(unit) { + if confirm_remove_unit(unit, assume_yes) { let _ = Command::new("systemctl") .args(["--user", "disable", "--now", unit]) .status(); @@ -121,6 +192,14 @@ pub fn remove_package(pkg_name: &str, bin_dir: &Path) -> Result<()> { println!(" data preserved at {}", data_dir.display()); } + if !failures.is_empty() { + eprintln!(" failed to remove {} binary/binaries:", failures.len()); + for f in &failures { + eprintln!(" {f}"); + } + bail!("{pkg_name} removed from state, but some binaries could not be deleted"); + } + println!(" {pkg_name} removed"); Ok(()) } @@ -130,6 +209,7 @@ fn scaffold_config(cfg: &crate::manifest::ConfigScaffold, pkg: &Package) -> Resu std::fs::create_dir_all(&dir)?; if let Some(example) = &cfg.example { + ensure_safe_component(example, "config.example")?; let dest = dir.join(example); if !dest.exists() { if let Some((primary, fallback)) = pkg.artifact_urls(example) { @@ -217,6 +297,7 @@ fn fetch_verify_write( } fn install_license(pkg: &Package, filename: &str) -> Result<()> { + ensure_safe_component(filename, "license_file")?; let dest = dirs::data_dir() .unwrap_or_else(|| PathBuf::from("~/.local/share")) .join("licenses") @@ -226,6 +307,7 @@ fn install_license(pkg: &Package, filename: &str) -> Result<()> { } fn install_desktop_file(pkg: &Package, filename: &str) -> Result<()> { + ensure_safe_component(filename, "desktop_file")?; let dest = dirs::data_dir() .unwrap_or_else(|| PathBuf::from("~/.local/share")) .join("applications") @@ -234,6 +316,7 @@ fn install_desktop_file(pkg: &Package, filename: &str) -> Result<()> { } fn install_data_archive(pkg: &Package, filename: &str) -> Result<()> { + ensure_safe_component(filename, "data_archive")?; let data_dir = dirs::data_dir() .unwrap_or_else(|| PathBuf::from("~/.local/share")) .join(&pkg.name); @@ -253,7 +336,15 @@ fn fetch_extract_archive( sha256: &Option, dest_dir: &Path, ) -> Result<()> { - let tmp_archive = std::env::temp_dir().join(format!("bakery-{}-{filename}", pkg.name)); + // A securely-named, process-unique temp file — the old + // `std::env::temp_dir().join(format!("bakery-{name}-{filename}"))` was a + // predictable path on a shared /tmp, so another local user could + // pre-plant a symlink there for `fetch_verify_write`'s write to follow. + let tmp_archive = tempfile::Builder::new() + .prefix(&format!("bakery-{}-", pkg.name)) + .tempfile() + .context("creating temp file for archive download")? + .into_temp_path(); fetch_verify_write(pkg, filename, sha256, &tmp_archive, "data archive")?; if !tmp_archive.exists() { @@ -261,13 +352,21 @@ fn fetch_extract_archive( return Ok(()); } + verify_archive_paths(&tmp_archive)?; + std::fs::create_dir_all(dest_dir)?; let status = Command::new("tar") - .args(["xzf", &tmp_archive.to_string_lossy(), "-C"]) + .args([ + "xzf", + &tmp_archive.to_string_lossy(), + "--no-same-owner", + "--no-same-permissions", + "-C", + ]) .arg(dest_dir) .status() .with_context(|| format!("running tar to extract {filename}"))?; - let _ = std::fs::remove_file(&tmp_archive); + // `tmp_archive` (a `TempPath` guard) deletes the file when it drops here. if status.success() { println!(" extracted {filename} to {}", dest_dir.display()); @@ -277,44 +376,99 @@ fn fetch_extract_archive( Ok(()) } +/// Lists `archive_path`'s contents via `tar tvf` and rejects the archive +/// outright (no extraction) if any entry is a symlink or has an unsafe path +/// (`..` component, or absolute). `--no-same-owner --no-same-permissions` on +/// the actual extraction covers ownership/permission escalation, but not a +/// symlink or `../` entry walking the extraction outside `dest_dir` — this +/// closes that gap before `tar` ever touches disk. +fn verify_archive_paths(archive_path: &Path) -> Result<()> { + let output = Command::new("tar") + .arg("tvf") + .arg(archive_path) + .output() + .context("listing archive contents")?; + if !output.status.success() { + bail!( + "tar tvf exited with {} listing archive contents — refusing to extract", + output.status + ); + } + + let listing = String::from_utf8_lossy(&output.stdout); + for line in listing.lines() { + if line.trim().is_empty() { + continue; + } + // tar -tvf: ` /