As a first step to onboarding tunnelbroker, add support
for it's websocket protocol messaging types. Initially, this
only consists of SessionRequest.
Details
Details
- Reviewers
ashoat
Type checks
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jonringer/keyserver-tunnelbroker (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
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? |
| 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 |
Comment Actions
Found some background on this: https://linear.app/comm/issue/ENG-1995#comment-210dfba5
Comment Actions
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. |
Comment Actions
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.
| lib/types/tunnelbroker-messages.js | ||
|---|---|---|
| 3 ↗ | (On Diff #26077) |
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? |
Comment Actions
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. |
Comment Actions
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 |
Comment Actions
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;
Comment Actions
Just noticed that VerifyUserAccessTokenRequest requires userID – does that need to be included in TBSharedConnectionInitializationMessage too?
Comment Actions
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) |
I was just going to remove notifyToken altogether, until it's clear what we need.
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. |
| 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 |
Comment Actions
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
Comment Actions
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.