Page MenuHomePhabricator

[Tunnelbroker] Authenticate connecting devices
ClosedPublic

Authored by kamil on Aug 22 2023, 10:46 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 3:17 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM
Unknown Object (File)
Sun, Dec 15, 6:21 AM

Details

Summary

When a device starts a session with tunnelbroker, verify
the device before continuing the session.

https://linear.app/comm/issue/ENG-3977

Depends on D8916

Test Plan
  • Start identity service, tunnelbroker
cd services/commtest
cargo test --test identity_tunnelbroker_tests

Should see logs in tunnelbroker that the device was autheticated successfully

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 22 2023, 10:48 AM
Harbormaster failed remote builds in B22027: Diff 30240!
Harbormaster returned this revision to the author for changes because remote builds failed.Aug 22 2023, 10:53 AM
Harbormaster failed remote builds in B22029: Diff 30242!

LGTM, letting others review as well

This revision is now accepted and ready to land.Aug 28 2023, 12:23 AM
services/tunnelbroker/src/identity/mod.rs
11–27

I'll likely refactor this later to be in grpc_clients, as this will be a common call from services...

Done in https://phab.comm.dev/D9015

@kamil, would you mind commandeering this revision?

kamil edited reviewers, added: jon; removed: kamil.
This revision now requires review to proceed.Sep 8 2023, 10:35 AM
This revision is now accepted and ready to land.Sep 9 2023, 12:55 PM
services/tunnelbroker/src/config.rs
26 ↗(On Diff #31124)

You can do shorter form. The default_value_t expects that you already provided correct type (String in this case) while default_value tries to convert it (&str -> String in this case)

services/tunnelbroker/src/identity/mod.rs
11 ↗(On Diff #31124)

I remember that Jon moved this function to shared/grpc_clients/src/identity/unauthenticated/client.rs. We might want to add this crate as dependency and use it instead.

You can do it in a separate diff, though.

26 ↗(On Diff #31124)

The same comment about return as below

services/tunnelbroker/src/websockets/session.rs
98 ↗(On Diff #31124)

I don't think you need to do return.
Rust functions automatically return last expression, unless it ends with semicolon.

You can check for such warnings by running Clippy linter: cargo clippy (you might need to install it)

michal added a subscriber: michal.
michal added inline comments.
services/tunnelbroker/src/websockets/session.rs
79 ↗(On Diff #31124)

This should probably be error! right?

thanks @michal and @bartek for feedback!

services/tunnelbroker/src/identity/mod.rs
11 ↗(On Diff #31124)
services/tunnelbroker/src/websockets/session.rs
79 ↗(On Diff #31124)

yes

98 ↗(On Diff #31124)

thanks for suggestion