Page MenuHomePhabricator

[Keyserver] Introduce tunnelbroker types
AbandonedPublic

Authored by jon on May 1 2023, 3:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 3, 4:21 AM
Unknown Object (File)
Sat, Mar 2, 11:08 AM
Unknown Object (File)
Sat, Mar 2, 1:10 AM
Unknown Object (File)
Thu, Feb 29, 8:49 PM
Unknown Object (File)
Thu, Feb 29, 8:49 PM
Unknown Object (File)
Thu, Feb 29, 8:49 PM
Unknown Object (File)
Thu, Feb 29, 8:49 PM
Unknown Object (File)
Thu, Feb 29, 8:49 PM
Subscribers

Details

Reviewers
ashoat
Summary

As a first step to onboarding tunnelbroker, add support
for it's websocket protocol messaging types. Initially, this
only consists of SessionRequest.

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

Test Plan

Type checks

Diff Detail

Repository
rCOMM Comm
Branch
jonringer/keyserver-tunnelbroker (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.May 1 2023, 3:42 PM
Harbormaster failed remote builds in B18970: Diff 25985!
ashoat requested changes to this revision.May 3 2023, 6:32 AM

Looks good, but some questions about the stuff we're passing to Tunnelbroker. Separately, I don't think we should be landing the "Hello world" example parent diff

lib/types/tunnelbroker-messages.js
3 ↗(On Diff #26031)

Is this message meant to initialize a Tunnelbroker connection? If so, maybe ConnectionInitializationMessage or ConnInitMessage or something like that would be more clear

5 ↗(On Diff #26031)

We usually do deviceID instead of deviceId

9 ↗(On Diff #26031)

What kind of version is this meant to be?

10 ↗(On Diff #26031)

Is this a version string, or more like ios vs. android? Should this be combined with deviceType in some way?

This revision now requires changes to proceed.May 3 2023, 6:32 AM
lib/types/tunnelbroker-messages.js
3 ↗(On Diff #26031)

(unsolicited and not my part of the codebase, but would prefer ConnectionInitializationMessage so I could eg fuzzy search "connec" and it would still come up)

lib/types/tunnelbroker-messages.js
3 ↗(On Diff #26031)

Also could be good to prefix with TB

jon marked 3 inline comments as done.

Address feedback

lib/types/tunnelbroker-messages.js
3 ↗(On Diff #26031)

Yes, it is used to establish a session. Websocket protocol already "takes care of" the connection aspect, so I would like to avoid that term.

However, I would rather have consistency than accuracy.

This name was chosen to reflect the existing messages on the tunnelbroker side. https://github.com/CommE2E/comm/blob/4bbb90307487a584e6a41b768ec78b116c502fa1/shared/tunnelbroker_messages/src/messages/session.rs#L33-L41

I would just need to rename it in both places. But now would be the time to rename it rather than later.

9 ↗(On Diff #26031)

This is a carry-over of the existing API. I assumed that there may be logic in the future where the types of messages may change depending on what the client supports.

I can remove if needed.

10 ↗(On Diff #26031)

I had assumed that it would be needed for push notifications. But would available by identity service.

I think we should remove this.

removed deviceOS and deviceAppVersion. As I think those were "legacy" concepts from the previous tunnelbroker paradigms. Those details should exist in the identity server, and should be available once we authenticate sessions.

ashoat requested changes to this revision.May 4 2023, 9:53 AM
ashoat added inline comments.
lib/types/tunnelbroker-messages.js
3 ↗(On Diff #26077)

I would just need to rename it in both places. But now would be the time to rename it rather than later.

Can we start by renaming this to ConnectionInitializationMessage?

7 ↗(On Diff #26077)

For this notifyToken to have any significance, we need to know what kind of notifyToken it is. That's probably when we previously needed the OS?

This revision now requires changes to proceed.May 4 2023, 9:53 AM
jon marked 5 inline comments as done.

Rename SessionRequest to ConnectionInitializationMessage

lib/types/tunnelbroker-messages.js
7 ↗(On Diff #26077)

I think we need to iron-out some details about how tunnelbroker would be handling notifications. At the very least, it shouldn't only be getting handle for active connections. We will likely need to persist this information in some way. Not sure if we want to do this on the identity side when someone first registers (or login) with a device, or have tunnelbroker have this information tracked separately (this is some duplication, but probably the best route forward to avoid hammering identity service).

Either way, since notifications are out-of-scope, I'm okay just removing it for now, and then adding proper support at a later date.

ashoat requested changes to this revision.May 4 2023, 10:27 AM

Not sure if we want to do this on the identity side when someone first registers (or login) with a device, or have tunnelbroker have this information tracked separately (this is some duplication, but probably the best route forward to avoid hammering identity service).

I agree... this data should be stored on the Tunnelbroker side. How hard would it be to persist it in DynamoDB? Do we currently not have per-device storage for Tunnelbroker in DDB?

lib/types/tunnelbroker-messages.js
7 ↗(On Diff #26083)

There is no point including a notifyToken without the OS. I think we should bring back the options you had originally, that are listed in this Linear comment. I realize we can bring this back later, but every time we do something like that we create more work for ourselves, since we'll need to find a way to get existing clients to provide that info later. Might as well get it in now

This revision now requires changes to proceed.May 4 2023, 10:27 AM

Discussed in our 1:1 and we settled on:

type TBSharedConnectionInitializationMessage = {
  +type: 'sessionRequest',
  +deviceID: string,
  +accessToken: string,
  +deviceAppVersion?: string,
};

type TBKeyserverConnectionInitializationMessage = {
  ...TBSharedConnectionInitializationMessage,
  +deviceType: 'keyserver',
};

type TBClientConnectionInitializationMessage = {
  ...TBSharedConnectionInitializationMessage,
  +deviceType: 'web' | 'mobile',
};

type TBNotifyClientConnectionInitializationMessage = {
  ...TBClientConnectionInitializationMessage,
  +notifyToken: string,
  +notifyPlatform: 'apns' | 'fcm' | 'web' | 'wns',
};

type TBConnectionInitializationMessage =
  | TBKeyserverConnectionInitializationMessage
  | TBClientConnectionInitializationMessage
  | TBNotifyClientConnectionInitializationMessage;

Just noticed that VerifyUserAccessTokenRequest requires userID – does that need to be included in TBSharedConnectionInitializationMessage too?

Just noticed that VerifyUserAccessTokenRequest requires userID – does that need to be included in TBSharedConnectionInitializationMessage too?

Yes it would, guess that would be a new addition to the existing protocol

lib/types/tunnelbroker-messages.js
7 ↗(On Diff #26083)

There is no point including a notifyToken without the OS. I think we should bring back the options you had originally, that are listed in this Linear comment.

I was just going to remove notifyToken altogether, until it's clear what we need.

but every time we do something like that we create more work for ourselves,

We've also moved forward with things which we had to undo later: opaque_ke 1.2, SessionInitializtion

However, since the fields are optional, I don't see the harm in it either. Changing it should be low pain-threshold as well.

jon added inline comments.
lib/types/tunnelbroker-messages.js
7 ↗(On Diff #26083)

didn't realize I hadn't posted this earlier, we already talked about it in the one on one

Apply feedback, rebase on master

JS looks good. Rust changes look simple but I'm not clear on how all of them connect to the JS. Might be good to add a reviewer who has context on that part of the codebase, but defer to @jon

This revision is now accepted and ready to land.Jun 1 2023, 6:03 AM

but I'm not clear on how all of them connect to the JS.

I just wanted consistency of naming. These are meant to be the same message, no reason to have them named differently across the codebase.

squashed this into D7691 because of rebase work