WebR Expose matrix-sdk-crypto-js bindings for KeyBackup #2196

BillCarsonFr · 2023-07-01T09:21:03Z

Expose matrix-sdk-crypto-js bindings for:

BackupMachine, including BackupMachine::backup_key::version, which is not currently exposed in the underlying crate
RoomKeyBackupInfo (? actually perhaps just pass the raw json into verify_backup).
MegolmV1BackupKey (this is the public part of the key)
RecoveryKey (this is the private part of the key)
SignatureVerification
Signatures::serialize (for signing the backup)

Fixes: https://github.com/vector-im/crypto-internal/issues/100

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2023-07-04T08:56:43Z

Codecov Report

Patch coverage: 15.38% and project coverage change: -0.05 ⚠️

Comparison is base (7bcc886) 76.71% compared to head (d6b30ae) 76.67%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2196      +/-   ##
==========================================
- Coverage   76.71%   76.67%   -0.05%     
==========================================
  Files         164      164              
  Lines       17646    17658      +12     
==========================================
+ Hits        13538    13540       +2     
- Misses       4108     4118      +10

Impacted Files	Coverage Δ
crates/matrix-sdk-crypto/src/store/memorystore.rs	`77.12% <15.38%> (-5.15%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

poljar

There are a bunch of unwraps that we need to get rid of and a couple of more nits. Looks promising though, thanks.

poljar · 2023-07-07T07:11:09Z

bindings/matrix-sdk-crypto-js/src/backup_recovery_key.rs

+            .map(|(k, v)| (k.to_string(), v.into_iter().map(|(k, v)| (k.to_string(), v)).collect()))
+            .collect();
+
+        serde_wasm_bindgen::to_value(&signatures).unwrap()


Left an unwrap here.

I'm far from convinced that we need this method anyway. What is the application supposed to do with a list of signatures once it has one? It should be up to the rust side to validate the signatures.

Will remove this for now.

poljar · 2023-07-07T07:11:44Z

bindings/matrix-sdk-crypto-js/src/backup_recovery_key.rs

+        self.inner
+            .decrypt_v1(&ephemeral_key, &mac, &ciphertext)
+            .map_err(|e| e.into())
+            .map(|r| JSON::parse(&r).unwrap())


Another one. Hmm, this now assumes that whatever you are decrypting is JSON, I guess that's currently true but worries me slightly.

let's just return the JSON. The javascript side can deserialise it more easily anyway.

poljar · 2023-07-07T07:15:45Z

bindings/matrix-sdk-crypto-js/src/machine.rs

+                recovery_key: inner.recovery_key.map(|k| k.to_base58()),
+                backup_version: inner.backup_version,
+            };
+            Ok(serde_wasm_bindgen::to_value(&backup_keys).unwrap())


More unwraps.

poljar · 2023-07-07T07:19:29Z

bindings/matrix-sdk-crypto-js/src/types.rs

+    pub fn as_json(&self) -> JsValue {
+        trace!(?self.inner, "The signature");
+        // can't use directly serde_wasm_bindgen as there is an issue with BTreeMap
+        // It's always returning an empty {} if I do:


Hmm, but you collect into a BTreeMap a couple of lines bellow as well, in fact the collect is almost the same thing the Serialize implementation does for the Signatures struct.

If serde_wasm_bindgen didn't work, did serde_json::to_string() work correctly? I don't see how that would be different to what you're doing here.

poljar · 2023-07-07T07:21:29Z

crates/matrix-sdk-crypto/src/store/memorystore.rs

+                // can't clone or copy it, so..
+                let key = export.recovery_key.as_ref().map(|e| serde_json::to_string(e).unwrap());
+                BackupKeys {
+                    recovery_key: key.map(|s| serde_json::from_str(&s).unwrap()),


Heh, I had to add this in #2227 as well. Though I added just a clone to the backup key.

So let's do that instead.

poljar · 2023-07-07T07:22:18Z

bindings/matrix-sdk-crypto-js/src/backup_recovery_key.rs

+
+    /// Create a new [`BackupRecoveryKey`] from the given passphrase.
+    #[wasm_bindgen(js_name = "newFromPassphrase")]
+    pub fn new_from_passphrase(passphrase: String) -> BackupRecoveryKey {


Are we certain that EW needs this? If so perhaps we should give in and put it into the crypto crate instead of having it twice in the bindings.

I didn't see it in web code, we can probably reove

jplatte · 2023-07-13T11:11:45Z

The bindings have moved to a separate repo, you'll have to re-open the PR there: https://github.com/matrix-org/matrix-rust-sdk-crypto-web. Sorry for the inconvenience.

richvdh · 2023-07-19T11:51:40Z

bindings/matrix-sdk-crypto-js/src/machine.rs

@@ -763,6 +767,155 @@ impl OlmMachine {
        }))
    }

+    /// Store the recovery key in the crypto store.


IMHO we should not use the term "recovery key" here: matrix-org/matrix-spec#1571

richvdh · 2023-07-20T11:50:25Z

bindings/matrix-sdk-crypto-js/src/types.rs

+
+        let raw_string = serde_json::to_string(&map).unwrap();
+
+        JSON::parse(&raw_string).unwrap()


why is it called asJSON if it's not returning JSON?

richvdh · 2023-07-20T11:52:57Z

bindings/matrix-sdk-crypto-js/src/types.rs

+    /// Get the json with all signatures
+    #[wasm_bindgen(js_name = "asJSON")]
+    pub fn as_json(&self) -> JsValue {
+        trace!(?self.inner, "The signature");
+        // can't use directly serde_wasm_bindgen as there is an issue with BTreeMap
+        // It's always returning an empty {} if I do:
+        // serde_wasm_bindgen::to_value(&self.inner).unwrap()
+
+        // Keep it like that for now as it's working
+        let map: BTreeMap<String, BTreeMap<String, String>> = self
+            .inner
+            .clone()
+            .into_iter()
+            .map(|(u, sign)| {
+                (
+                    u.as_str().to_owned(),
+                    sign.iter()
+                        .map(|(device, maybe_sign)| {
+                            (
+                                device.as_str().to_owned(),
+                                match maybe_sign {
+                                    Ok(s) => s.to_base64(),
+                                    Err(e) => e.source.to_owned(),
+                                },
+                            )
+                        })
+                        .collect(),
+                )
+            })
+            .collect();
+
+        let raw_string = serde_json::to_string(&map).unwrap();
+
+        JSON::parse(&raw_string).unwrap()
+    }


Again, I'm not really sure why we need this. What is the JS side supposed to do with it?

Answer: we have to assemble the POST /_matrix/client/v3/room_keys/version request on the application side, and we need the signature data for that.

Import Valere's changes from matrix-org/matrix-rust-sdk#2196, excluding the fixes to MemoryStore.

BillCarsonFr marked this pull request as ready for review July 1, 2023 11:33

BillCarsonFr requested a review from a team as a code owner July 1, 2023 11:33

BillCarsonFr requested review from poljar and removed request for a team July 1, 2023 11:33

BillCarsonFr force-pushed the valere/js_backup_bindings branch from 0f9faf7 to 00d73db Compare July 3, 2023 07:07

BillCarsonFr added 9 commits July 4, 2023 10:38

recovery key bindings

640ea63

backup management

09b8139

save and restore recovery key

5c89b6b

prettier

2d962bf

format

b9d038f

fix memory store not storing backup key

17477be

fix doc

fbb9941

fixup style

dca398e

clippy

d6b30ae

BillCarsonFr force-pushed the valere/js_backup_bindings branch from 00d73db to d6b30ae Compare July 4, 2023 08:38

BillCarsonFr mentioned this pull request Jul 6, 2023

rust backup support ground work matrix-org/matrix-js-sdk#3548

Closed

3 tasks

poljar requested changes Jul 7, 2023

View reviewed changes

jplatte closed this Jul 13, 2023

AndrewFerr mentioned this pull request Jul 14, 2023

Add bindings for room key backup & recovery matrix-org/matrix-rust-sdk-crypto-nodejs#7

Merged

richvdh changed the title ~~WebR Expose matrix-sdk-crypto-js bindings for KeyBackup#100~~ WebR Expose matrix-sdk-crypto-js bindings for KeyBackup Jul 19, 2023

richvdh reviewed Jul 19, 2023

View reviewed changes

richvdh reviewed Jul 20, 2023

View reviewed changes

richvdh added a commit to matrix-org/matrix-rust-sdk-crypto-wasm that referenced this pull request Jul 20, 2023

matrix-sdk-crypto-wasm bindings for KeyBackup

cb06c51

Import Valere's changes from matrix-org/matrix-rust-sdk#2196, excluding the fixes to MemoryStore.

richvdh mentioned this pull request Jul 20, 2023

Expose WASM bindings for key backup matrix-org/matrix-rust-sdk-crypto-wasm#10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebR Expose matrix-sdk-crypto-js bindings for KeyBackup #2196

WebR Expose matrix-sdk-crypto-js bindings for KeyBackup #2196

BillCarsonFr commented Jul 1, 2023

codecov bot commented Jul 4, 2023

poljar left a comment

poljar Jul 7, 2023

richvdh Jul 19, 2023

poljar Jul 7, 2023

richvdh Jul 19, 2023

poljar Jul 7, 2023

poljar Jul 7, 2023

poljar Jul 7, 2023

poljar Jul 7, 2023

BillCarsonFr Jul 8, 2023

jplatte commented Jul 13, 2023

richvdh Jul 19, 2023

richvdh Jul 20, 2023

richvdh Jul 20, 2023

richvdh Jul 20, 2023


		let raw_string = serde_json::to_string(&map).unwrap();

		JSON::parse(&raw_string).unwrap()

WebR Expose matrix-sdk-crypto-js bindings for KeyBackup #2196

WebR Expose matrix-sdk-crypto-js bindings for KeyBackup #2196

Conversation

BillCarsonFr commented Jul 1, 2023

codecov bot commented Jul 4, 2023

Codecov Report

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment