EmailConnectionInfo.EnableSsl setting is inconsisntent between implementations

[andriysavin]

When I was switching my library (consuming email sink) from net45 to netstandard1.3, I started observing an error with the same set of SMTP server settings (smtp.office365.com:587). Switching between mentioned TFMs I switched from System.Net.Mail to MailKit implementation of SMTP client. It turned out that these implementations treat EmailConnectionInfo.EnableSsl flag differently. Below are links to the flag usage: https://github.com/serilog/serilog-sinks-email/blob/498d862c021dbe4d2380e446e7378bf54f8cca0a/src/Serilog.Sinks.Email/Sinks/Email/EmailSink.cs#L154 https://github.com/serilog/serilog-sinks-email/blob/498d862c021dbe4d2380e446e7378bf54f8cca0a/src/Serilog.Sinks.Email/Sinks/Email/MailKitEmailSink.cs#L140

So in case of System.Net.Mail the flag allows to opt in/out for secure connection usage (usually TLS). But in case of MailKit the flag seems to be a switch between SSL and TLS. This behaviour (with EnableSsl = true) resulted in an error similar to this in my case, since apparently the server wanted to use TLS, but the client was forcing SSL.

My conclusion above is an assumption, since I didn't dive deeply into the network transport implementations. However, it seems to be true, and switching the flag to false magically solved my problem. BUT this is unstable, because if for example at some point the sink library is migrated to netstandard2.0 and fully switched to System.Net.Mail implementation, after blindly upgrading the package I will silently switch to unsecured connections! This is a big issue.

So I believe the code must be fixed to treat the flag in a consistent way or/and the flag should be renamed to make its intention more clear.

UPDATE: It looks like it's even more tricky, here is an example. If your library (which consumes Email Sink) targets netstandard, you may expect it will always use the netstandard version of the sink. However, if your library is referenced from a net4xx project, its dependencies are resolved for net4xx. So you may expect you're safe because you're referencing a version with particular implementation, but really the implementation will depend on the TFM the root project targets. E.g. you may reference your netstandard library project from two projects which target net4xx and netcoreapp, and get different behaviour in each of them!

[nblumhardt]

Sounds like a bug, thanks for investigating @andriysavin 👍

PRs welcome, if anyone is able to take a look.

[andriysavin]

As a simple solution I suggest releasing new package (major?) version which targets netstandard2.0 and uses System.Net.Mail. New version should not target any other TFMs (like .Net Framework). Thus, those who want backward compatibility (who rely on MailKit's EnableSsl interpretation), will have to stay with existing package version. Others, who want consistency among apps which target different frameworks, will use the new version. Luckily, System.Net.Mail has much more intuitive EnableSsl interpretation, and is "in the box" in any nestandard2.0 supporting framework.

[nblumhardt]

@andriysavin I think that's a decent proposal. We could also go the other way and standardize on MailKit? Worth also considering, especially because of https://www.infoq.com/news/2017/04/MailKit-MimeKit-Official

[andriysavin]

@nblumhardt Honestly, I'd prefer MailKit too. The only reason I suggested System.Net.Mail is that this way requires almost no work while retaining compile-time compatibility for clients. If MailKit has another option which provides the same sematic as EmailConnectionInfo.EnableSsl (enable or disable secure connection), then MailKit wins. One minor drawback of this way is that this relatively simple emailing functionality will require whole MailKit binaries and its dependencies (I observed MimeKit.dll and crypto.dll). This may or may not be an issue (but so far it was ok for .Net Core consumers).

[andriysavin]

Worth also considering, especially because of https://www.infoq.com/news/2017/04/MailKit-MimeKit-Official

Note that in the last comment somebody says SmtpClient is not really obsolete and marking it so happened by mistake.

[nblumhardt]

Ah, I see - thanks for pointing that out, @andriysavin.

Here's another possibility - we rev the major version of this package, point to System.Net.Mail only, and fork the repository to create a new alternative package Seq.Sinks.MailKit? Would need someone to champion the work, but feels like the least surprising path for users in the long run...

[andriysavin]

That could work as well. And, again, the first part can be done very fast, as I understand.

[nblumhardt]

3.0.0-dev-* resolves this. Thanks!