refactor: let mls_init() take ClientId directly

The FFI function already does this but we somehow didn't change the
internal function.

Author: fewerner

crypto-ffi/src/core_crypto_context/mls.rs

@@ -1,9 +1,8 @@
 use std::{sync::Arc, time::Duration};
 
 use core_crypto::{
-    Ciphersuite as CryptoCiphersuite, ClientIdentifier, CredentialFindFilters, MlsConversationConfiguration,
-    RecursiveError, VerifiableGroupInfo, mls::conversation::Conversation as _,
-    transaction_context::Error as TransactionError,
+    Ciphersuite as CryptoCiphersuite, CredentialFindFilters, MlsConversationConfiguration, RecursiveError,
+    VerifiableGroupInfo, mls::conversation::Conversation as _, transaction_context::Error as TransactionError,
 };
 use tls_codec::Deserialize as _;
 
@@ -49,10 +48,7 @@ impl CoreCryptoContext {
     pub async fn mls_init(&self, client_id: &Arc<ClientId>, transport: Arc<dyn MlsTransport>) -> CoreCryptoResult<()> {
         let transport = callback_shim(transport);
         self.inner
-            .mls_init(
-                ClientIdentifier::Basic(client_id.as_ref().as_ref().to_owned()),
-                transport,
-            )
+            .mls_init(client_id.as_ref().as_ref().to_owned(), transport)
             .await?;
         Ok(())
     }

crypto/benches/utils/mls.rs

@@ -5,10 +5,10 @@ use std::{
 
 use async_lock::RwLock;
 use core_crypto::{
-    CertificateBundle, Ciphersuite, ClientId, ClientIdentifier, ConnectionType, ConversationId, CoreCrypto,
-    Credential as CcCredential, CredentialFindFilters, CredentialRef, CredentialType, Database, DatabaseKey,
-    HistorySecret, MlsCommitBundle, MlsConversationConfiguration, MlsCryptoProvider, MlsGroupInfoBundle, MlsTransport,
-    MlsTransportData, MlsTransportResponse,
+    CertificateBundle, Ciphersuite, ClientId, ConnectionType, ConversationId, CoreCrypto, Credential as CcCredential,
+    CredentialFindFilters, CredentialRef, CredentialType, Database, DatabaseKey, HistorySecret, MlsCommitBundle,
+    MlsConversationConfiguration, MlsCryptoProvider, MlsGroupInfoBundle, MlsTransport, MlsTransportData,
+    MlsTransportResponse,
 };
 use criterion::BenchmarkId;
 use openmls::{
@@ -165,21 +165,20 @@ pub async fn new_central(
     } else {
         ConnectionType::Persistent(&path)
     };
-    let client_id = ClientId::from(Alphanumeric.sample_string(&mut rand::thread_rng(), 10).into_bytes());
-    let client_identifier = ClientIdentifier::from(client_id.clone());
+    let session_id = ClientId::from(Alphanumeric.sample_string(&mut rand::thread_rng(), 10).into_bytes());
     let db = Database::open(connection_type, &DatabaseKey::generate()).await.unwrap();
 
     let cc = CoreCrypto::new(db);
     let delivery_service = Arc::<CoreCryptoTransportSuccessProvider>::default();
     let tx = cc.new_transaction().await.unwrap();
-    tx.mls_init(client_identifier, delivery_service.clone()).await.unwrap();
+    tx.mls_init(session_id.clone(), delivery_service.clone()).await.unwrap();
     tx.finish().await.unwrap();
 
     let ctx = cc.new_transaction().await.unwrap();
 
     let credential = match certificate_bundle {
         Some(certificate_bundle) => CcCredential::x509(ciphersuite, certificate_bundle.to_owned()).unwrap(),
-        None => CcCredential::basic(ciphersuite, client_id).unwrap(),
+        None => CcCredential::basic(ciphersuite, session_id).unwrap(),
     };
     let credential_ref = ctx.add_credential(credential).await.unwrap();
 

crypto/src/ephemeral.rs

@@ -28,8 +28,8 @@ use obfuscate::{Obfuscate, Obfuscated};
 use openmls::prelude::KeyPackageSecretEncapsulation;
 
 use crate::{
-    Ciphersuite, ClientId, ClientIdRef, ClientIdentifier, CoreCrypto, CoreCryptoTransportNotImplementedProvider,
-    Credential, Error, MlsError, RecursiveError, Result, Session,
+    Ciphersuite, ClientId, ClientIdRef, CoreCrypto, CoreCryptoTransportNotImplementedProvider, Credential, Error,
+    MlsError, RecursiveError, Result, Session,
     mls_provider::{DatabaseKey, MlsCryptoProvider},
 };
 
@@ -66,10 +66,9 @@ impl Obfuscate for HistorySecret {
 /// This implementation lives here instead of there for organizational reasons.
 pub(crate) async fn generate_history_secret(ciphersuite: Ciphersuite) -> Result<HistorySecret> {
     // generate a new completely arbitrary client id
-    let client_id = uuid::Uuid::new_v4();
-    let client_id = format!("{HISTORY_CLIENT_ID_PREFIX}-{client_id}");
-    let client_id = ClientId::from(client_id.into_bytes());
-    let identifier = ClientIdentifier::Basic(client_id.clone());
+    let session_id = uuid::Uuid::new_v4();
+    let session_id = format!("{HISTORY_CLIENT_ID_PREFIX}-{session_id}");
+    let session_id = ClientId::from(session_id.into_bytes());
 
     let database = Database::open(ConnectionType::InMemory, &DatabaseKey::generate())
         .await
@@ -82,14 +81,14 @@ pub(crate) async fn generate_history_secret(ciphersuite: Ciphersuite) -> Result<
         .map_err(RecursiveError::transaction("creating new transaction"))?;
 
     let transport = Arc::new(CoreCryptoTransportNotImplementedProvider::default());
-    tx.mls_init(identifier, transport)
+    tx.mls_init(session_id.clone(), transport)
         .await
         .map_err(RecursiveError::transaction("initializing ephemeral cc"))?;
     let session = tx
         .session()
         .await
         .map_err(RecursiveError::transaction("Getting mls session"))?;
-    let credential = Credential::basic(ciphersuite, client_id.clone()).map_err(RecursiveError::mls_credential(
+    let credential = Credential::basic(ciphersuite, session_id.clone()).map_err(RecursiveError::mls_credential(
         "generating basic credential for ephemeral client",
     ))?;
     let credential_ref = tx
@@ -112,7 +111,10 @@ pub(crate) async fn generate_history_secret(ciphersuite: Ciphersuite) -> Result<
     // there
     let _ = tx.abort().await;
 
-    Ok(HistorySecret { client_id, key_package })
+    Ok(HistorySecret {
+        client_id: session_id,
+        key_package,
+    })
 }
 
 pub(crate) fn is_history_client(client_id: impl Borrow<ClientIdRef>) -> bool {

crypto/src/mls/mod.rs

@@ -110,16 +110,20 @@ mod tests {
             x509_test_chain.register_with_central(&context).await;
 
             // phase 2: init mls_client
-            let client_id = ClientId::from("alice");
+            let session_id = ClientId::from("alice");
             let identifier = match case.credential_type {
-                CredentialType::Basic => ClientIdentifier::Basic(client_id.clone()),
+                CredentialType::Basic => ClientIdentifier::Basic(session_id),
                 CredentialType::X509 => {
-                    CertificateBundle::rand_identifier(&client_id, &[x509_test_chain.find_local_intermediate_ca()])
+                    CertificateBundle::rand_identifier(&session_id, &[x509_test_chain.find_local_intermediate_ca()])
                 }
             };
+            let session_id = identifier
+                .get_id()
+                .expect("get session_id from identifier")
+                .into_owned();
             context
                 .mls_init(
-                    identifier.clone(),
+                    session_id.clone(),
                     Arc::new(CoreCryptoTransportSuccessProvider::default()),
                 )
                 .await

crypto/src/proteus.rs

@@ -637,15 +637,19 @@ mod tests {
         // proteus is initialized, prekeys can be generated
         assert!(transaction.proteus_new_prekey(1).await.is_ok());
         // 👇 and so a unique 'client_id' can be fetched from wire-server
-        let client_id = ClientId::from("alice");
+        let session_id = ClientId::from("alice");
+        let transport = Arc::new(CoreCryptoTransportSuccessProvider::default());
         let identifier = match case.credential_type {
-            CredentialType::Basic => ClientIdentifier::Basic(client_id),
+            CredentialType::Basic => ClientIdentifier::Basic(session_id),
             CredentialType::X509 => {
-                CertificateBundle::rand_identifier(&client_id, &[x509_test_chain.find_local_intermediate_ca()])
+                CertificateBundle::rand_identifier(&session_id, &[x509_test_chain.find_local_intermediate_ca()])
             }
         };
-        let transport = Arc::new(CoreCryptoTransportSuccessProvider::default());
-        transaction.mls_init(identifier.clone(), transport).await.unwrap();
+        let session_id = identifier
+            .get_id()
+            .expect("Getting session id from identifier")
+            .into_owned();
+        transaction.mls_init(session_id, transport).await.unwrap();
         let credential = Credential::from_identifier(&identifier, case.ciphersuite()).unwrap();
         let credential_ref = transaction.add_credential(credential).await.unwrap();
 

crypto/src/test_utils/mod.rs

@@ -129,8 +129,12 @@ impl SessionContext {
 
         let transaction = core_crypto.new_transaction().await.unwrap();
 
+        let session_id = identifier
+            .get_id()
+            .map_err(RecursiveError::mls_client("getting client id"))?
+            .into_owned();
         transaction
-            .mls_init(identifier.clone(), context.transport.clone())
+            .mls_init(session_id, context.transport.clone())
             .await
             .map_err(RecursiveError::transaction("mls init"))?;
 
@@ -275,9 +279,9 @@ impl SessionContext {
         self.set_session(new_session).await;
     }
 
-    pub async fn reinit_session(&self, identifier: ClientIdentifier) {
+    pub async fn reinit_session(&self, session_id: ClientId) {
         self.transaction
-            .mls_init(identifier, self.mls_transport().await)
+            .mls_init(session_id, self.mls_transport().await)
             .await
             .unwrap();
 
@@ -323,28 +327,31 @@ impl SessionContext {
     ) -> Result<()> {
         let user_uuid = uuid::Uuid::new_v4();
         let rnd_id = rand::random::<usize>();
-        let client_id = format!("{}:{rnd_id:x}@members.wire.com", user_uuid.hyphenated());
-        let client_id = ClientId(client_id.into_bytes());
+        let session_id = ClientId(format!("{}:{rnd_id:x}@members.wire.com", user_uuid.hyphenated()).into_bytes());
 
-        let credential;
-        let identifier;
-        match case.credential_type {
+        let (session_id, credential) = match case.credential_type {
             CredentialType::Basic => {
-                identifier = ClientIdentifier::Basic(client_id.clone());
-                credential = Credential::basic(case.ciphersuite(), client_id).unwrap();
+                let credential =
+                    Credential::basic(case.ciphersuite(), session_id.clone()).expect("creating basic credential ");
+
+                (session_id, credential)
             }
             CredentialType::X509 => {
-                let signer = signer.expect("Missing intermediate CA").to_owned();
-                let cert = CertificateBundle::rand(&client_id, &signer);
-                identifier = ClientIdentifier::X509([(case.signature_scheme(), cert.clone())].into());
-                credential = Credential::x509(case.ciphersuite(), cert).unwrap();
-            }
-        };
+                let signer = signer.expect("Missing intermediate CA");
+                let cert = CertificateBundle::rand(&session_id, signer);
+                let session_id = cert.get_client_id().expect("Getting client id from certificate bundle");
 
-        self.reinit_session(identifier).await;
+                let credential = Credential::x509(case.ciphersuite(), cert).expect("creating  x509 credential");
 
-        self.transaction.add_credential(credential).await.unwrap();
+                (session_id, credential)
+            }
+        };
 
+        self.reinit_session(session_id).await;
+        self.transaction
+            .add_credential(credential)
+            .await
+            .expect("adding credential");
         Ok(())
     }
 }

crypto/src/transaction_context/e2e_identity/mod.rs

@@ -7,18 +7,14 @@ mod init_certificates;
 mod rotate;
 mod stash;
 
-use std::{
-    collections::{HashMap, HashSet},
-    sync::Arc,
-};
+use std::{collections::HashSet, sync::Arc};
 
 pub use error::{Error, Result};
 use wire_e2e_identity::prelude::x509::extract_crl_uris;
 
 use super::TransactionContext;
 use crate::{
-    CertificateBundle, Ciphersuite, ClientId, ClientIdentifier, Credential, CredentialRef, E2eiEnrollment,
-    MlsTransport, RecursiveError,
+    CertificateBundle, Ciphersuite, ClientId, Credential, CredentialRef, E2eiEnrollment, MlsTransport, RecursiveError,
     e2e_identity::NewCrlDistributionPoints,
     mls::credential::{crl::get_new_crl_distribution_points, x509::CertificatePrivateKey},
 };
@@ -106,9 +102,10 @@ impl TransactionContext {
         let credential_ref = credential.save(database).await.map_err(RecursiveError::mls_credential(
             "saving credential in e2ei_mls_init_only",
         ))?;
-
-        let identifier = ClientIdentifier::X509(HashMap::from([(ciphersuite.signature_algorithm(), cert_bundle)]));
-        self.mls_init(identifier, transport)
+        let session_id = cert_bundle.get_client_id().map_err(RecursiveError::mls_credential(
+            "Getting session id from certificate bundle",
+        ))?;
+        self.mls_init(session_id, transport)
             .await
             .map_err(RecursiveError::transaction("initializing mls"))?;
         Ok((credential_ref, crl_new_distribution_points))

crypto/src/transaction_context/e2e_identity/rotate.rs

@@ -430,7 +430,7 @@ mod tests {
                 keystore.commit_transaction().await.unwrap();
                 keystore.new_transaction().await.unwrap();
 
-                alice.reinit_session(alice.identifier.clone()).await;
+                alice.reinit_session(alice.get_client_id().await).await;
 
                 let new_session = alice.session().await;
                 // Verify that Alice has the same credentials

crypto/src/transaction_context/mod.rs

@@ -14,8 +14,8 @@ use wire_e2e_identity::pki_env::PkiEnvironment;
 #[cfg(feature = "proteus")]
 use crate::proteus::ProteusCentral;
 use crate::{
-    ClientId, ClientIdentifier, CoreCrypto, CredentialFindFilters, CredentialRef, KeystoreError, MlsConversation,
-    MlsError, MlsTransport, RecursiveError, Session,
+    ClientId, CoreCrypto, CredentialFindFilters, CredentialRef, KeystoreError, MlsConversation, MlsError, MlsTransport,
+    RecursiveError, Session,
     group_store::GroupStore,
     mls::{self, HasSessionAndCrypto},
     mls_provider::{Database, MlsCryptoProvider},
@@ -261,12 +261,8 @@ impl TransactionContext {
     }
 
     /// Initializes the MLS client of [super::CoreCrypto].
-    pub async fn mls_init(&self, identifier: ClientIdentifier, transport: Arc<dyn MlsTransport>) -> Result<()> {
+    pub async fn mls_init(&self, session_id: ClientId, transport: Arc<dyn MlsTransport>) -> Result<()> {
         let database = self.database().await?;
-        let client_id = identifier
-            .get_id()
-            .map_err(RecursiveError::mls_client("getting client id"))?
-            .into_owned();
 
         let pki_env_provider = self
             .pki_environment_option()
@@ -276,7 +272,7 @@ impl TransactionContext {
 
         let crypto_provider = MlsCryptoProvider::new_with_pki_env(database, pki_env_provider);
         let database = self.database().await?;
-        let session = Session::new(client_id.clone(), crypto_provider, database, transport);
+        let session = Session::new(session_id.clone(), crypto_provider, database, transport);
         self.set_mls_session(session).await?;
 
         Ok(())

interop/src/main.rs

@@ -160,7 +160,7 @@ async fn run_mls_test(chrome_driver_addr: &std::net::SocketAddr, web_server: &st
     let transaction = cc.new_transaction().await?;
     let success_provider = Arc::new(MlsTransportSuccessProvider::default());
     transaction
-        .mls_init(master_client_id.clone().into(), success_provider.clone())
+        .mls_init(master_client_id.clone(), success_provider.clone())
         .await?;
     let credential_ref = transaction.add_credential(credential).await?;
     transaction