smtp2shoutrrr/backend.go
butterrobot 7565618efd
Some checks failed
CI / goreleaser-lint (push) Successful in 3s
CI / format (push) Successful in 1m47s
CI / lint (push) Successful in 2m7s
CI / test (push) Successful in 13m1s
CI / build (push) Successful in 12m51s
Release / release (push) Failing after 1m7s
bugfix: fix SMTP authentication bypass, remote DoS, and silent delivery failures (#9) (FMG-3)
Three defects in the SMTP backend, all predating the FMG-2 dependency upgrade.

## Authentication bypass

The credential check joined its two comparisons with `&&`, so it only rejected a client that got *both* halves wrong — a correct username with any password authenticated, as did a correct password with any username. Both comparisons now have to pass, and both are evaluated before either is acted on so the timing does not distinguish a wrong username from a wrong password. They compare SHA-256 digests through `subtle.ConstantTimeCompare`, which keeps the length of the configured credential out of the timing as well.

Fixing the comparison alone does not close the hole. go-smtp advertises `AUTH` but leaves enforcement to the backend, so a client could skip `AUTH` entirely and still have its message forwarded. The session now tracks authentication and refuses `MAIL`, `RCPT` and `DATA` without it.

Nor does the gate mean anything while the credentials it checks can be guessed. `SetDefaults` invented `Username = "username"` and `Password = "password"` when the config omitted them, with only a `slog.Warn` — so a `config.toml` naming nothing but a port and its recipients relayed to anyone who tried the obvious pair, for exactly the deployments that had configured the least. `Validate` now rejects unset credentials the way it already rejects an unusable target, and the invented defaults are gone.

**Behavior change:** a client that used to send without issuing `AUTH` now has to be configured with the credentials from `config.toml`, and a deployment that never set them will not start until it does. Both documented in the README.

## Remote denial of service

A malformed `Content-Type` reached `log.Fatalf`, which calls `os.Exit(1)`. One message with an attacker-controlled header terminated the daemon and took every other in-flight message with it. `Body()` now returns the parse error like the rest of the function does.

## Silent notification loss

`Data` logged forwarding errors and returned `250`, so a client whose notification reached no target at all treated the message as delivered and never retried. The reply now reflects what happened:

- `250` — at least one target accepted it. A *partial* failure is logged but still accepted, since a retry redelivers to every target and would duplicate the notification on the ones that already have it. This keeps the existing `TestPartialTargetFailure` contract.
- `451` — no target accepted it. Worth retrying.
- `550 5.6.0` — the message cannot be parsed. No retry can fix the same bytes.
- `550 5.3.5` — the recipient has no notification targets configured. A configuration fault; no retry can add targets. `LoadConfig` blocks such a config, so this is only reachable from one built in code.

## Session transaction state

`Session.Reset` was empty while go-smtp calls it after every message, so recipients accumulated across a connection and the second message on it was also forwarded to the first one's targets.

Fixing that introduced a data race, caught in review: `Reset` runs on the connection goroutine, but go-smtp runs `Data` on its own goroutine for a `BDAT` transfer and does not block `RSET` during one. A mutex now guards the recipients and the authentication flag across `Rcpt`, `Data` and `Reset`, and `Data` snapshots the recipients up front instead of reading the slice while forwarding.

## Validation

`make format`, `make ci-lint` (0 issues), `make test` (`-race`, 94.2% coverage), `make check` and `make build` all pass locally.

Every regression test was confirmed to fail against the code it guards:

- the six original ones against `origin/main` — the malformed `Content-Type` one by killing the test binary outright, which is the reported DoS;
- `TestResetDuringChunkedTransferIsSynchronized` against `da8e8e4`, where `-race` reports `Session.Reset` racing `Session.Data` on all three runs attempted;
- `TestLoadConfigRejectsUnsetCredentials` against `da8e8e4`'s `config.go`.

## Known gaps, not addressed here

- `Body()` returns `("", nil)` for a `multipart/*` with an unusable boundary and for any media type that is neither `multipart/` nor `text/`, so an empty notification is pushed to every target and answered `250`.
- `forwardEmail` breaks after the first matching `[[Recipients]]` entry, so a message addressed to two matching entries notifies only the first and still answers `250`. Pre-existing.
- A message for an address matching no entry, with no `[CatchAll]`, is accepted with `250` and dropped. Rejecting at `RCPT` would be more honest.
- `smtp.ErrAuthRequired` is go-smtp's `502 5.7.0` where RFC 4954 §6 specifies `530 5.7.0`. Left on the library's own constant.

Closes FMG-3

Co-authored-by: Full-Stack Developer <me@fmartingr.com>
Reviewed-on: #9
Reviewed-by: Felipe M <me@fmartingr.com>
2026-09-07 22:27:04 +02:00

327 lines
9.4 KiB
Go

package smtp2shoutrrr
import (
"crypto/sha256"
"crypto/subtle"
"errors"
"fmt"
"io"
"log/slog"
"net/mail"
"net/url"
"slices"
"strings"
"sync"
"github.com/containrrr/shoutrrr"
"github.com/emersion/go-sasl"
"github.com/emersion/go-smtp"
)
// errNoUsableTargets marks a configuration fault rather than a delivery one:
// the recipient matched, but there is nothing to notify. No retry can add
// targets to a configuration.
var errNoUsableTargets = errors.New("recipient has no usable targets")
var (
// errUnreadableMessage rejects a message for good. Both causes — a
// message that is not parseable as mail, and a Content-Type this server
// cannot make sense of — fail identically on every redelivery.
errUnreadableMessage = &smtp.SMTPError{
Code: 550,
EnhancedCode: smtp.EnhancedCode{5, 6, 0},
Message: "Message could not be parsed",
}
// errMisconfiguredRecipient is permanent for the same reason: the server
// has nowhere to send this recipient's mail until its configuration
// changes, which no amount of redelivery brings about.
errMisconfiguredRecipient = &smtp.SMTPError{
Code: 550,
EnhancedCode: smtp.EnhancedCode{5, 3, 5},
Message: "Recipient has no notification targets configured",
}
// errForwardingFailed asks the sender to try again later. The 250 this
// replaces told the client a message had been delivered when no target
// had received it, so the client dropped a notification nobody ever saw.
errForwardingFailed = &smtp.SMTPError{
Code: 451,
EnhancedCode: smtp.EnhancedCode{4, 3, 0},
Message: "Failed to forward message to notification targets",
}
)
type Backend struct {
config *Config
}
func (bkd *Backend) sendNotifications(recipient ConfigRecipient, email ReceivedEmail) error {
// Get all target URLs (handles merging and caching)
targetURLs := recipient.GetTargetURLs()
if len(targetURLs) == 0 {
return fmt.Errorf("%w: %s", errNoUsableTargets,
strings.Join(recipient.Addresses, ","))
}
// Prepare email body once (reuse for all targets)
body, err := email.Body()
if err != nil {
slog.Error("Error getting email body", slog.String("err", err.Error()))
return fmt.Errorf("failed to get email body: %w", err)
}
slog.Debug("Prepared email body",
slog.Int("body_length", len(body)),
slog.String("subject", email.Msg.Header.Get("Subject")),
slog.String("content_type", email.Msg.Header.Get("Content-Type")))
urlParams := url.Values{
"title": {email.Msg.Header.Get("Subject")},
}
// Send to all targets, collect errors
var errs []error
for i, targetURL := range targetURLs {
// Clone URL to avoid mutation
destinationURL := *targetURL
// Merge new params with existing query params
existingParams := destinationURL.Query()
for key, values := range urlParams {
for _, value := range values {
existingParams.Add(key, value)
}
}
destinationURL.RawQuery = existingParams.Encode()
if err := shoutrrr.Send(destinationURL.String(), body); err != nil {
slog.Error("Error sending message to target",
slog.Int("target_index", i),
slog.String("target", targetURL.Host),
slog.String("err", err.Error()))
errs = append(errs, fmt.Errorf("target %d: %w", i, err))
// Continue to next target even if this one fails
} else {
slog.Info("Successfully sent notification",
slog.String("target", targetURL.Host),
slog.String("recipient", strings.Join(recipient.Addresses, ",")))
}
}
// Only a total failure is reported to the sender. A retry redelivers to
// every target, so pushing back on a partial one would duplicate the
// notification on the targets that already accepted it.
if len(errs) == len(targetURLs) {
return fmt.Errorf("no target accepted the notification: %w", errors.Join(errs...))
}
if len(errs) > 0 {
slog.Warn("notification reached only some targets",
slog.Int("failed", len(errs)),
slog.Int("targets", len(targetURLs)),
slog.String("recipient", strings.Join(recipient.Addresses, ",")))
}
return nil
}
func (bkd *Backend) NewSession(c *smtp.Conn) (smtp.Session, error) {
return &Session{
forwarderFunc: bkd.forwardEmail,
config: bkd.config,
}, nil
}
func (bkd *Backend) forwardEmail(email ReceivedEmail) error {
slog.Info("forwading message", slog.String("to", strings.Join(email.Recipients, ",")))
// Try to match configured recipients first
matched := false
for _, r := range bkd.config.Recipients {
for _, a := range email.Recipients {
if slices.Contains(r.Addresses, a) {
if err := bkd.sendNotifications(r, email); err != nil {
return err
}
matched = true
break
}
}
if matched {
break
}
}
// If no recipient matched and catch-all is configured, use it
if !matched && bkd.config.CatchAll != nil {
slog.Info("using catch-all recipient for unmatched email")
if err := bkd.sendNotifications(*bkd.config.CatchAll, email); err != nil {
return err
}
}
return nil
}
type Session struct {
// mu guards the transaction state below. go-smtp runs Data on its own
// goroutine for a BDAT transfer and keeps serving commands on the
// connection goroutine meanwhile, so an RSET can land in the middle of
// one and reset the session from under it.
mu sync.Mutex
addresses []string
// authenticated gates the mail transaction. go-smtp advertises AUTH but
// never enforces it — that is the backend's job — so without this the
// server relays for anyone who can open a connection.
authenticated bool
config *Config
forwarderFunc func(ReceivedEmail) error
}
func (s *Session) isAuthenticated() bool {
s.mu.Lock()
defer s.mu.Unlock()
return s.authenticated
}
// recipients copies the transaction's recipients so a concurrent Reset cannot
// empty the slice a forward in progress is still reading.
func (s *Session) recipients() []string {
s.mu.Lock()
defer s.mu.Unlock()
return slices.Clone(s.addresses)
}
func (s *Session) AuthMechanisms() []string {
return []string{sasl.Plain}
}
func (s *Session) Auth(mech string) (sasl.Server, error) {
if mech != sasl.Plain {
return nil, smtp.ErrAuthUnknownMechanism
}
return sasl.NewPlainServer(func(identity, username, password string) error {
// PLAIN carries an authorization identity distinct from the
// authenticating one (RFC 4616). There is nobody to act on behalf of
// here, so only the empty and self-referential forms are accepted.
if identity != "" && identity != username {
return smtp.ErrAuthFailed
}
// Both comparisons run before either is acted on: short-circuiting on
// the username would time-distinguish a wrong username from a right
// one with a wrong password, handing an attacker the username.
usernameMatches := credentialMatches(username, s.config.Username)
passwordMatches := credentialMatches(password, s.config.Password)
if !usernameMatches || !passwordMatches {
// Logged without the attempted values: a misconfigured client
// sends its password in the username field often enough that
// recording them turns the log into a credential store.
slog.Warn("rejected authentication attempt")
return smtp.ErrAuthFailed
}
s.mu.Lock()
s.authenticated = true
s.mu.Unlock()
return nil
}), nil
}
// credentialMatches compares a supplied credential against the configured one
// in time independent of how much of it is right. Both sides are hashed first
// because subtle.ConstantTimeCompare returns early when the lengths differ,
// which would still leak the length of the configured credential.
func credentialMatches(supplied, configured string) bool {
suppliedSum := sha256.Sum256([]byte(supplied))
configuredSum := sha256.Sum256([]byte(configured))
return subtle.ConstantTimeCompare(suppliedSum[:], configuredSum[:]) == 1
}
func (s *Session) Mail(from string, opts *smtp.MailOptions) error {
if !s.isAuthenticated() {
return smtp.ErrAuthRequired
}
slog.Debug("Mail from", slog.String("from", from))
return nil
}
func (s *Session) Rcpt(to string, opts *smtp.RcptOptions) error {
if !s.isAuthenticated() {
return smtp.ErrAuthRequired
}
slog.Debug("Rcpt to", slog.String("to", to))
s.mu.Lock()
defer s.mu.Unlock()
s.addresses = append(s.addresses, to)
return nil
}
func (s *Session) Data(r io.Reader) error {
if !s.isAuthenticated() {
return smtp.ErrAuthRequired
}
// Taken before the message is read: an RSET arriving mid-transfer aborts
// the read, and this is the transaction the recipients belong to either
// way.
recipients := s.recipients()
msg, err := mail.ReadMessage(r)
if err != nil {
slog.Error("Error reading data", slog.String("err", err.Error()))
return errUnreadableMessage
}
slog.Info("Received email", slog.String("destination", strings.Join(recipients, ",")))
if err := s.forwarderFunc(ReceivedEmail{
Recipients: recipients,
Msg: msg,
}); err != nil {
slog.Error("Error forwarding email", slog.String("err", err.Error()))
switch {
case errors.Is(err, errMalformedMessage):
return errUnreadableMessage
case errors.Is(err, errNoUsableTargets):
return errMisconfiguredRecipient
default:
return errForwardingFailed
}
}
return nil
}
// Reset ends the current mail transaction. go-smtp calls it after every
// message, so the recipients have to go: leaving them would forward the next
// message on the connection to the previous one's targets as well.
// Authentication deliberately survives, as RFC 5321 scopes RSET to the
// transaction.
func (s *Session) Reset() {
s.mu.Lock()
defer s.mu.Unlock()
s.addresses = nil
}
func (s *Session) Logout() error {
return nil
}