Default body template including semicolon after named property in rendered message

[c0shea]

I'm not sure why, but the {{$Message}} that is rendered in the email body is including a semicolon after the second named property from the Health Check input, even though that semicolon is not in the event payload in Seq. When our operations team receives the error email, they go to click the link, but it always fails because our email client is treating the double quote as part of the URL because it sees the semicolon (that shouldn't be there) after it.

There's no semicolon in the JSON in Seq

[KodrAus]

Hi @c0shea 👋 That's strange! It looks like you might be using an older version of the Email app. Is it possible to first try upgrading to the current 3.1.0-dev-00179 version and see if you're still seeing that additional semicolon?

[c0shea]

Thanks @KodrAus. I can't get the latest version to send an email to test the semicolon issue. It keeps writing a diagnostic event to the stream about MailKit not being able to establish a TLS connection. I'm perplexed, though, since I don't have the Require TLS checkbox enabled in the app settings. Any thoughts?

[KodrAus]

Ah, the new version of the app is stricter on SSL, so if you're using port 465 or 587 it will try to automatically establish a secure connection, even without checking the TLS box. It looks like when this is happening the mail server is returning a certificate with a different hostname.

[c0shea]

I don't understand why it would try to use SSL/TLS, though. I have the port set to 25 and don't want it to try and establish a secure connection.

[KodrAus]

Sorry @c0shea, we will also try establish a secure connection on port 25 if the mail server advertises StartTLS. I think the options here are:

• Disable StartTLS on port 25 in your mail server if it's possible.
• Remove the offending certificate.
• Reconfigure the offending certificate so that it's valid.

The first option is probably the easiest if that's possible.

[c0shea]

Ah that'll be difficult to do, as our networking team handles this for the enterprise. I'll see what I can figure out.

[MattMofDoom]

@KodrAus @c0shea PR #81 and its downstream #83 would allow the TLS behaviour to be configured in the Email+ app, but the Seq team have been understandably busy with 2021.3 so it hasn't been reviewed.

[KodrAus]

Thanks for the ping @MattMofDoom! It's on our list to review through 😅 You're right that we've had our hands a bit full with 2021.3 so haven't had the headspace to give it the attention it deserves just yet.

[MattMofDoom]

All good @KodrAus and completely understand - I definitely don't mean to be a pest, but thought a ping might be helpful 'just in case'.

[c0shea]

I narrowed down the issue. Our mail server does actually have a valid certificate that is publicly trusted by DigiCert. However, our Seq server isn't allowed to call the OCSP URLs (e.g. ocsp.digicert.com) to check for certificate revocation. Even though the certificate isn't revoked, it fails the revocation check which means no email will ever be sent. Compared to web browsers like Chrome which use an offline CRL, MailKit attempts to use the online OCSP first and not default to an offline check with the built-in Windows CRL.

I'm trying to get our networking and security teams to allow OCSP checks, but in the meantime is it possible to add an option to disable the revocation checks?

client.CheckCertificateRevocation = false;

[c0shea]

Our networking team allowed the OCSP checks so now the email sending works with the latest version of the plugin. So now back to the original issue, the semicolon is still present after the URL in the title.

[c0shea]

Turns out this is an issue with the email client we use at work (Lotus Notes). When I send an alert email to my external email address and open it using any other mail client it doesn't render a semicolon after the URL. My apologies. I should have tested this sooner. Thanks for your help!

[MattMofDoom]

@c0shea I think your point on allowing the revocation check to be disabled may be valid. There are certainly reasons that the OCSP/CRL might not be accessible from Seq.

I had previously added an option to disable certificate validation altogether, which I removed after discussion with @nblumhardt .. but I think this one might fly since it has lower impact or risk. I'll look to add it as an option in #81

[nblumhardt]

@MattMofDoom thanks for the ping! Perhaps before adding it to another open PR, it might be good to figure out a strategy for moving this repo forward?

The two things that I think are holding up the works (besides our regrettable lack of time to spend on this right now :-)) are:

