new CoseKey data format

libraries/crypto/src/ecdh.rs

@@ -85,7 +85,7 @@ impl PubKey {
         self.p.to_bytes_uncompressed(bytes);
     }
 
-    /// Creates a new PubKey from their coordinates.
+    /// Creates a new PubKey from its coordinates on the elliptic curve.
     pub fn from_coordinates(x: &[u8; NBYTES], y: &[u8; NBYTES]) -> Option<PubKey> {
         PointP256::new_checked_vartime(Int256::from_bin(x), Int256::from_bin(y))
             .map(|p| PubKey { p })

libraries/crypto/src/ecdsa.rs

@@ -220,7 +220,6 @@ impl Signature {
 }
 
 impl PubKey {
-    pub const ES256_ALGORITHM: i64 = -7;
     #[cfg(feature = "with_ctap1")]
     const UNCOMPRESSED_LENGTH: usize = 1 + 2 * int256::NBYTES;
 

src/ctap/command.rs

@@ -317,7 +317,7 @@ impl TryFrom<cbor::Value> for AuthenticatorClientPinParameters {
 
         let pin_protocol = extract_unsigned(ok_or_missing(pin_protocol)?)?;
         let sub_command = ClientPinSubCommand::try_from(ok_or_missing(sub_command)?)?;
-        let key_agreement = key_agreement.map(extract_map).transpose()?.map(CoseKey);
+        let key_agreement = key_agreement.map(CoseKey::try_from).transpose()?;
         let pin_auth = pin_auth.map(extract_byte_string).transpose()?;
         let new_pin_enc = new_pin_enc.map(extract_byte_string).transpose()?;
         let pin_hash_enc = pin_hash_enc.map(extract_byte_string).transpose()?;
@@ -423,8 +423,8 @@ mod test {
     };
     use super::super::ES256_CRED_PARAM;
     use super::*;
-    use alloc::collections::BTreeMap;
     use cbor::{cbor_array, cbor_map};
+    use crypto::rng256::ThreadRng256;
 
     #[test]
     fn test_from_cbor_make_credential_parameters() {
@@ -534,10 +534,15 @@ mod test {
 
     #[test]
     fn test_from_cbor_client_pin_parameters() {
+        let mut rng = ThreadRng256 {};
+        let sk = crypto::ecdh::SecKey::gensk(&mut rng);
+        let pk = sk.genpk();
+        let cose_key = CoseKey::from(pk);
+
         let cbor_value = cbor_map! {
             1 => 1,
             2 => ClientPinSubCommand::GetPinRetries,
-            3 => cbor_map!{},
+            3 => cbor::Value::from(cose_key.clone()),
             4 => vec! [0xBB],
             5 => vec! [0xCC],
             6 => vec! [0xDD],
@@ -552,7 +557,7 @@ mod test {
         let expected_pin_protocol_parameters = AuthenticatorClientPinParameters {
             pin_protocol: 1,
             sub_command: ClientPinSubCommand::GetPinRetries,
-            key_agreement: Some(CoseKey(BTreeMap::new())),
+            key_agreement: Some(cose_key),
             pin_auth: Some(vec![0xBB]),
             new_pin_enc: Some(vec![0xCC]),
             pin_hash_enc: Some(vec![0xDD]),

src/ctap/data_formats.rs

@@ -17,12 +17,23 @@ use alloc::collections::BTreeMap;
 use alloc::string::String;
 use alloc::vec::Vec;
 use arrayref::array_ref;
-use cbor::{cbor_array_vec, cbor_bytes_lit, cbor_map_options, destructure_cbor_map};
+use cbor::{cbor_array_vec, cbor_map, cbor_map_options, destructure_cbor_map};
-use core::convert::{TryFrom, TryInto};
+use core::convert::TryFrom;
 use crypto::{ecdh, ecdsa};
 #[cfg(test)]
 use enum_iterator::IntoEnumIterator;
 
+// This is the algorithm specifier that is supposed to be used in a COSE key
+// map in ECDH. CTAP requests -25 which represents ECDH-ES + HKDF-256 here:
+// https://www.iana.org/assignments/cose/cose.xhtml#algorithms
+const ECDH_ALGORITHM: i64 = -25;
+// This is the identifier used for ECDSA and ECDH in OpenSSH.
+const ES256_ALGORITHM: i64 = -7;
+// The COSE key parameter behind map key 1.
+const EC2_KEY_TYPE: i64 = 2;
+// The COSE key parameter behind map key -1.
+const P_256_CURVE: i64 = 1;
+
 // https://www.w3.org/TR/webauthn/#dictdef-publickeycredentialrpentity
 #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))]
 pub struct PublicKeyCredentialRpEntity {
@@ -322,17 +333,17 @@ impl TryFrom<cbor::Value> for GetAssertionHmacSecretInput {
     fn try_from(cbor_value: cbor::Value) -> Result<Self, Ctap2StatusCode> {
         destructure_cbor_map! {
             let {
-                1 => cose_key,
+                1 => key_agreement,
                 2 => salt_enc,
                 3 => salt_auth,
             } = extract_map(cbor_value)?;
         }
 
-        let cose_key = extract_map(ok_or_missing(cose_key)?)?;
+        let key_agreement = CoseKey::try_from(ok_or_missing(key_agreement)?)?;
         let salt_enc = extract_byte_string(ok_or_missing(salt_enc)?)?;
         let salt_auth = extract_byte_string(ok_or_missing(salt_auth)?)?;
         Ok(Self {
-            key_agreement: CoseKey(cose_key),
+            key_agreement,
             salt_enc,
             salt_auth,
         })
@@ -432,7 +443,7 @@ impl From<PackedAttestationStatement> for cbor::Value {
 #[derive(PartialEq)]
 #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))]
 pub enum SignatureAlgorithm {
-    ES256 = ecdsa::PubKey::ES256_ALGORITHM as isize,
+    ES256 = ES256_ALGORITHM as isize,
     // This is the default for all numbers not covered above.
     // Unknown types should be ignored, instead of returning errors.
     Unknown = 0,
@@ -449,7 +460,7 @@ impl TryFrom<cbor::Value> for SignatureAlgorithm {
 
     fn try_from(cbor_value: cbor::Value) -> Result<Self, Ctap2StatusCode> {
         match extract_integer(cbor_value)? {
-            ecdsa::PubKey::ES256_ALGORITHM => Ok(SignatureAlgorithm::ES256),
+            ES256_ALGORITHM => Ok(SignatureAlgorithm::ES256),
             _ => Ok(SignatureAlgorithm::Unknown),
         }
     }
@@ -614,85 +625,31 @@ impl PublicKeyCredentialSource {
     }
 }
 
-// TODO(kaczmarczyck) we could decide to split this data type up
+// The COSE key is used for both ECDH and ECDSA public keys for transmission.
-// It depends on the algorithm though, I think.
-// So before creating a mess, this is my workaround.
 #[derive(Clone)]
 #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))]
-pub struct CoseKey(pub BTreeMap<cbor::KeyType, cbor::Value>);
+pub struct CoseKey {
-
+    x_bytes: [u8; ecdh::NBYTES],
-// This is the algorithm specifier that is supposed to be used in a COSE key
+    y_bytes: [u8; ecdh::NBYTES],
-// map. The CTAP specification says -25 which represents ECDH-ES + HKDF-256
+    algorithm: i64,
-// here: https://www.iana.org/assignments/cose/cose.xhtml#algorithms
+}
-// In fact, this is just used for compatibility with older specification versions.
-const ECDH_ALGORITHM: i64 = -25;
-// This is the identifier used by OpenSSH. To be compatible, we accept both.
-const ES256_ALGORITHM: i64 = -7;
-const EC2_KEY_TYPE: i64 = 2;
-const P_256_CURVE: i64 = 1;
 
+// This conversion accepts both ECDH and ECDSA.
 impl TryFrom<cbor::Value> for CoseKey {
     type Error = Ctap2StatusCode;
 
     fn try_from(cbor_value: cbor::Value) -> Result<Self, Ctap2StatusCode> {
-        if let cbor::Value::Map(cose_map) = cbor_value {
-            Ok(CoseKey(cose_map))
-        } else {
-            Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)
-        }
-    }
-}
-
-fn cose_key_from_bytes(x_bytes: [u8; ecdh::NBYTES], y_bytes: [u8; ecdh::NBYTES]) -> CoseKey {
-    let x_byte_cbor: cbor::Value = cbor_bytes_lit!(&x_bytes);
-    let y_byte_cbor: cbor::Value = cbor_bytes_lit!(&y_bytes);
-    // TODO(kaczmarczyck) do not write optional parameters, spec is unclear
-    let cose_cbor_value = cbor_map_options! {
-        1 => EC2_KEY_TYPE,
-        3 => ECDH_ALGORITHM,
-        -1 => P_256_CURVE,
-        -2 => x_byte_cbor,
-        -3 => y_byte_cbor,
-    };
-    // Unwrap is safe here since we know it's a map.
-    cose_cbor_value.try_into().unwrap()
-}
-
-impl From<ecdh::PubKey> for CoseKey {
-    fn from(pk: ecdh::PubKey) -> Self {
-        let mut x_bytes = [0; ecdh::NBYTES];
-        let mut y_bytes = [0; ecdh::NBYTES];
-        pk.to_coordinates(&mut x_bytes, &mut y_bytes);
-        cose_key_from_bytes(x_bytes, y_bytes)
-    }
-}
-
-impl TryFrom<CoseKey> for ecdh::PubKey {
-    type Error = Ctap2StatusCode;
-
-    fn try_from(cose_key: CoseKey) -> Result<Self, Ctap2StatusCode> {
         destructure_cbor_map! {
             let {
+                // This is sorted correctly, negative encoding is bigger.
                 1 => key_type,
                 3 => algorithm,
                 -1 => curve,
                 -2 => x_bytes,
                 -3 => y_bytes,
-            } = cose_key.0;
+            } = extract_map(cbor_value)?;
         }
 
-        let key_type = extract_integer(ok_or_missing(key_type)?)?;
-        if key_type != EC2_KEY_TYPE {
-            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
-        }
-        let algorithm = extract_integer(ok_or_missing(algorithm)?)?;
-        if algorithm != ECDH_ALGORITHM && algorithm != ES256_ALGORITHM {
-            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
-        }
-        let curve = extract_integer(ok_or_missing(curve)?)?;
-        if curve != P_256_CURVE {
-            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
-        }
         let x_bytes = extract_byte_string(ok_or_missing(x_bytes)?)?;
         if x_bytes.len() != ecdh::NBYTES {
             return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
@@ -701,11 +658,55 @@ impl TryFrom<CoseKey> for ecdh::PubKey {
         if y_bytes.len() != ecdh::NBYTES {
             return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
         }
+        let curve = extract_integer(ok_or_missing(curve)?)?;
+        if curve != P_256_CURVE {
+            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
+        }
+        let key_type = extract_integer(ok_or_missing(key_type)?)?;
+        if key_type != EC2_KEY_TYPE {
+            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
+        }
+        let algorithm = extract_integer(ok_or_missing(algorithm)?)?;
+        if algorithm != ECDH_ALGORITHM && algorithm != ES256_ALGORITHM {
+            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
+        }
 
-        let x_array_ref = array_ref![x_bytes.as_slice(), 0, ecdh::NBYTES];
+        Ok(CoseKey {
-        let y_array_ref = array_ref![y_bytes.as_slice(), 0, ecdh::NBYTES];
+            x_bytes: *array_ref![x_bytes.as_slice(), 0, ecdh::NBYTES],
-        ecdh::PubKey::from_coordinates(x_array_ref, y_array_ref)
+            y_bytes: *array_ref![y_bytes.as_slice(), 0, ecdh::NBYTES],
-            .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)
+            algorithm,
+        })
+    }
+}
+
+impl From<CoseKey> for cbor::Value {
+    fn from(cose_key: CoseKey) -> Self {
+        let CoseKey {
+            x_bytes,
+            y_bytes,
+            algorithm,
+        } = cose_key;
+
+        cbor_map! {
+            1 => EC2_KEY_TYPE,
+            3 => algorithm,
+            -1 => P_256_CURVE,
+            -2 => x_bytes,
+            -3 => y_bytes,
+        }
+    }
+}
+
+impl From<ecdh::PubKey> for CoseKey {
+    fn from(pk: ecdh::PubKey) -> Self {
+        let mut x_bytes = [0; ecdh::NBYTES];
+        let mut y_bytes = [0; ecdh::NBYTES];
+        pk.to_coordinates(&mut x_bytes, &mut y_bytes);
+        CoseKey {
+            x_bytes,
+            y_bytes,
+            algorithm: ECDH_ALGORITHM,
+        }
     }
 }
 
@@ -714,7 +715,32 @@ impl From<ecdsa::PubKey> for CoseKey {
         let mut x_bytes = [0; ecdh::NBYTES];
         let mut y_bytes = [0; ecdh::NBYTES];
         pk.to_coordinates(&mut x_bytes, &mut y_bytes);
-        cose_key_from_bytes(x_bytes, y_bytes)
+        CoseKey {
+            x_bytes,
+            y_bytes,
+            algorithm: ES256_ALGORITHM,
+        }
+    }
+}
+
+impl TryFrom<CoseKey> for ecdh::PubKey {
+    type Error = Ctap2StatusCode;
+
+    fn try_from(cose_key: CoseKey) -> Result<Self, Ctap2StatusCode> {
+        let CoseKey {
+            x_bytes,
+            y_bytes,
+            algorithm,
+        } = cose_key;
+
+        // Since algorithm can be used for different COSE key types, we check
+        // whether the current type is correct for ECDH. For an OpenSSH bugfix,
+        // the algorithm ES256_ALGORITHM is allowed here too.
+        if algorithm != ECDH_ALGORITHM && algorithm != ES256_ALGORITHM {
+            return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM);
+        }
+        ecdh::PubKey::from_coordinates(&x_bytes, &y_bytes)
+            .ok_or(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)
     }
 }
 
@@ -827,8 +853,8 @@ mod test {
     use super::*;
     use alloc::collections::BTreeMap;
     use cbor::{
-        cbor_array, cbor_bool, cbor_bytes, cbor_false, cbor_int, cbor_map, cbor_null, cbor_text,
+        cbor_array, cbor_bool, cbor_bytes, cbor_bytes_lit, cbor_false, cbor_int, cbor_null,
-        cbor_unsigned,
+        cbor_text, cbor_unsigned,
     };
     use crypto::rng256::{Rng256, ThreadRng256};
 
@@ -1151,7 +1177,7 @@ mod test {
 
     #[test]
     fn test_from_into_signature_algorithm() {
-        let cbor_signature_algorithm: cbor::Value = cbor_int!(ecdsa::PubKey::ES256_ALGORITHM);
+        let cbor_signature_algorithm: cbor::Value = cbor_int!(ES256_ALGORITHM);
         let signature_algorithm = SignatureAlgorithm::try_from(cbor_signature_algorithm.clone());
         let expected_signature_algorithm = SignatureAlgorithm::ES256;
         assert_eq!(signature_algorithm, Ok(expected_signature_algorithm));
@@ -1225,7 +1251,7 @@ mod test {
     fn test_from_into_public_key_credential_parameter() {
         let cbor_credential_parameter = cbor_map! {
             "type" => "public-key",
-            "alg" => ecdsa::PubKey::ES256_ALGORITHM,
+            "alg" => ES256_ALGORITHM,
         };
         let credential_parameter =
             PublicKeyCredentialParameter::try_from(cbor_credential_parameter.clone());
@@ -1279,7 +1305,7 @@ mod test {
         let cose_key = CoseKey::from(pk);
         let cbor_extensions = cbor_map! {
             "hmac-secret" => cbor_map! {
-                1 => cbor::Value::Map(cose_key.0.clone()),
+                1 => cbor::Value::from(cose_key.clone()),
                 2 => vec![0x02; 32],
                 3 => vec![0x03; 16],
             },
@@ -1343,6 +1369,83 @@ mod test {
         assert_eq!(created_cbor, cbor_packed_attestation_statement);
     }
 
+    #[test]
+    fn test_from_into_cose_key_cbor() {
+        for algorithm in &[ECDH_ALGORITHM, ES256_ALGORITHM] {
+            let cbor_value = cbor_map! {
+                1 => EC2_KEY_TYPE,
+                3 => algorithm,
+                -1 => P_256_CURVE,
+                -2 => [0u8; 32],
+                -3 => [0u8; 32],
+            };
+            let cose_key = CoseKey::try_from(cbor_value.clone()).unwrap();
+            let created_cbor_value = cbor::Value::from(cose_key);
+            assert_eq!(created_cbor_value, cbor_value);
+        }
+
+        let cbor_value = cbor_map! {
+            1 => EC2_KEY_TYPE,
+            // unknown algorithm
+            3 => 0,
+            -1 => P_256_CURVE,
+            -2 => [0u8; 32],
+            -3 => [0u8; 32],
+        };
+        assert_eq!(
+            CoseKey::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM)
+        );
+        let cbor_value = cbor_map! {
+            // unknown type
+            1 => 0,
+            3 => ECDH_ALGORITHM,
+            -1 => P_256_CURVE,
+            -2 => [0u8; 32],
+            -3 => [0u8; 32],
+        };
+        assert_eq!(
+            CoseKey::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM)
+        );
+        let cbor_value = cbor_map! {
+            1 => EC2_KEY_TYPE,
+            3 => ECDH_ALGORITHM,
+            // unknown curve
+            -1 => 0,
+            -2 => [0u8; 32],
+            -3 => [0u8; 32],
+        };
+        assert_eq!(
+            CoseKey::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_ALGORITHM)
+        );
+        let cbor_value = cbor_map! {
+            1 => EC2_KEY_TYPE,
+            3 => ECDH_ALGORITHM,
+            -1 => P_256_CURVE,
+            // wrong length
+            -2 => [0u8; 31],
+            -3 => [0u8; 32],
+        };
+        assert_eq!(
+            CoseKey::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)
+        );
+        let cbor_value = cbor_map! {
+            1 => EC2_KEY_TYPE,
+            3 => ECDH_ALGORITHM,
+            -1 => P_256_CURVE,
+            -2 => [0u8; 32],
+            // wrong length
+            -3 => [0u8; 33],
+        };
+        assert_eq!(
+            CoseKey::try_from(cbor_value),
+            Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)
+        );
+    }
+
     #[test]
     fn test_from_into_cose_key_ecdh() {
         let mut rng = ThreadRng256 {};
@@ -1359,17 +1462,7 @@ mod test {
         let sk = crypto::ecdsa::SecKey::gensk(&mut rng);
         let pk = sk.genpk();
         let cose_key = CoseKey::from(pk);
-        let cose_map = cose_key.0;
+        assert_eq!(cose_key.algorithm, ES256_ALGORITHM);
-        let template = cbor_map! {
-            1 => 0,
-            3 => 0,
-            -1 => 0,
-            -2 => 0,
-            -3 => 0,
-        };
-        for key in CoseKey::try_from(template).unwrap().0.keys() {
-            assert!(cose_map.contains_key(key));
-        }
     }
 
     #[test]

src/ctap/mod.rs

@@ -534,7 +534,7 @@ where
         }
         auth_data.extend(vec![0x00, credential_id.len() as u8]);
         auth_data.extend(&credential_id);
-        if !cbor::write(cbor::Value::Map(CoseKey::from(pk).0), &mut auth_data) {
+        if !cbor::write(cbor::Value::from(CoseKey::from(pk)), &mut auth_data) {
             return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR);
         }
         if has_extension_output {

src/ctap/response.rs

@@ -192,7 +192,7 @@ impl From<AuthenticatorClientPinResponse> for cbor::Value {
         } = client_pin_response;
 
         cbor_map_options! {
-            1 => key_agreement.map(|cose_key| cbor_map_btree!(cose_key.0)),
+            1 => key_agreement.map(cbor::Value::from),
             2 => pin_token,
             3 => retries,
         }