-
Notifications
You must be signed in to change notification settings - Fork 388
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
MSC3059: Limits API — Part 3: Federated rate limiting #3059
Conversation
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
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.
This describes a pretty elaborate rate limiting logic, without justifying the complexity. Given that rate limiting as described here gets intertwined with state resolution (affecting auth chains, unless I'm mistaking), I'd expect much more careful and detailed observation of why's and how's than this MSC provides as yet. Also, similar to Part 1, it's not clear if the per-user limiting should be added to CS API at all.
|
||
Rate limit events in an end-to-end encrpyted room that only cover end-to-end | ||
encrypted events shall also be sent end-to-end encrypted, and otherwise be | ||
rejected by the homeserver as unauthorised. |
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 would homeservers enforce such rate limits if they can't decrypt those, by definition?
In more detail: The limits shall be enforced by the initiating user's homeserver, | ||
based on the server's own time of receiving the event and checked by other servers | ||
and receiving clients, again based of the same `origin_server_ts` value. An event | ||
is to be rejected immediately if it exceeds the rate limit set for that kind of event. |
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.
Prefer active voice over passive - in this particular case, it's not clear if it's just the initiating user's homeserver that rejects the event, or also other parties you mentioned in the previous sentence (and in that latter case you really should leave a note on how that works given that homeservers' wallclocks can, and do, come in ridiculous discrepancy with each other; also in that latter case there's an opportunity to spoof timestamps, thereby evading rate limits).
unless all the limit has entirely been spent by rate limiting events. | ||
If a rate limit is reached by a rate limiting event otherwise, those | ||
actions are to be taken in order, until rate limits are obeyed again: | ||
1. The last rate limit of the same scope sent from same homeserver |
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.
"Last" by which order?
encrypted events shall also be sent end-to-end encrypted, and otherwise be | ||
rejected by the homeserver as unauthorised. | ||
|
||
## Per-user per-server event rate limiting semantics |
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 this whole group the same question as for Limits Part 1 applies: how much should this really be unified across homeserver implementations, as long as it's down to homeserver admins to set these anyway? And by the way, how do you check authorisation?
and at most `n` roles from the list, `exclude({n})`, exclude exactly `n` roles | ||
from the list. `all` is a placeholder representing all the roles in the list | ||
and can be used as a parameter in those inclusion or exclusion operators. | ||
The valid combinatoric operators for users are `include` and `exclude`. |
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.
Is there a practical need for that? That defines some elaborate machinery - have you seen a need for that in real-world situations?
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.
Microsoft Exchange and on-premises installations of Office 365 (and by extension, Microsoft Teams) have a group policy feature allowing domain managers and team managers to enable/disable various features using policies against users/groups. The proposed scoping mechanism is intended to provide a similar override mechanism on Matrix (though, only for lowering the limits or disabling a feature completely, not for increasing the limits), as a second layer over usual level or role based access control.
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.
Group policies in Active Directory are a huge beast, developed over a couple of decades(!). I really urge you to not try to stuff it in here. Also, "because has it" is a rather poor rationale. Again, you should explain WHY on this MSC's own merits, not something else's.
This MSC brings long wanted per-user rate limiting API, with various scoping choices.
Rendered