Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSC3202: Encrypted appservices #3202
base: old_master
Are you sure you want to change the base?
MSC3202: Encrypted appservices #3202
Changes from all commits
d56527b
efabfe4
3f394a8
f551ae5
4f6e256
9001a67
472226b
937e4c5
9893a5f
208decb
e586c39
f875ffe
47d2080
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should the AS distinguish between users without keys and users with unchanged keys if you can omit them for both cases?
In my implementation thus far I've been sending empty dicts for the 'without keys' case. I'm now reading that this is unnecessary*, but would be interested in the answer to the above.
*I'm especially sceptical of 'no unused fallback keys' and 'no change in your unused fallback keys' having the same representation over the wire (and thus being indistinguishable). I personally think it's 'wrong' :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSC2732 has this to say about including the field in /sync: The device_unused_fallback_key_types parameter must be present if the server supports fallback keys. Clients can thus treat this field as an indication that the server supports fallback keys, and so only upload fallback keys to servers that support them.
My personal opinion here is that the per-user entries should be omissible if and only if there have been no changes for that user.
If we wanted to require an indication that the homeserver supports fallback keys, we'd only need to require that the outer dict be present (rather than all the users' entries) — don't know if it makes sense to require this though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agh, after reading my code, I notice that I also include empty lists for all the devices if there are no unused fallback keys. I think this is in keeping with what
/sync
does, but again, it would be nice to have this commented on specifically by the MSC so I know whether what I'm doing is right or wrong before it gets merged.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory the appservice is doing its own state tracking for whether or not it has uploaded keys (of any variety). If for some reason it loses this state, it can do two things to recover:
So it should be perfectly safe to exclude fields in both cases, though the MSC is also assuming the server is working off a transaction model under the hood and over the wire: it's a lot easier to see that the appservice was already informed of a change when there's a transaction log to say that was sent.
Whether the server supports fallback keys or not is somewhat irrelevant for appservices: if an appservice requires it, it can just mark it in documentation for server compatibility. Plus, with the new Matrix versioning, the appservice can declare a minimum supported spec version that operators can compare against their homeserver installs. Clients are a bit different because it's a lot easier to accidentally/on purpose use a modern client against an old server, so the feature check becomes a bit more important (at least while fallback keys weren't in a spec release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@turt2live Mostly reasonable, but I'm left with the question of: how is the appservice meant to distinguish between 'no difference in your fallback keys' and 'all your fallback keys have been consumed' if both of them are represented by omission?
My suggestion for how this should work would be: if a device entry is specified, then the list (UFBKs)/dict (OTK counts) is the whole new state for that device and replaces the AS's current state for that device. If a device entry is omitted, then there's no change.
In this system, 'all your fallback keys have been consumed' would be represented by the empty list
[]
of fallback keys. Thereafter it could theoretically be omitted if the state continues to be 'no keys'.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The state change of being unused to used would be in a transaction, but further transactions don't need to include it. This is represented by
[]
already.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect; I think the wording of the '...users without keys can be omitted...' should be altered to clarity that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what should be the HTTP and matrix-level error code if it's not valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for lack of a better answer, let's reuse 400
M_EXCLUSIVE
for now. We'll probably want to introduce an error code in the future.