MSC4162: One-Time Key Reset Endpoint #4162
Conversation
|
|
||
| ### Proposal | ||
|
|
||
| #### Reset endpoint |
There was a problem hiding this comment.
There is a list of why OTKs could get desync, and an end-point to reset and re-resync.
But how technically do we detect that there is a desync? Unless I missed it I don't think it's noted here.
Would a new end-point that would allow to check that client/server are in sync would be helpfull?
Or will this be always detected too late?
There was a problem hiding this comment.
We can't detect desyncs because the client and server are not continually checking (and they do need to continually check, as DB corruption / rollbacks can happen at arbitrary points in time). Only /keys/upload does some basic sync checks by ensuring that if the client says key ID AAAAA is bytes X that it remains the case on the next upload (assuming the key hasn't been used).
| @@ -0,0 +1,90 @@ | |||
| ## MSC4162: One-Time Key Reset Endpoint | |||
There was a problem hiding this comment.
nit: Looks like this MSC is covering more than just reset.
|
|
||
| This does not define a sort ordering (i.e _which_ key should be given out to the caller). This MSC proposes tweaking this wording to be: | ||
|
|
||
| > Claims one-time keys for use in pre-key messages. The key claimed MUST be the lowest lexicographically sorted key ID for that algorithm. For example, given two key IDs of `AAAAAu` and `AAAAHg`, `AAAAAu` MUST be returned before `AAAAHg`. |
There was a problem hiding this comment.
I think there have been discussion for the KeyId to be the public key and not an "increment" like now. Would it be possible to have conflicts here? or it's not important? What about just using a timestamp ?
There was a problem hiding this comment.
There needs to be a way to sort the keys and perhaps more importantly, to generate new keys > old keys. Base 64 public keys don't allow for this, so that would make sorting impossible. We'd probably need to use timestamps.
There was a problem hiding this comment.
Per element-hq/element-meta#2356 (comment): lexicographical ordering on key IDs turns out to be somewhat disastrous anyway
|
|
||
| ### Rationale | ||
|
|
||
| The primary motivation of this endpoint is to provide a way for a client and server to resync OTKs. To do this, a client would call `/keys/reset` with `all: true` to delete all OTKs on the server. Once this 200 OKs, the client is then safe to perform `/keys/upload` with a fresh set of OTKs. This functionality is particularly important for existing clients who currently have desynced OTKs. |
There was a problem hiding this comment.
I wonder if we don't want something a bit different for fallback keys as they have a longer life time (option to not reset them?). As it may interfer with #4081
There was a problem hiding this comment.
Fallback keys are not one-time keys, so wouldn't be reset here. Fallback keys can be replaced in /keys/upload so a new endpoint isn't really needed.
turt2live
added
proposal
|
|
||
|
|
||
| ### Context | ||
| One-time keys (OTKs) consist of two parts: a public part which is stored on the server and a private part which is stored on the client device. One-time keys can be uploaded to the server with the `/keys/upload` endpoint. They can then be consumed via the `/keys/claim` endpoint. The client is kept informed of the total number of OTKs stored on the server via the `/sync` endpoint, with the JSON key `device_one_time_keys_count`. It is critical that the set of keys on the client and server remain in-sync, otherwise an encrypted channel between two devices cannot be established. "In-sync" is defined by the set of public keys on the server having their matching private keys on the client. |
There was a problem hiding this comment.
Please wrap lines to ~100 characters
There was a problem hiding this comment.
Implementation requirements:
- Client
- Server
|
|
||
| #### Client issues | ||
|
|
||
| - If the request to `/keys/upload` fails, the client must retry (including over restarts) the _same set of keys_ as it is not possible to know if the server received the request and has stored them already. If this doesn't happen, the server may return HTTP 400 errors indicating this. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1415)] |
There was a problem hiding this comment.
This can probably be fixed without a whole new endpoint?
There was a problem hiding this comment.
Yes, but it outlines a common failure mode which causes OTKs to desync.
There was a problem hiding this comment.
It would probably be helpful to break this down into "implementation bugs which could theoretically be fixed" vs "real protocol problems" (such as backup rollback)
| - If the client runs across multiple processes, both processes can upload different sets of OTKs in response to some trigger event (e.g being notified that a OTK has been consumed via `/sync`). [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/3110)] | ||
| - Clients should only delete a OTK once the key has been successfully used. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1761)] |
There was a problem hiding this comment.
These two feel like implementation bugs rather than spec concerns.
There was a problem hiding this comment.
Yes, but it outlines a common failure mode which causes OTKs to desync.
| - Server databases may be rolled back (e.g during a bad upgrade process). | ||
|
|
||
| #### Protocol issues | ||
| - Clients only store a limited set of OTKs. When they delete excess OTKs they have no mechanism to tell the server to delete those old OTKs. |
There was a problem hiding this comment.
Why are they deleting them? If it's just good hygiene, do we add expiry timestamps to the /upload endpoint instead?
There was a problem hiding this comment.
Clients delete them to not be forced into storing an unbounded set of OTKs. Clients try to keep to a number (typically 50 or 100) and when the server says the count is < this the client uploads more. But it can't delete any key as there may be in-flight requests. Hence, you can cause clients to use an unbounded amount of disk space by repeatedly telling the client that the OTK count is 0. This is bad. JS SDK and Rust SDK have maximum bounds to the number of OTKs it will store, currently this is set to 5000.
|
|
||
| #### Protocol issues | ||
| - Clients only store a limited set of OTKs. When they delete excess OTKs they have no mechanism to tell the server to delete those old OTKs. | ||
| - Multiple users could hit `/keys/claim` at the same time as a client is uploading OTKs, which creates race conditions which can cause the same key to be sent to multiple users. [[issue](https://github.com/matrix-org/matrix-spec/issues/1124)] |
There was a problem hiding this comment.
This feels like an implementation issue, though the spec may need to be clarified how the locking works.
| - Clients only store a limited set of OTKs. When they delete excess OTKs they have no mechanism to tell the server to delete those old OTKs. | ||
| - Multiple users could hit `/keys/claim` at the same time as a client is uploading OTKs, which creates race conditions which can cause the same key to be sent to multiple users. [[issue](https://github.com/matrix-org/matrix-spec/issues/1124)] | ||
|
|
||
| ### Proposal |
There was a problem hiding this comment.
The problem feels under-described: why is a reset required?
There was a problem hiding this comment.
The primary motivation of this endpoint is to provide a way for a client and server to resync OTKs. [...] This functionality is particularly important for existing clients who currently have desynced OTKs.
There's existing devices who have desynced OTKs. This is guaranteed to cause UTDs when those keys get used. It would be nice to fix that.
There was a problem hiding this comment.
As I see it, there are basically three usecases for a full reset:
- We know we have bugs in clients; we should fix those, but in the meantime we can improve resilience for selected deployments by implementing a healing mechanism.
- We know we have bugs in clients; we will fix those in time, but that doesn't help people who are already broken. We will need a healing mechanism for those users.
- There are some problems (in particular, client or server rollback from backup) that aren't really bugs, but will result in getting out of sync. Again, we need a healing mechanism.
Our best idea for resolving all three situations is to blow away all the OTKs on the server and start again.
It would be good if the MSC spelled this out.
Co-authored-by: Andy Balaam <[email protected]>
|
|
||
| #### Client issues | ||
|
|
||
| - If the request to `/keys/upload` fails, the client must retry (including over restarts) the _same set of keys_ as it is not possible to know if the server received the request and has stored them already. If this doesn't happen, the server may return HTTP 400 errors indicating this. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1415)] |
There was a problem hiding this comment.
I don't really follow this. Are you saying "clients might forget to re-upload keys if they do not get a successful response"? Why would that cause a 400?
|
|
||
| #### Client issues | ||
|
|
||
| - If the request to `/keys/upload` fails, the client must retry (including over restarts) the _same set of keys_ as it is not possible to know if the server received the request and has stored them already. If this doesn't happen, the server may return HTTP 400 errors indicating this. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1415)] |
There was a problem hiding this comment.
It would probably be helpful to break this down into "implementation bugs which could theoretically be fixed" vs "real protocol problems" (such as backup rollback)
|
|
||
| - If the request to `/keys/upload` fails, the client must retry (including over restarts) the _same set of keys_ as it is not possible to know if the server received the request and has stored them already. If this doesn't happen, the server may return HTTP 400 errors indicating this. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1415)] | ||
| - If the client runs across multiple processes, both processes can upload different sets of OTKs in response to some trigger event (e.g being notified that a OTK has been consumed via `/sync`). [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/3110)] | ||
| - Clients should only delete a OTK once the key has been successfully used. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1761)] |
There was a problem hiding this comment.
| - Clients should only delete a OTK once the key has been successfully used. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1761)] | |
| - Clients may incorrectly delete the private part of an OTK before it is successfully used. [[issue](https://github.com/matrix-org/matrix-rust-sdk/issues/1761)] |
|
|
||
| This does not define a sort ordering (i.e _which_ key should be given out to the caller). This MSC proposes tweaking this wording to be: | ||
|
|
||
| > Claims one-time keys for use in pre-key messages. The key claimed MUST be the lowest lexicographically sorted key ID for that algorithm. For example, given two key IDs of `AAAAAu` and `AAAAHg`, `AAAAAu` MUST be returned before `AAAAHg`. |
| A new endpoint `POST /keys/reset` is added. It accepts the following request body: | ||
|
|
||
| ```js | ||
| { | ||
| "all": true|false | ||
| "key_ids": [ | ||
| "<algorithm>:<key_id>", ... | ||
| ] | ||
| } | ||
| ``` | ||
| - If `all` is `true`, all stored one-time keys for the calling device are deleted on the server, and `key_ids` is ignored. | ||
| - If `all` is `false`, every key specified in `key_ids` is deleted from the database. An example key ID is `signed_curve25519:AAAAHQ`. |
There was a problem hiding this comment.
This feels like two separate endpoints to me. Indeed maybe we should make the selective-delete endpoint a separate MSC: it's quite a different usecase
| - Clients only store a limited set of OTKs. When they delete excess OTKs they have no mechanism to tell the server to delete those old OTKs. | ||
| - Multiple users could hit `/keys/claim` at the same time as a client is uploading OTKs, which creates race conditions which can cause the same key to be sent to multiple users. [[issue](https://github.com/matrix-org/matrix-spec/issues/1124)] | ||
|
|
||
| ### Proposal |
There was a problem hiding this comment.
As I see it, there are basically three usecases for a full reset:
- We know we have bugs in clients; we should fix those, but in the meantime we can improve resilience for selected deployments by implementing a healing mechanism.
- We know we have bugs in clients; we will fix those in time, but that doesn't help people who are already broken. We will need a healing mechanism for those users.
- There are some problems (in particular, client or server rollback from backup) that aren't really bugs, but will result in getting out of sync. Again, we need a healing mechanism.
Our best idea for resolving all three situations is to blow away all the OTKs on the server and start again.
It would be good if the MSC spelled this out.
|
|
||
|
|
||
| ### Context | ||
| One-time keys (OTKs) consist of two parts: a public part which is stored on the server and a private part which is stored on the client device. One-time keys can be uploaded to the server with the `/keys/upload` endpoint. They can then be consumed via the `/keys/claim` endpoint. The client is kept informed of the total number of OTKs stored on the server via the `/sync` endpoint, with the JSON key `device_one_time_keys_count`. It is critical that the set of keys on the client and server remain in-sync, otherwise an encrypted channel between two devices cannot be established. "In-sync" is defined by the set of public keys on the server having their matching private keys on the client. |
There was a problem hiding this comment.
It is critical that the set of keys on the client and server remain in-sync
I feel like this is misleading. What we really mean is "the set of keys on the server is a subset of those on the client". It's not a problem if the client has extra keys (generally, we assume they've been claimed but not used, but in any case they are harmless, modulo the fact they take up slots in the client).
Rendered