[protos][identity] Add RestoreUser RPCs
ClosedPublic
Actions

Authored by bartek on Sep 2 2024, 11:20 PM.

Details

Reviewers

ashoat
varun

Commits

rCOMMfbe4ab97d935: [protos][identity] Add RestoreUser RPCs

Summary

Address ENG-8210.

Added RestorePasswordUser and RestoreWalletUser RPCs. These are described as "a single RPC" in the Whitepaper 5.2.
The API is very similiar to LogInPasswordUser[Start] / LogInWalletUser, with the additional device list payload. Also,
we're not doing OPAQUE login here for password users.

Test Plan

Flow, identity compiles.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Sep 2 2024, 11:20 PM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptSep 2 2024, 11:20 PM

Harbormaster completed remote builds in B31397: Diff 43843.Sep 2 2024, 11:41 PM

bartek added a child revision: D13226: [identity] Implement RestoreUser RPCs.Sep 2 2024, 11:43 PM

bartek added a child revision: D13230: [native_rust_library] Add RestoreUser RPCs to client.Sep 2 2024, 11:53 PM

bartek published this revision for review.Sep 3 2024, 12:04 AM

bartek added inline comments.

shared/protos/identity_unauth.proto
241–242 ↗	(On Diff #43843)	We're not doing OPAQUE for password users here, so I wondered about replay attack protection. Not sure if this is required, since we're requiring both `curPrimarySignature` and `lastPrimarySignature` in the signed device list.

It would be helpful to include the parts of the whitepaper you're implementing:

Screenshot 2024-09-05 at 1.59.00 AM.png (1×1 px, 220 KB)

The referenced "Key Upload (6.2.2)" is here:

Overall I don't think the dichotomy between password and Ethereum users makes sense here, given the protocol is substantially similar, and given that we're trying to get rid of this distinction going forward.

Maybe we could have a single RPC that has an optional input of a social proof? If the account is using an Ethereum wallet as their display name, then the RPC will fail without a new social proof. What do you think?

shared/protos/identity_unauth.proto
240 ↗	(On Diff #43843)	Why username instead of user ID? What does this look like in the world where we get rid of the password vs Ethereum user dichotomy?
241–242 ↗	(On Diff #43843)	What's "replay attack" mean in this context? What exactly is being replayed? Usually "replay attack protection" is in the context of something like a signed nonce being replayed, but it's unclear to me why we would need to use a nonce in the first place.

This revision now requires changes to proceed.Sep 4 2024, 11:04 PM

In D13221#373282, @ashoat wrote:

Overall I don't think the dichotomy between password and Ethereum users makes sense here, given the protocol is substantially similar, and given that we're trying to get rid of this distinction going forward.

Maybe we could have a single RPC that has an optional input of a social proof? If the account is using an Ethereum wallet as their display name, then the RPC will fail without a new social proof. What do you think?

Generally, I don't mind merging these into one RPC. The main reason I split it into two was consistency with existing Login/Register RPCs

shared/protos/identity_unauth.proto
240 ↗	(On Diff #43843)	Why username instead of user ID? When restoring backup, we don't have access to user ID at this point. The `ct1` (backup keys) doesn't contain this information. Instead we have username/wallet address that is needed anyway to download and decrypt backup. Alternatively, we can call the `FindUserID` RPC before, but this makes clients call two RPCs instead of one. What does this look like in the world where we get rid of the password vs Ethereum user dichotomy? We can call this `user_identifier`, which can be either a username or wallet address, depending on the backup's security.
241–242 ↗	(On Diff #43843)	Sorry for the confusion, I was blindly following the `UploadKeysForRegisteredDeviceAndLogIn` RPC logic, which requires a nonce

Discussed my above feedback with @bartek today in our 1:1. We identified several potential paths forward for unifying these two RPCs:

Take userID as input here instead of username
- The backup service could return the userID in the initial RPC
- Alternately, if that is too complicated for some reason, the client could get the userID via FindUserID, but this would require an additional RPC
Take oneof identifier as input here, to match FindUserIDRequest
- This is probably slightly worse as it might require us to change / update this RPC later as part of ENG-8414

Based on this discussion it looks like the backup service doesn't query for the userID at all currently, and instead just uses the username. We'll need to update that as part of this work, so it's probably a good time to consider option 1a.

In D13221#373423, @ashoat wrote:

Discussed my above feedback with @bartek today in our 1:1. We identified several potential paths forward for unifying these two RPCs:

Take userID as input here instead of username

The backup service could return the userID in the initial RPC

Alternately, if that is too complicated for some reason, the client could get the userID via FindUserID, but this would require an additional RPC

Take oneof identifier as input here, to match FindUserIDRequest

This is probably slightly worse as it might require us to change / update this RPC later as part of ENG-8414

Based on this discussion it looks like the backup service doesn't query for the userID at all currently, and instead just uses the username. We'll need to update that as part of this work, so it's probably a good time to consider option 1a.

Option 1a makes the most sense to me too. This implies the following flow:

User asks Backup Service for backupID and userID, given username/wallet address as input
Backup Service queries Identity Service for userID, retrieves appropriate backupID and returns both values to the user
- Updating Backup endpoint to query for user ID is tracked in ENG-6145
User calls RestoreUser RPC, providing userID returned from Backup Service as input