Page MenuHomePhabricator

[identity] Scaffold RPC for device list updates
ClosedPublic

Authored by bartek on Jan 24 2024, 1:58 AM.
Tags
None
Referenced Files
F3284295: D10797.id36606.diff
Sat, Nov 16, 12:04 PM
F3281917: D10797.id36246.diff
Sat, Nov 16, 10:21 AM
Unknown Object (File)
Thu, Nov 7, 2:01 AM
Unknown Object (File)
Fri, Nov 1, 11:35 PM
Unknown Object (File)
Wed, Oct 23, 10:34 PM
Unknown Object (File)
Wed, Oct 23, 9:59 PM
Unknown Object (File)
Sun, Oct 20, 9:39 PM
Unknown Object (File)
Sun, Oct 20, 5:25 PM
Subscribers

Details

Summary

I need only basic part of it for ENG-5425.
Proper implementation is tracked by ENG-5101.

Test Plan

Compiled Identity Service. Implementation and integration tests are done in subsequent diffs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.

Update grpc-web generated files

bartek published this revision for review.Jan 29 2024, 4:11 AM
ashoat requested changes to this revision.Jan 29 2024, 10:22 AM

Some comments on the proto changes

shared/protos/identity_auth.proto
209 ↗(On Diff #36246)

All RawDeviceLists should have a timestamp, determined by the caller.

It's determined by the caller instead of identity, so that the caller can include it in their signature. However, identity verifies that the timestamp is larger than the current one, and that it's recent (within 300 seconds of current time).

I suppose this could get added later, but it's so easy to add now, and I figure it might save us some time. What do you think of adding it now?

211 ↗(On Diff #36246)

In addition to adding the timestamp, at a later point we'll be adding a couple of signatures.

When we add the signatures, we want the RawDeviceList part (definition in whitepaper, but basically the devices and timestamp, but NOT the signatures) to be a raw JSON-stringified string, so that its format is stable and easy to verify the signatures.

As such, I think maybe the format from below makes more sense:

// A stringified JSON object of the following format:
// {
//   "rawDeviceList": JSON.stringify({
//     "devices": [<device_id: string>, ...]
//     "timestamp": <UTC timestamp in milliseconds: int>,
//   })
// }

We can then later add the signatures as keys of the top-level object. (I think that makes more sense than separate proto fields, since we'll want to handle the SignedDeviceList as a discrete item across identity, clients, keyserver, etc.

What do you think?

222 ↗(On Diff #36246)

It's confusing that the output here is named signed_device_list, when the input is named new_device_list. They should both be the same, so I would expect the same now. (Actually, I'm not sure it makes sense to return anything here outside of success/fail, given we expect the output to be the same string as the input.)

Note that identity shouldn't be responsible for signing. I realized this confusion might be based on the task description of ENG-5101... I added this comment to clarify.

This revision now requires changes to proceed.Jan 29 2024, 10:22 AM

Okay, these comments make sense, thank you! I was indeed confused that we're "adding signing later" so I did this differently, but I agree it's good to have correct RPC format from the beginning. I'll update this accordingly

Updated RPC to accept full SignedDeviceList JSON as request payload, removed unused response object

Proto changes and grpc-web changes look good to me! Resigning so that somebody else can take a look at the Rust

This revision is now accepted and ready to land.Feb 6 2024, 6:47 AM