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

Make homeservers able to handle registration-with-email without depending on an Identity Server #5751

Closed
lampholder opened this issue Jul 24, 2019 · 14 comments

Comments

@lampholder
Copy link
Contributor

commented Jul 24, 2019

Ultimately we want to stop allowing third parties to use the Identity Servers at matrix.org and vector.im to send emails on their behalf for any reason.

@neilisfragile neilisfragile added this to Holding Pen in Homeserver Task Board via automation Jul 24, 2019
@neilisfragile neilisfragile moved this from Holding Pen to To Do in Homeserver Task Board Jul 24, 2019
@lampholder

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

I've been thinking a little more about whether the 'current behaviour' will represent a desirable state for any homeserver after this change.

The current behavior is that your homeserver will talk to whichever identity server your client tells it to (as long as it is on your whitelist), except when deactivating your account (when it will communicate with whichever identity servers it knows were used to bind specific threepids).

For homeserver admins who want to send registration and password-reset emails via an identity server, it's likely that they will have a specific identity server/servers in mind (since it would be atypical for anyone running an Identity Server to accept mail-send requests on behalf of just anyone).

Would it make sense then for the homeserver configuration options to be one of either:

  • Option A:
    • homeserver handles registration and password-reset emails itself
    • all identity activity is handled by the identity server specified by the client
  • Option B:
    • homeserver delegates registration and password-reset emails to an identity server specified in homeserver config
    • all identity activity is handled by the identity server specified by the client

This would have no impact on the account deactivation behaviour.

It would also be nice if there were a way to query the homeserver's whitelist of identity servers (I think not being able to do this will result in some glitchy edge cases that are hard to detect and handle).

@lampholder

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Having thought about this some more I think ^ is a great idea.

We should make it as clear as possible that the homeserver handles registration and password reset emails by itself, unless an account_threepid_delegate is specified in the homeserver.yaml. This account_threepid_delegate will probably be an Identity Server (though it could be any service that exposes registration and password-reset APIs).

Specifying an account_threepid_delegate will have no bearing on any other aspect of the system's interaction with identity servers. Lookups/invites will still be processed by whichever IS the client has asked the HS to communicate with on its behalf.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Is there a case where someone would want registration emails to be handled by the IS but the HS handles password resets?

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Lookups/invites will still be processed by whichever IS the client has asked the HS to communicate with on its behalf.

This is still filtered by the HS's whitelist, correct?

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Interestingly, /register already has a bind_email parameter. Have we taken this into account?

The homeserver should still handle the email sending based on a HS option though.


Edit: From vector-im/riot-web#10424, it seems we do.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Synapse currently has an email.trust_identity_server_for_password_resets option which is false by default.

So if we're going with the above solution, in that the IS is getting decoupled from sending registration emails/texts, then email.trust_identity_server_for_password_resets should probably be dropped from the config, and instead replaced with account_threepid_delegate above, which would default to None.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I'm currently trying to make account_threepid_delegate define the identity server address for use with account registration and password resets, however the client currently sends id_server as a field to the homeserver to specify which IS to bind with.

Should we just override this value?

Also

kind="email",
currently looks broken because it takes in a threepid object which would never have a threepid_creds key.

Edit: Turns out this function is called from somewhere else which does provide a threepid dict with a threepid_creds key.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

So we're going to go with having account_threepid_delegate be a string stating which one identity server you trust to handle password resets and registration emails.

If it's set, account/password resets will go through that set identity server. if it's not, then the HS will handle everything. id_server on /_matrix/client/r0/account/password/email/requestToken and /_matrix/client/r0/register/email/requestToken will be completely ignored.

We'd need to update the spec to reflect this.

This also means we can get rid of the server's identity server whitelist, since that was a weak attempt at preventing users from using malicious identity servers which could do nasty things with password resets.

The only place where id_server will still matter is /_matrix/client/r0/account/3pid/email/requestToken, where the user will be choosing which IS to bind with. We're not touching this endpoint at the moment.

Also /_matrix/client/r0/register/email|msisdn/requestToken and /_matrix/client/r0/account/3pid/email|msisdn/requestToken in the spec are functionally the same and the former should be removed.

@dbkr

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

The thing you're going to run into is that only the v1 endpoint of an IS will let you call requestToken without an account that's signed the terms, and the plan is to disable the v1 API, so account_threepid_delegate will essentially need to point to an old IS.

Also /_matrix/client/r0/register/email|msisdn/requestToken and /_matrix/client/r0/account/3pid/email|msisdn/requestToken in the spec are functionally the same and the former should be removed.

