Page MenuHomePhabricator

[Keyserver] Open websocket connection with tunnelbroker
ClosedPublic

Authored by kamil on Apr 30 2023, 10:58 PM.
Tags
None
Referenced Files
F3382228: D7691.diff
Thu, Nov 28, 9:16 AM
Unknown Object (File)
Sun, Nov 24, 7:48 PM
Unknown Object (File)
Sun, Nov 24, 7:07 PM
Unknown Object (File)
Sun, Nov 24, 7:03 PM
Unknown Object (File)
Sun, Nov 24, 7:01 PM
Unknown Object (File)
Sun, Nov 24, 6:57 PM
Unknown Object (File)
Sun, Nov 24, 6:48 PM
Unknown Object (File)
Sun, Nov 24, 6:46 PM

Details

Summary

Attempt to connect to tunnelbroker if available.
This is meant to be a "hello world" for establishing
a tunnelbroker connection.

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

Depends on D8476

Test Plan
(cd services/tunnelbroker && cargo run &)
# should see message about db migration, then tunnelbroker message
(cd keyserver && yarn dev)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jon marked 5 inline comments as done.

Address feedback

keyserver/src/socket/tunnelbroker.js
41–43 ↗(On Diff #28758)

It was useful, but I'll remove it now.

49 ↗(On Diff #28758)

Yea, testing this code path locally will cause it to dump just a massive amount of output in a short amount of time. The sleep is just there to make it manage-able. We should probably have some type of exponential back-off. But 1s seems fine for now.

ashoat requested changes to this revision.Jul 27 2023, 11:28 AM
ashoat added inline comments.
keyserver/src/keyserver.js
67–68 ↗(On Diff #28982)

It's a little weird to see this pattern, where we await one Promise and but then don't await the subsequent one.

It would be good to add a code comment explaining why we're doing this:

  1. verifyUserLoggedIn needs to be awaited because:
    • Currently, the code for getting one-time keys to initiate the notif ratchet could have a race with the verifyUserLoggedIn code, leading to one-time keys getting erroneously marked as published. This will no longer be true after we update that code to source one-time keys from identity service, but...
    • In the future, we expect that having a commServicesAccessToken will be required for normal keyserver functioning
  2. On the other hand, createAndMaintainTunnelbrokerWebsocket doesn't need to be awaited because:
    • The connection to Tunnelbroker isn't strictly necessary for normal keyserver functioning at this time
    • The call to createAndMaintainTunnelbrokerWebsocket doesn't return anything that we need ahead of forking the Express server threads
keyserver/src/socket/tunnelbroker.js
15 ↗(On Diff #28982)

Please apply this feedback to every place in the diff that it applies to

17 ↗(On Diff #28982)

You should not be awaiting something that doesn't return a promise

42 ↗(On Diff #28982)
46 ↗(On Diff #28982)
48 ↗(On Diff #28982)

Please apply this feedback everywhere

53 ↗(On Diff #28982)
55 ↗(On Diff #28982)

If we're not doing anything with err, we can omit it

This revision now requires changes to proceed.Jul 27 2023, 11:28 AM
jon marked 8 inline comments as done.

Address feedback

Add comments in keyserver.js

Capitalize Keyserver and Tunnelbroker in comments

ashoat requested changes to this revision.Aug 18 2023, 3:19 PM

I got 15 lines into the file, and then gave up on the review due to frustration. You completely ignored a comment from my previous review.

Please fix the issue, and then review all previous diff feedback carefully and thoroughly to make sure it has been addressed before putting this back on my diff queue.

keyserver/src/keyserver.js
67–73 ↗(On Diff #30013)

Minor changes

keyserver/src/socket/tunnelbroker.js
15 ↗(On Diff #30013)

My feedback from the last review appears to have been ignored:

{F695889}

Remember – a reviewer should never have to repeat feedback. This is a common issue for you... please review feedback carefully before updating a diff. Go line by line for each comment you received, and make sure you addressed it 100%. It shouldn't ever be necessary for me to repeat feedback to you.

This revision now requires changes to proceed.Aug 18 2023, 3:19 PM
jon added inline comments.
keyserver/src/socket/tunnelbroker.js
15 ↗(On Diff #30013)

missed this one.

You seem to have accidentally addressed my 2 comments from the previous review in D8752. Can you make sure you update this diff as well?

kamil added a reviewer: jon.
kamil removed a reviewer: jon.
  • update comment
  • deviceId -> deviceID
keyserver/src/socket/tunnelbroker.js
47–49 ↗(On Diff #30865)

I think we should remove this code until the staging is ready, otherwise, keyserver console will be spammed if someone will not run the local tunnelbroker. Thoughts?

lib/types/tunnelbroker-messages.js
8 ↗(On Diff #30865)

To update this to userID we need to make some changes in Rust struct definition, Created a task ENG-4880

keyserver/src/keyserver.js
71–72 ↗(On Diff #30865)

missed one line, will update shortly

Some more bugs I found while testing D8918, I will address them later

keyserver/src/socket/tunnelbroker.js
15 ↗(On Diff #30865)
38 ↗(On Diff #30865)
  • curve25519 -> ed25519
  • deviceID -> userID
  • remove auto-establishing session
keyserver/src/socket/tunnelbroker.js
44–46 ↗(On Diff #30954)

Some context in ENG-4924

lib/types/tunnelbroker-messages.js
3–9 ↗(On Diff #30954)

JS looks good, but I don't know Rust so couldn't review that part

This revision is now accepted and ready to land.Sep 12 2023, 5:57 AM
kamil added subscribers: bartek, michal.

@bartek, @michal could you take a brief look at Rust?

LGTM, mostly just a rename

services/tunnelbroker/src/websockets/session.rs
112–127 ↗(On Diff #30959)

Why was this removed?

kamil added inline comments.
services/tunnelbroker/src/websockets/session.rs
112–127 ↗(On Diff #30959)

discussed in the office: this code is moved higher, to accept_connection()