diff options
| -rw-r--r-- | CHANGELOG.md | 8 | ||||
| -rw-r--r-- | src/device/mod.rs | 108 | ||||
| -rw-r--r-- | src/device/storage.rs | 10 | ||||
| -rw-r--r-- | src/error.rs | 6 | ||||
| -rw-r--r-- | src/util.rs | 11 | ||||
| -rw-r--r-- | tests/device.rs | 13 | ||||
| -rw-r--r-- | tests/pws.rs | 2 |
7 files changed, 90 insertions, 68 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index b75904c..f5bb7cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,14 @@ Copyright (C) 2019-2020 Robin Krahl <robin.krahl@ireas.org> 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<String, + _>` to `Result<u32, _>`. + - Change the type of the field `DeviceInfo.serial_number` from + `Option<String>` to `Option<u32>`. + # v0.5.2 (2020-01-28) - Use `CString` to store the temporary password instead of `Vec<u8>`. - Regenerate temporary passwords if they would contain a null byte. 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<Model>, /// 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<String>, + /// The serial number of the device, or `None` if the device does not expose its serial number. + pub serial_number: Option<u32>, } 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: AsRef<str>>(s: S) -> Result<u32, Error> { + 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<String> { +fn get_hidapi_serial_number(serial_number: &str) -> Option<u32> { 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<Status, Error>; - /// 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<String, Error> { + fn get_serial_number(&self) -> Result<u32, Error> { 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/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(), + )), } } diff --git a/src/error.rs b/src/error.rs index f9af594..7bea3f2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -25,7 +25,7 @@ pub enum Error { /// An error that occurred during random number generation. RandError(Box<dyn error::Error>), /// An error that is caused by an unexpected value returned by libnitrokey. - UnexpectedError, + UnexpectedError(String), /// An unknown error returned by libnitrokey. UnknownError(i64), /// An error caused by a Nitrokey model that is not supported by this crate. @@ -102,7 +102,7 @@ impl error::Error for Error { Error::LibraryError(ref err) => Some(err), Error::PoisonError(ref err) => Some(err), Error::RandError(ref err) => Some(err.as_ref()), - Error::UnexpectedError => None, + Error::UnexpectedError(_) => None, Error::UnknownError(_) => None, Error::UnsupportedModelError => None, Error::Utf8Error(ref err) => Some(err), @@ -119,7 +119,7 @@ impl fmt::Display for Error { Error::LibraryError(ref err) => write!(f, "Library error: {}", err), Error::PoisonError(_) => write!(f, "Internal error: poisoned lock"), Error::RandError(ref err) => write!(f, "RNG error: {}", err), - Error::UnexpectedError => write!(f, "An unexpected error occurred"), + Error::UnexpectedError(ref s) => write!(f, "An unexpected error occurred: {}", s), Error::UnknownError(ref err) => write!(f, "Unknown error: {}", err), Error::UnsupportedModelError => write!(f, "Unsupported Nitrokey model"), Error::Utf8Error(ref err) => write!(f, "UTF-8 error: {}", err), diff --git a/src/util.rs b/src/util.rs index a0d0d1b..08946d6 100644 --- a/src/util.rs +++ b/src/util.rs @@ -39,7 +39,9 @@ pub fn owned_str_from_ptr(ptr: *const c_char) -> Result<String, Error> { pub fn result_from_string(ptr: *const c_char) -> Result<String, Error> { if ptr.is_null() { - return Err(Error::UnexpectedError); + return Err(Error::UnexpectedError( + "libnitrokey returned a null pointer".to_owned(), + )); } let s = owned_str_from_ptr(ptr)?; unsafe { free(ptr as *mut c_void) }; @@ -69,10 +71,9 @@ pub fn get_last_result() -> Result<(), Error> { } pub fn get_last_error() -> Error { - match get_last_result() { - Ok(()) => Error::UnexpectedError, - Err(err) => err, - } + get_last_result().err().unwrap_or_else(|| { + Error::UnexpectedError("Expected an error, but command status is zero".to_owned()) + }) } pub fn generate_password(length: usize) -> Result<CString, Error> { 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] diff --git a/tests/pws.rs b/tests/pws.rs index 7169695..47e9703 100644 --- a/tests/pws.rs +++ b/tests/pws.rs @@ -16,7 +16,7 @@ use nitrokey_test::test as test_device; fn get_slot_name_direct(slot: u8) -> Result<String, Error> { let ptr = unsafe { nitrokey_sys::NK_get_password_safe_slot_name(slot) }; if ptr.is_null() { - return Err(Error::UnexpectedError); + return Err(Error::UnexpectedError("null pointer".to_owned())); } let s = unsafe { CStr::from_ptr(ptr).to_string_lossy().into_owned() }; unsafe { free(ptr as *mut c_void) }; |