From 592d1a55f0c05c1dcba58261f327066b4832a703 Mon Sep 17 00:00:00 2001 From: Daniel Mueller Date: Fri, 28 Aug 2020 18:44:45 -0700 Subject: Use anyhow for error handling This patch changes our error handling approach from the ground up: instead of having a globally used Error enum that contains variants for all possible errors, we now use anyhow's Error type. This approach is more dynamic (and not statically typed), but it allows for more fine grained error messages and overall more user-friendly error reporting. Overall it also is a net simplification. While we have one dynamic cast now, in order to be able to handle erroneous password/PIN entries correctly, that is considered a reasonable compromise. --- src/arg_util.rs | 2 +- src/args.rs | 16 +-- src/commands.rs | 377 +++++++++++++++++++++++++++++----------------------- src/main.rs | 7 +- src/pinentry.rs | 62 +++++---- src/tests/otp.rs | 14 +- src/tests/pws.rs | 18 +-- src/tests/status.rs | 9 +- 8 files changed, 265 insertions(+), 240 deletions(-) (limited to 'src') diff --git a/src/arg_util.rs b/src/arg_util.rs index 3a4c001..be361c7 100644 --- a/src/arg_util.rs +++ b/src/arg_util.rs @@ -50,7 +50,7 @@ macro_rules! Command { pub fn execute( self, ctx: &mut crate::ExecCtx<'_>, - ) -> crate::Result<()> { + ) -> anyhow::Result<()> { match self { $( $name::$var$((tr!(args, $inner)))? => $exec(ctx $(,tr!(args, $inner))?), diff --git a/src/args.rs b/src/args.rs index 56a10b4..0f548e4 100644 --- a/src/args.rs +++ b/src/args.rs @@ -134,16 +134,14 @@ pub enum ConfigOption { } impl ConfigOption { - pub fn try_from(disable: bool, value: Option, name: &'static str) -> Result { + pub fn try_from(disable: bool, value: Option, name: &'static str) -> anyhow::Result { if disable { - if value.is_some() { - Err(format!( - "--{name} and --no-{name} are mutually exclusive", - name = name - )) - } else { - Ok(ConfigOption::Disable) - } + anyhow::ensure!( + value.is_none(), + "--{name} and --no-{name} are mutually exclusive", + name = name + ); + Ok(ConfigOption::Disable) } else { match value { Some(value) => Ok(ConfigOption::Enable(value)), diff --git a/src/commands.rs b/src/commands.rs index a2b6004..c9dd629 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -17,13 +17,15 @@ // * along with this program. If not, see . * // ************************************************************************* +use std::convert::TryFrom as _; use std::fmt; use std::mem; -use std::result; use std::thread; use std::time; use std::u8; +use anyhow::Context as _; + use libc::sync; use nitrokey::ConfigureOtp; @@ -32,16 +34,8 @@ use nitrokey::GenerateOtp; use nitrokey::GetPasswordSafe; use crate::args; -use crate::error; -use crate::error::Error; use crate::pinentry; use crate::ExecCtx; -use crate::Result; - -/// Create an `error::Error` with an error message of the format `msg: err`. -fn get_error(msg: &'static str, err: nitrokey::Error) -> Error { - Error::NitrokeyError(Some(msg), err) -} /// Set `libnitrokey`'s log level based on the execution context's verbosity. fn set_log_level(ctx: &mut ExecCtx<'_>) { @@ -60,53 +54,52 @@ fn set_log_level(ctx: &mut ExecCtx<'_>) { } /// Connect to any Nitrokey device and do something with it. -fn with_device(ctx: &mut ExecCtx<'_>, op: F) -> Result<()> +fn with_device(ctx: &mut ExecCtx<'_>, op: F) -> anyhow::Result<()> where - F: FnOnce(&mut ExecCtx<'_>, nitrokey::DeviceWrapper<'_>) -> Result<()>, + F: FnOnce(&mut ExecCtx<'_>, nitrokey::DeviceWrapper<'_>) -> anyhow::Result<()>, { - let mut manager = nitrokey::take()?; + let mut manager = + nitrokey::take().context("Failed to acquire access to Nitrokey device manager")?; + set_log_level(ctx); let device = match ctx.model { - Some(model) => manager.connect_model(model.into()).map_err(|_| { - let error = format!("Nitrokey {} device not found", model.as_user_facing_str()); - Error::Error(error) + Some(model) => manager.connect_model(model.into()).with_context(|| { + anyhow::anyhow!("Nitrokey {} device not found", model.as_user_facing_str()) })?, - None => manager - .connect() - .map_err(|_| Error::from("Nitrokey device not found"))?, + None => manager.connect().context("Nitrokey device not found")?, }; op(ctx, device) } /// Connect to a Nitrokey Storage device and do something with it. -fn with_storage_device(ctx: &mut ExecCtx<'_>, op: F) -> Result<()> +fn with_storage_device(ctx: &mut ExecCtx<'_>, op: F) -> anyhow::Result<()> where - F: FnOnce(&mut ExecCtx<'_>, nitrokey::Storage<'_>) -> Result<()>, + F: FnOnce(&mut ExecCtx<'_>, nitrokey::Storage<'_>) -> anyhow::Result<()>, { - let mut manager = nitrokey::take()?; + let mut manager = + nitrokey::take().context("Failed to acquire access to Nitrokey device manager")?; + set_log_level(ctx); if let Some(model) = ctx.model { if model != args::DeviceModel::Storage { - return Err(Error::from( - "This command is only available on the Nitrokey Storage", - )); + anyhow::bail!("This command is only available on the Nitrokey Storage"); } } let device = manager .connect_storage() - .map_err(|_| Error::from("Nitrokey Storage device not found"))?; + .context("Nitrokey Storage device not found")?; op(ctx, device) } /// Connect to any Nitrokey device, retrieve a password safe handle, and /// do something with it. -fn with_password_safe(ctx: &mut ExecCtx<'_>, mut op: F) -> Result<()> +fn with_password_safe(ctx: &mut ExecCtx<'_>, mut op: F) -> anyhow::Result<()> where - F: FnMut(&mut ExecCtx<'_>, nitrokey::PasswordSafe<'_, '_>) -> Result<()>, + F: FnMut(&mut ExecCtx<'_>, nitrokey::PasswordSafe<'_, '_>) -> anyhow::Result<()>, { with_device(ctx, |ctx, mut device| { let pin_entry = pinentry::PinEntry::from(args::PinType::User, &device)?; @@ -118,7 +111,7 @@ where move |ctx, _, pin| { let pws = device .get_password_safe(pin) - .map_err(|err| ((), Error::from(err)))?; + .map_err(|err| ((), err.into()))?; op(ctx, pws).map_err(|err| ((), err)) }, @@ -136,10 +129,10 @@ fn authenticate<'mgr, D, A, F>( pin_type: args::PinType, msg: &'static str, op: F, -) -> Result +) -> anyhow::Result where D: Device<'mgr>, - F: FnMut(&mut ExecCtx<'_>, D, &str) -> result::Result, + F: FnMut(&mut ExecCtx<'_>, D, &str) -> Result, { let pin_entry = pinentry::PinEntry::from(pin_type, &device)?; @@ -147,7 +140,10 @@ where } /// Authenticate the given device with the user PIN. -fn authenticate_user<'mgr, T>(ctx: &mut ExecCtx<'_>, device: T) -> Result> +fn authenticate_user<'mgr, T>( + ctx: &mut ExecCtx<'_>, + device: T, +) -> anyhow::Result> where T: Device<'mgr>, { @@ -156,12 +152,21 @@ where device, args::PinType::User, "Could not authenticate as user", - |_ctx, device, pin| device.authenticate_user(pin), + |_ctx, device, pin| { + device.authenticate_user(pin).or_else(|(x, err)| { + Err(err) + .context("Failed to authenticate as user") + .map_err(|err| (x, err)) + }) + }, ) } /// Authenticate the given device with the admin PIN. -fn authenticate_admin<'mgr, T>(ctx: &mut ExecCtx<'_>, device: T) -> Result> +fn authenticate_admin<'mgr, T>( + ctx: &mut ExecCtx<'_>, + device: T, +) -> anyhow::Result> where T: Device<'mgr>, { @@ -170,7 +175,13 @@ where device, args::PinType::Admin, "Could not authenticate as admin", - |_ctx, device, pin| device.authenticate_admin(pin), + |_ctx, device, pin| { + device.authenticate_admin(pin).or_else(|(x, err)| { + Err(err) + .context("Failed to authenticate as admin") + .map_err(|err| (x, err)) + }) + }, ) } @@ -201,16 +212,15 @@ fn get_volume_status(status: &nitrokey::VolumeStatus) -> &'static str { /// the first try, this function will call `op` with `data`. At the /// second or third try, it will call `op` with the data returned by the /// previous call to `op`. -fn try_with_pin_and_data_with_pinentry( +fn try_with_pin_and_data_with_pinentry( ctx: &mut ExecCtx<'_>, pin_entry: &pinentry::PinEntry, - msg: &'static str, + _msg: &'static str, data: D, mut op: F, -) -> Result +) -> anyhow::Result where - F: FnMut(&mut ExecCtx<'_>, D, &str) -> result::Result, - E: error::TryInto, + F: FnMut(&mut ExecCtx<'_>, D, &str) -> Result, { let mut data = data; let mut retry = 3; @@ -219,10 +229,10 @@ where let pin = pinentry::inquire(ctx, pin_entry, pinentry::Mode::Query, error_msg)?; match op(ctx, data, &pin) { Ok(result) => return Ok(result), - Err((new_data, err)) => match err.try_into() { + Err((new_data, err)) => match err.downcast::() { Ok(err) => match err { nitrokey::Error::CommandError(nitrokey::CommandError::WrongPassword) => { - pinentry::clear(pin_entry)?; + pinentry::clear(pin_entry).context("Failed to clear cached secret")?; retry -= 1; if retry > 0 { @@ -230,27 +240,26 @@ where data = new_data; continue; } - return Err(get_error(msg, err)); + anyhow::bail!(err); } - err => return Err(get_error(msg, err)), + err => anyhow::bail!(err), }, - Err(err) => return Err(err), + Err(err) => anyhow::bail!(err), }, }; } } /// Try to execute the given function with a PIN. -fn try_with_pin_and_data( +fn try_with_pin_and_data( ctx: &mut ExecCtx<'_>, pin_entry: &pinentry::PinEntry, msg: &'static str, data: D, mut op: F, -) -> Result +) -> anyhow::Result where - F: FnMut(&mut ExecCtx<'_>, D, &str) -> result::Result, - E: Into + error::TryInto, + F: FnMut(&mut ExecCtx<'_>, D, &str) -> Result, { let pin = match pin_entry.pin_type() { // Ideally we would not clone here, but that would require us to @@ -261,13 +270,10 @@ where }; if let Some(pin) = pin { - let pin = pin.to_str().ok_or_else(|| { - Error::Error(format!( - "{}: Failed to read PIN due to invalid Unicode data", - msg - )) - })?; - op(ctx, data, &pin).map_err(|(_, err)| err.into()) + let pin = pin + .to_str() + .ok_or_else(|| anyhow::anyhow!("Failed to read PIN: Invalid Unicode data found"))?; + op(ctx, data, &pin).map_err(|(_, err)| err) } else { try_with_pin_and_data_with_pinentry(ctx, pin_entry, msg, data, op) } @@ -277,15 +283,14 @@ where /// /// This function behaves exactly as `try_with_pin_and_data`, but /// it refrains from passing any data to it. -fn try_with_pin( +fn try_with_pin( ctx: &mut ExecCtx<'_>, pin_entry: &pinentry::PinEntry, msg: &'static str, mut op: F, -) -> Result<()> +) -> anyhow::Result<()> where - F: FnMut(&str) -> result::Result<(), E>, - E: Into + error::TryInto, + F: FnMut(&str) -> anyhow::Result<()>, { try_with_pin_and_data(ctx, pin_entry, msg, (), |_ctx, data, pin| { op(pin).map_err(|err| (data, err)) @@ -293,7 +298,10 @@ where } /// Pretty print the status of a Nitrokey Storage. -fn print_storage_status(ctx: &mut ExecCtx<'_>, status: &nitrokey::StorageStatus) -> Result<()> { +fn print_storage_status( + ctx: &mut ExecCtx<'_>, + status: &nitrokey::StorageStatus, +) -> anyhow::Result<()> { println!( ctx, r#" Storage: @@ -327,10 +335,10 @@ fn print_status( ctx: &mut ExecCtx<'_>, model: &'static str, device: &nitrokey::DeviceWrapper<'_>, -) -> Result<()> { +) -> anyhow::Result<()> { let serial_number = device .get_serial_number() - .map_err(|err| get_error("Could not query the serial number", err))?; + .context("Could not query the serial number")?; println!( ctx, @@ -342,15 +350,21 @@ fn print_status( admin retry count: {arc}"#, model = model, id = serial_number, - fwv = device.get_firmware_version()?, - urc = device.get_user_retry_count()?, - arc = device.get_admin_retry_count()?, + fwv = device + .get_firmware_version() + .context("Failed to retrieve firmware version")?, + urc = device + .get_user_retry_count() + .context("Failed to retrieve user retry count")?, + arc = device + .get_admin_retry_count() + .context("Failed to retrieve admin retry count")?, )?; if let nitrokey::DeviceWrapper::Storage(device) = device { let status = device .get_storage_status() - .map_err(|err| get_error("Getting Storage status failed", err))?; + .context("Failed to retrieve storage status")?; print_storage_status(ctx, &status) } else { @@ -359,7 +373,7 @@ fn print_status( } /// Inquire the status of the nitrokey. -pub fn status(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn status(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_device(ctx, |ctx, device| { let model = match device { nitrokey::DeviceWrapper::Pro(_) => "Pro", @@ -370,15 +384,17 @@ pub fn status(ctx: &mut ExecCtx<'_>) -> Result<()> { } /// List the attached Nitrokey devices. -pub fn list(ctx: &mut ExecCtx<'_>, no_connect: bool) -> Result<()> { +pub fn list(ctx: &mut ExecCtx<'_>, no_connect: bool) -> anyhow::Result<()> { set_log_level(ctx); - let device_infos = nitrokey::list_devices()?; + let device_infos = + nitrokey::list_devices().context("Failed to list connected Nitrokey devices")?; if device_infos.is_empty() { println!(ctx, "No Nitrokey device connected")?; } else { println!(ctx, "device path\tmodel\tserial number")?; - let mut manager = nitrokey::take()?; + let mut manager = + nitrokey::take().context("Failed to acquire access to Nitrokey device manager")?; for device_info in device_infos { let model = device_info @@ -394,8 +410,13 @@ pub fn list(ctx: &mut ExecCtx<'_>, no_connect: bool) -> Result<()> { if no_connect { "N/A".to_string() } else { - let device = manager.connect_path(device_info.path.clone())?; - device.get_serial_number()?.to_string() + let device = manager + .connect_path(device_info.path.clone()) + .context("Failed to connect to Nitrokey")?; + device + .get_serial_number() + .context("Failed to retrieve device serial number")? + .to_string() } } }; @@ -408,16 +429,18 @@ pub fn list(ctx: &mut ExecCtx<'_>, no_connect: bool) -> Result<()> { } /// Perform a factory reset. -pub fn reset(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn reset(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_device(ctx, |ctx, mut device| { let pin_entry = pinentry::PinEntry::from(args::PinType::Admin, &device)?; // To force the user to enter the admin PIN before performing a // factory reset, we clear the pinentry cache for the admin PIN. - pinentry::clear(&pin_entry)?; + pinentry::clear(&pin_entry).context("Failed to clear cached secret")?; try_with_pin(ctx, &pin_entry, "Factory reset failed", |pin| { - device.factory_reset(&pin)?; + device + .factory_reset(&pin) + .context("Failed to reset to factory settings")?; // Work around for a timing issue between factory_reset and // build_aes_key, see // https://github.com/Nitrokey/nitrokey-storage-firmware/issues/80 @@ -426,13 +449,18 @@ pub fn reset(ctx: &mut ExecCtx<'_>) -> Result<()> { // build_aes_key after a factory reset on Pro devices. // https://github.com/Nitrokey/nitrokey-pro-firmware/issues/57 let _ = device.get_user_retry_count(); - device.build_aes_key(nitrokey::DEFAULT_ADMIN_PIN) + device + .build_aes_key(nitrokey::DEFAULT_ADMIN_PIN) + .context("Failed to rebuild AES key") }) }) } /// Change the configuration of the unencrypted volume. -pub fn unencrypted_set(ctx: &mut ExecCtx<'_>, mode: args::UnencryptedVolumeMode) -> Result<()> { +pub fn unencrypted_set( + ctx: &mut ExecCtx<'_>, + mode: args::UnencryptedVolumeMode, +) -> anyhow::Result<()> { with_storage_device(ctx, |ctx, mut device| { let pin_entry = pinentry::PinEntry::from(args::PinType::Admin, &device)?; let mode = match mode { @@ -448,13 +476,17 @@ pub fn unencrypted_set(ctx: &mut ExecCtx<'_>, mode: args::UnencryptedVolumeMode) ctx, &pin_entry, "Changing unencrypted volume mode failed", - |pin| device.set_unencrypted_volume_mode(&pin, mode), + |pin| { + device + .set_unencrypted_volume_mode(&pin, mode) + .context("Failed to change unencrypted volume mode") + }, ) }) } /// Open the encrypted volume on the Nitrokey. -pub fn encrypted_open(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn encrypted_open(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_storage_device(ctx, |ctx, mut device| { let pin_entry = pinentry::PinEntry::from(args::PinType::User, &device)?; @@ -463,13 +495,15 @@ pub fn encrypted_open(ctx: &mut ExecCtx<'_>) -> Result<()> { unsafe { sync() }; try_with_pin(ctx, &pin_entry, "Opening encrypted volume failed", |pin| { - device.enable_encrypted_volume(&pin) + device + .enable_encrypted_volume(&pin) + .context("Failed to open encrypted volume") }) }) } /// Close the previously opened encrypted volume. -pub fn encrypted_close(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn encrypted_close(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_storage_device(ctx, |_ctx, mut device| { // Flush all filesystem caches to disk. We are mostly interested in // making sure that the encrypted volume on the Nitrokey we are @@ -479,40 +513,41 @@ pub fn encrypted_close(ctx: &mut ExecCtx<'_>) -> Result<()> { device .disable_encrypted_volume() - .map_err(|err| get_error("Closing encrypted volume failed", err)) + .context("Failed to close encrypted volume") }) } /// Create a hidden volume. -pub fn hidden_create(ctx: &mut ExecCtx<'_>, slot: u8, start: u8, end: u8) -> Result<()> { +pub fn hidden_create(ctx: &mut ExecCtx<'_>, slot: u8, start: u8, end: u8) -> anyhow::Result<()> { with_storage_device(ctx, |ctx, mut device| { let pwd_entry = pinentry::PwdEntry::from(&device)?; let pwd = if let Some(pwd) = &ctx.password { pwd .to_str() - .ok_or_else(|| Error::from("Failed to read password: invalid Unicode data found")) + .ok_or_else(|| anyhow::anyhow!("Failed to read password: invalid unicode data found")) .map(ToOwned::to_owned) } else { - pinentry::choose(ctx, &pwd_entry) + pinentry::choose(ctx, &pwd_entry).context("Failed to select new PIN") }?; device .create_hidden_volume(slot, start, end, &pwd) - .map_err(|err| get_error("Creating hidden volume failed", err)) + .context("Failed to create hidden volume") }) } /// Open a hidden volume. -pub fn hidden_open(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn hidden_open(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_storage_device(ctx, |ctx, mut device| { let pwd_entry = pinentry::PwdEntry::from(&device)?; let pwd = if let Some(pwd) = &ctx.password { pwd .to_str() - .ok_or_else(|| Error::from("Failed to read password: invalid Unicode data found")) + .ok_or_else(|| anyhow::anyhow!("Failed to read password: Invalid Unicode data found")) .map(ToOwned::to_owned) } else { pinentry::inquire(ctx, &pwd_entry, pinentry::Mode::Query, None) + .context("Failed to inquire PIN") }?; // We may forcefully close an encrypted volume, if active, so be sure @@ -521,18 +556,18 @@ pub fn hidden_open(ctx: &mut ExecCtx<'_>) -> Result<()> { device .enable_hidden_volume(&pwd) - .map_err(|err| get_error("Opening hidden volume failed", err)) + .context("Failed to open hidden volume") }) } /// Close a previously opened hidden volume. -pub fn hidden_close(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn hidden_close(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_storage_device(ctx, |_ctx, mut device| { unsafe { sync() }; device .disable_hidden_volume() - .map_err(|err| get_error("Closing hidden volume failed", err)) + .context("Failed to close hidden volume") }) } @@ -545,11 +580,9 @@ fn format_option(option: Option) -> String { } /// Read the Nitrokey configuration. -pub fn config_get(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn config_get(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_device(ctx, |ctx, device| { - let config = device - .get_config() - .map_err(|err| get_error("Could not get configuration", err))?; + let config = device.get_config().context("Failed to get configuration")?; println!( ctx, r#"Config: @@ -567,10 +600,13 @@ pub fn config_get(ctx: &mut ExecCtx<'_>) -> Result<()> { } /// Write the Nitrokey configuration. -pub fn config_set(ctx: &mut ExecCtx<'_>, args: args::ConfigSetArgs) -> Result<()> { - let numlock = args::ConfigOption::try_from(args.no_numlock, args.numlock, "numlock")?; - let capslock = args::ConfigOption::try_from(args.no_capslock, args.capslock, "capslock")?; - let scrollock = args::ConfigOption::try_from(args.no_scrollock, args.scrollock, "scrollock")?; +pub fn config_set(ctx: &mut ExecCtx<'_>, args: args::ConfigSetArgs) -> anyhow::Result<()> { + let numlock = args::ConfigOption::try_from(args.no_numlock, args.numlock, "numlock") + .context("Failed to apply numlock configuration")?; + let capslock = args::ConfigOption::try_from(args.no_capslock, args.capslock, "capslock") + .context("Failed to apply capslock configuration")?; + let scrollock = args::ConfigOption::try_from(args.no_scrollock, args.scrollock, "scrollock") + .context("Failed to apply scrollock configuration")?; let otp_pin = if args.otp_pin { Some(true) } else if args.no_otp_pin { @@ -583,7 +619,7 @@ pub fn config_set(ctx: &mut ExecCtx<'_>, args: args::ConfigSetArgs) -> Result<() let mut device = authenticate_admin(ctx, device)?; let config = device .get_config() - .map_err(|err| get_error("Could not get configuration", err))?; + .context("Failed to get current configuration")?; let config = nitrokey::Config { numlock: numlock.or(config.numlock), capslock: capslock.or(config.capslock), @@ -592,20 +628,18 @@ pub fn config_set(ctx: &mut ExecCtx<'_>, args: args::ConfigSetArgs) -> Result<() }; device .write_config(config) - .map_err(|err| get_error("Could not set configuration", err)) + .context("Failed to set new configuration") }) } /// Lock the Nitrokey device. -pub fn lock(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn lock(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_device(ctx, |_ctx, mut device| { - device - .lock() - .map_err(|err| get_error("Could not lock the device", err)) + device.lock().context("Failed to lock the device") }) } -fn get_otp(slot: u8, algorithm: args::OtpAlgorithm, device: &mut T) -> Result +fn get_otp(slot: u8, algorithm: args::OtpAlgorithm, device: &mut T) -> anyhow::Result where T: GenerateOtp, { @@ -613,13 +647,13 @@ where args::OtpAlgorithm::Hotp => device.get_hotp_code(slot), args::OtpAlgorithm::Totp => device.get_totp_code(slot), } - .map_err(|err| get_error("Could not generate OTP", err)) + .context("Failed to generate OTP") } -fn get_unix_timestamp() -> Result { +fn get_unix_timestamp() -> anyhow::Result { time::SystemTime::now() .duration_since(time::UNIX_EPOCH) - .map_err(|_| Error::from("Current system time is before the Unix epoch")) + .context("Current system time is before the Unix epoch") .map(|duration| duration.as_secs()) } @@ -629,22 +663,22 @@ pub fn otp_get( slot: u8, algorithm: args::OtpAlgorithm, time: Option, -) -> Result<()> { +) -> anyhow::Result<()> { with_device(ctx, |ctx, mut device| { if algorithm == args::OtpAlgorithm::Totp { device .set_time( match time { Some(time) => time, - None => get_unix_timestamp()?, + None => get_unix_timestamp().context("Failed to retrieve current time")?, }, true, ) - .map_err(|err| get_error("Could not set time", err))?; + .context("Failed to set new time")?; } let config = device .get_config() - .map_err(|err| get_error("Could not get device configuration", err))?; + .context("Failed to get get current device configuration")?; let otp = if config.user_password { let mut user = authenticate_user(ctx, device)?; get_otp(slot, algorithm, &mut user) @@ -670,25 +704,23 @@ fn format_bytes(bytes: &[u8]) -> String { /// libnitrokey expects secrets as hexadecimal strings. This function transforms an ASCII string /// into a hexadecimal string or returns an error if the given string contains non-ASCII /// characters. -fn prepare_ascii_secret(secret: &str) -> Result { +fn prepare_ascii_secret(secret: &str) -> anyhow::Result { if secret.is_ascii() { Ok(format_bytes(&secret.as_bytes())) } else { - Err(Error::from( - "The given secret is not an ASCII string despite --format ascii being set", - )) + anyhow::bail!("The given secret is not an ASCII string as expected") } } /// Prepare a base32 secret string for libnitrokey. -fn prepare_base32_secret(secret: &str) -> Result { +fn prepare_base32_secret(secret: &str) -> anyhow::Result { base32::decode(base32::Alphabet::RFC4648 { padding: false }, secret) .map(|vec| format_bytes(&vec)) - .ok_or_else(|| Error::from("Could not parse base32 secret")) + .ok_or_else(|| anyhow::anyhow!("Failed to parse base32 secret")) } /// Configure a one-time password slot on the Nitrokey device. -pub fn otp_set(ctx: &mut ExecCtx<'_>, mut args: args::OtpSetArgs) -> Result<()> { +pub fn otp_set(ctx: &mut ExecCtx<'_>, mut args: args::OtpSetArgs) -> anyhow::Result<()> { let mut data = nitrokey::OtpSlotData { number: args.slot, name: mem::take(&mut args.name), @@ -721,20 +753,24 @@ pub fn otp_set(ctx: &mut ExecCtx<'_>, mut args: args::OtpSetArgs) -> Result<()> args::OtpAlgorithm::Hotp => device.write_hotp_slot(data, args.counter), args::OtpAlgorithm::Totp => device.write_totp_slot(data, args.time_window), } - .map_err(|err| get_error("Could not write OTP slot", err))?; + .context("Failed to write OTP slot")?; Ok(()) }) } /// Clear an OTP slot. -pub fn otp_clear(ctx: &mut ExecCtx<'_>, slot: u8, algorithm: args::OtpAlgorithm) -> Result<()> { +pub fn otp_clear( + ctx: &mut ExecCtx<'_>, + slot: u8, + algorithm: args::OtpAlgorithm, +) -> anyhow::Result<()> { with_device(ctx, |ctx, device| { let mut device = authenticate_admin(ctx, device)?; match algorithm { args::OtpAlgorithm::Hotp => device.erase_hotp_slot(slot), args::OtpAlgorithm::Totp => device.erase_totp_slot(slot), } - .map_err(|err| get_error("Could not clear OTP slot", err))?; + .context("Failed to clear OTP slot")?; Ok(()) }) } @@ -744,19 +780,16 @@ fn print_otp_status( algorithm: args::OtpAlgorithm, device: &nitrokey::DeviceWrapper<'_>, all: bool, -) -> Result<()> { +) -> anyhow::Result<()> { let mut slot: u8 = 0; loop { let result = match algorithm { args::OtpAlgorithm::Hotp => device.get_hotp_slot_name(slot), args::OtpAlgorithm::Totp => device.get_totp_slot_name(slot), }; - slot = match slot.checked_add(1) { - Some(slot) => slot, - None => { - return Err(Error::from("Integer overflow when iterating OTP slots")); - } - }; + slot = slot + .checked_add(1) + .ok_or_else(|| anyhow::anyhow!("Encountered integer overflow when iterating OTP slots"))?; let name = match result { Ok(name) => name, Err(nitrokey::Error::LibraryError(nitrokey::LibraryError::InvalidSlot)) => return Ok(()), @@ -767,14 +800,14 @@ fn print_otp_status( continue; } } - Err(err) => return Err(get_error("Could not check OTP slot", err)), + Err(err) => return Err(err).context("Failed to check OTP slot"), }; println!(ctx, "{}\t{}\t{}", algorithm, slot - 1, name)?; } } /// Print the status of the OTP slots. -pub fn otp_status(ctx: &mut ExecCtx<'_>, all: bool) -> Result<()> { +pub fn otp_status(ctx: &mut ExecCtx<'_>, all: bool) -> anyhow::Result<()> { with_device(ctx, |ctx, device| { println!(ctx, "alg\tslot\tname")?; print_otp_status(ctx, args::OtpAlgorithm::Hotp, &device, all)?; @@ -784,10 +817,12 @@ pub fn otp_status(ctx: &mut ExecCtx<'_>, all: bool) -> Result<()> { } /// Clear the PIN stored by various operations. -pub fn pin_clear(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn pin_clear(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_device(ctx, |_ctx, device| { - pinentry::clear(&pinentry::PinEntry::from(args::PinType::Admin, &device)?)?; - pinentry::clear(&pinentry::PinEntry::from(args::PinType::User, &device)?)?; + pinentry::clear(&pinentry::PinEntry::from(args::PinType::Admin, &device)?) + .context("Failed to clear admin PIN")?; + pinentry::clear(&pinentry::PinEntry::from(args::PinType::User, &device)?) + .context("Failed to clear user PIN")?; Ok(()) }) } @@ -796,7 +831,11 @@ pub fn pin_clear(ctx: &mut ExecCtx<'_>) -> Result<()> { /// /// If the user has set the respective environment variable for the /// given PIN type, it will be used. -fn choose_pin(ctx: &mut ExecCtx<'_>, pin_entry: &pinentry::PinEntry, new: bool) -> Result { +fn choose_pin( + ctx: &mut ExecCtx<'_>, + pin_entry: &pinentry::PinEntry, + new: bool, +) -> anyhow::Result { let new_pin = match pin_entry.pin_type() { args::PinType::Admin => { if new { @@ -817,15 +856,15 @@ fn choose_pin(ctx: &mut ExecCtx<'_>, pin_entry: &pinentry::PinEntry, new: bool) if let Some(new_pin) = new_pin { new_pin .to_str() - .ok_or_else(|| Error::from("Failed to read PIN: invalid Unicode data found")) + .ok_or_else(|| anyhow::anyhow!("Failed to read PIN: Invalid Unicode data found")) .map(ToOwned::to_owned) } else { - pinentry::choose(ctx, pin_entry) + pinentry::choose(ctx, pin_entry).context("Failed to select PIN") } } /// Change a PIN. -pub fn pin_set(ctx: &mut ExecCtx<'_>, pin_type: args::PinType) -> Result<()> { +pub fn pin_set(ctx: &mut ExecCtx<'_>, pin_type: args::PinType) -> anyhow::Result<()> { with_device(ctx, |ctx, mut device| { let pin_entry = pinentry::PinEntry::from(pin_type, &device)?; let new_pin = choose_pin(ctx, &pin_entry, true)?; @@ -835,8 +874,12 @@ pub fn pin_set(ctx: &mut ExecCtx<'_>, pin_type: args::PinType) -> Result<()> { &pin_entry, "Could not change the PIN", |current_pin| match pin_type { - args::PinType::Admin => device.change_admin_pin(¤t_pin, &new_pin), - args::PinType::User => device.change_user_pin(¤t_pin, &new_pin), + args::PinType::Admin => device + .change_admin_pin(¤t_pin, &new_pin) + .context("Failed to change admin PIN"), + args::PinType::User => device + .change_user_pin(¤t_pin, &new_pin) + .context("Failed to change user PIN"), }, )?; @@ -848,7 +891,7 @@ pub fn pin_set(ctx: &mut ExecCtx<'_>, pin_type: args::PinType) -> Result<()> { } /// Unblock and reset the user PIN. -pub fn pin_unblock(ctx: &mut ExecCtx<'_>) -> Result<()> { +pub fn pin_unblock(ctx: &mut ExecCtx<'_>) -> anyhow::Result<()> { with_device(ctx, |ctx, mut device| { let pin_entry = pinentry::PinEntry::from(args::PinType::User, &device)?; let user_pin = choose_pin(ctx, &pin_entry, false)?; @@ -858,7 +901,11 @@ pub fn pin_unblock(ctx: &mut ExecCtx<'_>) -> Result<()> { ctx, &pin_entry, "Could not unblock the user PIN", - |admin_pin| device.unlock_user_pin(&admin_pin, &user_pin), + |admin_pin| { + device + .unlock_user_pin(&admin_pin, &user_pin) + .context("Failed to unblock user PIN") + }, ) }) } @@ -866,10 +913,10 @@ pub fn pin_unblock(ctx: &mut ExecCtx<'_>) -> Result<()> { fn print_pws_data( ctx: &mut ExecCtx<'_>, description: &'static str, - result: result::Result, + result: Result, quiet: bool, -) -> Result<()> { - let value = result.map_err(|err| get_error("Could not access PWS slot", err))?; +) -> anyhow::Result<()> { + let value = result.context("Failed to access PWS slot")?; if quiet { println!(ctx, "{}", value)?; } else { @@ -878,20 +925,17 @@ fn print_pws_data( Ok(()) } -fn check_slot(pws: &nitrokey::PasswordSafe<'_, '_>, slot: u8) -> Result<()> { +fn check_slot(pws: &nitrokey::PasswordSafe<'_, '_>, slot: u8) -> anyhow::Result<()> { if slot >= nitrokey::SLOT_COUNT { - return Err(nitrokey::Error::from(nitrokey::LibraryError::InvalidSlot).into()); + anyhow::bail!("Slot {} is not valid", slot); } let status = pws .get_slot_status() - .map_err(|err| get_error("Could not read PWS slot status", err))?; + .context("Failed to read PWS slot status")?; if status[slot as usize] { Ok(()) } else { - Err(get_error( - "Could not access PWS slot", - nitrokey::CommandError::SlotNotProgrammed.into(), - )) + anyhow::bail!("Slot {} is not programmed", slot) } } @@ -903,9 +947,9 @@ pub fn pws_get( show_login: bool, show_password: bool, quiet: bool, -) -> Result<()> { +) -> anyhow::Result<()> { with_password_safe(ctx, |ctx, pws| { - check_slot(&pws, slot)?; + check_slot(&pws, slot).context("Failed to access PWS slot")?; let show_all = !show_name && !show_login && !show_password; if show_all || show_name { @@ -928,20 +972,18 @@ pub fn pws_set( name: &str, login: &str, password: &str, -) -> Result<()> { +) -> anyhow::Result<()> { with_password_safe(ctx, |_ctx, mut pws| { pws .write_slot(slot, name, login, password) - .map_err(|err| get_error("Could not write PWS slot", err)) + .context("Failed to write PWS slot") }) } /// Clear a PWS slot. -pub fn pws_clear(ctx: &mut ExecCtx<'_>, slot: u8) -> Result<()> { +pub fn pws_clear(ctx: &mut ExecCtx<'_>, slot: u8) -> anyhow::Result<()> { with_password_safe(ctx, |_ctx, mut pws| { - pws - .erase_slot(slot) - .map_err(|err| get_error("Could not clear PWS slot", err)) + pws.erase_slot(slot).context("Failed to clear PWS slot") }) } @@ -950,15 +992,12 @@ fn print_pws_slot( pws: &nitrokey::PasswordSafe<'_, '_>, slot: usize, programmed: bool, -) -> Result<()> { - if slot > u8::MAX as usize { - return Err(Error::from("Invalid PWS slot number")); - } - let slot = slot as u8; +) -> anyhow::Result<()> { + let slot = u8::try_from(slot).map_err(|_| anyhow::anyhow!("Invalid PWS slot number"))?; let name = if programmed { pws .get_slot_name(slot) - .map_err(|err| get_error("Could not read PWS slot", err))? + .context("Failed to read PWS slot name")? } else { "[not programmed]".to_string() }; @@ -967,11 +1006,11 @@ fn print_pws_slot( } /// Print the status of all PWS slots. -pub fn pws_status(ctx: &mut ExecCtx<'_>, all: bool) -> Result<()> { +pub fn pws_status(ctx: &mut ExecCtx<'_>, all: bool) -> anyhow::Result<()> { with_password_safe(ctx, |ctx, pws| { let slots = pws .get_slot_status() - .map_err(|err| get_error("Could not read PWS slot status", err))?; + .context("Failed to read PWS slot status")?; println!(ctx, "slot\tname")?; for (i, &value) in slots.iter().enumerate().filter(|(_, &value)| all || value) { print_pws_slot(ctx, &pws, i, value)?; diff --git a/src/main.rs b/src/main.rs index 27097c9..4f08d21 100644 --- a/src/main.rs +++ b/src/main.rs @@ -79,11 +79,8 @@ use std::env; use std::ffi; use std::io; use std::process; -use std::result; -use crate::error::Error; - -type Result = result::Result; +use anyhow::Result; const NITROCLI_ADMIN_PIN: &str = "NITROCLI_ADMIN_PIN"; const NITROCLI_USER_PIN: &str = "NITROCLI_USER_PIN"; @@ -202,7 +199,7 @@ fn run<'ctx, 'io: 'ctx>(ctx: &'ctx mut RunCtx<'io>, args: Vec) -> i32 { match handle_arguments(ctx, args) { Ok(()) => 0, Err(err) => { - let _ = eprintln!(ctx, "{}", err); + let _ = eprintln!(ctx, "{:?}", err); 1 } } diff --git a/src/pinentry.rs b/src/pinentry.rs index e1dec3e..510d7b0 100644 --- a/src/pinentry.rs +++ b/src/pinentry.rs @@ -19,12 +19,12 @@ use std::borrow; use std::fmt; -use std::io; use std::process; use std::str; +use anyhow::Context as _; + use crate::args; -use crate::error::Error; use crate::ExecCtx; type CowStr = borrow::Cow<'static, str>; @@ -49,12 +49,15 @@ pub struct PinEntry { } impl PinEntry { - pub fn from<'mgr, D>(pin_type: args::PinType, device: &D) -> crate::Result + pub fn from<'mgr, D>(pin_type: args::PinType, device: &D) -> anyhow::Result where D: nitrokey::Device<'mgr>, { let model = device.get_model(); - let serial = device.get_serial_number()?; + let serial = device + .get_serial_number() + .context("Failed to retrieve serial number")?; + Ok(Self { pin_type, model, @@ -122,12 +125,15 @@ pub struct PwdEntry { } impl PwdEntry { - pub fn from<'mgr, D>(device: &D) -> crate::Result + pub fn from<'mgr, D>(device: &D) -> anyhow::Result where D: nitrokey::Device<'mgr>, { let model = device.get_model(); - let serial = device.get_serial_number()?; + let serial = device + .get_serial_number() + .context("Failed to retrieve serial number")?; + Ok(Self { model, serial }) } } @@ -186,7 +192,7 @@ impl Mode { } } -fn parse_pinentry_pin(response: R) -> crate::Result +fn parse_pinentry_pin(response: R) -> anyhow::Result where R: AsRef, { @@ -208,9 +214,9 @@ where // specially. if !lines.is_empty() && lines[0].starts_with("ERR ") { let (_, error) = lines[0].split_at(4); - return Err(Error::from(error)); + anyhow::bail!("{}", error); } - Err(Error::Error(format!("Unexpected response: {}", string))) + anyhow::bail!("Unexpected response: {}", string) } /// Inquire a secret from the user. @@ -226,7 +232,7 @@ pub fn inquire( entry: &E, mode: Mode, error_msg: Option<&str>, -) -> crate::Result +) -> anyhow::Result where E: SecretEntry, { @@ -256,30 +262,28 @@ where .arg(command) .arg("/bye") .output() - .map_err(|err| match err.kind() { - io::ErrorKind::NotFound => { - io::Error::new(io::ErrorKind::NotFound, "gpg-connect-agent not found") - } - _ => err, - })?; - parse_pinentry_pin(str::from_utf8(&output.stdout)?) + .context("Failed to invoke gpg-connect-agent")?; + + let response = + str::from_utf8(&output.stdout).context("Failed to parse gpg-connect-agent output as UTF-8")?; + parse_pinentry_pin(response).context("Failed to parse pinentry secret") } -fn check(entry: &E, secret: &str) -> crate::Result<()> +fn check(entry: &E, secret: &str) -> anyhow::Result<()> where E: SecretEntry, { if secret.len() < usize::from(entry.min_len()) { - Err(Error::Error(format!( + anyhow::bail!( "The secret must be at least {} characters long", entry.min_len() - ))) + ) } else { Ok(()) } } -pub fn choose(ctx: &mut ExecCtx<'_>, entry: &E) -> crate::Result +pub fn choose(ctx: &mut ExecCtx<'_>, entry: &E) -> anyhow::Result where E: SecretEntry, { @@ -292,13 +296,13 @@ where clear(entry)?; if chosen != confirmed { - Err(Error::from("Entered secrets do not match")) + anyhow::bail!("Entered secrets do not match") } else { Ok(chosen) } } -fn parse_pinentry_response(response: R) -> crate::Result<()> +fn parse_pinentry_response(response: R) -> anyhow::Result<()> where R: AsRef, { @@ -309,11 +313,11 @@ where // We got the only valid answer we accept. return Ok(()); } - Err(Error::Error(format!("Unexpected response: {}", string))) + anyhow::bail!("Unexpected response: {}", string) } /// Clear the cached secret represented by the given entry. -pub fn clear(entry: &E) -> crate::Result<()> +pub fn clear(entry: &E) -> anyhow::Result<()> where E: SecretEntry, { @@ -322,9 +326,13 @@ where let output = process::Command::new("gpg-connect-agent") .arg(command) .arg("/bye") - .output()?; + .output() + .context("Failed to invoke gpg-connect-agent")?; + + let response = str::from_utf8(&output.stdout) + .context("Failed to parse gpg-connect-agent output as UTF-8")?; - parse_pinentry_response(str::from_utf8(&output.stdout)?) + parse_pinentry_response(response).context("Failed to parse pinentry response") } else { Ok(()) } diff --git a/src/tests/otp.rs b/src/tests/otp.rs index 7361688..65c1cb5 100644 --- a/src/tests/otp.rs +++ b/src/tests/otp.rs @@ -28,7 +28,7 @@ fn set_invalid_slot_raw(model: nitrokey::Model) { assert_ne!(rc, 0); assert_eq!(out, b""); - assert_eq!(&err[..24], b"Could not write OTP slot"); + assert_eq!(&err[..24], b"Failed to write OTP slot"); } #[test_device] @@ -37,12 +37,8 @@ fn set_invalid_slot(model: nitrokey::Model) { .handle(&["otp", "set", "100", "name", "1234", "-f", "hex"]) .unwrap_err() .to_string(); - let expected = format!( - "Could not write OTP slot: {}", - nitrokey::Error::LibraryError(nitrokey::LibraryError::InvalidSlot) - ); - assert_eq!(err, expected); + assert_eq!(err, "Failed to write OTP slot"); } #[test_device] @@ -122,10 +118,6 @@ fn clear(model: nitrokey::Model) -> crate::Result<()> { let res = ncli.handle(&["otp", "get", "3"]); let err = res.unwrap_err().to_string(); - let expected = format!( - "Could not generate OTP: {}", - nitrokey::Error::CommandError(nitrokey::CommandError::SlotNotProgrammed) - ); - assert_eq!(err, expected); + assert_eq!(err, "Failed to generate OTP"); Ok(()) } diff --git a/src/tests/pws.rs b/src/tests/pws.rs index 7f8107e..910ef92 100644 --- a/src/tests/pws.rs +++ b/src/tests/pws.rs @@ -25,12 +25,8 @@ fn set_invalid_slot(model: nitrokey::Model) { .handle(&["pws", "set", "100", "name", "login", "1234"]) .unwrap_err() .to_string(); - let expected = format!( - "Could not write PWS slot: {}", - nitrokey::Error::LibraryError(nitrokey::LibraryError::InvalidSlot) - ); - assert_eq!(err, expected); + assert_eq!(err, "Failed to write PWS slot"); } #[test_device] @@ -97,11 +93,7 @@ fn set_reset_get(model: nitrokey::Model) -> crate::Result<()> { let res = ncli.handle(&["pws", "get", "2"]); let err = res.unwrap_err().to_string(); - let expected = format!( - "Could not access PWS slot: {}", - nitrokey::Error::CommandError(nitrokey::CommandError::SlotNotProgrammed) - ); - assert_eq!(err, expected); + assert_eq!(err, "Failed to access PWS slot"); Ok(()) } @@ -113,10 +105,6 @@ fn clear(model: nitrokey::Model) -> crate::Result<()> { let res = ncli.handle(&["pws", "get", "10"]); let err = res.unwrap_err().to_string(); - let expected = format!( - "Could not access PWS slot: {}", - nitrokey::Error::CommandError(nitrokey::CommandError::SlotNotProgrammed) - ); - assert_eq!(err, expected); + assert_eq!(err, "Failed to access PWS slot"); Ok(()) } diff --git a/src/tests/status.rs b/src/tests/status.rs index e80fadc..c617a9e 100644 --- a/src/tests/status.rs +++ b/src/tests/status.rs @@ -19,15 +19,18 @@ use super::*; -// This test acts as verification that conversion of Error::Error -// variants into the proper exit code works properly. #[test_device] fn not_found_raw() { let (rc, out, err) = Nitrocli::new().run(&["status"]); assert_ne!(rc, 0); assert_eq!(out, b""); - assert_eq!(err, b"Nitrokey device not found\n"); + let expected = r#"Nitrokey device not found + +Caused by: + Communication error: Could not connect to a Nitrokey device +"#; + assert_eq!(err, expected.as_bytes()); } #[test_device] -- cgit v1.2.3