From 9c673844d5251d888d711fca24a057a784922028 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 9 Jul 2020 19:06:42 +0200 Subject: [PATCH] improved documentation, especially with regards to the extension --- README.md | 6 +++++- src/ctap/command.rs | 4 ++-- src/ctap/pin_protocol_v1.rs | 8 ++++---- src/ctap/storage.rs | 20 ++++++++++++-------- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 8e31796..c5e4bfc 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,11 @@ a few things you can personalize: user verification. This helps privacy, but can make usage less comfortable for credentials that need less protection. 6. Increase the default minimum length for PINs in `ctap/storage.rs`. - The current minimum is 4. Values from 4 to 63 are allowed. + The current minimum is 4. Values from 4 to 63 are allowed. Requiring longer + PINs can help establish trust between users and relying parties. It makes + user verification harder to break, but less convenient. + NIST recommends 6 at least digit PINs in section 5.1.9.1: + https://pages.nist.gov/800-63-3/sp800-63b.html You can add relying parties to the list of readers of the minimum PIN length. ### 3D printed enclosure diff --git a/src/ctap/command.rs b/src/ctap/command.rs index c3627f7..84ca8df 100644 --- a/src/ctap/command.rs +++ b/src/ctap/command.rs @@ -492,8 +492,8 @@ mod test { #[test] fn test_from_cbor_client_pin_parameters() { - // TODO(kaczmarczyck) inline the #cfg when the ICE is resolved: - // https://github.com/rust-lang/rust/issues/73663 + // TODO(kaczmarczyck) inline the #cfg when #128 is resolved: + // https://github.com/google/OpenSK/issues/128 #[cfg(not(feature = "with_ctap2_1"))] let cbor_value = cbor_map! { 1 => 1, diff --git a/src/ctap/pin_protocol_v1.rs b/src/ctap/pin_protocol_v1.rs index 8b186c0..db06265 100644 --- a/src/ctap/pin_protocol_v1.rs +++ b/src/ctap/pin_protocol_v1.rs @@ -392,10 +392,6 @@ impl PinProtocolV1 { if persistent_store.pin_hash().is_some() { match pin_auth { Some(pin_auth) => { - // TODO(kaczmarczyck) not mentioned, but maybe useful? - // if persistent_store.pin_retries() == 0 { - // return Err(Ctap2StatusCode::CTAP2_ERR_PIN_BLOCKED); - // } if self.consecutive_pin_mismatches >= 3 { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED); } @@ -403,6 +399,7 @@ impl PinProtocolV1 { message.extend(&[0x06, 0x08]); message.extend(&[min_pin_length as u8, 0x00, 0x00, 0x00]); // TODO(kaczmarczyck) commented code is useful for the extension + // https://github.com/google/OpenSK/issues/129 // if !cbor::write(cbor_array_vec!(min_pin_length_rp_ids), &mut message) { // return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_RESPONSE_CANNOT_WRITE_CBOR); // } @@ -417,6 +414,8 @@ impl PinProtocolV1 { return Err(Ctap2StatusCode::CTAP2_ERR_PIN_POLICY_VIOLATION); } persistent_store.set_min_pin_length(min_pin_length); + // TODO(kaczmarczyck) commented code is useful for the extension + // https://github.com/google/OpenSK/issues/129 // if let Some(min_pin_length_rp_ids) = min_pin_length_rp_ids { // persistent_store.set_min_pin_length_rp_ids(min_pin_length_rp_ids)?; // } @@ -932,6 +931,7 @@ mod test { 0xFE, 0xC9, ]; // TODO(kaczmarczyck) implement test for the min PIN length extension + // https://github.com/google/OpenSK/issues/129 let response = pin_protocol_v1.process_set_min_pin_length( &mut persistent_store, min_pin_length, diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 415defd..24456c8 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -75,6 +75,8 @@ const ATTESTATION_PRIVATE_KEY_LENGTH: usize = 32; const AAGUID_LENGTH: usize = 16; #[cfg(feature = "with_ctap2_1")] const DEFAULT_MIN_PIN_LENGTH: u8 = 4; +// TODO(kaczmarczyck) use this for the minPinLength extension +// https://github.com/google/OpenSK/issues/129 #[cfg(feature = "with_ctap2_1")] const _DEFAULT_MIN_PIN_LENGTH_RP_IDS: Vec = Vec::new(); // TODO(kaczmarczyck) Check whether this constant is necessary, or replace it accordingly. @@ -396,13 +398,12 @@ impl PersistentStore { } pub fn pin_retries(&self) -> u8 { - match self.store.find_one(&Key::PinRetries) { - None => MAX_PIN_RETRIES, - Some((_, entry)) => { + self.store + .find_one(&Key::PinRetries) + .map_or(MAX_PIN_RETRIES, |(_, entry)| { debug_assert_eq!(entry.data.len(), 1); entry.data[0] - } - } + }) } pub fn decr_pin_retries(&mut self) { @@ -446,7 +447,10 @@ impl PersistentStore { pub fn min_pin_length(&self) -> u8 { self.store .find_one(&Key::MinPinLength) - .map_or(DEFAULT_MIN_PIN_LENGTH, |(_, entry)| entry.data[0]) + .map_or(DEFAULT_MIN_PIN_LENGTH, |(_, entry)| { + debug_assert_eq!(entry.data.len(), 1); + entry.data[0] + }) } #[cfg(feature = "with_ctap2_1")] @@ -1007,7 +1011,7 @@ mod test { let mut rng = ThreadRng256 {}; let mut persistent_store = PersistentStore::new(&mut rng); - // The minimum PIN lenght is initially at the default. + // The minimum PIN length is initially at the default. assert_eq!(persistent_store.min_pin_length(), DEFAULT_MIN_PIN_LENGTH); // Changes by the setter are reflected by the getter.. @@ -1022,7 +1026,7 @@ mod test { let mut rng = ThreadRng256 {}; let mut persistent_store = PersistentStore::new(&mut rng); - // The minimum PIN lenght is initially at the default. + // The minimum PIN length RP IDs are initially at the default. assert_eq!( persistent_store._min_pin_length_rp_ids(), _DEFAULT_MIN_PIN_LENGTH_RP_IDS