fix redundant CRLF response during SMTP negotiate

[avin3sh]

Fixes #105938

Removes redundant CRLF response appended by Negotiate module. SmtpCommand already handles appending right message to the response, Negotiate doing this it in the auth module appears wrong and leads to CRLF being appended twice -- leading to SMTP server throwing an error.

This fix is important because MailKit, which is currently recommended in the docs, does not support Kerberos(jstedfast/MailKit#1249). Not having a functional Kerberos support in the BCL SmtpClient blocks any efforts of migrating from .NET framework.

[filipnavara]

Looks correct to me. I had a bit of trouble following the code path for checking whether the null message is handled correctly. The nullability annotations seem to be lacking, at best. Ultimately it ends up in BufferBuilder.Append which handles null string correctly.

[wfurt]

LGTM

[avin3sh]

Just to confirm, this fix should also flow to .NET 8 in next patch updates (once this PR is merged) ?

[karelz]

@avin3sh no, today is last day for it to easily flow into .NET 9.0. Starting tomorrow, it will have to go through approval process first.
If we confirm it works on .NET 9, we can consider backporting it to .NET 8, however, we will need more information about the impact for approval in servicing -- for example, why it is problem for you and nobody else, when the problem was out there for so long?

[avin3sh]

We ran into this when porting our critical .NET Framework projects to .NET 8. I am certain more users will run into this for similar reasons. I suspect this broke somewhere after .NET 6, perhaps after #71777 (guessing from large refactor of SMTP Negotiate related items). This combined with lack of SMTP-Negotiate test infra - #19436 - meant the issue went undiscovered for a while.

As more enterprises migrate to the .NET 8 LTS version, I am sure there will be more reports of this core functionality breaking. All of this and no-real alternative for Negotiate with SMTP through third party packages is the reason why I am advocating to backport this to 8 😄

[filipnavara]

I suspect this broke somewhere after .NET 6,

I traced the existence of the \r\n code at least 8 years back to the introduction of Unix support for the library. It was there for quite a long time.

[kedia990]

@karelz @wfurt on the topic of backporting to .NET 8 - 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.

[rzikm]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/11460496629

[karelz]

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