Support Untrusted Certificates

[Jaben]

First of all, thanks for Seq and all its awesomeness.

We don't support secure (SSL/TLS) servers internally for sending emails. The latest version will check if TLS is available and "switch to it." But that fails because our certificates are self-signed. I realize this is a company IT problem -- but I doubt I'm the only one with this problem.

Possible Options for the plugin:

• Could support a "Force Insecure" option -- SecureSocketOptions.None option.
• Could support disabling SSL Verification option e.g.: https://github.com/jstedfast/MailKit/issues/1227

I'm happy to implement these solutions and make a PR. But I don't want to waste time if you don't want the plugin going down this road, @nblumhardt.

For now, I'm keeping the plugin version on v3.0.0.

Thanks!

[nblumhardt]

Thanks for the heads-up, @Jaben.

I'm assuming that trusting the self-signed cert on the Seq server isn't an option for you? That's generally the ideal approach, but understandably not always practical.

@MattMofDoom spotted this issue earlier, and kindly wrote up some thoughts in https://github.com/datalust/seq-app-htmlemail/pull/81#issuecomment-907538808; I think his suggestion to add UseTlsWhenAvailable is similar to your "Force Insecure".

I'd rather avoid an option to disable cert validation, so exploring that avenue first.....

The pair-of-checkboxes approach detracts from usability a bit, and leaves us having to communicate semantics carefully to the user; perhaps we should look at what's required to move this setting from a Boolean to an enum/select (effectively matching SecureSocketOptions)?

The options would be something like "Always", "When Available", and "Never" (though we could also consider explicit "On Connect"/"STARTTLS" options).

Seq already has support for this type of app setting; the main stumbling block is migrating existing instances of the app across. I might just quickly review what options we have, there, and loop back.... 🤔

[MattMofDoom]

@nblumhardt @Jaben You rang? 😂

By way of quick recap - we agreed in a previous discussion (#77) on not disabling SSL validation, and I withdrew an earlier pull request for that. I don't think we need to go back there, although my testing around #81 showed that there were certainly scenarios where the current SecureSocketOptions.StartTlsWhenAvailable is problematic and users will need to configure settings like SecureSocketOptions.None.

I think it's fundamental that we need an ability to disable TLS altogether - we lost that with the move to MailKit, hence why I was adding UseTlsWhenAvailable to reinstate the old SmtpClient behaviour and also duplicate the behaviour of the simpler UseSsl boolean in MailKit.

I definitely prefer an enum/select that matches SecureSocketOptions - that will get us to where I was trying to go with dual tickboxes. I wasn't aware that we could provide an select dropdown (didn't see it in the input types) or I'd probably have done it in the PR 😊

I also like this with reference to #81's overall enhancements for multiple servers and delivery via MX lookup in DNS, since fine grained control over TLS becomes more important.

Suggestion for how to migrate -

1. We preserve the 'old' EnableSsl setting, but stop presenting it to the UI
2. We add a new EnableTls setting based on the enum, as a nullable setting
3. First run, we find that EnableSsl is not null and EnableTls is null, so we migrate to the appropriate enum setting
4. Future runs will ignore EnableSsl because EnableTls is not null

@nblumhardt This should have only minimal impact to #81 and #83 since I'd consolidated the settings to a single GetSocketOptions call.

Cheers,

Matt

[MattMofDoom]

@nblumhardt Further thought and kind of consistent with #81 ... If both the EnableTls nullable enum AND the now-hidden EnableSsl bool? are null, then we use SecureSocketOptions.Auto, which again should have some consistency with MailKit behaviours observed. This covers a scenario that someone adds a new instance and does not configure the EnableTls enum.

Hell, I'd adjust the PR to this now if I could (or knew how to) expose the select dropdown input type 😂

[MattMofDoom]

@nblumhardt So I got off my backside and figured out where you had implemented the enum dropdown select functionality. The only thing I couldn't figure out was adding display names to the enums - just not sure if your comment on https://github.com/datalust/seqcli/pull/160 accurately reflects that there should be a [Display{..}] attribute.

I have implemented the migration from EnableSsl and added appropriate tests - all working. I did preserve the implicit TLS behaviour when the port is set to 465. We map an enum to SecureSocketOptions which allows it to be readily cast - this was because I was trying to implement the Display attribute, and I've left it as is because we can always circle back if/when a friendly display name is possible.

I made a temporary Nuget package available for my dev purposes and was able to do some testing around the TLS options - as well as the DNS delivery (failed because my ISP blocks outbound SMTP, boo, but the resolution and delivery attempt works!). The enhanced logging is helpful too.

Hoping this fits the bill!

[Jaben]

I'm assuming that trusting the self-signed cert on the Seq server isn't an option for you? That's generally the ideal approach, but understandably not always practical.

It's all hosted in Docker. But, I create my own flavor of Datalust Seq image so I could add the cert. It's a good idea -- worth a shot.

