findings of the reviews of audit stage 2b
Technical review (Opus) and security review (Fable) of 5be109b..5154bba;
reports in the session's scratchpad. Nothing above low from security; one
bug and three risks from the technical review. Migration 0009 is not
deployed, so it was extended.
- Bug: two refusals of one second swapped their NOQUEUE lines (each line
went to the newest lineless row). AttachRefusalLine now matches the
client address too (send_log.client_ip, written by the journal milter -
Postfix's line names the client and the sender, never the login: the
sasl_username part of the pattern never matched and could be fed by HELO,
security F1/F2), takes the nearest row in time and the oldest of equals,
and a line already on a row goes on no other (security F6).
- Waiting lines and recoveries lived only in memory while the saved offset
moved past them: a stop within 30 s lost them. The tracker now never saves
an offset past the oldest waiting item in its file (tracker.hold,
refusalLines.oldest) - for a recovery, the client= line that names its
login - so they are read again; the "once" guards make that harmless.
- A recovered row whose login the tailer did not see was nobody's: its
domain_id falls back to the envelope domain's.
- The fingerprint's file identity is the inode alone: the device number can
change across a host reboot and make the same log look rotated.
- A refused row carries its queue id; UpdateStatus's "newest row" now skips
refused rows, so a later message with that id does not flip it to sent.
- The alive mark: its age is read first and kept from then on, so asking
supervisord for 30 s cannot make it stale; the entrypoint removes it with
the sockets - a container restart is not the check's downtime, and a mark
from before would have counted the whole outage as "≈".
- Security F3: the sender check prints no ";" of the client's in the From:
text of its reply, and the tailer reads the envelope after the first ";"
- Postfix's own text.
- A refused row with a queue id and no stored lines says so (it was told the
mail.log text of 2.2.0); the fact's em dash is the house hyphen.
- Docs: architecture (client address, no login), guide § Health (warns on
either count), help topics log and sender-check (design 1ca87f2).
- e2e: refusals_show_postfixs_line wants the refusal's own sender in the
line; refusal_recovered_from_the_log waits past the tailer's 30 s and
checks that a refusal the panel wrote got no recovered twin.
Not changed, written down: review 9 (mail.log.1 read to its end at that
moment; postlogd may still write to it until the reload), 10 (a log under
512 bytes has no fingerprint, so places fall back to time briefly after a
rotation), 6 (a 451 at MAIL FROM of another milter reads the same - the
client and sender must match too now), security F5/F12 (info). Review 12:
the refused and closed-by-the-panel variants of out-message were shown to
the owner as pictures of the real templates, not drawn in out-message.html:
the guard ties every drawn screen to a page of the engine, and a variant
has the page's outline.
Evidence on the test server for this tree: go vet clean, go test ./... 23
packages ok, make e2e exit 0 (refusal_recovered_from_the_log 70 s).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -8,9 +8,12 @@ import (
|
||||
"syscall"
|
||||
)
|
||||
|
||||
// fileID names the file itself - device and inode - which a rename keeps and
|
||||
// a new file does not share. It is part of the log's fingerprint, so two log
|
||||
// files whose first bytes happen to match are still told apart.
|
||||
// fileID names the file itself - its inode - which a rename keeps and a new
|
||||
// file does not share. It is part of the log's fingerprint, so two log files
|
||||
// whose first bytes happen to match are still told apart. The device number
|
||||
// is left out: it can change across a host's reboot (LVM, btrfs, network
|
||||
// file systems), and the same log would then pass for a rotated one (review
|
||||
// of stage 2b).
|
||||
func fileID(f *os.File) string {
|
||||
info, err := f.Stat()
|
||||
if err != nil {
|
||||
@@ -20,5 +23,5 @@ func fileID(f *os.File) string {
|
||||
if !ok {
|
||||
return ""
|
||||
}
|
||||
return fmt.Sprintf("%d.%d", st.Dev, st.Ino)
|
||||
return fmt.Sprintf("%d", st.Ino)
|
||||
}
|
||||
|
||||
@@ -168,7 +168,7 @@ func Run(ctx context.Context, path string, st StatusStore, retention RetentionDa
|
||||
keeper.flush()
|
||||
refusals.retry(time.Now().UTC())
|
||||
}
|
||||
tr := &tracker{st: st, path: path}
|
||||
tr := &tracker{st: st, path: path, hold: refusals.oldest}
|
||||
return follow(ctx, path, tr, caughtUp, flush, func(line string) {
|
||||
// Until the first read reaches end-of-file the lines are those written
|
||||
// while the panel was down - and, after an unclean stop, some it had
|
||||
@@ -187,7 +187,7 @@ func Run(ctx context.Context, path string, st StatusStore, retention RetentionDa
|
||||
deferred.handle(line, now, catchingUp, tr.line)
|
||||
}
|
||||
keeper.add(line, now, catchingUp)
|
||||
refusals.handle(line, now)
|
||||
refusals.handle(line, now, Place{FP: tr.line.FP, End: tr.lineStart})
|
||||
queueID, recipient, status, ok := parseDelivery(line)
|
||||
if !ok {
|
||||
return
|
||||
@@ -647,6 +647,7 @@ func follow(ctx context.Context, path string, tr *tracker, caughtUp chan struct{
|
||||
full := pending + line
|
||||
pending = ""
|
||||
tr.line = Place{FP: tr.fp, End: consumed}
|
||||
tr.lineStart = consumed - int64(len(full))
|
||||
handle(strings.TrimRight(full, "\r\n"))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -47,9 +47,16 @@ type tracker struct {
|
||||
behind bool
|
||||
prev store.LogtailState
|
||||
|
||||
// line is the place of the line being handled: set by follow and
|
||||
// readRotated before each call of handle.
|
||||
line Place
|
||||
// line is the place of the line being handled, lineStart the offset it
|
||||
// starts at: set by follow and readRotated before each call of handle.
|
||||
line Place
|
||||
lineStart int64
|
||||
|
||||
// hold, when set, names the earliest place in a file that something read
|
||||
// from the log still waits on (a refusal's line, a refusal to recover):
|
||||
// no offset past it is saved, so a stop - clean or not - before the wait
|
||||
// is over reads it again (review of stage 2b).
|
||||
hold func(fp string) (Place, bool)
|
||||
}
|
||||
|
||||
// Place is where a line of the log ends: the fingerprint of its file and the
|
||||
@@ -158,6 +165,7 @@ func (t *tracker) readRotated(handle func(string)) bool {
|
||||
r := bufio.NewReader(f)
|
||||
for {
|
||||
line, err := r.ReadString('\n')
|
||||
t.lineStart = end
|
||||
end += int64(len(line))
|
||||
if line != "" {
|
||||
t.line = Place{FP: fp, End: end}
|
||||
@@ -198,6 +206,11 @@ func (t *tracker) adopt(f *os.File) {
|
||||
// record persists offset, at most once per persistInterval unless force is set
|
||||
// (rotation and shutdown, where the write must not be skipped).
|
||||
func (t *tracker) record(f *os.File, offset int64, force bool) {
|
||||
if t.hold != nil {
|
||||
if p, ok := t.hold(t.fp); ok && p.End < offset {
|
||||
offset = p.End
|
||||
}
|
||||
}
|
||||
if offset == t.saved {
|
||||
return
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ package logtail
|
||||
|
||||
import (
|
||||
"log"
|
||||
"net"
|
||||
"regexp"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -12,13 +13,14 @@ import (
|
||||
// Postfix's line about a message the journal milter refused at MAIL FROM - a
|
||||
// level-2 limit, a client address off the application's list - is put on the
|
||||
// row the milter wrote, so the message's page shows it (audit 2026-10-12,
|
||||
// stage 2b). The refusal has no queue id; the line is matched to its row by
|
||||
// sender, login and time. As the stand writes it:
|
||||
// stage 2b). The refusal has no queue id and the line no login; it is
|
||||
// matched to its row by client address, sender and time. As the stand writes
|
||||
// it:
|
||||
//
|
||||
// postfix/smtpd[1149]: NOQUEUE: milter-reject: MAIL from unknown[10.77.0.1]: 451 4.7.1 Service unavailable - try again later; from=<billing@sender.e2e.test> proto=ESMTP helo=<e2e-client.e2e.test>
|
||||
//
|
||||
// Port 25's smtpd (postfix/inbound/smtpd) is a stranger's and has no such row.
|
||||
var refusalLineRe = regexp.MustCompile(`^\S+ postfix/(?:([a-z0-9-]+)/)?smtpd\[\d+\]: NOQUEUE: milter-reject: MAIL from \S+: [45]\d\d .*?; from=<([^>]*)>(?:.*? sasl_username=<?([^>\s]*)>?)?`)
|
||||
var refusalLineRe = regexp.MustCompile(`^\S+ postfix/(?:([a-z0-9-]+)/)?smtpd\[\d+\]: NOQUEUE: milter-reject: MAIL from [^\[\s]*\[([^\]]+)\]: [45]\d\d .*?; from=<([^>]*)>`)
|
||||
|
||||
// refusalLineWait is how long a line waits for its row: the milter writes the
|
||||
// row when it refuses, which can be read a moment after the line.
|
||||
@@ -26,7 +28,7 @@ const refusalLineWait = 30 * time.Second
|
||||
|
||||
// RefusalStore is where the lines go. *store.Store satisfies it.
|
||||
type RefusalStore interface {
|
||||
AttachRefusalLine(login, from string, at time.Time, line string) (bool, error)
|
||||
AttachRefusalLine(clientIP, from string, at time.Time, line string) (bool, error)
|
||||
RecoverRefusal(e store.SendLogEntry, at time.Time) (bool, error)
|
||||
}
|
||||
|
||||
@@ -36,8 +38,10 @@ type RefusalStore interface {
|
||||
// postfix/cleanup[1339]: E543D3C75DA0: milter-reject: END-OF-MESSAGE from unknown[10.77.0.1]: 5.7.1 From: director@other.e2e.test - not an address this application sends as; from=<alerts@sender.e2e.test> to=<rcpt@sink.e2e.test> proto=ESMTP helo=<e2e-client.e2e.test>
|
||||
//
|
||||
// The From: line as the check printed it runs to the last " - " before the
|
||||
// semicolon; the reason after it holds neither.
|
||||
var fromRefusalRe = regexp.MustCompile(`^\S+ postfix/(?:[a-z0-9-]+/)?cleanup\[\d+\]: ([0-9A-Za-z]+): milter-reject: END-OF-MESSAGE from \S+: 5\.7\.1 From: (.*) - ([^;]*); from=<([^>]*)> to=<([^>]*)>`)
|
||||
// first semicolon; the reason after it holds neither, and the check prints no
|
||||
// ";" of the client's (sender.go printable), so that semicolon is Postfix's
|
||||
// and what follows it - the envelope - is Postfix's own text.
|
||||
var fromRefusalRe = regexp.MustCompile(`^\S+ postfix/(?:[a-z0-9-]+/)?cleanup\[\d+\]: ([0-9A-Za-z]+): milter-reject: END-OF-MESSAGE from \S+: 5\.7\.1 From: ([^;]*) - ([^;]*); from=<([^>]*)> to=<([^>]*)>`)
|
||||
|
||||
// clientLoginRe is smtpd's line naming the login a queue id was submitted
|
||||
// under: "<id>: client=host[addr], sasl_method=PLAIN, sasl_username=<login>".
|
||||
@@ -47,8 +51,9 @@ var clientLoginRe = regexp.MustCompile(`^\S+ postfix/(?:([a-z0-9-]+)/)?smtpd\[\d
|
||||
const refusalLogins = 4096
|
||||
|
||||
type pendingRefusal struct {
|
||||
login, from, line string
|
||||
at, until time.Time
|
||||
clientIP, from, line string
|
||||
at, until time.Time
|
||||
start Place // where the line starts in the log
|
||||
}
|
||||
|
||||
// pendingRecovery is a From: refusal whose row is looked for once the panel
|
||||
@@ -56,6 +61,14 @@ type pendingRefusal struct {
|
||||
type pendingRecovery struct {
|
||||
e store.SendLogEntry
|
||||
at, until time.Time
|
||||
start Place // where its client= line - or the refusal - starts
|
||||
}
|
||||
|
||||
// loginLine is the login a queue id was submitted under and where smtpd's
|
||||
// line saying so starts.
|
||||
type loginLine struct {
|
||||
login string
|
||||
start Place
|
||||
}
|
||||
|
||||
// refusalLines puts each refusal line on its row, and keeps for a while the
|
||||
@@ -65,24 +78,49 @@ type refusalLines struct {
|
||||
st RefusalStore
|
||||
pending []pendingRefusal
|
||||
recover []pendingRecovery
|
||||
logins map[string]string // queue id -> login, from smtpd's client= line
|
||||
logins map[string]loginLine // queue id -> login, from smtpd's client= line
|
||||
loginKeys []string
|
||||
}
|
||||
|
||||
// handle looks at one mail.log line.
|
||||
func (r *refusalLines) handle(line string, now time.Time) {
|
||||
// oldest is the earliest place in the file fp a waiting item starts at: the
|
||||
// tracker saves no offset past it, so a stop before the wait is over reads the
|
||||
// lines again - the client= line of a refusal to recover among them.
|
||||
func (r *refusalLines) oldest(fp string) (Place, bool) {
|
||||
var p Place
|
||||
found := false
|
||||
take := func(q Place) {
|
||||
if q.FP == fp && (!found || q.End < p.End) {
|
||||
p, found = q, true
|
||||
}
|
||||
}
|
||||
for _, x := range r.pending {
|
||||
take(x.start)
|
||||
}
|
||||
for _, x := range r.recover {
|
||||
take(x.start)
|
||||
}
|
||||
return p, found
|
||||
}
|
||||
|
||||
// handle looks at one mail.log line, which starts at start.
|
||||
func (r *refusalLines) handle(line string, now time.Time, start Place) {
|
||||
at, rest := lineTime(line, now)
|
||||
if m := clientLoginRe.FindStringSubmatch(rest); m != nil && m[1] != "inbound" {
|
||||
r.remember(m[2], m[3])
|
||||
r.remember(m[2], loginLine{login: m[3], start: start})
|
||||
return
|
||||
}
|
||||
if m := fromRefusalRe.FindStringSubmatch(rest); m != nil {
|
||||
from := start
|
||||
login, ok := r.logins[m[1]]
|
||||
if ok && login.start.FP == start.FP && login.start.End < start.End {
|
||||
from = login.start
|
||||
}
|
||||
r.recover = append(r.recover, pendingRecovery{
|
||||
e: store.SendLogEntry{
|
||||
QueueID: m[1], AppLogin: r.logins[m[1]], From: m[4], To: m[5], Domain: domainOf(m[4]),
|
||||
QueueID: m[1], AppLogin: login.login, From: m[4], To: m[5], Domain: domainOf(m[4]),
|
||||
Reason: store.ReasonFrom, HeaderFrom: m[2],
|
||||
},
|
||||
at: at, until: now.Add(refusalLineWait),
|
||||
at: at, until: now.Add(refusalLineWait), start: from,
|
||||
})
|
||||
return
|
||||
}
|
||||
@@ -90,7 +128,11 @@ func (r *refusalLines) handle(line string, now time.Time) {
|
||||
if m == nil || m[1] == "inbound" {
|
||||
return
|
||||
}
|
||||
p := pendingRefusal{login: m[3], from: m[2], line: line, at: at, until: now.Add(refusalLineWait)}
|
||||
ip := net.ParseIP(strings.TrimPrefix(m[2], "IPv6:"))
|
||||
if ip == nil {
|
||||
return
|
||||
}
|
||||
p := pendingRefusal{clientIP: ip.String(), from: m[3], line: line, at: at, until: now.Add(refusalLineWait), start: start}
|
||||
if !r.attach(p) {
|
||||
r.pending = append(r.pending, p)
|
||||
}
|
||||
@@ -127,9 +169,9 @@ func (r *refusalLines) retry(now time.Time) {
|
||||
|
||||
// remember keeps the login a queue id was submitted under, the oldest going
|
||||
// first beyond refusalLogins.
|
||||
func (r *refusalLines) remember(queueID, login string) {
|
||||
func (r *refusalLines) remember(queueID string, login loginLine) {
|
||||
if r.logins == nil {
|
||||
r.logins = make(map[string]string)
|
||||
r.logins = make(map[string]loginLine)
|
||||
}
|
||||
if _, ok := r.logins[queueID]; !ok {
|
||||
r.loginKeys = append(r.loginKeys, queueID)
|
||||
@@ -150,7 +192,7 @@ func domainOf(addr string) string {
|
||||
}
|
||||
|
||||
func (r *refusalLines) attach(p pendingRefusal) bool {
|
||||
ok, err := r.st.AttachRefusalLine(p.login, p.from, p.at, p.line)
|
||||
ok, err := r.st.AttachRefusalLine(p.clientIP, p.from, p.at, p.line)
|
||||
if err != nil {
|
||||
log.Printf("log-tailer: refusal line: %v", err)
|
||||
return true // not retried: the store said no for its own reason
|
||||
|
||||
@@ -19,13 +19,13 @@ func (r *refusalProbe) RecoverRefusal(e store.SendLogEntry, _ time.Time) (bool,
|
||||
return true, nil
|
||||
}
|
||||
|
||||
func (r *refusalProbe) AttachRefusalLine(login, from string, _ time.Time, line string) (bool, error) {
|
||||
func (r *refusalProbe) AttachRefusalLine(clientIP, from string, _ time.Time, line string) (bool, error) {
|
||||
if r.rows == 0 {
|
||||
return false, nil
|
||||
}
|
||||
r.rows--
|
||||
r.lines = append(r.lines, line)
|
||||
r.who = append(r.who, login+"|"+from)
|
||||
r.who = append(r.who, clientIP+"|"+from)
|
||||
return true, nil
|
||||
}
|
||||
|
||||
@@ -42,23 +42,23 @@ func TestRefusalLines(t *testing.T) {
|
||||
`2026-10-14T12:00:00+00:00 mail postfix/smtpd[11]: NOQUEUE: milter-reject: CONNECT from unknown[10.77.0.1]: 451 4.7.1 Service unavailable - try again later; proto=SMTP`,
|
||||
`2026-10-14T12:00:00+00:00 mail postfix/smtpd[11]: NOQUEUE: reject: RCPT from unknown[10.77.0.1]: 553 5.7.1 <a@x>: Sender address rejected: not owned by user app1; from=<a@x> to=<b@y> proto=ESMTP helo=<x>`,
|
||||
} {
|
||||
r.handle(other, now)
|
||||
r.handle(other, now, Place{})
|
||||
}
|
||||
if len(r.pending) != 0 {
|
||||
t.Fatalf("lines that are not an application's refusal at MAIL wait: %+v", r.pending)
|
||||
}
|
||||
|
||||
r.handle(limit, now) // its row is not there yet
|
||||
r.handle(limit, now, Place{}) // its row is not there yet
|
||||
if len(r.pending) != 1 {
|
||||
t.Fatalf("pending %d, want the line waiting", len(r.pending))
|
||||
}
|
||||
st.rows = 1 // the milter's row is written
|
||||
r.retry(now.Add(time.Second))
|
||||
if len(r.pending) != 0 || len(st.lines) != 1 || st.who[0] != "l2app|billing@sender.e2e.test" {
|
||||
if len(r.pending) != 0 || len(st.lines) != 1 || st.who[0] != "10.77.0.1|billing@sender.e2e.test" {
|
||||
t.Fatalf("after the retry: pending %d, attached %v", len(r.pending), st.who)
|
||||
}
|
||||
|
||||
r.handle(limit, now) // no row ever comes
|
||||
r.handle(limit, now, Place{}) // no row ever comes
|
||||
r.retry(now.Add(refusalLineWait + time.Second))
|
||||
if len(r.pending) != 0 {
|
||||
t.Fatal("a line kept waiting past its time")
|
||||
@@ -73,8 +73,8 @@ func TestFromRefusalIsRecoveredAfterTheWait(t *testing.T) {
|
||||
now := time.Date(2026, 10, 14, 12, 0, 0, 0, time.UTC)
|
||||
st := &refusalProbe{}
|
||||
r := &refusalLines{st: st}
|
||||
r.handle(`2026-10-14T12:00:00+00:00 mail postfix/smtpd[11]: E543D3C75DA0: client=unknown[10.77.0.1], sasl_method=PLAIN, sasl_username=app1`, now)
|
||||
r.handle(`2026-10-14T12:00:01+00:00 mail postfix/cleanup[13]: E543D3C75DA0: milter-reject: END-OF-MESSAGE from unknown[10.77.0.1]: 5.7.1 From: "The - Shop" <director@other.e2e.test> - it names another sender of this server; from=<alerts@sender.e2e.test> to=<rcpt@sink.e2e.test> proto=ESMTP helo=<x>`, now)
|
||||
r.handle(`2026-10-14T12:00:00+00:00 mail postfix/smtpd[11]: E543D3C75DA0: client=unknown[10.77.0.1], sasl_method=PLAIN, sasl_username=app1`, now, Place{})
|
||||
r.handle(`2026-10-14T12:00:01+00:00 mail postfix/cleanup[13]: E543D3C75DA0: milter-reject: END-OF-MESSAGE from unknown[10.77.0.1]: 5.7.1 From: "The - Shop" <director@other.e2e.test> - it names another sender of this server; from=<alerts@sender.e2e.test> to=<rcpt@sink.e2e.test> proto=ESMTP helo=<x>`, now, Place{})
|
||||
r.retry(now.Add(time.Second))
|
||||
if len(st.recovered) != 0 {
|
||||
t.Fatal("recovered before the panel had the time to write the row")
|
||||
@@ -90,3 +90,35 @@ func TestFromRefusalIsRecoveredAfterTheWait(t *testing.T) {
|
||||
t.Fatalf("recovered %+v, want %+v", e, want)
|
||||
}
|
||||
}
|
||||
|
||||
// While a line or a recovery waits, the offset saved for its file stays at the
|
||||
// earliest line it needs - for a recovery the client= line that names its
|
||||
// login - so a stop before the wait is over reads them again (review of stage
|
||||
// 2b).
|
||||
func TestWaitingLinesHoldTheSavedOffset(t *testing.T) {
|
||||
now := time.Date(2026, 10, 14, 12, 0, 0, 0, time.UTC)
|
||||
r := &refusalLines{st: &refusalProbe{}}
|
||||
if _, held := r.oldest("fp"); held {
|
||||
t.Fatal("nothing waits, yet the offset is held")
|
||||
}
|
||||
r.handle(`2026-10-14T12:00:00+00:00 mail postfix/smtpd[11]: Q1: client=unknown[10.77.0.1], sasl_method=PLAIN, sasl_username=app1`, now, Place{FP: "fp", End: 100})
|
||||
r.handle(`2026-10-14T12:00:01+00:00 mail postfix/cleanup[13]: Q1: milter-reject: END-OF-MESSAGE from unknown[10.77.0.1]: 5.7.1 From: d@other.example - not an address this application sends as; from=<a@sender.example> to=<r@x> proto=ESMTP helo=<x>`, now, Place{FP: "fp", End: 300})
|
||||
if p, held := r.oldest("fp"); !held || p.End != 100 {
|
||||
t.Fatalf("held at %+v %v, want the client= line at 100", p, held)
|
||||
}
|
||||
if _, held := r.oldest("another"); held {
|
||||
t.Fatal("a place in another file holds this one")
|
||||
}
|
||||
r.retry(now.Add(refusalLineWait + time.Second))
|
||||
if _, held := r.oldest("fp"); held {
|
||||
t.Fatal("still held after the wait")
|
||||
}
|
||||
|
||||
// The tracker clamps what it saves to the hold.
|
||||
cs := &captureStore{}
|
||||
tr := &tracker{st: cs, path: "x", fp: "fp", hold: func(string) (Place, bool) { return Place{FP: "fp", End: 50}, true }}
|
||||
tr.record(nil, 400, true)
|
||||
if cs.state.Offset != 50 {
|
||||
t.Fatalf("saved %d, want the hold at 50", cs.state.Offset)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user