From f37424d9fcea3f1efd000da9c40c0cbbcf3fb029 Mon Sep 17 00:00:00 2001 From: Mix Date: Sat, 10 Oct 2026 12:48:00 +0300 Subject: [PATCH] panel-redesign: the spam filter's socket is one of the milter sockets The owner's decision of 2026-10-10, as the plan has it (Who sees what: for the global administrator the filter's socket is one of the milter sockets on Health and Overview). Written by Sonnet, reviewed by Opus. - internal/health: CheckMilter probes an address in Postfix's two forms. unix:/path is the existing stat; inet:host:port is a TCP dial with a 1 s cap, closed at once, nothing written. An address in neither form is reported as not checkable, never as fine. Details are plain sentences ("No answer from antispam:11332 within 1 s.", "Connection to ... refused.", "The name ... could not be resolved.") and never the Go error text. - Health: with the filter on, a third row "Spam filter - inbound" with the configured address; it counts in the socket status and in Overview's Milter sockets card. A filter that does not answer is an error when INBOUND_ANTISPAM_MILTER_ACTION is tempfail (inbound mail is deferred) and a warning otherwise (mail goes through unfiltered), and the row says which. With the filter off nothing changes. - Both pages are global-only; a test checks a domain administrator never receives the address. The dial runs with the other checks on every request and poll of Overview and Health (5 s active, 30 s idle, none while the tab is hidden): a dead filter costs a poll at most 1 s, and a live one sees a connect-and-close at that rhythm per open tab. Not changed: fixtures and screenshots (the mockup draws two sockets), help texts. The CHANGELOG line comes with the documentation step, which is editing that file now. go test -count=1 ./internal/health/... ./internal/web/... ./cmd/panel ok (seven packages) Co-Authored-By: Claude Opus 5.5 --- internal/health/milter.go | 108 ++++++++++ internal/health/milter_test.go | 146 +++++++++++++ internal/health/socket.go | 5 +- .../handlers/handlers_filter_socket_test.go | 202 ++++++++++++++++++ internal/web/handlers/handlers_status.go | 25 ++- 5 files changed, 484 insertions(+), 2 deletions(-) create mode 100644 internal/health/milter.go create mode 100644 internal/health/milter_test.go create mode 100644 internal/web/handlers/handlers_filter_socket_test.go diff --git a/internal/health/milter.go b/internal/health/milter.go new file mode 100644 index 0000000..9985585 --- /dev/null +++ b/internal/health/milter.go @@ -0,0 +1,108 @@ +package health + +import ( + "errors" + "fmt" + "net" + "os" + "strconv" + "strings" + "syscall" + "time" +) + +// milterDialTimeout caps the TCP probe. The Health page and Overview run every +// check on each poll, one after another, so this is the most a dead address can +// add to a poll. +const milterDialTimeout = time.Second + +// CheckMilter probes a milter address in the two forms Postfix takes: +// unix:/path is stat-ed like any milter socket, and inet:host:port is dialled +// and the connection closed at once, with nothing written. required says what +// a failure costs, as for CheckSocket: an error when mail stops without the +// milter, a warning when it only goes through unscreened. An address in neither +// form is reported as not checkable (StatusUnknown), never as fine. Details say +// what failed in a sentence; they never carry the Go error. +func CheckMilter(name, addr string, required bool) Socket { + return checkMilter(name, addr, required, milterDialTimeout) +} + +func checkMilter(name, addr string, required bool, timeout time.Duration) Socket { + s := Socket{Name: name, Path: addr} + addr = strings.TrimSpace(addr) + if addr == "" { + s.Status = StatusUnknown + s.Detail = "No address is configured." + return s + } + + fail := func(detail string) Socket { + s.Status = missingStatus(required) + s.Detail = detail + return s + } + + switch { + case strings.HasPrefix(addr, "unix:") && len(addr) > len("unix:"): + path := strings.TrimPrefix(addr, "unix:") + fi, err := os.Stat(path) + switch { + case err != nil: + return fail("The socket file does not exist.") + case fi.Mode()&os.ModeSocket == 0: + return fail("The path exists but is not a socket.") + } + case strings.HasPrefix(addr, "inet:"): + host, port, ok := splitInet(strings.TrimPrefix(addr, "inet:")) + if !ok { + return notCheckable(s) + } + conn, err := net.DialTimeout("tcp", net.JoinHostPort(host, port), timeout) + if err != nil { + return fail(milterDialDetail(err, host, port)) + } + _ = conn.Close() + default: + return notCheckable(s) + } + + s.Present = true + s.Status = StatusOK + s.Detail = "Listening" + return s +} + +func notCheckable(s Socket) Socket { + s.Status = StatusUnknown + s.Detail = "The address is not unix:/path or inet:host:port, so it cannot be checked." + return s +} + +// splitInet reads host:port (or [v6]:port) with a numeric port. +func splitInet(hp string) (host, port string, ok bool) { + host, port, err := net.SplitHostPort(hp) + if err != nil || host == "" { + return "", "", false + } + if n, err := strconv.Atoi(port); err != nil || n < 1 || n > 65535 { + return "", "", false + } + return host, port, true +} + +// milterDialDetail turns a failed dial into a sentence that names only host and +// port. +func milterDialDetail(err error, host, port string) string { + hp := net.JoinHostPort(host, port) + var dns *net.DNSError + var nerr net.Error + switch { + case errors.As(err, &dns): + return fmt.Sprintf("The name %s could not be resolved.", host) + case errors.As(err, &nerr) && nerr.Timeout(): + return fmt.Sprintf("No answer from %s within %d s.", hp, int(milterDialTimeout/time.Second)) + case errors.Is(err, syscall.ECONNREFUSED) || strings.Contains(strings.ToLower(err.Error()), "refused"): + return fmt.Sprintf("Connection to %s refused.", hp) + } + return fmt.Sprintf("Could not connect to %s.", hp) +} diff --git a/internal/health/milter_test.go b/internal/health/milter_test.go new file mode 100644 index 0000000..b17f243 --- /dev/null +++ b/internal/health/milter_test.go @@ -0,0 +1,146 @@ +package health + +import ( + "errors" + "net" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +func TestCheckMilterInetAnswering(t *testing.T) { + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("tcp unavailable here: %v", err) + } + defer l.Close() + + got := CheckMilter("Spam filter", "inet:"+l.Addr().String(), true) + if got.Status != StatusOK || !got.Present || got.Detail != "Listening" { + t.Errorf("listening address: %+v", got) + } + if got.Name != "Spam filter" || got.Path != "inet:"+l.Addr().String() { + t.Errorf("name and path are not carried through: %+v", got) + } +} + +func TestCheckMilterInetRefused(t *testing.T) { + // A port that was just free and is closed again. + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("tcp unavailable here: %v", err) + } + addr := l.Addr().String() + l.Close() + + got := CheckMilter("Spam filter", "inet:"+addr, true) + if got.Present || got.Status != StatusError { + t.Errorf("closed port, required: %+v", got) + } + if want := "Connection to " + addr + " refused."; got.Detail != want { + t.Errorf("detail = %q, want %q", got.Detail, want) + } + if got := CheckMilter("Spam filter", "inet:"+addr, false); got.Status != StatusWarn { + t.Errorf("closed port, optional: status %q", got.Status) + } +} + +// What a failed dial says is a sentence about host:port, never the Go error. +func TestMilterDialDetail(t *testing.T) { + for _, c := range []struct { + name string + err error + want string + }{ + {"timeout", timeoutErr{}, "No answer from antispam:11332 within 1 s."}, + {"refused", &net.OpError{Op: "dial", Err: errors.New("connectex: No connection could be made because the target machine actively refused it.")}, "Connection to antispam:11332 refused."}, + {"unix-style refused", &net.OpError{Op: "dial", Err: errors.New("connect: connection refused")}, "Connection to antispam:11332 refused."}, + {"no such host", &net.DNSError{Err: "no such host", Name: "antispam", IsNotFound: true}, "The name antispam could not be resolved."}, + {"other", errors.New("dial tcp 10.0.0.1:11332: some internal thing 0xc000"), "Could not connect to antispam:11332."}, + } { + got := milterDialDetail(c.err, "antispam", "11332") + if got != c.want { + t.Errorf("%s: %q, want %q", c.name, got, c.want) + } + if strings.Contains(got, "dial tcp") || strings.Contains(got, "0xc0") { + t.Errorf("%s: raw error leaked: %q", c.name, got) + } + } +} + +type timeoutErr struct{} + +func (timeoutErr) Error() string { return "i/o timeout" } +func (timeoutErr) Timeout() bool { return true } +func (timeoutErr) Temporary() bool { return true } + +func TestCheckMilterUnix(t *testing.T) { + dir := t.TempDir() + sock := filepath.Join(dir, "filter.sock") + l, err := net.Listen("unix", sock) + if err != nil { + t.Skipf("unix sockets unavailable here: %v", err) + } + defer l.Close() + + if got := CheckMilter("Spam filter", "unix:"+sock, true); got.Status != StatusOK || !got.Present { + t.Errorf("live socket: %+v", got) + } + + missing := filepath.Join(dir, "gone.sock") + got := CheckMilter("Spam filter", "unix:"+missing, true) + if got.Present || got.Status != StatusError || got.Detail != "The socket file does not exist." { + t.Errorf("missing socket, required: %+v", got) + } + if got := CheckMilter("Spam filter", "unix:"+missing, false); got.Status != StatusWarn { + t.Errorf("missing socket, optional: status %q", got.Status) + } +} + +func TestCheckMilterUnixNotASocket(t *testing.T) { + plain := filepath.Join(t.TempDir(), "plain") + if err := os.WriteFile(plain, nil, 0o600); err != nil { + t.Fatal(err) + } + got := CheckMilter("Spam filter", "unix:"+plain, true) + if got.Present || got.Status != StatusError || got.Detail != "The path exists but is not a socket." { + t.Errorf("regular file in place of a socket: %+v", got) + } +} + +func TestCheckMilterNotCheckable(t *testing.T) { + for _, addr := range []string{ + "", "antispam:11332", "tcp:antispam:11332", "inet:antispam", "inet::11332", "inet:antispam:", "inet:antispam:http", + "inet:antispam:0", "inet:antispam:70000", "unix:", "unix", "inet:", " ", + } { + got := CheckMilter("Spam filter", addr, true) + if got.Status != StatusUnknown || got.Present { + t.Errorf("%q: status %q present=%v, want unknown and not present", addr, got.Status, got.Present) + } + if got.Detail == "" { + t.Errorf("%q: no detail", addr) + } + } + got := CheckMilter("Spam filter", "antispam:11332", true) + if want := "The address is not unix:/path or inet:host:port, so it cannot be checked."; got.Detail != want { + t.Errorf("detail = %q, want %q", got.Detail, want) + } +} + +// inet:[::1]:port is the form Postfix takes for an IPv6 address. +func TestCheckMilterInet6(t *testing.T) { + l, err := net.Listen("tcp", "[::1]:0") + if err != nil { + t.Skipf("IPv6 loopback unavailable here: %v", err) + } + defer l.Close() + _, port, _ := net.SplitHostPort(l.Addr().String()) + if _, err := strconv.Atoi(port); err != nil { + t.Fatal(err) + } + if got := CheckMilter("Spam filter", "inet:[::1]:"+port, true); got.Status != StatusOK { + t.Errorf("IPv6 listener: %+v", got) + } +} diff --git a/internal/health/socket.go b/internal/health/socket.go index 621ebfd..b9ec922 100644 --- a/internal/health/socket.go +++ b/internal/health/socket.go @@ -7,7 +7,10 @@ import ( // Socket is the state of one milter socket Postfix connects to. type Socket struct { - Name string + Name string + // Note qualifies the name where the socket is not one of the two Postfix + // always has ("inbound"); the Health table shows it beside the name. + Note string Path string Present bool Status Status diff --git a/internal/web/handlers/handlers_filter_socket_test.go b/internal/web/handlers/handlers_filter_socket_test.go new file mode 100644 index 0000000..b25cd1a --- /dev/null +++ b/internal/web/handlers/handlers_filter_socket_test.go @@ -0,0 +1,202 @@ +package handlers + +import ( + "net" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + + "github.com/mixeme/selfpost/internal/health" + "github.com/mixeme/selfpost/internal/web/auth" + "github.com/mixeme/selfpost/internal/web/view" +) + +// The inbound spam filter's socket is one of the milter sockets on Health and +// counts in the Milter sockets card of Overview — and only when the filter is on. + +// socketsServer is a server whose two Postfix milter sockets answer, so that +// every difference in the Milter sockets card comes from the filter. +func socketsServer(t *testing.T) *Handlers { + t.Helper() + h, _ := settingsServer(t) + h.machine = &health.MachineSampler{} + dir := t.TempDir() + h.cfg.OpenDKIMSocket = filepath.Join(dir, "opendkim.sock") + h.cfg.JournalSocket = filepath.Join(dir, "journal.sock") + for _, path := range []string{h.cfg.OpenDKIMSocket, h.cfg.JournalSocket} { + l, err := net.Listen("unix", path) + if err != nil { + t.Skipf("unix sockets unavailable here: %v", err) + } + t.Cleanup(func() { l.Close() }) + } + return h +} + +func socketsOf(t *testing.T, h *Handlers) []health.Socket { + t.Helper() + return h.healthChecks()["Sockets"].([]health.Socket) +} + +func socketCard(t *testing.T, h *Handlers) view.HealthCard { + t.Helper() + for _, c := range healthCards(h.healthChecks()) { + if c.Name == "Milter sockets" { + return c + } + } + t.Fatal("no Milter sockets card") + return view.HealthCard{} +} + +func TestFilterOffAddsNoSocket(t *testing.T) { + h := socketsServer(t) + h.cfg.InboundEnabled = true + h.cfg.InboundAntispamMilter = "" // no filter attached + if got := socketsOf(t, h); len(got) != 2 { + t.Fatalf("%d sockets with the filter off, want 2: %+v", len(got), got) + } + + // A milter address with the inbound relay off is not a filter either. + h.cfg.InboundEnabled = false + h.cfg.InboundAntispamMilter = "inet:127.0.0.1:1" + got := socketsOf(t, h) + if len(got) != 2 { + t.Fatalf("%d sockets with the inbound relay off, want 2: %+v", len(got), got) + } + card := socketCard(t, h) + if card.Value != "2 of 2 answering" || card.Sub != "OpenDKIM, send-log" || card.Level != "" { + t.Errorf("card = %+v", card) + } +} + +func TestFilterOnAnsweringIsTheThirdSocket(t *testing.T) { + h := socketsServer(t) + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("tcp unavailable here: %v", err) + } + defer l.Close() + h.cfg.InboundEnabled = true + h.cfg.InboundAntispamMilter, h.cfg.InboundAntispamAction = "inet:"+l.Addr().String(), "tempfail" + + got := socketsOf(t, h) + if len(got) != 3 || got[2].Name != "Spam filter" || got[2].Note != "inbound" || got[2].Path != "inet:"+l.Addr().String() { + t.Fatalf("sockets = %+v", got) + } + for _, s := range got { + if !s.Present || s.Status != health.StatusOK { + t.Errorf("%s: present=%v status=%q (%s)", s.Name, s.Present, s.Status, s.Detail) + } + } + card := socketCard(t, h) + if card.Value != "3 of 3 answering" || card.Sub != "OpenDKIM, send-log, Spam filter" || card.Level != "" { + t.Errorf("card = %+v", card) + } + + out := renderHealth(t, h, h.healthChecks(), "") + for _, want := range []string{ + `Spam filter · inbound`, "inet:" + l.Addr().String(), + } { + if !strings.Contains(out, want) { + t.Errorf("Health is missing %q", want) + } + } + if strings.Contains(out, "Listening") { + t.Error("a socket that answers repeats its detail beside the tag") + } +} + +// A filter that does not answer is a socket that is missing: an error when +// Postfix defers inbound mail while it is down (as for OpenDKIM), a warning when +// it accepts the mail unfiltered (as for the send log). +func TestFilterDownIsAMissingSocket(t *testing.T) { + h := socketsServer(t) + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Skipf("tcp unavailable here: %v", err) + } + dead := "inet:" + l.Addr().String() + l.Close() + h.cfg.InboundEnabled = true + + for _, tt := range []struct { + action, level, stamp, detail string + status health.Status + }{ + {"tempfail", "fail", "FAIL", "Inbound mail is deferred until it is back.", health.StatusError}, + {"accept", "warn", "WARN", "Inbound mail goes through unfiltered.", health.StatusWarn}, + {"", "warn", "WARN", "Inbound mail goes through unfiltered.", health.StatusWarn}, + } { + h.cfg.InboundAntispamMilter, h.cfg.InboundAntispamAction = dead, tt.action + got := socketsOf(t, h) + if len(got) != 3 { + t.Fatalf("action %q: %d sockets, want 3", tt.action, len(got)) + } + f := got[2] + refused := "Connection to " + l.Addr().String() + " refused. " + tt.detail + if f.Present || f.Status != tt.status || f.Detail != refused { + t.Errorf("action %q: filter = %+v, want status %q, detail %q", tt.action, f, tt.status, refused) + } + + c := h.healthChecks() + if c["SocketStatus"].(health.Status) != tt.status { + t.Errorf("action %q: SocketStatus = %q, want %q", tt.action, c["SocketStatus"], tt.status) + } + card := socketCard(t, h) + if card.Value != "2 of 3 answering" || card.Level != tt.level || !strings.HasSuffix(card.Sub, "Spam filter") { + t.Errorf("action %q: card = %+v", tt.action, card) + } + + // Health: the row is in the table with its detail under the path. + out := renderHealth(t, h, c, "") + if want := `` + dead + `
` + refused + ``; !strings.Contains(out, want) { + t.Errorf("action %q: Health is missing the path with its detail", tt.action) + } + + // Overview: the stamp takes the card's level and the lead names the card. + rec := httptest.NewRecorder() + h.HandleOverviewFragment(rec, reqAs("/overview/fragment", globalPrincipal)) + page := rec.Body.String() + for _, want := range []string{`>` + tt.stamp + `<`, `Milter sockets (2 of 3 answering)`} { + if !strings.Contains(page, want) { + t.Errorf("action %q: Overview is missing %q", tt.action, want) + } + } + } +} + +// Only the global administrator sees the address: Health and Overview are +// global-only, and a domain administrator gets none of them. +func TestFilterAddressIsGlobalOnly(t *testing.T) { + h, _ := settingsServer(t) + h.machine = &health.MachineSampler{} + h.cfg.InboundEnabled = true + h.cfg.InboundAntispamMilter, h.cfg.InboundAntispamAction = "inet:antispam-secret-name:11332", "accept" + domainAdmin := auth.Principal{ID: 2, Username: "dom", Role: auth.RoleDomain, Domains: []int64{1}} + + for name, handler := range map[string]func(*httptest.ResponseRecorder, *Handlers){ + "overview": func(w *httptest.ResponseRecorder, h *Handlers) { h.HandleOverview(w, reqAs("/overview", domainAdmin)) }, + "overview fragment": func(w *httptest.ResponseRecorder, h *Handlers) { + h.HandleOverviewFragment(w, reqAs("/overview/fragment", domainAdmin)) + }, + "health": func(w *httptest.ResponseRecorder, h *Handlers) { + h.HandleHealth(w, reqAs("/server/health", domainAdmin)) + }, + "health fragment": func(w *httptest.ResponseRecorder, h *Handlers) { + h.HandleHealthFragment(w, reqAs("/server/health/fragment", domainAdmin)) + }, + } { + rec := httptest.NewRecorder() + handler(rec, h) + if rec.Code == 200 || strings.Contains(rec.Body.String(), "antispam-secret-name") { + t.Errorf("%s answers a domain administrator with %d / carries the address", name, rec.Code) + } + } +} + +func reqAs(path string, p auth.Principal) *http.Request { + return auth.RequestWithPrincipal(httptest.NewRequest("GET", path, nil), p) +} diff --git a/internal/web/handlers/handlers_status.go b/internal/web/handlers/handlers_status.go index 7a2523b..65a0b10 100644 --- a/internal/web/handlers/handlers_status.go +++ b/internal/web/handlers/handlers_status.go @@ -167,7 +167,7 @@ func healthPage(m view.Meta, c map[string]any, flash string) *view.Health { page.Sockets.End = view.Verdict(string(c["SocketStatus"].(health.Status))) for _, s := range c["Sockets"].([]health.Socket) { - row := view.SocketRow{Name: s.Name, Path: s.Path, State: view.Tag{Status: string(s.Status)}} + row := view.SocketRow{Name: s.Name, Note: s.Note, Path: s.Path, State: view.Tag{Status: string(s.Status)}} if s.Status != health.StatusOK { row.Detail = s.Detail } @@ -351,6 +351,9 @@ func (h *Handlers) healthChecks() map[string]any { health.CheckSocket("OpenDKIM", h.cfg.OpenDKIMSocket, true), health.CheckSocket("send-log", h.cfg.JournalSocket, false), } + if on, milter, action := h.inboundFilter(); on { + sockets = append(sockets, filterSocket(milter, action)) + } socketStatus := health.StatusUnknown for _, sock := range sockets { socketStatus = health.Worst(socketStatus, sock.Status) @@ -375,6 +378,26 @@ func (h *Handlers) healthChecks() map[string]any { } } +// filterSocket is the inbound spam filter's row among the milter sockets. A +// filter that is down costs what the server's setting says: with the action +// tempfail Postfix defers inbound mail (an error, like OpenDKIM), otherwise it +// accepts the mail unfiltered (a warning, like the send log). The address is the +// one in INBOUND_ANTISPAM_MILTER as configured; Health and Overview, the only +// pages that show it, are for the global administrator. +func filterSocket(milter, action string) health.Socket { + tempfail := action == "tempfail" + s := health.CheckMilter("Spam filter", milter, tempfail) + s.Note = "inbound" + switch { + case s.Status != health.StatusError && s.Status != health.StatusWarn: + case tempfail: + s.Detail += " Inbound mail is deferred until it is back." + default: + s.Detail += " Inbound mail goes through unfiltered." + } + return s +} + func queueSummary(out string) string { lines := strings.Split(strings.TrimSpace(out), "\n") for i := len(lines) - 1; i >= 0; i-- {