MSC4140: rate limit management against delay_id

[AndrewFerr]

Do this instead of rate limiting based on IP address, to avoid the possibility of multiple unauthenticated clients on a shared IP address getting put into the same rate-limit quota and getting unfairly blocked.

This follows the suggestion made by the MSC: https://github.com/matrix-org/matrix-spec-proposals/blob/toger5/expiring-events-keep-alive/proposals/4140-delayed-events-futures.md#rate-limiting-for-heartbeats

Depends on #19794. Either that should be merged first, or this PR should absorb it. (Perhaps this would be a good use-case for stacked PRs.)

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)

[AndrewFerr]

If it would help with the review, I could rebase this PR's branch to exclude commits from #19794, as that is now merged.

[anoadragon453]

Does this weaken the rate-limiting too much? Now that a user can spam requests, as long as they cycle through (effectively unlimited) delay IDs?

I get the reasoning that an external service would end up getting rate-limited if it's managing lots of delay IDs for a bunch of users... but would it not make sense to just exclude such a service from rate-limiting?

And finally, I get that lots of logged-out users on the same corporate VPN would end up getting rate-limited due to sharing one IP... but at this point, what is the rate-limit protecting against? Seems it's only stopping a buggy client from spamming the same endpoint accidentally?

[AndrewFerr]

Does this weaken the rate-limiting too much? Now that a user can spam requests, as long as they cycle through (effectively unlimited) delay IDs?

The worst that such spammy requests would do is make the server look up from a database index (to check for the existence of the target delayed event). IMO this is an acceptable cost to allow for the kind of rate-limiting this PR is after, but if that's still too costly, we can consider another approach.

would it not make sense to just exclude such a service from rate-limiting?

That's not a bad idea, but then the question is how Synapse would identify such an external service, given the "service" is currently just any arbitrary HTTP client that's been given some delay IDs.

what is the rate-limit protecting against? Seems it's only stopping a buggy client from spamming the same endpoint accidentally?

Yes, be it a buggy or malicious client. It takes non-trivial work to reset a delayed event (past the initial quick check for the existence of the delayed event), so it's worth something to protect it with a rate limit.

[AndrewFerr]

It bears mentioning that the long-term solution to all of this is to have the external services be OAuth 2.0 clients, with a token that has scoped access to these management endpoints (detailed in MSC4140). Rate limits could then be applied per client, as the clients would be identifiable by their token. The idea of using delay_id grouping is just a stopgap measure to apply some kind of rate limit in the absence of a way to identify an unauthed external service.