diff --git a/changelog.d/6335.bugfix b/changelog.d/6335.bugfix
new file mode 100644
index 0000000000..a95f6b9eec
--- /dev/null
+++ b/changelog.d/6335.bugfix
@@ -0,0 +1 @@
+Fix bug where `rc_login` ratelimiting would prematurely kick in.
diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py
index c9d0db4823..54a71c49d2 100644
--- a/synapse/handlers/auth.py
+++ b/synapse/handlers/auth.py
@@ -102,8 +102,9 @@ class AuthHandler(BaseHandler):
login_types.append(t)
self._supported_login_types = login_types
- self._account_ratelimiter = Ratelimiter()
- self._failed_attempts_ratelimiter = Ratelimiter()
+ # Ratelimiter for failed auth during UIA. Uses same ratelimit config
+ # as per `rc_login.failed_attempts`.
+ self._failed_uia_attempts_ratelimiter = Ratelimiter()
self._clock = self.hs.get_clock()
@@ -133,12 +134,38 @@ class AuthHandler(BaseHandler):
AuthError if the client has completed a login flow, and it gives
a different user to `requester`
+
+ LimitExceededError if the ratelimiter's failed request count for this
+ user is too high to proceed
+
"""
+ user_id = requester.user.to_string()
+
+ # Check if we should be ratelimited due to too many previous failed attempts
+ self._failed_uia_attempts_ratelimiter.ratelimit(
+ user_id,
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
+ burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
+ update=False,
+ )
+
# build a list of supported flows
flows = [[login_type] for login_type in self._supported_login_types]
- result, params, _ = yield self.check_auth(flows, request_body, clientip)
+ try:
+ result, params, _ = yield self.check_auth(flows, request_body, clientip)
+ except LoginError:
+ # Update the ratelimite to say we failed (`can_do_action` doesn't raise).
+ self._failed_uia_attempts_ratelimiter.can_do_action(
+ user_id,
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
+ burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
+ update=True,
+ )
+ raise
# find the completed login type
for login_type in self._supported_login_types:
@@ -501,11 +528,8 @@ class AuthHandler(BaseHandler):
multiple matches
Raises:
- LimitExceededError if the ratelimiter's login requests count for this
- user is too high too proceed.
UserDeactivatedError if a user is found but is deactivated.
"""
- self.ratelimit_login_per_account(user_id)
res = yield self._find_user_id_and_pwd_hash(user_id)
if res is not None:
return res[0]
@@ -572,8 +596,6 @@ class AuthHandler(BaseHandler):
StoreError if there was a problem accessing the database
SynapseError if there was a problem with the request
LoginError if there was an authentication problem.
- LimitExceededError if the ratelimiter's login requests count for this
- user is too high too proceed.
"""
if username.startswith("@"):
@@ -581,8 +603,6 @@ class AuthHandler(BaseHandler):
else:
qualified_user_id = UserID(username, self.hs.hostname).to_string()
- self.ratelimit_login_per_account(qualified_user_id)
-
login_type = login_submission.get("type")
known_login_type = False
@@ -650,15 +670,6 @@ class AuthHandler(BaseHandler):
if not known_login_type:
raise SynapseError(400, "Unknown login type %s" % login_type)
- # unknown username or invalid password.
- self._failed_attempts_ratelimiter.ratelimit(
- qualified_user_id.lower(),
- time_now_s=self._clock.time(),
- rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
- burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
- update=True,
- )
-
# We raise a 403 here, but note that if we're doing user-interactive
# login, it turns all LoginErrors into a 401 anyway.
raise LoginError(403, "Invalid password", errcode=Codes.FORBIDDEN)
@@ -710,10 +721,6 @@ class AuthHandler(BaseHandler):
Returns:
Deferred[unicode] the canonical_user_id, or Deferred[None] if
unknown user/bad password
-
- Raises:
- LimitExceededError if the ratelimiter's login requests count for this
- user is too high too proceed.
"""
lookupres = yield self._find_user_id_and_pwd_hash(user_id)
if not lookupres:
@@ -742,7 +749,7 @@ class AuthHandler(BaseHandler):
auth_api.validate_macaroon(macaroon, "login", user_id)
except Exception:
raise AuthError(403, "Invalid token", errcode=Codes.FORBIDDEN)
- self.ratelimit_login_per_account(user_id)
+
yield self.auth.check_auth_blocking(user_id)
return user_id
@@ -912,35 +919,6 @@ class AuthHandler(BaseHandler):
else:
return defer.succeed(False)
- def ratelimit_login_per_account(self, user_id):
- """Checks whether the process must be stopped because of ratelimiting.
-
- Checks against two ratelimiters: the generic one for login attempts per
- account and the one specific to failed attempts.
-
- Args:
- user_id (unicode): complete @user:id
-
- Raises:
- LimitExceededError if one of the ratelimiters' login requests count
- for this user is too high too proceed.
- """
- self._failed_attempts_ratelimiter.ratelimit(
- user_id.lower(),
- time_now_s=self._clock.time(),
- rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
- burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
- update=False,
- )
-
- self._account_ratelimiter.ratelimit(
- user_id.lower(),
- time_now_s=self._clock.time(),
- rate_hz=self.hs.config.rc_login_account.per_second,
- burst_count=self.hs.config.rc_login_account.burst_count,
- update=True,
- )
-
@attr.s
class MacaroonGenerator(object):
diff --git a/synapse/rest/client/v1/login.py b/synapse/rest/client/v1/login.py
index 24a0ce74f2..19eb15003d 100644
--- a/synapse/rest/client/v1/login.py
+++ b/synapse/rest/client/v1/login.py
@@ -92,8 +92,11 @@ class LoginRestServlet(RestServlet):
self.auth_handler = self.hs.get_auth_handler()
self.registration_handler = hs.get_registration_handler()
self.handlers = hs.get_handlers()
+ self._clock = hs.get_clock()
self._well_known_builder = WellKnownBuilder(hs)
self._address_ratelimiter = Ratelimiter()
+ self._account_ratelimiter = Ratelimiter()
+ self._failed_attempts_ratelimiter = Ratelimiter()
def on_GET(self, request):
flows = []
@@ -202,6 +205,16 @@ class LoginRestServlet(RestServlet):
# (See add_threepid in synapse/handlers/auth.py)
address = address.lower()
+ # We also apply account rate limiting using the 3PID as a key, as
+ # otherwise using 3PID bypasses the ratelimiting based on user ID.
+ self._failed_attempts_ratelimiter.ratelimit(
+ (medium, address),
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
+ burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
+ update=False,
+ )
+
# Check for login providers that support 3pid login types
(
canonical_user_id,
@@ -211,7 +224,8 @@ class LoginRestServlet(RestServlet):
)
if canonical_user_id:
# Authentication through password provider and 3pid succeeded
- result = yield self._register_device_with_callback(
+
+ result = yield self._complete_login(
canonical_user_id, login_submission, callback_3pid
)
return result
@@ -225,6 +239,21 @@ class LoginRestServlet(RestServlet):
logger.warning(
"unknown 3pid identifier medium %s, address %r", medium, address
)
+ # We mark that we've failed to log in here, as
+ # `check_password_provider_3pid` might have returned `None` due
+ # to an incorrect password, rather than the account not
+ # existing.
+ #
+ # If it returned None but the 3PID was bound then we won't hit
+ # this code path, which is fine as then the per-user ratelimit
+ # will kick in below.
+ self._failed_attempts_ratelimiter.can_do_action(
+ (medium, address),
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
+ burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
+ update=True,
+ )
raise LoginError(403, "", errcode=Codes.FORBIDDEN)
identifier = {"type": "m.id.user", "user": user_id}
@@ -236,29 +265,84 @@ class LoginRestServlet(RestServlet):
if "user" not in identifier:
raise SynapseError(400, "User identifier is missing 'user' key")
- canonical_user_id, callback = yield self.auth_handler.validate_login(
- identifier["user"], login_submission
+ if identifier["user"].startswith("@"):
+ qualified_user_id = identifier["user"]
+ else:
+ qualified_user_id = UserID(identifier["user"], self.hs.hostname).to_string()
+
+ # Check if we've hit the failed ratelimit (but don't update it)
+ self._failed_attempts_ratelimiter.ratelimit(
+ qualified_user_id.lower(),
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
+ burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
+ update=False,
)
- result = yield self._register_device_with_callback(
+ try:
+ canonical_user_id, callback = yield self.auth_handler.validate_login(
+ identifier["user"], login_submission
+ )
+ except LoginError:
+ # The user has failed to log in, so we need to update the rate
+ # limiter. Using `can_do_action` avoids us raising a ratelimit
+ # exception and masking the LoginError. The actual ratelimiting
+ # should have happened above.
+ self._failed_attempts_ratelimiter.can_do_action(
+ qualified_user_id.lower(),
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_failed_attempts.per_second,
+ burst_count=self.hs.config.rc_login_failed_attempts.burst_count,
+ update=True,
+ )
+ raise
+
+ result = yield self._complete_login(
canonical_user_id, login_submission, callback
)
return result
@defer.inlineCallbacks
- def _register_device_with_callback(self, user_id, login_submission, callback=None):
- """ Registers a device with a given user_id. Optionally run a callback
- function after registration has completed.
+ def _complete_login(
+ self, user_id, login_submission, callback=None, create_non_existant_users=False
+ ):
+ """Called when we've successfully authed the user and now need to
+ actually login them in (e.g. create devices). This gets called on
+ all succesful logins.
+
+ Applies the ratelimiting for succesful login attempts against an
+ account.
Args:
user_id (str): ID of the user to register.
login_submission (dict): Dictionary of login information.
callback (func|None): Callback function to run after registration.
+ create_non_existant_users (bool): Whether to create the user if
+ they don't exist. Defaults to False.
Returns:
result (Dict[str,str]): Dictionary of account information after
successful registration.
"""
+
+ # Before we actually log them in we check if they've already logged in
+ # too often. This happens here rather than before as we don't
+ # necessarily know the user before now.
+ self._account_ratelimiter.ratelimit(
+ user_id.lower(),
+ time_now_s=self._clock.time(),
+ rate_hz=self.hs.config.rc_login_account.per_second,
+ burst_count=self.hs.config.rc_login_account.burst_count,
+ update=True,
+ )
+
+ if create_non_existant_users:
+ user_id = yield self.auth_handler.check_user_exists(user_id)
+ if not user_id:
+ user_id = yield self.registration_handler.register_user(
+ localpart=UserID.from_string(user_id).localpart
+ )
+
device_id = login_submission.get("device_id")
initial_display_name = login_submission.get("initial_device_display_name")
device_id, access_token = yield self.registration_handler.register_device(
@@ -285,7 +369,7 @@ class LoginRestServlet(RestServlet):
token
)
- result = yield self._register_device_with_callback(user_id, login_submission)
+ result = yield self._complete_login(user_id, login_submission)
return result
@defer.inlineCallbacks
@@ -313,15 +397,8 @@ class LoginRestServlet(RestServlet):
raise LoginError(401, "Invalid JWT", errcode=Codes.UNAUTHORIZED)
user_id = UserID(user, self.hs.hostname).to_string()
-
- registered_user_id = yield self.auth_handler.check_user_exists(user_id)
- if not registered_user_id:
- registered_user_id = yield self.registration_handler.register_user(
- localpart=user
- )
-
- result = yield self._register_device_with_callback(
- registered_user_id, login_submission
+ result = yield self._complete_login(
+ user_id, login_submission, create_non_existant_users=True
)
return result
|
