From 2c36c11a148af3ea6e62c591466da187f4ddaa47 Mon Sep 17 00:00:00 2001 From: beltram Date: Wed, 23 Aug 2023 11:43:25 +0200 Subject: [PATCH] fix: X509 Credential was serializing its identity whereas according to RFC9420 it mustn't --- openmls/src/credentials/mod.rs | 66 ++++++++++++++++++++++++---------- x509_credential/Cargo.toml | 2 ++ x509_credential/src/lib.rs | 47 ++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 19 deletions(-) diff --git a/openmls/src/credentials/mod.rs b/openmls/src/credentials/mod.rs index de82c53555..b5339be6db 100644 --- a/openmls/src/credentials/mod.rs +++ b/openmls/src/credentials/mod.rs @@ -35,6 +35,7 @@ mod codec; #[cfg(test)] mod tests; use errors::*; +use openmls_x509_credential::X509Ext; use x509_cert::{der::Decode, PkiPath}; use crate::ciphersuite::SignaturePublicKey; @@ -140,17 +141,39 @@ impl From for u16 { /// opaque cert_data; /// } Certificate; /// ``` -#[derive( - Debug, PartialEq, Eq, Clone, Serialize, Deserialize, TlsSerialize, TlsDeserialize, TlsSize, -)] +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct Certificate { - pub identity: VLBytes, - pub cert_data: Vec, + // TLS transient + pub identity: Vec, + pub certificates: Vec, +} + +impl tls_codec::Size for Certificate { + fn tls_serialized_len(&self) -> usize { + self.certificates.tls_serialized_len() + } +} + +impl tls_codec::Serialize for Certificate { + fn tls_serialize(&self, writer: &mut W) -> Result { + self.certificates.tls_serialize(writer) + } +} + +impl tls_codec::Deserialize for Certificate { + fn tls_deserialize(bytes: &mut R) -> Result + where + Self: Sized, + { + let certificates = Vec::>::tls_deserialize(bytes)?; + // we should not do this in a deserializer but otherwise we have to deal with a `identity: Option>` everywhere + Certificate::try_new(certificates).map_err(|_| tls_codec::Error::InvalidInput) + } } impl Certificate { pub(crate) fn pki_path(&self) -> Result { - self.cert_data.iter().try_fold( + self.certificates.iter().try_fold( PkiPath::new(), |mut acc, cert_data| -> Result { acc.push(x509_cert::Certificate::from_der(cert_data.as_slice())?); @@ -158,6 +181,20 @@ impl Certificate { }, ) } + + fn try_new(certificates: Vec>) -> Result { + let leaf = certificates + .get(0) + .ok_or(CredentialError::InvalidCertificateChain)?; + let leaf = x509_cert::Certificate::from_der(leaf)?; + let identity = leaf + .identity() + .map_err(|_| CredentialError::InvalidCertificateChain)?; + Ok(Self { + identity, + certificates: certificates.into_iter().map(|c| c.into()).collect(), + }) + } } /// MlsCredentialType. @@ -222,16 +259,10 @@ impl Credential { /// Creates and returns a new X509 [`Credential`] for the given identity. /// If the credential holds key material, this is generated and stored in /// the key store. - pub fn new_x509(identity: Vec, cert_data: Vec>) -> Result { - if cert_data.len() < 2 { - return Err(CredentialError::IncompleteCertificateChain); - } + pub fn new_x509(certificates: Vec>) -> Result { Ok(Self { credential_type: CredentialType::X509, - credential: MlsCredentialType::X509(Certificate { - identity: identity.into(), - cert_data: cert_data.into_iter().map(|c| c.into()).collect(), - }), + credential: MlsCredentialType::X509(Certificate::try_new(certificates)?), }) } @@ -239,8 +270,7 @@ impl Credential { pub fn identity(&self) -> &[u8] { match &self.credential { MlsCredentialType::Basic(basic_credential) => basic_credential.identity.as_slice(), - // TODO: implement getter for identity for X509 certificates. See issue #134. - MlsCredentialType::X509(cred) => cred.identity.as_slice(), + MlsCredentialType::X509(cert) => cert.identity.as_slice(), } } } @@ -350,9 +380,7 @@ pub mod test_utils { ) -> (CredentialWithKey, SignatureKeyPair) { let credential = match credential_type { CredentialType::Basic => Credential::new_basic(identity.into()), - CredentialType::X509 => { - Credential::new_x509(identity.into(), cert_data.unwrap()).unwrap() - } + CredentialType::X509 => Credential::new_x509(cert_data.unwrap()).unwrap(), CredentialType::Unknown(_) => unimplemented!(), }; let signature_keys = SignatureKeyPair::new( diff --git a/x509_credential/Cargo.toml b/x509_credential/Cargo.toml index f141e089a9..c520192af1 100644 --- a/x509_credential/Cargo.toml +++ b/x509_credential/Cargo.toml @@ -13,6 +13,8 @@ async-trait = { workspace = true } serde = "1.0" openmls_basic_credential = { version = "0.2.0", path = "../basic_credential" } fluvio-wasm-timer = "0.2" +base64 = "0.21" +uuid = "1.4" # Rust Crypto secrecy = { version = "0.8", features = ["serde"] } diff --git a/x509_credential/src/lib.rs b/x509_credential/src/lib.rs index c7c4093951..5156bc681f 100644 --- a/x509_credential/src/lib.rs +++ b/x509_credential/src/lib.rs @@ -2,6 +2,7 @@ //! //! An implementation of the x509 credential from the MLS spec. +use base64::Engine; use openmls_basic_credential::SignatureKeyPair; use x509_cert::der::Decode; use x509_cert::Certificate; @@ -100,6 +101,8 @@ pub trait X509Ext { backend: &impl OpenMlsCrypto, issuer: &Certificate, ) -> Result<(), CryptoError>; + + fn identity(&self) -> Result, CryptoError>; } impl X509Ext for Certificate { @@ -180,4 +183,48 @@ impl X509Ext for Certificate { ) .map_err(|_| CryptoError::InvalidSignature) } + + fn identity(&self) -> Result, CryptoError> { + let extensions = self + .tbs_certificate + .extensions + .as_ref() + .ok_or(CryptoError::InvalidCertificate)?; + let san = extensions + .iter() + .find(|e| { + e.extn_id.as_bytes() == oid_registry::OID_X509_EXT_SUBJECT_ALT_NAME.as_bytes() + }) + .and_then(|e| { + x509_cert::ext::pkix::SubjectAltName::from_der(e.extn_value.as_bytes()).ok() + }) + .ok_or(CryptoError::InvalidCertificate)?; + san.0 + .iter() + .filter_map(|n| match n { + x509_cert::ext::pkix::name::GeneralName::UniformResourceIdentifier(ia5_str) => { + Some(ia5_str.as_str()) + } + _ => None, + }) + .filter(|n| n.starts_with("im:wireapp=")) + .find_map(parse_client_id) + .map(|i| i.as_bytes().to_vec()) + .ok_or(CryptoError::InvalidCertificate) + } +} + +fn parse_client_id(client_id: &str) -> Option<&str> { + let (user_id, rest) = client_id.split_once('/')?; + parse_user_id(user_id)?; + let (device_id, _domain) = rest.split_once('@')?; + u64::from_str_radix(device_id, 16).ok()?; + Some(client_id) +} + +fn parse_user_id(user_id: impl AsRef<[u8]>) -> Option { + let user_id = base64::prelude::BASE64_URL_SAFE_NO_PAD + .decode(user_id) + .ok()?; + uuid::Uuid::from_slice(&user_id).ok() }