diff --git a/.github/workflows/interop.yml b/.github/workflows/interop.yml index 40f869fb..d937607a 100644 --- a/.github/workflows/interop.yml +++ b/.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: | diff --git a/age/CHANGELOG.md b/age/CHANGELOG.md index f74fe6d4..5597517d 100644 --- a/age/CHANGELOG.md +++ b/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 diff --git a/age/i18n/en-US/age.ftl b/age/i18n/en-US/age.ftl index 383c2323..36399742 100644 --- a/age/i18n/en-US/age.ftl +++ b/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 diff --git a/age/src/cli_common/file_io.rs b/age/src/cli_common/file_io.rs index bf1b111e..7e5a2651 100644 --- a/age/src/cli_common/file_io.rs +++ b/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), @@ -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>, @@ -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, + 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); + } } diff --git a/rage/CHANGELOG.md b/rage/CHANGELOG.md index 828c9c51..1fc70fc9 100644 --- a/rage/CHANGELOG.md +++ b/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 diff --git a/rage/src/bin/rage-keygen/error.rs b/rage/src/bin/rage-keygen/error.rs new file mode 100644 index 00000000..0e038bb3 --- /dev/null +++ b/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") + ) + } +} diff --git a/rage/src/bin/rage-keygen/main.rs b/rage/src/bin/rage-keygen/main.rs index 94f6d688..a00d7559 100644 --- a/rage/src/bin/rage-keygen/main.rs +++ b/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, } -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) } } diff --git a/rage/src/bin/rage/main.rs b/rage/src/bin/rage/main.rs index 3d5ff0ac..922cd95d 100644 --- a/rage/src/bin/rage/main.rs +++ b/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)) }