Support MSC3814: Dehydrated Devices Part 2 #16010
Conversation
H-Shay
force-pushed
the
shay/follow_up_dd
branch
from
b629f08
to
217e2eb
Compare
|
@poljar here is the follow-up PR to the first dehydrated devices PR, I think I got everything discussed on the first one but let me know if I am missing anything or incorrectly implemented the suggestions. |
|
General comment: how (if at all) does this interact with #16046? |
This shouldn't particularly interact with it - they are generally touching different areas of the code. |
synapse/handlers/device.py
Outdated
| async def _check_and_prepare_keys_for_dehydrated_device( | ||
| self, user_id: str, device_id: str, keys: JsonDict | ||
| ) -> dict: |
There was a problem hiding this comment.
Aren't we creating the dehydrated device now? How can it already have keys? Aren't devices unique per user/device ID?
There was a problem hiding this comment.
This is creating/storing the dehydrated device - to your question of how it can already have keys I actually don't know - per the MSC the keys were to be uploaded over the /keys/upload endpoint, implying that they already exist: "After the dehydrated device is uploaded, the client will upload the encryption
keys using POST /keys/upload/{device_id}, where the device_id parameter is
the device ID given in the response to PUT /dehydrated_device"
#15929 changed this so that the key upload was integrated into the PUT endpoint, and this PR is further refining the key upload such that storing the device and storing the keys are part of one single database transaction, addressing @poljar's concerns around storing the dehdrated device and it's keys being atomic.
There was a problem hiding this comment.
Uploading the public keys at the time of the dehydrated device creation tries to address this race: matrix-org/matrix-spec-proposals#3814 (comment).
As to how can it already have keys, well if we remember what a dehydrated device is then it becomes quite clear. A dehydrated device are the private identity and one-time keys of a device, of course they are encrypted so the server can't access them.
It becomes quite natural, that we would like to upload the public parts of those same keys to the server at the same time, once we realize this. I think that doing it in a separate POST /keys/upload/ call is a mistake and quite weird from an API point of view, which the above mentioned race condition showcases.
There was a problem hiding this comment.
OK, but part of this function checks if we already have keys uploaded for this device (and bails if they're not exactly equal to the new keys). I still don't follow the order of events that would let you upload keys before the device is created.
There was a problem hiding this comment.
This could be me being overzealous here - I basically made sure that all the checks that happen in the /keys/upload pathway happen in this pathway, but maybe that's not necessary and we can just store the keys without checking them?
There was a problem hiding this comment.
Part of it would be straightforward (Move _set_e2e_device_keys_txn to a method (instead of an inner function) and call it from both places.)
Although it seems this entire function is just a copy of upload_keys_for_user -- can we just call that instead?
There was a problem hiding this comment.
The way we use the API, they are guaranteed to be unique. Furthermore since we only upload keys once for a dehydrated device there's little chance of getting this wrong.
I think if this is the case we can probably call the store methods that set the values directly and avoid the complicated logic of reusing keys.
There was a problem hiding this comment.
call the store methods that set the values directly
so the reason I didn't do this was that my understanding of what was being asked was that the storing of the keys and the storing of the device needs to all happen in the same transaction - ie in _store_dehydrated_device_txn - wouldn't calling the store methods open a separate transaction?
There was a problem hiding this comment.
They would need to be refactored to take a transaction as the first parameter and current callers would call that also.
There was a problem hiding this comment.
Right I think this is sorted now, sorry for the confusion.
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
…ted_device_txn and do so
| user_id: id of user to get keys for | ||
| device_id: id of device to get keys for | ||
| time_now: insertion time to record (ms since epoch) | ||
| new_keys: keys to add - each a tuple of (algorithm, key_id, key json) |
There was a problem hiding this comment.
Please note that the key JSON must be in canonical JSON form.
Alternatively, update _upload_one_time_keys_for_user and _store_dehydrated_device_txn to not do the encoding and do it internally.
There was a problem hiding this comment.
I've fixed the comment, thanks for pointing that out!
H-Shay
force-pushed
the
shay/follow_up_dd
branch
from
187f933
to
c5f4357
Compare
|
Merged #16010 into develop, thanks for the reviews! |
This is a follow-up PR to #15929 to implement suggestions on that PR. In particular, this PR makes changes such that