Details

Reviewers

• karol
tomek
varun
ashoat

Commits

rCOMM609033818702: [services] Tunnelbroker - Add messages request and confirmation method to grpc…

Summary

This diff introduces the new gRPC method for the client checkpointing request to the server according to the ENG-1158 ashoat comment.
The purpose of this method is: the client sends the request to the tunnelbroker server to confirm successful checkpointing (the client successfully received and persisted the updates from the Tunnelbroker)

Related task: ENG-1306

Test Plan

gRPC protoc successfully generates the source code.

Diff Detail

Repository

rCOMM Comm

Branch

repeated-messages-proposal

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Jun 6 2022, 6:31 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJun 6 2022, 6:31 AM

• max retitled this revision from [services] Tunnelbroker - Add checkpoint request to grpc to [services] Tunnelbroker - Add checkpoint request method to grpc proto file.Jun 6 2022, 6:35 AM

• max edited the summary of this revision. (Show Details)

• max added reviewers: • karol, tomek, varun.

Harbormaster completed remote builds in B9641: Diff 13364.Jun 6 2022, 6:36 AM

• max published this revision for review.Jun 6 2022, 7:06 AM

Are we sure this should be a distinct RPC? Wondering how this relates to ENG-1132 (merging Send and Get into a bidirectional stream)... what are we planning to handle in the bidirectional stream vs. outside of it?

To be clear, I'm not insisting that this gets handled in the bidirectional stream... just want to understand tradeoffs, and to have a consistent story about what goes there and what doesn't.

This revision now requires changes to proceed.Jun 7 2022, 1:34 AM

PS – I realized I didn't put this in the Diff Review Rules, so just added – can you make sure to always put me on the review for .proto API changes?

• max planned changes to this revision.Jun 8 2022, 7:45 AM

Updating based on the ENG-1158 comments discussion.
Additional confirmation for the client from the tunnelbroker was added as a new generated message ID.

• max planned changes to this revision.Jun 20 2022, 4:46 PM

Harbormaster failed remote builds in B9839: Diff 13612!Jun 20 2022, 4:46 PM

Requesting review, based on the updates on the messaging flow from the discussion.

Harbormaster completed remote builds in B9839: Diff 13612.Jun 20 2022, 6:47 PM

There are a couple of issues here - I described them in the comment https://linear.app/comm/issue/ENG-1158/message-delivery-confirmation-checkpointing-in-tunnelbroker where a diagram was shared.

This revision now requires changes to proceed.Jun 21 2022, 12:57 AM

• max planned changes to this revision.Jun 21 2022, 8:00 AM

Changing checkpointTime from int32 to int64, based on the comments in the ENG-1158 discussion.

Harbormaster completed remote builds in B9862: Diff 13641.Jun 21 2022, 4:36 PM

Updating by using createdAt as a checkpointTime proposal from ENG-1158.

• max planned changes to this revision.Jun 22 2022, 9:38 AM

Harbormaster failed remote builds in B9881: Diff 13670!Jun 22 2022, 9:39 AM

• max requested review of this revision.Jun 22 2022, 9:52 AM

• max planned changes to this revision.Jun 22 2022, 10:05 AM

Fixing oneof+repeated field error by adding nested struct.

Harbormaster failed remote builds in B9885: Diff 13675!Jun 22 2022, 10:30 AM

• max planned changes to this revision.Jun 22 2022, 10:32 AM

Fixing an outbound message name.

Harbormaster completed remote builds in B9886: Diff 13677.Jun 22 2022, 10:40 AM

ashoat requested changes to this revision.Jun 22 2022, 12:03 PM

