Details

Reviewers

• jon
varun
marcin
tomek

Commits

rCOMM681c33890287: [services] Tunnelbroker - Adding of gRPC `NewSession` wrapper implementation

Summary

This diff introduces the implementation of NewSession gRPC handler using the Rust Tonic gRPC server by calling the newSessionHandler C++ handler wrapper function.
The new implementation is a wrapper of the current NewSession C++ handler for using and calling from Rust gRPC server codebase.

Linear task: ENG-2059

Test Plan

1. Building:
The service was successfully built with the CI process or can be built manually using the cargo build command in services/tunnelbroker directory.

2. gRPC testings:
First, run the service using the cargo run command.

Open the gRPC client and call the SessionSignature request with the valid deviceID.
For example:

{
  "deviceID": "mobile:foS4IBADcn3uUNF8DEoH2jj6b8hujMsKUtNAKhJID4U50oalGWyg9rdLpgJwlmRy"
}

The expected result should be the toSign generated string from the server:

{
  "toSign": "7u1iiQNBlrsn0qy9BhlX8BJxttS6cBNnuCtJR1N9Mp8up56Qc4BEZWc9OvJYjlou"
}

Generate a key pair and sign the toSign string by the private key. Then call NewSession gRPC request with the following data:

{
"deviceID" = "mobile:foS4IBADcn3uUNF8DEoH2jj6b8hujMsKUtNAKhJID4U50oalGWyg9rdLpgJwlmRy"
"publicKey = "Your public key"
"signature" = "Signed `toSign` string by your private key from the previous request"
"notifyToken" = "Notification token you can leave it empty"
"deviceType" = "MOBILE"
"deviceAppVersion" = "You can leave it empty for tests"
deviceOS" = "You can leave it empty for tests"
}

The successful response should be the sessionID with the new client session identification.

In case of the wrong signed toSign string, the server will return an error: "Signature for the verification message is not valid".

Diff Detail

Repository

rCOMM Comm

Branch

add-grpc-new-session-handler

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Oct 27 2022, 6:15 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 27 2022, 6:15 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

• max added a parent revision: D5487: [services] Tunnelbroker - Adding of gRPC `SessionSignature` implementation.Oct 27 2022, 6:15 AM

Harbormaster completed remote builds in B13066: Diff 17941.Oct 27 2022, 6:27 AM

• max edited the summary of this revision. (Show Details)Oct 27 2022, 6:31 AM

• max edited the test plan for this revision. (Show Details)

• max added reviewers: varun, marcin.

• max edited the test plan for this revision. (Show Details)

Removing unused imports.

• max published this revision for review.Oct 27 2022, 6:41 AM

Harbormaster completed remote builds in B13067: Diff 17942.Oct 27 2022, 6:45 AM

Rebasing on a parent changes.

Harbormaster completed remote builds in B13077: Diff 17954.Oct 27 2022, 8:17 AM

• max added inline comments.Oct 27 2022, 9:10 AM

services/tunnelbroker/src/server/mod.rs
45–51	Unfortunately in Rust, we can't cast from int to enum (but, can do the opposite) and we must use matching (( Related links: Github issue in Rust repo, rust-lang thread. We can possibly use num-derive crate, but it's not possible to do in the generated gRPC files and CXX because of the `impl` in the `enum` which is not supported by CXX.

otherwise LGTM

services/tunnelbroker/src/server/mod.rs
41–44	if you want to make this a bit smaller

• jon added inline comments.Nov 1 2022, 11:20 AM

services/tunnelbroker/src/server/mod.rs
45–51	https://github.com/dtolnay/serde-repr or https://serde.rs/enum-number.html might be of interest, then we can offload the deserialization from our domain of responsibility.

• max added a child revision: D5526: [services] Tunnelbroker - Adding of `constants.rs` file.Nov 2 2022, 2:24 PM

Using of the shorthand of cxx_bridge calls. Using of the unwrap_or instead of matching.

Harbormaster completed remote builds in B13170: Diff 18074.Nov 4 2022, 6:44 AM

• max planned changes to this revision.Nov 4 2022, 6:53 AM

• max removed a child revision: D5526: [services] Tunnelbroker - Adding of `constants.rs` file.Nov 7 2022, 9:15 AM

• max added a child revision: D5528: [services] Tunnelbroker - Adding of the client authentication by the `sessionID` from metadata.

Rebasing on master changes.

services/tunnelbroker/src/server/mod.rs
41–44	if you want to make this a bit smaller Good catch. Thanks, @jon!
45–51	https://github.com/dtolnay/serde-repr or https://serde.rs/enum-number.html might be of interest, then we can offload the deserialization from our domain of responsibility. To use `serde_json::from` we should wrap the enum into the `#[derive..]` macro from `serde`, but the enum is auto-generated from the proto file. Quick searching shows that we can apply the `derive` macro only on the declaration and we can't do this on imported things.

