Drop support for delegating email validation

[richvdh]

Delegating email validation to an IS is insecure (since it allows the owner of the IS to do a password reset on your HS), and has long been discouraged. It will now cause a config error at startup.

Part of #5881. Reviewable commit-by-commit.

[reivilibre]

LGTM — thanks :-).

[DMRobertson]

Complement failure is a known flake. Merging manually.

[3nprob]

@richvdh @DMRobertson Think it would be possible to either reconsider or delay the full deprecation of this and make enabling the setting a warning only for now? Installations currently depending on this for example those (still) on ma1sd or custom identity servers may need to figure out migration strategies.

has long been discouraged

where has this been communicated?

Consider cases where IS and HS are administered by the same entity - delegating account security to the IS is a feature, not a bug, and not a reduction in security, in some scenarios.

In any case, I know of multiple HSes that will need to either fork synapse or stay at 1.63.x if this is pursued like planned in 1.64. Some heads-up before breakages like this would be expected.

[theAeon]

Currently use ma1sd to restrict email verification by domain-would greatly appreciate at least a release cycle to figure out how the heck I'm migrating.

[richvdh]

Hrm, we've been talking about this for so long within the team that I assumed everyone knew about it, but now that I come to have a look it does indeed look like we've done a poor job of communicating it, so my apologies for landing it without fair warning.

The problem with this feature is that it relies on unspecified Identity Service APIs (POST /_matrix/identity/api/v1/validate/email/requestToken, GET /_matrix/identity/api/v1/3pid/getValidated3pid), both of which were removed from the spec two years ago, so it's not appropriate for mainline Synapse to continue using them.

I've not used ma1sd, but I'm afraid I don't entirely understand the usecase: why is it preferable to delegate email-sending to it rather than have synapse send email directly?

Currently use ma1sd to restrict email verification by domain

I think you may be able to achieve this result via allowed_local_3pids (possibly in conjunction with registrations_require_3pid).

Rather than continue to discuss on a closed PR, please could you open a new issue describing your usecase and explaining why it's not met by having synapse send verification emails directly?

[theAeon]

It's not necessarily a use-case thing so much as it is a short notice migration thing. I'm happy to drop it, I'd just appreciate an extra release cycle.

…

On Wed, Jul 27, 2022, 12:48 PM Richard van der Hoff < ***@***.***> wrote: Hrm, we've been talking about this for so long within the team that I assumed everyone knew about it, but now that I come to have a look it does indeed look like we've done a poor job of communicating it, so my apologies for landing it without fair warning. The problem with this feature is that it relies on unspecified Identity Service APIs (POST /_matrix/identity/api/v1/validate/email/requestToken, GET /_matrix/identity/api/v1/3pid/getValidated3pid), both of which were removed from the spec two years ago <matrix-org/matrix-spec-proposals#2713>, so it's not appropriate for mainline Synapse to continue using them. I've not used ma1sd, but I'm afraid I don't entirely understand the usecase: why is it preferable to delegate email-sending to it rather than have synapse send email directly? Currently use ma1sd to restrict email verification by domain I think you may be able to achieve this result via allowed_local_3pids <https://matrix-org.github.io/synapse/v1.63/usage/configuration/config_documentation.html#allowed_local_3pids> (possibly in conjunction with `registrations_require_3pid <https://matrix-org.github.io/synapse/v1.63/usage/configuration/config_documentation.html#registrations_require_3pid> . Rather than continue to discuss on a closed PR, please could you open a new issue describing your usecase and explaining why it's not met by having synapse send verification emails directly? — Reply to this email directly, view it on GitHub <#13192 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACFNSCMW7SQUSR4MFROJ6HTVWFR6RANCNFSM52XT336Q> . You are receiving this because you commented.Message ID: ***@***.***>

[richvdh]

I'm happy to drop it, I'd just appreciate an extra release cycle.

Well, you've got another week before this is released 😇. And even after that, I don't think there's any harm in remaining on 1.63 for a couple of weeks.

We'll try to do better on this in future, but I'm not sure there's much to be gained by backing it out now that it's landed.

[3nprob]

I'm really sorry for being grumpy here but ~1 month notice for a

• breaking account security change
• requiring infrastructure changes to resolve, which
• can result in users losing access to their accounts
• doesn't seem pressingly urgent (a recently discovered or actively exploited vulnerability would be another story), and
• the only publicly discoverable prior communication is this (maybe there's more but I generally try to keep updated and the rc release notes was the first time learning of this).

really needs some more planning and communication to roll out than <1month from proposal to merge and completed in the next release.

Like @theAeon , my bigger issue is not the change itself but the process around how a change of this nature and impact is made and rolled out.

What I'd really would have liked to see (and I'llopen a new issue for this, just getting this down here for now):

• Amend the change in this PR to still allow old behavior with existing configuration, but
  • Log a deprecation warning when doing so, indicating that it will be rmoved in a future point

Given way that MSC changes have been governed, made and communicated (which I up until now have not experienced as problematic, and is not my point of critique here), future changes like this really need to be handled differently to this.

To be clear how unfortunate this is, the current release 1.63.1 that has the even older unsupported configuration trust_identity_server_for_password_resets will still log an error saying:

The config option "trust_identity_server_for_password_resets has been replaced by "account_threepid_delegate".
Please consult the configuration manual at docs/usage/configuration/config_documentation.md for details and update your config file.

The linked section still instructs users that what is currently broken is the way to go:

Handle threepid (email/phone etc) registration and password resets through a set of trusted identity servers. Note that this allows the configured identity server to reset passwords for accounts!

, with an example of how to delegate e-mail sending.

So a hypothetical new synapse user who followed the develop version docs of 2022-07-11, and deployed the latest release on a fresh homeserver delegating e-mail accordingly, could get hit by this in their very first minor version upgrade.

For an example of how I think this should be handled, the previous deprecation of trust_identity_server_for_password_resets had similar (but smaller) impact. #5876 followed by #11333 shows how this can be a made in a non-disruptive way.

[3nprob]

It's worse than what I wrote above apparently. Latest official docs still encourages users delegating email validation using the feature being dropped here:

https://matrix-org.github.io/synapse/latest/usage/configuration/config_documentation.html#account_threepid_delegates

[theAeon]

With all due respect, this level of breaking change notice is something you'd expect in a beta release.

[theAeon]

Sure - the rant above is in context of the planned upcoming v1.64.0 including this as-is.

oh that was directed at op

[3nprob]

There is now a draft PR for changing this from an error to a warning (so keeping the functionality for now but deprecating it and reflecting that in docs) in #13406.