Support for importing stored data from libolm #77
Conversation
richvdh
force-pushed
the
rav/libolm_migration
branch
from
84c140a to
4ec15cb
Compare
|
@bnjbvr: Thanks for the review! Please take another look. |
src/libolm_migration.rs
Outdated
|
|
||
| /// The private, base64-encoded, master cross-signing key. | ||
| #[wasm_bindgen(js_name = "crossSigningMasterKey")] | ||
| pub cross_signing_master_key: Option<String>, |
There was a problem hiding this comment.
Maybe worth addind in the name of the field (or in the binding name) that it's the private part? Same for the 4 secrets?
| data.cross_signing_master_key.as_deref(), | ||
| data.cross_signing_self_signing_key.as_deref(), | ||
| data.cross_signing_user_signing_key.as_deref(), | ||
| ) |
There was a problem hiding this comment.
Maybe we should .zeroize() the data.cross_signing_* at this point?
There was a problem hiding this comment.
I wondered about that. I concluded it's not worthwhile, because even if we zeroize it on the Rust side, there may well still be copies kicking around in memory from the javascript side which we have no hope of zeroizing. (As https://github.com/veorq/cryptocoding#clean-memory-of-secret-data says: "there's virtually no way to reliably clean secret data in garbage-collected languages (such as ... Javascript)").
And we don't generally zeroize any other sensitive data that is passed through the wasm bindings.
| ) -> Result<JsValue, JsError> { | ||
| let pickle_key = pickle_key.to_vec(); | ||
|
|
||
| let rust_sessions = sessions |
There was a problem hiding this comment.
If one of the megolm session to import is for some reason malformed/corrupted, will the whole chunk fail to import?
Maybe it would be better to ignore "malformed" and log something but not interrupt all?
There was a problem hiding this comment.
I'm not entirely convinced. This sounds like we could end up not migrating any sessions, and the user would just be confused.
Also: once the batch is successfully imported, we delete it from the legacy store. So, if we do fail to import any sessions, we'd end up completely throwing them away, which seems very dangerous.
I'd prefer to leave it and see if any errors actually arise in practice.
There was a problem hiding this comment.
Not exacty related but we have seen sort of similar things when importing from backup, also when migrating tracked users (legacy is more lax on userIds).
Could we return a result with the succesfully migrated things? so that the caller would delete only these and handle the error on the application side?
I'll let you decide what's best
There was a problem hiding this comment.
Could we return a result with the succesfully migrated things? so that the caller would delete only these and handle the error on the application side?
Well, if we don't delete the broken sessions, then we'll just get stuck in a loop of repeatedly trying to migrate them. (There is no record of "which sessions have been migrated").)
I suppose one thing that we might think about is whether we can display an error to the user if migration goes wrong.
| /// * `store_handle` - A connection to the CryptoStore which will be used to | ||
| /// store the vodozemac data. | ||
| #[wasm_bindgen(js_name = "migrateMegolmSessions")] | ||
| pub async fn migrate_megolm_sessions( |
There was a problem hiding this comment.
This could be potentially a lot of sessions. Should we inform the caller to do that in chunks? Or have some progress reporting?
There was a problem hiding this comment.
The intention is that the application calls this with small enough batches that there is no need for an intermediate progress update.
| /// Can be imported into the rust store with {@link #migrateBaseData}. | ||
| #[wasm_bindgen(getter_with_clone)] | ||
| #[derive(Debug, Default)] | ||
| pub struct BaseMigrationData { |
There was a problem hiding this comment.
Shouldn't there be a list of tracked users here? or a migrate_tracked_users method?
There was a problem hiding this comment.
As discussed elsewhere: I don't think there's any need to migrate the tracked users.
... to emphasise that we need the private keys
Fixes #78
Based on #76