Skip to content
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

Open
wants to merge 21 commits into
base: old_master
Choose a base branch
from
Open

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Feb 24, 2020

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

@ara4n ara4n added the proposal A matrix spec change proposal label Feb 24, 2020
@turt2live turt2live self-requested a review February 25, 2020 21:11
Copy link
Member

@turt2live turt2live left a 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.

proposals/2444-peeking-over-federation-peek-api.md Outdated Show resolved Hide resolved
proposals/2444-peeking-over-federation-peek-api.md Outdated Show resolved Hide resolved
proposals/2444-peeking-over-federation-peek-api.md Outdated Show resolved Hide resolved
proposals/2444-peeking-over-federation-peek-api.md Outdated Show resolved Hide resolved
proposals/2444-peeking-over-federation-peek-api.md Outdated Show resolved Hide resolved
@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@ara4n ara4n mentioned this pull request Aug 30, 2020
15 tasks
ara4n added a commit that referenced this pull request Sep 14, 2020
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
Comment on lines +144 to +148
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.
Copy link
Member

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
Copy link
Member Author

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.
Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"renewal_interval": 3600000
"room_version": "6",
"renewal_interval": 3600000

Comment on lines +96 to +97
`[0-9a-zA-Z.=_-]`. Its length must not exceed 8 characters and it should not be
empty.
Copy link

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.

Suggested change
`[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.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also inconvenient for restricted spaces where the space just appears as an internal room ID before you try to join.

Copy link
Contributor

@ShadowJonathan ShadowJonathan left a 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
Copy link
Contributor

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:
Copy link
Contributor

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",
Copy link
Contributor

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

Comment on lines +125 to +126
If the room is not peekable, the target server should return a 403 error with
`M_FORBIDDEN`.
Copy link
Contributor

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)

Comment on lines +128 to +129
If the room is not known to the target server, it should return a 404 error
with `M_NOT_FOUND`.
Copy link
Contributor

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?

Comment on lines +268 to +270
* 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.
Copy link
Contributor

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.

Comment on lines +283 to +285
* 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.
Copy link
Contributor

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.

Comment on lines +292 to +295
* 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.
Copy link
Contributor

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.

Copy link
Contributor

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.

Comment on lines +314 to +315
How do we handle backpressure or rate limiting on the event stream (if at
all?)
Copy link
Contributor

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:)

Copy link
Contributor

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
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants