Prevent rage-keygen from overwriting existing key files

Closes str4d/rage#433.

.github/workflows/interop.yml

@@ -192,6 +192,15 @@ jobs:
           name: ${{ matrix.alice }}_${{ matrix.bob }}_${{ matrix.recipient }}_test4.age
           path: test4.age
 
+      - name: Keygen prevents overwriting an existing file
+        run: |
+          touch do_not_overwrite_key.txt
+          if $(${{ matrix.alice }}-keygen -o do_not_overwrite_key.txt); then
+              false
+          else
+              true
+          fi
+
       - name: Update FiloSottile/age status with result
         if: always() && github.event.action == 'age-interop-request'
         run: |

age/CHANGELOG.md

@@ -10,12 +10,18 @@ to 1.0.0 are beta releases.
 
 ## [Unreleased]
 ### Added
+- `age::cli_common::file_io`:
+  - `impl Debug for {LazyFile, OutputFormat, OutputWriter, StdoutWriter}`
 - `impl Eq for age::ssh::{ParseRecipientKeyError, UnsupportedKey}`
 - `impl {Debug, PartialEq, Eq, Hash} for age::x25519::Recipient`
 
 ### Changed
 - MSRV is now 1.65.0.
 - Migrated to `base64 0.21`, `rsa 0.9`.
+- `age::cli_common::file_io::OutputWriter::new` now takes an `allow_overwrite`
+  boolean argument. If `OutputWriter` will write to a file, this boolean enables
+  the caller to control whether the file will be overwritten if it exists
+  (instead of the implicit behaviour that was previously changed in 0.6.0).
 - `age::ssh`:
   - `ParseRecipientKeyError` has a new variant `RsaModulusTooLarge`.
   - The following trait implementations now return

age/i18n/en-US/age.ftl

@@ -34,6 +34,8 @@ rec-detected-binary = Force with '{-output-stdout}'.
 err-deny-binary-output = refusing to output binary to the terminal.
 rec-deny-binary-output = Did you mean to use {-flag-armor}? {rec-detected-binary}
 
+err-deny-overwrite-file = refusing to overwrite existing file '{$filename}'.
+
 ## Errors
 
 err-decryption-failed = Decryption failed

age/src/cli_common/file_io.rs

@@ -3,6 +3,7 @@
 use std::fmt;
 use std::fs::{File, OpenOptions};
 use std::io::{self, Read, Write};
+use std::path::Path;
 
 #[cfg(unix)]
 use std::os::unix::fs::OpenOptionsExt;
@@ -38,6 +39,17 @@ impl fmt::Display for DetectedBinaryOutputError {
 
 impl std::error::Error for DetectedBinaryOutputError {}
 
+#[derive(Debug)]
+struct DenyOverwriteFileError(String);
+
+impl fmt::Display for DenyOverwriteFileError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        wfl!(f, "err-deny-overwrite-file", filename = self.0.as_str())
+    }
+}
+
+impl std::error::Error for DenyOverwriteFileError {}
+
 /// Wrapper around either a file or standard input.
 pub enum InputReader {
     /// Wrapper around a file.
@@ -76,6 +88,7 @@ impl Read for InputReader {
 }
 
 /// A stdout write that optionally buffers the entire output before writing.
+#[derive(Debug)]
 enum StdoutBuffer {
     Direct(io::Stdout),
     Buffered(Vec<u8>),
@@ -135,6 +148,7 @@ impl Drop for StdoutBuffer {
 }
 
 /// The data format being written out.
+#[derive(Debug)]
 pub enum OutputFormat {
     /// Binary data that should not be sent to a TTY by default.
     Binary,
@@ -145,6 +159,7 @@ pub enum OutputFormat {
 }
 
 /// Writer that wraps standard output to handle TTYs nicely.
+#[derive(Debug)]
 pub struct StdoutWriter {
     inner: StdoutBuffer,
     count: usize,
@@ -243,8 +258,10 @@ impl Write for StdoutWriter {
 
 /// A lazy [`File`] that is not opened until the first call to [`Write::write`] or
 /// [`Write::flush`].
+#[derive(Debug)]
 pub struct LazyFile {
     filename: String,
+    allow_overwrite: bool,
     #[cfg(unix)]
     mode: u32,
     file: Option<io::Result<File>>,
@@ -256,7 +273,15 @@ impl LazyFile {
 
         if self.file.is_none() {
             let mut options = OpenOptions::new();
-            options.write(true).create(true).truncate(true);
+            options.write(true);
+            if self.allow_overwrite {
+                options.create(true).truncate(true);
+            } else {
+                // In addition to the check in `OutputWriter::new`, we enforce this at
+                // file opening time to avoid a race condition with the file being
+                // separately created between `OutputWriter` construction and usage.
+                options.create_new(true);
+            }
 
             #[cfg(unix)]
             options.mode(self.mode);
@@ -283,6 +308,7 @@ impl io::Write for LazyFile {
 }
 
 /// Wrapper around either a file or standard output.
+#[derive(Debug)]
 pub enum OutputWriter {
     /// Wrapper around a file.
     File(LazyFile),
@@ -291,9 +317,16 @@ pub enum OutputWriter {
 }
 
 impl OutputWriter {
-    /// Writes output to the given filename, or standard output if `None` or `Some("-")`.
+    /// Constructs a new `OutputWriter`.
+    ///
+    /// Writes to the file at path `output`, or standard output if `output` is `None` or
+    /// `Some("-")`.
+    ///
+    /// If `allow_overwrite` is `true`, the file at path `output` will be overwritten if
+    /// it exists. This option has no effect if `output` is `None` or `Some("-")`.
     pub fn new(
         output: Option<String>,
+        allow_overwrite: bool,
         mut format: OutputFormat,
         _mode: u32,
         input_is_tty: bool,
@@ -303,8 +336,19 @@ impl OutputWriter {
             // Respect the Unix convention that "-" as an output filename
             // parameter is an explicit request to use standard output.
             if filename != "-" {
+                // We open the file lazily, but as we don't want the caller to assume
+                // this, we eagerly confirm that the file does not exist if we can't
+                // overwrite it.
+                if !allow_overwrite && Path::new(&filename).exists() {
+                    return Err(io::Error::new(
+                        io::ErrorKind::AlreadyExists,
+                        DenyOverwriteFileError(filename),
+                    ));
+                }
+
                 return Ok(OutputWriter::File(LazyFile {
                     filename,
+                    allow_overwrite,
                     #[cfg(unix)]
                     mode: _mode,
                     file: None,
@@ -362,9 +406,10 @@ pub(crate) mod tests {
 
     #[cfg(unix)]
     #[test]
-    fn lazy_existing_file() {
+    fn lazy_existing_file_allow_overwrite() {
         OutputWriter::new(
             Some("/dev/null".to_string()),
+            true,
             OutputFormat::Text,
             0o600,
             false,
@@ -373,4 +418,20 @@ pub(crate) mod tests {
         .flush()
         .unwrap();
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn lazy_existing_file_forbid_overwrite() {
+        use std::io;
+
+        let e = OutputWriter::new(
+            Some("/dev/null".to_string()),
+            false,
+            OutputFormat::Text,
+            0o600,
+            false,
+        )
+        .unwrap_err();
+        assert_eq!(e.kind(), io::ErrorKind::AlreadyExists);
+    }
 }

rage/CHANGELOG.md

@@ -15,6 +15,11 @@ to 1.0.0 are beta releases.
 ### Fixed
 - OpenSSH private keys passed to `-i/--identity` that contain invalid public
   keys are no longer ignored when encrypting, and instead cause an error.
+- `rage-keygen` no longer overwrites existing key files with the `-o/--output`
+  flag. This was its behaviour prior to 0.6.0, but was unintentionally changed
+  when `rage` was modified to overwrite existing files. Key file overwriting can
+  still be achieved by omitting `-o/--output` and instead piping stdout to the
+  file.
 - `rage-keygen` now prints fatal errors directly instead of them being hidden
   behind the `RUST_LOG=error` environment variable. It also now sets its return
   code appropriately instead of always returning 0.

rage/src/bin/rage-keygen/main.rs

@@ -63,9 +63,14 @@ fn main() -> Result<(), error::Error> {
         println!("rage-keygen {}", env!("CARGO_PKG_VERSION"));
         Ok(())
     } else {
-        let mut output =
-            file_io::OutputWriter::new(opts.output, file_io::OutputFormat::Text, 0o600, false)
-                .map_err(error::Error::FailedToOpenOutput)?;
+        let mut output = file_io::OutputWriter::new(
+            opts.output,
+            false,
+            file_io::OutputFormat::Text,
+            0o600,
+            false,
+        )
+        .map_err(error::Error::FailedToOpenOutput)?;
 
         let sk = age::x25519::Identity::generate();
         let pk = sk.to_public();

rage/src/bin/rage/main.rs

@@ -298,7 +298,8 @@ fn set_up_io(
     let input = file_io::InputReader::new(input)?;
 
     // Create an output to the user-requested location.
-    let output = file_io::OutputWriter::new(output, output_format, 0o666, input.is_terminal())?;
+    let output =
+        file_io::OutputWriter::new(output, true, output_format, 0o666, input.is_terminal())?;
 
     Ok((input, output))
 }