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

Drop support for delegating email validation, round 2 #13596

Merged
merged 8 commits into from
Aug 23, 2022

Conversation

DMRobertson
Copy link
Contributor

Revert "Revert "Drop support for delegating email validation (#13192)" (#13406)"

This reverts commit 98fb610.

History:

1.66 is being cut, so I am reverting the revert.

@3nprob
Copy link
Contributor

3nprob commented Aug 23, 2022

We'd really like some more time. Months not days preferrably.

This is still just 21 days from that the intention to remove this (quite security-critical, I think you'd agree) was announced in the release of v1.64.0. It was discussed at length on the expectations of timelines on deprecations in #13192 at length so I won't go into lengths on that here again.

Also, we are still very confused on the original claim of

Delegating email validation to an IS is insecure (since it allows the owner of the IS to do a password reset on your HS)

It is arguably more secure and best practice for enterprise deployments to separate ID and secrets management from applications such as synapse.

And what's the rush? I don't believe we're the only organization discussing internally whether to start maintaining a fork of synapse or reconsider Matrix all-together, but that's very unfortunately what it comes down to if we're to stay compliant with established security policies if this PR gets merged and released as intended.

I have a strong feeling that either many users have just overlooked this and will have a harsh surprise when they upgrade, or corporate deployments of synapse are way less common than I have believed.

In any case, deprecating this will prevent us from staying on mainline synapse. And that's really it; convincing me won't convince the compliance team and I can't see how we can be unique in this.

@DMRobertson DMRobertson marked this pull request as ready for review August 23, 2022 10:26
@DMRobertson DMRobertson requested a review from a team as a code owner August 23, 2022 10:26
@richvdh
Copy link
Member

richvdh commented Aug 23, 2022

We'd really like some more time. Months not days preferrably.

Well first, thank you for the feedback.

However, in short, I'm afraid we're not prepared to keep this patch on the shelf indefinitely: apart from the security aspects, this is that we need to drive down in order that we can continue to maintain both Synapse and Sydent effectively.

By the time this change is released, it will be four complete weeks since the removal announcement was made, alongside the Synapse 1.64.0 release. We don't consider the changes needed as a Synapse admin to be particularly arduous (in short: configure Synapse with an email server), so I think this is a reasonable notice period. If you're unable to make that change in this timeframe, I think it's also reasonable for you to remain on Synapse 1.65.0 until such time as you are prepared to upgrade.

Also, we are still very confused on the original claim of

Delegating email validation to an IS is insecure (since it allows the owner of the IS to do a password reset on your HS)

It is arguably more secure and best practice for enterprise deployments to separate ID and secrets management from applications such as synapse.

I think there's some confusion about the feature being removed here, so let me be explicit. In short: Synapse sometimes has to confirm that a user owns an email address. The way it does this is by sending an email address which asks the user to click on a link. Currently, there are two options here:

  • Have Synapse send the email itself, and handle the link in the email itself.
  • Have Synapse ask Sydent to send the email, and have Sydent handle the link in the email. Synapse must then poll Sydent to see if the user has clicked the link.

It is the second of those two options that is being removed. Sydent does not perform any check that a given email corresponds to a given user - indeed it cannot, since it does not have that information, and is not told which user is performing this validation. Sydent is merely a dumb proxy acting on Synapse's request. So I certainly don't think this adds any security.

On the other hand, if Sydent is controlled by a different organisation to Synapse (a common deployment scenario), then the Sydent administrator is able to claim that a user has validated their email when they have not, and thus take over an account on Synapse. Even where the two organisations are the same, this simply increases the attack surface for a malicious actor.

So: this feature adds a big security hole in the common case, and is at best neutral in the unusual case. That's why it was removed from the Matrix spec over a year ago, and why it is well over time it was removed from Synapse.

I have a strong feeling that either many users have just overlooked this and will have a harsh surprise when they upgrade

You may be right. However, we've now (thanks to you, indeed), announced our plans for this clearly, and not received any pushback on it at all. Which either means everyone is happy with it, or they aren't reading our changelogs and blog posts. Either way, there's a limit to what else we can do to get the message out; the next step here has to be to make the change.

I don't believe we're the only organization discussing internally whether to start maintaining a fork of synapse or reconsider Matrix all-together, but that's very unfortunately what it comes down to if we're to stay compliant with established security policies if this PR gets merged and released as intended.

Fundamentally, I'm really struggling to understand this. I think you're saying that, on the one hand, you're not prepared to configure Synapse to talk to an smtp server; yet on the other, you're keen to stay on the very bleeding edge of Synapse. Possibly I lack context, but can you explain why upgrading Synapse every fortnight is easy but adding an smtp server is hard?

It is always disappointing to read that a user is considering forking Synapse or even leaving Matrix, so we don't take these decisions lightly, and do our best to make sure that existing usecases are maintained. But we have to balance that with our needs to maintain the software and protocol for the good of the entire ecosystem, and misfeatures like this one ultimately have to be removed to ensure we can do that.

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm

@DMRobertson DMRobertson enabled auto-merge (squash) August 23, 2022 11:23
@DMRobertson DMRobertson merged commit 956e015 into release-v1.66 Aug 23, 2022
@DMRobertson DMRobertson deleted the dmr/remove-email-validation-delegation branch August 23, 2022 11:40
@3nprob
Copy link
Contributor

3nprob commented Aug 23, 2022

There seems to be an assumption that deployments that use the identity server API use Sydent.

The Synapse API is public. There may be in-house deployment-specific alternative identity servers.

@richvdh It seems that we are talking around each other here. Everything you wrote has been very clear from the start.

Fundamentally, I'm really struggling to understand this. I think you're saying that, on the one hand, you're not prepared to configure Synapse to talk to an smtp server; yet on the other, you're keen to stay on the very bleeding edge of Synapse. Possibly I lack context, but can you explain why upgrading Synapse every fortnight is easy but adding an smtp server is hard?

It's not that "I am not prepared to". Policies (put in place for good reason) prevent us to.

We're not necessarily staying on the bleeding edge. But we're certainly not going to be staying indefinitely on 1.65.0 and manually backporting any security fixes either.

Most of what you wrote probably makes perfect sense for a deployment looking like the one at matrix.org and for small-to-medium-size self-hosted servers (I am not implying these two are similar here). Probably also for larger deployments where the security model happens to fit. But seems to be both a severe blind-spot here with regards to other architectures and the kinds of requirements that arise in other organizations, even when these needs are raised.

While Synapse implements the Matrix specification, and [must/should] follow the contract for any specified flow and methods, there is nothing (correct me if I am wrong here) saying that Synapse should not implement any functionality not specified in the Matrix API. Specifically, that functionality not required by the API must or should be removed in Synapse as well. Synapse implements Matrix but may also afford additional functionality - no?


Taking a step back: Consider that e-mail is just one of many 3pids. We understand that for the time being, e-mail alongside phone number are the most commonly used ones. Down the line we expect to see others taking prominence. Having specific hard requirements on the ways around verification of email specifically makes no sense in synapse from a bigger perspective. This has very high risk of becoming technical debt blocking the adoption of other (more secure!) methods, due to email working differently. Seeing this in Element is perfectly fine and expected but please don't neglect the long-term implications here.

@3nprob
Copy link
Contributor

3nprob commented Aug 23, 2022

What comes next is a bit of conjecture - so bear with me and understand I am doing my best to approach this in good faith despite the tone. Also feel free to hide this comment if you consider it OT here.

I am interpreting some dissonance between what the expectations that the Matrix Foundation have been setting in communication on what Synapse is supposed to be, vs the vibe I am receiving from the comments here and in #13192 (users are expected to take the software as a prepared product and use it according to instructions by maintainers; writing custom software to integrate with public APIs is done on your own risk and may be deprecated with a month's notice at any point; even documented functionality is there to facilitate endorsed flows).

If this kind of change was done in Element or any of the other repos under vector-im (imagine a hypothetical alternative homeserver there), we'd just say "oh well" and take it for what it is. The expectations have been set very differently but from our perspective it seems that the preferences of New Vector for the deployments they are supporting is at priority over needs of others.

Listen, I'd love to both see Matrix succeed and continue supporting its adoption across both businesses and communities. We're on the same side here, I think. But pushing this shows clearly that the short-term needs of the maintainers (which I could still understand has higher prio than the short-term needs of other users) comes above both the needs of other users (in this case enterprise deployments) and the stated long-term vision of the project.

@3nprob
Copy link
Contributor

3nprob commented Aug 23, 2022

Like, imagine if your SMTP server just announced "we're dropping support for certain specifics required for Active Directory. Users are advised to switch to a compatible LDAP server to continue receiving security updates. Us and our partners are all on Linux and BSD anyway. This will be fully deprecated one month from now". What would you do as an AD admin?

It just wouldn't fly. Please set ambitions higher because otherwise synapse won't even be considered in many places where ejabberd has gone. You're setting a presedence and this will inform decision-makers when assessing reliability, risks, and long-term viability.

@richvdh
Copy link
Member

richvdh commented Aug 23, 2022

@3nprob I've already given you a long and detailed explanation of why we need to make this change, and why we consider it necessary, so I'll keep this short and simple.

Fundamentally, I don't understand your usecase. I don't understand why it is so important to you that Synapse delegate its email validation to an external server. You've just said "because policies", but you've not given me enough information to understand why those policies are in place.

Every optional feature in Synapse requires code to drive it, and requires testing to confirm it doesn't conflict with any other option. It is clearly unsustainable that Synapse implement every conceivable option. This is nothing to do with the Matrix Foundation: the maintainers of any piece of software have to chose what to prioritise.

If you can explain what you're trying to achieve here, maybe we can find a better way of doing it. But we're certain this feature, as it currently exists, has no place in mainline Synapse.

@richvdh
Copy link
Member

richvdh commented Aug 23, 2022

There was some further discussion in #synapse-dev, so in the interests of posterity, and for other readers, I'll record some details here:

As we understand it, @3nprob's organisation was relying on Synapse's use of the the Identity Server /_matrix/identity/api/v1/validate/email/requestToken API to perform some additional checks when a user requested a password reset. We remain unclear what checks were being done, or how they could ever have been effective.

To circle back to something that was said earlier:

It is arguably more secure and best practice for enterprise deployments to separate ID and secrets management from applications such as synapse.

This is absolutely true; however delegating email-address validation to an external server doesn't achieve that. For now, the best approach for this is using Synapse's single-sign-on integrations. (In future, Synapse's OIDC support will be significantly improved).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants