Page MenuHomePhabricator

[Tunnelbroker] Authenticate connecting devices
ClosedPublic

Authored by kamil on Aug 22 2023, 10:46 AM.
Tags
None
Referenced Files
F1722314: D8918.diff
Thu, May 9, 9:28 PM
Unknown Object (File)
Thu, May 2, 6:52 PM
Unknown Object (File)
Mon, Apr 22, 9:26 PM
Unknown Object (File)
Tue, Apr 16, 8:08 AM
Unknown Object (File)
Tue, Apr 16, 7:50 AM
Unknown Object (File)
Tue, Apr 16, 7:50 AM
Unknown Object (File)
Sun, Apr 14, 4:14 PM
Unknown Object (File)
Sun, Apr 14, 4:14 PM

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
Branch
jonringer/keyserver-identity-only (branched from master)
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 ↗(On Diff #30240)

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