This meant to get consensus on how a device should
communicate with the identity service.
Details
Details
N/A, this is more about the design of the service than testing
Diff Detail
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
Comment Actions
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.
Comment Actions
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. :(
Comment Actions
- 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?
- I don't understand why we're separating into two services, can you explain that?
Comment Actions
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. |
Comment Actions
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.
Comment Actions
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:
|
| 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) | |
| 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 |
Comment Actions
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) |
|
Comment Actions
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) |
|
| 162 ↗ | (On Diff #23699) |
|
| 190 ↗ | (On Diff #23699) |
|
| 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. |
Comment Actions
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
Comment Actions
For now, I renamed them all deviceEd25519PublicKey as I think that was the goal initially
Comment Actions
yeah let's de-dupe and remove deviceEd25519PublicKey. one other nit inline
| shared/protos/identity_client.proto | ||
|---|---|---|
| 195 ↗ | (On Diff #23704) | opaqueLoginResponse |
| 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. |
Comment Actions
Still need to delete all instances of deviceEd25519PublicKey
| shared/protos/identity_client.proto | ||
|---|---|---|
| 24–27 ↗ | (On Diff #23723) | stream should be removed |