[client-backup] implement C++ function for computing `BackupKey`
ClosedPublic
Actions

Authored by kamil on Aug 21 2023, 4:57 AM.

Details

Reviewers

michal
marcin
varun

Commits

rCOMM470c122cbec5: [client-backup] implement C++ function for computing `BackupKey`

Summary

ENG-4512.

Note:
This is call to Rust, but it's related to cryptography, so I believe it should be executed on cryptoThread which is spawned via CommCoreModule - this is why I placed this here, not in CommRustModule. In the future, we might want to introduce new module to crypto-related things.

Depends on D8885

Test Plan

Call commCoreModule.computeBackupKey.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Aug 21 2023, 4:57 AM

kamil held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 21 2023, 4:57 AM

kamil added a child revision: D8887: [client-backup] call C++ function for secure generating random string.Aug 21 2023, 5:02 AM

Harbormaster completed remote builds in B21942: Diff 30134.Aug 21 2023, 5:15 AM

@varun, could you confirm if this is the proper way of using native_rust_library in this use case?

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
820 ↗	(On Diff #30134)	`32` is hardcoded, I don't think it's possible to use constant from Rust here, I checked this but it didn't work

marcin accepted this revision.Aug 22 2023, 1:42 AM

marcin added inline comments.

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
820 ↗	(On Diff #30134)	Even if it is not possible to access this constant from Rust it should still be defined as a named-variables to avoid magic number anti pattern.
822 ↗	(On Diff #30134)	Did you try to simulate the failure of the rust function to ensure that `std::exception` is actually thrown and caught or have some documentation that confirms this?
836 ↗	(On Diff #30134)	Why `double` if `backupKey` is vector of integers?

This revision is now accepted and ready to land.Aug 22 2023, 1:42 AM

kamil added inline comments.Aug 22 2023, 3:13 AM

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
820 ↗	(On Diff #30134)	Right, I will update this before landing
822 ↗	(On Diff #30134)	Yes, I tested it with too shoty `backupID` which cause throwing: `{"message": "Failed to compute backup key: salt is too short"}`
836 ↗	(On Diff #30134)	We use it in the codebase already (because JS numbers are represented as `double` in C++), but integer should also work, I can update this if you feel strongly it should be integer.

Would love to get @varun's approval on C++ <-> Rust integration

This revision now requires review to proceed.Aug 22 2023, 3:14 AM

LGTM. I wonder if it's possible to simplify the type conversion logic with folly/dynamic

https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h

This revision is now accepted and ready to land.Aug 29 2023, 9:54 AM

In D8886#265515, @varun wrote:

LGTM. I wonder if it's possible to simplify the type conversion logic with folly/dynamic

https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/jsi/jsi/JSIDynamic.h

Probably yes, but this approach I implemented is based on similar functions in the codebase so I will stick to the convention for now

Closed by commit rCOMM470c122cbec5: [client-backup] implement C++ function for computing `BackupKey` (authored by kamil). · Explain WhyAug 30 2023, 10:54 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM470c122cbec5: [client-backup] implement C++ function for computing `BackupKey`.