audit 2c: findings of the two reviews
test / test (push) Canceled after 0s

Technical review (Opus, 11 findings, 2 medium) and security review (Fable,
no high or medium) of 3c86280..HEAD. Both confirm the port gives
dkim_mail_parse()'s result on every input it parses, and that no line the
check passes is signed with another domain's key.

- F1 (medium): OpenDKIM's parser takes lines no mail program reads as one
  address - "ceo@bank.com:me@mine.com", "Name <ceo@bank.com:me@mine.com>",
  "<b@evil.com, a@mine.com>", "ceo@bank.com <me@mine.com>",
  "ceo@bank.com@mine.com" - and signs for the last while a lenient reader
  shows the first. The line must now also be one address in RFC 5322's
  grammar (net/mail, display names in any charset passed through), at the
  domain OpenDKIM signs for. A quoted display name may still hold anything.
- F2 (medium): the bindings were asked about OpenDKIM's form of the
  address, quotes and all, so "x"@mine.com passed for an application
  holding @mine.com while x@mine.com belonged to another. They are now
  asked about the reader's address, its local part unquoted, as Postfix
  holds the envelope's.
- F3 / security (low): Bindings.Allowed and DomainOf fold case in ASCII
  only: Postfix folds as Unicode (casefold.c, ICU), but every key and
  login the panel writes is ASCII, and the Unicode fold let the Kelvin sign
  pass for "k" (net/mail then refuses such a line on the domain test).
- F4 (low): a TERM between "postfix start-fg &" and "child=$!" is no longer
  lost: the wrapper stops Postfix as soon as the pid is known.
- F5 (low): for startsecs after a start any failed reload passed as "not
  running". postfix-reload is now build/postfix-reload.sh: exit 75 when
  master -t says no master runs, else exec postfix reload; RunOnce returns
  EndError with the status, and only 75 is taken as done. supervisor.State
  goes.
- F6 (low): crashexit spares postfix-reload too - a reload that could not
  be spawned is reported to whoever asked, not a reason to stop the
  container.
- F7 (low): a certs/ that is itself a link is resolved before the walk
  (filepath.WalkDir does not follow its root).
- F8 (low): at shutdown the tailer reads what is left of the rotated
  file before saving its place. Not changed: a refusal line still waiting
  on the old file at shutdown is not kept - the panel writes refused rows
  itself, only a kill loses one, and that window is the minute's grace.
- F9 (low): pct read as text - empty or unreadable is 100, the rest held
  to 0..100.
- F10 (low): the directory is synced after sasldb2's rename.
- F11 (low): POSTFIX_STARTSECS by expr, so "010" or "08" in
  MILTER_WAIT_TIMEOUT are decimal, not octal or an error.
- Security (low): pagehide also empties defaultValue - the input's value
  attribute and the textarea's text - not only .value. A sasldb2.new left
  by a killed panel is removed at start (entrypoint) and never archived.
  security.md says that links under certs/ are followed and by whom they
  can be placed.

