Don't ratelimit server-sent delayed events

[AndrewFerr]

Fixes #18021

When the server sends a delayed event upon timeout / on-demand sending, don't apply ratelimiting to the event create+send attempt, because it is a send that the server expects to be able to send.

What should (and does) get rate-limited are client requests to manage delayed events.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct
  (run the linters)

[hughns]

This change goes against the security considerations of MSC4140. Specifically: https://github.com/matrix-org/matrix-spec-proposals/blob/toger5/expiring-events-keep-alive/proposals/4140-delayed-events-futures.md#rate-limiting-at-the-point-of-sending

To make sure that this doesn't get forgotten I've made a tracking issue and added context there: #18021 (comment)

To be clear: I don't think it is a good idea to be removing this rate limit at this point without a planned action of how the risk would be mitigated in future.

[thisisjoesGH]

What should (and does) get rate-limited are client requests to manage delayed events.

The client request ratelimit is trivially overridden by a malicious homeserver without any code modifications since it is exposed in the Synapse config. Synapse should not be relying on other homeservers to play nice. If the proposed fix is merged it seems like this entire feature will just be a massive new abuse vector.

[AndrewFerr]

The client request ratelimit is trivially overridden by a malicious homeserver without any code modifications since it is exposed in the Synapse config.

If you're referring to non-ratelimited delayed events arriving via federation, I believe that would be protected against by federation-level ratelimiting. Otherwise, there'd already be the possibility of a malicious homeserver disabling all ratelimiting & flooding remote rooms with events.

[thisisjoesGH]

If you're referring to non-ratelimited delayed events arriving via federation, I believe that would be protected against by federation-level ratelimiting. Otherwise, there'd already be the possibility of a malicious homeserver disabling all ratelimiting & flooding remote rooms with events.

I misunderstood this change as affecting the processing of incoming events rather than outgoing. In that case, is this a potential federation DoS issue? It appears a user could currently schedule an unlimited number of delayed events. Is "a limit on the maximum number of outstanding delayed events" going to be implemented in Synapse as suggested by the MSC?

[AndrewFerr]

Is "a limit on the maximum number of outstanding delayed events" going to be implemented in Synapse as suggested by the MSC?

Yes, this is something I'm also working on.

[anoadragon453]

I'm going to take this off the review queue until we come up with a workable solution -> #18021

Feel free to add it back on afterwards (if you don't have permissions to, poke someone on the team).

[hughns]

I'm going to take this off the review queue until we come up with a workable solution -> #18021

Feel free to add it back on afterwards (if you don't have permissions to, poke someone on the team).

Marking it as draft until this happens.

[anoadragon453]

@hughns @AndrewFerr is this PR still relevant or should it be closed?

[AndrewFerr]

is this PR still relevant or should it be closed?

It may be closed. Server-sent delayed events should indeed get rate-limited, and the solution for helping users avoid scheduling a delayed event that is fated to get rate-limited is deserving of another PR entirely.

Note that with the notion of looking up "finalised" delayed events, it will be possible to check which delayed events failed to be sent due to being rate-limited (or any other error for that matter): #19038