Fix some race conditions between the add-app and update procedures.

Author: arrdalan

app_native/src/lib.rs

@@ -718,7 +718,8 @@ pub fn process_heartbeat_config_response(
 
                     for i in 0..add_app_resps_com.len() {
                         if i < NUM_COMMON_MLS_CLIENTS {
-                            // Merge the psk_proposal and commit for the add operation
+                            // Store update proposals, merge the psk_proposal, and commit for the add operation
+                            clients.as_mut().unwrap().mls_clients[i].store_update_proposals(add_app_resps_com[i].update_proposals_vec.clone()).unwrap();
                             clients.as_mut().unwrap().mls_clients[i].decrypt(add_app_resps_com[i].psk_proposal_vec.clone(), false).unwrap();
                             clients.as_mut().unwrap().mls_clients[i].decrypt_with_secret(add_app_resps_com[i].commit_msg_vec.clone(), false, secret.clone()).unwrap();
                             clients.as_mut().unwrap().mls_clients[i].save_group_state().unwrap();
@@ -827,7 +828,8 @@ pub fn process_add_app_config_response(
 
                     let new_app_data: [NewAppData; NUM_MLS_CLIENTS] = std::array::from_fn(|i| {
                         if i < NUM_COMMON_MLS_CLIENTS {
-                            // Merge the psk_proposal and commit for the add operation
+                            // Store update proposals, merge the psk_proposal, and commit for the add operation
+                            clients.as_mut().unwrap().mls_clients[i].store_update_proposals(add_app_resps_com[i].update_proposals_vec.clone()).unwrap();
                             clients.as_mut().unwrap().mls_clients[i].decrypt(add_app_resps_com[i].psk_proposal_vec.clone(), false).unwrap();
                             clients.as_mut().unwrap().mls_clients[i].decrypt_with_secret(add_app_resps_com[i].commit_msg_vec.clone(), false, secret.clone()).unwrap();
                             clients.as_mut().unwrap().mls_clients[i].save_group_state().unwrap();

camera_hub/src/config.rs

@@ -161,6 +161,8 @@ fn handle_add_app_request(
             )
             .unwrap();
 
+            let update_proposals_vec = clients_com[i].get_update_proposals().unwrap();
+
             let (welcome_msg_vec, psk_proposal_vec, commit_msg_vec) = clients_com[i]
                 .invite_with_secret(&camera_contact, secret.clone())
                 .unwrap();
@@ -172,6 +174,7 @@ fn handle_add_app_request(
                 welcome_msg_vec,
                 psk_proposal_vec,
                 commit_msg_vec,
+                update_proposals_vec,
             }
         });
 

client_lib/src/config.rs

@@ -4,7 +4,7 @@
 
 use crate::mls_clients::{MlsClients, MLS_CLIENT_TAGS, MOTION, NUM_MLS_CLIENTS, THUMBNAIL,
     MlsClientsCommon, MlsClientsDedicated, NUM_COMMON_MLS_CLIENTS, LIVESTREAM_DED};
-use openmls::prelude::KeyPackage;
+use openmls::prelude::{KeyPackage, QueuedProposal};
 use log::{error, info};
 use serde::{Deserialize, Serialize};
 use std::io;
@@ -67,13 +67,17 @@ impl HeartbeatRequest {
             if MLS_CLIENT_TAGS[i] == "motion"
                 || MLS_CLIENT_TAGS[i] == "thumbnail"
             {
+                // It is possible that the update proposal races with an add app commit and end up
+                // being in an older epoch. We just ignore such update proposals. The app will generate
+                // another update in the next heartbeat.
                 let _ =
-                    clients_com[i].decrypt(self.update_proposals[proposals_i].clone(), false)?;
+                    clients_com[i].decrypt(self.update_proposals[proposals_i].clone(), false);
                 clients_com[i].save_group_state().unwrap();
                 proposals_i += 1;
             } else if MLS_CLIENT_TAGS[i] == "livestream" {
+                // Same comment here as above.
                 let _ =
-                    clients_ded[i - NUM_COMMON_MLS_CLIENTS].decrypt(self.update_proposals[proposals_i].clone(), false)?;
+                    clients_ded[i - NUM_COMMON_MLS_CLIENTS].decrypt(self.update_proposals[proposals_i].clone(), false);
                 clients_ded[i - NUM_COMMON_MLS_CLIENTS].save_group_state().unwrap();
                 proposals_i += 1;
             }
@@ -229,6 +233,7 @@ pub struct AddAppResponseCommon {
     pub welcome_msg_vec: Vec<u8>,
     pub psk_proposal_vec: Vec<u8>,
     pub commit_msg_vec: Vec<u8>,
+    pub update_proposals_vec: Vec<QueuedProposal>,
 }
 
 #[derive(Serialize, Deserialize)]

client_lib/src/mls_client.rs

@@ -283,6 +283,15 @@ impl MlsClient {
             group.mls_group.set_aad(group_aad.as_bytes().to_vec());
         }
 
+        for contact in &mut group.contacts {
+            if let Some(proposal) = contact.update_proposal.take() {
+                group
+                    .mls_group
+                    .store_pending_proposal(self.provider.storage(), proposal)
+                    .map_err(|e| io::Error::other(format!("Error: could not store proposal - {e}")))?;
+            }
+        }
+
         // Build a proposal with this key package and do the MLS bits.
         let joiner_key_package = contact.key_package.clone();
 
@@ -1021,7 +1030,7 @@ impl MlsClient {
                     || !(staged_commit.psk_proposals().next().is_none()
                         || staged_commit.psk_proposals().collect::<Vec<_>>().len() == 1)
                     || !(staged_commit.queued_proposals().next().is_none()
-                        || staged_commit.queued_proposals().collect::<Vec<_>>().len() <= cmp::max(2, num_apps_in_group))
+                        || staged_commit.queued_proposals().collect::<Vec<_>>().len() <= cmp::max(2, num_apps_in_group + 2)) // 1 psk, 1 add, 1 update per member
                 {
                     return Err(io::Error::other(
                         "Error: staged commit message must contain at most one update/queued proposal and no other proposals.",

client_lib/src/tests.rs

@@ -11,6 +11,7 @@ mod tests {
     use crate::video::{encrypt_video_file, decrypt_video_file,
         encrypt_thumbnail_file, decrypt_thumbnail_file};
     use crate::thumbnail_meta_info::ThumbnailMetaInfo;
+    use openmls::prelude::QueuedProposal;
     use std::fs::{self, File};
     use std::io;
     use std::io::{Read, Write};
@@ -587,14 +588,14 @@ mod tests {
         assert!(msg == msg_dec);
     }
 
-    /// This function is a complete, successful pairing process with built-in secrets.
+    /// This function is a complete, successful pairing process with the first
+    /// secondary app (app2) with built-in secrets.
     /// It is used in other tests.
-    fn pair_with_two_more_apps(
+    fn pair_with_app2(
         camera: &mut MlsClient,
         app: &mut MlsClient,
-    ) -> (MlsClient, MlsClient) {
+    ) -> MlsClient {
         fs::create_dir("test_data/app2").unwrap();
-        fs::create_dir("test_data/app3").unwrap();
 
         // Add the second app
         let mut app2 = MlsClient::new(
@@ -616,15 +617,28 @@ mod tests {
         let (welcome_msg_vec, psk_proposal_vec, commit_msg_vec) = camera
             .invite_with_secret(&camera_contact, new_secret.clone()).unwrap();
         camera.save_group_state().unwrap();
+        let update_proposals = camera.get_update_proposals().unwrap();
 
         app2.process_welcome_with_secret(app2_contact, welcome_msg_vec, new_secret.clone(), GROUP_NAME).unwrap();
         app2.save_group_state().unwrap();
 
         // App merges the psk_proposal and commit for the add operation
+        app.store_update_proposals(update_proposals).unwrap();
         app.decrypt(psk_proposal_vec, false).unwrap();
         app.decrypt_with_secret(commit_msg_vec, false, new_secret).unwrap();
         app.save_group_state().unwrap();
 
+        app2
+    }
+
+    /// This function is the first step of adding the second secondary app (app3).
+    /// It is used in other tests.
+    fn pair_with_app3_handshake(
+        camera: &mut MlsClient,
+        app: &mut MlsClient,
+    ) -> (MlsClient, Vec<u8>, Vec<u8>, Vec<QueuedProposal>) {
+        fs::create_dir("test_data/app3").unwrap();
+
         // Add the third app
         let mut app3 = MlsClient::new(
             "app3".to_string(),
@@ -641,21 +655,49 @@ mod tests {
 
         let new_secret = vec![3u8; NUM_SECRET_BYTES];
 
+        let update_proposals = camera.get_update_proposals().unwrap();
         let (welcome_msg_vec, psk_proposal_vec, commit_msg_vec) = camera
             .invite_with_secret(&camera_contact, new_secret.clone()).unwrap();
         camera.save_group_state().unwrap();
 
         app3.process_welcome_with_secret(app3_contact, welcome_msg_vec, new_secret.clone(), GROUP_NAME).unwrap();
         app3.save_group_state().unwrap();
 
+        app.store_update_proposals(update_proposals.clone()).unwrap();
         app.decrypt(psk_proposal_vec.clone(), false).unwrap();
         app.decrypt_with_secret(commit_msg_vec.clone(), false, new_secret.clone()).unwrap();
         app.save_group_state().unwrap();
+
+        (app3, psk_proposal_vec, commit_msg_vec, update_proposals)
+    }
+
+    /// This function is the second step of adding the second secondary app (app3).
+    /// It is used in other tests.
+    fn pair_with_app3_inform_app2(
+        app2: &mut MlsClient,
+        psk_proposal_vec: Vec<u8>,
+        commit_msg_vec: Vec<u8>,
+        update_proposals: Vec<QueuedProposal>,
+    ) {
+        // Must be the same as the one in pair_with_app3_handshake()
+        let new_secret = vec![3u8; NUM_SECRET_BYTES];
 
+        app2.store_update_proposals(update_proposals).unwrap();
         app2.decrypt(psk_proposal_vec, false).unwrap();
         app2.decrypt_with_secret(commit_msg_vec, false, new_secret).unwrap();
         app2.save_group_state().unwrap();
+    }
 
+    /// This function is a complete, successful pairing process with built-in secrets.
+    /// It is used in other tests.
+    fn pair_with_two_more_apps(
+        camera: &mut MlsClient,
+        app: &mut MlsClient,
+    ) -> (MlsClient, MlsClient) {
+        let mut app2 = pair_with_app2(camera, app);
+        let (app3, psk_proposal_vec, commit_msg_vec, update_proposals) = pair_with_app3_handshake(camera, app);
+        pair_with_app3_inform_app2(&mut app2, psk_proposal_vec, commit_msg_vec, update_proposals);
+        
         (app2, app3)
     }
 
@@ -1456,4 +1498,150 @@ mod tests {
             check_decrypted_dummy_file(&dec_thumbnail_pathname, file_size);
         }
     }
+
+    /// The input app generates an update proposal and returns it.
+    fn app_update(
+        app: &mut MlsClient,
+    ) -> Vec<u8> {
+        let update_proposal = app.update_proposal().unwrap();
+        app.save_group_state().unwrap();
+
+        update_proposal
+    }
+
+    /// Camera receives the update proposal.
+    fn camera_receive_update_proposal(
+        camera: &mut MlsClient,
+        update_proposal: Vec<u8>,
+    ) {
+        camera.decrypt(update_proposal, false).unwrap();
+        camera.save_group_state().unwrap();
+    }
+
+    /// Camera receives the update proposal.
+    fn camera_receive_update_proposal_ignore_old(
+        camera: &mut MlsClient,
+        update_proposal: Vec<u8>,
+    ) {
+        let _ = camera.decrypt(update_proposal, false);
+        camera.save_group_state().unwrap();
+    }
+
+    /// Decrypts an encrypted file and check the decrypted file.
+    fn decrypt_and_check_file(
+        app: &mut MlsClient,
+        name: &str,
+        enc_video_pathname: &str,
+        file_size: usize,
+    ) {
+        // App decrypts video file
+        let dir = format!("test_data/{}/videos", name);
+        fs::create_dir_all(&dir).unwrap();
+
+        let dec_video_filename = decrypt_video_file(
+            app,
+            enc_video_pathname,
+        ).unwrap();
+
+        let dec_video_pathname = format!("{}/{}", dir, dec_video_filename);
+
+        // Check decrypted file
+        check_decrypted_dummy_file(&dec_video_pathname, file_size);
+    }
+
+    #[test]
+    /// Functionally, this test is supposed to be similar
+    /// to camera_to_more_apps_update_video_test.
+    /// However, it is one of the tests that shuffle
+    /// the operations to test race conditions that we find.
+    /// In this specific test, app3 is added after updates by other apps,
+    /// but before a video is encrypted.
+    fn camera_to_more_apps_update_video_race_test_1() {
+        let (mut camera, mut app) = pair();
+        //let (mut app2, mut app3) = pair_with_two_more_apps(&mut camera, &mut app);
+        let mut app2 = pair_with_app2(&mut camera, &mut app);
+
+        // app1 update
+        let update_proposal = app_update(&mut app);
+        camera_receive_update_proposal(&mut camera, update_proposal);
+
+        // app2 update
+        let update_proposal = app_update(&mut app2);
+        camera_receive_update_proposal(&mut camera, update_proposal);
+        
+        // Create input video file to be encrypted (all 0's)
+        let video_pathname = "test_data/video_file";
+        let file_size: usize = 96 * 1024 + 135;
+
+        generate_dummy_file(video_pathname, file_size);
+
+        // Add app3
+        let (mut app3, psk_proposal_vec, commit_msg_vec, update_proposals) = pair_with_app3_handshake(&mut camera, &mut app);
+        pair_with_app3_inform_app2(&mut app2, psk_proposal_vec, commit_msg_vec, update_proposals);
+
+        // Camera encrypts video file
+        let enc_video_pathname = "test_data/enc_video_file";
+
+        encrypt_video_file(
+            &mut camera,
+            video_pathname,
+            enc_video_pathname,
+            0,
+        ).unwrap();
+
+        decrypt_and_check_file(&mut app, "app", enc_video_pathname, file_size);
+        decrypt_and_check_file(&mut app2, "app2", enc_video_pathname, file_size);
+        decrypt_and_check_file(&mut app3, "app3", enc_video_pathname, file_size);
+    }
+
+    #[test]
+    /// Functionally, this test is supposed to be similar
+    /// to camera_to_more_apps_update_video_test.
+    /// However, it is one of the tests that shuffle
+    /// the operations to test race conditions that we find.
+    /// In this specific test, app2 is informed of the addition of
+    /// app3 after app performs an update.
+    fn camera_to_more_apps_update_video_race_test_2() {
+        let (mut camera, mut app) = pair();
+        //let (mut app2, mut app3) = pair_with_two_more_apps(&mut camera, &mut app);
+        let mut app2 = pair_with_app2(&mut camera, &mut app);
+        let (mut app3, psk_proposal_vec, commit_msg_vec, update_proposals) = pair_with_app3_handshake(&mut camera, &mut app);
+
+        // app1 update
+        let update_proposal = app_update(&mut app);
+        camera_receive_update_proposal(&mut camera, update_proposal);
+
+        // app2 update
+        // NOTE: diagnosis: update generated by app2 is on an old epoch.
+        // This is currently prevented by fetching config response before generating a new heartbeat.
+        // Can there be a race condition there? What if config response hasn't been submitted yet?
+        let update_proposal = app_update(&mut app2);
+
+        pair_with_app3_inform_app2(&mut app2, psk_proposal_vec, commit_msg_vec, update_proposals);
+        camera_receive_update_proposal_ignore_old(&mut camera, update_proposal);
+
+        // app3 update
+        let update_proposal = app_update(&mut app3);
+        camera_receive_update_proposal(&mut camera, update_proposal);
+
+        // Create input video file to be encrypted (all 0's)
+        let video_pathname = "test_data/video_file";
+        let file_size: usize = 96 * 1024 + 135;
+
+        generate_dummy_file(video_pathname, file_size);
+
+        // Camera encrypts video file
+        let enc_video_pathname = "test_data/enc_video_file";
+
+        encrypt_video_file(
+            &mut camera,
+            video_pathname,
+            enc_video_pathname,
+            0,
+        ).unwrap();
+
+        decrypt_and_check_file(&mut app, "app", enc_video_pathname, file_size);
+        decrypt_and_check_file(&mut app2, "app2", enc_video_pathname, file_size);
+        decrypt_and_check_file(&mut app3, "app3", enc_video_pathname, file_size);
+    }
 }