From da8727996efacec4280696caefee3feecea4eae7 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Tue, 28 Jan 2020 10:07:23 +0100 Subject: Add String value to the Error::UnexpectedError variant To make debugging of unexpected errors easier, this patch adds an associated String value with a description of the unexpected behavior to the UnexpectedError variant of the Error enum. --- src/device/storage.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'src/device') diff --git a/src/device/storage.rs b/src/device/storage.rs index deb2844..1e2c46d 100644 --- a/src/device/storage.rs +++ b/src/device/storage.rs @@ -678,7 +678,7 @@ impl<'a> Storage<'a> { if usage_data.write_level_min > usage_data.write_level_max || usage_data.write_level_max > 100 { - Err(Error::UnexpectedError) + Err(Error::UnexpectedError("Invalid write levels".to_owned())) } else { Ok(ops::Range { start: usage_data.write_level_min, @@ -708,10 +708,14 @@ impl<'a> Storage<'a> { match status { 0..=100 => u8::try_from(status) .map(OperationStatus::Ongoing) - .map_err(|_| Error::UnexpectedError), + .map_err(|_| { + Error::UnexpectedError("Cannot create u8 from operation status".to_owned()) + }), -1 => Ok(OperationStatus::Idle), -2 => Err(get_last_error()), - _ => Err(Error::UnexpectedError), + _ => Err(Error::UnexpectedError( + "Invalid operation status".to_owned(), + )), } } -- cgit v1.2.3 From 24eebcdaaa32d55bf49d069d8320be5dbd6fdab9 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Sun, 26 Jan 2020 14:26:01 +0100 Subject: Always store serial numbers as integers This patch consistently uses u32 integers to store and return the serial number of a Nitrokey device. This makes it easier to convert and compare the serial number, as it is a unique representation and as formatting an integer cannot fail. For more details, see this RFC: https://lists.sr.ht/~ireas/nitrokey-rs-dev/%3C20200126074816.GA1314%40ireas.org%3E --- CHANGELOG.md | 5 +++ src/device/mod.rs | 108 +++++++++++++++++++++++++++++++----------------------- tests/device.rs | 13 ++----- 3 files changed, 70 insertions(+), 56 deletions(-) (limited to 'src/device') diff --git a/CHANGELOG.md b/CHANGELOG.md index da9b2f3..4c0f6b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ SPDX-License-Identifier: CC0-1.0 # Unreleased - Add `String` value to the `Error::UnexpectedError` variant. +- Always store serial numbers as integers: + - Change the return type of `Device::get_serial_number` from `Result` to `Result`. + - Change the type of the field `DeviceInfo.serial_number` from + `Option` to `Option`. # v0.5.1 (2020-01-15) - Fix serial number formatting for Nitrokey Pro devices with firmware 0.8 or diff --git a/src/device/mod.rs b/src/device/mod.rs index 0234bf0..a25ad1b 100644 --- a/src/device/mod.rs +++ b/src/device/mod.rs @@ -78,9 +78,8 @@ pub struct DeviceInfo { pub model: Option, /// The USB device path. pub path: String, - /// The serial number as a 8-character hex string, or `None` if the device does not expose its - /// serial number. - pub serial_number: Option, + /// The serial number of the device, or `None` if the device does not expose its serial number. + pub serial_number: Option, } impl TryFrom<&nitrokey_sys::NK_device_info> for DeviceInfo { @@ -110,45 +109,48 @@ impl fmt::Display for DeviceInfo { None => write!(f, "Unsupported Nitrokey model")?, } write!(f, " at {} with ", self.path)?; - match &self.serial_number { - Some(ref serial_number) => write!(f, "serial no. {}", serial_number), + match self.serial_number { + Some(serial_number) => write!(f, "serial no. 0x{:08x}", serial_number), None => write!(f, "an unknown serial number"), } } } -/// Parses a serial number returned by hidapi and transforms it to the Nitrokey format. +/// Parses the given hex string and returns its integer representation. +fn parse_serial_number>(s: S) -> Result { + u32::from_str_radix(s.as_ref(), 16) + .map_err(|_| Error::UnexpectedError("Could not parse hex string".to_owned())) +} + +/// Parses a serial number returned by hidapi and returns its integer value. /// /// If the serial number is all zero, this function returns `None`. Otherwise, it uses the last -/// eight characters. If these are all zero, the first eight characters are used instead. This -/// function also makes sure that the returned string is lowercase, consistent with libnitrokey’s -/// hex string formatting. +/// eight characters. If these are all zero, the first eight characters are used instead. The +/// selected substring is parse as a hex string and its integer value is returned from the +/// function. If the string cannot be parsed, this function returns `None`. /// /// The reason for this behavior is that the Nitrokey Storage does not report its serial number at /// all (all zero value), while the Nitrokey Pro with firmware 0.9 or later writes its serial /// number to the last eight characters. Nitrokey Pro devices with firmware 0.8 or earlier wrote /// their serial number to the first eight characters. -fn get_hidapi_serial_number(serial_number: &str) -> Option { +fn get_hidapi_serial_number(serial_number: &str) -> Option { let len = serial_number.len(); if len < 8 { - // The serial number in the USB descriptor has 12 bytes, we need at least four of them + // The serial number in the USB descriptor has 12 bytes, we need at least four return None; } let iter = serial_number.char_indices().rev(); let first_non_null = iter.skip_while(|(_, c)| *c == '0').next(); if let Some((i, _)) = first_non_null { - if len - i < 8 { + let substr = if len - i < 8 { // The last eight characters contain at least one non-zero character --> use them - let mut serial_number = serial_number.split_at(len - 8).1.to_string(); - serial_number.make_ascii_lowercase(); - Some(serial_number) + serial_number.split_at(len - 8).1 } else { // The last eight characters are all zero --> use the first eight - let mut serial_number = serial_number.split_at(8).0.to_string(); - serial_number.make_ascii_lowercase(); - Some(serial_number) - } + serial_number.split_at(8).0 + }; + parse_serial_number(substr).ok() } else { // The serial number is all zero None @@ -263,7 +265,7 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// let device = manager.connect()?; /// let status = device.get_status()?; /// println!("Firmware version: {}", status.firmware_version); - /// println!("Serial number: {:x}", status.serial_number); + /// println!("Serial number: 0x{:08x}", status.serial_number); /// # Ok::<(), nitrokey::Error>(()) /// ``` /// @@ -273,8 +275,9 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// [`StorageStatus`]: struct.StorageStatus.html fn get_status(&self) -> Result; - /// Returns the serial number of the Nitrokey device. The serial number is the string - /// representation of a hex number. + /// Returns the serial number of the Nitrokey device. + /// + /// For display purpuses, the serial number should be formatted as an 8-digit hex string. /// /// # Example /// @@ -286,14 +289,15 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// let mut manager = nitrokey::take()?; /// let device = manager.connect()?; /// match device.get_serial_number() { - /// Ok(number) => println!("serial no: {}", number), + /// Ok(number) => println!("serial no: 0x{:08x}", number), /// Err(err) => eprintln!("Could not get serial number: {}", err), /// }; /// # Ok(()) /// # } /// ``` - fn get_serial_number(&self) -> Result { + fn get_serial_number(&self) -> Result { result_from_string(unsafe { nitrokey_sys::NK_device_serial_number() }) + .and_then(parse_serial_number) } /// Returns the number of remaining authentication attempts for the user. The total number of @@ -625,43 +629,55 @@ pub(crate) fn connect_enum(model: Model) -> bool { #[cfg(test)] mod tests { - use super::get_hidapi_serial_number; + use super::{get_hidapi_serial_number, parse_serial_number}; + + #[test] + fn test_parse_serial_number() { + fn assert_err(s: &str) { + match parse_serial_number(s).unwrap_err() { + super::Error::UnexpectedError(_) => {} + err => assert!(false, "expected UnexpectedError, got {} (input {})", err, s), + } + } + + assert_eq!(0x1234, parse_serial_number("1234").unwrap()); + assert_eq!(0x1234, parse_serial_number("01234").unwrap()); + assert_eq!(0x1234, parse_serial_number("001234").unwrap()); + assert_eq!(0x1234, parse_serial_number("0001234").unwrap()); + + assert_eq!(0, parse_serial_number("0").unwrap()); + + assert_eq!(0xdeadbeef, parse_serial_number("deadbeef").unwrap()); + assert_err("0xdeadbeef"); + assert_err("deadpork"); + assert_err("blubb"); + assert_err(""); + } #[test] - fn hidapi_serial_number() { + fn test_get_hidapi_serial_number() { assert_eq!(None, get_hidapi_serial_number("")); assert_eq!(None, get_hidapi_serial_number("00000000000000000")); + assert_eq!(None, get_hidapi_serial_number("blubb")); assert_eq!(None, get_hidapi_serial_number("1234")); + assert_eq!(Some(0x1234), get_hidapi_serial_number("00001234")); + assert_eq!(Some(0x1234), get_hidapi_serial_number("000000001234")); + assert_eq!(Some(0x1234), get_hidapi_serial_number("100000001234")); + assert_eq!(Some(0x12340000), get_hidapi_serial_number("123400000000")); assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("00001234") - ); - assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("000000001234") - ); - assert_eq!( - Some("00001234".to_string()), - get_hidapi_serial_number("100000001234") - ); - assert_eq!( - Some("12340000".to_string()), - get_hidapi_serial_number("123400000000") - ); - assert_eq!( - Some("00005678".to_string()), + Some(0x5678), get_hidapi_serial_number("000000000000000000005678") ); assert_eq!( - Some("00001234".to_string()), + Some(0x1234), get_hidapi_serial_number("000012340000000000000000") ); assert_eq!( - Some("0000ffff".to_string()), + Some(0xffff), get_hidapi_serial_number("00000000000000000000FFFF") ); assert_eq!( - Some("0000ffff".to_string()), + Some(0xffff), get_hidapi_serial_number("00000000000000000000ffff") ); } diff --git a/tests/device.rs b/tests/device.rs index a88c956..2989941 100644 --- a/tests/device.rs +++ b/tests/device.rs @@ -47,8 +47,7 @@ fn list_devices(_device: DeviceWrapper) { nitrokey::Model::Pro => { assert!(device.serial_number.is_some()); let serial_number = device.serial_number.unwrap(); - assert!(!serial_number.is_empty()); - assert_valid_serial_number(&serial_number); + assert!(serial_number != 0); } nitrokey::Model::Storage => { assert_eq!(None, device.serial_number); @@ -157,20 +156,14 @@ fn disconnect(device: DeviceWrapper) { fn get_status(device: DeviceWrapper) { let status = unwrap_ok!(device.get_status()); assert_ok!(status.firmware_version, device.get_firmware_version()); - let serial_number = format!("{:08x}", status.serial_number); - assert_ok!(serial_number, device.get_serial_number()); + assert_ok!(status.serial_number, device.get_serial_number()); assert_ok!(status.config, device.get_config()); } -fn assert_valid_serial_number(serial_number: &str) { - assert!(serial_number.is_ascii()); - assert!(serial_number.chars().all(|c| c.is_ascii_hexdigit())); -} - #[test_device] fn get_serial_number(device: DeviceWrapper) { let serial_number = unwrap_ok!(device.get_serial_number()); - assert_valid_serial_number(&serial_number); + assert!(serial_number != 0); } #[test_device] -- cgit v1.2.3 From 4a9dab94400cb00ae1e28485ddc64d46cf27ed3c Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Wed, 29 Jan 2020 14:04:06 +0100 Subject: Implement From<&NK_status> for RawConfig This makes it easier to parse only the config part of the NK_status struct and avoids code duplication in the upcoming get_config refactoring. --- src/config.rs | 11 +++++++++++ src/device/mod.rs | 8 +------- 2 files changed, 12 insertions(+), 7 deletions(-) (limited to 'src/device') diff --git a/src/config.rs b/src/config.rs index cb678d7..9b9de3c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -83,6 +83,17 @@ impl convert::TryFrom for RawConfig { } } +impl From<&nitrokey_sys::NK_status> for RawConfig { + fn from(status: &nitrokey_sys::NK_status) -> Self { + Self { + numlock: status.config_numlock, + capslock: status.config_capslock, + scrollock: status.config_scrolllock, + user_password: status.otp_user_password, + } + } +} + impl From<[u8; 5]> for RawConfig { fn from(data: [u8; 5]) -> Self { RawConfig { diff --git a/src/device/mod.rs b/src/device/mod.rs index a25ad1b..403bb1f 100644 --- a/src/device/mod.rs +++ b/src/device/mod.rs @@ -191,13 +191,7 @@ impl From for Status { minor: status.firmware_version_minor, }, serial_number: status.serial_number_smart_card, - config: RawConfig { - numlock: status.config_numlock, - capslock: status.config_capslock, - scrollock: status.config_scrolllock, - user_password: status.otp_user_password, - } - .into(), + config: RawConfig::from(&status).into(), } } } -- cgit v1.2.3 From c1f48ce6c614586042db8891d2eebf19d2212ce4 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Wed, 29 Jan 2020 13:52:19 +0100 Subject: Use NK_get_status to implement Device::get_config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libnitrokey’s NK_read_config function returns a pointer to an array that has been allocated using new[]. We would have to delete this pointer using delete[], but we only have access to free. Therefore this patch modifies the Device::get_config function to call NK_get_status instead of NK_read_config. This also makes the code more safe as we get the data as a struct instead of an array. It does not add much overhead as NK_read_config also executes the GET_STATUS command on the Nitrokey device. --- CHANGELOG.md | 2 ++ src/config.rs | 11 ----------- src/device/mod.rs | 23 ++++++++++++----------- 3 files changed, 14 insertions(+), 22 deletions(-) (limited to 'src/device') diff --git a/CHANGELOG.md b/CHANGELOG.md index f5bb7cb..c94e170 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ SPDX-License-Identifier: CC0-1.0 _>` to `Result`. - Change the type of the field `DeviceInfo.serial_number` from `Option` to `Option`. +- Use `NK_get_status` instead of `NK_read_config` to implement the + `Device::get_config` function. # v0.5.2 (2020-01-28) - Use `CString` to store the temporary password instead of `Vec`. diff --git a/src/config.rs b/src/config.rs index 9b9de3c..120a51b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -94,17 +94,6 @@ impl From<&nitrokey_sys::NK_status> for RawConfig { } } -impl From<[u8; 5]> for RawConfig { - fn from(data: [u8; 5]) -> Self { - RawConfig { - numlock: data[0], - capslock: data[1], - scrollock: data[2], - user_password: data[3] != 0, - } - } -} - impl Into for RawConfig { fn into(self) -> Config { Config { diff --git a/src/device/mod.rs b/src/device/mod.rs index 403bb1f..e9ca0dc 100644 --- a/src/device/mod.rs +++ b/src/device/mod.rs @@ -9,7 +9,6 @@ use std::convert::{TryFrom, TryInto}; use std::ffi; use std::fmt; -use libc; use nitrokey_sys; use crate::auth::Authenticate; @@ -18,8 +17,7 @@ use crate::error::{CommunicationError, Error}; use crate::otp::GenerateOtp; use crate::pws::GetPasswordSafe; use crate::util::{ - get_command_result, get_cstring, get_last_error, owned_str_from_ptr, result_from_string, - result_or_error, + get_command_result, get_cstring, owned_str_from_ptr, result_from_string, result_or_error, }; pub use pro::Pro; @@ -386,14 +384,17 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// # } /// ``` fn get_config(&self) -> Result { - 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()) + let mut raw_status = nitrokey_sys::NK_status { + firmware_version_major: 0, + firmware_version_minor: 0, + serial_number_smart_card: 0, + config_numlock: 0, + config_capslock: 0, + config_scrolllock: 0, + otp_user_password: false, + }; + get_command_result(unsafe { nitrokey_sys::NK_get_status(&mut raw_status) })?; + Ok(RawConfig::from(&raw_status).into()) } /// Changes the administrator PIN. -- cgit v1.2.3 From 4fb865af37093d9b0ee039d8ae48fb2a820f3760 Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Tue, 28 Jan 2020 17:38:47 +0100 Subject: Represent serial numbers using SerialNumber struct In a previous commit, we changed the serial number representation from a string to an integer. This made it easier to compare serial numbers, but also introduced new problems: - Serial numbers should be formatted consistently, for example as "{:#010x}". It is hard to ensure this for an integer value. - The format of the serial number may be subject to change. Users should not rely too much on the u32 representation. Therefore we introduce a new SerialNumber struct that represents a serial number. Currently it only stores a u32 value. The following traits and functions can be used to access its value: - FromStr for string parsing - ToString/Display for string formatting - as_u32 to access the underlying integer value --- CHANGELOG.md | 7 +- examples/list-devices.rs | 2 +- src/device/mod.rs | 216 +++++++++++++++++++++++++++++++++++------------ src/device/storage.rs | 4 +- src/lib.rs | 4 +- tests/device.rs | 6 +- 6 files changed, 174 insertions(+), 65 deletions(-) (limited to 'src/device') diff --git a/CHANGELOG.md b/CHANGELOG.md index c94e170..9e675f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,12 @@ SPDX-License-Identifier: CC0-1.0 # Unreleased - Add `String` value to the `Error::UnexpectedError` variant. -- Always store serial numbers as integers: +- Always store serial numbers as structs: + - Introduce the `SerialNumber` struct. - Change the return type of `Device::get_serial_number` from `Result` to `Result`. + _>` to `Result`. - Change the type of the field `DeviceInfo.serial_number` from - `Option` to `Option`. + `Option` to `Option`. - Use `NK_get_status` instead of `NK_read_config` to implement the `Device::get_config` function. diff --git a/examples/list-devices.rs b/examples/list-devices.rs index 47fa054..0066f8c 100644 --- a/examples/list-devices.rs +++ b/examples/list-devices.rs @@ -17,7 +17,7 @@ fn main() -> Result<(), nitrokey::Error> { let model = device.get_model(); let status = device.get_status()?; println!( - "{}\t{}\t{}\t\t\t{:08x}", + "{}\t{}\t{}\t\t\t{}", device_info.path, model, status.firmware_version, status.serial_number ); } diff --git a/src/device/mod.rs b/src/device/mod.rs index e9ca0dc..83ab90d 100644 --- a/src/device/mod.rs +++ b/src/device/mod.rs @@ -8,12 +8,13 @@ mod wrapper; use std::convert::{TryFrom, TryInto}; use std::ffi; use std::fmt; +use std::str; use nitrokey_sys; use crate::auth::Authenticate; use crate::config::{Config, RawConfig}; -use crate::error::{CommunicationError, Error}; +use crate::error::{CommunicationError, Error, LibraryError}; use crate::otp::GenerateOtp; use crate::pws::GetPasswordSafe; use crate::util::{ @@ -69,6 +70,98 @@ impl TryFrom for Model { } } +/// Serial number of a Nitrokey device. +/// +/// The serial number can be formatted as a string using the [`ToString`][] trait, and it can be +/// parsed from a string using the [`FromStr`][] trait. It can also be represented as a 32-bit +/// unsigned integer using [`as_u32`][]. This integer is the ID of the smartcard of the Nitrokey +/// device. +/// +/// Neither the format of the string representation nor the integer representation are guaranteed +/// to stay the same for new firmware versions. +/// +/// [`as_u32`]: #method.as_u32 +/// [`FromStr`]: #impl-FromStr +/// [`ToString`]: #impl-ToString +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct SerialNumber { + value: u32, +} + +impl SerialNumber { + /// Creates an emtpty serial number. + /// + /// This function can be used to create a placeholder value or to compare a `SerialNumber` + /// instance with an empty serial number. + pub fn empty() -> Self { + SerialNumber::new(0) + } + + fn new(value: u32) -> Self { + SerialNumber { value } + } + + /// Returns the integer reprensentation of this serial number. + /// + /// This integer currently is the ID of the smartcard of the Nitrokey device. Upcoming + /// firmware versions might change the meaning of this representation, or add additional + /// components to the serial number. + // To provide a stable API even if the internal representation of SerialNumber changes, we want + // to borrow SerialNumber instead of copying it even if it might be less efficient. + #[allow(clippy::trivially_copy_pass_by_ref)] + pub fn as_u32(&self) -> u32 { + self.value + } +} + +impl fmt::Display for SerialNumber { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:#010x}", self.value) + } +} + +impl str::FromStr for SerialNumber { + type Err = Error; + + /// Try to parse a serial number from a hex string. + /// + /// The input string must be a valid hex string. Optionally, it can include a `0x` prefix. + /// + /// # Errors + /// + /// - [`InvalidHexString`][] if the given string is not a valid hex string + /// + /// # Example + /// + /// ```no_run + /// use std::convert::TryFrom; + /// use nitrokey::{DeviceInfo, Error, SerialNumber}; + /// + /// fn find_device(serial_number: &str) -> Result, Error> { + /// let serial_number: SerialNumber = serial_number.parse()?; + /// Ok(nitrokey::list_devices()? + /// .into_iter() + /// .filter(|device| device.serial_number == Some(serial_number)) + /// .next()) + /// } + /// + /// ``` + /// + /// [`InvalidHexString`]: enum.LibraryError.html#variant.InvalidHexString + fn from_str(s: &str) -> Result { + // ignore leading 0x + let hex_string = if s.starts_with("0x") { + s.split_at(2).1 + } else { + s + }; + + u32::from_str_radix(hex_string, 16) + .map(SerialNumber::new) + .map_err(|_| LibraryError::InvalidHexString.into()) + } +} + /// Connection information for a Nitrokey device. #[derive(Clone, Debug, PartialEq)] pub struct DeviceInfo { @@ -77,7 +170,7 @@ pub struct DeviceInfo { /// The USB device path. pub path: String, /// The serial number of the device, or `None` if the device does not expose its serial number. - pub serial_number: Option, + pub serial_number: Option, } impl TryFrom<&nitrokey_sys::NK_device_info> for DeviceInfo { @@ -108,19 +201,13 @@ impl fmt::Display for DeviceInfo { } write!(f, " at {} with ", self.path)?; match self.serial_number { - Some(serial_number) => write!(f, "serial no. 0x{:08x}", serial_number), + Some(serial_number) => write!(f, "serial no. {}", serial_number), None => write!(f, "an unknown serial number"), } } } -/// Parses the given hex string and returns its integer representation. -fn parse_serial_number>(s: S) -> Result { - u32::from_str_radix(s.as_ref(), 16) - .map_err(|_| Error::UnexpectedError("Could not parse hex string".to_owned())) -} - -/// Parses a serial number returned by hidapi and returns its integer value. +/// Parses a serial number returned by hidapi. /// /// If the serial number is all zero, this function returns `None`. Otherwise, it uses the last /// eight characters. If these are all zero, the first eight characters are used instead. The @@ -131,7 +218,7 @@ fn parse_serial_number>(s: S) -> Result { /// all (all zero value), while the Nitrokey Pro with firmware 0.9 or later writes its serial /// number to the last eight characters. Nitrokey Pro devices with firmware 0.8 or earlier wrote /// their serial number to the first eight characters. -fn get_hidapi_serial_number(serial_number: &str) -> Option { +fn get_hidapi_serial_number(serial_number: &str) -> Option { let len = serial_number.len(); if len < 8 { // The serial number in the USB descriptor has 12 bytes, we need at least four @@ -148,7 +235,7 @@ fn get_hidapi_serial_number(serial_number: &str) -> Option { // The last eight characters are all zero --> use the first eight serial_number.split_at(8).0 }; - parse_serial_number(substr).ok() + substr.parse().ok() } else { // The serial number is all zero None @@ -176,7 +263,7 @@ pub struct Status { /// The firmware version of the device. pub firmware_version: FirmwareVersion, /// The serial number of the device. - pub serial_number: u32, + pub serial_number: SerialNumber, /// The configuration of the device. pub config: Config, } @@ -188,7 +275,7 @@ impl From for Status { major: status.firmware_version_major, minor: status.firmware_version_minor, }, - serial_number: status.serial_number_smart_card, + serial_number: SerialNumber::new(status.serial_number_smart_card), config: RawConfig::from(&status).into(), } } @@ -257,7 +344,7 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// let device = manager.connect()?; /// let status = device.get_status()?; /// println!("Firmware version: {}", status.firmware_version); - /// println!("Serial number: 0x{:08x}", status.serial_number); + /// println!("Serial number: {}", status.serial_number); /// # Ok::<(), nitrokey::Error>(()) /// ``` /// @@ -281,15 +368,15 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// let mut manager = nitrokey::take()?; /// let device = manager.connect()?; /// match device.get_serial_number() { - /// Ok(number) => println!("serial no: 0x{:08x}", number), + /// Ok(number) => println!("serial no: {}", number), /// Err(err) => eprintln!("Could not get serial number: {}", err), /// }; /// # Ok(()) /// # } /// ``` - fn get_serial_number(&self) -> Result { + fn get_serial_number(&self) -> Result { result_from_string(unsafe { nitrokey_sys::NK_device_serial_number() }) - .and_then(parse_serial_number) + .and_then(|s| s.parse()) } /// Returns the number of remaining authentication attempts for the user. The total number of @@ -624,26 +711,50 @@ pub(crate) fn connect_enum(model: Model) -> bool { #[cfg(test)] mod tests { - use super::{get_hidapi_serial_number, parse_serial_number}; + use std::str::FromStr; + + use super::{get_hidapi_serial_number, LibraryError, SerialNumber}; + + #[test] + fn test_serial_number_display() { + fn assert_str(s: &str, n: u32) { + assert_eq!(s.to_owned(), SerialNumber::new(n).to_string()); + } + + assert_str("0x00000000", 0); + assert_str("0x00001000", 0x1000); + assert_str("0x12345678", 0x12345678); + } #[test] - fn test_parse_serial_number() { + fn test_serial_number_try_from() { + fn assert_ok(v: u32, s: &str) { + assert_eq!(SerialNumber::new(v), SerialNumber::from_str(s).unwrap()); + assert_eq!( + SerialNumber::new(v), + SerialNumber::from_str(format!("0x{}", s).as_ref()).unwrap() + ); + } + fn assert_err(s: &str) { - match parse_serial_number(s).unwrap_err() { - super::Error::UnexpectedError(_) => {} - err => assert!(false, "expected UnexpectedError, got {} (input {})", err, s), + match SerialNumber::from_str(s).unwrap_err() { + super::Error::LibraryError(LibraryError::InvalidHexString) => {} + err => assert!( + false, + "expected InvalidHexString error, got {} (input {})", + err, s + ), } } - assert_eq!(0x1234, parse_serial_number("1234").unwrap()); - assert_eq!(0x1234, parse_serial_number("01234").unwrap()); - assert_eq!(0x1234, parse_serial_number("001234").unwrap()); - assert_eq!(0x1234, parse_serial_number("0001234").unwrap()); + assert_ok(0x1234, "1234"); + assert_ok(0x1234, "01234"); + assert_ok(0x1234, "001234"); + assert_ok(0x1234, "0001234"); - assert_eq!(0, parse_serial_number("0").unwrap()); + assert_ok(0, "0"); + assert_ok(0xdeadbeef, "deadbeef"); - assert_eq!(0xdeadbeef, parse_serial_number("deadbeef").unwrap()); - assert_err("0xdeadbeef"); assert_err("deadpork"); assert_err("blubb"); assert_err(""); @@ -651,29 +762,26 @@ mod tests { #[test] fn test_get_hidapi_serial_number() { - assert_eq!(None, get_hidapi_serial_number("")); - assert_eq!(None, get_hidapi_serial_number("00000000000000000")); - assert_eq!(None, get_hidapi_serial_number("blubb")); - assert_eq!(None, get_hidapi_serial_number("1234")); - assert_eq!(Some(0x1234), get_hidapi_serial_number("00001234")); - assert_eq!(Some(0x1234), get_hidapi_serial_number("000000001234")); - assert_eq!(Some(0x1234), get_hidapi_serial_number("100000001234")); - assert_eq!(Some(0x12340000), get_hidapi_serial_number("123400000000")); - assert_eq!( - Some(0x5678), - get_hidapi_serial_number("000000000000000000005678") - ); - assert_eq!( - Some(0x1234), - get_hidapi_serial_number("000012340000000000000000") - ); - assert_eq!( - Some(0xffff), - get_hidapi_serial_number("00000000000000000000FFFF") - ); - assert_eq!( - Some(0xffff), - get_hidapi_serial_number("00000000000000000000ffff") - ); + fn assert_none(s: &str) { + assert_eq!(None, get_hidapi_serial_number(s)); + } + + fn assert_some(n: u32, s: &str) { + assert_eq!(Some(SerialNumber::new(n)), get_hidapi_serial_number(s)); + } + + assert_none(""); + assert_none("00000000000000000"); + assert_none("blubb"); + assert_none("1234"); + + assert_some(0x1234, "00001234"); + assert_some(0x1234, "000000001234"); + assert_some(0x1234, "100000001234"); + assert_some(0x12340000, "123400000000"); + assert_some(0x5678, "000000000000000000005678"); + assert_some(0x1234, "000012340000000000000000"); + assert_some(0xffff, "00000000000000000000FFFF"); + assert_some(0xffff, "00000000000000000000ffff"); } } diff --git a/src/device/storage.rs b/src/device/storage.rs index 1e2c46d..5669a91 100644 --- a/src/device/storage.rs +++ b/src/device/storage.rs @@ -7,7 +7,7 @@ use std::ops; use nitrokey_sys; -use crate::device::{Device, FirmwareVersion, Model, Status}; +use crate::device::{Device, FirmwareVersion, Model, SerialNumber, Status}; use crate::error::{CommandError, Error}; use crate::otp::GenerateOtp; use crate::util::{get_command_result, get_cstring, get_last_error}; @@ -827,7 +827,7 @@ impl<'a> Device<'a> for Storage<'a> { let storage_status = self.get_storage_status()?; status.firmware_version = storage_status.firmware_version; - status.serial_number = storage_status.serial_number_smart_card; + status.serial_number = SerialNumber::new(storage_status.serial_number_smart_card); Ok(status) } diff --git a/src/lib.rs b/src/lib.rs index 9efad91..92247d7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -138,8 +138,8 @@ use nitrokey_sys; pub use crate::auth::{Admin, Authenticate, User}; pub use crate::config::Config; pub use crate::device::{ - Device, DeviceInfo, DeviceWrapper, Model, OperationStatus, Pro, SdCardData, Status, Storage, - StorageProductionInfo, StorageStatus, VolumeMode, VolumeStatus, + Device, DeviceInfo, DeviceWrapper, Model, OperationStatus, Pro, SdCardData, SerialNumber, + Status, Storage, StorageProductionInfo, StorageStatus, VolumeMode, VolumeStatus, }; pub use crate::error::{CommandError, CommunicationError, Error, LibraryError}; pub use crate::otp::{ConfigureOtp, GenerateOtp, OtpMode, OtpSlotData}; diff --git a/tests/device.rs b/tests/device.rs index 2989941..1669d9d 100644 --- a/tests/device.rs +++ b/tests/device.rs @@ -10,7 +10,7 @@ use std::{thread, time}; use nitrokey::{ Authenticate, CommandError, CommunicationError, Config, ConfigureOtp, Device, DeviceInfo, Error, GenerateOtp, GetPasswordSafe, LibraryError, OperationStatus, OtpMode, OtpSlotData, - Storage, VolumeMode, DEFAULT_ADMIN_PIN, DEFAULT_USER_PIN, + SerialNumber, Storage, VolumeMode, DEFAULT_ADMIN_PIN, DEFAULT_USER_PIN, }; use nitrokey_test::test as test_device; @@ -47,7 +47,7 @@ fn list_devices(_device: DeviceWrapper) { nitrokey::Model::Pro => { assert!(device.serial_number.is_some()); let serial_number = device.serial_number.unwrap(); - assert!(serial_number != 0); + assert!(serial_number != SerialNumber::empty()); } nitrokey::Model::Storage => { assert_eq!(None, device.serial_number); @@ -163,7 +163,7 @@ fn get_status(device: DeviceWrapper) { #[test_device] fn get_serial_number(device: DeviceWrapper) { let serial_number = unwrap_ok!(device.get_serial_number()); - assert!(serial_number != 0); + assert!(serial_number != SerialNumber::empty()); } #[test_device] -- cgit v1.2.3 From d905aafe208536203dd319e5dc48bd250180a8ef Mon Sep 17 00:00:00 2001 From: Robin Krahl Date: Tue, 28 Jan 2020 17:56:53 +0100 Subject: Refactor string handling in util The util module provides helper methods to deal with the C strings returned by libnitrokey. The current implementation has to problems: - It causes unnecessary allocations if we only want to look at the string, for example in get_serial_number. - If the conversion from a CStr to a String fails, the string pointer is not freed. Therefore this patch introduces the run_with_str function that executes a function with the string returned by libnitrokey and then makes sure that the pointer is freed correctly. --- src/device/mod.rs | 7 ++++--- src/util.rs | 36 +++++++++++++++++++++++------------- 2 files changed, 27 insertions(+), 16 deletions(-) (limited to 'src/device') diff --git a/src/device/mod.rs b/src/device/mod.rs index 83ab90d..067fdf6 100644 --- a/src/device/mod.rs +++ b/src/device/mod.rs @@ -18,7 +18,7 @@ use crate::error::{CommunicationError, Error, LibraryError}; use crate::otp::GenerateOtp; use crate::pws::GetPasswordSafe; use crate::util::{ - get_command_result, get_cstring, owned_str_from_ptr, result_from_string, result_or_error, + get_command_result, get_cstring, owned_str_from_ptr, result_or_error, run_with_string, }; pub use pro::Pro; @@ -375,8 +375,9 @@ pub trait Device<'a>: Authenticate<'a> + GetPasswordSafe<'a> + GenerateOtp + fmt /// # } /// ``` fn get_serial_number(&self) -> Result { - result_from_string(unsafe { nitrokey_sys::NK_device_serial_number() }) - .and_then(|s| s.parse()) + run_with_string(unsafe { nitrokey_sys::NK_device_serial_number() }, |s| { + s.parse() + }) } /// Returns the number of remaining authentication attempts for the user. The total number of diff --git a/src/util.rs b/src/util.rs index 08946d6..b17b071 100644 --- a/src/util.rs +++ b/src/util.rs @@ -30,28 +30,38 @@ pub enum LogLevel { DebugL2, } +pub fn str_from_ptr<'a>(ptr: *const c_char) -> Result<&'a str, Error> { + unsafe { CStr::from_ptr(ptr) }.to_str().map_err(Error::from) +} + pub fn owned_str_from_ptr(ptr: *const c_char) -> Result { - unsafe { CStr::from_ptr(ptr) } - .to_str() - .map(String::from) - .map_err(Error::from) + str_from_ptr(ptr).map(ToOwned::to_owned) } -pub fn result_from_string(ptr: *const c_char) -> Result { +pub fn run_with_string(ptr: *const c_char, op: F) -> Result +where + F: FnOnce(&str) -> Result, +{ if ptr.is_null() { return Err(Error::UnexpectedError( "libnitrokey returned a null pointer".to_owned(), )); } - let s = owned_str_from_ptr(ptr)?; + let result = str_from_ptr(ptr).and_then(op); 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) - } + result +} + +pub fn result_from_string(ptr: *const c_char) -> Result { + run_with_string(ptr, |s| { + // 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.to_owned()) + } else { + Ok(s.to_owned()) + } + }) } pub fn result_or_error(value: T) -> Result { -- cgit v1.2.3