Page MenuHomePhabricator

[identity] Add RPC for secondary device login
ClosedPublic

Authored by bartek on Jan 29 2024, 4:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 12:15 PM
Unknown Object (File)
Thu, Nov 7, 2:19 AM
Unknown Object (File)
Mon, Nov 4, 5:50 AM
Unknown Object (File)
Fri, Nov 1, 11:58 PM
Unknown Object (File)
Fri, Nov 1, 11:57 PM
Unknown Object (File)
Fri, Nov 1, 11:57 PM
Unknown Object (File)
Fri, Nov 1, 11:56 PM
Unknown Object (File)
Fri, Nov 1, 11:34 PM
Subscribers

Details

Summary

This diff adds a RPC that is called by secondary device, after receiving information from primary device that it has been added to the device list.

This RPC serves two purposes:

  • Perform a device key upload for newly added secondary device
  • Mint an access token for the secondary device

The implementation is done in the next diff

Depends on D10800

Test Plan
  • Identity service and grpc_cliends compile
  • Flow for grpc-web codegen

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Jan 29 2024, 5:29 AM
ashoat requested changes to this revision.Jan 29 2024, 11:21 AM

Concerned that we're short-circuiting the proper "Existing Device Log-in" protocol here

shared/protos/identity_unauth.proto
29

Would this RPC ever be used in a different case that isn't secondary device auth? I'm wondering if we should pick a different name. For instance, UploadKeysForRegisteredDeviceAndLogIn.

I spent some time looking at the whitepaper, and I can't find any other examples of "New Device and Comm run Key Upload (5.2.2) and Existing Device Log-in (5.3.6)" occurring, so I think we can probably leave it as-is, unless you prefer the alternate name I proposed.

218–220

In the whitepaper, I think this request is described as "New Device and Comm run Key Upload (5.2.2) and Existing Device Log-in (5.3.6)".

I think you're taking a slightly different approach here. Instead of a proper "Existing Device Log-in", you're authing the user just based on their DeviceKeyUpload.

I don't think this is as safe. By skipping the nonce and timestamp steps, you make the system hypothetically vulnerable to replay attacks.

223–227

Realized that this message is repeated many times in this file:

  • RegistrationFinishResponse
  • OpaqueLoginFinishResponse
  • WalletLoginResponse
  • This case: SecondaryDeviceLoginResponse

Is there a reason we're repeating it so many times? Does it make it easier to make changes later if we need to?

This revision now requires changes to proceed.Jan 29 2024, 11:21 AM
bartek added inline comments.
shared/protos/identity_unauth.proto
29

Glad you raised this. Yes, this RPC is intended to be called in this only specific case (QR code auth).
My name isn't perfect but I didn't have any better idea. I'll follow your proposal.

218–220

This is my bad, I treated our password/wallet login RPCs as "Existing Device Log-in (5.3.6)" but now looking at LogInWalletUser RPC implementation I see that it indeed validates the nonce.

223–227

Yeah I noticed this pattern too, so I followed it.
This probably can be merged into one message unless I'm missing something, cc @varun.
If so, I can create a task and do this. But I prefer doing it separately from this stack, for clarity.

Renamed the RPC; added request field for the nonce/timestamp challenge

shared/protos/identity_unauth.proto
218 ↗(On Diff #36611)

Not sure of the best name for this field.

Assuming the RPC for vending the challenge will be handled separately. Proto changes and grpc-web changes look good to me – resigning so that somebody else can take a look at the Rust

shared/protos/identity_unauth.proto
218 ↗(On Diff #36613)

For this to work, don't we need another RPC to vend the challenge?

varun added inline comments.
shared/protos/identity_unauth.proto
223–227

i think we can merge them

This revision is now accepted and ready to land.Feb 6 2024, 7:14 AM
shared/protos/identity_unauth.proto
224–228 ↗(On Diff #36613)

we can remove this now or create a new task to replace this with the message introduced in D11061