Details

Reviewers

varun
bartek
ashoat
tomek
atul

Commits

rCOMM265abf6197f0: [Identity] Add X3DH RPCs

Summary

Separate diff for discussion around X3DH actions

Part of https://linear.app/comm/issue/ENG-3264

Depends on D7003

Test Plan

N/A, meant for consensus around data-plane considerations
of key exchanges.

Diff Detail

Repository

rCOMM Comm

Branch

jonringer/client-proto (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jon created this revision.Mar 13 2023, 10:28 AM

ashoat added inline comments.Mar 13 2023, 10:33 AM

shared/protos/identity_client.proto
251 ↗	(On Diff #23700)	This appears to be the name of a string... did you mean to use `RemoteDeviceInfo` as a type? If not, should it be lowercased?

Harbormaster completed remote builds in B17301: Diff 23700.Mar 13 2023, 10:43 AM

• jon requested review of this revision.Mar 13 2023, 10:43 AM

Correct KeyserverKeysResponse structure

Harbormaster completed remote builds in B17302: Diff 23701.Mar 13 2023, 11:42 AM

Mostly requesting changes to get rid of the keyserverID concept

shared/protos/identity_client.proto
247	We don't have a concept of a "keyserver ID". Instead, each user may have a keyserver, and that keyserver is generally addressable via the user's ID. Can we update this to use the same `identifier` we've defined on line 233?
251	Users are not required to have a keyserver. What's our behavior if the requested keyserver does not exist? I'm guessing we send an error of some sort?
267	A little weird that this contains `optional string onetimePrekey`. Should we consider introducing a new type that is like `PreKeyResponse`, but only contains `preKey` and `preKeySignature`?

This revision now requires changes to proceed.Mar 13 2023, 11:50 AM

Fix refreshPreKeys message. response -> upload

shared/protos/identity_client.proto
247	From the whitepaper: Keyserver. A keyserver is a primary device that can also host communities. Keyservers are necessary to host communities because they can handle background processing and respond to queries on-demand. As keyservers are self-hosted, Comm is not able to control who has access to the data on a keyserver. Multiple users can share the same keyserver. This makes it sound like: Keyservers are independent of a particular user (shared) All keyservers are primary devices (not yet implemented) But your feedback, makes some additional assumptions: An (already established) user can have 0 or 1 keyservers (no more). All keyservers are directly associated (in some way) with one and only one user. This brings up a few questions for me for the many-keyserver world: Can you register a new keyserver without an existing user? If you can't, what does a user on one keyserver have to do with another? In discord, when you start a server, you just get added as the initial admin, but not much more. The originating user can delete their account, or leave the server, and the server still moves on. What happens if i want to spin up another keyserver for a different community (e.g. Nix keyserver vs rust keyserver)? Seems like I would need to make another user, as pulling keyserver information by userID would need to return more than one result otherwise.
251	correct, there would be a `grpc::not_found` error issued.
267	This should have been upload, thanks.

I think it would be easier to make the requested change RE keyserverID and follow up with questions you might have for me separately.

Can you register a new keyserver without an existing user? If you can't, what does a user on one keyserver have to do with another? In discord, when you start a server, you just get added as the initial admin, but not much more. The originating user can delete their account, or leave the server, and the server still moves on.

No, you can't. Not sure what the second question is about. User B uses user A's keyserver by merit of being in a community hosted on that keyserver.

What happens if i want to spin up another keyserver for a different community (e.g. Nix keyserver vs rust keyserver)? Seems like I would need to make another user, as pulling keyserver information by userID would need to return more than one result otherwise.

Yes, that's right.

This revision now requires changes to proceed.Mar 13 2023, 12:35 PM

ashoat added inline comments.Mar 13 2023, 12:37 PM

shared/protos/identity_client.proto
282 ↗	(On Diff #23703)	Not sure how this is better than `PreKeyResponse`. Effectively copy-pasting my feedback below: A little weird that this contains `repeated string onetimePrekeys = 3`. Should we consider introducing a new type that is like `PreKeyRegistrationUpload`, but only contains `preKey` and `preKeySignature`?