• max added a reviewer: tomek.Nov 8 2022, 7:08 AM

Harbormaster completed remote builds in B13259: Diff 18194.Nov 8 2022, 7:14 AM

• max planned changes to this revision.Nov 8 2022, 8:21 AM

• max mentioned this in D5560: [services] Tunnelbroker - Changes to store the `deviceType` as an int instead of string.Nov 8 2022, 10:09 AM

• max added a parent revision: D5560: [services] Tunnelbroker - Changes to store the `deviceType` as an int instead of string.Nov 8 2022, 10:22 AM

Using of the ::prost::Enumeration instead of the string matching for the deviceTypes enum.

• max added inline comments.Nov 8 2022, 10:28 AM

services/tunnelbroker/src/server/mod.rs
45–51	https://github.com/dtolnay/serde-repr or https://serde.rs/enum-number.html might be of interest, then we can offload the deserialization from our domain of responsibility. I found that the Tonic generated headers for gRPC enums add `#[derive(::prost::Enumeration)]` and we can use it directly to match the enums by integers. We can use `is_valid()` and `from_i32()` from the `::prost::Enumeration` for our needs and no strings match anymore. The diff is updated with this.

Harbormaster failed remote builds in B13281: Diff 18223!Nov 8 2022, 10:32 AM

Harbormaster completed remote builds in B13281: Diff 18223.Nov 8 2022, 11:00 AM

tomek requested changes to this revision.Nov 9 2022, 8:44 AM

tomek added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
63 ↗	(On Diff #18223)	This looks like a new code. Why don't we write it in Rust from the beginning?
78 ↗	(On Diff #18223)	Can we avoid using magic number and instead have consts / enums for them?
100–105 ↗	(On Diff #18223)	Why do we need to check if the sent keys matches the one from the db? We could simply check the signature against the key from the db instead.

This revision now requires changes to proceed.Nov 9 2022, 8:44 AM

• max requested review of this revision.Nov 16 2022, 3:39 AM

• max marked 3 inline comments as done.

• max added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
63 ↗	(On Diff #18223)	This looks like a new code. Why don't we write it in Rust from the beginning? Not exactly. This is not a new code it's a modified gRPC handler from the C++ gRPC server. The modifications are just to pass the result to the Rust gRPC server. To rewrite gRPC handlers completely we should rewrite the database first into Rust because our database-using approach is not suitable for Rust. In this case, we should expose to Rust using the CXX a lot of the functions and structs including modifications to the C++ side (shared parameters, returns). The easier and better way in case of productivity here is to make a computation on the C++ side and just pass the result to the gRPC Rust server. When (in the future maybe) we will start to rewrite the Tunnelbroker completely to Rust, the Database shows be the first.
78 ↗	(On Diff #18223)	Can we avoid using magic number and instead have consts / enums for them? It's a gRPC status code. I don't think we should redefine gRPC codes somewhere as this is a commonly used and variable name is self-describing in this case.
100–105 ↗	(On Diff #18223)	Why do we need to check if the sent keys matches the one from the db? We could simply check the signature against the key from the db instead. It's for better feedback to the user. The user should get the different errors to better solve and know the problem with the signing. If we return the "Signature didn't match" error there are two possible errors on the user side: He has the wrong signature string or something with the signer (the key is ok) The key is don't match. So it's for better and more precise errors to the client.

Accepting, but please update the diff so that magic numbers are avoided

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
63 ↗	(On Diff #18223)	Ah, now it makes a lot of sense! Thanks for the explanation! In the future, please try to highlight facts like that and add inline comments explaining the important changes. Ideally, we would have one diff that moves code and one that changes it, but if we want the code to compile, it's not always possible.
78 ↗	(On Diff #18223)	The variable name is descriptive, but the value `3` isn't descriptive at all. We should have a place where we define a constant, give it a name and assign `3` to it. (the same for `5` and `7` - how would anyone reading the code know what's their meaning?)

This revision is now accepted and ready to land.Nov 16 2022, 10:03 AM

• max mentioned this in rCOMM32d6628a1db5: [services] Tunnelbroker - Changes to store the `deviceType` as an int instead….Nov 19 2022, 12:41 PM

Rebasing on the master and parent diffs changes.

Harbormaster failed remote builds in B13605: Diff 18634!Nov 21 2022, 2:33 AM

Rebasing and fixing merge.

• max marked 2 inline comments as done.Nov 21 2022, 2:48 AM

• max added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
78 ↗	(On Diff #18223)	The variable name is descriptive, but the value `3` isn't descriptive at all. We should have a place where we define a constant, give it a name and assign `3` to it. (the same for `5` and `7` - how would anyone reading the code know what's their meaning?) The parent diffs switched to use an enum instead, this one was rebased to use the enum as well. Marking this as fixed.