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.
Details
Details
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];
ffi::generate_key(key)?;
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)?;
Ok(format!(
"
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/Tools/AESCrypto.h | ||
|---|---|---|
| 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++. |
Comment Actions
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.
| native/cpp/CommonCpp/Tools/AESCrypto.h | ||
|---|---|---|
| 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. |
| native/native_rust_library/RustAESCrypto.cpp | ||
| 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. |
| native/native_rust_library/src/constants.rs | ||
| 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. |
Comment Actions
Improve error handling
| native/cpp/CommonCpp/Tools/AESCrypto.h | ||
|---|---|---|
| 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. |
| native/native_rust_library/RustAESCrypto.cpp | ||
| 13 ↗ | (On Diff #33433) | They rust::Slice represents the same underlying memory as the Rust object, there is no copying. |
| native/native_rust_library/src/constants.rs | ||
| 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:
(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. |