From a5aa9ef0d9bbc26fd2dd26fbd16a8bc0f2a17bce Mon Sep 17 00:00:00 2001
From: Robin Krahl <robin@nitrokey.com>
Date: Thu, 13 Jul 2023 12:34:28 +0200
Subject: [PATCH] fixup! Reduce ID length for new credentials

---
 Cargo.toml        |  1 +
 src/credential.rs | 94 +++++++++++++++++++++++++++++++++++++++++++----
 src/ctap2.rs      |  3 +-
 3 files changed, 89 insertions(+), 9 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 97363ba..b6b8485 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -40,6 +40,7 @@ log-error = []
 [dev-dependencies]
 # quickcheck = "1"
 rand = "0.8.4"
+trussed = { version = "0.1", features = ["virt"] }
 
 [package.metadata.docs.rs]
 features = ["dispatch"]
diff --git a/src/credential.rs b/src/credential.rs
index 06649db..2f2cd91 100644
--- a/src/credential.rs
+++ b/src/credential.rs
@@ -383,7 +383,13 @@ impl FullCredential {
             StrippedCredential::from(self).id(trussed, key_encryption_key, &rp_id_hash)
         } else {
             let stripped_credential = self.strip();
-            CredentialId::new(trussed, &stripped_credential, key_encryption_key, &rp_id_hash, &self.nonce)
+            CredentialId::new(
+                trussed,
+                &stripped_credential,
+                key_encryption_key,
+                &rp_id_hash,
+                &self.nonce,
+            )
         }
     }
 
@@ -403,6 +409,7 @@ impl FullCredential {
 
     // This method is only kept for compatibility.  To strip new credentials, use
     // `StrippedCredential`.
+    #[must_use]
     fn strip(&self) -> Self {
         info_now!(":: stripping ID");
         let mut stripped = self.clone();
@@ -459,13 +466,7 @@ impl StrippedCredential {
         key_encryption_key: KeyId,
         rp_id_hash: &Bytes<32>,
     ) -> Result<CredentialId> {
-        CredentialId::new(
-            trussed,
-            self,
-            key_encryption_key,
-            rp_id_hash,
-            &self.nonce,
-        )
+        CredentialId::new(trussed, self, key_encryption_key, rp_id_hash, &self.nonce)
     }
 }
 
@@ -487,6 +488,10 @@ impl From<&FullCredential> for StrippedCredential {
 #[cfg(test)]
 mod test {
     use super::*;
+    use trussed::{
+        client::{Chacha8Poly1305, Sha256},
+        types::Location,
+    };
 
     fn credential_data() -> CredentialData {
         use ctap_types::webauthn::{PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity};
@@ -610,6 +615,79 @@ mod test {
         assert_eq!(credential_data, deserialized);
     }
 
+    #[test]
+    fn credential_ids() {
+        trussed::virt::with_ram_client("fido", |mut client| {
+            let kek = syscall!(client.generate_chacha8poly1305_key(Location::Internal)).key;
+            let mut nonce = Bytes::new();
+            nonce.extend_from_slice(&[0; 12]).unwrap();
+            let data = credential_data();
+            let mut full_credential = FullCredential {
+                ctap: CtapVersion::Fido21Pre,
+                data,
+                nonce,
+            };
+            let rp_id_hash = syscall!(client.hash_sha256(full_credential.rp.id.as_ref()))
+                .hash
+                .to_bytes()
+                .unwrap();
+
+            // Case 1: credential with use_short_id = Some(true) uses new (short) format
+            full_credential.data.use_short_id = Some(true);
+            let stripped_credential = StrippedCredential::from(&full_credential);
+            let full_id = full_credential
+                .id(&mut client, kek, Some(&rp_id_hash))
+                .unwrap();
+            let short_id = stripped_credential
+                .id(&mut client, kek, &rp_id_hash)
+                .unwrap();
+            assert_eq!(full_id.0, short_id.0);
+
+            // Case 2: credential with use_short_id = None uses old (long) format
+            full_credential.data.use_short_id = None;
+            let stripped_credential = full_credential.strip();
+            let full_id = full_credential
+                .id(&mut client, kek, Some(&rp_id_hash))
+                .unwrap();
+            let long_id = CredentialId::new(
+                &mut client,
+                &stripped_credential,
+                kek,
+                &rp_id_hash,
+                &full_credential.nonce,
+            )
+            .unwrap();
+            assert_eq!(full_id.0, long_id.0);
+
+            assert!(short_id.0.len() < long_id.0.len());
+        });
+    }
+
+    #[test]
+    fn max_credential_id() {
+        let rp_id: String<256> = core::iter::repeat('?').take(256).collect();
+        let key = Bytes::from_slice(&[u8::MAX; 128]).unwrap();
+        let credential = StrippedCredential {
+            ctap: CtapVersion::Fido21Pre,
+            creation_time: u32::MAX,
+            use_counter: true,
+            algorithm: i32::MAX,
+            key: Key::WrappedKey(key),
+            nonce: Bytes::from_slice(&[u8::MAX; 12]).unwrap(),
+            hmac_secret: Some(true),
+            cred_protect: Some(CredentialProtectionPolicy::Required),
+        };
+        trussed::virt::with_ram_client("fido", |mut client| {
+            let kek = syscall!(client.generate_chacha8poly1305_key(Location::Internal)).key;
+            let rp_id_hash = syscall!(client.hash_sha256(rp_id.as_ref()))
+                .hash
+                .to_bytes()
+                .unwrap();
+            let id = credential.id(&mut client, kek, &rp_id_hash).unwrap();
+            assert_eq!(id.0.len(), 204);
+        });
+    }
+
     // use quickcheck::TestResult;
     // quickcheck::quickcheck! {
     //   fn prop(
diff --git a/src/ctap2.rs b/src/ctap2.rs
index d5d911c..26208b1 100644
--- a/src/ctap2.rs
+++ b/src/ctap2.rs
@@ -356,7 +356,8 @@ impl<UP: UserPresence, T: TrussedRequirements> Authenticator for crate::Authenti
         );
 
         // note that this does the "stripping" of OptionalUI etc.
-        let credential_id = StrippedCredential::from(&credential).id(&mut self.trussed, kek, &rp_id_hash)?;
+        let credential_id =
+            StrippedCredential::from(&credential).id(&mut self.trussed, kek, &rp_id_hash)?;
 
         if rk_requested {
             // serialization with all metadata