Remove check scoring system

It fits poorly with limited amount of checks that are (and will be)
implemented in maddy.

Advanced filtering that requires "spam score" logic should be performed
by external software such as rspamd. At this point duplicating that
logic in maddy makes no sense, since it is highly problematic to
integrate it with external software.

check/action.go

@@ -1,16 +1,13 @@
 package check
 
 import (
-	"strconv"
-
 	"github.com/foxcpp/maddy/config"
 	"github.com/foxcpp/maddy/module"
 )
 
 type FailAction struct {
-	Quarantine  bool
-	Reject      bool
-	ScoreAdjust int
+	Quarantine bool
+	Reject     bool
 }
 
 func FailActionDirective(m *config.Map, node *config.Node) (interface{}, error) {
@@ -30,17 +27,6 @@ func FailActionDirective(m *config.Map, node *config.Node) (interface{}, error)
 			Reject:     node.Args[0] == "reject",
 			Quarantine: node.Args[0] == "quarantine",
 		}, nil
-	case "score":
-		if len(node.Args) != 2 {
-			return nil, m.MatchErr("expected 2 arguments")
-		}
-		scoreAdj, err := strconv.Atoi(node.Args[1])
-		if err != nil {
-			return nil, m.MatchErr("%v", err)
-		}
-		return FailAction{
-			ScoreAdjust: scoreAdj,
-		}, nil
 	default:
 		return nil, m.MatchErr("invalid action")
 	}
@@ -53,8 +39,7 @@ func (cfa FailAction) Apply(originalRes module.CheckResult) module.CheckResult {
 		return originalRes
 	}
 
-	originalRes.Quarantine = cfa.Quarantine
-	originalRes.ScoreAdjust = int32(cfa.ScoreAdjust)
+	originalRes.Quarantine = cfa.Quarantine || originalRes.Quarantine
 	if !cfa.Reject {
 		originalRes.RejectErr = nil
 	}

check/dkim/dkim.go

