Page MenuHomePhabricator

[native] Introduce Rust encryption functions

Authored by michal on Mon, Nov 20, 9:27 AM.
Referenced Files
Unknown Object (File)
Fri, Dec 1, 12:28 PM
Unknown Object (File)
Fri, Dec 1, 6:23 AM
Unknown Object (File)
Fri, Dec 1, 5:55 AM
Unknown Object (File)
Fri, Dec 1, 3:34 AM
Unknown Object (File)
Fri, Nov 24, 7:53 AM
Unknown Object (File)
Fri, Nov 24, 7:53 AM
Unknown Object (File)
Fri, Nov 24, 7:52 AM
Unknown Object (File)
Tue, Nov 21, 2:24 AM




Rust part of the AES encryption functions. For now the platform-specific code is temporary and will be filled in the next diffs with platform-specific encryption APIs.

Test Plan

Run this code on iOS and Android:

fn test(promise_id: u32) {
  RUNTIME.spawn(async move {
    let f = move || -> Result<String, cxx::Exception> {
      let key = &mut [0; constants::aes::KEY_SIZE];
      let plaintext = &mut [1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
      let sealed_data =
        &mut [0; 10 + constants::aes::IV_LENGTH + constants::aes::TAG_LENGTH];
      ffi::encrypt(key, plaintext, sealed_data)?;
      let plaintext2 = &mut [0; 10];
      ffi::decrypt(key, sealed_data, plaintext2)?;

    Key: {key:?}
    Plaintext: {plaintext:?}
    Sealed Data: {sealed_data:?}
    Plaintext2: {plaintext2:?}"
    handle_string_result_as_callback(f(), promise_id);

Got the expected answer - the values didn't change (there is no logic yet, this is mostly to check for build issues).

Also modified AESCrypto::encrypt function to return a non-empty string (an error). This was correctly converted to an exception in C++ which was then converted to Result::Err in Rust.

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

9–17 ↗(On Diff #33433)

This return strings as an error (empty string indicates lack of an error). This is done because I wasn't able to catch Objective-C++ exceptions from C++.

Technically this differential is fine and I don't fell like requesting changes would be justified. However there are some code quality issues and I would like to know whether they are actually inevitable.

9–17 ↗(On Diff #33433)

I would ask myself whether someone will call those C++ methods from C++ or are they just meant to be called from Rust? If the first is true, then you should aim for better design - you can implement code in Objective-C that catches Objective-C exception and re-throws it as C++ exception. Alternatively you could define some struct/classes to represent errors that can be thrown from those functions.

If the second is true then perhaps you could use some dedicated mechanism to make those function impossible to call from anywhere apart from the particular Rust<-> C++ interop class.

Using string length to indicate whether public API call was successful looks brittle to me. I don't feel strong enough to request changes but I am pretty positive that alternative approaches are not to costly to implement.

13 ↗(On Diff #33433)

Are data received as rust::Slice types copied across Rust<-> c++ boundary or passed by pointers? If they are copied then is it necessary to copy or can we use another mechanism to avoid copying.

28 ↗(On Diff #33433)

Please add new line here.

2 ↗(On Diff #33433)

Are we forced to copy those constants? How much work is necessary to create functions in AECCrypto.{kt, swift, cpp} so that they can be defined once and accessed everywhere so that we have better code quailty.

Improve error handling

9–17 ↗(On Diff #33433)

They aren't supposed to be called from C++. I thought I couldn't throw error from Objective-C, because when I tried I couldn't catch them from C++, but it was my mistake. I was throwing NSException which didn't work, but I can change it to std::runtime_error and it worked! So I changed these function to void.

13 ↗(On Diff #33433)

They rust::Slice represents the same underlying memory as the Rust object, there is no copying.

2 ↗(On Diff #33433)

These constants are already copied across js (in two places - lib and native, plus a third one in web tests if we count those), kotlin and swift. I'm also not a fan of this but:

  1. Even if we add a function for returning these constant they will still be duplicated across kotlin and swift. If we do this we could also add some additional code to expose them to JS but that's more work.
  2. These specific constants will probably never change as they depend on our encryption method
  3. Maybe we should create a task to think about constants more holistically. There are other places where we duplicate constants (mostly between JS and Rust) it would be good to handle them too and maybe find a language-agnostic way of defining them.

(Also it's a bit better performance if we use constants but that doesn't really matter) If you feel strongly that we should add functions for these values, there shouldn't be a problem with adding them.

This revision is now accepted and ready to land.Mon, Nov 27, 3:05 AM

Fix mistake during rebase

This revision was landed with ongoing or failed builds.Fri, Dec 1, 6:19 AM
This revision was automatically updated to reflect the committed changes.