Skip to content
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

Only do `rc_login` ratelimiting on succesful login. #6335

Merged
merged 5 commits into from Nov 20, 2019

Conversation

@erikjohnston
Copy link
Member

erikjohnston commented Nov 5, 2019

We were doing this in a number of places which meant that some login code paths incremented the counter multiple times.

It was also applying ratelimiting to UIA endpoints, which was probably not intentional.

In particular, some custom auth modules were calling check_user_exists, which incremented the counters, meaning that people would fail to login sometimes.

A side effect of this is that we don't ratelimit the SSO path (as that was obscurely relying on check_user_exists being ratelimited??) until after we've successfully authed with the SSO provider, and rely on the remote SSO provider to do its own ratelimiting. We do still ratelimit when the client then logs in with the provider token via /login.

This also applies a separate rate limit to failed UIA auth attempts, to protect someone attempting to extract a password from a logged in session.

Note: no failed rate limiting is applied by token style logins as they're not associated with any user ID.

@erikjohnston erikjohnston added this to In progress in Homeserver Task Board via automation Nov 5, 2019
@erikjohnston erikjohnston force-pushed the erikj/rc_login_cleanups branch 2 times, most recently from 3e49197 to 17f6269 Nov 6, 2019
erikjohnston added 3 commits Nov 5, 2019
We were doing this in a number of places which meant that some login
code paths incremented the counter multiple times.

It was also applying ratelimiting to UIA endpoints, which was probably
not intentional.

In particular, some custom auth modules were calling
`check_user_exists`, which incremented the counters, meaning that people
would fail to login sometimes.
@erikjohnston erikjohnston force-pushed the erikj/rc_login_cleanups branch from 17f6269 to 4fc53bf Nov 6, 2019
@erikjohnston erikjohnston marked this pull request as ready for review Nov 6, 2019
@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Nov 6, 2019
@erikjohnston erikjohnston moved this from In progress to Review in Homeserver Task Board Nov 6, 2019
Copy link
Member

babolivier left a comment

lgtm otherwise

synapse/rest/client/v1/login.py Outdated Show resolved Hide resolved
Copy link
Member

anoadragon453 left a comment

tiny nits

synapse/handlers/auth.py Outdated Show resolved Hide resolved
synapse/handlers/auth.py Outdated Show resolved Hide resolved
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-Authored-By: Brendan Abolivier <babolivier@matrix.org>
@babolivier babolivier merged commit 83446a1 into develop Nov 20, 2019
22 checks passed
22 checks passed
buildkite/synapse Build #5570 passed (21 minutes, 51 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 35 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 44 seconds)
Details
buildkite/synapse/isort Passed (17 seconds)
Details
buildkite/synapse/mypy Passed (26 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (42 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pipeline Passed (3 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (18 minutes, 14 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (7 minutes, 39 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (9 minutes, 27 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (6 minutes, 39 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (17 minutes, 40 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (18 minutes, 31 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (7 minutes, 10 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-5-slash-postgres-9-dot-5 Passed (1 minute, 47 seconds)
Details
buildkite/synapse/synapse-port-db-slash-python-3-dot-7-slash-postgres-11 Passed (1 minute, 24 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (14 minutes, 26 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (14 minutes, 35 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (14 minutes, 51 seconds)
Details
buildkite/synapse/sytest-python-3-dot-7-slash-postgres-11-slash-monolith Passed (12 minutes, 38 seconds)
Details
buildkite/synapse/sytest-python-3-dot-7-slash-postgres-11-slash-workers Passed (12 minutes, 22 seconds)
Details
Homeserver Task Board automation moved this from Review to Done Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.