Merge pull request #440 from str4d/rage-keygen-fixes

`rage-keygen` fixes

.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,14 @@ 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.
 
 ## [0.9.2] - 2023-06-12
 ### Changed

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

@@ -0,0 +1,40 @@
+use std::fmt;
+use std::io;
+
+macro_rules! wlnfl {
+    ($f:ident, $message_id:literal) => {
+        writeln!($f, "{}", $crate::fl!($message_id))
+    };
+
+    ($f:ident, $message_id:literal, $($args:expr),* $(,)?) => {
+        writeln!($f, "{}", $crate::fl!($message_id, $($args), *))
+    };
+}
+
+pub(crate) enum Error {
+    FailedToOpenOutput(io::Error),
+    FailedToWriteOutput(io::Error),
+}
+
+// Rust only supports `fn main() -> Result<(), E: Debug>`, so we implement `Debug`
+// manually to provide the error output we want.
+impl fmt::Debug for Error {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            Error::FailedToOpenOutput(e) => {
+                wlnfl!(f, "err-failed-to-open-output", err = e.to_string())?
+            }
+            Error::FailedToWriteOutput(e) => {
+                wlnfl!(f, "err-failed-to-write-output", err = e.to_string())?
+            }
+        }
+        writeln!(f)?;
+        writeln!(f, "[ {} ]", crate::fl!("err-ux-A"))?;
+        write!(
+            f,
+            "[ {}: https://str4d.xyz/rage/report {} ]",
+            crate::fl!("err-ux-B"),
+            crate::fl!("err-ux-C")
+        )
+    }
+}

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

@@ -7,10 +7,11 @@ use i18n_embed::{
     DesktopLanguageRequester,
 };
 use lazy_static::lazy_static;
-use log::error;
 use rust_embed::RustEmbed;
 use std::io::Write;
 
+mod error;
+
 #[derive(RustEmbed)]
 #[folder = "i18n"]
 struct Localizations;
@@ -19,6 +20,7 @@ lazy_static! {
     static ref LANGUAGE_LOADER: FluentLanguageLoader = fluent_language_loader!();
 }
 
+#[macro_export]
 macro_rules! fl {
     ($message_id:literal) => {{
         i18n_embed_fl::fl!($crate::LANGUAGE_LOADER, $message_id)
@@ -41,7 +43,7 @@ struct AgeOptions {
     output: Option<String>,
 }
 
-fn main() {
+fn main() -> Result<(), error::Error> {
     env_logger::builder()
         .format_timestamp(None)
         .filter_level(log::LevelFilter::Off)
@@ -59,35 +61,36 @@ fn main() {
 
     if opts.version {
         println!("rage-keygen {}", env!("CARGO_PKG_VERSION"));
-        return;
-    }
-
-    let mut output =
-        match file_io::OutputWriter::new(opts.output, file_io::OutputFormat::Text, 0o600, false) {
-            Ok(output) => output,
-            Err(e) => {
-                error!("{}", fl!("err-failed-to-open-output", err = e.to_string()));
-                return;
+        Ok(())
+    } else {
+        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();
+
+        (|| {
+            writeln!(
+                output,
+                "# {}: {}",
+                fl!("identity-file-created"),
+                chrono::Local::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true)
+            )?;
+            writeln!(output, "# {}: {}", fl!("identity-file-pubkey"), pk)?;
+            writeln!(output, "{}", sk.to_string().expose_secret())?;
+
+            if !output.is_terminal() {
+                eprintln!("{}: {}", fl!("tty-pubkey"), pk);
             }
-        };
-
-    let sk = age::x25519::Identity::generate();
-    let pk = sk.to_public();
-
-    if let Err(e) = (|| {
-        if !output.is_terminal() {
-            eprintln!("{}: {}", fl!("tty-pubkey"), pk);
-        }
-
-        writeln!(
-            output,
-            "# {}: {}",
-            fl!("identity-file-created"),
-            chrono::Local::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true)
-        )?;
-        writeln!(output, "# {}: {}", fl!("identity-file-pubkey"), pk)?;
-        writeln!(output, "{}", sk.to_string().expose_secret())
-    })() {
-        error!("{}", fl!("err-failed-to-write-output", err = e.to_string()));
+
+            Ok(())
+        })()
+        .map_err(error::Error::FailedToWriteOutput)
     }
 }

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))
 }