@@ -27,7 +27,6 @@ type Check struct {
 	allowBodySubset bool
 	brokenSigAction check.FailAction
 	noSigAction     check.FailAction
-	okScore         int32
 }
 
 func New(_, instName string, _, inlineArgs []string) (module.Module, error) {
@@ -54,7 +53,6 @@ func (c *Check) Init(cfg *config.Map) error {
 		func() (interface{}, error) {
 			return check.FailAction{}, nil
 		}, check.FailActionDirective, &c.noSigAction)
-	cfg.Int32("ok_score", false, false, 0, &c.okScore)
 	_, err := cfg.Process()
 	if err != nil {
 		return err
@@ -179,7 +177,6 @@ func (d dkimCheckState) CheckBody(header textproto.Header, body buffer.Buffer) m
 	if !goodSigs {
 		return d.c.brokenSigAction.Apply(res)
 	}
-	res.ScoreAdjust = d.c.okScore
 	return res
 }
 

check/stateless_check.go

@@ -41,7 +41,6 @@ type statelessCheck struct {
 	defaultFailAction FailAction
 	// The actual fail action that should be applied.
 	failAction FailAction
-	okScore    int
 
 	connCheck   FuncConnCheck
 	senderCheck FuncSenderCheck
@@ -144,8 +143,7 @@ func (c *statelessCheck) InstanceName() string {
 // Note about CheckResult that is returned by the functions:
 // StatelessCheck supports different action types based on the user configuration, but the particular check
 // code doesn't need to know about it. It should assume that it is always "Reject" and hence it should
-// populate RejectErr field of the result object with the relevant error description. Fields ScoreAdjust and
-// Quarantine will be ignored.
+// populate RejectErr field of the result object with the relevant error description.
 func RegisterStatelessCheck(name string, defaultFailAction FailAction, connCheck FuncConnCheck, senderCheck FuncSenderCheck, rcptCheck FuncRcptCheck, bodyCheck FuncBodyCheck) {
 	module.Register(name, func(modName, instName string, aliases, inlineArgs []string) (module.Module, error) {
 		if len(inlineArgs) != 0 {

man/maddy-checks.5.scd

@@ -38,7 +38,7 @@ check {
 
 *maddy.conf*(5) for main man page.
 
-# ACTIONS & SCORING
+# ACTIONS
 
 When a certain check module thinks the message is "bad", it takes some actions depending
 on its configuration. Most checks follow the same configuration structure and allow
@@ -58,20 +58,6 @@ Mark message as 'quarantined'. If message is then delivered to the local storage
 backend can place the message in the 'Junk' mailbox. Another thing to keep in mind that
 'remote' module (see maddy-targets(5)) will refuse to send quarantined messages.
 
-- Adjust message score
-
-Message score is a integer that defines how likely the message is unwanted. Using message
-scores instead of direct actions (as above) allows to combine decisions of various checks
-and allow checks to override results of other checks.
-
-After all checks are executed, the message score is compared against two values.
-First one is 'quarantine score', if message score is higher than that value, it will 
-be quarantined (see above). Second one is 'reject score', if message score is higher than 
-that value, message will be rejected at connection time.  These values are configured
-using 'quarantine_score' and 'reject_score' directives in 'smtp' module 
-configuration (see maddy.conf(5)). Both values are undefined by default and so are
-not used.
-
 # COMMON DIRECTIVES
 
 All check modules have 'debug' directive to enable debug logging, just like most of maddy modules.
@@ -91,16 +77,6 @@ Reject the message instead of quarantining it.
 
 Quarantine the message, this is the default for most checks.
 
-- action score <number>
-
-Increase checks score by <number> (can be negative to decrease it).
-
-Additionally, most checks define the following configuration directive:
-
-- ok_score <number>
-
-Increase checks score by <number> if check passed (can be negative to decrease it).
-
 # DNS CHECKS
 
 ## require_matching_ehlo

module/check.go

@@ -53,10 +53,6 @@ type CheckResult struct {
 	// This value is copied into MsgMetadata by the msgpipeline.
 	Quarantine bool
 
-	// ScoreAdjust is the value that is added to the MsgMetadata.CheckScore
-	// by the msgpipeline after check execution.
-	ScoreAdjust int32
-
 	// AuthResult is the information that is supposed to
 	// be included in Authentication-Results header.
 	AuthResult []authres.Result

msgpipeline/check_runner.go

@@ -1,13 +1,10 @@
 package msgpipeline
 
 import (
-	"fmt"
 	"sync"
-	"sync/atomic"
 
 	"github.com/emersion/go-message/textproto"
 	"github.com/emersion/go-msgauth/authres"
-	"github.com/emersion/go-smtp"
 	"github.com/foxcpp/maddy/atomicbool"
 	"github.com/foxcpp/maddy/buffer"
 	"github.com/foxcpp/maddy/log"
@@ -15,16 +12,12 @@ import (
 )
 
 // checkRunner runs groups of checks, collects and merges results.
-// It takes care of quarantine/reject scores.
 // It also makes sure that each check gets only one state object created.
 type checkRunner struct {
 	msgMeta      *module.MsgMetadata
 	mailFrom     string
 	checkedRcpts []string
 
-	quarantineScore *int
-	rejectScore     *int
-
 	log log.Logger
 
 	states map[module.Check]module.CheckState
@@ -32,13 +25,11 @@ type checkRunner struct {
 	mergedRes module.CheckResult
 }
 
-func newCheckRunner(msgMeta *module.MsgMetadata, log log.Logger, quarantineScore, rejectScore *int) *checkRunner {
+func newCheckRunner(msgMeta *module.MsgMetadata, log log.Logger) *checkRunner {
 	return &checkRunner{
-		msgMeta:         msgMeta,
-		quarantineScore: quarantineScore,
-		rejectScore:     rejectScore,
-		log:             log,
-		states:          make(map[module.Check]module.CheckState),
+		msgMeta: msgMeta,
+		log:     log,
+		states:  make(map[module.Check]module.CheckState),
 	}
 }
 
@@ -122,7 +113,6 @@ func (cr *checkRunner) checkStates(checks []module.Check) ([]module.CheckState,
 
 func (cr *checkRunner) runAndMergeResults(states []module.CheckState, runner func(module.CheckState) module.CheckResult) error {
 	data := struct {
-		checkScore     int32
 		quarantineFlag atomicbool.AtomicBool
 		authResLock    sync.Mutex
 		headerLock     sync.Mutex
@@ -153,9 +143,6 @@ func (cr *checkRunner) runAndMergeResults(states []module.CheckState, runner fun
 				data.headerLock.Unlock()
 			}
 
-			if subCheckRes.ScoreAdjust != 0 {
-				atomic.AddInt32(&data.checkScore, subCheckRes.ScoreAdjust)
-			}
 			if subCheckRes.Quarantine {
 				data.quarantineFlag.Set(true)
 			}
@@ -175,7 +162,6 @@ func (cr *checkRunner) runAndMergeResults(states []module.CheckState, runner fun
 	if data.quarantineFlag.IsSet() {
 		cr.mergedRes.Quarantine = true
 	}
-	cr.mergedRes.ScoreAdjust += data.checkScore
 
 	return nil
 }
@@ -236,23 +222,6 @@ func (cr *checkRunner) applyResults(hostname string, header *textproto.Header) e
 		cr.msgMeta.Quarantine = true
 	}
 
-	checkScore := cr.mergedRes.ScoreAdjust
-
-	if cr.rejectScore != nil && checkScore >= int32(*cr.rejectScore) {
-		cr.log.Debugf("score %d >= %d, rejecting", checkScore, *cr.rejectScore)
-		return &smtp.SMTPError{
-			Code:         550,
-			EnhancedCode: smtp.EnhancedCode{5, 7, 0},
-			Message:      fmt.Sprintf("Message is rejected due to multiple local policy violations (score %d)", checkScore),
-		}
-	}
-	if cr.quarantineScore != nil && checkScore >= int32(*cr.quarantineScore) {
-		if !cr.msgMeta.Quarantine {
-			cr.log.Printf("quarantined message due to score %d >= %d", checkScore, *cr.quarantineScore)
-		}
-		cr.msgMeta.Quarantine = true
-	}
-
 	// After results for all checks are checked, authRes will be populated with values
 	// we should put into Authentication-Results header.
 	if len(cr.mergedRes.AuthResult) != 0 {

msgpipeline/check_test.go

@@ -10,13 +10,9 @@ import (
 	"github.com/foxcpp/maddy/testutils"
 )
 
-func TestMsgPipeline_NoScoresChecked(t *testing.T) {
+func TestMsgPipeline_Checks(t *testing.T) {
 	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
+	check1, check2 := testutils.Check{}, testutils.Check{}
 	d := MsgPipeline{
 		msgpipelineCfg: msgpipelineCfg{
 			globalChecks: []module.Check{&check1, &check2},
@@ -31,81 +27,6 @@ func TestMsgPipeline_NoScoresChecked(t *testing.T) {
 		Log: testutils.Logger(t, "msgpipeline"),
 	}
 
-	// No rejectScore or quarantineScore.
-	testutils.DoTestDelivery(t, &d, "whatever@whatever", []string{"whatever@whatever"})
-
-	if len(target.Messages) != 1 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 1, len(target.Messages))
-	}
-	if target.Messages[0].MsgMeta.Quarantine {
-		t.Fatalf("message is quarantined when it shouldn't")
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
-func TestMsgPipeline_RejectScore(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
-	rejectScore := 10
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-			rejectScore: &rejectScore,
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
-	// Should be rejected.
-	if _, err := testutils.DoTestDeliveryErr(t, &d, "whatever@whatever", []string{"whatever@whatever"}); err == nil {
-		t.Fatalf("expected an error")
-	}
-
-	if len(target.Messages) != 0 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 0, len(target.Messages))
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
-func TestMsgPipeline_RejectScore_notEnough(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
-	rejectScore := 15
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-			rejectScore: &rejectScore,
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
 	testutils.DoTestDelivery(t, &d, "whatever@whatever", []string{"whatever@whatever"})
 
 	if len(target.Messages) != 1 {
@@ -120,197 +41,6 @@ func TestMsgPipeline_RejectScore_notEnough(t *testing.T) {
 	}
 }
 
-func TestMsgPipeline_Quarantine(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{Quarantine: true},
-	}
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
-	// Should be quarantined.
-	testutils.DoTestDelivery(t, &d, "whatever@whatever", []string{"whatever@whatever"})
-
-	if len(target.Messages) != 1 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 1, len(target.Messages))
-	}
-	if !target.Messages[0].MsgMeta.Quarantine {
-		t.Fatalf("message is not quarantined when it should")
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
-func TestMsgPipeline_QuarantineScore(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
-	quarantineScore := 10
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-			quarantineScore: &quarantineScore,
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
-	// Should be quarantined.
-	testutils.DoTestDelivery(t, &d, "whatever@whatever", []string{"whatever@whatever"})
-
-	if len(target.Messages) != 1 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 1, len(target.Messages))
-	}
-	if !target.Messages[0].MsgMeta.Quarantine {
-		t.Fatalf("message is not quarantined when it should")
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
-func TestMsgPipeline_QuarantineScore_notEnough(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
-	quarantineScore := 15
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-			quarantineScore: &quarantineScore,
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
-	// Should be quarantined.
-	testutils.DoTestDelivery(t, &d, "whatever@whatever", []string{"whatever@whatever"})
-
-	if len(target.Messages) != 1 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 1, len(target.Messages))
-	}
-	if target.Messages[0].MsgMeta.Quarantine {
-		t.Fatalf("message is quarantined when it shouldn't")
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
-func TestMsgPipeline_BothScores_Quarantined(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
-	quarantineScore := 10
-	rejectScore := 15
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-			quarantineScore: &quarantineScore,
-			rejectScore:     &rejectScore,
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
-	// Should be quarantined.
-	testutils.DoTestDelivery(t, &d, "whatever@whatever", []string{"whatever@whatever"})
-
-	if len(target.Messages) != 1 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 1, len(target.Messages))
-	}
-	if !target.Messages[0].MsgMeta.Quarantine {
-		t.Fatalf("message is not quarantined when it should")
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
-func TestMsgPipeline_BothScores_Rejected(t *testing.T) {
-	target := testutils.Target{}
-	check1, check2 := testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}, testutils.Check{
-		BodyRes: module.CheckResult{ScoreAdjust: 5},
-	}
-	quarantineScore := 5
-	rejectScore := 10
-	d := MsgPipeline{
-		msgpipelineCfg: msgpipelineCfg{
-			globalChecks: []module.Check{&check1, &check2},
-			perSource:    map[string]sourceBlock{},
-			defaultSource: sourceBlock{
-				perRcpt: map[string]*rcptBlock{},
-				defaultRcpt: &rcptBlock{
-					targets: []module.DeliveryTarget{&target},
-				},
-			},
-			quarantineScore: &quarantineScore,
-			rejectScore:     &rejectScore,
-		},
-		Log: testutils.Logger(t, "msgpipeline"),
-	}
-
-	// Should be quarantined.
-	if _, err := testutils.DoTestDeliveryErr(t, &d, "whatever@whatever", []string{"whatever@whatever"}); err == nil {
-		t.Fatalf("message not rejected")
-	}
-
-	if len(target.Messages) != 0 {
-		t.Fatalf("wrong amount of messages received, want %d, got %d", 0, len(target.Messages))
-	}
-
-	if check1.UnclosedStates != 0 || check2.UnclosedStates != 0 {
-		t.Fatalf("checks state objects leak or double-closed, alive counters: %v, %v", check1.UnclosedStates, check2.UnclosedStates)
-	}
-}
-
 func TestMsgPipeline_AuthResults(t *testing.T) {
 	target := testutils.Target{}
 	check1, check2 := testutils.Check{

msgpipeline/config.go

@@ -18,14 +18,6 @@ type msgpipelineCfg struct {
 	globalModifiers modify.Group
 	perSource       map[string]sourceBlock
 	defaultSource   sourceBlock
-
-	// If MsgMeta.CheckScore is higher than that value,
-	// message will be rejected.
-	rejectScore *int
-
-	// If MsgMeta.CheckScore is higher than that value,
-	// MsgMeta.Quarantine will be set.
-	quarantineScore *int
 }
 
 func parseMsgPipelineRootCfg(globals map[string]interface{}, nodes []config.Node) (msgpipelineCfg, error) {
@@ -78,26 +70,6 @@ func parseMsgPipelineRootCfg(globals map[string]interface{}, nodes []config.Node
 				return msgpipelineCfg{}, config.NodeErr(&node, "duplicate 'default_source' block")
 			}
 			defaultSrcRaw = node.Children
-		case "quarantine_score":
-			if len(node.Args) != 1 {
-				return msgpipelineCfg{}, config.NodeErr(&node, "exactly one argument required")
-			}
-
-			quarantineScore, err := strconv.Atoi(node.Args[0])
-			if err != nil {
-				return msgpipelineCfg{}, config.NodeErr(&node, "%v", err)
-			}
-			cfg.quarantineScore = &quarantineScore
-		case "reject_score":
-			if len(node.Args) != 1 {
-				return msgpipelineCfg{}, config.NodeErr(&node, "exactly one argument required")
-			}
-
-			rejectScore, err := strconv.Atoi(node.Args[0])
-			if err != nil {
-				return msgpipelineCfg{}, config.NodeErr(&node, "%v", err)
-			}
-			cfg.rejectScore = &rejectScore
 		default:
 			othersRaw = append(othersRaw, node)
 		}

msgpipeline/msgpipeline.go

@@ -64,7 +64,7 @@ func (d *MsgPipeline) Start(msgMeta *module.MsgMetadata, mailFrom string) (modul
 		msgMeta:            msgMeta,
 		log:                target.DeliveryLogger(d.Log, msgMeta),
 	}
-	dd.checkRunner = newCheckRunner(msgMeta, dd.log, d.quarantineScore, d.rejectScore)
+	dd.checkRunner = newCheckRunner(msgMeta, dd.log)
 
 	if msgMeta.OriginalRcpts == nil {
 		msgMeta.OriginalRcpts = map[string]string{}