From 15dbd14cbaf64f8b062884d9a918eb394ab3b45d Mon Sep 17 00:00:00 2001 From: Marco Minetti Date: 2026年9月10日 10:04:02 +0000 Subject: [PATCH] check/milter: apply fail_open to milter session-dial failures CheckStateForMsg dials the milter backend directly and returned any dial error unconditionally, before a *state existed to route the failure through ioError()'s fail_open handling. This made fail_open silently ineffective for the specific case of the milter being unreachable: the original SMTP sender got a hard reject regardless of the directive. Handle a dial failure the same way ioError() handles a later I/O failure: skip checks and let the message through when fail_open is set, otherwise keep today's behavior and reject. Guard CheckConnection and Close() for the resulting sessionless state. --- internal/check/milter/milter.go | 23 ++++++++ internal/check/milter/milter_test.go | 80 ++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/internal/check/milter/milter.go b/internal/check/milter/milter.go index 2a6d6e9e7..02923be59 100644 --- a/internal/check/milter/milter.go +++ b/internal/check/milter/milter.go @@ -119,6 +119,22 @@ type state struct { func (c *Check) CheckStateForMsg(ctx context.Context, msgMeta *module.MsgMetadata) (module.CheckState, error) { session, err := c.cl.Session() if err != nil { + // fail_open is meant to apply to any I/O failure talking to the + // milter, but a failure to even establish the session (e.g. the + // milter is down or unreachable) happens before a *state exists to + // route through ioError, so it has to be handled here explicitly - + // otherwise fail_open is silently ineffective for this specific + // failure mode and the message is hard-rejected regardless of the + // directive. + if c.failOpen { + c.log.Error("I/O error, skipping checks", err) + return &state{ + c: c, + msgMeta: msgMeta, + skipChecks: true, + log: target.DeliveryLogger(c.log, msgMeta), + }, nil + } return nil, err } return &state{ @@ -226,6 +242,10 @@ func (s *state) apply(modifyActs []milter.ModifyAction, res module.CheckResult) } func (s *state) CheckConnection(ctx context.Context) module.CheckResult { + if s.skipChecks { + return module.CheckResult{} + } + if s.msgMeta.Conn == nil { // Submit some dummy values as the message is likely generated locally. @@ -437,6 +457,9 @@ func (s *state) CheckBody(ctx context.Context, header textproto.Header, body buf } func (s *state) Close() error { + if s.session == nil { + return nil + } return s.session.Close() } diff --git a/internal/check/milter/milter_test.go b/internal/check/milter/milter_test.go index 50e3954e9..8c4df0182 100644 --- a/internal/check/milter/milter_test.go +++ b/internal/check/milter/milter_test.go @@ -19,9 +19,15 @@ along with this program. If not, see . package milter import ( + "context" + "net" "testing" + "time" + "github.com/emersion/go-milter" "github.com/foxcpp/maddy/framework/config" + "github.com/foxcpp/maddy/framework/log" + "github.com/foxcpp/maddy/framework/module" ) func TestAcceptValidEndpoints(t *testing.T) { @@ -59,3 +65,77 @@ func TestRejectInvalidEndpoints(t *testing.T) { } } } + +// unreachableAddr returns a loopback TCP address that is guaranteed to +// refuse connections: it binds a listener to get a free port, then closes +// it immediately, so dialing it fails fast with ECONNREFUSED instead of +// waiting out a dial timeout. +func unreachableAddr(t *testing.T) string { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to allocate a port to make unreachable: %v", err) + } + addr := l.Addr().String() + if err := l.Close(); err != nil { + t.Fatalf("failed to close listener: %v", err) + } + return addr +} + +func newUnreachableCheck(t *testing.T, failOpen bool) *Check { + t.Helper() + return &Check{ + failOpen: failOpen, + log: &log.Logger{Out: log.NopOutput{}}, + cl: milter.NewClientWithOptions("tcp", unreachableAddr(t), milter.ClientOptions{ + Dialer: &net.Dialer{Timeout: 2 * time.Second}, + ReadTimeout: 2 * time.Second, + WriteTimeout: 2 * time.Second, + }), + } +} + +// TestCheckStateForMsg_DialFailure_FailOpenFalse locks in the existing, +// correct behavior: without fail_open, an unreachable milter must still +// hard-reject at state-creation time. +func TestCheckStateForMsg_DialFailure_FailOpenFalse(t *testing.T) { + c := newUnreachableCheck(t, false) + _, err := c.CheckStateForMsg(context.Background(), &module.MsgMetadata{ID: "test"}) + if err == nil { + t.Fatal("expected an error when the milter is unreachable and fail_open is false, got nil") + } +} + +// TestCheckStateForMsg_DialFailure_FailOpenTrue reproduces the fail_open +// gap: a dial failure while establishing the milter session used to bypass +// fail_open entirely (CheckStateForMsg returned the raw dial error +// unconditionally), hard-rejecting the message despite fail_open being set. +// It must instead behave like ioError() does for a later I/O failure: let +// the message through unchecked. +func TestCheckStateForMsg_DialFailure_FailOpenTrue(t *testing.T) { + c := newUnreachableCheck(t, true) + + st, err := c.CheckStateForMsg(context.Background(), &module.MsgMetadata{ID: "test"}) + if err != nil { + t.Fatalf("expected fail_open to let the message through despite the unreachable milter, got error: %v", err) + } + if st == nil { + t.Fatal("expected a non-nil check state with fail_open, got nil") + } + defer func() { + if err := st.Close(); err != nil { + t.Errorf("Close: expected nil error on a sessionless (dial-failed) state, got: %v", err) + } + }() + + if res := st.CheckConnection(context.Background()); res.Reject { + t.Errorf("CheckConnection: expected no rejection with fail_open after a dial failure, got Reject=true, reason=%v", res.Reason) + } + if res := st.CheckSender(context.Background(), "sender@example.org"); res.Reject { + t.Errorf("CheckSender: expected no rejection with fail_open after a dial failure, got Reject=true, reason=%v", res.Reason) + } + if res := st.CheckRcpt(context.Background(), "rcpt@example.org"); res.Reject { + t.Errorf("CheckRcpt: expected no rejection with fail_open after a dial failure, got Reject=true, reason=%v", res.Reason) + } +}

AltStyle によって変換されたページ (->オリジナル) /