SSLMate / certspotter

Certificate Transparency Log Monitor
https://sslmate.com/certspotter
Mozilla Public License 2.0
962 stars 83 forks source link

Replicating email functionality using -script? (aka missing text for certain events) #69

Closed paravoid closed 1 year ago

paravoid commented 1 year ago

As you might remember, in Debian we ship with a certspotter-script that run-parts a directory with scripts. I'd like to offer the equivalent kind of functionality that -email provides, but without users having to disable other scripts, or to have to resort to override the systemd unit to add -email.

I've been gearing towards shipping an example shell script hook, to be placed under /etc/certspotter/hooks.d, with something like:

#!/bin/sh
TO=$(cat /etc/certspotter/email-notify)

( cat <<EOF
To: $TO
Subject: [certspotter] ${SUMMARY}
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
X-Mailer: certspotter

EOF
cat ${TEXT_FILENAME} ) | /usr/sbin/sendmail -i -- $TO

This is based off discoveredcert.go and seems to be able replicate its functionality. (Error handling to be added, as well as handling for a non-existent $TO etc. X-Mailer to be reconsidered).

1) How do you feel about that? Risks, issues or other thoughts that you have? Any other alternatives that you can think of? 2) For EVENT=error (staleSTHEvent, backlogEvent, stateLogListEvent) as well as EVENT=malformed_cert (malformedLogEntry) events, it seems like there is no way for the script to access Text(). Do you think we could find a way for this to find its way on the filesystem as well, so that it could be cat'ed into the output as well? It seems like a useful thing to be storing for further inspection anyway, no? 3) This is more minor but I couldn't help to notice the duplication between $SUMMARY and EmailSubject() with very subtle differences (capitalization). Perhaps that's something that could be DRY'ed?

Thanks again for all of your efforts!

AGWA commented 1 year ago

I'd like to offer the equivalent kind of functionality that -email provides, but without users having to disable other scripts, or to have to resort to override the systemd unit to add -email.

Makes sense. Note that -email can be used in addition to -script so there shouldn't be any need to disable the other scripts, but it would indeed require overriding the systemd unit, which I can see would be undesirable.

I see two possible approaches:

  1. Make the value of Text() available to scripts in $TEXT_FILENAME, in which case your proposed script looks good.
  2. Have certspotter itself read $CERTSPOTTER_CONFIG_DIR/email-notify to find the addresses to email. A big advantage of this approach is that it would help provide a consistent experience across distros (at some point certspotter could also support $CERTSPOTTER_CONFIG_DIR/hooks.d directly).

I think the second approach would be easier to implement. The first approach is probably fine but I would like more time to think about possible implications. What's the latest date that certspotter could be uploaded in time for bookworm?

paravoid commented 1 year ago

Thank you for your -always insightful- response!

I see two possible approaches:

1. Make the value of `Text()` available to scripts in `$TEXT_FILENAME`, in which case your proposed script looks good.

I believe that's already the case for discoveredCerts, no? The part that's missing is for other events, and right now I believe Text() just isn't saved anywhere on the filesystem for those, as far as I can tell?

2. Have certspotter itself read `$CERTSPOTTER_CONFIG_DIR/email-notify` to find the addresses to email. A big advantage of this approach is that it would help provide a consistent experience across distros (at some point certspotter could also support `$CERTSPOTTER_CONFIG_DIR/hooks.d` directly).

That makes sense to me :) The downside here is that one cannot just modify the hook to replace /usr/bin/sendmail by e.g. a /usr/bin/curl , and pass Text() to a web service with something like SendGrid. (That's theoretical, I haven't heard anyone request that, yet!). But I do like the upside that you mentioned, plus of not having to implement the same functionality twice with who knows what kind of subtle breakages in the future! Certainly makes my work easier!

I think the second approach would be easier to implement. The first approach is probably fine but I would like more time to think about possible implications. What's the latest date that certspotter could be uploaded in time for bookworm?

Thank you for caring for our timelines!

Bookworm has 0.15.0 as of this past weekend, and will have 0.15.1 in ~10 days or so. Per the timeline, as of this past Sunday, we are now in the "soft freeze" period, which means "only small, targeted fixes". We will have the opportunity to upload such fixes for the next 3-4 months or so. I believe a change like the one we are talking about here would stretch the rules a bit, but qualify.

Separately, Ubuntu 22.04 LTS release timeline suggests that the last day for the "Debian import freeze" is February 23rd, so we may miss the opportunity to make any further changes there. I'll try to figure out whether I can ask for an exception when the time comes (I'm not super familiar with their processes).

Thanks again!

AGWA commented 1 year ago

I just released v0.16.0 which adds $TEXT_FILENAME for error and malformed_cert events, and adds native support for executing scripts in $CERTSPOTTER_CONFIG_DIR/hooks.d and reading email addresses from $CERTSPOTTER_CONFIG_DIR/email_recipients. This should provide a great out-of-the-box experience for certspotter users and let you simplify the Debian package.

I also took your suggestion to remove the redundancy between $SUMMARY and EmailSubject().

If you could get an exception for the Ubuntu import that would be great, though Ubuntu 23.04 is fortunately not an LTS so if it ships with 0.15.0-1 it would not be the end of the world.

Thanks for all your ideas and work on the Debian packaging!

paravoid commented 1 year ago

That's amazing! Thank you so much :) I'll have a look ASAP, I expect this to be in the next 1-2 days or so.