ashoat added inline comments.

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
93 ↗	(On Diff #13677)	Why do we need to always include the `sessionID` here? Won't the client know its own `sessionID`?
96 ↗	(On Diff #13677)	This is "client confirming the checkpoint to the server", whereas below on line 116 the same `checkpointTimeConfirmation` is "the checkpoint that the client will confirm after receiving", right? If so – I think it's confusing that we use the same name for both... if I was reading this, I would assume line 116 is "server confirming the checkpoint to the client"
113 ↗	(On Diff #13677)	Same question here

This revision now requires changes to proceed.Jun 22 2022, 12:03 PM

• max planned changes to this revision.Jun 24 2022, 7:33 AM

• max edited the summary of this revision. (Show Details)Jun 28 2022, 7:11 AM

Freezing this diff until ENG-1158 discussion reaches consensus.

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
93 ↗	(On Diff #13677)	Why do we need to always include the `sessionID` here? Won't the client know its own `sessionID`? Yes, the client knows it's sessionID, and making the double-check for the sessionID at the client-side is not necessary. I think it's not necessary for the Tunnelbroker -> Client response. It can be used only for the Client -> Tunnelbroker request to authenticate a client session. We can omit it at the Tunnelbroker -> Client response and I'll remove it.
96 ↗	(On Diff #13677)	This is "client confirming the checkpoint to the server", whereas below on line 116 the same `checkpointTimeConfirmation` is "the checkpoint that the client will confirm after receiving", right? If so – I think it's confusing that we use the same name for both... if I was reading this, I would assume line 116 is "server confirming the checkpoint to the client" I agree, that I should change these two fields' names to be more self-describing and remove this ambiguity.

Removing time-based checkpointing in favor of messageIDs.

Harbormaster failed remote builds in B10330: Diff 14238!Jul 6 2022, 2:52 PM

OpenStream was renamed to MessagesStream.
The time-based checkpoint was removed in favor of 'messageIDs' according to the ENG-1158 discussion.

We will send a messageIDs list as a confirmation in Client → Tunnelbroker and Tunnelbroker → Client as well. The messageIDs generation will be on the client-side in this case (we will generate a unique UUID) and we will check the correctness of it on the Tunnelbroker-side to avoid wrong formats.

More detail and the flow diagram are in the ENG-1158 comment.

• max retitled this revision from [services] Tunnelbroker - Add checkpoint request method to grpc proto file to [services] Tunnelbroker - Add messages request and confirmation method to grpc proto file.Jul 6 2022, 3:05 PM

Rebased on master.

Harbormaster completed remote builds in B10331: Diff 14239.Jul 6 2022, 3:19 PM

Mostly questions, one small request. This is close!

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
88 ↗	(On Diff #14239)	Is this just used so the server can return `ProcessedMessages`, or does the `messageID` get used elsewhere? Is there a possible attack that the client can make by choosing `messageID`s, eg. a collision of some sort?
98 ↗	(On Diff #14239)	It's a bit unclear to me what inbound vs. outbound means... it depends on whether you're looking at things from the perspective of the client or the server. How about `MessageToClient` / `MessageFromClient`? Or `MessageToClient` / `MessageToTunnelbroker`? Open to other alternatives
99 ↗	(On Diff #14239)	It feels wasteful to be repeating this information for every message. Open to following up on this in a separate Linear issue, though

This revision now requires changes to proceed.Jul 6 2022, 3:46 PM

Changed names to MessageToClient / MessageToTunnelbroker.

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
88 ↗	(On Diff #14239)	Is this just used so the server can return `ProcessedMessages`, or does the `messageID` get used elsewhere? Is there a possible attack that the client can make by choosing `messageID`s, eg. a collision of some sort? The `messageID`s are used for messages identification in confirmation (both from the server and client-side) and for messages removed from the database (along with the receiver deviceID). The UUID collision probability itself is a find a duplicate within 103 trillion version-4 UUIDs is one in a billion. It's near-zero. But in our case, it will not lead to any collision because: When we get the saved messages for the certain deviceID we will fetch them by deviceID. When the new device is registered we check for the unique deviceID and check collision. When we'll remove the certain messages from the database we will use the unique `messageID` and unique receiver's `deviceID` as a second key. That's why deleting some messages in a collision is zero (because of using a combination of messagesID + deviceID) as well as fetching a "wrong" message by a collision for the certain `deviceID` is zero (because of using the same combination of deviceID + messageID).
98 ↗	(On Diff #14239)	It's a bit unclear to me what inbound vs. outbound means... it depends on whether you're looking at things from the perspective of the client or the server. How about `MessageToClient` / `MessageFromClient`? Or `MessageToClient` / `MessageToTunnelbroker`? Open to other alternatives I think `MessageToClient / MessageToTunnelbroker` is more self-describing in this case. I've changed to use of these names.
99 ↗	(On Diff #14239)	It feels wasteful to be repeating this information for every message. Open to following up on this in a separate Linear issue, though Yes, that's a way to improve it. I've created a separate ENG-1359 task for that.

Harbormaster completed remote builds in B10332: Diff 14240.Jul 6 2022, 5:42 PM

Regarding the potential of an messageID collision attack, this would be a scenario where an attacker would pretend to be a Tunnelbroker client, and send Tunnelbroker a MessageToTunnelbroker specially crafted to cause an issue.

The probability of collision is not relevant because that assumes that the client is randomly selecting a messageID. In this attack scenario, the messageIDs would not be selected randomly... instead, they would be selected to cause an issue.

One of the unique traits of Tunnelbroker is that a message is sent by user A, but then it is queued up to be delivered by user B. Is it possible that user B could have a messageID queued up for delivery, and an attacker user A could find out the same messageID and use it, and then cause an issue for user B?

For instance, perhaps there could be an entropy attack where attacker user A happens to know user B has low entropy. Or perhaps user A and user B are using the web client on the same computer.

I understand this probably isn't extremely likely, but I'd just like to get some perspective on this potential attack.

• max added inline comments.Jul 7 2022, 5:49 AM

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
88 ↗	(On Diff #14239)	Is there a possible attack that the client can make by choosing `messageID`s, eg. a collision of some sort? Update: We can use a composite key (messageID + toDeviceID) for the DynamoDB messages table. Composite keys are unique (DynamoDB will throw a duplication error) and this composition will lead the possible collisions to zero. I've created a task ENG-1362 for that change.

tomek accepted this revision.Jul 8 2022, 1:57 AM

tomek added inline comments.

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
94–117	Why do we need an additional layer with `MessagesToSent` and `MessageToClient`? What's the downside of using `MessageToTunnelbrokerStruct` and `MessageToClientStruct` directly? Also, `MessagesToSent` should be probably renamed to `MessagesToSend`

This revision is now accepted and ready to land.Jul 8 2022, 1:57 AM

Names was changed from using *Sent to *Send.

• max marked an inline comment as done.Jul 13 2022, 2:28 AM

• max added inline comments.

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto
94–117	Why do we need an additional layer with `MessagesToSent` and `MessageToClient`? What's the downside of using `MessageToTunnelbrokerStruct` and `MessageToClientStruct` directly? The reason to use an additional layer here is that `oneof` can not be `repeated`: ...You can add fields of any type, except map fields and repeated fields. That's why we need to wrap repeated field into an additional layer. Also, `MessagesToSent` should be probably renamed to `MessagesToSend` I've changed these names to use *Send instead.

Harbormaster completed remote builds in B10477: Diff 14424.Jul 13 2022, 2:36 AM

Closed by commit rCOMM609033818702: [services] Tunnelbroker - Add messages request and confirmation method to grpc… (authored by • max). · Explain WhyJul 18 2022, 6:28 AM

This revision was automatically updated to reflect the committed changes.

• max marked an inline comment as done.

• max added a commit: rCOMM609033818702: [services] Tunnelbroker - Add messages request and confirmation method to grpc….

[services] Tunnelbroker - Add messages request and confirmation method to grpc proto file
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 14240

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto

[services] Tunnelbroker - Add messages request and confirmation method to grpc proto fileClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 14240

native/cpp/CommonCpp/grpc/protos/tunnelbroker.proto

[services] Tunnelbroker - Add messages request and confirmation method to grpc proto file
ClosedPublic
Actions

Revision Contents
Changeset List