-
Notifications
You must be signed in to change notification settings - Fork 384
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
MSC2444: peeking over federation via /peek #2444
base: old_master
Are you sure you want to change the base?
Conversation
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.
Seems to be in the right direction, just some clarification-style questions.
Co-authored-by: Travis Ralston <[email protected]>
A first cut at an MSC to define how to massively speed up joins (and MSC #2444 peeks) by sending irrelevant events after you join rather than up front
XXX: it might be better to split this into two operations: first fetch the | ||
state data, then begin the peek operation by sending your idea of the forward | ||
extremities, to bring you up to date with anything you missed. This would | ||
reduce the chance of having to immediately cancel a peek, and would be more | ||
efficient in the case of rapid `peek-unpeek-peek` switches. |
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.
I'm leaving this open for now, pending work on an implementation, which I hope will provide guidance.
### Joining a room | ||
|
||
When the user joins the peeked room, the peeking server should just emit the | ||
right membership event rather than calling `/make_join` or `/send_join`, to |
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.
❤️
of idempotency. The ID must be unique for a given `{ peeking_server, room_id, | ||
target_server }` tuple, and should be a string consisting of the characters | ||
`[0-9a-zA-Z.=_-]`. Its length must not exceed 8 characters and it should not be | ||
empty. |
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.
Can we relax these requirements? Reasoning is that we often just want to use the room ID as the peek ID as in https://github.com/matrix-org/dendrite/pull/1391/files#r561821829
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.
why not use a fixed constant, given it is scoped to room id anyway?
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.
(I have a vague memory that I convinced myself that you needed to support more than one-peek-per-room and hence it was insufficient to use either a fixed constant or room id, but in any case I can't see the advantage of using room id rather than a fixed MY_PEEK
)
"content": { /* ... */ } | ||
} | ||
], | ||
"renewal_interval": 3600000 |
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.
"renewal_interval": 3600000 | |
"room_version": "6", | |
"renewal_interval": 3600000 |
`[0-9a-zA-Z.=_-]`. Its length must not exceed 8 characters and it should not be | ||
empty. |
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.
8 chars is too few for large instances.
`[0-9a-zA-Z.=_-]`. Its length must not exceed 8 characters and it should not be | |
empty. | |
`[0-9a-zA-Z.=_-]`. It shall not be empty. |
* You can't use rooms as generic pubsub mechanisms for synchronising data like | ||
profiles, groups, reputation lists, device-lists etc if you can't peek into | ||
them remotely. | ||
* Matrix-speaking search engines can't work if they can't peek remote rooms. |
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.
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.
Just a couple-a clarification notes before i take a crack at implementing this
* listed in the `auth_events` field of any of the above events, or: | ||
* listed in the `auth_events` of the `auth_events`, recursively. | ||
|
||
* `renewal_interval`: a duration in milliseconds after which the target server |
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.
Wouldn't seconds make more sense here? I don't think any value under a second is going to be reasonably useful.
* `common_state_ids`: A list of the IDs of any events which are common to the room | ||
states after *all* of the forward extremities in the room. | ||
|
||
* `events`: The bodies of any events whose IDs are: |
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.
Arent these, in effect, only state events then? The language makes it ambiguous as to what events, exactly, would be included here.
Also, the language makes it uncertain as to what the exact body would look like of these events, should these be stripped down state events (like StrippedState
as referenced under /sync
)?
"events": [ | ||
{ | ||
"type": "m.room.member", | ||
"room_id": "!somewhere:example.org", |
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.
Wouldn't room_id
be redundant here, considering we were querying the room id in the first place?
Also, see my note about the layout of events here (in another comment).
If the room is not peekable, the target server should return a 403 error with | ||
`M_FORBIDDEN`. |
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.
Under which conditions? A room has to be globally readable? (for explicitness' sake)
If the room is not known to the target server, it should return a 404 error | ||
with `M_NOT_FOUND`. |
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.
Can't a server leak metadata about having joined a room by another server trying every known room ID on all servers it knows about? Shouldn't plausible deniability (always 404) be the default response here?
* MSC1777 offers a solution for EDU transmission which this MSC does not, | ||
given we don't currently have any data flows for mirroring other servers' | ||
EDUs. |
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.
I question how useful some EDUs (such as typing, or even read receipts) would be in a peeked room. Unless an EDU type is added which is also critical to a room's data, I don't foresee this being a problem.
* It may be useful to allow peeking servers to "filter" the events to be | ||
returned - for example, if you only care about particular events, or | ||
particular servers - e.g. if load-balancing peeking via multiple servers. |
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.
About filters; Is it still useful to accommodate for them in a forward-compatible manner? I think that just opening a singular peek to a room, without peek IDs, would be a best way forward for now, it'd be easy to add in the /{peek_id}
/ "peek_id": "{peek_id}"
parts of the API later on, right now they feel like incomplete elements which a future MSC defining filters should instead be adding on.
* A malicious server could set up multiple peeks to multiple target servers by | ||
way of attempting a denial-of-service attack. Server implementations should | ||
rate-limit requests to establish peeks, as well as limiting the absolute | ||
number of active peeks each server may have, to mitigate this. |
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.
Some care should be taken when ratelimiting legitimate-interest rooms, such as profiles, if a large server is interested in the profiles of another large server. Maybe these ratelimits should apply to normal rooms that users peek into.
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.
Additionally, a server might also deny peeking into a profile room if the server shares no rooms with the other server that that requested-profile-user is also into, for extra privacy protection. This may be worth noting.
How do we handle backpressure or rate limiting on the event stream (if at | ||
all?) |
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.
(I think this is missing an XXX:
)
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.
Also, re: the comment; I think currently it should be handled like normal federation mechanics, where delays in /send
requests already sorta back-pressure other servers sending federation events. This does it on a server-to-server basis, not a room basis, but discussing solutions to that is out of scope for this MSC.
How do we handle backpressure or rate limiting on the event stream (if at | ||
all?) | ||
|
||
## Dependencies |
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.
(Shouldn't this be "dependants"? :P)
A new MSC for proposing how peeking over federation could work, hopefully replacing #1777
This one introduces the idea of a
/peek
API in S2S which lets a peeking server subscribe to events from given server(s) so it can have a read-only peek view of peekable rooms. This is based on the review of #1777 in Oct 2019.Rendered
Obsoletes #1777