Details
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
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. |
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