improved documentation, especially with regards to the extension

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

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:
+        // TODO(kaczmarczyck) inline the #cfg when #128 is resolved:
-        // https://github.com/rust-lang/rust/issues/73663
+        // https://github.com/google/OpenSK/issues/128
         #[cfg(not(feature = "with_ctap2_1"))]
         let cbor_value = cbor_map! {
             1 => 1,

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,

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<String> = 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) {
+        self.store
-            None => MAX_PIN_RETRIES,
+            .find_one(&Key::PinRetries)
-            Some((_, entry)) => {
+            .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