The options would be something like "Always", "When Available", and "Never" (though we could also consider explicit "On Connect"/"STARTTLS" options). Seq already has support for this type of app setting; the main stumbling block is migrating existing instances of the app across. I might just quickly review what options we have, there, and loop back.... 🤔

Had this exact thought -- that it should be a SELECT -- but didn't see that option. Makes sense it would create backward compatibility issues. Looks like @MattMofDoom is way ahead on all of this ;)

[MattMofDoom]

I've published https://www.nuget.org/packages/Seq.App.EmailPlus-Enhanced/ to Nuget as a test package.

This has the code from my two open pull requests, which allows for testing of the enhanced code.

This includes the drop down list for TLS options - so for self signed certs it's viable to set TLS to "None" which will avoid the problem that occurs with the current default.

[mateuszligeza]

@nblumhardt @liammclennan I created simple PR #95 that is meant to fully disable the server certifiate validation. I can't force organisation to change the way it operates and fix the reverse proxy that is messing up the mail server certificates (and preventing me from getting alerts from seq). I'm currently using the code from the above PR as a custom nuget package, but it would be nice to get this workaround added to the official repo.

[MattMofDoom]

@mateuszligeza The above will likely be rejected based on a previous PR where I attempted to add the same.

However take a look at the https://www.nuget.org/packages/Seq.App.EmailPlus-Enhanced/ which is #81 and #83 as a full package. This includes the dropdown list option to set TLS to None, which is the effect you need. These just haven't been merged yet because they're quite big enhancements, due to the nature of the features added.

For the major part, the added features are entirely optional.

[mateuszligeza]

@MattMofDoom correct me if I'm wrong, but your code is more trying to manage the TLS options and DNS names. I need to use the TLS (and current app settings are good enough for me) but just skip the certificate validation and trust whatever the reverse proxy is giving me. This is something different to what you are trying to achieve with your changes. I could not see anywhere in your code changes made to the SmtpClient.ServerCertificateValidationCallback that my PR uses.

As you mentioned your changes are quite big, where as mine are pretty small and dealing with just the problem at hand.

I believe it would be good to have both merged to the official repo, but it will be up to maintainers to decide. I don't mind rebasing my changes on top of yours if yours are merged earlier.

[MattMofDoom]

Hi @mateuszligeza,

The overall code set for those pulls is a set of significant enhancements to both SMTP delivery and envelopes. As part of this I addressed the ongoing issue with certificate validation. The earlier discussion in #77 is relevant here - in short @nblumhardt was not comfortable with that same change (ServerCertificateValidationCallback) in the pull request. This did pre-date the move to MailKit so had a broader implication when used, but I believe his feeling hasn't changed on this per the above discussions.

From Nick's standpoint, we should either use TLS and validate the certificates, or else not use TLS. Hence specifically in #81 I wound up adding the dropdown list for TLS options, which includes the None option. #83 is just built on top of #81 so of course includes the same, and hence "EmailPlus Enhanced Edition" is made available on Nuget in the interim.

I don't oppose the ability to disable certificate validation myself - I see Nick's point and I've gone with that guidance, but obviously if you get a different answer, that's cool too.

Frankly if Nick chimed in and said "yes, I see the point, and agree", I could readily merge yours into the #81 and #83 pulls without much pain, so it wouldn't really have an impact on my pull requests. I'm just being helpful and letting you know of what's already been discussed 😊

Cheers,

Matt

[mateuszligeza]

@MattMofDoom thank you for comprehensive explanation.

I didn't have a chance to speak with Nick. I do agree that bypassing the Certificate validation pretty much is same as not using the TLS/SSL in the first place, as it enables the MITM. However reverse proxy is exactly the MITM. In my case when connecting to the mail server mail.contoso.com client would get the certificate generated on the fly by reverse proxy, unfortunately the certificate would use the mailServer01 (individual server host name instead of it's dns name) as certificate subject effectively breaking trust. I can't force the infrastructure team to fix it.

However, you are right, by dropping the support for SSL/TLS by using the SecureSocketOptions.None I am able to send emails as well. Luckily for me the non secure connection is still available. I was more inclined to use the TLS with skipped certificate validation in case the infrastructure team decides to drop the support for non TLS connections.

TLDR I'm happy to use your version and abandon my PR given Nick is not comfortable with ServerCertificateValidationCallback approach.

[MattMofDoom]

No worries @mateuszligeza ... I have a similar situation in certain cases myself, so I understand the pain point you're addressing. I tend to think TLS with a non-validated certificate is better than no TLS - however like yourself, using None works for me as well. I've looked at trusting certs etc but for one reason or another it doesn't seem to work for me.

I wound up using my version in production particularly because I could leverage the TLS = None, as well as the added multiple mailhost functionality and the CC field. The added logging is also helpful, because it means a log entry is generated whenever an email is sent, with all the properties you could want for troubleshooting/debugging. I actually had a case where the older Email+ was not sending emails at all in some cases and not logging an error. This version seems substantially more robust, but if an issue occurred, I'd have a log to look at or alert on. It's underscoring some critical notifications so I'm really pleased that it's working so well.

Have a great day mate.