Details

Reviewers

• jon
marcin
tomek

Commits

rCOMM56ac25fc5332: [services] Tunnelbroker - Adding of the client authentication by the…

Summary

This diff introduces using of the metadata to provide the sessionID by the client for the client authentication in the gRPC MessagesStream bidirectional stream.

We are making the following security checks with the sessionID:

Check if the sessionID is provided in metadata;
Check the sessionID format validity;
Check if the sessionID exists in the database session table.

Linear task: ENG-1359

Test Plan

Opening a MessagesStream from the gRPC client without providing the sessionID in metadata results in gRPC invalid argument error.
Opening a MessagesStream from the gRPC client providing the sessionID in a wrong format in metadata results in gRPC unauthenticated error.
Opening a MessagesStream from the gRPC client providing the unexistent sessionID in metadata results in gRPC unauthenticated error.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Nov 2 2022, 2:50 PM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 2 2022, 2:50 PM

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

• max added parent revisions: D5527: [services] Tunnelbroker - Adding `tokio-stream` dependency to the Cargo, D5525: [services] Tunnelbroker - Removing of 'sessionID' from 'MessageToTunnelbroker'.Nov 2 2022, 2:50 PM

• max added a child revision: D5529: [services] Tunnelbroker - Adding the pinging loop and online status of the client check.Nov 2 2022, 2:59 PM

Harbormaster completed remote builds in B13147: Diff 18042.Nov 2 2022, 3:01 PM

• max published this revision for review.Nov 2 2022, 4:30 PM

• max edited the summary of this revision. (Show Details)

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

• max added a reviewer: marcin. • max added 1 blocking reviewer(s): • jon.

• max mentioned this in D5525: [services] Tunnelbroker - Removing of 'sessionID' from 'MessageToTunnelbroker'.Nov 3 2022, 3:16 AM

• max mentioned this in D5527: [services] Tunnelbroker - Adding `tokio-stream` dependency to the Cargo.Nov 3 2022, 5:33 AM

• jon added inline comments.Nov 3 2022, 10:03 AM

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
148 ↗	(On Diff #18042)
services/tunnelbroker/src/server/mod.rs
87 ↗	(On Diff #18042)	not a big fan of `unwraps`
94–97 ↗	(On Diff #18042)	in the future, might be nice to have a facade around the cxx_bridge stuff so we can have cleaner rust code base

Small changes based on comments.

• max marked 3 inline comments as done.Nov 4 2022, 6:52 AM

• max added inline comments.

services/tunnelbroker/src/server/mod.rs
87 ↗	(On Diff #18042)	not a big fan of `unwraps` Makes sense to use expect here. Changed!
94–97 ↗	(On Diff #18042)	in the future, might be nice to have a facade around the cxx_bridge stuff so we can have cleaner rust code base We have only two C++ functions that can return compatible to Tonic gRPC responses. Most part of them should return different results and we will process them on the Rust side in the following bidirectional messages stream handler. Not sure it makes sense for two requests. I've shortened the calls by moving the `cxx_bridge::ffi` to `use` as we have a lot of calls for it, this increases readability a bit.

Harbormaster completed remote builds in B13171: Diff 18076.Nov 4 2022, 6:58 AM

• jon accepted this revision.Nov 4 2022, 10:59 AM

• jon added inline comments.

services/tunnelbroker/src/server/mod.rs
94–97 ↗	(On Diff #18042)	We have only two C++ functions that can return compatible to Tonic gRPC responses. Most part of them should return different results and we will process them on the Rust side in the following bidirectional messages stream handler. Not sure it makes sense for two requests. Makes sense, if they are the exception.

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

• max removed a parent revision: D5525: [services] Tunnelbroker - Removing of 'sessionID' from 'MessageToTunnelbroker'.Nov 7 2022, 6:25 AM

• max edited parent revisions, added: D5490: [services] Tunnelbroker - Adding of gRPC `NewSession` wrapper implementation; removed: D5527: [services] Tunnelbroker - Adding `tokio-stream` dependency to the Cargo.Nov 7 2022, 9:17 AM

• max mentioned this in rCOMM597fa55ea368: [services] Tunnelbroker - Adding `tokio-stream` dependency to the Cargo.Nov 7 2022, 10:01 AM

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

This revision now requires review to proceed.Nov 8 2022, 7:07 AM

Rebasing on master changes, small refactoring ot use let ... match.

Harbormaster completed remote builds in B13263: Diff 18198.Nov 8 2022, 7:30 AM

Rebasing on parent changes.

Harbormaster failed remote builds in B13283: Diff 18225!Nov 8 2022, 10:35 AM

Fixing int variable casting based on D5560

Harbormaster completed remote builds in B13287: Diff 18230.Nov 8 2022, 11:09 AM

tomek accepted this revision.Nov 9 2022, 8:50 AM

tomek added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp
154 ↗	(On Diff #18230)	Why is this cast required?

This revision is now accepted and ready to land.Nov 9 2022, 8:50 AM

• max added a child revision: D5662: [services] Tunnelbroker - Add test for the `findSessionItemByDeviceID` function.Nov 17 2022, 2:48 AM

• max added a child revision: D5661: [services] Tunnelbroker - Adding `getSessionItemsByDeviceID` function.

• max removed a child revision: D5662: [services] Tunnelbroker - Add test for the `findSessionItemByDeviceID` function.

Rebasing on master and parents changes.

• max marked an inline comment as done.Nov 21 2022, 3:38 AM

• max added inline comments.

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp

154 ↗

(On Diff #18230)

Why is this cast required?

The compiler forces us to use casting for the enum -> int:

Tunnelbroker.cpp:168:21: error: non-constant-expression cannot be narrowed from type 'size_t' (aka 'unsigned long') to '::std::int32_t' (aka 'int') in initializer list [-Wc++11-narrowing]
  cargo:warning=      .deviceType = sessionItem->getDeviceType(),
  cargo:warning=                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  cargo:warning=src/libcpp/Tunnelbroker.cpp:168:21: note: insert an explicit cast to silence this issue
  cargo:warning=      .deviceType = sessionItem->getDeviceType(),
  cargo:warning=                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  cargo:warning=                    static_cast<int32_t>(       )

Harbormaster completed remote builds in B13616: Diff 18646.Nov 21 2022, 3:49 AM