Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Describe which rate limiter was hit in logs #16135

Merged
merged 9 commits into from
Aug 29, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/16135.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Describe which rate limiter was hit in logs.
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 12 additions & 2 deletions synapse/api/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,11 @@ def __init__(
def error_dict(self, config: Optional["HomeServerConfig"]) -> "JsonDict":
return cs_error(self.msg, self.errcode, **self._additional_fields)

@property
def debug_context(self) -> Optional[str]:
"""Override this to add debugging context that shouldn't be sent to clients."""
clokep marked this conversation as resolved.
Show resolved Hide resolved
return None


class InvalidAPICallError(SynapseError):
"""You called an existing API endpoint, but fed that endpoint
Expand Down Expand Up @@ -505,17 +510,22 @@ class LimitExceededError(SynapseError):

def __init__(
self,
limiter_name: str,
code: int = 429,
msg: str = "Too Many Requests",
retry_after_ms: Optional[int] = None,
errcode: str = Codes.LIMIT_EXCEEDED,
):
super().__init__(code, msg, errcode)
super().__init__(code, "Too Many Requests", errcode)
self.retry_after_ms = retry_after_ms
self.limiter_name = limiter_name

def error_dict(self, config: Optional["HomeServerConfig"]) -> "JsonDict":
return cs_error(self.msg, self.errcode, retry_after_ms=self.retry_after_ms)

@property
def debug_context(self) -> Optional[str]:
return self.limiter_name
Comment on lines +533 to +535
Copy link
Member

Choose a reason for hiding this comment

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

debug_context could probably just be a class-property too and __init__ could set self.debug_context = limiter_name. I don't feel strongly either way though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong feelings here either. Suggest we leave it as-is in the interests of landing this sooner rather than later.



class RoomKeysVersionError(SynapseError):
"""A client has tried to upload to a non-current version of the room_keys store"""
Expand Down
20 changes: 13 additions & 7 deletions synapse/api/ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,16 @@ class Ratelimiter:
"""

def __init__(
self, store: DataStore, clock: Clock, rate_hz: float, burst_count: int
self,
store: DataStore,
clock: Clock,
cfg: RatelimitSettings,
Copy link
Member

Choose a reason for hiding this comment

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

@sumnerevans pointed out to me that the docstring needs updating for this.

Copy link
Member

Choose a reason for hiding this comment

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

See #16255.

):
self.clock = clock
self.rate_hz = rate_hz
self.burst_count = burst_count
self.rate_hz = cfg.per_second
self.burst_count = cfg.burst_count
self.store = store
self._limiter_name = cfg.key
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

# An ordered dictionary representing the token buckets tracked by this rate
# limiter. Each entry maps a key of arbitrary type to a tuple representing:
Expand Down Expand Up @@ -305,7 +309,8 @@ async def ratelimit(

if not allowed:
raise LimitExceededError(
retry_after_ms=int(1000 * (time_allowed - time_now_s))
limiter_name=self._limiter_name,
retry_after_ms=int(1000 * (time_allowed - time_now_s)),
)


Expand All @@ -322,7 +327,9 @@ def __init__(

# The rate_hz and burst_count are overridden on a per-user basis
self.request_ratelimiter = Ratelimiter(
store=self.store, clock=self.clock, rate_hz=0, burst_count=0
store=self.store,
clock=self.clock,
cfg=RatelimitSettings(key=rc_message.key, per_second=0, burst_count=0),
)
self._rc_message = rc_message

Expand All @@ -332,8 +339,7 @@ def __init__(
self.admin_redaction_ratelimiter: Optional[Ratelimiter] = Ratelimiter(
store=self.store,
clock=self.clock,
rate_hz=rc_admin_redaction.per_second,
burst_count=rc_admin_redaction.burst_count,
cfg=rc_admin_redaction,
)
else:
self.admin_redaction_ratelimiter = None
Expand Down
133 changes: 87 additions & 46 deletions synapse/config/ratelimiting.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from typing import Any, Dict, Optional
from typing import Any, Dict, Optional, cast

import attr

Expand All @@ -21,16 +21,46 @@
from ._base import Config


@attr.s(slots=True, frozen=True, auto_attribs=True)
class RatelimitSettings:
def __init__(
self,
config: Dict[str, float],
defaults: Optional[Dict[str, float]] = None,
):
defaults = defaults or {"per_second": 0.17, "burst_count": 3.0}
key: str
per_second: float
burst_count: int

self.per_second = config.get("per_second", defaults["per_second"])
self.burst_count = int(config.get("burst_count", defaults["burst_count"]))

def parse_rate_limit(
Copy link
Member

Choose a reason for hiding this comment

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

Could also use a class-method, which is what we usually do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was my first instinct---though I shied away from it because it's mildly irritating to type classmethods correctly in an inheritance-aware way. (typing.Self is the one true way, but that's in Python 3.11 and I didn't want to fiddle with typing_extensions etc).

I'll quickly make this a class method and just -> "RatelimitSettings", which is good enough.

config: Dict[str, Any],
key: str,
defaults: Optional[Dict[str, float]] = None,
) -> RatelimitSettings:
"""Parse config[key] as a new-style rate limiter config.

The key may refer to a nested dictionary using a full stop (.) to separate
each nested key. For example, use the key "a.b.c" to parse the following:

a:
b:
c:
per_second: 10
burst_count: 200

If this lookup fails, we'll fallback to the defaults.
"""
defaults = defaults or {"per_second": 0.17, "burst_count": 3.0}

rl_config = config
for part in key.split("."):
rl_config = rl_config.get(part, {})

# By this point we should have hit the rate limiter parameters.
# We don't actually check this though!
rl_config = cast(Dict[str, float], rl_config)

return RatelimitSettings(
key=key,
per_second=rl_config.get("per_second", defaults["per_second"]),
burst_count=int(rl_config.get("burst_count", defaults["burst_count"])),
)


@attr.s(auto_attribs=True)
Expand All @@ -49,15 +79,14 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# Load the new-style messages config if it exists. Otherwise fall back
# to the old method.
if "rc_message" in config:
self.rc_message = RatelimitSettings(
config["rc_message"], defaults={"per_second": 0.2, "burst_count": 10.0}
self.rc_message = parse_rate_limit(
config, "rc_message", defaults={"per_second": 0.2, "burst_count": 10.0}
)
else:
self.rc_message = RatelimitSettings(
{
"per_second": config.get("rc_messages_per_second", 0.2),
"burst_count": config.get("rc_message_burst_count", 10.0),
}
key="rc_messages",
per_second=config.get("rc_messages_per_second", 0.2),
burst_count=config.get("rc_message_burst_count", 10.0),
)

# Load the new-style federation config, if it exists. Otherwise, fall
Expand All @@ -79,51 +108,57 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
}
)

self.rc_registration = RatelimitSettings(config.get("rc_registration", {}))
self.rc_registration = parse_rate_limit(config, "rc_registration", {})

self.rc_registration_token_validity = RatelimitSettings(
config.get("rc_registration_token_validity", {}),
self.rc_registration_token_validity = parse_rate_limit(
config,
"rc_registration_token_validity",
defaults={"per_second": 0.1, "burst_count": 5},
)

# It is reasonable to login with a bunch of devices at once (i.e. when
# setting up an account), but it is *not* valid to continually be
# logging into new devices.
rc_login_config = config.get("rc_login", {})
self.rc_login_address = RatelimitSettings(
rc_login_config.get("address", {}),
self.rc_login_address = parse_rate_limit(
config,
"rc_login.address",
defaults={"per_second": 0.003, "burst_count": 5},
)
self.rc_login_account = RatelimitSettings(
rc_login_config.get("account", {}),
self.rc_login_account = parse_rate_limit(
config,
"rc_login.account",
defaults={"per_second": 0.003, "burst_count": 5},
)
self.rc_login_failed_attempts = RatelimitSettings(
rc_login_config.get("failed_attempts", {})
self.rc_login_failed_attempts = parse_rate_limit(
config,
"rc_login.failed_attempts",
{},
)

self.federation_rr_transactions_per_room_per_second = config.get(
"federation_rr_transactions_per_room_per_second", 50
)

rc_admin_redaction = config.get("rc_admin_redaction")
self.rc_admin_redaction = None
if rc_admin_redaction:
self.rc_admin_redaction = RatelimitSettings(rc_admin_redaction)
if "rc_admin_redaction" in config:
self.rc_admin_redaction = parse_rate_limit(config, "rc_admin_redaction", {})

self.rc_joins_local = RatelimitSettings(
config.get("rc_joins", {}).get("local", {}),
self.rc_joins_local = parse_rate_limit(
config,
"rc_joins.local",
defaults={"per_second": 0.1, "burst_count": 10},
)
self.rc_joins_remote = RatelimitSettings(
config.get("rc_joins", {}).get("remote", {}),
self.rc_joins_remote = parse_rate_limit(
config,
"rc_joins.remote",
defaults={"per_second": 0.01, "burst_count": 10},
)

# Track the rate of joins to a given room. If there are too many, temporarily
# prevent local joins and remote joins via this server.
self.rc_joins_per_room = RatelimitSettings(
config.get("rc_joins_per_room", {}),
self.rc_joins_per_room = parse_rate_limit(
config,
"rc_joins_per_room",
defaults={"per_second": 1, "burst_count": 10},
)

Expand All @@ -132,31 +167,37 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# * For requests received over federation this is keyed by the origin.
#
# Note that this isn't exposed in the configuration as it is obscure.
self.rc_key_requests = RatelimitSettings(
config.get("rc_key_requests", {}),
self.rc_key_requests = parse_rate_limit(
config,
"rc_key_requests",
defaults={"per_second": 20, "burst_count": 100},
)

self.rc_3pid_validation = RatelimitSettings(
config.get("rc_3pid_validation") or {},
self.rc_3pid_validation = parse_rate_limit(
config,
"rc_3pid_validation",
defaults={"per_second": 0.003, "burst_count": 5},
)

self.rc_invites_per_room = RatelimitSettings(
config.get("rc_invites", {}).get("per_room", {}),
self.rc_invites_per_room = parse_rate_limit(
config,
"rc_invites.per_room",
defaults={"per_second": 0.3, "burst_count": 10},
)
self.rc_invites_per_user = RatelimitSettings(
config.get("rc_invites", {}).get("per_user", {}),
self.rc_invites_per_user = parse_rate_limit(
config,
"rc_invites.per_user",
defaults={"per_second": 0.003, "burst_count": 5},
)

self.rc_invites_per_issuer = RatelimitSettings(
config.get("rc_invites", {}).get("per_issuer", {}),
self.rc_invites_per_issuer = parse_rate_limit(
config,
"rc_invites.per_issuer",
defaults={"per_second": 0.3, "burst_count": 10},
)

self.rc_third_party_invite = RatelimitSettings(
config.get("rc_third_party_invite", {}),
self.rc_third_party_invite = parse_rate_limit(
config,
"rc_third_party_invite",
defaults={"per_second": 0.0025, "burst_count": 5},
)
8 changes: 3 additions & 5 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,19 +218,17 @@ def __init__(self, hs: "HomeServer"):
self._failed_uia_attempts_ratelimiter = Ratelimiter(
store=self.store,
clock=self.clock,
rate_hz=self.hs.config.ratelimiting.rc_login_failed_attempts.per_second,
burst_count=self.hs.config.ratelimiting.rc_login_failed_attempts.burst_count,
cfg=self.hs.config.ratelimiting.rc_login_failed_attempts,
)

# The number of seconds to keep a UI auth session active.
self._ui_auth_session_timeout = hs.config.auth.ui_auth_session_timeout

# Ratelimitier for failed /login attempts
# Ratelimiter for failed /login attempts
self._failed_login_attempts_ratelimiter = Ratelimiter(
store=self.store,
clock=hs.get_clock(),
rate_hz=self.hs.config.ratelimiting.rc_login_failed_attempts.per_second,
burst_count=self.hs.config.ratelimiting.rc_login_failed_attempts.burst_count,
cfg=self.hs.config.ratelimiting.rc_login_failed_attempts,
)

self._clock = self.hs.get_clock()
Expand Down
3 changes: 1 addition & 2 deletions synapse/handlers/devicemessage.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ def __init__(self, hs: "HomeServer"):
self._ratelimiter = Ratelimiter(
store=self.store,
clock=hs.get_clock(),
rate_hz=hs.config.ratelimiting.rc_key_requests.per_second,
burst_count=hs.config.ratelimiting.rc_key_requests.burst_count,
cfg=hs.config.ratelimiting.rc_key_requests,
)

async def on_direct_to_device_edu(self, origin: str, content: JsonDict) -> None:
Expand Down
6 changes: 2 additions & 4 deletions synapse/handlers/identity.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,12 @@ def __init__(self, hs: "HomeServer"):
self._3pid_validation_ratelimiter_ip = Ratelimiter(
store=self.store,
clock=hs.get_clock(),
rate_hz=hs.config.ratelimiting.rc_3pid_validation.per_second,
burst_count=hs.config.ratelimiting.rc_3pid_validation.burst_count,
cfg=hs.config.ratelimiting.rc_3pid_validation,
)
self._3pid_validation_ratelimiter_address = Ratelimiter(
store=self.store,
clock=hs.get_clock(),
rate_hz=hs.config.ratelimiting.rc_3pid_validation.per_second,
burst_count=hs.config.ratelimiting.rc_3pid_validation.burst_count,
cfg=hs.config.ratelimiting.rc_3pid_validation,
)

async def ratelimit_request_token_requests(
Expand Down
Loading