diff --git a/crates/didbot-tls/src/storage.rs b/crates/didbot-tls/src/storage.rs index e51e0957..357bd4e9 100644 --- a/crates/didbot-tls/src/storage.rs +++ b/crates/didbot-tls/src/storage.rs @@ -206,21 +206,25 @@ impl CertStore { /// Persists a certificate chain, its key and metadata, each `0600`. /// - /// Writes the key first, then the certificate, then the metadata: a - /// crash between them leaves either nothing usable yet (no cert.pem, so - /// [`certificate`](Self::certificate) reports `None` and a restart just - /// re-issues) or a complete, internally consistent set. It never leaves - /// a cert.pem paired with the wrong key on disk. + /// All three are staged beside their final paths before any is renamed + /// into place (see `Staged`), so a save that fails leaves the previous + /// set untouched, and a crash leaves every file whole: its old contents + /// or its new ones, never empty or cut short. `meta.json` is renamed + /// last, so its presence means the key and certificate beside it were + /// both written in full. The renames are three steps, not one: a crash + /// between them can leave a new key beside the old certificate. pub fn save_certificate( &self, cert_pem: &str, key_pem: &str, meta: &CertMeta, ) -> Result<(), StorageError> { - self.write_string(KEY_FILE, key_pem)?; - self.write_string(CERT_FILE, cert_pem)?; - self.write_json(META_FILE, meta)?; - Ok(()) + let key = Staged::new(&self.dir.join(KEY_FILE), key_pem.as_bytes())?; + let cert = Staged::new(&self.dir.join(CERT_FILE), cert_pem.as_bytes())?; + let meta = Staged::json(&self.dir.join(META_FILE), meta)?; + key.commit()?; + cert.commit()?; + meta.commit() } fn read_string(&self, name: &str) -> Result { @@ -251,24 +255,12 @@ impl CertStore { .map(Some) .map_err(|source| StorageError::Parse { path, source }) } - - fn write_string(&self, name: &str, contents: &str) -> Result<(), StorageError> { - let path = self.dir.join(name); - write_private(&path, contents.as_bytes()) - } - - fn write_json(&self, name: &str, value: &T) -> Result<(), StorageError> { - write_json_at(&self.dir.join(name), value) - } } -/// Serialises `value` and writes it `0600` at `path`. +/// Serialises `value` and writes it `0600` at `path`, replacing whatever was +/// there in one step. fn write_json_at(path: &Path, value: &T) -> Result<(), StorageError> { - let text = serde_json::to_string_pretty(value).map_err(|source| StorageError::Parse { - path: path.to_owned(), - source, - })?; - write_private(path, text.as_bytes()) + Staged::json(path, value)?.commit() } /// The single path component `zone`'s certificate directory is named after: @@ -291,37 +283,113 @@ fn zone_directory_component(zone: &str) -> Result { Ok(component) } -/// Writes `contents` to `path`, `0600`, replacing whatever was there. -fn write_private(path: &Path, contents: &[u8]) -> Result<(), StorageError> { - let mut options = fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - options.mode(FILE_MODE); +/// A file's next contents, written and synced beside its final path but not +/// yet renamed over it. +/// +/// The shape `didbot-pds`'s write-ahead log gives its compaction: the +/// replacement is built whole, `0600`, at a scratch path, and one +/// `rename(2)` makes it the file. A crash before the rename leaves the old +/// file as it was, plus a scratch file the next write removes; a crash after +/// it leaves the new one. The directory is synced after the rename, because +/// on most filesystems a rename that has not been durably recorded in its +/// parent can be undone by a power cut that kept the file it pointed at. +/// +/// Dropped without [`commit`](Self::commit), it removes its scratch file, so +/// a save that fails partway leaves no private key under a name nothing +/// reads. +struct Staged { + scratch: PathBuf, + path: PathBuf, + committed: bool, +} + +impl Staged { + /// Writes `contents`, `0600`, to a scratch file beside `path`. + fn new(path: &Path, contents: &[u8]) -> Result { + let scratch = scratch_path(path); + // A scratch file left by a write that died is removed rather than + // reused: `create_new` below is what guarantees the mode, and + // `rename(2)` carries the source's mode with it, so a leftover at a + // loose mode would widen the file it replaced. + match fs::remove_file(&scratch) { + Ok(()) => {} + Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Err(source) => return Err(io_error(&scratch, source)), + } + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(FILE_MODE); + } + let mut file = options + .open(&scratch) + .map_err(|source| io_error(&scratch, source))?; + let staged = Self { + scratch, + path: path.to_owned(), + committed: false, + }; + io::Write::write_all(&mut file, contents) + .map_err(|source| io_error(&staged.scratch, source))?; + file.sync_all() + .map_err(|source| io_error(&staged.scratch, source))?; + Ok(staged) } - let mut file = options.open(path).map_err(|source| StorageError::Io { - path: path.to_owned(), - source, - })?; - io::Write::write_all(&mut file, contents).map_err(|source| StorageError::Io { + + /// Serialises `value` and stages it beside `path`. + fn json(path: &Path, value: &T) -> Result { + let text = serde_json::to_string_pretty(value).map_err(|source| StorageError::Parse { + path: path.to_owned(), + source, + })?; + Self::new(path, text.as_bytes()) + } + + /// Renames the scratch file over its final path, and makes that durable. + fn commit(mut self) -> Result<(), StorageError> { + fs::rename(&self.scratch, &self.path).map_err(|source| io_error(&self.scratch, source))?; + self.committed = true; + sync_dir(self.path.parent().unwrap_or_else(|| Path::new("."))) + } +} + +impl Drop for Staged { + fn drop(&mut self) { + if !self.committed { + let _ = fs::remove_file(&self.scratch); + } + } +} + +/// Where `path`'s replacement is built: `.tmp` in the same directory, +/// so the rename that finishes it never crosses a filesystem. +fn scratch_path(path: &Path) -> PathBuf { + let mut name = path + .file_name() + .map(std::ffi::OsStr::to_os_string) + .unwrap_or_default(); + name.push(".tmp"); + path.with_file_name(name) +} + +/// Syncs a directory entry, so a rename cannot be undone by a power cut. +fn sync_dir(dir: &Path) -> Result<(), StorageError> { + // Not every platform lets a directory be opened as a file. Where it does + // not, the rename is as durable as that platform makes it and there is + // nothing further to do here. + match fs::File::open(dir) { + Ok(handle) => handle.sync_all().map_err(|source| io_error(dir, source)), + Err(_) => Ok(()), + } +} + +fn io_error(path: &Path, source: io::Error) -> StorageError { + StorageError::Io { path: path.to_owned(), source, - })?; - #[cfg(unix)] - { - // `create(true).truncate(true)` on a file that already existed keeps - // its old mode; a key file that predates this fix, or one restored - // by hand, is tightened on every write rather than trusted. - use std::os::unix::fs::PermissionsExt; - fs::set_permissions(path, fs::Permissions::from_mode(FILE_MODE)).map_err(|source| { - StorageError::Io { - path: path.to_owned(), - source, - } - })?; } - Ok(()) } /// Creates `dir` `0700` if absent, and tightens it to `0700` if it was @@ -407,10 +475,8 @@ mod tests { /// A key file that was already on disk at a loose mode — restored from a /// backup, copied into place by hand, or written by a build from before - /// the mode was set — is tightened by the write that replaces it. - /// `OpenOptions::mode` alone would not do it: it applies only to a file - /// the call creates, and `create(true).truncate(true)` on an existing - /// path reuses the mode it already had. + /// the mode was set — is replaced by the next write with one created + /// `0600`. #[test] fn a_key_file_that_was_already_loose_is_tightened_by_the_next_write() { let tmp = tempdir(); @@ -460,12 +526,86 @@ mod tests { assert!(store.certificate().unwrap().is_none()); } + /// A write that died before its rename leaves a scratch file beside the + /// real one, at whatever mode and length it had reached. Reads never see + /// it, and the next save replaces it rather than renaming it into place. + #[test] + fn a_scratch_file_left_by_a_dead_write_neither_hides_nor_replaces_the_pair() { + let tmp = tempdir(); + let store = CertStore::open(tmp.path()).unwrap(); + let meta = CertMeta { + directory_url: "https://example.invalid/directory".into(), + issued_at: time::OffsetDateTime::now_utc(), + }; + store + .save_certificate("the-cert", "the-key", &meta) + .unwrap(); + + let key = tmp.path().join("tls").join(KEY_FILE); + let leftover = scratch_path(&key); + fs::write(&leftover, "half of a k").unwrap(); + fs::set_permissions(&leftover, fs::Permissions::from_mode(0o644)).unwrap(); + + let (cert, key_pem, _) = store.certificate().unwrap().unwrap(); + assert_eq!((cert.as_str(), key_pem.as_str()), ("the-cert", "the-key")); + + store + .save_certificate("the-cert", "a renewed key", &meta) + .unwrap(); + + let (_, key_pem, _) = store.certificate().unwrap().unwrap(); + assert_eq!(key_pem, "a renewed key"); + let mode = fs::metadata(&key).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, FILE_MODE, + "the leftover's mode {mode:04o} travelled with the rename" + ); + assert!( + !leftover.exists(), + "a committed save leaves no scratch file" + ); + } + + /// A save that cannot finish must not touch the files a restart boots + /// from. Staging the metadata is made to fail here — its scratch path is + /// taken by a directory, which cannot be removed as a file or created as + /// one — after the key and certificate were both staged, and the pair on + /// disk is the one from before. + #[test] + fn a_save_that_fails_leaves_the_previous_pair_untouched() { + let tmp = tempdir(); + let store = CertStore::open(tmp.path()).unwrap(); + let meta = CertMeta { + directory_url: "https://example.invalid/directory".into(), + issued_at: time::OffsetDateTime::now_utc(), + }; + store + .save_certificate("the-cert", "the-key", &meta) + .unwrap(); + + let dir = tmp.path().join("tls"); + fs::create_dir(scratch_path(&dir.join(META_FILE))).unwrap(); + + store + .save_certificate("a renewed cert", "a renewed key", &meta) + .unwrap_err(); + + let (cert, key, _) = store.certificate().unwrap().unwrap(); + assert_eq!((cert.as_str(), key.as_str()), ("the-cert", "the-key")); + for name in [CERT_FILE, KEY_FILE] { + assert!( + !scratch_path(&dir.join(name)).exists(), + "{name}'s scratch file outlived the save that failed" + ); + } + } + fn tempdir() -> TempDir { TempDir::new() } /// A minimal self-cleaning temp directory, so this crate does not take a - /// `tempfile` dependency just for four tests. + /// `tempfile` dependency just for these tests. struct TempDir(PathBuf); impl TempDir {