From ae55db38d45bb8e1415973cec5d32db1e599fe67 Mon Sep 17 00:00:00 2001 From: archipelago Date: Sun, 2 Aug 2026 13:14:10 -0400 Subject: [PATCH] fix(tls): make cert regeneration on rename atomic and validated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit regenerate_tls_cert() passed -keyout /etc/archipelago/ssl/archipelago.key and -out .../archipelago.crt, so openssl wrote straight into the files nginx is serving from. If openssl died partway, was killed, or the disk filled, the live key and cert were already truncated — a routine `server.set-name` could take HTTPS down with no way back. Reproduced: the live key goes from a valid 2048-bit PEM to 33 unparseable bytes. Mirror the discipline gen_tls() already uses in the ISO builder: generate into .new siblings of the destinations (same directory, so the final mv is a rename(2) and therefore atomic), parse both halves back with `openssl pkey` and `openssl x509` and compare the extracted public keys to prove they are valid and belong together, and only then swap them in. On any failure the existing key and cert are left byte-for-byte untouched and the error is returned. Staging files are cleared before the attempt and on every exit path, success or failure. Permissions: the staging key is created by `install -m` carrying the live key's own mode and owner *before* openssl writes into it (openssl truncates an existing -keyout file rather than recreating it), so the new private key is never group- or world-readable, not even between generation and a chmod. A live mode that grants group/other any access is not reproduced — the key falls back to 0600 — so the swap can never widen permissions. Cert content and parameters are unchanged: same subject, same SAN construction, same rsa:2048, same 3650 days. This is an atomicity and validation fix, not a crypto change. Testing seam: the hardcoded sudo prefix and absolute paths made this untestable, so the logic moved into a small TlsMaterial struct holding the ssl dir, the openssl binary path and a privileged flag. Production is TlsMaterial::production(); tests point it at a temp dir, drop sudo, and substitute a stub openssl. Against the pre-fix shape the two atomicity tests fail (live key modified; garbage accepted); against this change all five pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/api/rpc/system/handlers.rs | 542 ++++++++++++++++-- 1 file changed, 504 insertions(+), 38 deletions(-) diff --git a/core/archipelago/src/api/rpc/system/handlers.rs b/core/archipelago/src/api/rpc/system/handlers.rs index 27b4e273..45774355 100644 --- a/core/archipelago/src/api/rpc/system/handlers.rs +++ b/core/archipelago/src/api/rpc/system/handlers.rs @@ -1,6 +1,7 @@ use super::*; use crate::api::rpc::RpcHandler; use anyhow::{Context, Result}; +use std::path::{Path, PathBuf}; use tracing::{debug, info, warn}; impl RpcHandler { @@ -478,50 +479,291 @@ async fn set_system_hostname(hostname: &str) -> Result<()> { Ok(()) } +const TLS_SSL_DIR: &str = "/etc/archipelago/ssl"; +const TLS_KEY_NAME: &str = "archipelago.key"; +const TLS_CRT_NAME: &str = "archipelago.crt"; +const TLS_KEY_STAGING_NAME: &str = "archipelago.key.new"; +const TLS_CRT_STAGING_NAME: &str = "archipelago.crt.new"; + +const SUDO_BIN: &str = "/usr/bin/sudo"; +const OPENSSL_BIN: &str = "/usr/bin/openssl"; +const STAT_BIN: &str = "/usr/bin/stat"; +const INSTALL_BIN: &str = "/usr/bin/install"; +const MKDIR_BIN: &str = "/usr/bin/mkdir"; +const MV_BIN: &str = "/usr/bin/mv"; +const RM_BIN: &str = "/usr/bin/rm"; + +/// Modes used only when there is no existing file to copy them from (a node +/// whose TLS material has somehow gone missing entirely). The key is private +/// material; the cert is public. +const TLS_KEY_FALLBACK_MODE: &str = "600"; +const TLS_CRT_FALLBACK_MODE: &str = "644"; + +/// Where this node's TLS material lives, and how we are allowed to touch it. +/// +/// This type exists for two reasons. The first is that every step of the +/// generate/validate/swap dance below has to run as root — the live key is +/// root-owned 0600 and the daemon is not root — so each step needs the same +/// `sudo -n` prefix and the same error handling. The second is testability: +/// production uses [`TlsMaterial::production`], while tests point `ssl_dir` at +/// a temp dir, drop the `sudo` prefix, and can substitute `openssl_bin` for a +/// stub that fails partway through generation. That is the only seam added +/// here; nothing else about the module changed. +#[derive(Clone, Debug)] +struct TlsMaterial { + ssl_dir: PathBuf, + openssl_bin: PathBuf, + /// Prefix every command with `sudo -n`. + privileged: bool, +} + +impl TlsMaterial { + fn production() -> Self { + Self { + ssl_dir: PathBuf::from(TLS_SSL_DIR), + openssl_bin: PathBuf::from(OPENSSL_BIN), + privileged: true, + } + } + + fn key(&self) -> PathBuf { + self.ssl_dir.join(TLS_KEY_NAME) + } + + fn crt(&self) -> PathBuf { + self.ssl_dir.join(TLS_CRT_NAME) + } + + // The staging files are siblings of their destinations on purpose: same + // directory means same filesystem, which means the final `mv` is a + // rename(2) and therefore atomic. + fn key_staging(&self) -> PathBuf { + self.ssl_dir.join(TLS_KEY_STAGING_NAME) + } + + fn crt_staging(&self) -> PathBuf { + self.ssl_dir.join(TLS_CRT_STAGING_NAME) + } + + fn cmd>(&self, program: S) -> tokio::process::Command { + if self.privileged { + let mut cmd = tokio::process::Command::new(SUDO_BIN); + cmd.arg("-n").arg(program); + cmd + } else { + tokio::process::Command::new(program) + } + } + + /// `(mode, uid, gid)` of an existing file, or `None` if it is not there. + async fn stat_attrs(&self, path: &Path) -> Option<(String, String, String)> { + let mut cmd = self.cmd(STAT_BIN); + cmd.arg("-c").arg("%a:%u:%g").arg(path); + let out = cmd.output().await.ok()?; + if !out.status.success() { + return None; + } + let text = String::from_utf8_lossy(&out.stdout).trim().to_string(); + let mut parts = text.split(':'); + let mode = parts.next()?.to_string(); + let uid = parts.next()?.to_string(); + let gid = parts.next()?.to_string(); + if mode.is_empty() || uid.is_empty() || gid.is_empty() { + return None; + } + Some((mode, uid, gid)) + } + + /// Create `dest` as an empty file that *already* carries `mode` (and the + /// live file's owner, when we know it). + /// + /// Pre-creating the staging file is the whole trick for permissions: + /// openssl's `-keyout` truncates an existing file rather than recreating + /// it, so the mode set here is the mode the new key has from its very + /// first byte. There is no window — not even a microsecond — in which the + /// freshly generated private key sits on disk group- or world-readable + /// waiting for a follow-up `chmod`. + async fn create_empty( + &self, + dest: &Path, + mode: &str, + owner: Option<(&str, &str)>, + ) -> Result<()> { + let mut cmd = self.cmd(INSTALL_BIN); + cmd.arg("-m").arg(mode); + if let Some((uid, gid)) = owner { + cmd.arg("-o").arg(uid).arg("-g").arg(gid); + } + cmd.arg("/dev/null").arg(dest); + run_checked(cmd, "install (create staging file)").await?; + Ok(()) + } + + /// Best-effort removal of both staging files. Called before an attempt (to + /// clear anything a previous crash left) and after every attempt, success + /// or failure, so staging artefacts never accumulate next to the live cert. + async fn clear_staging(&self) { + let mut cmd = self.cmd(RM_BIN); + cmd.arg("-f").arg(self.key_staging()).arg(self.crt_staging()); + let _ = cmd.output().await; + } + + /// Regenerate the keypair, validate it, and swap it into place. + /// + /// The contract this upholds: on *any* failure the existing key and cert + /// are left byte-for-byte untouched and an error is returned. A rename + /// whose cert regeneration fails is a no-op on disk, never a partial write + /// that leaves nginx serving a truncated key. + async fn regenerate(&self, hostname: &str) -> Result<()> { + self.clear_staging().await; + let result = self.stage_validate_and_swap(hostname).await; + self.clear_staging().await; + result + } + + async fn stage_validate_and_swap(&self, hostname: &str) -> Result<()> { + let mut mkdir = self.cmd(MKDIR_BIN); + mkdir.arg("-p").arg(&self.ssl_dir); + run_checked(mkdir, "mkdir -p ssl dir").await?; + + let key_attrs = self.stat_attrs(&self.key()).await; + let crt_attrs = self.stat_attrs(&self.crt()).await; + + let key_mode = private_key_mode(key_attrs.as_ref().map(|(m, _, _)| m.as_str())); + let crt_mode = crt_attrs + .as_ref() + .map(|(m, _, _)| m.clone()) + .unwrap_or_else(|| TLS_CRT_FALLBACK_MODE.to_string()); + + self.create_empty( + &self.key_staging(), + &key_mode, + key_attrs.as_ref().map(|(_, u, g)| (u.as_str(), g.as_str())), + ) + .await?; + self.create_empty( + &self.crt_staging(), + &crt_mode, + crt_attrs.as_ref().map(|(_, u, g)| (u.as_str(), g.as_str())), + ) + .await?; + + // Cert parameters are deliberately identical to what this function has + // always produced: same subject, same SAN construction, same rsa:2048, + // same 3650 days. Only *where* openssl writes has changed. + let subj = format!("/C=XX/ST=Bitcoin/L=Node/O=Archipelago/CN={hostname}"); + let san = format!( + "subjectAltName=DNS:{hostname},DNS:{hostname}.local,DNS:localhost,IP:127.0.0.1" + ); + let mut gen = self.cmd(&self.openssl_bin); + gen.arg("req") + .arg("-x509") + .arg("-nodes") + .arg("-days") + .arg("3650") + .arg("-newkey") + .arg("rsa:2048") + .arg("-keyout") + .arg(self.key_staging()) + .arg("-out") + .arg(self.crt_staging()) + .arg("-subj") + .arg(&subj) + .arg("-addext") + .arg(&san); + run_checked(gen, "openssl cert regen").await?; + + // Parse both halves back before trusting either. A zero-exit openssl + // that somehow produced an empty or truncated artefact still fails + // here — nginx is never handed material we have not read back + // ourselves. Extracting the public key from each half also proves the + // two belong together, so a stale cert can never be paired with a + // fresh key. + let mut key_pub = self.cmd(&self.openssl_bin); + key_pub + .arg("pkey") + .arg("-in") + .arg(self.key_staging()) + .arg("-pubout"); + let key_pub = run_checked(key_pub, "openssl pkey (validate new key)").await?; + + let mut crt_pub = self.cmd(&self.openssl_bin); + crt_pub + .arg("x509") + .arg("-in") + .arg(self.crt_staging()) + .arg("-noout") + .arg("-pubkey"); + let crt_pub = run_checked(crt_pub, "openssl x509 (validate new cert)").await?; + + if key_pub.trim_ascii().is_empty() || key_pub.trim_ascii() != crt_pub.trim_ascii() { + anyhow::bail!( + "regenerated key and certificate do not match — refusing to install them" + ); + } + + // rename(2) within one directory: a reader sees either the whole old + // file or the whole new one, never a partial write. Two files cannot + // be swapped in a single atomic step, so there is a sub-millisecond + // window between the two renames in which cert and key are from + // different generations — but both files are fully written and + // validated by this point, so the only way to land in that window is a + // rename failure on an already-created sibling, which does not need + // space or allocation and effectively cannot fail here. + self.swap_into_place(&self.crt_staging(), &self.crt()).await?; + self.swap_into_place(&self.key_staging(), &self.key()).await?; + Ok(()) + } + + async fn swap_into_place(&self, staging: &Path, live: &Path) -> Result<()> { + let mut cmd = self.cmd(MV_BIN); + cmd.arg("-f").arg(staging).arg(live); + run_checked(cmd, "mv (install new TLS material)").await?; + Ok(()) + } +} + +/// The mode to give the freshly generated private key. +/// +/// Copy the live key's mode so a node that deliberately tightened it keeps +/// that, but never copy a mode that grants group or other any access — a +/// world-readable private key is a bug we should not faithfully reproduce, and +/// widening is never allowed. +fn private_key_mode(existing: Option<&str>) -> String { + match existing.and_then(|m| u32::from_str_radix(m, 8).ok()) { + Some(mode) if mode & 0o077 == 0 => format!("{mode:o}"), + _ => TLS_KEY_FALLBACK_MODE.to_string(), + } +} + +/// Run a command, failing with its stderr if it exits non-zero. +async fn run_checked(mut cmd: tokio::process::Command, what: &str) -> Result> { + let out = cmd + .output() + .await + .with_context(|| format!("failed to run {what}"))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr).trim().to_string(); + if stderr.is_empty() { + anyhow::bail!("{what} failed"); + } + anyhow::bail!("{what} failed: {stderr}"); + } + Ok(out.stdout) +} + /// Regenerate the self-signed HTTPS cert (`/etc/archipelago/ssl/archipelago.{crt,key}`) /// with a SAN covering `hostname`, `hostname.local`, `localhost`, and 127.0.0.1, then /// reload nginx so it picks up the new cert. Still self-signed (browsers will warn /// on first visit regardless), but avoids stacking a hostname-mismatch warning on /// top once a node has been renamed away from the install-time default. +/// +/// Generation goes to staging siblings and is parsed back before being swapped +/// in — see [`TlsMaterial::regenerate`]. A failed regeneration leaves the live +/// key and cert exactly as they were, because a routine rename must never be +/// able to take HTTPS down. async fn regenerate_tls_cert(hostname: &str) -> Result<()> { - let subj = format!("/C=XX/ST=Bitcoin/L=Node/O=Archipelago/CN={hostname}"); - let san = - format!("subjectAltName=DNS:{hostname},DNS:{hostname}.local,DNS:localhost,IP:127.0.0.1"); - let output = tokio::process::Command::new("/usr/bin/sudo") - .args([ - "-n", - "/usr/bin/openssl", - "req", - "-x509", - "-nodes", - "-days", - "3650", - "-newkey", - "rsa:2048", - "-keyout", - "/etc/archipelago/ssl/archipelago.key", - "-out", - "/etc/archipelago/ssl/archipelago.crt", - "-subj", - &subj, - "-addext", - &san, - ]) - .output() - .await - .context("Failed to run openssl")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); - anyhow::bail!( - "{}", - if stderr.is_empty() { - "openssl cert regen failed".to_string() - } else { - stderr - } - ); - } + TlsMaterial::production().regenerate(hostname).await?; let reload = tokio::process::Command::new("/usr/bin/sudo") .args(["-n", "/usr/bin/systemctl", "reload", "nginx"]) @@ -840,3 +1082,227 @@ impl RpcHandler { } const KIOSK_DISPLAY_CONF: &str = "/etc/archipelago/kiosk-display.conf"; + +#[cfg(test)] +mod tls_regen_tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + /// A `TlsMaterial` pointed at a temp dir, running unprivileged so the test + /// needs no sudo. Everything else — the command sequence, the staging + /// paths, the validation, the swap — is the production code path. + fn material(dir: &Path, openssl_bin: &Path) -> TlsMaterial { + TlsMaterial { + ssl_dir: dir.to_path_buf(), + openssl_bin: openssl_bin.to_path_buf(), + privileged: false, + } + } + + /// Seed the directory the way an installed node looks: a real keypair with + /// the key at 0600. + fn seed_live_material(dir: &Path) { + let key = dir.join(TLS_KEY_NAME); + let crt = dir.join(TLS_CRT_NAME); + let status = std::process::Command::new(OPENSSL_BIN) + .args(["req", "-x509", "-nodes", "-days", "3650", "-newkey", "rsa:2048"]) + .arg("-keyout") + .arg(&key) + .arg("-out") + .arg(&crt) + .arg("-subj") + .arg("/C=XX/ST=Bitcoin/L=Node/O=Archipelago/CN=oldname") + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .expect("seed openssl"); + assert!(status.success(), "seeding the live keypair failed"); + std::fs::set_permissions(&key, std::fs::Permissions::from_mode(0o600)).unwrap(); + } + + /// Write an executable stand-in for openssl. It intercepts `req` (the + /// generation step) and delegates everything else — the `pkey`/`x509` + /// validation calls — to the real binary. + fn stub_openssl(dir: &Path, name: &str, exit_code: u8) -> PathBuf { + let path = dir.join(name); + let script = format!( + r#"#!/bin/sh +# Stand-in for openssl that produces the wreckage a killed/ENOSPC openssl +# leaves behind: partial PEM in both output files. Exits {exit_code}. +if [ "$1" = "req" ]; then + key=""; out="" + while [ $# -gt 0 ]; do + case "$1" in + -keyout) key="$2"; shift ;; + -out) out="$2"; shift ;; + esac + shift + done + [ -n "$key" ] && printf -- '-----BEGIN PRIVATE KEY-----\ntruncated' > "$key" + [ -n "$out" ] && printf -- '-----BEGIN CERTIFICATE-----\ntruncated' > "$out" + echo "simulated openssl failure" >&2 + exit {exit_code} +fi +exec {OPENSSL_BIN} "$@" +"# + ); + std::fs::write(&path, script).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); + path + } + + fn snapshot(dir: &Path) -> (Vec, Vec, u32) { + let key = dir.join(TLS_KEY_NAME); + let mode = std::fs::metadata(&key).unwrap().permissions().mode() & 0o777; + ( + std::fs::read(&key).unwrap(), + std::fs::read(dir.join(TLS_CRT_NAME)).unwrap(), + mode, + ) + } + + fn assert_no_staging_left(dir: &Path) { + assert!( + !dir.join(TLS_KEY_STAGING_NAME).exists(), + "staging key was left behind" + ); + assert!( + !dir.join(TLS_CRT_STAGING_NAME).exists(), + "staging cert was left behind" + ); + } + + /// The defect this whole change exists to fix: when openssl dies partway + /// through, the live key and cert must be byte-identical to what they were + /// before. Against the previous implementation (openssl writing straight + /// to the live paths) this fails — the live key is truncated to a few + /// bytes of PEM header and nginx has nothing to serve. + #[tokio::test] + async fn failed_generation_leaves_live_key_and_cert_untouched() { + let dir = tempfile::tempdir().unwrap(); + seed_live_material(dir.path()); + let before = snapshot(dir.path()); + + let openssl = stub_openssl(dir.path(), "openssl-crash", 1); + let err = material(dir.path(), &openssl) + .regenerate("newname") + .await + .expect_err("a failing openssl must surface as an error"); + assert!( + err.to_string().contains("openssl cert regen failed"), + "unexpected error: {err}" + ); + + let after = snapshot(dir.path()); + assert_eq!(before.0, after.0, "live private key was modified"); + assert_eq!(before.1, after.1, "live certificate was modified"); + assert_eq!(before.2, after.2, "live private key mode changed"); + assert_no_staging_left(dir.path()); + } + + /// The other half of the guard: openssl exiting 0 is not proof it produced + /// anything usable. Truncated output must be caught by the parse-back and + /// must likewise leave the live material alone. + #[tokio::test] + async fn unparseable_output_is_rejected_and_live_material_untouched() { + let dir = tempfile::tempdir().unwrap(); + seed_live_material(dir.path()); + let before = snapshot(dir.path()); + + let openssl = stub_openssl(dir.path(), "openssl-garbage", 0); + let err = material(dir.path(), &openssl) + .regenerate("newname") + .await + .expect_err("unparseable material must be rejected"); + assert!( + err.to_string().contains("validate new key"), + "unexpected error: {err}" + ); + + let after = snapshot(dir.path()); + assert_eq!(before.0, after.0, "live private key was modified"); + assert_eq!(before.1, after.1, "live certificate was modified"); + assert_no_staging_left(dir.path()); + } + + /// Happy path: a real regeneration swaps in a matching pair carrying the + /// new hostname, and the key keeps its 0600 mode across the swap. + #[tokio::test] + async fn successful_regeneration_swaps_in_a_matching_pair_and_keeps_mode() { + let dir = tempfile::tempdir().unwrap(); + seed_live_material(dir.path()); + let before = snapshot(dir.path()); + + material(dir.path(), Path::new(OPENSSL_BIN)) + .regenerate("newname") + .await + .expect("regeneration should succeed"); + + let after = snapshot(dir.path()); + assert_ne!(before.0, after.0, "the key should have been replaced"); + assert_eq!(after.2, 0o600, "the new key must still be 0600"); + assert_no_staging_left(dir.path()); + + // The installed pair parses and the two halves belong together. + let key_pub = std::process::Command::new(OPENSSL_BIN) + .arg("pkey") + .arg("-in") + .arg(dir.path().join(TLS_KEY_NAME)) + .arg("-pubout") + .output() + .unwrap(); + let crt_pub = std::process::Command::new(OPENSSL_BIN) + .arg("x509") + .arg("-in") + .arg(dir.path().join(TLS_CRT_NAME)) + .arg("-noout") + .arg("-pubkey") + .output() + .unwrap(); + assert!(key_pub.status.success() && crt_pub.status.success()); + assert_eq!(key_pub.stdout, crt_pub.stdout, "installed pair is mismatched"); + + // And the SAN carries the new hostname, which is why we regenerate. + let text = std::process::Command::new(OPENSSL_BIN) + .arg("x509") + .arg("-in") + .arg(dir.path().join(TLS_CRT_NAME)) + .arg("-noout") + .arg("-text") + .output() + .unwrap(); + let text = String::from_utf8_lossy(&text.stdout); + assert!(text.contains("DNS:newname"), "SAN missing hostname: {text}"); + assert!(text.contains("DNS:newname.local")); + assert!(text.contains("IP Address:127.0.0.1")); + } + + /// The staging private key must never exist world- or group-readable, not + /// even transiently: it is created pre-moded and openssl only truncates it. + #[tokio::test] + async fn staging_key_is_created_already_locked_down() { + let dir = tempfile::tempdir().unwrap(); + let m = material(dir.path(), Path::new(OPENSSL_BIN)); + m.create_empty(&m.key_staging(), TLS_KEY_FALLBACK_MODE, None) + .await + .unwrap(); + let mode = std::fs::metadata(m.key_staging()) + .unwrap() + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600); + } + + #[test] + fn private_key_mode_preserves_tight_modes_and_never_widens() { + assert_eq!(private_key_mode(Some("600")), "600"); + assert_eq!(private_key_mode(Some("400")), "400"); + // A live key that is somehow group/world readable is not reproduced. + assert_eq!(private_key_mode(Some("644")), "600"); + assert_eq!(private_key_mode(Some("640")), "600"); + // No existing file, or an unparseable mode, falls back to 0600. + assert_eq!(private_key_mode(None), "600"); + assert_eq!(private_key_mode(Some("garbage")), "600"); + } +}