smtp2shoutrrr/config.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

155 lines
4.2 KiB
Go

package smtp2shoutrrr
import (
"bytes"
"errors"
"fmt"
"log/slog"
"net/url"
"os"
"strings"
"github.com/pelletier/go-toml/v2"
)
// LoadConfig reads the TOML configuration at path, applies its defaults and
// checks that it can actually deliver something.
func LoadConfig(path string) (*Config, error) {
raw, err := os.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("reading config file %q: %w", path, err)
}
var config Config
if err := toml.Unmarshal(raw, &config); err != nil {
return nil, fmt.Errorf("decoding config file %q: %w", path, err)
}
warnUnknownKeys(path, raw)
config.SetDefaults()
if err := config.Validate(); err != nil {
return nil, fmt.Errorf("invalid config file %q: %w", path, err)
}
return &config, nil
}
// warnUnknownKeys reports keys the decode silently ignored. Rejecting them
// would break configurations that already carry a stray key, but dropping them
// without a word hides the mistypes ([[Recipient]], Adresses) that otherwise
// produce a server which accepts mail and forwards none of it.
func warnUnknownKeys(path string, raw []byte) {
var discard Config
var strict *toml.StrictMissingError
if !errors.As(toml.NewDecoder(bytes.NewReader(raw)).DisallowUnknownFields().Decode(&discard), &strict) {
return
}
for i := range strict.Errors {
slog.Warn("ignoring unknown key in config file",
slog.String("path", path),
slog.String("key", strings.Join(strict.Errors[i].Key(), ".")))
}
}
type Config struct {
Port int
Username string
Password string
Recipients []ConfigRecipient
CatchAll *ConfigRecipient
}
func (c *Config) SetDefaults() {
if c.Port == 0 {
c.Port = 11125
}
}
// Validate rejects configuration that would leave the server relaying for
// anyone, or accepting mail it can never forward. A TOML decode ignores
// unknown keys, so a mistyped table or field name ([[Recipient]], Adresses)
// otherwise yields a server that answers 250 OK to everything and sends
// nothing.
func (c *Config) Validate() error {
// Credentials used to fall back to username/password when unset, which
// left the AUTH gate open to the most obvious guess there is — for
// exactly the deployments that had configured the least.
if c.Username == "" {
return errors.New("no Username configured")
}
if c.Password == "" {
return errors.New("no Password configured")
}
for i := range c.Recipients {
r := &c.Recipients[i]
if len(r.Addresses) == 0 {
return fmt.Errorf("recipient %d has no Addresses", i)
}
if len(r.GetTargetURLs()) == 0 {
return fmt.Errorf("recipient %d (%s) has no usable Targets",
i, strings.Join(r.Addresses, ","))
}
}
if c.CatchAll != nil && len(c.CatchAll.GetTargetURLs()) == 0 {
return errors.New("CatchAll has no usable Targets")
}
if len(c.Recipients) == 0 && c.CatchAll == nil {
return errors.New("no Recipients and no CatchAll configured")
}
return nil
}
type ConfigRecipient struct {
Addresses []string // email addresses
Target string // deprecated: use Targets instead
Targets []string // shoutrrr addresses (supports multiple)
targetURLs []*url.URL // cached parsed URLs
}
func (cr *ConfigRecipient) GetTargetURLs() []*url.URL {
if cr.targetURLs == nil {
cr.targetURLs = make([]*url.URL, 0)
// Collect all targets, merging Target into Targets
allTargets := make([]string, 0)
// Handle deprecated Target field
if cr.Target != "" {
allTargets = append(allTargets, cr.Target)
slog.Warn("Target field is deprecated, use Targets instead",
slog.String("addresses", strings.Join(cr.Addresses, ",")))
}
// Add all Targets
allTargets = append(allTargets, cr.Targets...)
// Parse and cache all URLs
for _, target := range allTargets {
parsedURL, err := url.Parse(target)
if err != nil {
slog.Error("failed to parse shoutrrr target URL",
slog.String("target", target),
slog.String("err", err.Error()))
continue // Skip invalid URLs
}
// url.Parse accepts almost any string, so the scheme is what
// actually distinguishes a shoutrrr target from a typo.
if parsedURL.Scheme == "" {
slog.Error("shoutrrr target URL has no scheme",
slog.String("target", target))
continue
}
cr.targetURLs = append(cr.targetURLs, parsedURL)
}
}
return cr.targetURLs
}