[native] Implement native backup
ClosedPublic
Actions

Authored by michal on Nov 27 2023, 5:38 AM.

Details

Reviewers

marcin
kamil
tomek

Commits

rCOMM4fc156bb5c23: [native] Implement native backup

Summary

ENG-4678

Actually create, encrypt and decrypt the backup in native rust code. We create the backupID and pickle the crypto account in C++ CommCoreModule as this we done only on backup creation which will be triggered from js. Then we pass it to Rust which handles the encryption. The Rust logic is run on tokio runtime. While this is not strictly needed because we don't use any async functions:

we want to run encryption on a separate thread, which this gives us for free
I will be adding rust networking to this in the next month, which will use the async features

The new dependency base64 (which is used for JS<->Rust data transfer) is already used in our services codebase, at the same exact version.

Depends on D9969, D9939, D9933

Test Plan

Run this code on physical Android and iOS devices:

const password = 'password';
const backupStr = await commCoreModule.createNewBackup(
  password,
  'testing user data',
);

const { backup_id, encrypted_user_keys, encrypted_user_data } =
  JSON.parse(backupStr);

console.log(backup_id);

const decryptedUserData = await commCoreModule.restoreBackup(
  backup_id,
  password,
  encrypted_user_keys,
  encrypted_user_data,
);
const userData = base64.decode(decryptedUserData);

and check that userData === 'testing user data'.
I also triggered an error on the rust side to make sure it was handled gracefully (as a JS promise rejection).

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Nov 27 2023, 5:38 AM

Herald added a subscriber: ashoat. · View Herald TranscriptNov 27 2023, 5:38 AM

Harbormaster completed remote builds in B24505: Diff 33710.Nov 27 2023, 6:53 AM

michal requested review of this revision.Nov 27 2023, 6:53 AM

I've got a few high level questions that I left in comments near the code. I would appreciate if @kamil or @tomek, who have more experience with Rust, carefully analysed this code in terms of any unnecessary copying.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1093 ↗	(On Diff #33710)	Does this function return immediately and executes on separate thread? I think so but I just want to confirm that entire backup process doesn't run on `cryptoThread`.
1123 ↗	(On Diff #33710)	Does this function return immediately and executes on separate thread? I think so but I just want to confirm that entire backup process doesn't run on cryptoThread. Regardless of the answer I think `cryptoThread` is not the right place to call this block of code. If it is asynchronous and returns immediately we should call it right away without using auxiliary thread.
native/native_rust_library/src/backup.rs
106 ↗	(On Diff #33710)	Why don't you return anything from `user_keys`? Looks like backup of `content` olm account is never restored.

michal added inline comments.Nov 28 2023, 5:25 AM

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1093 ↗	(On Diff #33710)	Call to `::createBackup` is basically a call to `tokio::Runtime::spawn` and from docs: Spawns a future onto the Tokio runtime. This spawns the given future onto the runtime’s executor, usually a thread pool. There are two options for tokio runtime: `new_current_thread` docs, which uses the current thread `new_multi_thread` docs which creates a new threadpool We are using the second option so backup logic executes on a separate thread.
1123 ↗	(On Diff #33710)	Does this function return immediately and executes on separate thread? I think so but I just want to confirm that entire backup process doesn't run on cryptoThread. Same answer as above Regardless of the answer I think cryptoThread is not the right place to call this block of code Right, I was just copying code from `createNewBackup` without thinking, I will remove it.
native/native_rust_library/src/backup.rs
106 ↗	(On Diff #33710)	So far this code is used only for testing purposes and the current backup testing code doesn't test for correctness of olm account. But it makes sense to check it too, so I'm going to add it.

Remove cryptoThread from restore function and return restored olm account information.

michal added a child revision: D10055: [native] Increase the number of threads available for rust.Nov 28 2023, 6:54 AM

LGTM from my perspective, but please make sure to get feedback from @kamil or @tomek regarding Rust technicalities.

This revision is now accepted and ready to land.Nov 28 2023, 7:06 AM

Harbormaster completed remote builds in B24563: Diff 33938.Nov 28 2023, 7:33 AM

tomek accepted this revision.Nov 29 2023, 6:07 AM

tomek added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1093 ↗	(On Diff #33938)	Is it possible for this call to fail? Would that result in promise rejection?

michal added inline comments.Nov 29 2023, 7:15 AM

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1093 ↗	(On Diff #33938)	Yes, any failures from Rust are collected into one `Result::Err` which is then handled by the `RustPromiseManager` by rejecting the promise.