Tests: sender check cases for F1 (four lines), F2 (a bound address
quoted, for its owner and for another), F3; EndError's state and status;
pct cases. architecture.md and CHANGELOG describe the reader rule. Stand
(2026-10-15, make e2e, exit 0); go vet and go test ./... pass on the
server (24 packages).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
mix
2026-10-15 09:17:33 +03:00
co-authored by Claude Opus 5.5
parent ec08c8606b
commit 9eddd49602
20 changed files with 221 additions and 50 deletions
+8
View File
@@ -42,6 +42,14 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
and so is an address over 256 bytes, which OpenDKIM would match against its and so is an address over 256 bytes, which OpenDKIM would match against its
signing table cut short - cut there, a quoted local part could end in signing table cut short - cut there, a quoted local part could end in
another domain's name. another domain's name.
- **The `From:` line must also read as one address to a mail program, at the
domain that is signed** (review of the stage). OpenDKIM's parser takes
lines no mail program reads as one address - `ceo@bank.com:me@mine.com`,
`ceo@bank.com <me@mine.com>` - and signs for the last address while a
lenient program shows the first; such lines are refused. A quoted display
name may still say anything. And the address a reader sees, its local part
unquoted, is the one held against the application's addresses: `"x"@…` no
longer passes where `x@…` belongs to another application.
- **Over IPv6 every client looks the same to SelfPost** - documented, not - **Over IPv6 every client looks the same to SelfPost** - documented, not
changed. The compose network has no IPv6, so Docker's proxy carries an changed. The compose network has no IPv6, so Docker's proxy carries an
IPv6 connection and the container sees the network's gateway IPv6 connection and the container sees the network's gateway
+1
View File
@@ -120,6 +120,7 @@ COPY --chmod=0644 internal/web/view/static/OFL.txt /usr/share/doc/selfpost/OFL.t
COPY --chmod=0644 build/opendkim.conf /etc/opendkim.conf COPY --chmod=0644 build/opendkim.conf /etc/opendkim.conf
COPY --chmod=0644 build/logrotate-mail.conf /etc/logrotate.d/mail COPY --chmod=0644 build/logrotate-mail.conf /etc/logrotate.d/mail
COPY --chmod=0755 build/postfix-wrapper.sh /usr/local/bin/postfix-wrapper.sh COPY --chmod=0755 build/postfix-wrapper.sh /usr/local/bin/postfix-wrapper.sh
COPY --chmod=0755 build/postfix-reload.sh /usr/local/bin/postfix-reload.sh
COPY --chmod=0755 build/postfix-config.sh /usr/local/bin/postfix-config.sh COPY --chmod=0755 build/postfix-config.sh /usr/local/bin/postfix-config.sh
COPY --chmod=0755 build/postfix-cert-reload.sh /usr/local/bin/postfix-cert-reload.sh COPY --chmod=0755 build/postfix-cert-reload.sh /usr/local/bin/postfix-cert-reload.sh
COPY --chmod=0755 build/logrotate-loop.sh /usr/local/bin/logrotate-loop.sh COPY --chmod=0755 build/logrotate-loop.sh /usr/local/bin/logrotate-loop.sh
+4 -2
View File
@@ -24,8 +24,10 @@ import os
import signal import signal
import sys import sys
# Programs whose FATAL does not take the container down (build/supervisord.conf). # Programs whose FATAL does not take the container down (build/supervisord.conf):
SPARED = {"logrotate"} # 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"}
def write_stdout(s): def write_stdout(s):
+7 -1
View File
@@ -101,6 +101,10 @@ mkdir -p /data/sasl
chown -R panel:selfpost /data/sasl chown -R panel:selfpost /data/sasl
chmod 2750 /data/sasl chmod 2750 /data/sasl
[ -e /data/sasl/sasldb2 ] && chmod 0640 /data/sasl/sasldb2 [ -e /data/sasl/sasldb2 ] && chmod 0640 /data/sasl/sasldb2
# The copy a password change is made on (internal/app, replaceSASLDB), left
# by a panel killed mid-change: a second copy of every password, which the
# next change would remove anyway.
rm -f /data/sasl/sasldb2.new
# Postfix state under /data (architecture.md § Persistence). The panel writes # Postfix state under /data (architecture.md § Persistence). The panel writes
# sender_login_maps; Postfix owns the on-disk queue tree under queue/. # sender_login_maps; Postfix owns the on-disk queue tree under queue/.
@@ -211,6 +215,8 @@ case "$wait_timeout" in
exit 1 exit 1
;; ;;
esac esac
export POSTFIX_STARTSECS=$((wait_timeout + 10)) # expr, not $((...)): dash's arithmetic reads "010" as octal and refuses
# "08"; expr takes decimal (review of stage 2c, F11).
export POSTFIX_STARTSECS=$(expr "$wait_timeout" + 10)
exec /usr/bin/supervisord -c /etc/supervisor/supervisord.conf exec /usr/bin/supervisord -c /etc/supervisor/supervisord.conf
+19
View File
@@ -0,0 +1,19 @@
#!/bin/sh
# The reload the panel asks supervisord for (program postfix-reload,
# build/supervisord.conf; internal/supervisor RunOnce reads its exit status).
#
# Exit 75 says no Postfix master runs - it is stopped, or still in its start
# behind the milter wait (build/postfix-wrapper.sh) - and the master that
# starts next reads the files as they are now, so the panel takes that as
# done. Anything else is "postfix reload" itself: 0, or its failure (Postfix
# 3.7.11, conf/postfix-script, "reload": exit 1 when not running or when
# postsuper fails; src/postfix/postfix.c, a main.cf it cannot read). "master
# -t" exits 0, silently, while no master holds the lock (src/master/master.c,
# test_lock). A master that stops between the two is a failure the panel
# reports - the rarer mistake (review of stage 2c, F5).
d=$(postconf -h daemon_directory) || exit 1
if "$d/master" -t 2>/dev/null; then
echo "postfix-reload: Postfix is not running; its start reads the files" >&2
exit 75
fi
exec /usr/sbin/postfix reload
+10 -1
View File
@@ -63,9 +63,15 @@ echo "postfix-wrapper: milter sockets ready, starting postfix"
# holds the lock (src/master/master.c, test_lock). # holds the lock (src/master/master.c, test_lock).
daemon_directory=$(postconf -h daemon_directory) daemon_directory=$(postconf -h daemon_directory)
child= child=
stop_asked=
stop_postfix() { stop_postfix() {
if [ -z "$child" ]; then
# Between "&" and "child=$!": stopped right after the pid is known.
stop_asked=1
return
fi
trap '' TERM INT trap '' TERM INT
while [ -n "$child" ] && kill -0 "$child" 2>/dev/null; do while kill -0 "$child" 2>/dev/null; do
if "$daemon_directory/master" -t 2>/dev/null; then if "$daemon_directory/master" -t 2>/dev/null; then
sleep 1 sleep 1
else else
@@ -77,6 +83,9 @@ stop_postfix() {
trap stop_postfix TERM INT trap stop_postfix TERM INT
postfix start-fg & postfix start-fg &
child=$! child=$!
if [ -n "$stop_asked" ]; then
stop_postfix
fi
# A trapped signal makes wait return at once, the trap is taken right after # A trapped signal makes wait return at once, the trap is taken right after
# (POSIX, Shell Command Language, 2.11 Signals and Error Handling) and exits. # (POSIX, Shell Command Language, 2.11 Signals and Error Handling) and exits.
# Otherwise wait returns when Postfix ended on its own: its status is this # Otherwise wait returns when Postfix ended on its own: its status is this
+6 -4
View File
@@ -99,16 +99,18 @@ stderr_logfile_maxbytes=0
; foreground process never reaches it (unlike OpenDKIM, which runs in the ; foreground process never reaches it (unlike OpenDKIM, which runs in the
; foreground as the supervised process itself). Instead the panel asks ; foreground as the supervised process itself). Instead the panel asks
; supervisord — over the group-accessible control socket — to run this program, ; supervisord — over the group-accessible control socket — to run this program,
; which executes the canonical `postfix reload` as root. autostart is off (it ; which runs the canonical `postfix reload` as root - or exits 75 when no
; only runs on demand) and a fast, clean exit is expected, so it never trips the ; master runs (build/postfix-reload.sh). autostart is off (it only runs on
; crashexit listener. (start-fg: see build/postfix-wrapper.sh. supervisor ; demand). A program that cannot even be spawned twice in a row ends FATAL
; (startretries=1); crashexit leaves that to the page that asked for the
; reload, which reports it (build/crashexit.py; review of stage 2c, F6). (start-fg: see build/postfix-wrapper.sh. supervisor
; 4.2.5, supervisor/process.py, Subprocess.finish(): with startsecs=0 no exit ; 4.2.5, supervisor/process.py, Subprocess.finish(): with startsecs=0 no exit
; counts as "too quickly", so even a failed run ends EXITED - never BACKOFF, ; counts as "too quickly", so even a failed run ends EXITED - never BACKOFF,
; never FATAL - with its exit status kept, which the panel reads ; never FATAL - with its exit status kept, which the panel reads
; (internal/supervisor, RunOnce). Its resting state is STOPPED until the first ; (internal/supervisor, RunOnce). Its resting state is STOPPED until the first
; run, EXITED after it; assumptions C-V10.) ; run, EXITED after it; assumptions C-V10.)
[program:postfix-reload] [program:postfix-reload]
command=/usr/sbin/postfix reload command=/usr/local/bin/postfix-reload.sh
autostart=false autostart=false
autorestart=false autorestart=false
startsecs=0 startsecs=0
+4 -1
View File
@@ -296,7 +296,10 @@ server itself.
holds; a parse failure, no address, or a second one after it is refused, holds; a parse failure, no address, or a second one after it is refused,
and so is an address over 256 bytes, which OpenDKIM would match its and so is an address over 256 bytes, which OpenDKIM would match its
`SigningTable` against cut short (`dkimf_apply_signtable()`, `SigningTable` against cut short (`dkimf_apply_signtable()`,
`MAXADDRESS`). Encoded words are not decoded, by either. `MAXADDRESS`). Encoded words are not decoded, by either. The line must also
be one address in RFC 5322's grammar (`net/mail`), at the domain OpenDKIM
signs for, so that a mail program shows the address that is signed; that
address, its local part unquoted, is what the bindings are asked about.
- **The verdict** comes at the end of the headers: `550 5.7.1`, permanent, - **The verdict** comes at the end of the headers: `550 5.7.1`, permanent,
naming the line, as a reply code of the milter's own. What the client wrote naming the line, as a reply code of the milter's own. What the client wrote
is reduced to printable ASCII and cut before it is said back, logged or is reduced to printable ASCII and cut before it is said back, logged or
+5
View File
@@ -303,6 +303,11 @@ operator can act on it.
private key from `certs/` when present; a domain export carries the DKIM key private key from `certs/` when present; a domain export carries the DKIM key
and **working** application passwords in the clear (otherwise a transfer and **working** application passwords in the clear (otherwise a transfer
without recreating credentials would be impossible). without recreating credentials would be impossible).
- Links under `certs/` (and `certs` itself as a link) are followed, in the
container's view: the archive carries whatever regular file they lead to
that the panel's user may read. `certs/` is the operator's, mounted
read-only; only someone who can write the project directory places a link
there, and only a global user can download the archive (`2.5.2`).
- Since 2.2.0 a full backup also carries, in the database, what Postfix logged - Since 2.2.0 a full backup also carries, in the database, what Postfix logged
about each message still in the send log (`send_log_lines`): the submitting about each message still in the send log (`send_log_lines`): the submitting
client's address and SASL login, the message id, the receiving server and client's address and SASL login, the message id, the receiving server and
+9
View File
@@ -9,6 +9,7 @@ import (
"io" "io"
"os" "os"
"os/exec" "os/exec"
"path/filepath"
"strings" "strings"
"sync" "sync"
) )
@@ -113,6 +114,14 @@ func replaceSASLDB(path string, edit func(path string) error) error {
_ = os.Remove(tmp) _ = os.Remove(tmp)
return fmt.Errorf("rename %s -> %s: %w", tmp, path, err) return fmt.Errorf("rename %s -> %s: %w", tmp, path, err)
} }
// The rename is the directory's change; without its own sync a power
// loss may bring the old database back, out of step with the panel's
// records (review of stage 2c, F10). A directory that cannot be synced
// - not on every system - leaves the change as durable as before.
if dir, err := os.Open(filepath.Dir(path)); err == nil {
_ = dir.Sync()
dir.Close()
}
return nil return nil
} }
+16 -2
View File
@@ -200,9 +200,16 @@ func validateDeployRoot(deployRoot string) error {
// left out of the archive. A directory left out is not descended into. // left out of the archive. A directory left out is not descended into.
type skipFunc func(path, rel string, d fs.DirEntry) bool type skipFunc func(path, rel string, d fs.DirEntry) bool
// skipQueue leaves the Postfix queue out of every archive. // skipQueue leaves the Postfix queue out of every archive, and the copy of
// sasldb2 a change is made on (internal/app, replaceSASLDB): a panel killed
// mid-change leaves it, a second copy of every application's password, until
// the next start removes it (build/entrypoint.sh; review of stage 2c).
func skipQueue(_, rel string, _ fs.DirEntry) bool { func skipQueue(_, rel string, _ fs.DirEntry) bool {
return filepath.ToSlash(rel) == QueueDir switch filepath.ToSlash(rel) {
case QueueDir, "sasl/sasldb2.new":
return true
}
return false
} }
// Check reads through everything Create would put into the archive, without // Check reads through everything Create would put into the archive, without
@@ -258,6 +265,13 @@ func addDeployFiles(tw *tar.Writer, deployRoot string, onWarn func(string)) erro
} }
certsDir := filepath.Join(deployRoot, CertsDirName) certsDir := filepath.Join(deployRoot, CertsDirName)
// certs/ itself may be a link - the guide once had it linked to
// certbot's live/<host> - and the walk below does not follow its root
// (path/filepath, WalkDir: "WalkDir does not follow symbolic links"), so
// the link is resolved first (review of stage 2c, F7).
if resolved, err := filepath.EvalSymlinks(certsDir); err == nil {
certsDir = resolved
}
if _, err := os.Stat(certsDir); err != nil { if _, err := os.Stat(certsDir); err != nil {
if os.IsNotExist(err) { if os.IsNotExist(err) {
if onWarn != nil { if onWarn != nil {
+9 -4
View File
@@ -7,6 +7,7 @@ import (
"encoding/xml" "encoding/xml"
"fmt" "fmt"
"io" "io"
"strconv"
"strings" "strings"
"time" "time"
"unicode/utf8" "unicode/utf8"
@@ -60,7 +61,7 @@ type feedbackXML struct {
Domain string `xml:"domain"` Domain string `xml:"domain"`
P string `xml:"p"` P string `xml:"p"`
SP string `xml:"sp"` SP string `xml:"sp"`
Pct *int `xml:"pct"` Pct *string `xml:"pct"`
ADKIM string `xml:"adkim"` ADKIM string `xml:"adkim"`
ASPF string `xml:"aspf"` ASPF string `xml:"aspf"`
} `xml:"policy_published"` } `xml:"policy_published"`
@@ -139,11 +140,15 @@ func ParseAggregate(raw []byte) (ParsedReport, error) {
PolicyASPF: field(doc.Policy.ASPF), PolicyASPF: field(doc.Policy.ASPF),
} }
// RFC 7489 § 6.3: pct is "between 0 and 100, inclusive", 100 when absent. // RFC 7489 § 6.3: pct is "between 0 and 100, inclusive", 100 when absent.
// An explicit 0 is a policy applied to no message, and stays 0; only a // An explicit 0 is a policy applied to no message, and stays 0; a report
// report without the element gets the default (assumptions C-M7). // without the element, or with an empty or unreadable one, gets the
// default, and a value outside the range is held to it (assumptions
// C-M7; review of stage 2c, F9).
out.PolicyPct = 100 out.PolicyPct = 100
if doc.Policy.Pct != nil { if doc.Policy.Pct != nil {
out.PolicyPct = *doc.Policy.Pct if n, err := strconv.Atoi(strings.TrimSpace(*doc.Policy.Pct)); err == nil {
out.PolicyPct = min(max(n, 0), 100)
}
} }
for _, rec := range doc.Records { for _, rec := range doc.Records {
row := ParsedRecord{ row := ParsedRecord{
+5
View File
@@ -271,6 +271,11 @@ func TestPolicyPct(t *testing.T) {
{"<pct>0</pct>", 0}, {"<pct>0</pct>", 0},
{"<pct>25</pct>", 25}, {"<pct>25</pct>", 25},
{"", 100}, {"", 100},
{"<pct/>", 100},
{"<pct> 50 </pct>", 50},
{"<pct>150</pct>", 100},
{"<pct>-5</pct>", 0},
{"<pct>x</pct>", 100},
} { } {
doc := strings.Replace(sampleXML, "<pct>100</pct>", c.pct, 1) doc := strings.Replace(sampleXML, "<pct>100</pct>", c.pct, 1)
got, err := ParseAggregate([]byte(doc)) got, err := ParseAggregate([]byte(doc))
+4
View File
@@ -758,6 +758,10 @@ func follow(ctx context.Context, path string, tr *tracker, caughtUp chan struct{
select { select {
case <-ctx.Done(): case <-ctx.Done():
blind.see(time.Now(), false) blind.see(time.Now(), false)
// What postlogd wrote to the rotated file since the last tick:
// the next start resumes in the new file only (review of stage
// 2c, F8).
drainOld(time.Now())
tr.record(f, read(), true) // shutdown: the next start resumes here tr.record(f, read(), true) // shutdown: the next start resumes here
return nil return nil
case <-ticker.C: case <-ticker.C:
+47 -5
View File
@@ -3,8 +3,11 @@ package milter
import ( import (
"context" "context"
"fmt" "fmt"
"io"
"log" "log"
"mime"
"net" "net"
"net/mail"
"net/textproto" "net/textproto"
"os" "os"
"strings" "strings"
@@ -201,13 +204,19 @@ func splitTableLine(line string) (key, value string, ok bool) {
// whole domain (assumptions B-PS1). The bare local part MA_FIND_DEFAULT also // whole domain (assumptions B-PS1). The bare local part MA_FIND_DEFAULT also
// tries for a local domain is never a key here: every key the panel writes // tries for a local domain is never a key here: every key the panel writes
// has an "@" (internal/postfix renderSenderLoginMaps). // has an "@" (internal/postfix renderSenderLoginMaps).
//
// Case is folded in ASCII only. Postfix folds as Unicode (src/util/casefold.c
// with ICU, which Debian builds with), but every key and login the panel
// writes is ASCII; a Unicode fold would only let a look-alike pass - the
// Kelvin sign for "k" - for an address OpenDKIM, folding ASCII, does not
// sign (review of stage 2c).
func (b *Bindings) Allowed(login, addr string) (bool, error) { func (b *Bindings) Allowed(login, addr string) (bool, error) {
b.mu.Lock() b.mu.Lock()
defer b.mu.Unlock() defer b.mu.Unlock()
if err := b.fresh(); err != nil { if err := b.fresh(); err != nil {
return false, err return false, err
} }
login, addr = strings.ToLower(login), strings.ToLower(addr) login, addr = asciiLower(login), asciiLower(addr)
keys := []string{addr} keys := []string{addr}
// recipient_delimiter is "+" (build/postfix-config.sh), so an // recipient_delimiter is "+" (build/postfix-config.sh), so an
// application listed as invoices@ may also send as invoices+march@. // application listed as invoices@ may also send as invoices+march@.
@@ -256,7 +265,7 @@ func (b *Bindings) DomainOf(login string) string {
if b.fresh() != nil { if b.fresh() != nil {
return "" return ""
} }
return b.domainOf[strings.ToLower(login)] return b.domainOf[asciiLower(login)]
} }
// Refusals is where the check leaves the send-log rows of what it refused. // Refusals is where the check leaves the send-log rows of what it refused.
@@ -384,7 +393,8 @@ func (s *senderSession) judge() (shown, why string, err error) {
// OpenDKIM takes the first; a reader may show the others. // OpenDKIM takes the first; a reader may show the others.
return shown, "it must be one address", nil return shown, "it must be one address", nil
} }
addr := r.user + "@" + asciiLower(r.domain) signedDomain := asciiLower(r.domain)
addr := r.user + "@" + signedDomain
// OpenDKIM matches its SigningTable against "user@domain" as // OpenDKIM matches its SigningTable against "user@domain" as
// snprintf() leaves it in a buffer of MAXADDRESS + 1 = 257 bytes // snprintf() leaves it in a buffer of MAXADDRESS + 1 = 257 bytes
// (opendkim.c, dkimf_apply_signtable(); libopendkim/dkim-internal.h). // (opendkim.c, dkimf_apply_signtable(); libopendkim/dkim-internal.h).
@@ -394,8 +404,26 @@ func (s *senderSession) judge() (shown, why string, err error) {
if len(addr) > 256 { if len(addr) > 256 {
return printable(addr), "the address is longer than OpenDKIM reads whole", nil return printable(addr), "the address is longer than OpenDKIM reads whole", nil
} }
shown = printable(addr) // The address a mail program shows. OpenDKIM's parser reads lines no
ok, err := s.bind.Allowed(s.login, asciiLower(addr)) // standard reader takes for one address - "ceo@bank.com:me@mine.com",
// "ceo@bank.com <me@mine.com>", "<b@evil.com, a@mine.com>" - and signs
// for the last of them while a lenient program shows the first. So the
// line must also be one address in RFC 5322's grammar (net/mail), at the
// domain OpenDKIM signs for; a quoted display name may say anything.
// And that address, its local part unquoted as Postfix holds the
// envelope's (Postfix 3.7.11, src/global/tok822_parse.c), is the one
// held against the bindings: "x"@mine.com is x@mine.com, which may be
// another application's (review of stage 2c, F1, F2).
list, perr := fromParser.ParseList(unfold(raw))
if perr != nil || len(list) != 1 {
return shown, "it must be one address, as a mail program reads it", nil
}
seen := asciiLower(list[0].Address)
if domainOf(seen) != signedDomain {
return shown, "a mail program would show another address than the one signed", nil
}
shown = printable(seen)
ok, err := s.bind.Allowed(s.login, seen)
if err != nil { if err != nil {
return shown, "", fmt.Errorf("sender bindings: %w", err) return shown, "", fmt.Errorf("sender bindings: %w", err)
} }
@@ -405,6 +433,20 @@ func (s *senderSession) judge() (shown, why string, err error) {
return shown, "", nil return shown, "", nil
} }
// fromParser reads the From: line as a mail program would. A display name
// encoded in a charset Go does not convert (windows-1251, koi8-r, ...) is
// still a display name: its bytes are passed through unread.
var fromParser = mail.AddressParser{WordDecoder: &mime.WordDecoder{
CharsetReader: func(charset string, input io.Reader) (io.Reader, error) { return input, nil },
}}
// unfold joins the lines of a folded header value (RFC 5322 2.2.3): a line
// break is dropped, the white space that follows it stays. Postfix hands a
// milter the line breaks as LF (assumptions B-PM12).
func unfold(v string) string {
return strings.NewReplacer("\r\n", "", "\n", "", "\r", "").Replace(v)
}
// asciiLower lower-cases A-Z only, as OpenDKIM's dkimf_lowercase() and // asciiLower lower-cases A-Z only, as OpenDKIM's dkimf_lowercase() and
// Postfix's table lookups do; strings.ToLower would also fold characters // Postfix's table lookups do; strings.ToLower would also fold characters
// outside ASCII - the Kelvin sign to "k" - into names they are not. // outside ASCII - the Kelvin sign to "k" - into names they are not.
+19 -2
View File
@@ -20,6 +20,8 @@ import (
// example.org, and two applications share shop.example.org. // example.org, and two applications share shop.example.org.
const senderMaps = `@news.example.net news const senderMaps = `@news.example.net news
@shop.example.org shop,shop2 @shop.example.org shop,shop2
desk@shop.example.org shop2
@kiosk.example.net kiosk
invoices@example.org billing invoices@example.org billing
billing@example.org billing billing@example.org billing
` `
@@ -102,9 +104,24 @@ func TestSenderCheck(t *testing.T) {
{"a stranger's address in the display name", "shop", [][2]string{from(`"someone@gmail.example" <a@shop.example.org>`)}, true, ""}, {"a stranger's address in the display name", "shop", [][2]string{from(`"someone@gmail.example" <a@shop.example.org>`)}, true, ""},
{"another sender in an encoded display name", "shop", [][2]string{from("=?utf-8?q?director=40example=2Eorg?= <a@shop.example.org>")}, true, ""}, {"another sender in an encoded display name", "shop", [][2]string{from("=?utf-8?q?director=40example=2Eorg?= <a@shop.example.org>")}, true, ""},
{"a stranger's address in a comment", "shop", [][2]string{from("a@shop.example.org (someone@gmail.example)")}, true, ""}, {"a stranger's address in a comment", "shop", [][2]string{from("a@shop.example.org (someone@gmail.example)")}, true, ""},
{"an unquoted address as the display name", "shop", [][2]string{from("director@example.org <a@shop.example.org>")}, true, ""}, {"an unquoted address as the display name", "shop", [][2]string{from("director@example.org <a@shop.example.org>")}, false, "as a mail program reads it"},
{"another domain inside a quoted local part", "shop", [][2]string{from(`"director@example.org"@shop.example.org`)}, true, ""}, {"another domain inside a quoted local part", "shop", [][2]string{from(`"director@example.org"@shop.example.org`)}, true, ""},
{"no local part: OpenDKIM takes the domain for it", "shop", [][2]string{from("@shop.example.org")}, true, ""}, {"no local part: OpenDKIM takes the domain for it", "shop", [][2]string{from("@shop.example.org")}, false, "as a mail program reads it"},
// Lines OpenDKIM signs for the application's domain while a lenient
// mail program shows another address first (review of stage 2c, F1).
{"another address before a colon", "shop", [][2]string{from("ceo@bank.example:me@shop.example.org")}, false, "as a mail program reads it"},
{"a route-like colon inside the brackets", "shop", [][2]string{from("Name <ceo@bank.example:me@shop.example.org>")}, false, "as a mail program reads it"},
{"a list inside the brackets", "shop", [][2]string{from("<b@evil.example, a@shop.example.org>")}, false, "as a mail program reads it"},
{"an address with two @", "shop", [][2]string{from("ceo@bank.example@shop.example.org")}, false, "as a mail program reads it"},
// An address bound to another application, its local part quoted
// (F2): a reader and Postfix take "desk" for desk.
{"another application's address, quoted", "shop", [][2]string{from(`"desk"@shop.example.org`)}, false, "not an address this application sends as"},
{"its own bound address, quoted", "shop2", [][2]string{from(`"desk"@shop.example.org`)}, true, ""},
// A look-alike of the domain in a character a Unicode fold maps
// to ASCII (F3): OpenDKIM would not sign it, and it is not the
// application's.
{"the Kelvin sign for k in the domain", "kiosk", [][2]string{from("a@\u212Aiosk.example.net")}, false, "a mail program would show another address"},
{"the same domain in ASCII", "kiosk", [][2]string{from("a@kiosk.example.net")}, true, ""},
{"another registered domain", "shop", [][2]string{from("director@example.org")}, false, "not an address this application sends as"}, {"another registered domain", "shop", [][2]string{from("director@example.org")}, false, "not an address this application sends as"},
{"the server's own name", "shop", [][2]string{from("postmaster@mail.example.org")}, false, "not an address this application sends as"}, {"the server's own name", "shop", [][2]string{from("postmaster@mail.example.org")}, false, "not an address this application sends as"},
+14 -9
View File
@@ -6,6 +6,7 @@
package postfix package postfix
import ( import (
"errors"
"fmt" "fmt"
"log" "log"
"path/filepath" "path/filepath"
@@ -194,18 +195,22 @@ func assertMapSafe(address, login string) error {
// saved shows. Before, supervisorctl's start returned as the reload was // saved shows. Before, supervisorctl's start returned as the reload was
// spawned, and a reload already running counted as done though it may have // spawned, and a reload already running counted as done though it may have
// read the files before this change (supervisor.RunOnce; assumptions B-U2). // read the files before this change (supervisor.RunOnce; assumptions B-U2).
// A reload that fails while program postfix is not RUNNING - stopped, or // A reload while no master runs - Postfix stopped, or still in its start,
// still in its start, the wrapper waiting for the milter sockets - is no // the wrapper waiting for the milter sockets - is no error: the program exits
// error: "the Postfix mail system is not running", and the master that starts // 75 then (build/postfix-reload.sh), and the master that starts next reads
// next reads the files as they are now. // the files as they are now. Before, the program's state stood in for that,
// and for the whole of startsecs after a start any failed reload passed as
// one (review of stage 2c, F5).
func reloadViaSupervisor() error { func reloadViaSupervisor() error {
err := supervisor.RunOnce("postfix-reload") err := supervisor.RunOnce("postfix-reload")
if err == nil { var end *supervisor.EndError
return nil if errors.As(err, &end) && end.State == "EXITED" && end.Status == reloadNotRunning {
} log.Printf("postfix: no Postfix master runs; its start reads the files")
if state, serr := supervisor.State("postfix"); serr == nil && state != "RUNNING" {
log.Printf("postfix: reload while program postfix is %s, left to its start: %v", state, err)
return nil return nil
} }
return err return err
} }
// reloadNotRunning is build/postfix-reload.sh's exit status when no master
// runs.
const reloadNotRunning = 75
+18 -14
View File
@@ -66,16 +66,6 @@ func RunOnce(program string) error {
return newRPC(socketPath).runOnce(program) return newRPC(socketPath).runOnce(program)
} }
// State is the state supervisord gives a program, by its name: RUNNING,
// STARTING, EXITED ... (supervisor 4.2.5, supervisor/states.py).
func State(program string) (string, error) {
v, err := newRPC(socketPath).call("supervisor.getProcessInfo", program)
if err != nil {
return "", fmt.Errorf("supervisor: state of %s: %w", program, err)
}
return v.members()["statename"].str(), nil
}
type rpc struct { type rpc struct {
client *http.Client client *http.Client
} }
@@ -125,11 +115,25 @@ func (c *rpc) runOnce(program string) error {
if info.state == stateExited && info.exitStatus == 0 { if info.state == stateExited && info.exitStatus == 0 {
return nil return nil
} }
msg := fmt.Sprintf("supervisor: %s ended %s, exit status %d", program, info.stateName, info.exitStatus) return &EndError{Program: program, State: info.stateName, Status: info.exitStatus, SpawnErr: info.spawnErr}
if info.spawnErr != "" {
msg += ": " + info.spawnErr
} }
return errors.New(msg)
// EndError is a run of RunOnce that did not end EXITED with status 0: the
// state it ended in, its exit status, and supervisord's account of a failed
// spawn.
type EndError struct {
Program string
State string
Status int
SpawnErr string
}
func (e *EndError) Error() string {
msg := fmt.Sprintf("supervisor: %s ended %s, exit status %d", e.Program, e.State, e.Status)
if e.SpawnErr != "" {
msg += ": " + e.SpawnErr
}
return msg
} }
// processInfo is the part of getProcessInfo's struct RunOnce reads. // processInfo is the part of getProcessInfo's struct RunOnce reads.
+5
View File
@@ -1,6 +1,7 @@
package supervisor package supervisor
import ( import (
"errors"
"fmt" "fmt"
"io" "io"
"net" "net"
@@ -120,6 +121,10 @@ func TestRunOnceReportsAFailedRun(t *testing.T) {
if err == nil || !strings.Contains(err.Error(), "ended EXITED, exit status 1") { if err == nil || !strings.Contains(err.Error(), "ended EXITED, exit status 1") {
t.Fatalf("runOnce: %v; want the exit status 1", err) t.Fatalf("runOnce: %v; want the exit status 1", err)
} }
var end *EndError
if !errors.As(err, &end) || end.State != "EXITED" || end.Status != 1 {
t.Fatalf("runOnce: %#v; want an EndError, EXITED, status 1", err)
}
} }
// A run in progress may have read the files before the caller wrote them: // A run in progress may have read the files before the caller wrote them:
+6
View File
@@ -74,8 +74,14 @@
// traversal (its salvageable state, the event's persisted). // traversal (its salvageable state, the event's persisted).
window.addEventListener("pagehide", function () { window.addEventListener("pagehide", function () {
var secrets = document.querySelectorAll("[data-secret]"); var secrets = document.querySelectorAll("[data-secret]");
// defaultValue too: it is the input's value attribute and the
// textarea's text, which the kept page still holds after .value is
// emptied (HTML, "The input element": defaultValue reflects the value
// content attribute; "The textarea element": its setter replaces the
// child text).
for (var i = 0; i < secrets.length; i++) { for (var i = 0; i < secrets.length; i++) {
secrets[i].value = ""; secrets[i].value = "";
secrets[i].defaultValue = "";
} }
}); });