• The world of SMTP mail seems to be pretty nuanced and fragile, making it hard to confidently review and accept PRs that span multiple interlocking features (including our own :-)). It's much easier to review individual changes with bounded scope. For example, adding a skip-CRL-checks option on its own would be quick and easy to inspect/discuss/review, so would have a chance of moving forwards (or being abandoned) much faster than if it goes onto the long Delivery enhancements #81 PR, which we'll make harder to progress with each additional change.
• The Seq app settings UI doesn't accommodate a lot of settings for each app, so there's a tension between adding functionality and keeping the configuration experience approachable. Perhaps we should do some foundational work adding an IsAdvanced or Category option to SeqAppSettingAttribute first, that can drive a default-collapsed "Advanced" section in the Seq UI? We're nudging this forward in 2021.4 with the new Syntax option, perhaps there's time to squeeze support for advanced settings in there, too? (We wouldn't yet be able to surface it in the UI, but apps could start being updated for a 2022.x release.) The important part for making it into the 2021.3 milestone would be support in seqcli as in this PR.

Just a bit of a brain-dump, but keen to know your thoughts!

[MattMofDoom]

@nblumhardt Thanks Nick. I took time to perform recon on MailKit and it's clear that there isn't a way to add OCSP/CRL revocation support via proxy, so a firewalled Seq would need the option to disable CRL checks. That code is quite simple, but yes - it's another option in the UI.

In terms of the approach - agree that it becomes more complex to review larger PRs in the SMTP arena, and hence I took the code over to Lurgle.Alerting to be able to even have confidence in my own contributions! #81 in particular is an architectural change to allow fallback hosts and DNS delivery - so it's particularly difficult to break up into smaller chunks.

The complexity is that I then have #83 queued behind it to add additional cc/bcc/replyto and priority fields, plus alternate plain text, so it touches many of the same areas of code, and any amendment to #81 necessitates merging updates to that as well. Understanding this complexity, I've just been patiently waiting and merging upstream changes when they arise 😊 but I definitely take your point that adding yet another option may not be ideal, particularly to review/merge quickly.

To your point - I'd love to see an opportunity for advanced/category. I definitely run into scenarios where the settings page is much longer/more complex than I'd like in order to provide the functionality of an app - Event Timeout and its siblings are a very clear example and at the very least, functionality like the Abstract API Holidays that may not be used could go to an Advanced section. There's a lot of advanced functionality, but even many of my own configs for these apps don't use most of the settings.

For Email+, trying to visualise how an advanced section would work; if we took options like DeliverUsingDns for #81, and the Priority, Cc, Bcc, ReplyTo for #83, to an advanced section as examples - would we surface them in the config "if" they are configured? eg. You need to expand Advanced to configure them but once they are set, they are visible in the main UI. High level, that approach would likely mean all advanced properties have to be nullable, or otherwise have a checkbox to allow users to enable/disable each item.

Going down this path and thinking further again - outside of this app and not necessarily just for 'advanced' - I'd probably vote to add support for a dictionary type setting so that KeyValuePairs could be used, if that's viable. For example, Event Timeout offers matching multiple properties of an event, and I arbitrarily decided on allowing up to 5 properties - which means 10 individual string type configs (property name, value to match). Great for taking full advantage of structured logging, but clunky and potentially confusing. If a dictionary were available, it could be pivoted to an extensible set of Key = property name, Value = string to match.

You've also seen one way that I'm working around this in other apps such as Seq.App.Opsgenie with comma delimited key=value strings, but that has its definite drawbacks.

One added thought would be that in some cases an enumerable of type string would be helpful - for example in #83, rather than a comma-delimited list of mail hosts, this could be an extensible list. A dictionary might be usable as well, but we wouldn't really be making use of the key (except possibly for logging a 'friendly' name for the mail host).

Then the follow on thoughts are that To, Cc, Bcc could also be enumerables, etc, etc.

[c0shea]

@nblumhardt I like the idea of an advanced section that can be + out. I envision that being similar to what you guys did with the "Customize this notification" in the new alerts UI that expands more options.