Actually now they're probably more relevant since a HS could send different emails depending on what the user was trying to do.

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Also /_matrix/client/r0/register/email|msisdn/requestToken and /_matrix/client/r0/account/3pid/email|msisdn/requestToken in the spec are functionally the same and the former should be removed.

Actually now they're probably more relevant since a HS could send different emails depending on what the user was trying to do.

Well one would be the IS sending an email, and the other being the HS sending an email, so you could put different things in the emails that way, no?

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

The thing you're going to run into is that only the v1 endpoint of an IS will let you call requestToken without an account that's signed the terms, and the plan is to disable the v1 API, so account_threepid_delegate will essentially need to point to an old IS.

Right... so we can't delegate sending registration emails via the IS anymore.

So this option would only be useful for password resets, but I don't really think anyone in their right mind would want to do homeserver password resets through an IS. So... should we just ditch homeserver's calling to IS' for registration/password resets altogether?

Edit: This means that homeservers would no longer be able to offer signing up with email if they didn't configure their SMTP options, which does make intuitive sense, but may catch some people off-guard upon upgrading.

@dbkr

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Well one would be the IS sending an email, and the other being the HS sending an email, so you could put different things in the emails that way, no?

Doesn't that mean they're not the same? But yes,register/email/requestToken will be the HS sending the email, account/3pid/email/requestToken will be the IS (for now at least).

So this option would only be useful for password resets, but I don't really think anyone in their right mind would want to do homeserver password resets through an IS. So... should we just ditch homeserver's calling to IS' for registration/password resets altogether?

I think we'd already decided to ditch it for registration (although I don't know if that was before we decided on the account_threepid_delegate thing). Either way, wfm but it's not really my decision. :)

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

After a meeting IRL, we're keeping account_threepid_delegate around but must be clear it is only for use with identity servers which support the v1 endpoints (as the v2 /register endpoint will require authentication). We might also rename this option to something else. What do you think @lampholder?

We also need to be very clear during the next Synapse update that homeserver admins are aware of this, as to avoid confusing them when we remove trust_identity_server_for_password_resets.

@anoadragon453 anoadragon453 moved this from To Do to In progress in Homeserver Task Board Aug 15, 2019
anoadragon453 added a commit that referenced this issue Aug 30, 2019
…il (#5835)

~~Fixes #5833 Moved out to ~~#5863
Part of fixing #5751

Decouples the activity of sending registration emails and binding them to an identity server.

This PR simply sends the registration email, but clicking it does not approve the user for registration. That will come in PR #2.

Some of this makes use of existing stuff for sending password reset emails from Synapse. Some work was done to make that stuff even more generic.

Upgrade notes:
* There is a new top-level config option, `account_threepid_delegate` which defines the address of an identity server that you would like to send registration/password reset emails on your behalf.

  The option `email.trust_identity_server_for_password_resets` has been replaced by this. If you set `email.trust_identity_server_for_password_resets` in your config to `true`, please remove it and configure `account_threepid_delegate` instead. The [sample config](https://github.com/matrix-org/synapse/blob/anoa/reg_email_sending_email/docs/sample_config.yaml) has information on how to configure it.

Note: This PR does not allow homeservers to send emails when simply adding an email to your account. That will come after this and will be blocked on a new MSC.

Part [2/2] will be successfully completing the registration step when `/submit_token` is hit with the correct details, and clearing out the `password_servlet` flag stuff, which is no longer needed. Will be a much smaller PR than this one.

~~Requires #5863 has been merged into the base branch.
~~Requires #5876 has been merged into the base branch.
anoadragon453 added a commit that referenced this issue Sep 5, 2019
…erification (#5940)

Fixes: #5751
Fixes: #5928

#5835 allowed Synapse to send registration emails to the user. Now we need to accept them and have it succeed the `m.login.email.identity` registration flow step.

`account_threepid_handler` will also be switched from a `str` in the config file to a dictionary which contains entries for `msisdn` and `email`, each with their own `str`. This will let people use an external server to handle `msisdn` registration and password reset requests, while using Synapse for email-based things.

And the `password_servlet` hack that was introduced in https://github.com/matrix-org/synapse/pull/5377/files#diff-b8464485d36f6f87caee3f4d82524213R189 to distinguish a registration call from a password reset call will be removed.
@neilisfragile neilisfragile moved this from In progress to Review in Homeserver Task Board Sep 12, 2019
@richvdh

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

this is now fixed, thanks to #6011 and all the other PRs which have formed the epic privacy project 🎉

@richvdh richvdh closed this Sep 26, 2019
Homeserver Task Board automation moved this from Review to Done Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.