Skip to content

Add SSL validation suppression option#77

Closed
MattMofDoom wants to merge 3 commits into
datalust:devfrom
MattMofDoom:dev
Closed

Add SSL validation suppression option#77
MattMofDoom wants to merge 3 commits into
datalust:devfrom
MattMofDoom:dev

Conversation

@MattMofDoom
Copy link
Copy Markdown
Contributor

Hi @nblumhardt,

This is a simple pull to add an option to suppress SSL validation errors for specific scenarios, such as self-signed certs and some wildcard certificate scenarios. I've noted in the description that it should only be used if the SMTP server is explicitly trusted, but there's definitely valid scenarios for this where SSL should be used but is prevented by the standard .NET TLS validation.

I know you have some pending PRs which are obviously linked to 2021.3 and the alerting improvements. Some additional enhancement ideas that I'm contemplating to uplift the capabilities of Email Plus ...

  • Given deprecation of SmtpClient, move EmailPlus to use the recommended replacement, MailKit (it's great and supports cross platform) ... more info at https://docs.microsoft.com/en-us/dotnet/api/system.net.mail.smtpclient?view=netframework-4.7.2#remarks, but I'd be happy to take a look soon; quite straightforward overall
  • Add an optional fallback mail host if delivery to the primary host fails; this would allow mail to still be delivered if you have an alternate path when the primary host is unavailable
  • Optional direct delivery using DNS? Possibly option for a tertiary fallback, but might also be viable as a primary if Host is made optional, eg. if not configured use DNS ... need to think on this but seems do-able at first thought
  • Add additional envelope options - eg. priority, cc, bcc, replyTo
  • Option to map mail priority to event properties such as @Level or other property, like we did with Seq.App.Opsgenie
  • Option to map the to/cc/bcc addresses to an event property (eg. like the Responders property from Opsgenie app)
  • Possible option to send the json event as an attachment? Could be useful for some scenarios.
  • Passing event tags might not fit for an SMTP scenario, although it's possible they could be passed through to the EmailPlus debug logs (below)
  • Add optional alternate plain text body template; it's 'good practice' at least to include a plain text alternative
  • Validation of mail addresses - probably can't return an error back to the Seq app settings when misconfigured, but we could prevent invalid mail addresses being sent and log an event, eg. warning if we suppressed one address, error if there are no valid email addresses
  • Add some debug logging that would permit for alerting if any errors etc arise

All of these would be relatively straightforward to pull in, and you can probably appreciate the benefit of these. More than happy to hear your own thoughts, of course 😊

Cheers,

Matt

@MattMofDoom
Copy link
Copy Markdown
Contributor Author

I'll update this when I convert to MailKit so that only the mail client uses the certificate validation call back, as better practice.

@nblumhardt
Copy link
Copy Markdown
Member

Hi Matt! Thanks for the PR! Some really nice-sounding improvements in your list; maybe to streamline things we should discuss each separately, in whatever order you think makes sense?

I'm not keen on certificate validation suppression; it sounds to a consumer like it's "support my self-signed certificate", but in reality, it's "don't use most of SSL". When certificate validation is suppressed, most (all?) benefits of SSL are foregone - traffic can be MITM'd and observed, the identity of the server is not verified (and so can be spoofed), etc.

For the self-signed certificate case, it's easy on most platforms just to add the certificate to the trust store on the machine hosting Seq; on Windows it can be added to the Trusted Root CAs store and all will be well (as long as the cert lists the correct hostname etc.).

Given that the right thing is reasonably easy to do, I don't think we should go to any effort to enable doing the (probably) wrong thing - or at least that's the line of reasoning I've ended up with when I've encountered this issue myself in the past. What do you think?

@MattMofDoom
Copy link
Copy Markdown
Contributor Author

@nblumhardt Of course you're right from a practical sense. The "better" option is not to provide the option, so I'd be happy to withdraw the PR.

Happy to chat about the list. Some are fairly straightforward, like MailKit. Would you prefer that I send a PR with that and maybe some modelling of some of the simpler thoughts?

@nblumhardt
Copy link
Copy Markdown
Member

Thanks for your reply, @MattMofDoom; sounds good. The MailKit change sounds like a good one - I wonder if there are any gotchas to think about? (Does System.Net.Mail use any system-wide credentials/settings that MailKit won't?)

@MattMofDoom
Copy link
Copy Markdown
Contributor Author

@nblumhardt I found it was a pretty straightforward translation .. in fact for Lurgle.Alerting, I created the ability to configure either, with property equivalency on both options, including credentials etc. MailKit is pretty strong, and I guess that's why MS is recommending it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants