Page MenuHomePhabricator

[native] Generate device ID using Rust
ClosedPublic

Authored by inka on Aug 5 2022, 7:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM
Unknown Object (File)
Sat, Dec 14, 7:26 AM

Details

Summary

Generate unique device ID on native using rust's crates, as suggested in D4616.

Test Plan

Run cargo test in native/native_rust_library

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
.gitignore
11 ↗(On Diff #15417)

This was the only reasonable place I found for now. I figured once we start using the services there will be some subfolder handling communication with them, and this code could then probably be moved there. I could create such a subfolder now, but it seemed like an overkill, and I don't really know where it will need to be placed.

jon added inline comments.
native/cpp/CommonCpp/CryptoTools/rust-utils/Cargo.toml
10 ↗(On Diff #15417)

Since regex follows semver, "1" would be correct in the version range. But upstream seems to recommend a major minor version.

https://crates.io/crates/regex

native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
29–58 ↗(On Diff #15417)

Could use something like https://github.com/BurntSushi/quickcheck to dynamically generate input.

native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
29–58 ↗(On Diff #15417)

I don't understand what exactly you want to generate... We have three possible values of the input so...

varun requested changes to this revision.Aug 8 2022, 11:06 AM
varun added inline comments.
native/cpp/CommonCpp/CryptoTools/rust-utils/Cargo.toml
10 ↗(On Diff #15417)

I think we should just specify major and minor versions, so 0.8 and 1.6

native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
5 ↗(On Diff #15417)

I think this should just be DeviceType

33 ↗(On Diff #15417)

nit: I think it's helpful to prefix tests with the function they're testing. so in this case, something like generate_device_id_ks would be better

This revision now requires changes to proceed.Aug 8 2022, 11:06 AM

Dependencies seem reasonable

native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
5 ↗(On Diff #15417)

In D4587 I gave the same name to an 'enum' on web, so I'll create a new diff changing these names

address Varun's comments

tomek requested changes to this revision.Aug 9 2022, 1:47 AM
tomek added inline comments.
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
17–21 ↗(On Diff #15466)

It's ok now, but we can reduce the duplication a little bit more by determining prefix separately

32 ↗(On Diff #15466)

Printing all the cases is noisy. We should print the result in case of failure: to do that you can use the second and subsequent parameters of assert!.

This revision now requires changes to proceed.Aug 9 2022, 1:47 AM
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
32 ↗(On Diff #15466)

Normally the output of test that have passed is captured anyway. To see the output of non-failed tests we need to explicitly run cargo test -- --nocapture.

native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
40 ↗(On Diff #15467)

I know I could have extracted that as well, but I think it's more clear if test phases (preparation (nothing to do in this case), execution, assertion) are more or less visible in the test itself.

varun added inline comments.
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
53 ↗(On Diff #15467)

moble -> mobile

native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
17–21 ↗(On Diff #15467)

Why not just call Alphanumeric.sample_string()?

Example here: https://github.com/CommE2E/comm/blob/master/services/identity/src/token.rs#L35

tomek added inline comments.
native/cpp/CommonCpp/CryptoTools/rust-utils/src/lib.rs
40 ↗(On Diff #15467)

You can say that result: {} does not match regex {} so that the message is less ambiguous

This revision is now accepted and ready to land.Aug 9 2022, 7:25 AM

Move code to our shared rust library native/native_rust_library

inka edited the test plan for this revision. (Show Details)
inka removed a reviewer: karol.
inka removed subscribers: karol, adrian.

The code was initially moved in D5300, but it should have been done as an amend to this diff. Adding comments from D5300 that still might be relevant.

native/native_rust_library/src/crypto_tools.rs
17 ↗(On Diff #17444)

I couldn't find a nice way of using DEVICE_ID_CHAR_LENGTH for defining this regex (I mean sth like "^(ks|mobile|web):[a-zA-Z0-9]{${DEVICE_ID_CHAR_LENGTH}}$"). The best way I found is using const_format create, but I'm not sure we want do introduce another dependency for something like this...

-inka

17 ↗(On Diff #17444)

If we can separate stuff related to tests from application code then I think we should do so.

-Marcin

Originally this was in mod tests, but because of the need to keep this in sync with other files (as explained in the comment above the constant) I've put these constants that relate to some application logic next to each other, like in the other files. In rust code this is just related to tests, but in other places this regex is actually used. Eg. in services/tunnelbroker/src/Tools/Tools.cpp DEVICEID_FORMAT_REGEX from services/tunnelbroker/src/Constants.h is used validate device id passed to tunnelbroker.
I'm open to changing this if you still think it's best, just wanted to explain why I moved it.

-inka

native/native_rust_library/src/lib.rs
37–41 ↗(On Diff #17444)

Declaration outside of extern "Rust" scope is essential here since C++ would not be able to understand this type. Context is here: https://cxx.rs/shared.html. I am leaving this comment in case future reviewers get confused here.

-Marcin

81–82 ↗(On Diff #17444)

I am not requesting it but in my opinion it would be nice to use some source of separation here like a comment line: //crypto_tools or something. Unfortunately we cannot create separate module for each function set we want to generate C++ bindings for, so we could at least provide "logical" separation with comments.

-Marcin, before // Crypto Tools was added

native/native_rust_library/src/crypto_tools.rs
26–29 ↗(On Diff #17444)

Because we are using cxx we now need a wildcard for the enum. Context: cxx.rs/shared

Adding @marcin because he reviewed D5300, which changes have been moved here.
@marcin Can you please take a look at this inline, and tell me if you still want this changed?

inka requested review of this revision.Oct 10 2022, 6:31 AM

Dependencies look good

This revision is now accepted and ready to land.Oct 12 2022, 12:16 AM
This revision now requires review to proceed.Oct 12 2022, 1:37 AM
tomek added inline comments.
native/native_rust_library/src/crypto_tools.rs
27–28 ↗(On Diff #17445)

Are you sure we need to call the macro and return an Err? I guess that returning should be enough, as the caller may decide what to do with Result

42–43 ↗(On Diff #17445)

Can we find a different name for the argument? It feels like using type name might be confusing.

This revision is now accepted and ready to land.Oct 12 2022, 2:50 AM
native/native_rust_library/src/crypto_tools.rs
27–28 ↗(On Diff #17445)

I looked into it and I think we actually don't need this, sorry

This comment was removed by inka.

Address Tomek's comments

native/native_rust_library/src/crypto_tools.rs
40–41 ↗(On Diff #17510)

I renamed the variable whose only characteristic in this context is being a string to s based on function declarations in std::string::String