Encrypt and Decrypt support #39

neekolas · 2023-04-03T21:48:15Z

Summary

Trying to teach myself some Rust here. Attempted to add Encrypt and Decrypt support. This also required me to change the output type from createInboundSession and createOutboundSession to include the sessionId in the payload. I went for the Box<JsValue> type and am returning the two values as a tuple for simplicity, but I'm not sure what the best pattern is here.

Notes

I initially wrote the last test with Alice decrypting her own message. That didn't work. Is that not possible with the way we have Olm configured? The error I got was Bad MAC.
This is my first ever Rust PR. Have at me. Be ruthless. I have no idea what I'm doing.

michaelx11 · 2023-04-03T21:55:31Z

@neekolas I don't think vodozemac/olm support decryption of your own encrypted message. I tried it briefly and came to the conclusion that the way they set up their ratchet chains etc, the onus is on the library consumer to record outgoing plaintexts separately. I could be wrong here.

michaelx11

LGTM - I think we're about evenly matched already in Rust so I found nothing to be harsh about

michaelx11 · 2023-04-03T21:59:58Z

bindings/wasm/crate/src/lib.rs

+
+    let result = instance.encrypt_message_serialized(session_id, message);
+    // Do we actually need to do this?
+    instances.get(sending_handle).unwrap().set(instance);


You don't have to do this here. Instead of using take() you should be able to use get_mut() on the cell (so .get(sending_handle).ok_or(...)?.get_mut() approximately.

This pattern comes from my own lack of Rust borrow-checker knowledge when implementing the inbound/outbound session functions. I couldn't take mutable references of two INSTANCE_MAP values in the same scope so ended up doing it via Cell in a weird way.

In fact, now that we're using .public_account, I think we can do away with this pattern in those existing functions too. I might tackle that in a separate PR.

The type checker doesn't seem to like that, but it recommends that I replace the cell with a RefCell. RefCell then has a method borrow_mut. If I use that, everything is happy and I don't have to manually set the cell to updated values.

Does have some scary language about concurrent usage leading to a panic, but I think that's fine for now given that all our code is synchronous.

I've committed that change here

Sounds good, anything works for now as long as borrow/type checker likes it. We can take a holistic look at the map/cell strategy in a subsequent PR.

Actually, on second look: RefCell seems much better than the take/set strategy. Nice!

bindings/wasm/crate/src/lib.rs

michaelx11 · 2023-04-03T22:01:06Z

bindings/wasm/tests/wasmpkg.test.ts

+
+  // Alice can't decrypt her own message. Does work for Bob though
+  // const decrypted = await alice.decryptMessage(sessionId, encrypted);
+  const decrypted = await bob.decryptMessage(sessionId, encrypted);


jazzz · 2023-04-03T22:00:52Z

bindings/wasm/crate/src/lib.rs

@@ -29,7 +29,7 @@ pub fn create_outbound_session(
    sending_handle: &str,
    receiving_handle: &str,
    message: &str,
-) -> Result<String, JsValue> {


I'm not sure I follow the need for the Box here.

My understanding was that JsValues through WASM_BINDGEN get copied between the memory space?

Heap Allocating here seems unnecessary at a high level.

jazzz · 2023-04-03T22:02:23Z

bindings/wasm/crate/src/lib.rs

+        Ok((session_id, ciphertext_json)) => Ok(vec![
+            JsValue::from_str(&session_id),
+            JsValue::from_str(&ciphertext_json),
+        ]


Using the Vec here changes the memory structure. Given the size cannot be known at compile time.

Imagine using a tuple would be easier to use and would avoid the need for boxing of the values.

Unfortunately, wasm-bindgen doesn't appear to support tuples as return values. rustwasm/wasm-bindgen#122

jazzz · 2023-04-03T22:12:16Z

bindings/wasm/crate/src/lib.rs

+    match result {
+        Ok(ciphertext_json) => Ok(ciphertext_json),
+        Err(e) => Err(JsValue::from_str(&e.to_string())),
+    }


This seems fine to me, however there are more idomatic ways to use a Match statement.

Pretty sure that RHS Ok() is not needed. Values are implicitly Ok

result is an 'Result' which you are trying to map the error. You can use result.map_error(|e| JsValue::from_str(&e.to_string()))

bindings/wasm/crate/src/lib.rs

jazzz · 2023-04-03T22:26:37Z

bindings/wasm/crate/src/lib.rs

+    sending_handle: &str,
+    session_id: &str,
+    message: &str,
+) -> Result<String, JsValue> {


These types look both to be Strings, only the error gets converted to a wasmbind:JsValue. Shouldn't they be the same type?

jazzz · 2023-04-03T22:33:37Z

bindings/wasm/crate/src/lib.rs

+    let instances = INSTANCE_MAP.lock().unwrap();
+    let mut instance = instances
+        .get(sending_handle)
+        .ok_or("sending_handle not found")?


I like this conversion from an Option to Result.

You could have used a match, but this is much cleaner

jazzz · 2023-04-03T22:46:20Z

bindings/wasm/crate/src/lib.rs

@@ -20,7 +20,7 @@ lazy_static! {
 pub fn new_voodoo_instance() -> String {
    let mut instances = INSTANCE_MAP.lock().unwrap();
    let handle = (instances.len() as u64).to_string();
-    instances.insert(handle.clone(), Cell::new(VoodooInstance::new()));
+    instances.insert(handle.clone(), RefCell::new(VoodooInstance::new()));


RefCell seems odd here: Though WASM is odd so it might be the best way. Naively I would have thought we could handle borrows at compile time.

neekolas added 2 commits April 3, 2023 14:46

Initial commit

a7dc2eb

Fix failing test

fceab4a

neekolas requested a review from michaelx11 April 3, 2023 21:51

neekolas marked this pull request as ready for review April 3, 2023 21:52

neekolas requested review from jazzz and richardhuaaa April 3, 2023 21:53

michaelx11 approved these changes Apr 3, 2023

View reviewed changes

michaelx11 assigned m-005 Apr 3, 2023

neekolas added 2 commits April 3, 2023 15:27

Switch to a RefCell

4781a67

Use regular borrow when possible

efa7486

neekolas merged commit 7701394 into vodozemac_dev Apr 3, 2023

neekolas deleted the nmolnar/encrypt-and-decrypt branch April 3, 2023 22:33

jazzz approved these changes Apr 3, 2023

View reviewed changes

jazzz reviewed Apr 3, 2023

View reviewed changes

neekolas mentioned this pull request Apr 4, 2023

Return JsValue instead of string and remove match statements #41

Merged

michaelx11 mentioned this pull request Apr 20, 2023

Tracking: Technical Proof of XMTP v3 Alpha (AKA Project Mudpup) xmtp/xmtp-js#341

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypt and Decrypt support #39

Encrypt and Decrypt support #39

neekolas commented Apr 3, 2023 •

edited

Loading

michaelx11 commented Apr 3, 2023

michaelx11 left a comment

michaelx11 Apr 3, 2023

michaelx11 Apr 3, 2023

neekolas Apr 3, 2023 •

edited

Loading

neekolas Apr 3, 2023

michaelx11 Apr 3, 2023 •

edited

Loading

michaelx11 Apr 3, 2023

jazzz Apr 3, 2023

jazzz Apr 3, 2023

neekolas Apr 3, 2023

jazzz Apr 3, 2023

jazzz Apr 3, 2023

jazzz Apr 3, 2023

jazzz Apr 3, 2023

Encrypt and Decrypt support #39

Encrypt and Decrypt support #39

Conversation

neekolas commented Apr 3, 2023 • edited Loading

Summary

Notes

michaelx11 commented Apr 3, 2023

michaelx11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neekolas Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelx11 Apr 3, 2023 • edited Loading

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

neekolas commented Apr 3, 2023 •

edited

Loading

neekolas Apr 3, 2023 •

edited

Loading

michaelx11 Apr 3, 2023 •

edited

Loading