diff --git a/NEWS b/NEWS index 2986d003..13708b2e 100644 --- a/NEWS +++ b/NEWS @@ -60,6 +60,11 @@ if the certificate has the self-signed user ID "Alice ", then `--userid-by-email alice@example.org` selects "Alice " for revocation. + - When writing to a file output, we first write to a temporary + file, then rename the file at the end of the operation so that it + has its desired name. There are two benefits: no one sees + partially written files, and one can safely use the same file as + input and output. * Changes in 0.41.0 ** New functionality diff --git a/src/commands/decrypt.rs b/src/commands/decrypt.rs index c1ac8cf6..5f6a0c80 100644 --- a/src/commands/decrypt.rs +++ b/src/commands/decrypt.rs @@ -69,6 +69,10 @@ pub fn dispatch(sq: Sq, command: cli::decrypt::Command) -> Result<()> { session_keys); if result.is_err() { if let Some(path) = command.output.path() { + // Drop output here so that the file is persisted and + // can be deleted. + drop(output); + if let Err(err) = std::fs::remove_file(path) { weprintln!("Decryption failed, failed to remove \ output saved to {}: {}", diff --git a/src/commands/verify.rs b/src/commands/verify.rs index f1569045..320307f0 100644 --- a/src/commands/verify.rs +++ b/src/commands/verify.rs @@ -46,6 +46,10 @@ pub fn dispatch(sq: Sq, command: cli::verify::Command) &mut output, signatures, signers); if result.is_err() { if let Some(path) = command.output.path() { + // Drop output here so that the file is persisted and + // can be deleted. + drop(output); + if let Err(err) = std::fs::remove_file(path) { weprintln!("Verification failed, failed to remove \ unverified output saved to {}: {}", diff --git a/src/common/file.rs b/src/common/file.rs index 394ce298..62fb70a7 100644 --- a/src/common/file.rs +++ b/src/common/file.rs @@ -1,12 +1,14 @@ /// Common file handling support. use std::{ - fs::OpenOptions, io::{self, Write, stdout}, + path::{Path, PathBuf}, }; use anyhow::{Context, Result}; +use tempfile::NamedTempFile; + use sequoia_openpgp::{ self as openpgp, armor, @@ -89,11 +91,7 @@ impl FileOrStdout { if let Some(path) = self.path() { if !path.exists() || sq.overwrite { Ok(Box::new( - OpenOptions::new() - .write(true) - .truncate(true) - .create(true) - .open(path) + PartFileWriter::create(path) .context("Failed to create output file")?, )) } else { @@ -108,6 +106,98 @@ impl FileOrStdout { } } +/// A writer that writes to a temporary file first, then persists the +/// file under the desired name. +/// +/// This has two benefits. First, consumers only see the file once we +/// are done writing to it, i.e. they don't see a partial file. +/// +/// Second, we guarantee not to overwrite the file until the operation +/// is finished. Therefore, it is safe to use the same file as input +/// and output. +struct PartFileWriter { + path: PathBuf, + sink: Option, +} + +impl io::Write for PartFileWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.sink()?.write(buf) + } + + fn flush(&mut self) -> io::Result<()> { + self.sink()?.flush() + } +} + +impl Drop for PartFileWriter { + fn drop(&mut self) { + if let Err(e) = self.persist() { + weprintln!(initial_indent = "Error: ", "{}", e); + std::process::exit(1); + } + } +} + +impl PartFileWriter { + /// Opens a file for writing. + /// + /// The file will be created under a different name in the target + /// directory, and will only be renamed to `path` once + /// [`PartFileWriter::persist`] is called or the object is + /// dropped. + pub fn create>(path: P) -> Result { + let path = path.as_ref().to_path_buf(); + let parent = path.parent() + .ok_or(anyhow::anyhow!("cannot write to the root"))?; + let file_name = path.file_name() + .ok_or(anyhow::anyhow!("cannot write to .."))?; + + let mut sink = tempfile::Builder::new(); + + // By default, temporary files are 0x600 on Unix. But, we + // rather want created files to respect umask. + platform! { + unix => { + use std::os::unix::fs::PermissionsExt; + let all_read_write = + std::fs::Permissions::from_mode(0o666); + + // The permissions will be masked by the user's umask. + sink.permissions(all_read_write); + }, + windows => { + // We cannot do the same on Windows. + }, + } + + let sink = sink + .prefix(file_name) + .suffix(".part") + .tempfile_in(parent)?; + + Ok(PartFileWriter { + path, + sink: Some(sink), + }) + } + + /// Returns a mutable reference to the file, or an error. + fn sink(&mut self) -> io::Result<&mut NamedTempFile> { + self.sink.as_mut().ok_or(io::Error::new( + io::ErrorKind::Other, + anyhow::anyhow!("file already persisted"))) + } + + /// Persists the file under its final name. + pub fn persist(&mut self) -> Result<()> { + if let Some(file) = self.sink.take() { + file.persist(&self.path)?; + } + Ok(()) + } +} + /// A writer that buffers all data, and scans for secret keys on drop. /// /// This is used to assert that we only write secret keys in places