Address comments

src/api/attestation_store.rs

@@ -1,13 +1,13 @@
-use alloc::string::String;
 use alloc::vec::Vec;
 use persistent_store::{StoreError, StoreUpdate};
 
 use crate::env::Env;
 
 /// Identifies an attestation.
+#[derive(Clone, PartialEq, Eq)]
 pub enum Id {
     Batch,
-    Enterprise { rp_id: String },
+    Enterprise,
 }
 
 #[cfg_attr(feature = "std", derive(Debug, PartialEq, Eq))]
@@ -18,9 +18,6 @@ pub struct Attestation {
 }
 
 /// Stores enterprise or batch attestations.
-///
-/// Implementations don't need to distinguish different attestations. In particular, setting one
-/// attestation may set other ones.
 pub trait AttestationStore {
     /// Returns an attestation given its id, if it exists.
     ///
@@ -48,11 +45,17 @@ pub const STORAGE_KEYS: &[usize] = &[1, 2];
 
 /// Implements a default attestation store using the environment store.
 ///
-/// The same attestation is used for batch and enterprise.
+/// Supports only one attestation at a time.
-pub trait Helper: Env {}
+pub trait Helper: Env {
+    /// Returns the current attestation id.
+    fn attestation_id(&self) -> Id;
+}
 
 impl<T: Helper> AttestationStore for T {
-    fn get(&mut self, _: &Id) -> Result<Option<Attestation>, Error> {
+    fn get(&mut self, id: &Id) -> Result<Option<Attestation>, Error> {
+        if id != &self.attestation_id() {
+            return Err(Error::NoSupport);
+        }
         let private_key = self.store().find(PRIVATE_KEY_STORAGE_KEY)?;
         let certificate = self.store().find(CERTIFICATE_STORAGE_KEY)?;
         let (private_key, certificate) = match (private_key, certificate) {
@@ -69,7 +72,10 @@ impl<T: Helper> AttestationStore for T {
         }))
     }
 
-    fn set(&mut self, _: &Id, attestation: Option<&Attestation>) -> Result<(), Error> {
+    fn set(&mut self, id: &Id, attestation: Option<&Attestation>) -> Result<(), Error> {
+        if id != &self.attestation_id() {
+            return Err(Error::NoSupport);
+        }
         let updates = match attestation {
             None => [
                 StoreUpdate::Remove {

src/ctap/apdu.rs

@@ -16,6 +16,8 @@ use alloc::vec::Vec;
 use byteorder::{BigEndian, ByteOrder};
 use core::convert::TryFrom;
 
+use crate::api::attestation_store;
+
 const APDU_HEADER_LEN: usize = 4;
 
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -44,6 +46,17 @@ impl From<ApduStatusCode> for u16 {
     }
 }
 
+impl From<attestation_store::Error> for ApduStatusCode {
+    fn from(error: attestation_store::Error) -> Self {
+        use attestation_store::Error;
+        match error {
+            Error::Storage => ApduStatusCode::SW_MEMERR,
+            Error::Internal => ApduStatusCode::SW_INTERNAL_EXCEPTION,
+            Error::NoSupport => ApduStatusCode::SW_INTERNAL_EXCEPTION,
+        }
+    }
+}
+
 #[allow(dead_code)]
 pub enum ApduInstructions {
     Select = 0xA4,

src/ctap/ctap1.rs

@@ -262,8 +262,7 @@ impl Ctap1Command {
             certificate,
         } = env
             .attestation_store()
-            .get(&attestation_store::Id::Batch)
+            .get(&attestation_store::Id::Batch)?
-            .map_err(|_| Ctap1StatusCode::SW_MEMERR)?
             .ok_or(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?;
 
         let mut response = Vec::with_capacity(105 + key_handle.len() + certificate.len());

src/ctap/mod.rs

@@ -861,7 +861,7 @@ impl CtapState {
                 key_type: PublicKeyCredentialType::PublicKey,
                 credential_id: random_id.clone(),
                 private_key: private_key.clone(),
-                rp_id: rp_id.clone(),
+                rp_id,
                 user_handle: user.user_id,
                 // This input is user provided, so we crop it to 64 byte for storage.
                 // The UTF8 encoding is always preserved, so the string might end up shorter.
@@ -922,7 +922,7 @@ impl CtapState {
         let attestation_id = if env.customization().use_batch_attestation() {
             Some(attestation_store::Id::Batch)
         } else if ep_att {
-            Some(attestation_store::Id::Enterprise { rp_id })
+            Some(attestation_store::Id::Enterprise)
         } else {
             None
         };
@@ -2123,6 +2123,7 @@ mod test {
     #[test]
     fn test_process_make_credential_with_enterprise_attestation_vendor_facilitated() {
         let mut env = TestEnv::new();
+        env.set_attestation_id(attestation_store::Id::Enterprise);
         env.customization_mut().setup_enterprise_attestation(
             Some(EnterpriseAttestationMode::VendorFacilitated),
             Some(vec!["example.com".to_string()]),
@@ -2169,6 +2170,7 @@ mod test {
     #[test]
     fn test_process_make_credential_with_enterprise_attestation_platform_managed() {
         let mut env = TestEnv::new();
+        env.set_attestation_id(attestation_store::Id::Enterprise);
         env.customization_mut().setup_enterprise_attestation(
             Some(EnterpriseAttestationMode::PlatformManaged),
             Some(vec!["example.com".to_string()]),
@@ -2205,6 +2207,7 @@ mod test {
     #[test]
     fn test_process_make_credential_with_enterprise_attestation_invalid() {
         let mut env = TestEnv::new();
+        env.set_attestation_id(attestation_store::Id::Enterprise);
         env.customization_mut()
             .setup_enterprise_attestation(Some(EnterpriseAttestationMode::PlatformManaged), None);
 

src/ctap/storage.rs

@@ -14,6 +14,7 @@
 
 mod key;
 
+use crate::api::attestation_store::{self, AttestationStore};
 use crate::api::customization::Customization;
 use crate::api::key_store::KeyStore;
 use crate::ctap::client_pin::PIN_AUTH_LENGTH;
@@ -493,9 +494,14 @@ pub fn enterprise_attestation(env: &mut impl Env) -> Result<bool, Ctap2StatusCod
 }
 
 /// Marks enterprise attestation as enabled.
-///
-/// Doesn't check whether an attestation is setup because it depends on the RP id.
 pub fn enable_enterprise_attestation(env: &mut impl Env) -> Result<(), Ctap2StatusCode> {
+    if env
+        .attestation_store()
+        .get(&attestation_store::Id::Enterprise)?
+        .is_none()
+    {
+        return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR);
+    }
     if !enterprise_attestation(env)? {
         env.store().insert(key::ENTERPRISE_ATTESTATION, &[])?;
     }
@@ -1135,13 +1141,14 @@ mod test {
     #[test]
     fn test_enterprise_attestation() {
         let mut env = TestEnv::new();
+        env.set_attestation_id(attestation_store::Id::Enterprise);
 
         let dummy_attestation = Attestation {
             private_key: [0x41; key_material::ATTESTATION_PRIVATE_KEY_LENGTH],
             certificate: vec![0xdd; 20],
         };
         env.attestation_store()
-            .set(&attestation_store::Id::Batch, Some(&dummy_attestation))
+            .set(&attestation_store::Id::Enterprise, Some(&dummy_attestation))
             .unwrap();
 
         assert!(!enterprise_attestation(&mut env).unwrap());

src/env/test/mod.rs

@@ -36,6 +36,7 @@ pub struct TestEnv {
     store: Store<BufferStorage>,
     upgrade_storage: Option<BufferUpgradeStorage>,
     customization: TestCustomization,
+    attestation_id: attestation_store::Id,
 }
 
 pub struct TestRng256 {
@@ -106,12 +107,14 @@ impl TestEnv {
         let store = Store::new(storage).ok().unwrap();
         let upgrade_storage = Some(BufferUpgradeStorage::new().unwrap());
         let customization = DEFAULT_CUSTOMIZATION.into();
+        let attestation_id = attestation_store::Id::Batch;
         TestEnv {
             rng,
             user_presence,
             store,
             upgrade_storage,
             customization,
+            attestation_id,
         }
     }
 
@@ -126,6 +129,10 @@ impl TestEnv {
     pub fn rng(&mut self) -> &mut TestRng256 {
         &mut self.rng
     }
+
+    pub fn set_attestation_id(&mut self, id: attestation_store::Id) {
+        self.attestation_id = id;
+    }
 }
 
 impl TestUserPresence {
@@ -149,7 +156,12 @@ impl FirmwareProtection for TestEnv {
 }
 
 impl key_store::Helper for TestEnv {}
-impl attestation_store::Helper for TestEnv {}
+
+impl attestation_store::Helper for TestEnv {
+    fn attestation_id(&self) -> attestation_store::Id {
+        self.attestation_id.clone()
+    }
+}
 
 impl Env for TestEnv {
     type Rng = TestRng256;

src/env/tock/mod.rs

@@ -195,7 +195,12 @@ impl FirmwareProtection for TockEnv {
 }
 
 impl key_store::Helper for TockEnv {}
-impl attestation_store::Helper for TockEnv {}
+
+impl attestation_store::Helper for TockEnv {
+    fn attestation_id(&self) -> attestation_store::Id {
+        attestation_store::Id::Batch
+    }
+}
 
 impl Env for TockEnv {
     type Rng = TockRng256;

src/test_helpers/mod.rs

@@ -12,10 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use crate::api::attestation_store::{self, Attestation, AttestationStore};
 use crate::clock::CtapInstant;
 use crate::ctap::command::{
-    AuthenticatorAttestationMaterial, AuthenticatorConfigParameters,
+    AuthenticatorAttestationMaterial, AuthenticatorConfigParameters, Command,
-    AuthenticatorVendorConfigureParameters, Command,
 };
 use crate::ctap::data_formats::ConfigSubCommand;
 use crate::ctap::status_code::Ctap2StatusCode;
@@ -32,22 +32,17 @@ pub fn enable_enterprise_attestation(
     state: &mut CtapState,
     env: &mut impl Env,
 ) -> Result<AuthenticatorAttestationMaterial, Ctap2StatusCode> {
-    let dummy_key = [0x41; key_material::ATTESTATION_PRIVATE_KEY_LENGTH];
-    let dummy_cert = vec![0xdd; 20];
     let attestation_material = AuthenticatorAttestationMaterial {
-        certificate: dummy_cert,
+        certificate: vec![0xdd; 20],
-        private_key: dummy_key,
+        private_key: [0x41; key_material::ATTESTATION_PRIVATE_KEY_LENGTH],
     };
-    let configure_params = AuthenticatorVendorConfigureParameters {
+
-        lockdown: false,
+    let attestation = Attestation {
-        attestation_material: Some(attestation_material.clone()),
+        private_key: attestation_material.private_key,
+        certificate: attestation_material.certificate.clone(),
     };
-    #[cfg(feature = "vendor_hid")]
+    env.attestation_store()
-    let vendor_channel = VENDOR_CHANNEL;
+        .set(&attestation_store::Id::Enterprise, Some(&attestation))?;
-    #[cfg(not(feature = "vendor_hid"))]
-    let vendor_channel = DUMMY_CHANNEL;
-    let vendor_command = Command::AuthenticatorVendorConfigure(configure_params);
-    state.process_parsed_command(env, vendor_command, vendor_channel, CtapInstant::new(0))?;
 
     let config_params = AuthenticatorConfigParameters {
         sub_command: ConfigSubCommand::EnableEnterpriseAttestation,