Skip to content

Commit

Permalink
smtpserver: add prometheus metric for failing starttls handshakes for…
Browse files Browse the repository at this point in the history
… incoming deliveries

and add an alerting rule if the failure rate becomes >10% (e.g. expired
certificate).

the prometheus metrics includes a reason, including potential tls alerts, if
remote smtp clients would send those (openssl s_client -starttls does).

inspired by issue #237, where incoming connections were aborted by remote. such
errors would show up as "eof" in the metrics.
  • Loading branch information
mjl- committed Nov 29, 2024
1 parent 09e7ddb commit afb182c
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 5 deletions.
7 changes: 6 additions & 1 deletion prometheus.rules
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,14 @@ groups:
# the alerts below can be used to keep a closer eye or when starting to use mox,
# but can be noisy, or you may not be able to prevent them.

- alert: mox-incoming-delivery-starttls-errors
expr: sum by (instance) (increase(mox_smtpserver_delivery_starttls_errors_total[1h])) / sum by (instance) (increase(mox_smtpserver_delivery_starttls_total[1h])) > 0.1
annotations:
summary: starttls handshake errors for >10% of incoming smtp delivery connections

# change period to match your expected incoming message rate.
- alert: mox-no-deliveries
expr: sum(rate(mox_smtpserver_delivery_total{result="delivered"}[6h])) == 0
expr: sum by (instance) (rate(mox_smtpserver_delivery_total{result="delivered"}[6h])) == 0
annotations:
summary: no mail delivered for 6 hours

Expand Down
51 changes: 51 additions & 0 deletions smtpserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net"
"net/textproto"
"os"
"reflect"
"runtime/debug"
"slices"
"sort"
Expand Down Expand Up @@ -59,6 +60,7 @@ import (
"github.com/mjl-/mox/smtp"
"github.com/mjl-/mox/spf"
"github.com/mjl-/mox/store"
"github.com/mjl-/mox/tlsrpt"
"github.com/mjl-/mox/tlsrptdb"
)

Expand Down Expand Up @@ -171,6 +173,21 @@ var (
"error",
},
)
metricDeliveryStarttls = promauto.NewCounter(
prometheus.CounterOpts{
Name: "mox_smtpserver_delivery_starttls_total",
Help: "Total number of STARTTLS handshakes for incoming deliveries.",
},
)
metricDeliveryStarttlsErrors = promauto.NewCounterVec(
prometheus.CounterOpts{
Name: "mox_smtpserver_delivery_starttls_errors_total",
Help: "Errors with TLS handshake during STARTTLS for incoming deliveries.",
},
[]string{
"reason", // "eof", "sslv2", "unsupportedversions", "nottls", "alert-<num>-<msg>", "other"
},
)
)

var jitterRand = mox.NewPseudoRand()
Expand Down Expand Up @@ -955,7 +972,25 @@ func (c *conn) cmdStarttls(p *parser) {
ctx, cancel := context.WithTimeout(cidctx, time.Minute)
defer cancel()
c.log.Debug("starting tls server handshake")
metricDeliveryStarttls.Inc()
if err := tlsConn.HandshakeContext(ctx); err != nil {
// Errors from crypto/tls mostly aren't typed. We'll have to look for strings...
reason := "other"
if errors.Is(err, io.EOF) {
reason = "eof"
} else if alert, ok := asTLSAlert(err); ok {
reason = tlsrpt.FormatAlert(alert)
} else {
s := err.Error()
if strings.Contains(s, "tls: client offered only unsupported versions") {
reason = "unsupportedversions"
} else if strings.Contains(s, "tls: first record does not look like a TLS handshake") {
reason = "nottls"
} else if strings.Contains(s, "tls: unsupported SSLv2 handshake received") {
reason = "sslv2"
}
}
metricDeliveryStarttlsErrors.WithLabelValues(reason).Inc()
panic(fmt.Errorf("starttls handshake: %s (%w)", err, errIO))
}
cancel()
Expand All @@ -971,6 +1006,22 @@ func (c *conn) cmdStarttls(p *parser) {
c.tls = true
}

func asTLSAlert(err error) (alert uint8, ok bool) {
// If the remote client aborts the connection, it can send an alert indicating why.
// crypto/tls gives us a net.OpError with "Op" set to "remote error", an an Err
// with the unexported type "alert", a uint8. So we try to read it.

var opErr *net.OpError
if !errors.As(err, &opErr) || opErr.Op != "remote error" || opErr.Err == nil {
return
}
v := reflect.ValueOf(opErr.Err)
if v.Kind() != reflect.Uint8 || v.Type().Name() != "alert" {
return
}
return uint8(v.Uint()), true
}

// ../rfc/4954:139
func (c *conn) cmdAuth(p *parser) {
c.xneedHello()
Expand Down
3 changes: 2 additions & 1 deletion tlsrpt/alert.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import (
"strings"
)

func formatAlert(alert uint8) string {
// FormatAlert formats a TLS alert in the form "alert-<num>" or "alert-<num>-<shortcode>".
func FormatAlert(alert uint8) string {
s := fmt.Sprintf("alert-%d", alert)
err := tls.AlertError(alert) // Since go1.21.0
// crypto/tls returns messages like "tls: short message" or "tls: alert(321)".
Expand Down
3 changes: 2 additions & 1 deletion tlsrpt/alert_go120.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
)

func formatAlert(alert uint8) string {
// FormatAlert formats a TLS alert in the form "alert-<num>".
func FormatAlert(alert uint8) string {
return fmt.Sprintf("alert-%d", alert)
}
4 changes: 2 additions & 2 deletions tlsrpt/report.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ func TLSFailureDetails(err error) (ResultType, string) {
// todo: ideally, crypto/tls would let us check if this is an alert. it could be another uint8-typed error.
v := reflect.ValueOf(netErr.Err)
if v.Kind() == reflect.Uint8 && v.Type().Name() == "alert" {
reasonCode = "tls-remote-" + formatAlert(uint8(v.Uint()))
reasonCode = "tls-remote-" + FormatAlert(uint8(v.Uint()))
}
}
return ResultValidationFailure, reasonCode
Expand Down Expand Up @@ -429,7 +429,7 @@ func TLSFailureDetails(err error) (ResultType, string) {
}
v := reflect.ValueOf(err)
if v.Kind() == reflect.Uint8 && v.Type().Name() == "alert" {
reasonCode = "tls-local-" + formatAlert(uint8(v.Uint()))
reasonCode = "tls-local-" + FormatAlert(uint8(v.Uint()))
}
}
return ResultValidationFailure, reasonCode
Expand Down

0 comments on commit afb182c

Please sign in to comment.