dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

Backporting fixes to SmtpClient #108382

Open avin3sh opened 1 month ago

avin3sh commented 1 month ago

As of today, Negotiate/Kerberos with SmtpClient is broken (https://github.com/dotnet/runtime/pull/105938). This was fixed in https://github.com/dotnet/runtime/pull/105939 which I can confirm resolves the problem. However, there is no remedy that is available for .NET 8 / LTS for this regression. Third party mail libraries do not yet support Negotiate with SMTP.

Quoting @kedia990 from https://github.com/dotnet/runtime/pull/105939: This is a clear regression vs. .NET Framework. Sending authenticated mail using Negotiate works in Framework, and it did not in Core until this patched was implemented. This was verified against both stock Microsoft Exchange, as well as Postfix, using .NET 9 RC 1. So it's not SMTP server dependent either, at least in any way that we can tell.

As to why it's a problem for us and nobody else - I can only speculate. Maybe authenticated mail (at least using Negotiate) isn't being used by a lot of folks out there. Maybe enterprises that have that kind of code are still on Framework. Or maybe they're using MailKit with .NET Core (based on Microsoft's recommendation) and therefore haven't run into SmtpClient-specific limitations. (To be clear, we would have followed that recommendation and used MailKit too, except it doesn't support Kerberos yet for authenticated email.)

That said, it's not clear that that should matter - this PR fixes the regression from Framework, and more importantly, a regression that forces us into an insecure posture. .NET 8 is the LTS release which the overwhelming majority of our applications are on and we intend to stick to that for mission-critical apps due to lifecycle reasons. Those are also the areas where we require authenticated mail for security policy reasons. So as far as this capability is concerned, we're currently at a disadvantage because we migrated from Framework to Core.

If you have suggestions for any workaround that would allow us to use Negotiate for authentication with SmtpClient in .NET 8 without backporting this, we're listening. And if you have a compelling argument for why it would be unsafe to backport this, we'd like to understand that as well. But absent either, I'd respectfully contend that this should be backported into .NET 8 as the active LTS release.

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

karelz commented 1 month ago

@avin3sh @kedia990 are you representing the same company? Can you please share more info here about the impact on your companies / applications? If it is sensitive, we can come up with different ways to communicate the info (email, official Microsoft support, etc.) - let me know if that's the case.

I don't think I saw explicit confirmation that the fix resolved the problem which was reported. Can you please confirm you tested it in test environment / production? We want to make sure we are fixing the right thing and that there are no unintended regressions.

Regarding quote from @kedia990

As to why it's a problem for us and nobody else - I can only speculate.

Exactly - we want to avoid speculations and rather based our decisions on data. Number of affected customers is one way (and an easy one) what to measure. We used that successfully for quite some time.

That said, it's not clear that that should matter - this PR fixes the regression from Framework, and more importantly, a regression that forces us into an insecure posture ...

I agree, that's why we fixed it in .NET 9 as soon as possible.

.NET 8 is the LTS release which the overwhelming majority of our applications are on and we intend to stick to that for mission-critical apps due to lifecycle reasons.

I assume that majority of your applications do not have the SmtpClient problem. Otherwise, I expect you would hit the problem much earlier, right? I would recommend to move the impacted application(s) to .NET 9 as an immediate mitigation of the problem. I assume it is an application you just migrated from .NET Framework and perhaps is not in production yet, right?

And if you have a compelling argument for why it would be unsafe to backport this, we'd like to understand that as well.

In general, we do not backport fixes, unless the customers are seriously impacted (no workaround), or the number of customers is higher. Keep in mind that even the safest looking change can cause a breaking change and that is what we are trying to avoid -- it helps ALL customers to not see surprises in servicing releases (which are super-painful for large number of customers to deal with).

I hope it helped to explain our position on the topics. If you have further questions, I will be happy to try to answer them.

kedia990 commented 1 month ago

@karelz - Appreciate your point-wise response. Yes, we represent the same company. Happy to discuss any relevant context / business impact further over email or a phone call whenever.

I don't think I saw explicit confirmation that the fix resolved the problem which was reported. Can you please confirm you tested it in test environment / production?

Confirming that .NET 9 (with the patch from #105939) fixed the issue for us with no regressions observed. This is called out in the first line of this issue, although not mentioned on the PR we made.

I assume that majority of your applications do not have the SmtpClient problem. Otherwise, I expect you would hit the problem much earlier, right? I would recommend to move the impacted application(s) to .NET 9 as an immediate mitigation of the problem. I assume it is an application you just migrated from .NET Framework and perhaps is not in production yet, right?

Almost all of our workloads are on .NET 8 / ASP.NET Core 8, and most of them use SmtpClient. We stumbled across this issue a few months ago when we started enforcing the requirement for mail to be authenticated on our mail servers to improve our security posture. We were able to successfully meet that requirement for every other language stack we use (Python, Java, Go etc.), our .NET Core apps are the only thing left behind. We try to stay as current as possible with .NET releases (I hope you'd agree that it's not unreasonable to be on the latest LTS release), but expecting that we shift double-digit number of applications from LTS to STS for a capability that's flat-out broken doesn't seem reasonable.

(To be clear - we didn't report the issue as soon as we discovered it months ago because we weren't sure where the issue was, and also explored alternatives / workarounds. Once we identified the precise fix required to un-break the feature, we straightaway went for the PR, with the reasonable expectation that a bugfix in a security capability would be backported.)

In general, we do not backport fixes, unless the customers are seriously impacted (no workaround)

We are seriously impacted (unable to comply with organization's security policy) as described above, and as I noted previously, we're happy to consider any workaround if you can share one with us (we've tried coming up with maintainable workarounds ourselves without luck so far). We need an answer to help developers across our firm do authenticated mail with .NET Core, but the answer cannot be "forklift all your codebases onto .NET 9". That's a bad look for us, and it really doesn't help me convince teams across our organization to use .NET.

Keep in mind that even the safest looking change can cause a breaking change and that is what we are trying to avoid

The fix in question isn't in the common path of SmtpClient; it's specifically in the module that's used for Negotiate. Is there any evidence to suggest that without our patch, that module works at all? Has anyone (either via CI, or a one-off) tested against an appropriately-configured mail server, and verified that it works as advertised? I'm more than happy for you to demonstrate otherwise, but if not, and this is just a regression from Framework in a very specific module that doesn't work at all, then it's really not clear to me what the risk is (but feel free to clarify).

kedia990 commented 1 week ago

Hey @karelz - I'm addressing your question in https://github.com/dotnet/runtime/pull/105939#issuecomment-2468328834 here since that PR is locked and I'm unable to respond there:

were you able to validate the .NET 9 fix on your scenario? (I need it for 8.0 backport)

Yes, @avin3sh had validated that the patch as shipped with .NET 9 fixed the Negotiate issue for us. (This was confirmed in https://github.com/dotnet/runtime/issues/108382#issuecomment-2388369981 as well). Let us know if you need any other detail on this, and thanks a ton for considering the backport!