inbound-antispam-panel: findings of the technical and security reviews
test / test (push) Canceled after 0s
test / test (push) Canceled after 0s
Steps 11 and 12 of docs/plans/inbound-antispam-panel.md: 12/12. Two
read-only reviews of d65d632..HEAD by different models (Opus,
technical; Fable, security), each given the code and questions, not
conclusions. Nothing Critical or High in the feature; the High the
technical review found outside it is the commit before this one.
Log tailer (internal/logtail/inbound.go):
- the reply of a refusal ends at the first "; from=<", Postfix's own,
and the HELO name after it is whatever is left. Taking the longest
reply let a HELO name that spelt "; from=<..> to=<..> proto=.."
supply the row's sender, recipient and reply - and so make a
refusal read as the deny list's;
- an address with a quote, a bracket or a space is not kept: a quoted
sender could otherwise supply the recipient;
- the journal is cut to its row cap every 500 rows written, not only
every six hours;
- a queue id is forgotten when the message is queued or the id is
opened by another listener, and a stale place in the queue of
arrivals no longer drops a live id;
- milter-hold lines are not read: the held message reaches the
journal milter, which writes its row - it was written twice.
Lists (internal/inbound/lists.go): a change of the table and the
rewrite of the files are one step under a mutex; a failed rewrite puts
a removed entry back and rewrites after taking a new one out.
Store: the row cap removes the oldest by time, as the page orders them.
Page: on a sheet a refusal's text is a Reply whatever it starts with.
Help and the guide say that an allow entry for a sender address trusts
an address anyone can claim (design commit before this one).
docs/security.md: the two reviews; the roles table gains Inbound > Log
and Filter lists; Mail path describes the milters, the tailer and the
lists; four accepted risks - the filter's verdict while it is down, an
allow entry by sender address, the list files' mode, log rows held by
domain name. The plan's stale lines (size and rate refusals, who reads
the journal) are corrected.
Left as found, told to the owner: the journal's rows after an unclean
stop may be written twice (the tailer saves its place every five
seconds); a "Deny sender" link is offered for an address the form then
refuses; 1.2.3.4 and 1.2.3.4/32 are two entries.
Checked on the test server, uncommitted tree: go vet ./... ok,
go test ./... ok (22 packages), make e2e ok.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -7,6 +7,7 @@ import (
|
||||
"path/filepath"
|
||||
"sort"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
|
||||
@@ -33,6 +34,12 @@ type ListStore interface {
|
||||
type Lists struct {
|
||||
store ListStore
|
||||
dir string
|
||||
// mu makes a change of the table and the rewrite of the files one step.
|
||||
// Two requests at once could otherwise write the files in the other order
|
||||
// than they changed the table - the first to read the entries being the
|
||||
// last to write them - and leave the filter with an entry the page no
|
||||
// longer lists. One Lists serves the panel, so a mutex is enough.
|
||||
mu sync.Mutex
|
||||
}
|
||||
|
||||
// NewLists builds the filter lists over the store and the map directory
|
||||
@@ -76,31 +83,63 @@ func (l *Lists) Add(list, kind, rawValue, rawNote string) (store.SpamListEntry,
|
||||
if err != nil {
|
||||
return store.SpamListEntry{}, err
|
||||
}
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
e, err := l.store.AddSpamListEntry(list, kind, value, note)
|
||||
if err != nil {
|
||||
return store.SpamListEntry{}, err
|
||||
}
|
||||
if err := l.Sync(); err != nil {
|
||||
if err := l.sync(); err != nil {
|
||||
// An entry the filter never got is worse than no entry: the page
|
||||
// would show a rule that does nothing.
|
||||
// would show a rule that does nothing. The files are written again
|
||||
// after the row is taken back, since some of the six may already
|
||||
// hold it; if that fails too, the next change or start puts it right.
|
||||
_ = l.store.DeleteSpamListEntry(e.ID)
|
||||
_ = l.sync()
|
||||
return store.SpamListEntry{}, err
|
||||
}
|
||||
return e, nil
|
||||
}
|
||||
|
||||
// Delete removes an entry and rewrites the maps.
|
||||
// Delete removes an entry and rewrites the maps. If the maps cannot be
|
||||
// written the entry is put back: an entry that is gone from the page and
|
||||
// still in the filter's file could not even be removed a second time.
|
||||
func (l *Lists) Delete(id int64) error {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
entries, err := l.store.ListSpamListEntries()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
var gone *store.SpamListEntry
|
||||
for i := range entries {
|
||||
if entries[i].ID == id {
|
||||
gone = &entries[i]
|
||||
}
|
||||
}
|
||||
if err := l.store.DeleteSpamListEntry(id); err != nil {
|
||||
return err
|
||||
}
|
||||
return l.Sync()
|
||||
if err := l.sync(); err != nil {
|
||||
if gone != nil {
|
||||
_, _ = l.store.AddSpamListEntry(gone.List, gone.Kind, gone.Value, gone.Note)
|
||||
}
|
||||
return err
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// Sync writes all six map files from SQLite, each atomically, so the filter
|
||||
// reads a whole old file or a whole new one. Every file is written even when
|
||||
// empty: the filter's configuration names all six.
|
||||
func (l *Lists) Sync() error {
|
||||
l.mu.Lock()
|
||||
defer l.mu.Unlock()
|
||||
return l.sync()
|
||||
}
|
||||
|
||||
// sync is Sync for a caller that holds mu.
|
||||
func (l *Lists) sync() error {
|
||||
entries, err := l.store.ListSpamListEntries()
|
||||
if err != nil {
|
||||
return err
|
||||
|
||||
Reference in New Issue
Block a user