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

Allow for identifier dicts in User Interactive Auth dicts #7438

Closed
wants to merge 17 commits into from
Closed

Allow for identifier dicts in User Interactive Auth dicts #7438

wants to merge 17 commits into from

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented May 6, 2020 •
edited
Loading

A stab at fixing #5665

Allows identifier dicts to be used in UIA flows and relaxes the requirement of a user parameter, which has since been deprecated. Relevant spec: https://matrix.org/docs/spec/client_server/r0.6.1#identifier-types

Warning: This deals with UIA and login and may break both - careful review required.

Requires: #7687

Should be reviewable commit-by-commit.

Note: This PR reworks the end of _check_auth_dict, which was essentially the place where m.login.password UI Auth steps were handled. In a future PR, it may be cleaner to move that code into an instance of UserInteractiveAuthChecker in synapse/handlers/ui_auth/checkers.py.

@anoadragon453 anoadragon453 changed the title Allow for m.id.user identifier dicts in UI auth Allow for UIA identifier dicts in UI auth May 28, 2020
@anoadragon453 anoadragon453 changed the title Allow for UIA identifier dicts in UI auth Allow for identifier dicts in User Interactive Auth dicts May 28, 2020
clokep
clokep reviewed May 28, 2020
View reviewed changes
synapse/handlers/auth.py Outdated Show resolved Hide resolved
@babolivier babolivier linked an issue Jun 3, 2020 that may be closed by this pull request
synapse requires unspecified user for m.login.password UI auth #5665
Closed
@anoadragon453 anoadragon453 force-pushed the anoa/user_param_ui_auth branch 4 times, most recently from cc02e3c to cc2576f Compare June 11, 2020 17:32
@anoadragon453 anoadragon453 marked this pull request as ready for review June 12, 2020 13:45
@anoadragon453 anoadragon453 requested a review from a team June 12, 2020 15:30
@clokep clokep self-assigned this Jun 12, 2020
clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/handlers/auth.py Outdated Show resolved Hide resolved
clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/handlers/auth.py Outdated Show resolved Hide resolved
clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/handlers/auth.py Outdated Show resolved Hide resolved
clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/handlers/auth.py Outdated Show resolved Hide resolved
clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/handlers/auth.py Outdated
Comment on lines 66 to 67
SynapseError: if the dict contains a "medium" parameter that is anything other than
"email"
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems wrong? It raises if the format is invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that was a check I added, as that's technically required by the spec for login although it's a bit of a weird requirement if identifier is allowed to be m.id.phone.

Will remove the comment though.

clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/rest/client/v1/login.py Outdated
)
if medium and address:
self._failed_attempts_ratelimiter.ratelimit(
(medium.lower(), address.lower()), update=False
Copy link
Member

@clokep clokep Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The lower-casing of medium here seems to be a change in behavior? Or maybe I can't follow the logic...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Yes it is, and apologies for not calling it out during the commit.

This would've allowed someone to bypass the ratelimiter by specifying eMail or some other combination of capitalisation.

However, now that I'm testing it, this trickery will get caught later on in the flow anyways, and return an immediate 403 before trying anything CPU-heavy like password hashing.

So it's not too sensitive - but I am going to pull it out of here into another PR as I think it's worthwhile, but needs more careful thought.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do! 👍

clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/rest/client/v1/login.py
Comment on lines -200 to -207
# 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.
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems more helpful than the new version.

Copy link
Member Author

@anoadragon453 anoadragon453 Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I think things have slightly changed now though (which may or may not be an issue as well).

> # If it returned None but the 3PID was bound then we won't hit
> # this code path,

This is no longer true. If None is returned by username_from_identifier and a medium and address was used, then we will check the ratelimiter. This sounds sensible, but it is a change from before, where it seems like the code would rate limit on the User ID instead.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm...I think I was thinking the part about the incorrect password vs. the account not existing seems useful to keep (if that's still true).

clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/rest/client/v1/login.py Outdated
Comment on lines 186 to 189
result = await self._complete_login(
canonical_user_id, login_submission, callback_3pid
)
return result
Copy link
Member

@clokep clokep Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This code path seems to now go through an additional self.auth_handler.validate_login, I don't know if that's OK or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, yes good spot.

I believe this originally did not as we have already validated the password via the password auth provider, so there's no need to validate_login at that point (which will itself ask password auth providers with a user_id/password combo).

Considering this would change Synapse's behaviour related to third-party code we should probably try to get the behaviour to match.

clokep
clokep reviewed Jun 12, 2020
View reviewed changes
synapse/rest/client/v1/login.py
Comment on lines -220 to -223
if identifier["user"].startswith("@"):
qualified_user_id = identifier["user"]
else:
qualified_user_id = UserID(identifier["user"], self.hs.hostname).to_string()
Copy link
Member

Choose a reason for hiding this comment

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

This logic seems to have disappeared.

Copy link
Member Author

Choose a reason for hiding this comment

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

It moved to AuthHandler.validate_login.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, actually this was always in validate_login, I think? So this was just duplicated?

clokep
clokep reviewed Jun 12, 2020
View reviewed changes
tests/rest/client/v2_alpha/test_auth.py
Comment on lines -41 to -43
class DummyPasswordChecker(UserInteractiveAuthChecker):
def check_auth(self, authdict, clientip):
return succeed(authdict["identifier"]["user"])
Copy link
Member

Choose a reason for hiding this comment

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

Is this just not used?

Copy link
Member Author

@anoadragon453 anoadragon453 Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It was used by 1807115#diff-5fc780094ae3ec668024efd505af6a3dL170

This checker was a patch to allow for identifiers being passed to AuthHandler.check_auth, but this PR removes the need for it as it providers that functionality.

clokep
clokep reviewed Jun 12, 2020
View reviewed changes
tests/rest/client/v1/test_login.py
# https://github.com/matrix-org/synapse/issues/5665
# "identifier": {"type": "m.id.user", "user": user_id},
"user": user_id,
"identifier": {"type": "m.id.user", "user": user_id},
Copy link
Member

Choose a reason for hiding this comment

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

We likely still want a test for the legacy login type.

Copy link
Member Author

@anoadragon453 anoadragon453 Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, so the login unittest function still uses the old method:

synapse/tests/unittest.py

Lines 512 to 518 in c2e1a21

def login(self, username, password, device_id=None):
"""
Log in a user, and get an access token. Requires the Login API be
registered.
"""
body = {"type": "m.login.password", "user": username, "password": password}
So it's definitely still tested.

But it may be a better idea to update this to the new format and just have a specific test for the old behaviour?

clokep
clokep suggested changes Jun 12, 2020
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

This doesn't look crazy, but it does modify a lot of code.

I do wonder if the conversion of the legacy login to identifier dicts should be done at the REST layer and the handlers become a bit purer. I don't know how many callers there are though (and this isn't a requirement, just a thought).

@anoadragon453 anoadragon453 mentioned this pull request Jun 15, 2020
Merged
@anoadragon453
Copy link
Member Author

I've noticed that the User ID ratelimiter now ratelimits on username, rather than qualified_user_id, which means that technically one can beat the ratelimiter by switching from localpart to full user_id. Will need to figure out a way to restore the original behaviour.

@anoadragon453
Merge branch 'develop' of github.com:matrix-org/synapse into anoa/use…
7affcd0 
…r_param_ui_auth

* 'develop' of github.com:matrix-org/synapse: (369 commits)
  Add functions to `MultiWriterIdGen` used by events stream (#8164)
  Do not allow send_nonmember_event to be called with shadow-banned users. (#8158)
  Changelog fixes
  1.19.1rc1
  Make StreamIdGen `get_next` and `get_next_mult` async  (#8161)
  Wording fixes to 'name' user admin api filter (#8163)
  Fix missing double-backtick in RST document
  Search in columns 'name' and 'displayname' in the admin users endpoint (#7377)
  Add type hints for state. (#8140)
  Stop shadow-banned users from sending non-member events. (#8142)
  Allow capping a room's retention policy (#8104)
  Add healthcheck for default localhost 8008 port on /health endpoint. (#8147)
  Fix flaky shadow-ban tests. (#8152)
  Fix join ratelimiter breaking profile updates and idempotency (#8153)
  Do not apply ratelimiting on joins to appservices (#8139)
  Don't fail /submit_token requests on incorrect session ID if request_token_inhibit_3pid_errors is turned on (#7991)
  Do not apply ratelimiting on joins to appservices (#8139)
  Micro-optimisations to get_auth_chain_ids (#8132)
  Allow denying or shadow banning registrations via the spam checker (#8034)
  Stop shadow-banned users from sending invites. (#8095)
  ...
@anoadragon453 anoadragon453 mentioned this pull request Aug 26, 2020
Merged
anoadragon453 added a commit that referenced this pull request Aug 28, 2020
@anoadragon453
Move and refactor LoginRestServlet helper methods (#8182)
2c2e649 
This is split out from #7438, which had gotten rather large.

`LoginRestServlet` has a couple helper methods, `login_submission_legacy_convert` and `login_id_thirdparty_from_phone`. They're primarily used for converting legacy user login submissions to "identifier" dicts ([see spec](https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-login)). Identifying information such as usernames or 3PID information used to be top-level in the login body. They're now supposed to be put inside an [identifier](https://matrix.org/docs/spec/client_server/r0.6.1#identifier-types) parameter instead.

#7438's purpose is to allow using the new identifier parameter during User-Interactive Authentication, which is currently handled in AuthHandler. That's why I've moved these helper methods there. I also moved the refactoring of these method from #7438 as they're relevant.
@anoadragon453
Copy link
Member Author

Closing this for now as the code is just bitrotting. Though I do plan to break this up further and apply it against $current_mainline once I find some time to do so.

@anoadragon453 anoadragon453 mentioned this pull request Dec 1, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep requested changes

Assignees

@clokep clokep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

synapse requires unspecified user for m.login.password UI auth
2 participants
@anoadragon453 @clokep