check/milter: apply fail_open to milter session-dial failures

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()
 }
 

internal/check/milter/milter_test.go

@@ -19,9 +19,15 @@ along with this program.  If not, see <https://www.gnu.org/licenses/>.
 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)
+	}
+}