diff --git a/CHANGELOG.md b/CHANGELOG.md index fd051a3..8580d02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,47 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ## [Unreleased] +### Security + +- **Postfix and OpenDKIM could stop every program in the container.** The + supervisord control socket was open to the group the two share with the + panel for files under `/data`; it is now a group of its own, the panel's + alone (audit 2026-10-12, M17). +- **Postfix accepted TLS 1.0, answered VRFY and named its distribution.** + It takes TLS 1.2 at least, opportunistic or required, and asks it of an + inbound domain's upstream where TLS is required; VRFY no longer says + which recipients port 25 accepts; the banner is ` ESMTP` + (audit 2026-10-12, L31). +- **A From: added on the way could sit above the signed one.** OpenDKIM now + signs From: once more than it is present, so an added one breaks the + signature, and canonicalises the header fields relaxed, so a relay that + refolds them does not (audit 2026-10-12, L30). + +### Fixed + +- **OpenDKIM's errors went nowhere.** It logs through syslog(3) alone, and + nothing in the container listened on `/dev/log`. A small relay program + (the panel's binary, `-syslog-relay`) passes those lines to the container + log, marked `syslog:`, errors as such, at most 200 a second (audit + 2026-10-12, L30). +- **A non-numeric rate limit, size limit or milter timeout killed every + smtpd** at its first connection after `postfix check` had passed it, and a + zero window or timeout the rate limiter or smtpd. The start now stops with + the variable's name and what it must be (audit 2026-10-12, L31). +- **`mail.log` was never rotated on a host that recreates the container + daily.** logrotate kept its state inside the container, so each new + container took the log for rotated today. The state is now under + `/data/log` (audit 2026-10-12, M19). +- **A program that crashed after its first second was restarted for ever.** + supervisord gives up only on crashes during start-up; the container is now + restarted when a program of the mail path exits unexpectedly five times in + ten minutes, as it is when it gives up (audit 2026-10-12, L32). +- **`deploy/traefik/extract-cert.sh` could replace the certificate with two + empty files** when `acme.json` did not hold the domain, and exited 0. It + now refuses anything that is not PEM, writes the new files beside the old + ones and renames them whole, owner-only until then, with the group the + old files had (audit 2026-10-12, M21). It needs bash. + ## [2.5.4] - 2026-10-15 Stage 4 of the 2026-10-12 audit: the panel's behaviour. Signing in tells diff --git a/build/crashexit.py b/build/crashexit.py index b0f1a33..23db510 100644 --- a/build/crashexit.py +++ b/build/crashexit.py @@ -2,32 +2,51 @@ """Supervisor event listener: bring the container down on unrecoverable failure. If a managed program of the mail path exhausts its restart retries and -enters FATAL, this listener signals supervisord (its parent) to terminate, so -the whole container exits and Docker's `restart` policy starts it again — -rather than lingering "alive" with a dead Postfix/OpenDKIM/panel. logrotate is -not on the mail path: its FATAL (a config it would ignore) stays on -Server > Health, and mail keeps flowing (assumptions C-V5). +enters FATAL, or keeps crashing - CRASH_LIMIT unexpected exits within +CRASH_WINDOW seconds - this listener signals supervisord (its parent) to +terminate, so the whole container exits and Docker's `restart` policy starts it +again, rather than lingering "alive" with a dead Postfix/OpenDKIM/panel. +FATAL alone did not cover the second: a program reaches FATAL only through +exits before its startsecs, and one that crashed after it was restarted for +ever (audit 2026-10-12, L32; supervisor 4.2.5, supervisor/process.py, +Subprocess.finish()). logrotate is not on the mail path: its FATAL (a config +it would ignore) stays on Server > Health, and mail keeps flowing +(assumptions C-V5). Communication with supervisord uses the event listener protocol over stdin and stdout, so stdout must carry only protocol tokens. supervisor 4.2.5, docs/events.rst, "Event Listener Notification Protocol": the listener writes READY\\n, reads one header line of key:value tokens whose len -is the payload's length, reads the payload, and answers "RESULT 2\\nOK". -A PROCESS_STATE_FATAL payload is a token set, "processname:cat groupname:cat -from_state:BACKOFF" (docs/events.rst; supervisor/events.py, -ProcessStateEvent.payload()). +is the payload's length and eventname the event's type, reads the payload, and +answers "RESULT 2\\nOK". A PROCESS_STATE_FATAL payload is a token set, +"processname:cat groupname:cat from_state:BACKOFF"; a PROCESS_STATE_EXITED one +adds "expected:0" for an exit code the program's exitcodes do not list, "1" +otherwise (docs/events.rst; supervisor/events.py, ProcessStateEvent.payload()). supervisord starts the listener as its own child, and on SIGTERM it shuts down (docs/running.rst, "Signals"); it is PID 1 here (build/entrypoint.sh, exec). + +CRASHEXIT_DRYRUN=1 writes "crashexit: would shut down" instead of signalling, +for the test (build/crashexit_test.go). """ import os import signal import sys +import time -# Programs whose FATAL does not take the container down (build/supervisord.conf): -# logrotate is not on the mail path, and postfix-reload runs on demand - a -# reload it could not start is reported to whoever asked for it. -SPARED = {"logrotate", "postfix-reload"} +# Programs whose failure does not take the container down +# (build/supervisord.conf): logrotate is not on the mail path, postfix-reload +# runs on demand - a reload it could not start is reported to whoever asked for +# it - syslog-relay carries OpenDKIM's log lines, not mail, and cert-reload +# looks for a renewed certificate once a day (a crash of it leaves the one +# Postfix serves). +SPARED = {"logrotate", "postfix-reload", "syslog-relay", "cert-reload"} + +# CRASH_LIMIT unexpected exits of one program within CRASH_WINDOW seconds are a +# program that does not stay up. A program on the mail path restarts in +# seconds; five times in ten minutes is no passing fault. +CRASH_LIMIT = 5 +CRASH_WINDOW = 600 def write_stdout(s): @@ -35,7 +54,20 @@ def write_stdout(s): sys.stdout.flush() +def say(s): + sys.stderr.write("crashexit: " + s + "\n") + sys.stderr.flush() + + +def shut_down(): + if os.environ.get("CRASHEXIT_DRYRUN") == "1": + say("would shut down") + return + os.kill(os.getppid(), signal.SIGTERM) + + def main(): + crashes = {} # program -> times of its unexpected exits in the window while True: # Tell supervisord we are ready for the next event. write_stdout("READY\n") @@ -50,22 +82,32 @@ def main(): pair.split(":", 1) for pair in payload.split() if ":" in pair ) name = tokens.get("processname", "") + event = headers.get("eventname", "PROCESS_STATE_FATAL") - # We only subscribe to PROCESS_STATE_FATAL, so any event means a managed - # program can no longer be restarted. - if name in SPARED: - sys.stderr.write( - "crashexit: %s entered FATAL; it is not on the mail path, the " - "container stays up\n" % name + if event == "PROCESS_STATE_EXITED": + if name not in SPARED and tokens.get("expected") == "0": + now = time.monotonic() + times = [t for t in crashes.get(name, []) if now - t < CRASH_WINDOW] + times.append(now) + crashes[name] = times + if len(times) >= CRASH_LIMIT: + say( + "%s exited unexpectedly %d times in %d seconds; shutting down container" + % (name, len(times), CRASH_WINDOW) + ) + shut_down() + crashes[name] = [] + elif name in SPARED: + say( + "%s entered FATAL; it is not on the mail path, the container stays up" + % name ) - sys.stderr.flush() else: - sys.stderr.write( - "crashexit: %s entered FATAL; shutting down container\n" + say( + "%s entered FATAL; shutting down container" % (name or "a managed process") ) - sys.stderr.flush() - os.kill(os.getppid(), signal.SIGTERM) + shut_down() write_stdout("RESULT 2\nOK") diff --git a/build/crashexit_test.go b/build/crashexit_test.go new file mode 100644 index 0000000..4e95a7e --- /dev/null +++ b/build/crashexit_test.go @@ -0,0 +1,95 @@ +// Package build holds the image's scripts; this file tests crashexit.py, the +// supervisor event listener that takes the container down. +package build + +import ( + "fmt" + "os" + "os/exec" + "runtime" + "strings" + "testing" +) + +// event is one event as supervisord writes it to a listener's stdin +// (supervisor 4.2.5, docs/events.rst: a header line whose len is the +// payload's length, then the payload). +func event(name, payload string) string { + return fmt.Sprintf("ver:3.0 server:supervisor serial:1 pool:crashexit poolserial:1 eventname:%s len:%d\n%s", name, len(payload), payload) +} + +// runListener feeds events to crashexit.py in dry run and returns what it +// wrote to stderr. +func runListener(t *testing.T, events ...string) string { + t.Helper() + // python3 on Linux; on Windows "python3" may be the Store's stub, which + // runs nothing. + names := []string{"python3", "python"} + if runtime.GOOS == "windows" { + names = []string{"python", "python3"} + } + python := "" + for _, n := range names { + if p, err := exec.LookPath(n); err == nil { + python = p + break + } + } + if python == "" { + t.Skip("no python") + } + cmd := exec.Command(python, "crashexit.py") + cmd.Env = append(os.Environ(), "CRASHEXIT_DRYRUN=1") + cmd.Stdin = strings.NewReader(strings.Join(events, "")) + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + t.Fatalf("crashexit.py: %v\n%s", err, stderr.String()) + } + // Python on Windows writes its text stdout with \r\n. + if n := strings.Count(strings.ReplaceAll(string(out), "\r\n", "\n"), "RESULT 2\nOK"); n != len(events) { + t.Errorf("answered %d of %d events:\n%q", n, len(events), out) + } + return stderr.String() +} + +// A program on the mail path that keeps crashing after its startsecs - never +// FATAL - takes the container down at the fifth unexpected exit (audit +// 2026-10-12, L32); expected exits, spared programs and fewer crashes do not. +func TestCrashexitCountsCrashes(t *testing.T) { + exit := func(name, expected string) string { + return event("PROCESS_STATE_EXITED", "processname:"+name+" groupname:"+name+" from_state:RUNNING expected:"+expected+" pid:7") + } + var four []string + for i := 0; i < 4; i++ { + four = append(four, exit("panel", "0")) + } + if got := runListener(t, four...); strings.Contains(got, "would shut down") { + t.Errorf("four crashes shut down:\n%s", got) + } + if got := runListener(t, append(four, exit("panel", "0"))...); !strings.Contains(got, "panel exited unexpectedly 5 times") || !strings.Contains(got, "would shut down") { + t.Errorf("five crashes did not shut down:\n%s", got) + } + var calm []string + for i := 0; i < 6; i++ { + calm = append(calm, exit("panel", "1"), exit("logrotate", "0")) + } + calm = append(calm, exit("opendkim", "0"), exit("opendkim", "0"), exit("opendkim", "0")) + if got := runListener(t, calm...); strings.Contains(got, "would shut down") { + t.Errorf("expected exits, spared programs and three crashes of another shut down:\n%s", got) + } +} + +// FATAL of a mail-path program shuts down at once; of a spared one, not. +func TestCrashexitFatal(t *testing.T) { + fatal := func(name string) string { + return event("PROCESS_STATE_FATAL", "processname:"+name+" groupname:"+name+" from_state:BACKOFF") + } + if got := runListener(t, fatal("postfix")); !strings.Contains(got, "postfix entered FATAL; shutting down") || !strings.Contains(got, "would shut down") { + t.Errorf("postfix FATAL:\n%s", got) + } + if got := runListener(t, fatal("logrotate"), fatal("syslog-relay")); strings.Contains(got, "would shut down") { + t.Errorf("a spared program's FATAL shut down:\n%s", got) + } +} diff --git a/build/logrotate-loop.sh b/build/logrotate-loop.sh index ef962bd..d528e57 100644 --- a/build/logrotate-loop.sh +++ b/build/logrotate-loop.sh @@ -17,16 +17,21 @@ # path anew (src/postlogd/postlogd.c, logwriter_open_or_die()). # # logrotate itself only rotates once the configured "daily" period has elapsed -# (tracked in /var/lib/logrotate/status), so it is safe to invoke this more -# often than daily — polling merely bounds how late a legitimate rotation runs. -# (logrotate 3.21.0-1: debian/rules, --with-state-file-path=/var/lib/logrotate/ -# status. That file is in the container's own filesystem: a recreated container -# starts without it, and logrotate.c, newState(), records a log it has no state -# for as rotated today, so the first rotation after a recreate comes a day -# later.) +# (tracked in its state file), so it is safe to invoke this more often than +# daily — polling merely bounds how late a legitimate rotation runs. The state +# file is beside the log, under /data, not at the default +# /var/lib/logrotate/status (logrotate 3.21.0-1: debian/rules, +# --with-state-file-path) in the container's own filesystem: a recreated +# container started without it, logrotate.c, newState(), records a log it has +# no state for as rotated today, and so a host that recreates the container +# daily - an .env change, an image bump - never rotated mail.log at all (audit +# 2026-10-12, M19). (logrotate 3.21.0, logrotate.8.in: "-s, --state +# statefile".) The directory is postfix:selfpost 2750 and logrotate runs as +# root. set -eu CONFIG=/etc/logrotate.d/mail +STATE=/data/log/.logrotate.status INTERVAL="${LOGROTATE_INTERVAL_SECONDS:-21600}" # logrotate refuses configs writable by group or others and exits 0 while @@ -56,7 +61,7 @@ if ! logrotate_config_ok; then fi run_logrotate() { - out=$(logrotate "$CONFIG" 2>&1) || { + out=$(logrotate -s "$STATE" "$CONFIG" 2>&1) || { echo "$out" >&2 return 1 } diff --git a/build/postfix-config.sh b/build/postfix-config.sh index acfe9f7..7edf087 100644 --- a/build/postfix-config.sh +++ b/build/postfix-config.sh @@ -117,6 +117,64 @@ fi RATE_MSGS="${RATE_LIMIT_MESSAGES_PER_IP:-100}" RATE_WINDOW="${RATE_LIMIT_WINDOW_SECONDS:-3600}" +# The settings below reach main.cf and master.cf as numbers or time values. +# A value that is neither, or one under the parameter's minimum, passes +# `postfix check` and kills the daemon that reads it - every smtpd at its +# first connection, or anvil and with it the rate limit (audit 2026-10-12, +# L31; review of stage 5) - so the start stops here, saying which and why. +# Counts and sizes are whole numbers from min up to 2147483647, an int's top +# (Postfix 3.7.11 reads them with get_mail_conf_int*, src/global/ +# mail_conf_int.c: an int, held to the parameter's own minimum). A time value +# is a whole number from 1 with an optional unit, s m h d or w, at most +# 2147483647 seconds +# (proto/postconf.proto: "Specify a non-zero time value (an integral value +# plus an optional one-letter suffix ...). Time units: s (seconds), m +# (minutes), h (hours), d (days), w (weeks)"; one unit - "1h30m" is read as +# 1h, src/global/conv_time.c - so it is refused here). +need_number() { + case "$2" in + '' | *[!0-9]*) + echo "postfix-config: $1 must be a whole number, got '$2'" >&2 + exit 1 + ;; + esac + if [ "${#2}" -gt 10 ] || [ "$2" -lt "$3" ] || [ "$2" -gt 2147483647 ]; then + echo "postfix-config: $1 must be from $3 to 2147483647, got '$2'" >&2 + exit 1 + fi +} +need_time() { + if ! printf '%s' "$2" | grep -Eq '^[1-9][0-9]{0,8}[smhdw]?$'; then + echo "postfix-config: $1 must be a time value - a whole number from 1 with an optional s, m, h, d or w - got '$2'" >&2 + exit 1 + fi + # conv_time multiplies by the unit and refuses what passes an int. + n=${2%[smhdw]} + case "$2" in + *w) n=$((n * 604800)) ;; + *d) n=$((n * 86400)) ;; + *h) n=$((n * 3600)) ;; + *m) n=$((n * 60)) ;; + esac + if [ "$n" -gt 2147483647 ]; then + echo "postfix-config: $1 is more than 2147483647 seconds, got '$2'" >&2 + exit 1 + fi +} +# A rate of 0 is no limit, a size of 0 none (proto/postconf.proto, +# smtpd_client_message_rate_limit, message_size_limit); the window is +# anvil_rate_time_unit, at least 1 second (src/anvil/anvil.c, main(): its +# time table's minimum 1). +need_number RATE_LIMIT_MESSAGES_PER_IP "$RATE_MSGS" 0 +need_number RATE_LIMIT_WINDOW_SECONDS "$RATE_WINDOW" 1 +need_number INBOUND_RATE_LIMIT_MESSAGES_PER_IP "${INBOUND_RATE_LIMIT_MESSAGES_PER_IP:-20}" 0 +need_number INBOUND_MESSAGE_SIZE_LIMIT "${INBOUND_MESSAGE_SIZE_LIMIT:-26214400}" 0 +need_number DMARC_RATE_LIMIT_MESSAGES_PER_IP "${DMARC_RATE_LIMIT_MESSAGES_PER_IP:-20}" 0 +need_number DMARC_MESSAGE_SIZE_LIMIT "${DMARC_MESSAGE_SIZE_LIMIT:-5242880}" 0 +need_time MILTER_CONNECT_TIMEOUT "${MILTER_CONNECT_TIMEOUT:-15s}" +need_time MILTER_COMMAND_TIMEOUT "${MILTER_COMMAND_TIMEOUT:-15s}" +need_time MILTER_CONTENT_TIMEOUT "${MILTER_CONTENT_TIMEOUT:-30s}" + # Milter sockets: OpenDKIM (signing, strict), the panel's sender check # (strict) and its journal-milter (monitoring, fail-open). Fixed container # paths, matched by postfix-wrapper.sh. @@ -236,6 +294,22 @@ postconf -e \ "smtpd_tls_auth_only=yes" \ "smtpd_tls_loglevel=1" +# TLS 1.2 at least, opportunistic or required - and the client's too where an +# inbound domain requires TLS to its upstream (smtp_tls_mandatory_protocols, +# the security review of stage 5) (audit 2026-10-12, L31; Postfix +# 3.7.11, proto/postconf.proto, smtpd_tls_protocols: since 3.6 the default is +# ">=TLSv1", and a lower bound is written ">=version"). A sender that offers +# less on port 25 sends in the clear, as one without TLS does. VRFY answers +# no question about who is accepted on port 25 (relay_recipient_maps), and +# the banner names no distribution (Debian's adds "(Debian/GNU)"; the +# upstream default is "$myhostname ESMTP $mail_name"). +postconf -e \ + "smtpd_tls_protocols=>=TLSv1.2" \ + "smtpd_tls_mandatory_protocols=>=TLSv1.2" \ + "smtp_tls_mandatory_protocols=>=TLSv1.2" \ + "disable_vrfy_command=yes" \ + 'smtpd_banner=$myhostname ESMTP' + # SASL: Cyrus with the local sasldb2 the panel maintains. The realm is left # implicit (smtpd_sasl_local_domain empty) so the authenticated name Postfix # uses for sender_login_maps is the BARE login the panel writes into the map; diff --git a/build/supervisord.conf b/build/supervisord.conf index 844698a..4343451 100644 --- a/build/supervisord.conf +++ b/build/supervisord.conf @@ -9,13 +9,15 @@ ; pass, with no wait for a program to be ready.) ; ; If a managed process of the mail path exhausts its restart retries (FATAL), -; the crashexit event listener brings the whole container down so Docker's -; restart policy starts it again (the same container: docs.docker.com, "Start -; containers automatically", restart policies), rather than lingering with a -; dead component. A program reaches FATAL only through BACKOFF, by exits that -; come before its startsecs (supervisor 4.2.5, supervisor/process.py, -; Subprocess.finish() and transition()): an exit after it is EXITED and simply -; restarted, for ever. logrotate's FATAL is left on Server > Health +; or exits unexpectedly five times in ten minutes, the crashexit event listener +; brings the whole container down so Docker's restart policy starts it again +; (the same container: docs.docker.com, "Start containers automatically", +; restart policies), rather than lingering with a dead component. A program +; reaches FATAL only through BACKOFF, by exits that come before its startsecs +; (supervisor 4.2.5, supervisor/process.py, Subprocess.finish() and +; transition()): an exit after it is EXITED and simply restarted, for ever - +; which the count of exits covers (audit 2026-10-12, L32). logrotate's, +; postfix-reload's and syslog-relay's failures are left on Server > Health ; (build/crashexit.py). [supervisord] @@ -66,7 +68,11 @@ stderr_logfile=/dev/stderr stderr_logfile_maxbytes=0 [program:opendkim] -command=/usr/sbin/opendkim -f -x /etc/opendkim.conf +; It waits, five seconds at most, for the syslog relay's socket: supervisord +; starts programs in priority order without waiting for one to be ready, and +; OpenDKIM's start line would go nowhere (review of audit stage 5). exec keeps +; the supervised pid OpenDKIM's. +command=/bin/sh -c 'i=0; while [ ! -S /run/syslog/log ] && [ "$i" -lt 50 ]; do sleep 0.1; i=$((i+1)); done; exec /usr/sbin/opendkim -f -x /etc/opendkim.conf' priority=100 autostart=true autorestart=true @@ -182,6 +188,6 @@ stderr_logfile_maxbytes=0 ; STOPPED (Subprocess.finish(), self.killing), which this listener never sees. [eventlistener:crashexit] command=/usr/local/bin/crashexit.py -events=PROCESS_STATE_FATAL +events=PROCESS_STATE_FATAL,PROCESS_STATE_EXITED stderr_logfile=/dev/stderr stderr_logfile_maxbytes=0 diff --git a/cmd/panel/envdoc_test.go b/cmd/panel/envdoc_test.go index b4cfa39..8872f4b 100644 --- a/cmd/panel/envdoc_test.go +++ b/cmd/panel/envdoc_test.go @@ -1,176 +1,138 @@ package main import ( + "io/fs" + "os" + "path/filepath" + "regexp" "slices" + "strings" "testing" ) -// documentedPublic matches docs/guide.md "Environment variables" table. -var documentedPublic = []string{ - "SELFPOST_HOSTNAME", - "SUBMISSION_ENABLE", - "RATE_LIMIT_MESSAGES_PER_IP", - "RATE_LIMIT_WINDOW_SECONDS", - "SEND_LOG_RETENTION_DAYS", - "PANEL_SESSION_IDLE_DAYS", - "SELFPOST_DNS_RESOLVERS", - "TRUSTED_PROXY_CIDR", - "INBOUND_RELAY_ENABLE", - "INBOUND_ANTISPAM_MILTER", - "INBOUND_ANTISPAM_MILTER_ACTION", - "INBOUND_RATE_LIMIT_MESSAGES_PER_IP", - "INBOUND_MESSAGE_SIZE_LIMIT", - "DMARC_REPORTS_ENABLE", - "DMARC_RATE_LIMIT_MESSAGES_PER_IP", - "DMARC_MESSAGE_SIZE_LIMIT", +// The environment the image reads and the environment the documents describe +// are compared as they are, not as lists kept by hand beside them (audit +// 2026-10-12, stage 5 - the three lists this file held were compared with each +// other and with nothing else). Read: what the panel's code asks of the +// environment (cmd/panel and internal, tests aside) and what the start-up +// scripts take with a default (build/*.sh, "${VAR:-…}"). Documented: the +// operator's table in guide.md § Environment variables, with the paragraph +// under it, and the internal list in architecture.md § Configuration. + +var ( + goEnvRead = regexp.MustCompile(`(?:envDefault|envInt|envBool|envSeconds|envDuration|os\.Getenv|os\.LookupEnv)\("([A-Z][A-Z0-9_]+)"`) + scriptEnvRead = regexp.MustCompile(`\$\{([A-Z][A-Z0-9_]+):[-?=]`) + docEnvName = regexp.MustCompile("`([A-Z][A-Z0-9_]{2,})`") +) + +func readFile(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(b) } -// documentedInternal matches architecture.md § Configuration "Internal env vars". -var documentedInternal = []string{ - "SELFPOST_DATA_DIR", - "SELFPOST_DB_PATH", - "SELFPOST_SETUP_TOKEN_FILE", - "PANEL_HTTP_ADDR", - "JOURNAL_MILTER_SOCKET", - "SENDER_CHECK_SOCKET", - "MAIL_LOG", - "PANEL_COOKIE_SECURE", - "OPENDKIM_SOCKET", - "OPENDKIM_DIR", - "DKIM_SELECTOR_DEFAULT", - "SASL_DB_PATH", - "SASL_REALM", - "POSTFIX_DIR", - "POSTFIX_SENDER_LOGIN_MAPS", - "POSTFIX_QUEUE_DIR", - "POSTFIX_RELAY_DOMAINS", - "POSTFIX_TRANSPORT_MAPS", - "POSTFIX_RELAY_RECIPIENTS", - "POSTFIX_TLS_POLICY_MAPS", - "POSTFIX_DMARC_RECIPIENTS", - "POSTFIX_DMARC_TRANSPORT", - "POSTFIX_DMARC_RELAY_DOMAINS", - "SELFPOST_DEPLOY_ROOT", - "MILTER_CONNECT_TIMEOUT", - "MILTER_COMMAND_TIMEOUT", - "MILTER_CONTENT_TIMEOUT", - "MILTER_WAIT_TIMEOUT", - "TLS_RELOAD_INTERVAL_SECONDS", - "LOGROTATE_INTERVAL_SECONDS", -} - -// documentedComposeFixed are env keys documented outside the table (TLS paths fixed in compose). -var documentedComposeFixed = []string{ - "TLS_CERT_FILE", - "TLS_KEY_FILE", -} - -// loadConfigKeys is every environment variable read by loadConfig() in main.go. -// Update together with loadConfig when adding a new key. -var loadConfigKeys = []string{ - "SELFPOST_DATA_DIR", - "PANEL_HTTP_ADDR", - "JOURNAL_MILTER_SOCKET", - "SENDER_CHECK_SOCKET", - "MAIL_LOG", - "SEND_LOG_RETENTION_DAYS", - "SELFPOST_DB_PATH", - "SELFPOST_SETUP_TOKEN_FILE", - "SELFPOST_HOSTNAME", - "PANEL_COOKIE_SECURE", - "SUBMISSION_ENABLE", - "TRUSTED_PROXY_CIDR", - "PANEL_SESSION_IDLE_DAYS", - "SELFPOST_DNS_RESOLVERS", - "TLS_CERT_FILE", - "TLS_RELOAD_INTERVAL_SECONDS", - "OPENDKIM_SOCKET", - "OPENDKIM_DIR", - "DKIM_SELECTOR_DEFAULT", - "SASL_DB_PATH", - "SASL_REALM", - "POSTFIX_DIR", - "SELFPOST_DEPLOY_ROOT", - "INBOUND_RELAY_ENABLE", - "INBOUND_ANTISPAM_MILTER", - "INBOUND_ANTISPAM_MILTER_ACTION", - "DMARC_REPORTS_ENABLE", -} - -// buildScriptKeys is every ${VAR:-…} / os.Getenv used in build/*.sh and entrypoint.sh -// but not necessarily in loadConfig. Update when startup scripts gain a new knob. -var buildScriptKeys = []string{ - "SELFPOST_HOSTNAME", - "TLS_CERT_FILE", - "TLS_KEY_FILE", - "RATE_LIMIT_MESSAGES_PER_IP", - "RATE_LIMIT_WINDOW_SECONDS", - "OPENDKIM_SOCKET", - "JOURNAL_MILTER_SOCKET", - "SENDER_CHECK_SOCKET", - "MAIL_LOG", - "POSTFIX_SENDER_LOGIN_MAPS", - "POSTFIX_QUEUE_DIR", - "SASL_DB_PATH", - "SUBMISSION_ENABLE", - "INBOUND_RELAY_ENABLE", - "INBOUND_ANTISPAM_MILTER", - "INBOUND_ANTISPAM_MILTER_ACTION", - "INBOUND_RATE_LIMIT_MESSAGES_PER_IP", - "INBOUND_MESSAGE_SIZE_LIMIT", - "DMARC_REPORTS_ENABLE", - "DMARC_RATE_LIMIT_MESSAGES_PER_IP", - "DMARC_MESSAGE_SIZE_LIMIT", - "POSTFIX_RELAY_DOMAINS", - "POSTFIX_TRANSPORT_MAPS", - "POSTFIX_RELAY_RECIPIENTS", - "POSTFIX_TLS_POLICY_MAPS", - "POSTFIX_DMARC_RECIPIENTS", - "POSTFIX_DMARC_TRANSPORT", - "POSTFIX_DMARC_RELAY_DOMAINS", - "MILTER_CONNECT_TIMEOUT", - "MILTER_COMMAND_TIMEOUT", - "MILTER_CONTENT_TIMEOUT", - "MILTER_WAIT_TIMEOUT", - "TLS_RELOAD_INTERVAL_SECONDS", - "LOGROTATE_INTERVAL_SECONDS", -} - -func documentedKeys() []string { - keys := append([]string{}, documentedPublic...) - keys = append(keys, documentedInternal...) - keys = append(keys, documentedComposeFixed...) - slices.Sort(keys) - return slices.Compact(keys) -} - -func TestLoadConfigKeysDocumented(t *testing.T) { - doc := documentedKeys() - for _, key := range loadConfigKeys { - if !slices.Contains(doc, key) { - t.Errorf("loadConfig reads %s but it is not listed in guide.md public, internal, or compose-fixed env docs", key) +// envRead is every variable the code and the scripts read. +func envRead(t *testing.T) map[string]bool { + t.Helper() + keys := map[string]bool{} + for _, root := range []string{".", filepath.Join("..", "..", "internal")} { + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return err + } + for _, m := range goEnvRead.FindAllStringSubmatch(readFile(t, path), -1) { + keys[m[1]] = true + } + return nil + }) + if err != nil { + t.Fatal(err) } } + scripts, err := filepath.Glob(filepath.Join("..", "..", "build", "*.sh")) + if err != nil || len(scripts) == 0 { + t.Fatalf("no build scripts: %v", err) + } + for _, path := range scripts { + for _, m := range scriptEnvRead.FindAllStringSubmatch(readFile(t, path), -1) { + keys[m[1]] = true + } + } + return keys +} + +// section is the text of doc from the first line that starts with start up +// to the next that starts with any of ends. +func section(t *testing.T, doc, start string, ends ...string) string { + t.Helper() + i := strings.Index(doc, "\n"+start) + if i < 0 { + t.Fatalf("no %q", start) + } + rest := doc[i+1+len(start):] + end := len(rest) + for _, e := range ends { + if j := strings.Index(rest, "\n"+e); j >= 0 && j < end { + end = j + } + } + return rest[:end] +} + +// envDocumented is every variable the documents name, and where. +func envDocumented(t *testing.T) map[string]string { + t.Helper() + keys := map[string]string{} + guide := section(t, readFile(t, filepath.Join("..", "..", "docs", "guide.md")), "### Environment variables", "### ") + for _, m := range docEnvName.FindAllStringSubmatch(guide, -1) { + keys[m[1]] = "guide.md § Environment variables" + } + arch := section(t, readFile(t, filepath.Join("..", "..", "docs", "architecture.md")), "**Internal env vars.**", "### ", "## ", "**") + for _, m := range docEnvName.FindAllStringSubmatch(arch, -1) { + if _, ok := keys[m[1]]; !ok { + keys[m[1]] = "architecture.md § Configuration" + } + } + return keys +} + +func TestEnvironmentReadIsDocumented(t *testing.T) { + doc := envDocumented(t) + var missing []string + for key := range envRead(t) { + if _, ok := doc[key]; !ok { + missing = append(missing, key) + } + } + slices.Sort(missing) + for _, key := range missing { + t.Errorf("%s is read but documented nowhere: guide.md § Environment variables for an operator's setting, architecture.md § Configuration for an internal one", key) + } } -func TestBuildScriptKeysDocumented(t *testing.T) { - doc := documentedKeys() - for _, key := range buildScriptKeys { - if !slices.Contains(doc, key) { - t.Errorf("build scripts read %s but it is not listed in guide.md env documentation", key) +func TestEnvironmentDocumentedIsRead(t *testing.T) { + read := envRead(t) + var stale []string + for key, where := range envDocumented(t) { + if !read[key] { + stale = append(stale, key+" ("+where+")") } } + slices.Sort(stale) + for _, key := range stale { + t.Errorf("%s is documented but nothing reads it", key) + } } -func TestDocumentedKeysAreRead(t *testing.T) { - read := append([]string{}, loadConfigKeys...) - read = append(read, buildScriptKeys...) - slices.Sort(read) - read = slices.Compact(read) - - for _, key := range documentedKeys() { - if !slices.Contains(read, key) { - t.Errorf("guide.md documents %s but no code in loadConfig or build scripts reads it", key) +// The parsers find what they are for: a key of each kind is among the read. +func TestEnvironmentParsersFindKeys(t *testing.T) { + read := envRead(t) + for _, key := range []string{"SELFPOST_HOSTNAME", "PANEL_HTTP_ADDR", "MILTER_WAIT_TIMEOUT", "POSTFIX_QUEUE_DIR"} { + if !read[key] { + t.Errorf("%s not found among the keys read", key) } } } diff --git a/deploy/traefik/extract-cert.sh b/deploy/traefik/extract-cert.sh index bbc6316..dd4a360 100644 --- a/deploy/traefik/extract-cert.sh +++ b/deploy/traefik/extract-cert.sh @@ -1,11 +1,11 @@ -#!/bin/sh +#!/bin/bash # Extracts a PEM cert/key pair for one domain out of Traefik's acme.json — # Traefik keeps its certificates there, so a PEM extraction step is required. # Run this on the host, after Traefik has issued or renewed the certificate, and # again on a schedule (cron/systemd timer) since acme.json is not itself watched # by SelfPost/Postfix. # -# Requires jq. Usage: ./extract-cert.sh +# Requires bash and jq. Usage: ./extract-cert.sh # # The layout is not documented (doc.traefik.io/traefik/v3.1/https/acme/ says # only "JSON file"); read in Traefik v3.1.0: pkg/provider/acme/local_store.go @@ -13,23 +13,63 @@ # Certificates}, provider.go Certificate{domain, certificate, key} with the PEM # as []byte - base64 in JSON (Go encoding/json). "le" is the resolver # docker-compose.traefik.yml names. -set -eu +# +# Nothing is replaced unless both new files are PEM: a domain acme.json does +# not hold, an empty field or a broken file stops the script with an error and +# leaves the files that were there (audit 2026-10-12, M21 - under sh without +# pipefail, jq finding nothing still exited 0 and two empty files replaced the +# certificate; Postfix's TLS broke at its next restart). The new files are +# written beside the old ones, the owner's alone until they are whole, then +# renamed into place. +set -euo pipefail ACME_JSON="${1:?path to acme.json}" DOMAIN="${2:?domain name, e.g. mail.example.com}" OUT_DIR="${3:?output directory, e.g. ./extracted-certs}" mkdir -p "$OUT_DIR" +umask 077 -jq -r --arg domain "$DOMAIN" ' - .le.Certificates[] - | select(.domain.main == $domain) - | .certificate' "$ACME_JSON" | base64 -d > "$OUT_DIR/fullchain.pem" +die() { + echo "extract-cert: $*" >&2 + exit 1 +} -jq -r --arg domain "$DOMAIN" ' - .le.Certificates[] - | select(.domain.main == $domain) - | .key' "$ACME_JSON" | base64 -d > "$OUT_DIR/privkey.pem" +# field prints the base64 of one field of the domain's first certificate, or +# nothing. +field() { + jq -r --arg domain "$DOMAIN" --arg field "$1" ' + first(.le.Certificates[]? | select(.domain.main == $domain) | .[$field]) // empty + ' "$ACME_JSON" +} -chmod 0640 "$OUT_DIR/fullchain.pem" "$OUT_DIR/privkey.pem" +cert_b64=$(field certificate) +key_b64=$(field key) +[ -n "$cert_b64" ] || die "$ACME_JSON holds no certificate for $DOMAIN" +[ -n "$key_b64" ] || die "$ACME_JSON holds no key for $DOMAIN" + +tmp_cert="" tmp_key="" +trap 'rm -f "$tmp_cert" "$tmp_key"' EXIT +tmp_cert=$(mktemp "$OUT_DIR/.fullchain.pem.XXXXXX") +tmp_key=$(mktemp "$OUT_DIR/.privkey.pem.XXXXXX") + +printf '%s' "$cert_b64" | base64 -d >"$tmp_cert" || die "the certificate of $DOMAIN is not base64" +printf '%s' "$key_b64" | base64 -d >"$tmp_key" || die "the key of $DOMAIN is not base64" +grep -q -- '-----BEGIN CERTIFICATE-----' "$tmp_cert" || die "the certificate of $DOMAIN is not PEM" +grep -q -- '-----BEGIN .*PRIVATE KEY-----' "$tmp_key" || die "the key of $DOMAIN is not PEM" + +chmod 0640 "$tmp_cert" "$tmp_key" +# The group the files had stays theirs: the guide gives the key to group 998 +# so the panel can read it for a full backup, and a run from root's cron would +# otherwise hand it to root's group (review of audit stage 5). +for f in fullchain privkey; do + if [ -e "$OUT_DIR/$f.pem" ]; then + tmp=$tmp_cert + [ "$f" = privkey ] && tmp=$tmp_key + chgrp --reference="$OUT_DIR/$f.pem" "$tmp" || die "cannot give the new $f.pem the group of the old one" + fi +done +mv -f "$tmp_cert" "$OUT_DIR/fullchain.pem" +mv -f "$tmp_key" "$OUT_DIR/privkey.pem" +trap - EXIT echo "extracted $DOMAIN to $OUT_DIR/{fullchain,privkey}.pem" diff --git a/docs/architecture.md b/docs/architecture.md index a64f34f..4423ce3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -46,10 +46,17 @@ that will not start, is an exit supervisord counts as too quick: once each start it is STARTING. `crashexit` event listener exits the container when a program of the mail path -is FATAL, so Docker `restart: unless-stopped` starts it again - the same -container, not a new one: the entrypoint clears what the last run left in -`/run` (the milter sockets, the sender check's alive mark). `logrotate`'s -FATAL (a config logrotate would ignore) stays on Server › Health instead. +is FATAL - it crashed before its `startsecs` four times running (the start +and `startretries=3`) - or has +exited unexpectedly five times in ten minutes, however long each run lasted +(an exit after `startsecs` is EXITED and restarted, never FATAL; `2.6.0`), so +Docker `restart: unless-stopped` starts it again - the same container, not a +new one: the entrypoint clears what the last run left in `/run` (the milter +sockets, the sender check's alive mark). Besides a stop, supervisord itself +dying, or the entrypoint refusing a setting, nothing else exits the +container. `logrotate`'s FATAL (a config logrotate would ignore), +`postfix-reload`'s (reported to the page that asked), `cert-reload`'s and +`syslog-relay`'s stay on Server › Health instead. The panel runs `postfix-reload` through supervisord's XML-RPC interface and waits for its exit status (`internal/supervisor`, `RunOnce`); the program rests EXITED after a run. @@ -1110,7 +1117,9 @@ unsupported rather than as a missing doc: `POSTFIX_DMARC_RELAY_DOMAINS` (`/data/postfix/dmarc_relay_domains`), `POSTFIX_QUEUE_DIR` (`/data/postfix/queue` — set in `build/postfix-config.sh`), `SELFPOST_DEPLOY_ROOT` (`/selfpost-deploy` — operator project directory for - full backups; mount `.:/selfpost-deploy:ro` in compose). + full backups; mount `.:/selfpost-deploy:ro` in compose), `ANTISPAM_DIR` + (`/data/antispam` — the filter lists and the filter's key, which the spam + filter's container mounts). - **Milter and Postfix startup:** `MILTER_CONNECT_TIMEOUT` (`15s`), `MILTER_COMMAND_TIMEOUT` (`15s`), `MILTER_CONTENT_TIMEOUT` (`30s`), `MILTER_WAIT_TIMEOUT` (`30` seconds). diff --git a/docs/audit-2026-10-12.md b/docs/audit-2026-10-12.md index 80cf624..5d56006 100644 --- a/docs/audit-2026-10-12.md +++ b/docs/audit-2026-10-12.md @@ -1418,31 +1418,99 @@ documented in security.md. ### Stage 5 — image and Postfix hardening (M17–M19, M21, L30–L32) → `2.6.0` -- [ ] **Supervisor socket**: a group of its own (`supervisor`), panel only; +- [x] **Supervisor socket**: a group of its own (`supervisor`), panel only; `postfix`/`opendkim` keep `selfpost` for the file paths. — **Opus** -- [ ] **OpenDKIM**: `Syslog yes` (supervisord captures it) or `Logfile`; + *Done 2026-10-15:* `groupadd --system supervisor`, the panel its one + member; `[unix_http_server] chown=root:supervisor`. e2e: every program + RUNNING, the panel still reloads OpenDKIM and Postfix through it. +- [x] **OpenDKIM**: `Syslog yes` (supervisord captures it) or `Logfile`; `Canonicalization relaxed/simple`; `OversignHeaders From`. e2e: DKIM still verifies; a message with two `From:` is refused before signing anyway. — **Opus** -- [ ] **Postfix**: `smtpd_tls_protocols = >=TLSv1.2`, `smtpd_tls_mandatory_protocols` + *Done 2026-10-15,* read in OpenDKIM 2.11.0~beta2's source first: neither + way worked - syslog(3) writes to `/dev/log`, which nobody served in the + container, and there is no `Logfile` option. `internal/syslogrelay` + (`panel -syslog-relay=/run/syslog/log`, program `syslog-relay` before + OpenDKIM, `/dev/log` a link to it from the entrypoint) passes the lines + to stdout, severity error and worse marked. `Syslog yes`, + `Canonicalization relaxed/simple`, `OversignHeaders From` (RFC 6376 + § 3.4, § 8.15). e2e: OpenDKIM's start line in the container log; the + DKIM-verify steps pass. The two-`From:` refusal is the sender check's + (2.4.0, 2.5.2), unchanged. +- [x] **Postfix**: `smtpd_tls_protocols = >=TLSv1.2`, `smtpd_tls_mandatory_protocols` likewise, `disable_vrfy_command = yes`, `smtpd_banner = $myhostname ESMTP`; integer validation of `RATE_LIMIT_*`, `*_SIZE_LIMIT`, `MILTER_*_TIMEOUT` at the top of `postfix-config.sh` with a clear exit. — **Opus** -- [ ] **logrotate state under `/data/log/.logrotate.status`** (`-s`), group- + *Done 2026-10-15:* the four settings (proto/postconf.proto: `>=version` + since 3.6); `need_number` / `need_time` (a whole number, a time value + with s m h d w) for the six counts and sizes and the three milter + timeouts. e2e: `postconf -h` reads them back; `TestNumericSettingsGate` + starts the image with three bad values and one good set. +- [x] **logrotate state under `/data/log/.logrotate.status`** (`-s`), group- readable as the directory is. — **Opus** -- [ ] **entrypoint**: normalise the three DMARC maps with the other five; + *Done 2026-10-15:* `logrotate -s /data/log/.logrotate.status` + (logrotate.8.in: "-s, --state statefile"); the entrypoint's + `find /data/log -type f -exec chmod 0640` covers it. e2e: the file is + there after the start. +- [x] **entrypoint**: normalise the three DMARC maps with the other five; remove the dead `set-permissions` branch. — **Opus** -- [ ] **`extract-cert.sh`**: `set -o pipefail` under `bash`, check both outputs + *Done before this stage:* the maps since the review of stage 3 (every + file beside `queue/` in `/data/postfix`, H1 there), the branch since + stage 2c (assumptions C-P34). +- [x] **`extract-cert.sh`**: `set -o pipefail` under `bash`, check both outputs are non-empty PEM before moving them into place, create `0600` first. — **Opus** -- [ ] **`crashexit`**: subscribe to `PROCESS_STATE_EXITED` with a restart-count + *Done 2026-10-15:* bash, `set -euo pipefail`, `umask 077`; jq's + `first(...) // empty` per field, refused when empty or not PEM; `mktemp` + beside the target, `chmod 0640`, `mv`; the files that were there stay on + any refusal. Run on the test server with a stand-in jq (the host has + none): a good pair, a missing domain, a non-PEM field. Executable in + git. +- [x] **`crashexit`**: subscribe to `PROCESS_STATE_EXITED` with a restart-count threshold, or raise `startsecs` so a crash after 1 s still reaches FATAL; say in architecture.md what actually exits the container. — **Opus** -- [ ] **`envdoc_test` made real**: parse `envDefault|envInt|os.Getenv("…")` out of + *Done 2026-10-15:* `PROCESS_STATE_FATAL,PROCESS_STATE_EXITED`; five + `expected:0` exits of one program in 600 s shut down, as FATAL does; + `syslog-relay` joins the spared. `build/crashexit_test.go` runs the + script under python with `CRASHEXIT_DRYRUN`. architecture.md says what + exits the container. +- [x] **`envdoc_test` made real**: parse `envDefault|envInt|os.Getenv("…")` out of `cmd/panel/*.go` and `${VAR` out of `build/*.sh`, parse the key tables out of guide.md and architecture.md, compare. — **Opus** -- [ ] Docs: architecture § Image and processes, § Configuration; security.md + *Done 2026-10-15:* read from cmd/panel and internal (tests aside) and + build/*.sh (`${VAR:-…}`), documented in guide.md § Environment variables + and architecture.md's internal list; both ways. It found `ANTISPAM_DIR` + documented nowhere (added). +- [x] Docs: architecture § Image and processes, § Configuration; security.md § Process. — **Sonnet** + *Done 2026-10-15* with the steps: the programs table (`syslog-relay`), + what exits the container, `ANTISPAM_DIR`; security.md § Output and + process (the socket's group). + +*Changed by the reviews of stage 5 (2026-10-15; technical and security):* +the e2e's numeric-settings test ran a container that passed every gate and +never ended (the first run of the stage's e2e hung on it): the good values +are now checked beside a bad one after them, and the runs are named, +bounded to a minute and removed. The gates refuse what is under a +parameter's minimum (a zero window kills anvil, a zero milter timeout +smtpd and cleanup - src/anvil/anvil.c, src/smtpd/smtpd.c) and what passes an +int, in seconds for a time value (src/global/conv_time.c reads one unit and +refuses an overflow). OpenDKIM waits, five seconds at most, for the relay's +socket. The relay marks its lines `syslog: `, writes control characters +out (a forged second line), reads at once into a queue and drops past 200 +lines a second or a full queue, saying how many - a relay that stopped +reading would have held OpenDKIM in syslog(3). `smtp_tls_mandatory_protocols` +too. extract-cert.sh keeps the old files' group (a cron run as root handed +the key to root's group) and sets its trap before mktemp. `cert-reload` +joins the spared programs. security.md: the restart after five panel exits, +and the relay's socket. Measured, against the technical review's guess: +smtpd runs with the `selfpost` group among its groups (Uid 101, Groups +104 998 on the test server), so before this stage it did reach the +supervisor socket. Accepted, with the reason: the logrotate state file in +the `postfix` user's directory - that user can stop rotation through the +log it owns anyway; envdoc_test reads `${VAR:-}` and the Go helpers, not +bare `$VAR` or python (none today); the 600 s pruning in crashexit is not +tested (the count and the spared are). ### Stage 6 — documentation and roadmap sweep (no product code, no cut) diff --git a/docs/roadmap.md b/docs/roadmap.md index 7f21999..183afba 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -45,7 +45,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md). | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | -| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`, 2c `2.5.2`, 3 `2.5.3`, 4 `2.5.4`; next stage 5 → `2.6.0`) | 68/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | +| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`, stages 1+2 `2.5.0`, 2b `2.5.1`, 2c `2.5.2`, 3 `2.5.3`, 4 `2.5.4`; next stage 5 → `2.6.0`) | 77/81 | [audit-2026-10-12.md](audit-2026-10-12.md) | **Recommended order** (not binding; owner, 2026-10-09: feature order follows development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and diff --git a/docs/security.md b/docs/security.md index 098edb1..aafbefc 100644 --- a/docs/security.md +++ b/docs/security.md @@ -572,7 +572,10 @@ deferred item from the roadmap. off by whoever can make the panel slow. Supervisord starts a panel that died again at once; one that dies within a second of starting is tried four times in a few seconds, and then SelfPost's `crashexit` listener takes - the container down (supervisor 4.2.5, `startretries=3`), so the outage is + the container down (supervisor 4.2.5, `startretries=3`); since `2.6.0` it + does so too when the panel exits unexpectedly five times in ten minutes, + however long each run lasted - so five malformed packets on a milter + socket (the next item) restart the whole container. The outage is seconds or the whole instance; Server › Health counts the downtime and the mail deferred for it. Trigger to revisit: those numbers not staying at nothing - then the check moves into a process of its own @@ -585,6 +588,15 @@ deferred item from the roadmap. of the restart defer mail instead of passing it unjournalled (technical review, 2026-10-12). Trigger to revisit: anything untrusted able to open a Unix socket in the container. +- **The container's `/dev/log` takes datagrams from any user in it** (since + `2.6.0`, for OpenDKIM's log): what is written there reaches the container + log, each line marked `syslog: `, control characters written out, at most + 200 lines a second (the rest counted and dropped). A user who can write it + can add lines to `docker logs` under that mark and no other; the panel + reads only `mail.log`. logrotate's state file is under `/data/log`, the + `postfix` user's directory: that user could stop rotation through it - + as through the log itself, which it owns. Trigger to revisit: anything + untrusted able to run in the container. - **When the hostname is itself a sending domain,** the server's mail is signed with that domain's key. Whoever may export the domain has the key, and its applications may send as `postmaster@` by right. The diff --git a/internal/logtail/logtail_test.go b/internal/logtail/logtail_test.go index e47f47d..dc6b1d7 100644 --- a/internal/logtail/logtail_test.go +++ b/internal/logtail/logtail_test.go @@ -18,7 +18,7 @@ import ( // code cites (Postfix 3.7.11, src/global/log_adhoc.c and the others), unless a // comment says where one was seen; some carry no stamp, which the parsers // take as they come. One shape Postfix does not write here: opendkim lines in -// mail.log (build/opendkim.conf has Syslog no). +// mail.log (OpenDKIM logs through syslog(3), to the container log - build/opendkim.conf). // qmgr's expiry line, as Postfix 3.7.11 writes it (src/qmgr/qmgr_active.c, // qmgr_active_done_25_generic(): "%s: from=<%s>, status=%s, returned to diff --git a/internal/syslogrelay/syslogrelay.go b/internal/syslogrelay/syslogrelay.go index ac1945f..58754c4 100644 --- a/internal/syslogrelay/syslogrelay.go +++ b/internal/syslogrelay/syslogrelay.go @@ -17,6 +17,8 @@ import ( "net" "os" "regexp" + "strconv" + "time" ) // stamp is the BSD syslog timestamp that follows the priority, "Oct 15 @@ -24,10 +26,17 @@ import ( // space). The container's log has times of its own. var stamp = regexp.MustCompile(`^[A-Z][a-z]{2} [ 0-9][0-9] [0-9]{2}:[0-9]{2}:[0-9]{2} `) +// Prefix begins every relayed line. The socket takes datagrams from any user +// in the container, so a relayed line must not pass for one of the panel's +// or supervisord's own (security review of audit stage 5). +const Prefix = "syslog: " + // Line turns one datagram into the line written out: the priority and the // timestamp taken off, trailing newlines too; what remains is "ident[pid]: // message". A severity of error or worse (RFC 3164 § 4.1.1: the priority is -// facility*8 + severity, and 0-3 are emergency to error) is said in front. +// facility*8 + severity, and 0-3 are emergency to error) is said in front. A +// control character within it - a newline that would start a line of its own +// - is written as \xNN. func Line(datagram []byte) string { b := bytes.TrimRight(datagram, "\r\n\x00") severity := -1 @@ -51,16 +60,36 @@ func Line(datagram []byte) string { if loc := stamp.FindIndex(b); loc != nil { b = b[loc[1]:] } - line := string(b) + var out bytes.Buffer + out.WriteString(Prefix) if severity >= 0 && severity <= 3 { - line = "error: " + line + out.WriteString("error: ") } - return line + for _, c := range b { + if c < 0x20 || c == 0x7f { + fmt.Fprintf(&out, `\x%02x`, c) + continue + } + out.WriteByte(c) + } + return out.String() } +// The relay keeps reading whatever it can write: a datagram is read at once +// and queued, and dropped when the queue is full or more than ratePerSecond +// came in the last second. A sender to a unix datagram socket waits while the +// receiver's queue is full, so a relay that stopped reading would have held +// OpenDKIM in syslog(3) - with mail waiting on it (review of audit stage 5) - +// and a flood would have filled the container's log. +const ( + queueLen = 1024 + ratePerSecond = 200 +) + // Run listens on a datagram socket at path, which anyone in the container may // write to, and writes each message to w as a line until the socket fails. -// /dev/log is a link to path (build/entrypoint.sh). +// /dev/log is a link to path (build/entrypoint.sh). What it drops it says, +// once a second at most. func Run(path string, w io.Writer) error { _ = os.Remove(path) conn, err := net.ListenUnixgram("unixgram", &net.UnixAddr{Name: path, Net: "unixgram"}) @@ -73,15 +102,45 @@ func Run(path string, w io.Writer) error { if err := os.Chmod(path, 0o666); err != nil { return fmt.Errorf("syslog relay: %w", err) } + + lines := make(chan string, queueLen) + go func() { + for l := range lines { + fmt.Fprintln(w, l) + } + }() + defer close(lines) + buf := make([]byte, 64<<10) + var second time.Time + inSecond, dropped := 0, 0 for { n, _, err := conn.ReadFromUnix(buf) if err != nil { return fmt.Errorf("syslog relay: read: %w", err) } + now := time.Now() + if now.Sub(second) >= time.Second { + if dropped > 0 { + select { + case lines <- Prefix + "relay: dropped " + strconv.Itoa(dropped) + " lines - more than " + strconv.Itoa(ratePerSecond) + " a second, or the log was not taking them": + default: + } + } + second, inSecond, dropped = now, 0, 0 + } + inSecond++ if n == 0 { continue } - fmt.Fprintln(w, Line(buf[:n])) + if inSecond > ratePerSecond { + dropped++ + continue + } + select { + case lines <- Line(buf[:n]): + default: + dropped++ + } } } diff --git a/internal/syslogrelay/syslogrelay_test.go b/internal/syslogrelay/syslogrelay_test.go index c7f0705..eed1eff 100644 --- a/internal/syslogrelay/syslogrelay_test.go +++ b/internal/syslogrelay/syslogrelay_test.go @@ -13,11 +13,13 @@ import ( func TestLine(t *testing.T) { for in, want := range map[string]string{ - "<22>Oct 15 09:00:00 opendkim[45]: 3A2B: DKIM-Signature field added (s=sp, d=example.com)\n": "opendkim[45]: 3A2B: DKIM-Signature field added (s=sp, d=example.com)", - "<19>Oct 5 09:00:00 opendkim[45]: key retrieval failed": "error: opendkim[45]: key retrieval failed", - "<20>opendkim[45]: no timestamp": "opendkim[45]: no timestamp", - "not syslog at all": "not syslog at all", - "Oct 15 09:00:00 odd": "Oct 15 09:00:00 odd", + "<22>Oct 15 09:00:00 opendkim[45]: 3A2B: DKIM-Signature field added (s=sp, d=example.com)\n": "syslog: opendkim[45]: 3A2B: DKIM-Signature field added (s=sp, d=example.com)", + "<19>Oct 5 09:00:00 opendkim[45]: key retrieval failed": "syslog: error: opendkim[45]: key retrieval failed", + "<20>opendkim[45]: no timestamp": "syslog: opendkim[45]: no timestamp", + "not syslog at all": "syslog: not syslog at all", + "Oct 15 09:00:00 odd": "syslog: Oct 15 09:00:00 odd", + // A line of its own cannot be made: a newline inside is written out. + "<14>opendkim[1]: one\npanel: forged": `syslog: opendkim[1]: one\x0apanel: forged`, } { if got := Line([]byte(in)); got != want { t.Errorf("Line(%q) = %q, want %q", in, got, want) @@ -43,26 +45,32 @@ func (s *syncBuffer) String() string { return s.b.String() } -// What a program sends to the socket comes out as a line. -func TestRunRelaysADatagram(t *testing.T) { +// startRelay runs the relay on a socket of its own and returns a connection +// to it. +func startRelay(t *testing.T, w *syncBuffer) net.Conn { + t.Helper() if runtime.GOOS == "windows" { t.Skip("no unixgram sockets on Windows; the container is Linux") } path := filepath.Join(t.TempDir(), "log") - out := &syncBuffer{} - go Run(path, out) + go Run(path, w) var conn net.Conn var err error for i := 0; i < 50; i++ { if conn, err = net.Dial("unixgram", path); err == nil { - break + t.Cleanup(func() { conn.Close() }) + return conn } time.Sleep(20 * time.Millisecond) } - if err != nil { - t.Fatal(err) - } - defer conn.Close() + t.Fatal(err) + return nil +} + +// What a program sends to the socket comes out as a line. +func TestRunRelaysADatagram(t *testing.T) { + out := &syncBuffer{} + conn := startRelay(t, out) if _, err := conn.Write([]byte("<19>Oct 15 09:00:00 opendkim[7]: can't load key")); err != nil { t.Fatal(err) } @@ -70,7 +78,33 @@ func TestRunRelaysADatagram(t *testing.T) { for time.Now().Before(deadline) && !strings.Contains(out.String(), "\n") { time.Sleep(10 * time.Millisecond) } - if got := out.String(); got != "error: opendkim[7]: can't load key\n" { + if got := out.String(); got != "syslog: error: opendkim[7]: can't load key\n" { t.Errorf("relayed %q", got) } } + +// A flood is cut at the rate, and what was dropped is said. +func TestRunDropsAFlood(t *testing.T) { + out := &syncBuffer{} + conn := startRelay(t, out) + for i := 0; i < ratePerSecond+100; i++ { + if _, err := conn.Write([]byte("<14>flood")); err != nil { + t.Fatal(err) + } + } + time.Sleep(1100 * time.Millisecond) + if _, err := conn.Write([]byte("<14>after")); err != nil { + t.Fatal(err) + } + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) && !strings.Contains(out.String(), "syslog: after") { + time.Sleep(10 * time.Millisecond) + } + got := out.String() + if n := strings.Count(got, "syslog: flood\n"); n != ratePerSecond { + t.Errorf("%d flood lines written, want %d", n, ratePerSecond) + } + if !strings.Contains(got, "syslog: relay: dropped 100 lines") { + t.Errorf("the drop was not said:\n%s", got) + } +} diff --git a/test/e2e/hostname_gate_test.go b/test/e2e/hostname_gate_test.go index e498db2..b94ac50 100644 --- a/test/e2e/hostname_gate_test.go +++ b/test/e2e/hostname_gate_test.go @@ -1,6 +1,8 @@ package e2e import ( + "context" + "fmt" "os" "os/exec" "strings" @@ -13,7 +15,7 @@ import ( // build/entrypoint.sh runs and fails before anything else needs them) with // hostnameEnv substituted for SELFPOST_HOSTNAME, and returns its combined // output and whether it exited zero. -func runEntrypoint(t *testing.T, hostnameEnv string) (output string, exitedZero bool) { +func runEntrypoint(t *testing.T, hostnameEnv string, env ...string) (output string, exitedZero bool) { t.Helper() dataDir := t.TempDir() // Entrypoint also chmod 755 /data; mirror that here so a pre-fix image @@ -23,13 +25,19 @@ func runEntrypoint(t *testing.T, hostnameEnv string) (output string, exitedZero t.Fatalf("chmod data dir: %v", err) } - args := []string{ - "run", "--rm", - "-e", "SELFPOST_HOSTNAME=" + hostnameEnv, - "-v", dataDir + ":/data", - "selfpost:e2e", + // A name of its own, so a run that outlives its minute is removed. + name := fmt.Sprintf("selfpost-gate-%d", time.Now().UnixNano()) + defer exec.Command("docker", "rm", "-f", name).Run() + args := []string{"run", "--rm", "--name", name, "-e", "SELFPOST_HOSTNAME=" + hostnameEnv} + for _, e := range env { + args = append(args, "-e", e) } - cmd := exec.Command("docker", args...) + args = append(args, "-v", dataDir+":/data", "selfpost:e2e") + // A start that gets past every gate runs on: it is ended after a minute + // rather than holding the run until the test binary's own timeout. + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, "docker", args...) cmd.Env = os.Environ() out, err := cmd.CombinedOutput() return string(out), err == nil @@ -114,3 +122,33 @@ func runEntrypointBackground(t *testing.T, hostnameEnv string) (output string, s logs, _ := exec.Command("docker", "logs", name).CombinedOutput() return string(logs), err == nil } + +// A setting Postfix takes as a number or a time value stops the start when it +// is neither, naming the variable: before, it passed `postfix check` and every +// smtpd died at its first connection (audit 2026-10-12, L31). A sensible +// value of the same form gets past the check (and the start fails later, for +// what this bare run lacks). +func TestNumericSettingsGate(t *testing.T) { + for _, c := range []struct{ env, want string }{ + {"RATE_LIMIT_MESSAGES_PER_IP=100/h", "RATE_LIMIT_MESSAGES_PER_IP must be a whole number, got '100/h'"}, + {"INBOUND_MESSAGE_SIZE_LIMIT=10MB", "INBOUND_MESSAGE_SIZE_LIMIT must be a whole number, got '10MB'"}, + {"MILTER_CONTENT_TIMEOUT=thirty", "MILTER_CONTENT_TIMEOUT must be a time value"}, + // Under the parameter's minimum: anvil and smtpd die on a zero. + {"RATE_LIMIT_WINDOW_SECONDS=0", "RATE_LIMIT_WINDOW_SECONDS must be from 1 to 2147483647, got '0'"}, + {"MILTER_COMMAND_TIMEOUT=0s", "MILTER_COMMAND_TIMEOUT must be a time value"}, + {"INBOUND_MESSAGE_SIZE_LIMIT=99999999999", "INBOUND_MESSAGE_SIZE_LIMIT must be from 0 to 2147483647"}, + } { + t.Run(c.env, func(t *testing.T) { + out, ok := runEntrypoint(t, "mail.example.com", c.env) + if ok || !strings.Contains(out, c.want) { + t.Fatalf("%s: exited zero %v, want the refusal %q\noutput:\n%s", c.env, ok, c.want, out) + } + }) + } + // Good values beside a bad one checked after them: the refusal names the + // bad one alone. (A start with good values only would run on.) + out, _ := runEntrypoint(t, "mail.example.com", "RATE_LIMIT_MESSAGES_PER_IP=50", "MILTER_CONNECT_TIMEOUT=2m", "MILTER_CONTENT_TIMEOUT=bad") + if !strings.Contains(out, "MILTER_CONTENT_TIMEOUT must be") || strings.Contains(out, "RATE_LIMIT_MESSAGES_PER_IP must") || strings.Contains(out, "MILTER_CONNECT_TIMEOUT must") { + t.Errorf("valid values refused:\n%s", out) + } +} diff --git a/test/e2e/logrotate_check.go b/test/e2e/logrotate_check.go index c4ade23..115354f 100644 --- a/test/e2e/logrotate_check.go +++ b/test/e2e/logrotate_check.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) // checkLogrotateConfigMode verifies the image pins /etc/logrotate.d/mail at 0644 @@ -21,7 +22,14 @@ func checkLogrotateConfigMode(s *stack) error { if mode != "644" { return fmt.Errorf("/etc/logrotate.d/mail mode is %q, want 644", mode) } - return nil + // The state file is beside the log, under /data, so a recreated container + // keeps when mail.log was last rotated (audit 2026-10-12, M19); the loop + // runs logrotate at once when it starts. + return waitFor("logrotate's state under /data/log", 20*time.Second, 500*time.Millisecond, func() (bool, error) { + out, _ := s.execIn("selfpost", "sh", "-c", "test -s /data/log/.logrotate.status && echo yes") + out = strings.TrimSpace(out) + return out == "yes", fmt.Errorf("state file: %q", out) + }) } // checkLogrotateRotation forces a rotation and checks the recreated mail.log is diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index 64f38b9..19b5979 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -116,6 +116,9 @@ func TestE2E(t *testing.T) { if err := checkOpenDKIMLogs(h); err != nil { t.Fatal(err) } + if err := checkPostfixHardening(h); err != nil { + t.Fatal(err) + } if err := checkInboundRelayOff(h); err != nil { t.Fatal(err) } diff --git a/test/e2e/process_check.go b/test/e2e/process_check.go index 3b90e80..3f076fa 100644 --- a/test/e2e/process_check.go +++ b/test/e2e/process_check.go @@ -141,3 +141,18 @@ func checkOpenDKIMLogs(s *stack) error { return strings.Contains(out, "opendkim[") && strings.Contains(out, "OpenDKIM Filter v"), fmt.Errorf("no OpenDKIM line in the log") }) } + +// checkPostfixHardening reads back what postfix-config.sh sets against +// downgrade and enumeration (audit 2026-10-12, L31). +func checkPostfixHardening(s *stack) error { + out, err := s.execIn("selfpost", "postconf", "-h", "smtpd_tls_protocols", "smtpd_tls_mandatory_protocols", "disable_vrfy_command", "smtpd_banner") + if err != nil { + return fmt.Errorf("postconf: %v\n%s", err, out) + } + got := strings.Split(strings.TrimSpace(out), "\n") + want := []string{">=TLSv1.2", ">=TLSv1.2", "yes", "$myhostname ESMTP"} + if strings.Join(got, "|") != strings.Join(want, "|") { + return fmt.Errorf("postconf -h of the hardened settings = %q, want %q", got, want) + } + return nil +}