From 5540ca5e76ffe5efe27d8819efb9e62066a10219 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Wed, 23 Jan 2019 03:46:09 +0000 Subject: Refactor and clean up all code This includes: - using idiomatic Rust - limiting the scope of unsafe blocks - simplifying code --- src/auth.rs | 59 ++++++++++-------------- src/config.rs | 20 ++++----- src/device.rs | 141 +++++++++++++++++++++++----------------------------------- src/otp.rs | 12 ++--- src/pws.rs | 24 +++++----- src/util.rs | 35 +++++++-------- 6 files changed, 121 insertions(+), 170 deletions(-) (limited to 'src') diff --git a/src/auth.rs b/src/auth.rs index 541de35..b97bee6 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -161,10 +161,10 @@ where }; let password_ptr = password.as_ptr(); let temp_password_ptr = temp_password.as_ptr() as *const c_char; - return match callback(password_ptr, temp_password_ptr) { + match callback(password_ptr, temp_password_ptr) { 0 => Ok(A::new(device, temp_password)), rv => Err((device, Error::from(rv))), - }; + } } fn authenticate_user_wrapper( @@ -218,22 +218,15 @@ impl Deref for User { impl GenerateOtp for User { fn get_hotp_code(&self, slot: u8) -> Result { - unsafe { - let temp_password_ptr = self.temp_password_ptr(); - return result_from_string(nitrokey_sys::NK_get_hotp_code_PIN(slot, temp_password_ptr)); - } + result_from_string(unsafe { + nitrokey_sys::NK_get_hotp_code_PIN(slot, self.temp_password_ptr()) + }) } fn get_totp_code(&self, slot: u8) -> Result { - unsafe { - return result_from_string(nitrokey_sys::NK_get_totp_code_PIN( - slot, - 0, - 0, - 0, - self.temp_password_ptr(), - )); - } + result_from_string(unsafe { + nitrokey_sys::NK_get_totp_code_PIN(slot, 0, 0, 0, self.temp_password_ptr()) + }) } } @@ -295,30 +288,23 @@ impl Admin { /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot pub fn write_config(&self, config: Config) -> Result<(), Error> { let raw_config = RawConfig::try_from(config)?; - unsafe { - get_command_result(nitrokey_sys::NK_write_config( + get_command_result(unsafe { + nitrokey_sys::NK_write_config( raw_config.numlock, raw_config.capslock, raw_config.scrollock, raw_config.user_password, false, self.temp_password_ptr(), - )) - } - } - - fn write_otp_slot(&self, data: OtpSlotData, callback: C) -> Result<(), Error> - where - C: Fn(RawOtpSlotData, *const c_char) -> c_int, - { - let raw_data = RawOtpSlotData::new(data)?; - get_command_result(callback(raw_data, self.temp_password_ptr())) + ) + }) } } impl ConfigureOtp for Admin { fn write_hotp_slot(&self, data: OtpSlotData, counter: u64) -> Result<(), Error> { - self.write_otp_slot(data, |raw_data: RawOtpSlotData, temp_password_ptr| unsafe { + let raw_data = RawOtpSlotData::new(data)?; + get_command_result(unsafe { nitrokey_sys::NK_write_hotp_slot( raw_data.number, raw_data.name.as_ptr(), @@ -328,13 +314,14 @@ impl ConfigureOtp for Admin { raw_data.use_enter, raw_data.use_token_id, raw_data.token_id.as_ptr(), - temp_password_ptr, + self.temp_password_ptr(), ) }) } fn write_totp_slot(&self, data: OtpSlotData, time_window: u16) -> Result<(), Error> { - self.write_otp_slot(data, |raw_data: RawOtpSlotData, temp_password_ptr| unsafe { + let raw_data = RawOtpSlotData::new(data)?; + get_command_result(unsafe { nitrokey_sys::NK_write_totp_slot( raw_data.number, raw_data.name.as_ptr(), @@ -344,19 +331,21 @@ impl ConfigureOtp for Admin { raw_data.use_enter, raw_data.use_token_id, raw_data.token_id.as_ptr(), - temp_password_ptr, + self.temp_password_ptr(), ) }) } fn erase_hotp_slot(&self, slot: u8) -> Result<(), Error> { - let temp_password_ptr = self.temp_password_ptr(); - unsafe { get_command_result(nitrokey_sys::NK_erase_hotp_slot(slot, temp_password_ptr)) } + get_command_result(unsafe { + nitrokey_sys::NK_erase_hotp_slot(slot, self.temp_password_ptr()) + }) } fn erase_totp_slot(&self, slot: u8) -> Result<(), Error> { - let temp_password_ptr = self.temp_password_ptr(); - unsafe { get_command_result(nitrokey_sys::NK_erase_totp_slot(slot, temp_password_ptr)) } + get_command_result(unsafe { + nitrokey_sys::NK_erase_totp_slot(slot, self.temp_password_ptr()) + }) } } diff --git a/src/config.rs b/src/config.rs index 6aa6d10..329f7a6 100644 --- a/src/config.rs +++ b/src/config.rs @@ -30,21 +30,21 @@ pub struct RawConfig { fn config_otp_slot_to_option(value: u8) -> Option { if value < 3 { - return Some(value); + Some(value) + } else { + None } - None } fn option_to_config_otp_slot(value: Option) -> Result { - match value { - Some(value) => { - if value < 3 { - Ok(value) - } else { - Err(LibraryError::InvalidSlot.into()) - } + if let Some(value) = value { + if value < 3 { + Ok(value) + } else { + Err(LibraryError::InvalidSlot.into()) } - None => Ok(255), + } else { + Ok(255) } } diff --git a/src/device.rs b/src/device.rs index 40d6ba4..287268b 100644 --- a/src/device.rs +++ b/src/device.rs @@ -22,14 +22,10 @@ pub enum Model { impl fmt::Display for Model { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "{}", - match *self { - Model::Pro => "Pro", - Model::Storage => "Storage", - } - ) + f.write_str(match *self { + Model::Pro => "Pro", + Model::Storage => "Storage", + }) } } @@ -44,10 +40,10 @@ pub enum VolumeMode { impl fmt::Display for VolumeMode { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self { - VolumeMode::ReadOnly => f.write_str("read-only"), - VolumeMode::ReadWrite => f.write_str("read-write"), - } + f.write_str(match *self { + VolumeMode::ReadOnly => "read-only", + VolumeMode::ReadWrite => "read-write", + }) } } @@ -330,7 +326,7 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { /// # } /// ``` fn get_serial_number(&self) -> Result { - unsafe { result_from_string(nitrokey_sys::NK_device_serial_number()) } + result_from_string(unsafe { nitrokey_sys::NK_device_serial_number() }) } /// Returns the number of remaining authentication attempts for the user. The total number of @@ -435,16 +431,14 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { /// # } /// ``` fn get_config(&self) -> Result { - unsafe { - let config_ptr = nitrokey_sys::NK_read_config(); - if config_ptr.is_null() { - return Err(get_last_error()); - } - let config_array_ptr = config_ptr as *const [u8; 5]; - let raw_config = RawConfig::from(*config_array_ptr); - libc::free(config_ptr as *mut libc::c_void); - return Ok(raw_config.into()); + let config_ptr = unsafe { nitrokey_sys::NK_read_config() }; + if config_ptr.is_null() { + return Err(get_last_error()); } + let config_array_ptr = config_ptr as *const [u8; 5]; + let raw_config = unsafe { RawConfig::from(*config_array_ptr) }; + unsafe { libc::free(config_ptr as *mut libc::c_void) }; + Ok(raw_config.into()) } /// Changes the administrator PIN. @@ -475,12 +469,9 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { fn change_admin_pin(&self, current: &str, new: &str) -> Result<(), Error> { let current_string = get_cstring(current)?; let new_string = get_cstring(new)?; - unsafe { - get_command_result(nitrokey_sys::NK_change_admin_PIN( - current_string.as_ptr(), - new_string.as_ptr(), - )) - } + get_command_result(unsafe { + nitrokey_sys::NK_change_admin_PIN(current_string.as_ptr(), new_string.as_ptr()) + }) } /// Changes the user PIN. @@ -511,12 +502,9 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { fn change_user_pin(&self, current: &str, new: &str) -> Result<(), Error> { let current_string = get_cstring(current)?; let new_string = get_cstring(new)?; - unsafe { - get_command_result(nitrokey_sys::NK_change_user_PIN( - current_string.as_ptr(), - new_string.as_ptr(), - )) - } + get_command_result(unsafe { + nitrokey_sys::NK_change_user_PIN(current_string.as_ptr(), new_string.as_ptr()) + }) } /// Unlocks the user PIN after three failed login attempts and sets it to the given value. @@ -547,12 +535,12 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { fn unlock_user_pin(&self, admin_pin: &str, user_pin: &str) -> Result<(), Error> { let admin_pin_string = get_cstring(admin_pin)?; let user_pin_string = get_cstring(user_pin)?; - unsafe { - get_command_result(nitrokey_sys::NK_unlock_user_password( + get_command_result(unsafe { + nitrokey_sys::NK_unlock_user_password( admin_pin_string.as_ptr(), user_pin_string.as_ptr(), - )) - } + ) + }) } /// Locks the Nitrokey device. @@ -576,7 +564,7 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { /// # } /// ``` fn lock(&self) -> Result<(), Error> { - unsafe { get_command_result(nitrokey_sys::NK_lock_device()) } + get_command_result(unsafe { nitrokey_sys::NK_lock_device() }) } /// Performs a factory reset on the Nitrokey device. @@ -610,7 +598,7 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { /// [`build_aes_key`]: #method.build_aes_key fn factory_reset(&self, admin_pin: &str) -> Result<(), Error> { let admin_pin_string = get_cstring(admin_pin)?; - unsafe { get_command_result(nitrokey_sys::NK_factory_reset(admin_pin_string.as_ptr())) } + get_command_result(unsafe { nitrokey_sys::NK_factory_reset(admin_pin_string.as_ptr()) }) } /// Builds a new AES key on the Nitrokey. @@ -644,7 +632,7 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { /// [`factory_reset`]: #method.factory_reset fn build_aes_key(&self, admin_pin: &str) -> Result<(), Error> { let admin_pin_string = get_cstring(admin_pin)?; - unsafe { get_command_result(nitrokey_sys::NK_build_aes_key(admin_pin_string.as_ptr())) } + get_command_result(unsafe { nitrokey_sys::NK_build_aes_key(admin_pin_string.as_ptr()) }) } } @@ -670,14 +658,13 @@ pub trait Device: Authenticate + GetPasswordSafe + GenerateOtp + fmt::Debug { /// /// [`NotConnected`]: enum.CommunicationError.html#variant.NotConnected pub fn connect() -> Result { - unsafe { - match nitrokey_sys::NK_login_auto() { - 1 => match get_connected_device() { - Some(wrapper) => Ok(wrapper), - None => Err(CommunicationError::NotConnected.into()), - }, - _ => Err(CommunicationError::NotConnected.into()), + if unsafe { nitrokey_sys::NK_login_auto() } == 1 { + match get_connected_device() { + Some(wrapper) => Ok(wrapper), + None => Err(CommunicationError::NotConnected.into()), } + } else { + Err(CommunicationError::NotConnected.into()) } } @@ -711,12 +698,10 @@ pub fn connect_model(model: Model) -> Result { } fn get_connected_model() -> Option { - unsafe { - match nitrokey_sys::NK_get_device_model() { - nitrokey_sys::NK_device_model_NK_PRO => Some(Model::Pro), - nitrokey_sys::NK_device_model_NK_STORAGE => Some(Model::Storage), - _ => None, - } + match unsafe { nitrokey_sys::NK_get_device_model() } { + nitrokey_sys::NK_device_model_NK_PRO => Some(Model::Pro), + nitrokey_sys::NK_device_model_NK_STORAGE => Some(Model::Storage), + _ => None, } } @@ -889,12 +874,9 @@ impl Storage { pub fn change_update_pin(&self, current: &str, new: &str) -> Result<(), Error> { let current_string = get_cstring(current)?; let new_string = get_cstring(new)?; - unsafe { - get_command_result(nitrokey_sys::NK_change_update_password( - current_string.as_ptr(), - new_string.as_ptr(), - )) - } + get_command_result(unsafe { + nitrokey_sys::NK_change_update_password(current_string.as_ptr(), new_string.as_ptr()) + }) } /// Enables the firmware update mode. @@ -928,11 +910,9 @@ impl Storage { /// [`WrongPassword`]: enum.CommandError.html#variant.WrongPassword pub fn enable_firmware_update(&self, update_pin: &str) -> Result<(), Error> { let update_pin_string = get_cstring(update_pin)?; - unsafe { - get_command_result(nitrokey_sys::NK_enable_firmware_update( - update_pin_string.as_ptr(), - )) - } + get_command_result(unsafe { + nitrokey_sys::NK_enable_firmware_update(update_pin_string.as_ptr()) + }) } /// Enables the encrypted storage volume. @@ -964,7 +944,7 @@ impl Storage { /// [`WrongPassword`]: enum.CommandError.html#variant.WrongPassword pub fn enable_encrypted_volume(&self, user_pin: &str) -> Result<(), Error> { let user_pin = get_cstring(user_pin)?; - unsafe { get_command_result(nitrokey_sys::NK_unlock_encrypted_volume(user_pin.as_ptr())) } + get_command_result(unsafe { nitrokey_sys::NK_unlock_encrypted_volume(user_pin.as_ptr()) }) } /// Disables the encrypted storage volume. @@ -998,7 +978,7 @@ impl Storage { /// # } /// ``` pub fn disable_encrypted_volume(&self) -> Result<(), Error> { - unsafe { get_command_result(nitrokey_sys::NK_lock_encrypted_volume()) } + get_command_result(unsafe { nitrokey_sys::NK_lock_encrypted_volume() }) } /// Enables a hidden storage volume. @@ -1041,11 +1021,9 @@ impl Storage { /// [`InvalidString`]: enum.LibraryError.html#variant.InvalidString pub fn enable_hidden_volume(&self, volume_password: &str) -> Result<(), Error> { let volume_password = get_cstring(volume_password)?; - unsafe { - get_command_result(nitrokey_sys::NK_unlock_hidden_volume( - volume_password.as_ptr(), - )) - } + get_command_result(unsafe { + nitrokey_sys::NK_unlock_hidden_volume(volume_password.as_ptr()) + }) } /// Disables a hidden storage volume. @@ -1080,7 +1058,7 @@ impl Storage { /// # } /// ``` pub fn disable_hidden_volume(&self) -> Result<(), Error> { - unsafe { get_command_result(nitrokey_sys::NK_lock_hidden_volume()) } + get_command_result(unsafe { nitrokey_sys::NK_lock_hidden_volume() }) } /// Creates a hidden volume. @@ -1125,14 +1103,9 @@ impl Storage { password: &str, ) -> Result<(), Error> { let password = get_cstring(password)?; - unsafe { - get_command_result(nitrokey_sys::NK_create_hidden_volume( - slot, - start, - end, - password.as_ptr(), - )) - } + get_command_result(unsafe { + nitrokey_sys::NK_create_hidden_volume(slot, start, end, password.as_ptr()) + }) } /// Sets the access mode of the unencrypted volume. @@ -1221,8 +1194,7 @@ impl Storage { stick_initialized: false, }; let raw_result = unsafe { nitrokey_sys::NK_get_status_storage(&mut raw_status) }; - let result = get_command_result(raw_result); - result.and(Ok(StorageStatus::from(raw_status))) + get_command_result(raw_result).map(|_| StorageStatus::from(raw_status)) } /// Returns the production information for the connected storage device. @@ -1263,8 +1235,7 @@ impl Storage { SD_Card_Manufacturer_u8: 0, }; let raw_result = unsafe { nitrokey_sys::NK_get_storage_production_info(&mut raw_data) }; - let result = get_command_result(raw_result); - result.and(Ok(StorageProductionInfo::from(raw_data))) + get_command_result(raw_result).map(|_| StorageProductionInfo::from(raw_data)) } /// Clears the warning for a new SD card. diff --git a/src/otp.rs b/src/otp.rs index 7535a77..430b127 100644 --- a/src/otp.rs +++ b/src/otp.rs @@ -219,7 +219,7 @@ pub trait GenerateOtp { /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed fn get_hotp_slot_name(&self, slot: u8) -> Result { - unsafe { result_from_string(nitrokey_sys::NK_get_hotp_slot_name(slot)) } + result_from_string(unsafe { nitrokey_sys::NK_get_hotp_slot_name(slot) }) } /// Returns the name of the given TOTP slot. @@ -248,7 +248,7 @@ pub trait GenerateOtp { /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed fn get_totp_slot_name(&self, slot: u8) -> Result { - unsafe { result_from_string(nitrokey_sys::NK_get_totp_slot_name(slot)) } + result_from_string(unsafe { nitrokey_sys::NK_get_totp_slot_name(slot) }) } /// Generates an HOTP code on the given slot. This operation may require user authorization, @@ -279,9 +279,7 @@ pub trait GenerateOtp { /// [`NotAuthorized`]: enum.CommandError.html#variant.NotAuthorized /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed fn get_hotp_code(&self, slot: u8) -> Result { - unsafe { - return result_from_string(nitrokey_sys::NK_get_hotp_code(slot)); - } + result_from_string(unsafe { nitrokey_sys::NK_get_hotp_code(slot) }) } /// Generates a TOTP code on the given slot. This operation may require user authorization, @@ -324,9 +322,7 @@ pub trait GenerateOtp { /// [`NotAuthorized`]: enum.CommandError.html#variant.NotAuthorized /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed fn get_totp_code(&self, slot: u8) -> Result { - unsafe { - return result_from_string(nitrokey_sys::NK_get_totp_code(slot, 0, 0, 0)); - } + result_from_string(unsafe { nitrokey_sys::NK_get_totp_code(slot, 0, 0, 0) }) } } diff --git a/src/pws.rs b/src/pws.rs index a21527c..c89b73f 100644 --- a/src/pws.rs +++ b/src/pws.rs @@ -121,12 +121,8 @@ fn get_password_safe<'a>( user_pin: &str, ) -> Result, Error> { let user_pin_string = get_cstring(user_pin)?; - let result = unsafe { - get_command_result(nitrokey_sys::NK_enable_password_safe( - user_pin_string.as_ptr(), - )) - }; - result.map(|()| PasswordSafe { _device: device }) + get_command_result(unsafe { nitrokey_sys::NK_enable_password_safe(user_pin_string.as_ptr()) }) + .map(|_| PasswordSafe { _device: device }) } fn get_pws_result(s: String) -> Result { @@ -211,7 +207,7 @@ impl<'a> PasswordSafe<'a> { /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed pub fn get_slot_name(&self, slot: u8) -> Result { - unsafe { result_from_string(nitrokey_sys::NK_get_password_safe_slot_name(slot)) } + result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_name(slot) }) .and_then(get_pws_result) } @@ -244,7 +240,7 @@ impl<'a> PasswordSafe<'a> { /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed pub fn get_slot_login(&self, slot: u8) -> Result { - unsafe { result_from_string(nitrokey_sys::NK_get_password_safe_slot_login(slot)) } + result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_login(slot) }) .and_then(get_pws_result) } @@ -277,7 +273,7 @@ impl<'a> PasswordSafe<'a> { /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot /// [`SlotNotProgrammed`]: enum.CommandError.html#variant.SlotNotProgrammed pub fn get_slot_password(&self, slot: u8) -> Result { - unsafe { result_from_string(nitrokey_sys::NK_get_password_safe_slot_password(slot)) } + result_from_string(unsafe { nitrokey_sys::NK_get_password_safe_slot_password(slot) }) .and_then(get_pws_result) } @@ -317,14 +313,14 @@ impl<'a> PasswordSafe<'a> { let name_string = get_cstring(name)?; let login_string = get_cstring(login)?; let password_string = get_cstring(password)?; - unsafe { - get_command_result(nitrokey_sys::NK_write_password_safe_slot( + get_command_result(unsafe { + nitrokey_sys::NK_write_password_safe_slot( slot, name_string.as_ptr(), login_string.as_ptr(), password_string.as_ptr(), - )) - } + ) + }) } /// Erases the given slot. Erasing clears the stored name, login and password (if the slot was @@ -353,7 +349,7 @@ impl<'a> PasswordSafe<'a> { /// /// [`InvalidSlot`]: enum.LibraryError.html#variant.InvalidSlot pub fn erase_slot(&self, slot: u8) -> Result<(), Error> { - unsafe { get_command_result(nitrokey_sys::NK_erase_password_safe_slot(slot)) } + get_command_result(unsafe { nitrokey_sys::NK_erase_password_safe_slot(slot) }) } } diff --git a/src/util.rs b/src/util.rs index 2542a7b..f8ad9c9 100644 --- a/src/util.rs +++ b/src/util.rs @@ -29,32 +29,31 @@ pub enum LogLevel { } pub fn owned_str_from_ptr(ptr: *const c_char) -> String { - unsafe { - return CStr::from_ptr(ptr).to_string_lossy().into_owned(); - } + unsafe { CStr::from_ptr(ptr) } + .to_string_lossy() + .into_owned() } pub fn result_from_string(ptr: *const c_char) -> Result { if ptr.is_null() { return Err(Error::UnexpectedError); } - unsafe { - let s = owned_str_from_ptr(ptr); - free(ptr as *mut c_void); - // An empty string can both indicate an error or be a valid return value. In this case, we - // have to check the last command status to decide what to return. - if s.is_empty() { - get_last_result().map(|_| s) - } else { - Ok(s) - } + let s = owned_str_from_ptr(ptr); + unsafe { free(ptr as *mut c_void) }; + // An empty string can both indicate an error or be a valid return value. In this case, we + // have to check the last command status to decide what to return. + if s.is_empty() { + get_last_result().map(|_| s) + } else { + Ok(s) } } pub fn get_command_result(value: c_int) -> Result<(), Error> { - match value { - 0 => Ok(()), - other => Err(Error::from(other)), + if value == 0 { + Ok(()) + } else { + Err(Error::from(value)) } } @@ -63,10 +62,10 @@ pub fn get_last_result() -> Result<(), Error> { } pub fn get_last_error() -> Error { - return match get_last_result() { + match get_last_result() { Ok(()) => Error::UnexpectedError, Err(err) => err, - }; + } } pub fn generate_password(length: usize) -> Result, Error> { -- cgit v1.2.3