Address first round of comments.

README.md

@@ -68,7 +68,7 @@ For a more detailed guide, please refer to our
     ./deploy.py --board=nrf52840_dongle --opensk
     ```
 
-1.  Finally you need to inejct the cryptographic material if you enabled
+1.  Finally you need to inject the cryptographic material if you enabled
     batch attestation or CTAP1/U2F compatibility (which is the case by
     default):
 

docs/install.md

@@ -137,7 +137,7 @@ File              | Purpose
 If you want to use your own attestation certificate and private key, simply
 replace `opensk_cert.pem` and `opensk.key` files.
 
-Our build script `build.rs` is responsible for converting `aaguid.txt` file
+Our build script `build.rs` is responsible for converting the `aaguid.txt` file
 into raw data that is then used by the Rust file `src/ctap/key_material.rs`.
 
 Our configuration script `tools/configure.py` is responsible for configuring

src/ctap/command.rs

@@ -400,10 +400,10 @@ impl TryFrom<cbor::Value> for AuthenticatorAttestationMaterial {
                 2 => private_key,
             } = extract_map(cbor_value)?;
         }
-        let certificate = certificate.map(extract_byte_string).transpose()?.unwrap();
-        let private_key = private_key.map(extract_byte_string).transpose()?.unwrap();
+        let certificate = extract_byte_string(ok_or_missing(certificate)?)?;
+        let private_key = extract_byte_string(ok_or_missing(private_key)?)?;
         if private_key.len() != key_material::ATTESTATION_PRIVATE_KEY_LENGTH {
-            return Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR);
+            return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
         }
         let private_key = array_ref!(private_key, 0, key_material::ATTESTATION_PRIVATE_KEY_LENGTH);
         Ok(AuthenticatorAttestationMaterial {
@@ -638,4 +638,83 @@ mod test {
         let command = Command::deserialize(&cbor_bytes);
         assert_eq!(command, Ok(Command::AuthenticatorSelection));
     }
+
+    #[test]
+    fn test_vendor_configure() {
+        // Incomplete command
+        let mut cbor_bytes = vec![Command::AUTHENTICATOR_VENDOR_CONFIGURE];
+        let command = Command::deserialize(&cbor_bytes);
+        assert_eq!(command, Err(Ctap2StatusCode::CTAP2_ERR_INVALID_CBOR));
+
+        cbor_bytes.extend(&[0xA1, 0x01, 0xF5]);
+        let command = Command::deserialize(&cbor_bytes);
+        assert_eq!(
+            command,
+            Ok(Command::AuthenticatorVendorConfigure(
+                AuthenticatorVendorConfigureParameters {
+                    lockdown: true,
+                    attestation_material: None
+                }
+            ))
+        );
+
+        let dummy_cert = [0xddu8; 20];
+        let dummy_pkey = [0x41u8; key_material::ATTESTATION_PRIVATE_KEY_LENGTH];
+
+        // Attestation key is too short.
+        let cbor_value = cbor_map! {
+            1 => false,
+            2 => cbor_map! {
+                1 => dummy_cert,
+                2 => dummy_pkey[..key_material::ATTESTATION_PRIVATE_KEY_LENGTH - 1]
+            }
+        };
+        assert_eq!(
+            AuthenticatorVendorConfigureParameters::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)
+        );
+
+        // Missing private key
+        let cbor_value = cbor_map! {
+            1 => false,
+            2 => cbor_map! {
+                1 => dummy_cert
+            }
+        };
+        assert_eq!(
+            AuthenticatorVendorConfigureParameters::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)
+        );
+
+        // Missing certificate
+        let cbor_value = cbor_map! {
+            1 => false,
+            2 => cbor_map! {
+                2 => dummy_pkey
+            }
+        };
+        assert_eq!(
+            AuthenticatorVendorConfigureParameters::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)
+        );
+
+        // Valid
+        let cbor_value = cbor_map! {
+            1 => false,
+            2 => cbor_map! {
+                1 => dummy_cert,
+                2 => dummy_pkey
+            }
+        };
+        assert_eq!(
+            AuthenticatorVendorConfigureParameters::try_from(cbor_value),
+            Ok(AuthenticatorVendorConfigureParameters {
+                lockdown: false,
+                attestation_material: Some(AuthenticatorAttestationMaterial {
+                    certificate: dummy_cert.to_vec(),
+                    private_key: dummy_pkey
+                })
+            })
+        );
+    }
 }

src/ctap/mod.rs

@@ -931,22 +931,64 @@ where
         (self.check_user_presence)(cid)?;
 
         // Sanity checks
-        let has_priv_key = self.persistent_store.attestation_private_key()?.is_some();
-        let has_cert = self.persistent_store.attestation_certificate()?.is_some();
+        let current_priv_key = self.persistent_store.attestation_private_key()?;
+        let current_cert = self.persistent_store.attestation_certificate()?;
 
-        if params.attestation_material.is_some() {
+        let response = if params.attestation_material.is_some() {
             let data = params.attestation_material.unwrap();
-            if !has_cert {
-                self.persistent_store
-                    .set_attestation_certificate(&data.certificate)?;
+
+            match (current_cert, current_priv_key) {
+                (Some(_), Some(_)) => {
+                    // Device is fully programmed.
+                    // We don't compare values to avoid giving an oracle
+                    // about the private key.
+                    AuthenticatorVendorResponse {
+                        cert_programmed: true,
+                        pkey_programmed: true,
+                    }
+                }
+                // Device is not programmed.
+                (None, None) => {
+                    self.persistent_store
+                        .set_attestation_certificate(&data.certificate)?;
+                    self.persistent_store
+                        .set_attestation_private_key(&data.private_key)?;
+                    AuthenticatorVendorResponse {
+                        cert_programmed: true,
+                        pkey_programmed: true,
+                    }
+                }
+                // Device is partially programmed. Ensure the programmed value
+                // matched the given one before programming anything.
+                (Some(cert), None) => {
+                    if cert != data.certificate {
+                        return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
+                    }
+                    self.persistent_store
+                        .set_attestation_private_key(&data.private_key)?;
+                    AuthenticatorVendorResponse {
+                        cert_programmed: true,
+                        pkey_programmed: true,
+                    }
+                }
+                (None, Some(key)) => {
+                    if key != data.private_key {
+                        return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
+                    }
+                    self.persistent_store
+                        .set_attestation_certificate(&data.certificate)?;
+                    AuthenticatorVendorResponse {
+                        cert_programmed: true,
+                        pkey_programmed: true,
+                    }
+                }
             }
-            if !has_priv_key {
-                self.persistent_store
-                    .set_attestation_private_key(&data.private_key)?;
+        } else {
+            AuthenticatorVendorResponse {
+                cert_programmed: current_cert.is_some(),
+                pkey_programmed: current_priv_key.is_some(),
             }
         };
-        let has_priv_key = self.persistent_store.attestation_private_key()?.is_some();
-        let has_cert = self.persistent_store.attestation_certificate()?.is_some();
         if params.lockdown {
             // To avoid bricking the authenticator, we only allow lockdown
             // to happen if both values are programmed or if both U2F/CTAP1 and
@@ -956,28 +998,13 @@ where
             #[cfg(not(feature = "with_ctap1"))]
             let need_certificate = USE_BATCH_ATTESTATION;
 
-            if (need_certificate && !(has_priv_key && has_cert))
+            if (need_certificate && !(response.pkey_programmed && response.cert_programmed))
                 || libtock_drivers::crp::protect().is_err()
             {
-                Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)
-            } else {
-                Ok(ResponseData::AuthenticatorVendor(
-                    AuthenticatorVendorResponse {
-                        cert_programmed: has_cert,
-                        pkey_programmed: has_priv_key,
-                        lockdown_enabled: true,
-                    },
-                ))
+                return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR);
             }
-        } else {
-            Ok(ResponseData::AuthenticatorVendor(
-                AuthenticatorVendorResponse {
-                    cert_programmed: has_cert,
-                    pkey_programmed: has_priv_key,
-                    lockdown_enabled: false,
-                },
-            ))
         }
+        Ok(ResponseData::AuthenticatorVendor(response))
     }
 
     pub fn generate_auth_data(
@@ -2135,7 +2162,6 @@ mod test {
                 AuthenticatorVendorResponse {
                     cert_programmed: false,
                     pkey_programmed: false,
-                    lockdown_enabled: false
                 }
             ))
         );
@@ -2159,7 +2185,6 @@ mod test {
                 AuthenticatorVendorResponse {
                     cert_programmed: true,
                     pkey_programmed: true,
-                    lockdown_enabled: false
                 }
             ))
         );
@@ -2180,7 +2205,7 @@ mod test {
             dummy_key
         );
 
-        // Try to inject other dummy values and check that intial values are retained.
+        // Try to inject other dummy values and check that initial values are retained.
         let other_dummy_key = [0x44u8; key_material::ATTESTATION_PRIVATE_KEY_LENGTH];
         let response = ctap_state.process_vendor_configure(
             AuthenticatorVendorConfigureParameters {
@@ -2198,7 +2223,6 @@ mod test {
                 AuthenticatorVendorResponse {
                     cert_programmed: true,
                     pkey_programmed: true,
-                    lockdown_enabled: false
                 }
             ))
         );
@@ -2233,7 +2257,6 @@ mod test {
                 AuthenticatorVendorResponse {
                     cert_programmed: true,
                     pkey_programmed: true,
-                    lockdown_enabled: true
                 }
             ))
         );

src/ctap/response.rs

@@ -238,21 +238,18 @@ impl From<AuthenticatorClientPinResponse> for cbor::Value {
 pub struct AuthenticatorVendorResponse {
     pub cert_programmed: bool,
     pub pkey_programmed: bool,
-    pub lockdown_enabled: bool,
 }
 
 impl From<AuthenticatorVendorResponse> for cbor::Value {
     fn from(vendor_response: AuthenticatorVendorResponse) -> Self {
         let AuthenticatorVendorResponse {
             cert_programmed,
             pkey_programmed,
-            lockdown_enabled,
         } = vendor_response;
 
         cbor_map_options! {
             1 => cert_programmed,
             2 => pkey_programmed,
-            3 => lockdown_enabled,
         }
     }
 }

src/ctap/storage.rs

@@ -964,8 +964,7 @@ mod test {
             .unwrap()
             .is_none());
 
-        // Make sure the persistent keys are initialized.
-        // Put dummy values
+        // Make sure the persistent keys are initialized to dummy values.
         let dummy_key = [0x41u8; key_material::ATTESTATION_PRIVATE_KEY_LENGTH];
         let dummy_cert = [0xddu8; 20];
         persistent_store

tools/configure.py

@@ -75,11 +75,11 @@ def get_opensk_devices(batch_mode):
 
 
 def get_private_key(data, password=None):
-  # First we try without password
+  # First we try without password.
   try:
     return serialization.load_pem_private_key(data, password=None)
   except TypeError:
-    # Maybe we need a password then
+    # Maybe we need a password then.
     if sys.stdin.isatty():
       password = getpass.getpass(prompt="Private key password: ")
     else:
@@ -134,7 +134,7 @@ def main(args):
 
   for authenticator in tqdm(get_opensk_devices(args.batch)):
     # If the device supports it, wink to show which device
-    # we're going to program
+    # we're going to program.
     if authenticator.device.capabilities & hid.CAPABILITY.WINK:
       authenticator.device.wink()
     aaguid = uuid.UUID(bytes=authenticator.get_info().aaguid)
@@ -149,11 +149,20 @@ def main(args):
       )
       info("Certificate: {}".format("Present" if result[1] else "Missing"))
       info("Private Key: {}".format("Present" if result[2] else "Missing"))
-      if result[3]:
-        info("Device locked down!")
+      if args.lock:
+        info("Device is now locked down!")
     except ctap.CtapError as ex:
       if ex.code.value == ctap.CtapError.ERR.INVALID_COMMAND:
         error("Failed to configure OpenSK (unsupported command).")
+      elif ex.code.value == 0xF2:  # VENDOR_INTERNAL_ERROR
+        error(("Failed to configure OpenSK (lockdown conditions not met "
+               "or hardware error)."))
+      elif ex.code.value == ctap.CtapError.ERR.INVALID_PARAMETER:
+        error(
+            ("Failed to configure OpenSK (device is partially programmed but "
+             "the given cert/key don't match the ones currently programmed)."))
+      else:
+        error("Failed to configure OpenSK (unknown error: {}".format(ex))
 
 
 if __name__ == "__main__":
@@ -174,7 +183,7 @@ def main(args):
       metavar="PEM_FILE",
       dest="certificate",
       help=("PEM file containing the certificate to inject into "
-            "OpenSK authenticator."),
+            "the OpenSK authenticator."),
   )
   parser.add_argument(
       "--private-key",