From 909773da36f7bbcd88117c047fde26e0bbab60ac Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Mon, 8 Jun 2020 16:36:57 +0200 Subject: [PATCH] changes extensions to proper parsed data structures --- src/ctap/command.rs | 14 ++-- src/ctap/data_formats.rs | 175 ++++++++++++--------------------------- src/ctap/mod.rs | 67 +++++++-------- 3 files changed, 93 insertions(+), 163 deletions(-) diff --git a/src/ctap/command.rs b/src/ctap/command.rs index 21ac767..d6dd0fa 100644 --- a/src/ctap/command.rs +++ b/src/ctap/command.rs @@ -14,9 +14,9 @@ use super::data_formats::{ extract_array, extract_byte_string, extract_map, extract_text_string, extract_unsigned, - ok_or_missing, ClientPinSubCommand, CoseKey, Extensions, GetAssertionOptions, - MakeCredentialOptions, PublicKeyCredentialDescriptor, PublicKeyCredentialParameter, - PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, + ok_or_missing, ClientPinSubCommand, CoseKey, GetAssertionExtensions, GetAssertionOptions, + MakeCredentialExtensions, MakeCredentialOptions, PublicKeyCredentialDescriptor, + PublicKeyCredentialParameter, PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, }; use super::status_code::Ctap2StatusCode; use alloc::string::String; @@ -113,7 +113,7 @@ pub struct AuthenticatorMakeCredentialParameters { pub user: PublicKeyCredentialUserEntity, pub pub_key_cred_params: Vec, pub exclude_list: Option>, - pub extensions: Option, + pub extensions: Option, // Even though options are optional, we can use the default if not present. pub options: MakeCredentialOptions, pub pin_uv_auth_param: Option>, @@ -159,7 +159,7 @@ impl TryFrom for AuthenticatorMakeCredentialParameters { let extensions = param_map .remove(&cbor_unsigned!(6)) - .map(Extensions::try_from) + .map(MakeCredentialExtensions::try_from) .transpose()?; let options = match param_map.remove(&cbor_unsigned!(7)) { @@ -199,7 +199,7 @@ pub struct AuthenticatorGetAssertionParameters { pub rp_id: String, pub client_data_hash: Vec, pub allow_list: Option>, - pub extensions: Option, + pub extensions: Option, // Even though options are optional, we can use the default if not present. pub options: GetAssertionOptions, pub pin_uv_auth_param: Option>, @@ -233,7 +233,7 @@ impl TryFrom for AuthenticatorGetAssertionParameters { let extensions = param_map .remove(&cbor_unsigned!(4)) - .map(Extensions::try_from) + .map(GetAssertionExtensions::try_from) .transpose()?; let options = match param_map.remove(&cbor_unsigned!(5)) { diff --git a/src/ctap/data_formats.rs b/src/ctap/data_formats.rs index e838c2c..e5556b8 100644 --- a/src/ctap/data_formats.rs +++ b/src/ctap/data_formats.rs @@ -14,7 +14,7 @@ use super::status_code::Ctap2StatusCode; use alloc::collections::BTreeMap; -use alloc::string::{String, ToString}; +use alloc::string::String; use alloc::vec::Vec; use core::convert::TryFrom; use crypto::{ecdh, ecdsa}; @@ -243,66 +243,51 @@ impl From for cbor::Value { } } -#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))] -pub struct Extensions(BTreeMap); +#[cfg_attr(any(test, feature = "debug_ctap"), derive(Clone, Debug, PartialEq))] +pub struct MakeCredentialExtensions { + pub hmac_secret: bool, + pub cred_protect: Option, +} -impl TryFrom for Extensions { +impl TryFrom for MakeCredentialExtensions { type Error = Ctap2StatusCode; fn try_from(cbor_value: cbor::Value) -> Result { - let mut extensions = BTreeMap::new(); - for (extension_key, extension_value) in extract_map(cbor_value)? { - if let cbor::KeyType::TextString(extension_key_string) = extension_key { - extensions.insert(extension_key_string.to_string(), extension_value.clone()); - } else { - return Err(Ctap2StatusCode::CTAP2_ERR_CBOR_UNEXPECTED_TYPE); - } - } - Ok(Extensions(extensions)) - } -} - -impl From for cbor::Value { - fn from(extensions: Extensions) -> Self { - cbor_map_btree!(extensions - .0 - .into_iter() - .map(|(key, value)| (cbor_text!(key), value)) - .collect()) - } -} - -impl Extensions { - #[cfg(test)] - pub fn new(extension_map: BTreeMap) -> Self { - Extensions(extension_map) - } - - pub fn has_make_credential_hmac_secret(&self) -> Result { - self.0 - .get("hmac-secret") - .map(|b| extract_bool(b.clone())) - .unwrap_or(Ok(false)) - } - - pub fn get_assertion_hmac_secret( - mut self, - ) -> Option> { - self.0 - .remove("hmac-secret") - .map(GetAssertionHmacSecretInput::try_from) - } - - pub fn make_credential_cred_protect_policy( - mut self, - ) -> Option> { - self.0 - .remove("credProtect") + let mut extensions_map = extract_map(cbor_value)?; + let hmac_secret = extensions_map + .remove(&cbor_text!("hmac-secret")) + .map(extract_bool) + .unwrap_or(Ok(false))?; + let cred_protect = extensions_map + .remove(&cbor_text!("credProtect")) .map(CredentialProtectionPolicy::try_from) + .transpose()?; + Ok(Self { + hmac_secret, + cred_protect, + }) } } -#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))] +#[cfg_attr(any(test, feature = "debug_ctap"), derive(Clone, Debug, PartialEq))] +pub struct GetAssertionExtensions { + pub hmac_secret: Option, +} + +impl TryFrom for GetAssertionExtensions { + type Error = Ctap2StatusCode; + + fn try_from(cbor_value: cbor::Value) -> Result { + let mut extensions_map = extract_map(cbor_value)?; + let hmac_secret = extensions_map + .remove(&cbor_text!("hmac-secret")) + .map(GetAssertionHmacSecretInput::try_from) + .transpose()?; + Ok(Self { hmac_secret }) + } +} + +#[cfg_attr(any(test, feature = "debug_ctap"), derive(Clone, Debug, PartialEq))] pub struct GetAssertionHmacSecretInput { pub key_agreement: CoseKey, pub salt_enc: Vec, @@ -318,32 +303,13 @@ impl TryFrom for GetAssertionHmacSecretInput { let salt_enc = extract_byte_string(ok_or_missing(input_map.remove(&cbor_unsigned!(2)))?)?; let salt_auth = extract_byte_string(ok_or_missing(input_map.remove(&cbor_unsigned!(3)))?)?; Ok(Self { - key_agreement: CoseKey(cose_key.clone()), + key_agreement: CoseKey(cose_key), salt_enc, salt_auth, }) } } -#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))] -pub struct GetAssertionHmacSecretOutput(Vec); - -impl From for cbor::Value { - fn from(message: GetAssertionHmacSecretOutput) -> cbor::Value { - cbor_bytes!(message.0) - } -} - -impl TryFrom for GetAssertionHmacSecretOutput { - type Error = Ctap2StatusCode; - - fn try_from(cbor_value: cbor::Value) -> Result { - Ok(GetAssertionHmacSecretOutput(extract_byte_string( - cbor_value, - )?)) - } -} - // Even though options are optional, we can use the default if not present. #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))] pub struct MakeCredentialOptions { @@ -594,7 +560,7 @@ impl PublicKeyCredentialSource { // TODO(kaczmarczyck) we could decide to split this data type up // It depends on the algorithm though, I think. // So before creating a mess, this is my workaround. -#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))] +#[cfg_attr(any(test, feature = "debug_ctap"), derive(Clone, Debug, PartialEq))] pub struct CoseKey(pub BTreeMap); // This is the algorithm specifier that is supposed to be used in a COSE key @@ -1191,44 +1157,21 @@ mod test { } #[test] - fn test_from_into_extensions() { - let cbor_extensions = cbor_map! { - "the_answer" => 42, - }; - let extensions = Extensions::try_from(cbor_extensions.clone()); - let mut expected_extensions = Extensions(BTreeMap::new()); - expected_extensions - .0 - .insert("the_answer".to_string(), cbor_int!(42)); - assert_eq!(extensions, Ok(expected_extensions)); - let created_cbor: cbor::Value = extensions.unwrap().into(); - assert_eq!(created_cbor, cbor_extensions); - } - - #[test] - fn test_from_into_get_assertion_hmac_secret_output() { - let cbor_output: cbor::Value = cbor_bytes![vec![0xC0; 32]]; - let output = GetAssertionHmacSecretOutput::try_from(cbor_output.clone()); - let expected_output = GetAssertionHmacSecretOutput(vec![0xC0; 32]); - assert_eq!(output, Ok(expected_output)); - let created_cbor: cbor::Value = output.unwrap().into(); - assert_eq!(created_cbor, cbor_output); - } - - #[test] - fn test_hmac_secret_extension() { + fn test_from_make_credential_extensions() { let cbor_extensions = cbor_map! { "hmac-secret" => true, + "credProtect" => CredentialProtectionPolicy::UserVerificationRequired, }; - let extensions = Extensions::try_from(cbor_extensions).unwrap(); - assert!(extensions.has_make_credential_hmac_secret().unwrap()); - - let cbor_extensions = cbor_map! { - "hmac-secret" => false, + let extensions = MakeCredentialExtensions::try_from(cbor_extensions); + let expected_extensions = MakeCredentialExtensions { + hmac_secret: true, + cred_protect: Some(CredentialProtectionPolicy::UserVerificationRequired), }; - let extensions = Extensions::try_from(cbor_extensions).unwrap(); - assert!(!extensions.has_make_credential_hmac_secret().unwrap()); + assert_eq!(extensions, Ok(expected_extensions)); + } + #[test] + fn test_from_get_assertion_extensions() { let mut rng = ThreadRng256 {}; let sk = crypto::ecdh::SecKey::gensk(&mut rng); let pk = sk.genpk(); @@ -1240,26 +1183,16 @@ mod test { 3 => vec![0x03; 32], }, }; - let extensions = Extensions::try_from(cbor_extensions).unwrap(); - let get_assertion_input = extensions.get_assertion_hmac_secret(); + let extensions = GetAssertionExtensions::try_from(cbor_extensions); let expected_input = GetAssertionHmacSecretInput { key_agreement: cose_key, salt_enc: vec![0x02; 32], salt_auth: vec![0x03; 32], }; - assert_eq!(get_assertion_input, Some(Ok(expected_input))); - } - - #[test] - fn test_cred_protect_extension() { - let cbor_extensions = cbor_map! { - "credProtect" => CredentialProtectionPolicy::UserVerificationRequired, + let expected_extensions = GetAssertionExtensions { + hmac_secret: Some(expected_input), }; - let extensions = Extensions::try_from(cbor_extensions).unwrap(); - assert_eq!( - extensions.make_credential_cred_protect_policy(), - Some(Ok(CredentialProtectionPolicy::UserVerificationRequired)) - ); + assert_eq!(extensions, Ok(expected_extensions)); } #[test] diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 8c78fe8..3809e3f 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -435,17 +435,14 @@ where } let (use_hmac_extension, cred_protect_policy) = if let Some(extensions) = extensions { - let has_hmac_secret = extensions.has_make_credential_hmac_secret()?; - let mut cred_protect = extensions - .make_credential_cred_protect_policy() - .transpose()?; + let mut cred_protect = extensions.cred_protect; if cred_protect.unwrap_or(CredentialProtectionPolicy::UserVerificationOptional) < DEFAULT_CRED_PROTECT .unwrap_or(CredentialProtectionPolicy::UserVerificationOptional) { cred_protect = DEFAULT_CRED_PROTECT; } - (has_hmac_secret, cred_protect) + (extensions.hmac_secret, cred_protect) } else { (false, None) }; @@ -546,11 +543,11 @@ where auth_data.extend(cose_key); if has_extension_output { let hmac_secret_output = if use_hmac_extension { Some(true) } else { None }; - let extensions = cbor_map_options! { + let extensions_output = cbor_map_options! { "hmac-secret" => hmac_secret_output, "credProtect" => cred_protect_policy, }; - if !cbor::write(extensions, &mut auth_data) { + if !cbor::write(extensions_output, &mut auth_data) { return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_RESPONSE_CANNOT_WRITE_CBOR); } } @@ -640,11 +637,8 @@ where } } - let get_assertion_hmac_secret_input = match extensions { - Some(extensions) => extensions.get_assertion_hmac_secret().transpose()?, - None => None, - }; - if get_assertion_hmac_secret_input.is_some() && !options.up { + let hmac_secret_input = extensions.map(|e| e.hmac_secret).flatten(); + if hmac_secret_input.is_some() && !options.up { // The extension is actually supported, but we need user presence. return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_EXTENSION); } @@ -674,7 +668,7 @@ where if options.up { flags |= UP_FLAG; } - if get_assertion_hmac_secret_input.is_some() { + if hmac_secret_input.is_some() { flags |= ED_FLAG; } @@ -721,12 +715,12 @@ where let mut auth_data = self.generate_auth_data(&rp_id_hash, flags); // Process extensions. - if let Some(get_assertion_hmac_secret_input) = get_assertion_hmac_secret_input { + if let Some(hmac_secret_input) = hmac_secret_input { let GetAssertionHmacSecretInput { key_agreement, salt_enc, salt_auth, - } = get_assertion_hmac_secret_input; + } = hmac_secret_input; let pk: crypto::ecdh::PubKey = CoseKey::try_into(key_agreement)?; let shared_secret = self.key_agreement_key.exchange_x_sha256(&pk); // HMAC-secret does the same 16 byte truncated check. @@ -741,10 +735,10 @@ where None => return Err(Ctap2StatusCode::CTAP2_ERR_UNSUPPORTED_EXTENSION), }; - let extensions = cbor_map! { + let extensions_output = cbor_map! { "hmac-secret" => encrypted_output, }; - if !cbor::write(extensions, &mut auth_data) { + if !cbor::write(extensions_output, &mut auth_data) { return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_RESPONSE_CANNOT_WRITE_CBOR); } } @@ -1119,8 +1113,8 @@ where #[cfg(test)] mod test { use super::data_formats::{ - Extensions, GetAssertionOptions, MakeCredentialOptions, PublicKeyCredentialRpEntity, - PublicKeyCredentialUserEntity, + GetAssertionExtensions, GetAssertionOptions, MakeCredentialExtensions, + MakeCredentialOptions, PublicKeyCredentialRpEntity, PublicKeyCredentialUserEntity, }; use super::*; use crypto::rng256::ThreadRng256; @@ -1220,9 +1214,10 @@ mod test { fn create_make_credential_parameters_with_cred_protect_policy( policy: CredentialProtectionPolicy, ) -> AuthenticatorMakeCredentialParameters { - let mut extension_map = BTreeMap::new(); - extension_map.insert("credProtect".to_string(), cbor::Value::from(policy)); - let extensions = Some(Extensions::new(extension_map)); + let extensions = Some(MakeCredentialExtensions { + hmac_secret: false, + cred_protect: Some(policy), + }); let mut make_credential_params = create_minimal_make_credential_parameters(); make_credential_params.extensions = extensions; make_credential_params @@ -1409,9 +1404,10 @@ mod test { let user_immediately_present = |_| Ok(()); let mut ctap_state = CtapState::new(&mut rng, user_immediately_present); - let mut extension_map = BTreeMap::new(); - extension_map.insert("hmac-secret".to_string(), cbor_bool!(true)); - let extensions = Some(Extensions::new(extension_map)); + let extensions = Some(MakeCredentialExtensions { + hmac_secret: true, + cred_protect: None, + }); let mut make_credential_params = create_minimal_make_credential_parameters(); make_credential_params.extensions = extensions; let make_credential_response = @@ -1521,9 +1517,10 @@ mod test { let user_immediately_present = |_| Ok(()); let mut ctap_state = CtapState::new(&mut rng, user_immediately_present); - let mut extension_map = BTreeMap::new(); - extension_map.insert("hmac-secret".to_string(), cbor_bool!(true)); - let make_extensions = Some(Extensions::new(extension_map)); + let make_extensions = Some(MakeCredentialExtensions { + hmac_secret: true, + cred_protect: None, + }); let mut make_credential_params = create_minimal_make_credential_parameters(); make_credential_params.extensions = make_extensions; assert!(ctap_state @@ -1531,15 +1528,15 @@ mod test { .is_ok()); let pk = sk.genpk(); - let hmac_secret_parameters = cbor_map! { - 1 => cbor::Value::Map(CoseKey::from(pk).0), - 2 => vec![0; 32], - 3 => vec![0; 16], + let hmac_secret_input = GetAssertionHmacSecretInput { + key_agreement: CoseKey::from(pk), + salt_enc: vec![0x02; 32], + salt_auth: vec![0x03; 32], }; - let mut extension_map = BTreeMap::new(); - extension_map.insert("hmac-secret".to_string(), hmac_secret_parameters); + let get_extensions = Some(GetAssertionExtensions { + hmac_secret: Some(hmac_secret_input), + }); - let get_extensions = Some(Extensions::new(extension_map)); let get_assertion_params = AuthenticatorGetAssertionParameters { rp_id: String::from("example.com"), client_data_hash: vec![0xCD],