From 23d52bab7c24d7a8cf915c5e7c7cd99b559d5e97 Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Wed, 19 Aug 2026 14:05:25 +0300 Subject: [PATCH] fix: port-25 milter chain, DMARC report validation, auto rate-limit window Review pass over the 1.4.0-1.9.1 work. Port 25 no longer inherits main.cf's submission milter chain: smtpd_milters is always overridden on smtp/inet, to the antispam milter or to nothing. Without that, OpenDKIM (strict, tempfail) gated relayed mail during a signing outage, and the journal-milter filed inbound mail into the outbound send log under the sender's domain -- so Deliveries showed other people's mail and a forged From: consumed that domain's level-2 budget. architecture.md already described the intended behaviour; the code now matches. DMARC reports are checked before they are stored: the policy domain must be a configured sending domain, and must match the +tag of a per-domain hosted address. The rua address form is predictable, so anyone able to reach it could file reports under any domain and evict genuine ones through the retention cap. Zip attachments are now decompressed as well as gzip. The auto level-2 ceiling is scaled to the level-1 window before the multiplier is applied; any window other than 3600s shifted the allowance by window/3600. Migration 0010 indexes send_log(app_login, created_at) -- both the per-MAIL FROM rate-limit count and the per-application statistics were scanning the created_at range. e2e now regenerates the Postfix config in-container for all four port-25 flag combinations and requires postfix check to pass; the 1.9.1 crash-loop shipped in 1.4.0 and survived three releases because only the flags-off path was exercised. Migration 0009 stays as shipped -- its conversion branch only fires for a database carrying application-scope allowed_ips from before 1.9.0. The schema reference notes that the old list was permissive and the new column is restrictive; the guide documents the allow-list as a sending control on a fail-open path, not an authentication boundary. Also: gofmt across cmd/ and internal/, unused duplicate validators dropped from internal/web/validate, export no longer swallows store errors. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 67 ++++++++ build/postfix-config.sh | 11 +- cmd/panel/httpserver.go | 38 ++--- docs/architecture.md | 15 +- docs/guide.md | 34 +++- docs/plans/inbound-antispam-panel.md | 4 +- docs/schema-migrations.md | 27 ++-- internal/dmarc/addresses.go | 18 +++ internal/dmarc/ingest.go | 35 ++++- internal/dmarc/parse.go | 62 ++++++-- internal/dmarc/parse_test.go | 100 ++++++++++++ internal/domain/transfer.go | 42 +++-- internal/postfix/inbound.go | 9 +- internal/store/applications.go | 16 +- internal/store/domains.go | 15 ++ .../0010_send_log_app_login_index.sql | 9 ++ internal/store/ratelimit_auto.go | 20 ++- internal/store/ratelimits_test.go | 41 ++++- internal/store/stats.go | 2 +- .../web/handlers/handlers_dmarc_reports.go | 6 +- internal/web/handlers/handlers_monitor.go | 12 +- .../web/handlers/handlers_ratelimit_test.go | 10 +- internal/web/validate/validate.go | 103 ------------ internal/web/validate/validate_test.go | 66 -------- .../web/view/templates/domain_detail.html | 7 +- internal/web/view/templates/help_drawer.html | 7 +- internal/web/web.go | 32 ++-- test/e2e/main_test.go | 3 + test/e2e/port25_config_check.go | 147 ++++++++++++++++++ 29 files changed, 664 insertions(+), 294 deletions(-) create mode 100644 internal/store/migrations/0010_send_log_app_login_index.sql create mode 100644 test/e2e/port25_config_check.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 733efe6..3fa8e1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,75 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ## [Unreleased] +Review pass over `1.4.0`…`1.9.1`: port 25 no longer shares the submission +milter chain, DMARC reports are validated before they are stored, and the auto +rate limit is correct for a non-default level-1 window. Schema migration +`0010` (index only). Upgrading is a tag bump. + +### Added + +- SQLite migration `0010_send_log_app_login_index.sql` — + `idx_send_log_app_login_created_at`. The journal-milter counts an + application's messages on every `MAIL FROM`, and the domain page computes + per-application statistics on every render; both were scanning the + `created_at` range because `app_login` was unindexed. +- DMARC aggregate reports are accepted as **zip** as well as gzip or plain XML + (several reporters ship a single-entry zip; those reports previously counted + as parse failures). +- e2e: `checkPort25ConfigCombinations` regenerates the Postfix configuration + inside the container for all four combinations of `INBOUND_RELAY_ENABLE` / + `DMARC_REPORTS_ENABLE` / `INBOUND_ANTISPAM_MILTER` and requires + `postfix check` to pass. The `1.9.1` crash-loop shipped in `1.4.0` and + survived three releases because the suite only ever exercised the flags-off + path. + +### Fixed + +- **Port 25 no longer runs the submission milter chain.** `smtpd_milters` is + now always overridden on `smtp/inet` — to the optional antispam milter, or + to nothing. Previously, with `INBOUND_ANTISPAM_MILTER` unset, inbound mail + inherited main.cf's chain: OpenDKIM (strict, `tempfail`) could defer relayed + mail during a signing outage, and the journal-milter filed inbound mail into + the outbound send log under the **sender's** domain — so Deliveries showed + other people's mail, and a forged `From:` on an inbound message consumed that + sending domain's level-2 rate-limit budget. [architecture.md](docs/architecture.md) + already described the intended behaviour; the code now matches it. +- **DMARC reports are validated before they are stored.** A report is accepted + only when the domain in `policy_published` is a sending domain configured + here and, for a per-domain hosted address, matches that address's `+tag`. + The rua address form is predictable, so anyone able to reach it could + previously file reports under any domain, skew the panel's alignment figures + and the tighten-`p=` hint, and evict genuine reports through the 500-report + retention cap. +- Auto level-2 rate limit: the ceiling is now scaled to the level-1 window + before the multiplier is applied. `ceil(avg msg/h × multiplier)` was stored + as the ceiling for a window of `RATE_LIMIT_WINDOW_SECONDS`, so any window + other than the default `3600` shifted the effective allowance by + `window/3600` (a 1800s window allowed twice the intended rate). A domain + that has sent anything now also gets a floor of 1. +- Single-domain export no longer drops rate limits silently when the store + errors — the error is returned instead of being swallowed by `err == nil && ok`. + ### Changed +- [schema-migrations.md](docs/schema-migrations.md) notes that migration `0009` + changed what the IP list *means* — permissive (those IPs got the application + ceiling) before 1.9.0, restrictive (every other IP refused) after. Its + conversion branch only fires for a database that carried application-scope + `allowed_ips` from before 1.9.0. +- Panel help, [guide.md](docs/guide.md) and [architecture.md](docs/architecture.md): + the client IP allow-list is a **sending** control, not an authentication + boundary — the SASL login still succeeds, the check runs in the + journal-milter at `MAIL FROM`, and it is fail-open like the rate limits. + architecture.md's rate-limit description still documented the pre-1.9.0 + trusted-IP semantics. +- Removed unused duplicates of the inbound validators (`Host`, `Port`, + `TLSMode`, `RecipientMode`, `MailboxInDomain`, `NormalizeHost`) from + `internal/web/validate` — the live path uses `internal/inbound/validate.go`; + the copies were reachable only from their own tests. +- `gofmt` across `cmd/` and `internal/` (10 files had drifted); dead branch + removed from `postfix.inboundNexthop`. +- The planned inbound-antispam migration is renumbered `0011_inbound_spam_log.sql`. - Operator and as-built docs aligned with the code after a full pass — [architecture.md](docs/architecture.md) (port 25 opens for inbound relay and/or DMARC ingest; antispam requires inbound relay; DMARC pipe transport diff --git a/build/postfix-config.sh b/build/postfix-config.sh index 0c3075f..c83d767 100644 --- a/build/postfix-config.sh +++ b/build/postfix-config.sh @@ -301,6 +301,13 @@ if [ "${INBOUND_RELAY_ENABLE}" = "true" ] || [ "${DMARC_REPORTS_ENABLE}" = "true "smtp_tls_policy_maps=" fi + # Milters on port 25. main.cf's chain (OpenDKIM strict + journal-milter) is + # for the SUBMISSION path only: OpenDKIM must not gate someone else's + # inbound mail (a signing outage would defer it), and the journal-milter + # would file inbound mail into the outbound send log under the *sender's* + # domain — polluting Deliveries and letting a forged From: consume that + # domain's level-2 rate-limit budget. So smtpd_milters is always overridden + # here: either the optional antispam milter, or nothing at all. INBOUND_MILTERS="" if [ "${INBOUND_RELAY_ENABLE}" = "true" ] && [ -n "${INBOUND_ANTISPAM_MILTER}" ]; then case "${INBOUND_ANTISPAM_MILTER}" in @@ -346,9 +353,7 @@ if [ "${INBOUND_RELAY_ENABLE}" = "true" ] || [ "${DMARC_REPORTS_ENABLE}" = "true "smtp/inet/smtpd_client_message_rate_limit=${PORT25_RATE}" \ "smtp/inet/message_size_limit=${PORT25_SIZE}" master_cf_set_smtp_inet_o smtpd_recipient_restrictions "${RECIPIENT_RESTRICTIONS}" - if [ -n "${INBOUND_MILTERS}" ]; then - master_cf_set_smtp_inet_o smtpd_milters "${INBOUND_MILTERS}" - fi + master_cf_set_smtp_inet_o smtpd_milters "${INBOUND_MILTERS}" else postconf -MX "smtp/inet" 2>/dev/null || true postconf -MX "dmarc-ingest/unix-pipe" 2>/dev/null || true diff --git a/cmd/panel/httpserver.go b/cmd/panel/httpserver.go index f7ac478..35f1d9f 100644 --- a/cmd/panel/httpserver.go +++ b/cmd/panel/httpserver.go @@ -92,25 +92,25 @@ func newPanel(cfg config, st *store.Store) (*web.Server, error) { // snapshot for the process lifetime (architecture.md). retryPolicy := postfix.LoadRetryPolicy() return web.New(st, ms.Domains, ms.Apps, ms.Inbound, ms.DMARC, web.Config{ - Hostname: cfg.hostname, - CookieSecure: cfg.cookieSecure, - SubmissionEnabled: cfg.submissionEnabled, - MailLogPath: cfg.mailLog, - DataDir: cfg.dataDir, - DBPath: cfg.dbPath, - DeployRoot: cfg.deployRoot, - Version: buildinfo.Version, - TrustedProxyCIDRs: cfg.trustedProxies, - TLSCertFile: cfg.tlsCertFile, - OpenDKIMSocket: cfg.opendkimSocket, - JournalSocket: cfg.journalSocket, - SessionIdleDays: cfg.sessionIdleDays, - DNSResolvers: cfg.dnsResolvers, - RateLimitMessagesPerIP: cfg.rateLimitMessagesPerIP, - RateLimitWindowSeconds: cfg.rateLimitWindowSeconds, - RetryPolicy: retryPolicy, - InboundEnabled: cfg.inboundEnabled, - DMARCEnabled: cfg.dmarcEnabled, + Hostname: cfg.hostname, + CookieSecure: cfg.cookieSecure, + SubmissionEnabled: cfg.submissionEnabled, + MailLogPath: cfg.mailLog, + DataDir: cfg.dataDir, + DBPath: cfg.dbPath, + DeployRoot: cfg.deployRoot, + Version: buildinfo.Version, + TrustedProxyCIDRs: cfg.trustedProxies, + TLSCertFile: cfg.tlsCertFile, + OpenDKIMSocket: cfg.opendkimSocket, + JournalSocket: cfg.journalSocket, + SessionIdleDays: cfg.sessionIdleDays, + DNSResolvers: cfg.dnsResolvers, + RateLimitMessagesPerIP: cfg.rateLimitMessagesPerIP, + RateLimitWindowSeconds: cfg.rateLimitWindowSeconds, + RetryPolicy: retryPolicy, + InboundEnabled: cfg.inboundEnabled, + DMARCEnabled: cfg.dmarcEnabled, SendLogRetentionEnvDefault: cfg.retentionDays, }, cfg.setupTokenPath) } diff --git a/docs/architecture.md b/docs/architecture.md index 85377eb..3a99cca 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -73,7 +73,11 @@ smtp/inet` removes Debian's stock smtpd). Outbound delivery still uses the - **587/submission** — only when `SUBMISSION_ENABLE=true`; STARTTLS with `smtpd_tls_security_level=encrypt`. - **25/smtp inet** — when `INBOUND_RELAY_ENABLE=true` and/or - `DMARC_REPORTS_ENABLE=true`. No SASL, no OpenDKIM, no journal-milter. + `DMARC_REPORTS_ENABLE=true`. No SASL, no OpenDKIM, no journal-milter — + `smtpd_milters` is always overridden on this service (to the antispam milter + or to nothing), because main.cf's submission chain would otherwise let a + signing outage defer relayed mail and would file inbound mail into the + outbound send log under the sender's domain. **Inbound relay** (`INBOUND_RELAY_ENABLE=true`): accepts only `relay_domains` + `relay_recipient_maps` (`reject_unauth_destination`, `reject_unlisted_recipient`). Maps under `/data/postfix/` (`relay_domains`, @@ -117,9 +121,12 @@ One process, four roles: 2. **journal-milter** — unix socket `JOURNAL_MILTER_SOCKET`; records From/To/ Subject/SASL user at DATA; enforces level-2 rate limits; **fail-open** (`default_action=accept`) so milter failure does not stop mail. Domain - ceilings apply to every client IP; an application ceiling with trusted IPs - raises the limit for those IPs only and skips the domain check (guide § Rate - limiting). The level-2 count is the stored send-log rows plus the messages + ceilings apply to every client IP; an application ceiling **overrides** the + domain one for that login, higher or lower (guide § Rate limiting). Since + 1.9.0 the client IP allow-list is a separate, restrictive control on the + application (`applications.auth_ip_restrict`), checked here at `MAIL FROM` + and fail-open like the rest of this milter — not an authentication + boundary. The level-2 count is the stored send-log rows plus the messages this process has admitted but not yet written (`internal/milter/inflight.go`), so concurrent sessions cannot each spend the same last slot; a reservation is released at end-of-message, on ABORT, or after a 10-minute TTL. diff --git a/docs/guide.md b/docs/guide.md index 7360d48..d4d2b20 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -729,6 +729,13 @@ only sees SelfPost. Default action is fail-open (`accept`) so a down sidecar does not block backup-MX; set `INBOUND_ANTISPAM_MILTER_ACTION=tempfail` to defer instead. +**Inbound mail is not in Deliveries.** Port 25 runs its own milter chain: the +optional anti-spam milter, and nothing else. OpenDKIM does not sign relayed +mail (and a signing outage therefore cannot defer it), and the journal-milter +does not file it — [Deliveries](#deliveries) stays a record of what *you* sent. +This also keeps a forged `From:` on inbound mail from counting against a +sending domain's [level-2 rate limit](#rate-limiting--level-2-domain-and-application). + Inbound configuration lives in SQLite and `/data/postfix/` map files, so it is included in a [full backup](#full-backup-and-restore). Single-domain export/import is sending domains only. @@ -768,6 +775,15 @@ alone, `check_recipient_access` permits only configured report addresses; everything else is rejected. With inbound relay enabled too, both allow-lists apply. +**Reports are checked before they are stored.** The address form is +predictable, so the report XML is treated as untrusted input: a report is +accepted only when the domain it claims in `policy_published` is a sending +domain configured here, and — when it arrived at a per-domain hosted address — +only when that domain matches the address's `+tag`. Anything else is refused +and counted in *parse failures* on the DMARC page, so a stranger cannot file +reports under your domain or push genuine ones out through the retention cap. +Reports are accepted as gzip, zip, or plain XML. + Parsed report data lives in SQLite and is included in a [full backup](#full-backup-and-restore). @@ -844,9 +860,16 @@ sending as that domain. When unset, only level 1 applies for non-privileged senders. **Client IP allow-list (application)** — optional restriction on an -application: when enabled, list one or more client IPs that may authenticate -and submit mail as that application. When disabled, any client IP is allowed. -This is independent of rate limits. +application: when enabled, list one or more client IPs that may **submit** mail +as that application; a session from any other address is refused with a 4xx at +`MAIL FROM`. When disabled, any client IP is allowed. This is independent of +rate limits, and it takes exact addresses only — CIDR ranges are not parsed. + +It is **not an authentication boundary.** The SASL login itself still succeeds; +the check runs in the journal-milter, on the same fail-open path as level 2 +(below), so if the milter is unavailable the restriction is skipped rather than +enforced. Use it to narrow where an application may send from, not to contain a +leaked password — for that, regenerate the password. **Level 2 — application** — optional override of the domain limit for one application (≤ level 1). The ceiling may be **higher or lower** than the domain @@ -868,7 +891,10 @@ hours and on demand via **Recalculate now**. journal-milter and is deliberately fail-open: if the rate-limit lookup hits a store error, or the connecting client's IP is not available to the milter, level 2 is skipped and the message is accepted rather than held up. -Level 1 is the backstop that keeps working even when level 2 cannot run. +The same applies if the milter itself is down — Postfix is configured to +accept rather than defer when it cannot reach it. Level 1 is the backstop +that keeps working even when level 2 cannot run, and it is the only one of +the two that does not depend on the panel process. ### Deliveries diff --git a/docs/plans/inbound-antispam-panel.md b/docs/plans/inbound-antispam-panel.md index bb2c5da..2100744 100644 --- a/docs/plans/inbound-antispam-panel.md +++ b/docs/plans/inbound-antispam-panel.md @@ -125,7 +125,7 @@ New nav item **Inbound spam** (or subsection under **Inbound**) — journal tabl with filters (decision, domain, date range), list management on the same page or a tab. Mockup: add `docs/assets/panel-ui/inbound_spam.html` in the panel step. -## Schema (migration `0010_inbound_spam_log.sql`) +## Schema (migration `0011_inbound_spam_log.sql`) **`inbound_spam_log`** @@ -238,7 +238,7 @@ Target version cut: **`1.10.0`** (MINOR). One commit per step; [development.md](../development.md) § Plan checklists. - [x] Agree journal fields, retention, and RBAC (this plan § Decisions) — **Composer** -- [ ] Migration `0010_inbound_spam_log.sql` — **Composer** +- [ ] Migration `0011_inbound_spam_log.sql` — **Composer** - [ ] Inbound journal-milter + Postfix wiring — **Composer** - [ ] mail.log tailer: inbound reject rows — **Composer** - [ ] List CRUD + validation + atomic rspamd map sync — **Composer** diff --git a/docs/schema-migrations.md b/docs/schema-migrations.md index f876281..75e6e59 100644 --- a/docs/schema-migrations.md +++ b/docs/schema-migrations.md @@ -18,13 +18,14 @@ Documentation. | Field | Value | |---|---| -| Chain head | `user_version = 9` | -| Files | `0001_init.sql` … `0009_application_auth_ips.sql` (9 files) | +| Chain head | `user_version = 10` | +| Files | `0001_init.sql` … `0010_send_log_app_login_index.sql` (10 files) | | Database file | `/data/selfpost.db` (bind mount) | | Compatibility | 1.x MINOR releases must boot a `1.0.0` data directory | -Last updated with release **1.9.0** (`0009_application_auth_ips`); **1.9.1** -had no schema change. +Last updated in **[Unreleased]** (`0010_send_log_app_login_index`); **1.9.1** +had no schema change. The release column below records the cut a migration +shipped in — fill `0010`'s in at the next version cut. --- @@ -42,6 +43,7 @@ had no schema change. 0002_sessions.sql → user_version 2 … 0009_application_auth_ips.sql → user_version 9 +0010_send_log_app_login_index.sql → user_version 10 ``` **Implication:** never delete, rename, or reorder migration files while 1.x must @@ -63,6 +65,7 @@ replace the embedded set (see [Planned 2.x squash](#planned-2x-squash)). | 7 | `0007_rate_limit_auto.sql` | 1.6.0 | DDL | `rate_limits.mode`, `auto_multiplier`, `auto_updated_at` | | 8 | `0008_dmarc_reports.sql` | 1.7.0 | DDL | `dmarc_reports`, `dmarc_report_records` | | 9 | `0009_application_auth_ips.sql` | 1.9.0 | DDL + data | `applications.auth_ip_restrict`, `auth_allowed_ips`; move legacy app `rate_limits.allowed_ips` into auth columns; clear those IPs on rate limits | +| 10 | `0010_send_log_app_login_index.sql` | Unreleased | DDL | `idx_send_log_app_login_created_at` — app-scoped rate-limit counts and per-application send statistics were scanning the `created_at` range | **Kind:** *DDL* — schema only; *data* — `INSERT`/`UPDATE` that must stay correct for operators upgrading from older 1.x images. @@ -117,11 +120,17 @@ code. A 2.x baseline should define `users` directly and omit `admin`. ### Application trusted IPs (0009) -Before 1.9.0, client IP restriction for an application lived in -`rate_limits.allowed_ips` (`scope = application`). Migration 0009 copies non-empty -values into `applications.auth_allowed_ips`, sets `auth_ip_restrict = 1`, and -clears `rate_limits.allowed_ips` for application scope. Level-2 limits no longer -carry IP bindings; auth and rate limiting are separate concerns. +Before 1.9.0, client IPs for an application lived in `rate_limits.allowed_ips` +(`scope = application`). Migration 0009 copies non-empty values into +`applications.auth_allowed_ips`, sets `auth_ip_restrict = 1`, and clears +`rate_limits.allowed_ips` for application scope. Level-2 limits no longer carry +IP bindings; auth and rate limiting are separate concerns. + +Note that the two columns do not mean the same thing: the old list was +*permissive* (those IPs got the application ceiling, every other IP fell back +to the domain limit), the new one is *restrictive* (every IP not on the list is +refused for that application). The conversion branch only fires for a database +that carried application-scope `allowed_ips` from before 1.9.0. ### Profile DMARC email (0004 → 0005) diff --git a/internal/dmarc/addresses.go b/internal/dmarc/addresses.go index 24a0d08..b3902a2 100644 --- a/internal/dmarc/addresses.go +++ b/internal/dmarc/addresses.go @@ -29,3 +29,21 @@ func IsHostedOnHostname(addr, hostname string) bool { at := strings.LastIndex(addr, "@") return addr[at+1:] == hostname } + +// TaggedDomain returns the domain a hosted report address is tagged for +// (dmarc-reports+@ → ), or "" when addr carries no +// such tag. Used to check a report's claimed domain against the address it +// actually arrived at. +func TaggedDomain(addr string) string { + addr = strings.ToLower(strings.TrimSpace(addr)) + at := strings.LastIndex(addr, "@") + if at <= 0 { + return "" + } + local := addr[:at] + plus := strings.Index(local, "+") + if plus < 0 || local[:plus] != DefaultLocalPart { + return "" + } + return local[plus+1:] +} diff --git a/internal/dmarc/ingest.go b/internal/dmarc/ingest.go index 4296d0a..ece9f5f 100644 --- a/internal/dmarc/ingest.go +++ b/internal/dmarc/ingest.go @@ -18,6 +18,15 @@ import ( // IngestMessage parses a raw RFC 5322 message from r and stores any DMARC // aggregate attachment it finds. +// +// The report XML is unauthenticated input: anyone who can reach an allow-listed +// rua= address can post one, and the address form is predictable. So the domain +// the report claims is checked twice before anything is stored — it must be a +// domain this relay actually sends for, and, when the report arrived at a +// per-domain hosted address (dmarc-reports+@), it must be the +// domain that address is tagged for. Without this a stranger could file reports +// under someone else's domain, skew the panel's alignment figures, and push +// genuine reports out through the retention cap. func IngestMessage(st *store.Store, r io.Reader, recipient string, receivedAt time.Time) error { raw, err := io.ReadAll(io.LimitReader(r, 12<<20)) if err != nil { @@ -31,6 +40,10 @@ func IngestMessage(st *store.Store, r io.Reader, recipient string, receivedAt ti if err != nil { return err } + recipient = strings.ToLower(strings.TrimSpace(recipient)) + if err := checkReportDomain(st, parsed.Domain, recipient); err != nil { + return err + } rep := store.DMARCReport{ Domain: parsed.Domain, Reporter: parsed.Reporter, @@ -46,7 +59,7 @@ func IngestMessage(st *store.Store, r io.Reader, recipient string, receivedAt ti PolicyASPF: parsed.PolicyASPF, PassCount: parsed.PassCount, FailCount: parsed.FailCount, - Recipient: strings.ToLower(strings.TrimSpace(recipient)), + Recipient: recipient, } for _, rec := range parsed.Records { rep.Records = append(rep.Records, store.DMARCReportRecord{ @@ -64,6 +77,26 @@ func IngestMessage(st *store.Store, r io.Reader, recipient string, receivedAt ti return st.PruneDMARCReports() } +// checkReportDomain refuses a report whose policy domain is not one of this +// relay's sending domains, or that contradicts the +tag of the address it was +// delivered to. +func checkReportDomain(st *store.Store, domain, recipient string) error { + if domain == "" { + return fmt.Errorf("dmarc report: no policy domain") + } + if tagged := TaggedDomain(recipient); tagged != "" && tagged != domain { + return fmt.Errorf("dmarc report: domain %q does not match the address it was sent to (%s)", domain, recipient) + } + ok, err := st.DomainExists(domain) + if err != nil { + return err + } + if !ok { + return fmt.Errorf("dmarc report: %q is not a domain configured on this relay", domain) + } + return nil +} + func extractAggregatePayload(raw []byte) ([]byte, error) { msg, err := mail.ReadMessage(bytes.NewReader(raw)) if err != nil { diff --git a/internal/dmarc/parse.go b/internal/dmarc/parse.go index 1cfdc97..080a6e2 100644 --- a/internal/dmarc/parse.go +++ b/internal/dmarc/parse.go @@ -1,6 +1,7 @@ package dmarc import ( + "archive/zip" "bytes" "compress/gzip" "encoding/xml" @@ -73,9 +74,9 @@ type feedbackXML struct { } `xml:"record"` } -// ParseAggregate decodes gzip-compressed or raw DMARC aggregate XML. +// ParseAggregate decodes gzip- or zip-compressed, or raw, DMARC aggregate XML. func ParseAggregate(raw []byte) (ParsedReport, error) { - data, err := maybeGunzip(raw) + data, err := decompress(raw) if err != nil { return ParsedReport{}, err } @@ -128,20 +129,59 @@ func dmarcRecordPasses(r ParsedRecord) bool { return strings.EqualFold(r.SPFResult, "pass") || strings.EqualFold(r.DKIMResult, "pass") } -func maybeGunzip(raw []byte) ([]byte, error) { - if len(raw) >= 2 && raw[0] == 0x1f && raw[1] == 0x8b { - zr, err := gzip.NewReader(bytes.NewReader(raw)) - if err != nil { - return nil, fmt.Errorf("dmarc gzip: %w", err) +// maxAggregateXML bounds the decompressed payload, so a small archive cannot +// expand into an unbounded allocation. +const maxAggregateXML = 8 << 20 + +// decompress unwraps the container the reporter used. Both are seen in the +// wild: most senders gzip the XML, some (notably several Microsoft and Yahoo +// endpoints) ship it as a single-entry zip. +func decompress(raw []byte) ([]byte, error) { + switch { + case len(raw) >= 2 && raw[0] == 0x1f && raw[1] == 0x8b: + return gunzip(raw) + case len(raw) >= 4 && raw[0] == 'P' && raw[1] == 'K' && raw[2] == 0x03 && raw[3] == 0x04: + return unzip(raw) + } + return raw, nil +} + +func gunzip(raw []byte) ([]byte, error) { + zr, err := gzip.NewReader(bytes.NewReader(raw)) + if err != nil { + return nil, fmt.Errorf("dmarc gzip: %w", err) + } + defer zr.Close() + data, err := io.ReadAll(io.LimitReader(zr, maxAggregateXML)) + if err != nil { + return nil, fmt.Errorf("dmarc gzip read: %w", err) + } + return data, nil +} + +// unzip returns the first .xml entry of a zip archive. Directories and any +// other members are skipped rather than trusted. +func unzip(raw []byte) ([]byte, error) { + zr, err := zip.NewReader(bytes.NewReader(raw), int64(len(raw))) + if err != nil { + return nil, fmt.Errorf("dmarc zip: %w", err) + } + for _, f := range zr.File { + if f.FileInfo().IsDir() || !strings.HasSuffix(strings.ToLower(f.Name), ".xml") { + continue } - defer zr.Close() - data, err := io.ReadAll(io.LimitReader(zr, 8<<20)) + rc, err := f.Open() if err != nil { - return nil, fmt.Errorf("dmarc gzip read: %w", err) + return nil, fmt.Errorf("dmarc zip open %s: %w", f.Name, err) + } + data, err := io.ReadAll(io.LimitReader(rc, maxAggregateXML)) + rc.Close() + if err != nil { + return nil, fmt.Errorf("dmarc zip read %s: %w", f.Name, err) } return data, nil } - return raw, nil + return nil, fmt.Errorf("dmarc zip: no xml entry") } // TightenPolicyHint summarises whether raising p= looks reasonable. diff --git a/internal/dmarc/parse_test.go b/internal/dmarc/parse_test.go index 9579d8b..0d46993 100644 --- a/internal/dmarc/parse_test.go +++ b/internal/dmarc/parse_test.go @@ -1,6 +1,7 @@ package dmarc import ( + "archive/zip" "bytes" "compress/gzip" "testing" @@ -76,6 +77,9 @@ func TestIngestMessageStoresReport(t *testing.T) { t.Fatal(err) } defer st.Close() + if _, err := st.AddDomain("example.com", "selfpost"); err != nil { + t.Fatal(err) + } var gz bytes.Buffer zw := gzip.NewWriter(&gz) @@ -114,3 +118,99 @@ func TestHostedReportAddress(t *testing.T) { t.Fatalf("got %q want %q", got, want) } } + +func TestIngestMessageRejectsUnknownDomain(t *testing.T) { + st, err := store.Open(t.TempDir() + "/test.db") + if err != nil { + t.Fatal(err) + } + defer st.Close() + + if err := IngestMessage(st, bytes.NewBufferString(gzipMessage(t, sampleXML)), "dmarc-reports@mail.example.com", time.Now().UTC()); err == nil { + t.Fatal("report for an unconfigured domain was accepted") + } + list, err := st.ListDMARCReports(nil, 10) + if err != nil { + t.Fatal(err) + } + if len(list) != 0 { + t.Fatalf("stored %d report(s) for an unconfigured domain", len(list)) + } +} + +// A report claiming example.com must not be accepted at the address tagged for +// another domain, even when both domains are configured here. +func TestIngestMessageRejectsTagMismatch(t *testing.T) { + st, err := store.Open(t.TempDir() + "/test.db") + if err != nil { + t.Fatal(err) + } + defer st.Close() + if _, err := st.AddDomain("example.com", "selfpost"); err != nil { + t.Fatal(err) + } + if _, err := st.AddDomain("other.example", "selfpost"); err != nil { + t.Fatal(err) + } + + err = IngestMessage(st, bytes.NewBufferString(gzipMessage(t, sampleXML)), + "dmarc-reports+other.example@mail.example.com", time.Now().UTC()) + if err == nil { + t.Fatal("report was accepted at an address tagged for another domain") + } +} + +func TestTaggedDomain(t *testing.T) { + cases := map[string]string{ + "dmarc-reports+example.com@mail.example.com": "example.com", + "dmarc-reports@mail.example.com": "", + "postmaster+example.com@mail.example.com": "", + "not-an-address": "", + } + for addr, want := range cases { + if got := TaggedDomain(addr); got != want { + t.Errorf("TaggedDomain(%q) = %q, want %q", addr, got, want) + } + } +} + +func TestParseAggregateFromZip(t *testing.T) { + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + w, err := zw.Create("report.xml") + if err != nil { + t.Fatal(err) + } + if _, err := w.Write([]byte(sampleXML)); err != nil { + t.Fatal(err) + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + + got, err := ParseAggregate(buf.Bytes()) + if err != nil { + t.Fatalf("ParseAggregate(zip): %v", err) + } + if got.Domain != "example.com" || got.PassCount != 10 { + t.Fatalf("zip report = %+v", got) + } +} + +// gzipMessage wraps xml in the single-part gzip message the ingest pipe sees. +func gzipMessage(t *testing.T, xml string) string { + t.Helper() + var gz bytes.Buffer + zw := gzip.NewWriter(&gz) + if _, err := zw.Write([]byte(xml)); err != nil { + t.Fatal(err) + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + return "From: noreply@google.com\r\n" + + "To: dmarc-reports@mail.example.com\r\n" + + "Subject: Report\r\n" + + "Content-Type: application/gzip; name=\"report.xml.gz\"\r\n" + + "\r\n" + gz.String() +} diff --git a/internal/domain/transfer.go b/internal/domain/transfer.go index 36dde8f..a3b9d5f 100644 --- a/internal/domain/transfer.go +++ b/internal/domain/transfer.go @@ -19,14 +19,14 @@ const FormatDomainExport = "selfpost-domain-export" // transfer without regeneration). The file is therefore as sensitive as a full // backup and must be handled as a secret. type DomainExport struct { - Format string `json:"format"` - Version string `json:"version"` - Domain string `json:"domain"` - DKIMSelector string `json:"dkim_selector"` - DKIMPrivateKey string `json:"dkim_private_key"` // PKCS#1 PEM - DMARCRua *string `json:"dmarc_rua,omitempty"` // nil = inherit profile; set = override ("" = none) + Format string `json:"format"` + Version string `json:"version"` + Domain string `json:"domain"` + DKIMSelector string `json:"dkim_selector"` + DKIMPrivateKey string `json:"dkim_private_key"` // PKCS#1 PEM + DMARCRua *string `json:"dmarc_rua,omitempty"` // nil = inherit profile; set = override ("" = none) RateLimit *RateLimitExport `json:"rate_limit,omitempty"` - Applications []AppExport `json:"applications"` + Applications []AppExport `json:"applications"` } // RateLimitExport is the transferable level-2 limit for a domain or application. @@ -40,13 +40,13 @@ type RateLimitExport struct { // AppExport is one application within a DomainExport. type AppExport struct { - Login string `json:"login"` - AddressMode string `json:"address_mode"` - Addresses []string `json:"addresses,omitempty"` // list mode only - Password string `json:"password"` - AuthIPRestrict bool `json:"auth_ip_restrict,omitempty"` - AuthAllowedIPs []string `json:"auth_allowed_ips,omitempty"` - RateLimit *RateLimitExport `json:"rate_limit,omitempty"` + Login string `json:"login"` + AddressMode string `json:"address_mode"` + Addresses []string `json:"addresses,omitempty"` // list mode only + Password string `json:"password"` + AuthIPRestrict bool `json:"auth_ip_restrict,omitempty"` + AuthAllowedIPs []string `json:"auth_allowed_ips,omitempty"` + RateLimit *RateLimitExport `json:"rate_limit,omitempty"` } // Export builds the transferable representation of a domain: its DKIM key, its @@ -78,7 +78,11 @@ func (s *Service) Export(id int64) (DomainExport, error) { s := d.DMARCRua.String exp.DMARCRua = &s } - if rl, ok, err := s.store.GetRateLimit(store.RateLimitScopeDomain, id); err == nil && ok { + rl, ok, err := s.store.GetRateLimit(store.RateLimitScopeDomain, id) + if err != nil { + return DomainExport{}, fmt.Errorf("export rate limit for %s: %w", d.Name, err) + } + if ok { exp.RateLimit = exportRateLimit(rl) } for _, a := range apps { @@ -94,8 +98,12 @@ func (s *Service) Export(id int64) (DomainExport, error) { AuthIPRestrict: a.AuthIPRestrict, AuthAllowedIPs: a.AuthAllowedIPs, } - if rl, ok, err := s.store.GetRateLimit(store.RateLimitScopeApp, a.ID); err == nil && ok { - appExp.RateLimit = exportRateLimit(rl) + appRL, ok, err := s.store.GetRateLimit(store.RateLimitScopeApp, a.ID) + if err != nil { + return DomainExport{}, fmt.Errorf("export rate limit for %s: %w", a.Login, err) + } + if ok { + appExp.RateLimit = exportRateLimit(appRL) } exp.Applications = append(exp.Applications, appExp) } diff --git a/internal/postfix/inbound.go b/internal/postfix/inbound.go index 680f2ac..5b833d3 100644 --- a/internal/postfix/inbound.go +++ b/internal/postfix/inbound.go @@ -2,7 +2,6 @@ package postfix import ( "fmt" - "net" "path/filepath" "sort" "strconv" @@ -83,12 +82,10 @@ func renderInboundMaps(routes []InboundRoute) (relay, transport, recipients, tls return []byte(relayB.String()), []byte(transportB.String()), []byte(recipB.String()), []byte(tlsB.String()), nil } -// inboundNexthop is the Postfix next-hop [host]:port form that disables MX -// lookup for the explicit upstream. +// inboundNexthop is the Postfix next-hop [host]:port form. The brackets are +// what disable the MX lookup, so the operator's explicit upstream is used as +// given; they also delimit a bare IPv6 address. func inboundNexthop(host string, port int) string { - if ip := net.ParseIP(host); ip != nil && ip.To4() == nil { - return "[" + host + "]:" + strconv.Itoa(port) - } return "[" + host + "]:" + strconv.Itoa(port) } diff --git a/internal/store/applications.go b/internal/store/applications.go index 6a94b5c..c7f7e56 100644 --- a/internal/store/applications.go +++ b/internal/store/applications.go @@ -29,14 +29,14 @@ const ( // hashed — so it can be shown exactly once at creation/regeneration // (security.md). Addresses is populated only in 'list' mode. type Application struct { - ID int64 - DomainID int64 - Login string - AddressMode string - CreatedAt time.Time - Addresses []string - AuthIPRestrict bool // when true, only AuthAllowedIPs may submit as this login - AuthAllowedIPs []string // client IPs permitted when AuthIPRestrict is set + ID int64 + DomainID int64 + Login string + AddressMode string + CreatedAt time.Time + Addresses []string + AuthIPRestrict bool // when true, only AuthAllowedIPs may submit as this login + AuthAllowedIPs []string // client IPs permitted when AuthIPRestrict is set } // AllowsAuthFromIP reports whether a client at ip may submit mail authenticated diff --git a/internal/store/domains.go b/internal/store/domains.go index 0c28e2a..6983f95 100644 --- a/internal/store/domains.go +++ b/internal/store/domains.go @@ -4,6 +4,7 @@ import ( "database/sql" "errors" "fmt" + "strings" "time" "modernc.org/sqlite" @@ -182,3 +183,17 @@ func (s *Store) UpdateDomainDMARCRua(id int64, rua sql.NullString) error { } return nil } + +// DomainExists reports whether a sending domain with this name is configured. +// Used by the DMARC ingest path to refuse reports for domains this relay does +// not send for (security.md: report XML is attacker-supplied input). +func (s *Store) DomainExists(name string) (bool, error) { + var n int + err := s.db.QueryRow( + "SELECT COUNT(*) FROM domains WHERE name = ? COLLATE NOCASE", strings.TrimSpace(name), + ).Scan(&n) + if err != nil { + return false, fmt.Errorf("domain exists %q: %w", name, err) + } + return n > 0, nil +} diff --git a/internal/store/migrations/0010_send_log_app_login_index.sql b/internal/store/migrations/0010_send_log_app_login_index.sql new file mode 100644 index 0000000..35626ac --- /dev/null +++ b/internal/store/migrations/0010_send_log_app_login_index.sql @@ -0,0 +1,9 @@ +-- Send-log lookups by application login run on two hot paths: the journal +-- milter counts messages for a level-2 rate limit on every MAIL FROM, and the +-- domain page computes 30-day statistics per application on every render. +-- 0001 indexed queue_id, domain and created_at, but not app_login, so both +-- fell back to scanning the created_at range. The composite matches the shape +-- of those queries (equality on app_login, range on created_at). + +CREATE INDEX IF NOT EXISTS idx_send_log_app_login_created_at + ON send_log (app_login, created_at); diff --git a/internal/store/ratelimit_auto.go b/internal/store/ratelimit_auto.go index 92e8f67..7820e3a 100644 --- a/internal/store/ratelimit_auto.go +++ b/internal/store/ratelimit_auto.go @@ -116,7 +116,7 @@ func (s *Store) recalcAutoRateLimit(rl RateLimit, retentionDays, l1Max, l1Window return fmt.Errorf("unknown scope %q", rl.Scope) } - maxMsgs := computeAutoMaxMessages(stats, mult, l1Max) + maxMsgs := computeAutoMaxMessages(stats, mult, l1Max, l1Window) rl.MaxMessages = maxMsgs rl.WindowSeconds = l1Window @@ -128,14 +128,26 @@ func (s *Store) recalcAutoRateLimit(rl RateLimit, retentionDays, l1Max, l1Window return s.SetRateLimit(rl) } -func computeAutoMaxMessages(stats SendStats, multiplier float64, l1Max int) int { +// computeAutoMaxMessages turns an average send rate into a ceiling for one +// level-1 window. AvgPerHour is per hour, the ceiling counts messages per +// windowSeconds, so the rate is scaled to the window before the multiplier is +// applied — otherwise a window other than the default 3600s would silently +// shift the effective allowance by windowSeconds/3600. The result is never +// below 1 for a domain that has sent anything, and never above level 1. +func computeAutoMaxMessages(stats SendStats, multiplier float64, l1Max, windowSeconds int) int { if stats.Total == 0 { return 0 } - max := int(math.Ceil(stats.AvgPerHour * multiplier)) + if windowSeconds <= 0 { + windowSeconds = 3600 + } + perWindow := stats.AvgPerHour * float64(windowSeconds) / 3600 + max := int(math.Ceil(perWindow * multiplier)) + if max < 1 { + max = 1 + } if max > l1Max { max = l1Max } return max } - diff --git a/internal/store/ratelimits_test.go b/internal/store/ratelimits_test.go index f562c7c..cabecba 100644 --- a/internal/store/ratelimits_test.go +++ b/internal/store/ratelimits_test.go @@ -154,8 +154,8 @@ func TestRateLimitActive(t *testing.T) { inactive := []RateLimit{ {}, {Scope: RateLimitScopeDomain, AllowedIPs: []string{"203.0.113.1"}}, // no ceiling - {Scope: RateLimitScopeDomain, MaxMessages: 5}, // no window - {Scope: RateLimitScopeApp, MaxMessages: 5}, // no window + {Scope: RateLimitScopeDomain, MaxMessages: 5}, // no window + {Scope: RateLimitScopeApp, MaxMessages: 5}, // no window } for i, rl := range inactive { if rl.Active() { @@ -261,3 +261,40 @@ func TestAutoRateLimitZeroTrafficInactive(t *testing.T) { t.Fatalf("zero traffic auto should be inactive: %+v", rl) } } + +// The auto ceiling counts messages per level-1 window, so the per-hour average +// must be scaled to that window: half an hour of traffic, not a full hour's. +func TestComputeAutoMaxMessagesScalesToWindow(t *testing.T) { + stats := SendStats{Total: 100, AvgPerHour: 10} + cases := []struct { + name string + windowSeconds int + want int + }{ + {"one hour", 3600, 20}, + {"half an hour", 1800, 10}, + {"two hours", 7200, 40}, + {"unset falls back to an hour", 0, 20}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := computeAutoMaxMessages(stats, 2.0, 1000, tc.windowSeconds) + if got != tc.want { + t.Fatalf("max = %d, want %d", got, tc.want) + } + }) + } +} + +func TestComputeAutoMaxMessagesBounds(t *testing.T) { + if got := computeAutoMaxMessages(SendStats{}, 2.0, 1000, 3600); got != 0 { + t.Fatalf("no traffic: max = %d, want 0 (inactive)", got) + } + // A trickle still has to leave room for one message. + if got := computeAutoMaxMessages(SendStats{Total: 1, AvgPerHour: 0.001}, 1.0, 1000, 3600); got != 1 { + t.Fatalf("trickle: max = %d, want 1", got) + } + if got := computeAutoMaxMessages(SendStats{Total: 100, AvgPerHour: 500}, 2.0, 100, 3600); got != 100 { + t.Fatalf("capped: max = %d, want the level-1 ceiling 100", got) + } +} diff --git a/internal/store/stats.go b/internal/store/stats.go index 9e97f09..2fecdd3 100644 --- a/internal/store/stats.go +++ b/internal/store/stats.go @@ -15,7 +15,7 @@ type SendStats struct { Total int64 PeakPerHour int64 AvgPerHour float64 - WindowDays int // actual days queried (min(30, retention)) + WindowDays int // actual days queried (min(30, retention)) WindowHours float64 // hours used for the average-rate denominator } diff --git a/internal/web/handlers/handlers_dmarc_reports.go b/internal/web/handlers/handlers_dmarc_reports.go index 41f564a..6b35cd7 100644 --- a/internal/web/handlers/handlers_dmarc_reports.go +++ b/internal/web/handlers/handlers_dmarc_reports.go @@ -14,9 +14,9 @@ import ( type dmarcListRow struct { store.DMARCReportSummary - DomainID int64 - ReceivedLabel string - PeriodLabel string + DomainID int64 + ReceivedLabel string + PeriodLabel string } func (h *Handlers) requireDMARC(w http.ResponseWriter, r *http.Request) (auth.Principal, bool) { diff --git a/internal/web/handlers/handlers_monitor.go b/internal/web/handlers/handlers_monitor.go index 4292d79..b3ce9f2 100644 --- a/internal/web/handlers/handlers_monitor.go +++ b/internal/web/handlers/handlers_monitor.go @@ -108,12 +108,12 @@ func (h *Handlers) HandleDelivery(w http.ResponseWriter, r *http.Request) { row.Subject = mailhdr.DecodeSubject(row.Subject) logRows, logNote := h.deliveryLog(row) h.view.Render(w, http.StatusOK, "delivery", map[string]any{ - "Title": "SelfPost — delivery", - "User": auth.CurrentUser(r), - "Active": "deliveries", - "IsGlobal": p.IsGlobal(), - "SendLogRetentionDays": h.sendLogRetentionDays(), - "Row": row, + "Title": "SelfPost — delivery", + "User": auth.CurrentUser(r), + "Active": "deliveries", + "IsGlobal": p.IsGlobal(), + "SendLogRetentionDays": h.sendLogRetentionDays(), + "Row": row, // The status in the panel's own badge vocabulary, so the headline reads // the same way as every other health signal in the panel. "Level": deliveryLevel(row.Status), diff --git a/internal/web/handlers/handlers_ratelimit_test.go b/internal/web/handlers/handlers_ratelimit_test.go index d094e75..4caaa51 100644 --- a/internal/web/handlers/handlers_ratelimit_test.go +++ b/internal/web/handlers/handlers_ratelimit_test.go @@ -82,7 +82,7 @@ func TestParseAppAuthIPsForm(t *testing.T) { restrict, ips, err := parseAppAuthIPsForm(form(url.Values{ "auth_ip_restrict": {"1"}, - "auth_allowed_ips": {"203.0.113.10"}, + "auth_allowed_ips": {"203.0.113.10"}, })) if err != nil || !restrict || len(ips) != 1 { t.Fatalf("enabled with IP = restrict=%v ips=%v err=%v", restrict, ips, err) @@ -107,15 +107,15 @@ func TestParseDomainRateLimitFormAuto(t *testing.T) { return r } in, err := parseDomainRateLimitForm(form(url.Values{ - "mode": {"auto"}, - "auto_multiplier": {"2.5"}, + "mode": {"auto"}, + "auto_multiplier": {"2.5"}, }), 100) if err != nil || in.mode != "auto" || in.autoMultiplier != 2.5 { t.Fatalf("auto domain = %+v err=%v", in, err) } _, err = parseDomainRateLimitForm(form(url.Values{ - "mode": {"auto"}, - "auto_multiplier": {"10"}, + "mode": {"auto"}, + "auto_multiplier": {"10"}, }), 100) if err == nil { t.Fatal("multiplier out of range should fail") diff --git a/internal/web/validate/validate.go b/internal/web/validate/validate.go index da8b72d..6ba5cde 100644 --- a/internal/web/validate/validate.go +++ b/internal/web/validate/validate.go @@ -3,8 +3,6 @@ package validate import ( "fmt" - "net" - "strconv" "strings" "unicode" ) @@ -148,104 +146,3 @@ func Email(addr string) error { } return nil } - -const maxHostLen = 253 - -// NormalizeHost trims, lower-cases, and strips wrapping IPv6 brackets so the -// stored value is a bare hostname or IP, safe to wrap again when writing maps. -func NormalizeHost(host string) string { - host = strings.ToLower(strings.TrimSpace(host)) - if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { - host = host[1 : len(host)-1] - } - return host -} - -// Host enforces a whitelist for an upstream hostname or IP (security.md): a -// dotted domain, a single DNS label (LAN names), or an IPv4/IPv6 address. -func Host(host string) error { - if host == "" { - return fmt.Errorf("host is required") - } - if len(host) > maxHostLen { - return fmt.Errorf("host must be at most %d characters", maxHostLen) - } - if ip := net.ParseIP(host); ip != nil { - return nil - } - labels := strings.Split(host, ".") - for _, label := range labels { - if err := domainLabel(label); err != nil { - return fmt.Errorf("host is invalid: %w", err) - } - } - return nil -} - -// Port checks a TCP port number parsed from form input. -func Port(raw string) (int, error) { - raw = strings.TrimSpace(raw) - if raw == "" { - return 0, fmt.Errorf("port is required") - } - n, err := strconv.Atoi(raw) - if err != nil || n < 1 || n > 65535 { - return 0, fmt.Errorf("port must be between 1 and 65535") - } - return n, nil -} - -// TLSMode checks a Postfix smtp_tls_policy_maps level. -func TLSMode(mode string) error { - switch mode { - case "may", "encrypt", "none": - return nil - default: - return fmt.Errorf("invalid TLS mode") - } -} - -// RecipientMode checks an inbound-domain recipient policy. -func RecipientMode(mode string) error { - switch mode { - case "list", "any": - return nil - default: - return fmt.Errorf("invalid recipient mode") - } -} - -// MailboxInDomain checks that addr is a conservative mailbox on domain -// (security.md). domain must already be normalised. -func MailboxInDomain(addr, domain string) error { - at := strings.LastIndexByte(addr, '@') - if at <= 0 || at >= len(addr)-1 { - return fmt.Errorf("%q is not a valid email address", addr) - } - local, host := addr[:at], addr[at+1:] - if host != domain { - return fmt.Errorf("%q does not belong to domain %s", addr, domain) - } - if err := mailboxLocalPart(local); err != nil { - return fmt.Errorf("%q: %w", addr, err) - } - return nil -} - -func mailboxLocalPart(local string) error { - if local == "" { - return fmt.Errorf("missing the part before '@'") - } - if local[0] == '.' || local[len(local)-1] == '.' { - return fmt.Errorf("local part must not start or end with '.'") - } - for i := 0; i < len(local); i++ { - c := local[i] - lower := c >= 'a' && c <= 'z' - digit := c >= '0' && c <= '9' - if !lower && !digit && c != '.' && c != '-' && c != '_' && c != '+' { - return fmt.Errorf("local part may contain only lower-case letters, digits, '.', '-', '_' and '+'") - } - } - return nil -} diff --git a/internal/web/validate/validate_test.go b/internal/web/validate/validate_test.go index cbd3cd8..6a20bf4 100644 --- a/internal/web/validate/validate_test.go +++ b/internal/web/validate/validate_test.go @@ -65,69 +65,3 @@ func TestValidateDomainLongLabelRejected(t *testing.T) { t.Error("expected error for over-long label") } } - -func TestHostValid(t *testing.T) { - valid := []string{ - "10.0.0.8", - "192.0.2.20", - "2001:db8::1", - "mail.internal.example", - "mx1", - "mail-1.lan", - } - for _, h := range valid { - if err := Host(h); err != nil { - t.Errorf("Host(%q) unexpected error: %v", h, err) - } - } -} - -func TestHostInvalid(t *testing.T) { - invalid := []string{ - "", - "exa mple", - "host/name", - "host;rm", - "-bad", - "bad-", - "host\nname", - } - for _, h := range invalid { - if err := Host(h); err == nil { - t.Errorf("Host(%q) = nil, want error", h) - } - } -} - -func TestNormalizeHostStripsIPv6Brackets(t *testing.T) { - if got := NormalizeHost(" [2001:DB8::1] "); got != "2001:db8::1" { - t.Errorf("NormalizeHost IPv6 = %q", got) - } - if got := NormalizeHost("Mail.Example.COM"); got != "mail.example.com" { - t.Errorf("NormalizeHost hostname = %q", got) - } -} - -func TestPort(t *testing.T) { - n, err := Port("25") - if err != nil || n != 25 { - t.Fatalf("Port(25) = %d, %v", n, err) - } - for _, raw := range []string{"", "0", "65536", "abc", "-1"} { - if _, err := Port(raw); err == nil { - t.Errorf("Port(%q) = nil, want error", raw) - } - } -} - -func TestMailboxInDomain(t *testing.T) { - if err := MailboxInDomain("staff@lists.example.com", "lists.example.com"); err != nil { - t.Fatal(err) - } - if err := MailboxInDomain("staff@other.com", "lists.example.com"); err == nil { - t.Fatal("expected domain mismatch error") - } - if err := MailboxInDomain("bad addr@lists.example.com", "lists.example.com"); err == nil { - t.Fatal("expected local-part error") - } -} diff --git a/internal/web/view/templates/domain_detail.html b/internal/web/view/templates/domain_detail.html index d434e34..fae1611 100644 --- a/internal/web/view/templates/domain_detail.html +++ b/internal/web/view/templates/domain_detail.html @@ -330,8 +330,11 @@

Client IP allow-list {{if .AuthIPRestrict}}active{{else}}off{{end}}

-

When enabled, only these addresses may authenticate - and submit mail as this application. When off, any client IP is allowed.

+

When enabled, only these addresses may submit mail as + this application; a login from anywhere else is refused with a 4xx at + MAIL FROM. When off, any client IP is allowed. Like the + level-2 rate limit this runs in the journal-milter and is fail-open — + it is a sending control, not an authentication boundary.