Page MenuHomePhabricator
Differential D5528

[services] Tunnelbroker - Adding of the client authentication by the `sessionID` from metadata
ClosedPublic
Actions

Authored by • max on Nov 2 2022, 2:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 7:42 PM
Unknown Object (File)
Wed, Apr 2, 4:37 AM
Unknown Object (File)
Mon, Mar 31, 5:11 PM
Unknown Object (File)
Mon, Mar 24, 6:53 PM
Unknown Object (File)
Thu, Mar 20, 9:56 PM
Unknown Object (File)
Wed, Mar 19, 11:58 PM
Unknown Object (File)
Mon, Mar 17, 3:10 AM
Unknown Object (File)
Mon, Mar 17, 3:10 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

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
Branch
add-grpc-messages-authentication
Lint
No Lint Coverage
Unit
No Test Coverage

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

• max updated this revision to Diff 18076.Nov 4 2022, 6:48 AM

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
• max updated this revision to Diff 18198.Nov 8 2022, 7:20 AM
• max marked 2 inline comments as done.

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

Harbormaster completed remote builds in B13263: Diff 18198.Nov 8 2022, 7:30 AM
• max updated this revision to Diff 18225.Nov 8 2022, 10:33 AM

Rebasing on parent changes.

Harbormaster failed remote builds in B13283: Diff 18225!Nov 8 2022, 10:35 AM
• max updated this revision to Diff 18230.Nov 8 2022, 10:58 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.
• max updated this revision to Diff 18646.Nov 21 2022, 3:37 AM

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
• max updated this revision to Diff 18654.Nov 21 2022, 4:27 AM
• max marked an inline comment as done.

Fix merging.

Harbormaster completed remote builds in B13619: Diff 18654.Nov 21 2022, 4:40 AM
Closed by commit rCOMM56ac25fc5332: [services] Tunnelbroker - Adding of the client authentication by the… (authored by • max). · Explain WhyNov 21 2022, 4:43 AM
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMM56ac25fc5332: [services] Tunnelbroker - Adding of the client authentication by the….
• max removed a child revision: D5661: [services] Tunnelbroker - Adding `getSessionItemsByDeviceID` function.Nov 21 2022, 5:22 AM

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
src/
10 lines
libcpp/
1 line
22 lines
server/
35 lines

Diff 18198

View Options

services/tunnelbroker/src/cxx_bridge.rs

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

services/tunnelbroker/src/libcpp/Tunnelbroker.h

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

services/tunnelbroker/src/libcpp/Tunnelbroker.cpp

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net
View Options

services/tunnelbroker/src/server/mod.rs

Show First 20 Lines • Show All 55 Lines • ▼ Show 20 Lines networks:
- services-net - services-net
build: build:
dockerfile: services/identity/Dockerfile dockerfile: services/identity/Dockerfile
context: ../ context: ../
image: commapp/identity-server:0.1 image: commapp/identity-server:0.1
container_name: identity-server container_name: identity-server
ports: ports:
- "${COMM_SERVICES_PORT_IDENTITY}:50051" - "${COMM_SERVICES_PORT_IDENTITY}:50051"
# localstack
localstack:
image: localstack/localstack
container_name: localstack
hostname: localstack
ports:
- "4566:4566"
environment:
- SERVICES=s3,dynamodb
- DATA_DIR=${TMPDIR}/localstack/data
- HOSTNAME_EXTERNAL=localstack
volumes:
jimUnsubmitted
Not Done
Inline Actions

Why bind mount a temp directory instead of using a data volume? It's just gonna create issues with permissions. Also ${TMPDIR} is not used to refer to a path in both the host and the container which is a bad idea.

jim: Why bind mount a temp directory instead of using a data volume? It's just gonna create issues…
- ${TMPDIR}:/tmp/localstack
networks:
- services-net