Page MenuHomePhabricator

[Identity] Add client <-> identity protobuf defintions
ClosedPublic

Authored by jon on Mar 8 2023, 9:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Unknown Object (File)
Fri, Apr 12, 1:55 AM
Subscribers

Details

Summary

This meant to get consensus on how a device should
communicate with the identity service.

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

Test Plan

N/A, this is more about the design of the service than testing

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Hmm are we using usernames as an identifier? Ideally we use user IDs for that

A user doesn't know their user ID, they fill in username and password. So that what a device will send over the wire (well, the password is wrapped into the opaque protocol). The server can resolve this to the userID, but at that point, the service are already retrieved the information.

how about identity.general? identity.client feels weird

shared/protos/identity_client.proto
10 ↗(On Diff #23545)

can we add a comment explaining this rpc like we do with the others?

15 ↗(On Diff #23545)

can we add a comment explaining this rpc like we do with the others?

how about identity.general? identity.client feels weird

I would prefer to use identity.device as that's the term used in the comm whitepaper. https://linear.app/comm/issue/ENG-3264#comment-7d5db5ea

EDIT: I was wrong, "client" is the term used for iOS, Android, or web. Devices could be a client or keyserver in the whitepaper. :(

ashoat requested changes to this revision.Mar 8 2023, 3:25 PM
  1. Huh wait a user should know their userID, did you guys discard of the concept of a userID entirely because you didn't see it in the whitepaper?
  2. I don't understand why we're separating into two services, can you explain that?
This revision now requires changes to proceed.Mar 8 2023, 3:25 PM

Huh wait a user should know their userID, did you guys discard of the concept of a userID entirely because you didn't see it in the whitepaper?

No, we aren't throwing it out. For a RegisterUser or OpaqueLoginUser message between a client to the identity service, all that is known is the "username" and "password" (in this case a opaque-wrapped password envelope), because that's all the information available from the user; we can't assume any previous session information (might be a new device, and irrelevant for new users). After a successful registration or login, they will receive a token (which can be correlated to a userID), which will then be used to authenticate future requests. the userID in this case is moved to an implementation detail for DDB in those two cases. Other workflows will need userID available (e.g. GetUserID and GetSessionInitializationInfo currently)

We should probably have a discussion about these workflows honestly. It would be best to get them all ironed out and for all of us on the same page before we start writing code with assumptions.

I don't understand why we're separating into two services, can you explain that?

I'll refer to https://phab.comm.dev/D7001#208031 and https://phab.comm.dev/D7001#208032

shared/protos/identity_client.proto
127 ↗(On Diff #23545)

this should probably be a token, and just refer to the userID on the access token.

Ahh sorry that was a bad misreading on my part, I get what you mean about user IDs now.

If you guys think it's helpful to separate into two services that makes sense.

I think your suggestion over chat of a meeting for a quick architecture review is a good one. I'll try to give this a closer read tomorrow.

jon marked 2 inline comments as done.

UpdateUser -> UpdateUserPassword
Add comment on rpcs
Use a single "Empty" message instead of rpc specific versions

Address feedback in review meeting:

  • UpdateUserPassword takes a token
  • GetSessionInitializationInfoRequest differientiates on login type
shared/protos/identity_client.proto
50 ↗(On Diff #23595)

I think we need to add:

  1. One-time identity prekey
  2. One-time notif prekey
  3. Signed identity prekey
  4. Signed notif prekey
varun requested changes to this revision.Mar 9 2023, 11:35 PM
varun added inline comments.
shared/protos/identity_client.proto
5 ↗(On Diff #23595)
18 ↗(On Diff #23595)

we can remove this extra line

21 ↗(On Diff #23595)

think we can just get rid of this RPC, right?

37 ↗(On Diff #23595)

i introduced this name originally, but it's a little ambiguous. primaryEd25519PublicKey would be my suggestion. we should use whatever we decide on everywhere in this file

https://linear.app/comm/issue/ENG-3224/change-signingpublickey-to-something-more-descriptive

50 ↗(On Diff #23595)

question for @jon and @ashoat : should we have separate fields for each prekey or just have a map of prekeys in the message?

60 ↗(On Diff #23595)

this should be opaqueRegistrationUpload for consistency with opaque-ke's terminology

69 ↗(On Diff #23595)

prefix this with opaque?

97 ↗(On Diff #23595)

same feedback as above about keeping consistent with opaque-ke terminology

104 ↗(On Diff #23595)

maybe emphasize that this is a new token

111 ↗(On Diff #23595)

we're sure that we don't need to send the CredentialFinalization bytes to the server after getting back the CredentialResponse?

I know we're not using the session key, but want to make sure the server has properly authenticated the client at this point, and is not relying on some piece of information sent in the third message (CredentialFinalization) of the protocol

This revision now requires changes to proceed.Mar 9 2023, 11:35 PM
jon marked 11 inline comments as done.

Address feedback:

  • clarify service comment
  • Make LoginPasswordUser into stream, and finalize OPAQUE
  • Remove GetUserID (at least for now, until we have a use case)
  • signingPublicKey -> deviceEd25519PublicKey
  • align opaque message with opaque_ke terms
shared/protos/identity_client.proto
5 ↗(On Diff #23595)

made it from -> to

// RPCs from a client (iOS, Android, or web) to identity service

To show client vs server distinction

18 ↗(On Diff #23595)

Personally I like the separation, I think it makes is a lot easier to parse. Currently it's being used to block the CRUD account RPCs vs the others.

21 ↗(On Diff #23595)

There's nothing that should need it short term, so why not.

37 ↗(On Diff #23595)

I would rather have deviceEd25519PublicKey, as primary makes it sound as if you're registering the primary device which is out of scope for this work.

clientEd25519PublicKey might be more appropriate if keyservers will never touch this endpoint.

50 ↗(On Diff #23595)

If we have to handle them differently, then they should be different.

I'm still a little confused on the use case of many prekeys, so I'll defer to @ashoat on this.

From my understanding of X3DH, this should be:

// Split SessionInitialization into two parts:
//   GetSessionInitializationInfo: Data needed to start a session with another user
//   PutSessionInitializationInfo: Data needed for other start a session with you
message GetSessionInitializationInfo {
  string payload = 1; // JSON encoded Identity key + signed PreKey (IK + SPK)
  string payloadSignature = 2; // ed25519 prekey signature: Sig(IK, Encode(SPK))
  optional string socialProof = 3; // signed message used for SIWE (optional)
  string defaultOpk = 4; // one-time prekey used for all non-notif traffic with user
  string notifOpk = 4; // one-time prekey used for issuing notifs to user
}

// Part of ClientRegistrationRequest
message PutSessionInitializationInfo {
  string payload = 1; // JSON encoded Identity key + signed PreKey (IK + SPK)
  string payloadSignature = 2; // ed25519 prekey signature: Sig(IK, Encode(SPK))
  optional string socialProof = 3; // signed message used for SIWE (optional)
  repeated string onetimePrekeys = 4; // ~10 or more onetime keys. Used to establish new connections.
}

The need for different "threads" of communication (identity vs notifs) can be achieved by using different onetime prekeys.

111 ↗(On Diff #23595)

I was initially wrong, we still need to finish on the server side. https://docs.rs/opaque-ke/2.0.0/opaque_ke/struct.ServerLogin.html#method.start

the ServerLogin::start creates a challenge, which isn't verified until ServerLogin::finish

shared/protos/identity_client.proto
50 ↗(On Diff #23595)
  1. Signed prekeys need to be separated out of payload. Since they are recycled, they will be delivered separately. They should be listed separately in both Get / Put
  2. You have inconsistent naming for the one-time prekeys between Get / Put... would be good to be consistent
  3. Having an initial set of one-time prekeys is def a good idea, but we need separate payloads for refreshing the list of one-time keys as well as recycling the signed prekey
ashoat requested changes to this revision.Mar 10 2023, 11:14 AM
This revision now requires changes to proceed.Mar 10 2023, 11:14 AM

Limit diff to just account actions

Split opaque RPCs into two unary RPCs

ashoat requested changes to this revision.Mar 13 2023, 11:48 AM

Mostly requesting changes to revise this API to get rid of all streams for gRPC-web compatibility, but see inline comments

shared/protos/identity_client.proto
52–53 ↗(On Diff #23699)

We might consider "combining" these in some way as a single value called signedPreKey (eg. it could be a JSON blob containing these two values). I don't feel strongly, but worth noting that these two should always be sent / uploaded together in all APIs

100 ↗(On Diff #23699)
  1. We name this signingPublicKey elsewhere. Should we be consistent?
  2. Do we even need to provide this separately given it's already provided via the DeviceKeyUpload?
162 ↗(On Diff #23699)
  1. We name this deviceEd25519PublicKey elsewhere. Should we be consistent?
  2. Do we even need to provide this separately given it's already provided via the DeviceKeyUpload?
190 ↗(On Diff #23699)
  1. We name this deviceEd25519PublicKey elsewhere. Should we be consistent?
  2. Do we even need to provide this separately given it's already provided via the DeviceKeyUpload?
This revision now requires changes to proceed.Mar 13 2023, 11:48 AM
shared/protos/identity_client.proto
52–53 ↗(On Diff #23699)

I separated them out to avoid having to deserialize and serialize the information often. I was optimizing for the "how much do I need to do in code" rather than "how is this thought of conceptually in crypto".

Personally, I would like to just have a comment saying that the signature is also required. But wondering what @varun thinks.

162 ↗(On Diff #23699)

my arc diff update landed right before you submitted your review, I'm not sure which line this is referring to.

shared/protos/identity_client.proto
162 ↗(On Diff #23699)

I see now, the JSON encoded payload will contain the same key.

I'll defer to @varun since this was carried over from the previous RPC.

My personal inclination is to remove the duplication, and just parse of the keys, as we should be validating the IdentityKey payload anyway.

This comment was removed by ashoat.
shared/protos/identity_client.proto
162 ↗(On Diff #23699)

You can click the link to figure it out?

My personal inclination is to remove the duplication, and just parse of the keys, as we should be validating the IdentityKey payload anyway.

I agree

I'm also asking for consistent naming of the ed25519 primary device public key

signingPublicKey -> deviceEd25519PublicKey

For now, I renamed them all deviceEd25519PublicKey as I think that was the goal initially

Cool! My feedback now is that deviceEd25519PublicKey should be removed to dedup, but I think @jon is waiting on @varun to confirm before making that change

yeah let's de-dupe and remove deviceEd25519PublicKey. one other nit inline

shared/protos/identity_client.proto
195 ↗(On Diff #23704)

opaqueLoginResponse

opaqueServerResponse -> opaqueLoginResponse

jon added inline comments.
shared/protos/identity_client.proto
52–53 ↗(On Diff #23699)

Talked to varun offline, and we came to agree that we should have separate fields to avoid serializing/deserializing the values.

Still need to delete all instances of deviceEd25519PublicKey

shared/protos/identity_client.proto
24–27 ↗(On Diff #23723)

stream should be removed

jon marked an inline comment as done.

Remove stream from update RPC, remove deviceEd25519 fields

Awesome, let's land this thing!!

This revision is now accepted and ready to land.Mar 14 2023, 12:21 PM