Some checks failed
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>
327 lines
9.4 KiB
Go
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
|
|
}
|