Finish move to exterrors.SMTPError

Use of properly annotated errors is important since cf9e81d
to make logs useful.

endpoint/smtp/smtp.go

@@ -159,7 +159,7 @@ func (s *Session) Data(r io.Reader) error {
 	}
 
 	if s.endp.submission {
-		if err := SubmissionPrepare(s.msgMeta, header, s.endp.serv.Domain); err != nil {
+		if err := s.submissionPrepare(header); err != nil {
 			s.log.Error("DATA error", err)
 			return s.endp.wrapErr(s.msgMeta.ID, err)
 		}

endpoint/smtp/submission.go

@@ -7,38 +7,45 @@ import (
 	"time"
 
 	"github.com/emersion/go-message/textproto"
-	"github.com/emersion/go-smtp"
+	"github.com/foxcpp/maddy/exterrors"
-	"github.com/foxcpp/maddy/log"
-	"github.com/foxcpp/maddy/module"
 	"github.com/google/uuid"
 )
 
-func SubmissionPrepare(msgMeta *module.MsgMetadata, header textproto.Header, ourHostname string) error {
+func (s *Session) submissionPrepare(header textproto.Header) error {
-	msgMeta.DontTraceSender = true
+	s.msgMeta.DontTraceSender = true
 
 	if header.Get("Message-ID") == "" {
 		msgId, err := uuid.NewRandom()
 		if err != nil {
 			return errors.New("Message-ID generation failed")
 		}
-		// TODO: Move this function to smtpSession and use its logger.
+		s.log.Msg("adding missing Message-ID")
-		log.Debugf("adding missing Message-ID header to message from %s (%s)", msgMeta.SrcHostname, msgMeta.SrcAddr)
+		header.Set("Message-ID", "<"+msgId.String()+"@"+s.endp.serv.Domain+">")
-		header.Set("Message-ID", "<"+msgId.String()+"@"+ourHostname+">")
 	}
 
 	if header.Get("From") == "" {
-		return &smtp.SMTPError{
+		return &exterrors.SMTPError{
-			Code:    554,
+			Code:         554,
-			Message: "Message does not contains a From header field",
+			EnhancedCode: exterrors.EnhancedCode{5, 6, 0},
+			Message:      "Message does not contains a From header field",
+			Misc: map[string]interface{}{
+				"modifier": "submission_prepare",
+			},
 		}
 	}
 
 	for _, hdr := range [...]string{"Sender"} {
 		if value := header.Get(hdr); value != "" {
 			if _, err := mail.ParseAddress(value); err != nil {
-				return &smtp.SMTPError{
+				return &exterrors.SMTPError{
-					Code:    554,
+					Code:         554,
-					Message: fmt.Sprintf("Invalid address in %s: %v", hdr, err),
+					EnhancedCode: exterrors.EnhancedCode{5, 6, 0},
+					Message:      fmt.Sprintf("Invalid address in %s", hdr),
+					Misc: map[string]interface{}{
+						"modifier": "submission_prepare",
+						"addr":     value,
+					},
+					Err: err,
 				}
 			}
 		}
@@ -46,9 +53,15 @@ func SubmissionPrepare(msgMeta *module.MsgMetadata, header textproto.Header, our
 	for _, hdr := range [...]string{"To", "Cc", "Bcc", "Reply-To"} {
 		if value := header.Get(hdr); value != "" {
 			if _, err := mail.ParseAddressList(value); err != nil {
-				return &smtp.SMTPError{
+				return &exterrors.SMTPError{
-					Code:    554,
+					Code:         554,
-					Message: fmt.Sprintf("Invalid address in %s: %v", hdr, err),
+					EnhancedCode: exterrors.EnhancedCode{5, 6, 0},
+					Message:      fmt.Sprintf("Invalid address in %s", hdr),
+					Misc: map[string]interface{}{
+						"modifier": "submission_prepare",
+						"addr":     value,
+					},
+					Err: err,
 				}
 			}
 		}
@@ -56,31 +69,47 @@ func SubmissionPrepare(msgMeta *module.MsgMetadata, header textproto.Header, our
 
 	addrs, err := mail.ParseAddressList(header.Get("From"))
 	if err != nil {
-		return &smtp.SMTPError{
+		return &exterrors.SMTPError{
-			Code:    554,
+			Code:         554,
-			Message: fmt.Sprintf("Invalid address in From: %v", err),
+			EnhancedCode: exterrors.EnhancedCode{5, 6, 0},
+			Message:      fmt.Sprintf("Invalid address in From"),
+			Misc: map[string]interface{}{
+				"modifier": "submission_prepare",
+				"addr":     header.Get("From"),
+			},
+			Err: err,
 		}
 	}
 
 	// https://tools.ietf.org/html/rfc5322#section-3.6.2
 	// If From contains multiple addresses, Sender field must be present.
 	if len(addrs) > 1 && header.Get("Sender") == "" {
-		return &smtp.SMTPError{
+		return &exterrors.SMTPError{
-			Code:    554,
+			Code:         554,
-			Message: "Missing Sender header field",
+			EnhancedCode: exterrors.EnhancedCode{5, 6, 0},
+			Message:      "Missing Sender header field",
+			Misc: map[string]interface{}{
+				"modifier": "submission_prepare",
+				"from":     header.Get("From"),
+			},
 		}
 	}
 
 	if dateHdr := header.Get("Date"); dateHdr != "" {
 		_, err := time.Parse("Mon, 2 Jan 2006 15:04:05 -0700", dateHdr)
 		if err != nil {
-			return &smtp.SMTPError{
+			return &exterrors.SMTPError{
 				Code:    554,
 				Message: "Malformed Data header",
+				Misc: map[string]interface{}{
+					"modifier": "submission_prepare",
+					"date":     dateHdr,
+				},
+				Err: err,
 			}
 		}
 	} else {
-		log.Debugf("adding missing Date header to message from %s (%s)", msgMeta.SrcHostname, msgMeta.SrcAddr)
+		s.log.Msg("adding missing Date header")
 		header.Set("Date", time.Now().Format("Mon, 2 Jan 2006 15:04:05 -0700"))
 	}
 

log/log.go

@@ -97,7 +97,11 @@ func (l Logger) Error(msg string, err error, fields ...interface{}) {
 	}
 	sort.Strings(errKeys)
 
-	allFields = append(allFields, "reason", err.Error())
+	// If there is already a 'reason' field - use it, it probably
+	// provides a better explaination than error text itself.
+	if errFields["reason"] == nil {
+		allFields = append(allFields, "reason", err.Error())
+	}
 	for _, key := range errKeys {
 		allFields = append(allFields, key, errFields[key])
 	}

msgpipeline/config.go

@@ -5,10 +5,10 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/emersion/go-smtp"
 	"github.com/foxcpp/maddy/address"
 	"github.com/foxcpp/maddy/config"
 	modconfig "github.com/foxcpp/maddy/config/module"
+	"github.com/foxcpp/maddy/exterrors"
 	"github.com/foxcpp/maddy/modify"
 	"github.com/foxcpp/maddy/module"
 )
@@ -241,10 +241,10 @@ func parseMsgPipelineRcptCfg(globals map[string]interface{}, nodes []config.Node
 	return &rcpt, nil
 }
 
-func parseRejectDirective(node config.Node) (*smtp.SMTPError, error) {
+func parseRejectDirective(node config.Node) (*exterrors.SMTPError, error) {
 	code := 554
-	enchCode := smtp.EnhancedCode{5, 7, 0}
+	enchCode := exterrors.EnhancedCode{5, 7, 0}
-	msg := "Message rejected due to local policy"
+	msg := "Message rejected due to a local policy"
 	var err error
 	switch len(node.Args) {
 	case 3:
@@ -274,20 +274,23 @@ func parseRejectDirective(node config.Node) (*smtp.SMTPError, error) {
 	default:
 		return nil, config.NodeErr(&node, "invalid count of arguments")
 	}
-	return &smtp.SMTPError{
+	return &exterrors.SMTPError{
-		Message:      msg,
 		Code:         code,
 		EnhancedCode: enchCode,
+		Message:      msg,
+		Misc: map[string]interface{}{
+			"reason": "reject directive used",
+		},
 	}, nil
 }
 
-func parseEnhancedCode(s string) (smtp.EnhancedCode, error) {
+func parseEnhancedCode(s string) (exterrors.EnhancedCode, error) {
 	parts := strings.Split(s, ".")
 	if len(parts) != 3 {
-		return smtp.EnhancedCode{}, fmt.Errorf("wrong amount of enhanced code parts")
+		return exterrors.EnhancedCode{}, fmt.Errorf("wrong amount of enhanced code parts")
 	}
 
-	code := smtp.EnhancedCode{}
+	code := exterrors.EnhancedCode{}
 	for i, part := range parts {
 		num, err := strconv.Atoi(part)
 		if err != nil {

msgpipeline/config_test.go

@@ -5,15 +5,18 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/emersion/go-smtp"
 	"github.com/foxcpp/maddy/config/parser"
+	"github.com/foxcpp/maddy/exterrors"
 )
 
 func policyError(code int) error {
-	return &smtp.SMTPError{
+	return &exterrors.SMTPError{
-		Message:      "Message rejected due to local policy",
+		Message:      "Message rejected due to a local policy",
 		Code:         code,
-		EnhancedCode: smtp.EnhancedCode{5, 7, 0},
+		EnhancedCode: exterrors.EnhancedCode{5, 7, 0},
+		Misc: map[string]interface{}{
+			"reason": "reject directive used",
+		},
 	}
 }
 

msgpipeline/msgpipeline.go

@@ -8,6 +8,7 @@ import (
 	"github.com/foxcpp/maddy/address"
 	"github.com/foxcpp/maddy/buffer"
 	"github.com/foxcpp/maddy/config"
+	"github.com/foxcpp/maddy/exterrors"
 	"github.com/foxcpp/maddy/log"
 	"github.com/foxcpp/maddy/modify"
 	"github.com/foxcpp/maddy/module"
@@ -159,10 +160,14 @@ func (dd *msgpipelineDelivery) srcBlockForAddr(mailFrom string) (sourceBlock, er
 		// is not a valid RFC 282 address and only a special
 		// value for SMTP.
 		if err != nil && mailFrom != "" {
-			return sourceBlock{}, &smtp.SMTPError{
+			return sourceBlock{}, &exterrors.SMTPError{
 				Code:         501,
-				EnhancedCode: smtp.EnhancedCode{5, 1, 3},
+				EnhancedCode: exterrors.EnhancedCode{5, 1, 3},
-				Message:      "Invalid sender address: " + err.Error(),
+				Message:      "Invalid sender address",
+				Err:          err,
+				Misc: map[string]interface{}{
+					"reason": "Can't extract local-part and host-part",
+				},
 			}
 		}
 
@@ -418,10 +423,14 @@ func (dd *msgpipelineDelivery) rcptBlockForAddr(rcptTo string) (*rcptBlock, erro
 		// Then try domain-only.
 		_, domain, err := address.Split(rcptTo)
 		if err != nil {
-			return nil, &smtp.SMTPError{
+			return nil, &exterrors.SMTPError{
 				Code:         501,
-				EnhancedCode: smtp.EnhancedCode{5, 1, 3},
+				EnhancedCode: exterrors.EnhancedCode{5, 1, 3},
-				Message:      "Invalid recipient address: " + err.Error(),
+				Message:      "Invalid recipient address",
+				Err:          err,
+				Misc: map[string]interface{}{
+					"reason": "Can't extract local-part and host-part",
+				},
 			}
 		}
 
@@ -452,9 +461,8 @@ func (dd *msgpipelineDelivery) getRcptModifiers(rcptBlock *rcptBlock, rcptTo str
 
 	newSender, err := rcptModifiersState.RewriteSender(dd.sourceAddr)
 	if err == nil && newSender != dd.sourceAddr {
-		dd.log.Printf("Per-recipient modifier changed sender address. This is not supported and will "+
+		dd.log.Msg("Per-recipient modifier changed sender address. This is not supported and will "+
-			"be ignored. RCPT TO = %s, original MAIL FROM = %s, modified MAIL FROM = %s",
+			"be ignored.", "rcpt", rcptTo, "originalFrom", dd.sourceAddr, "modifiedFrom", newSender)
-			rcptTo, dd.sourceAddr, newSender)
 	}
 
 	dd.rcptModifiersState[rcptBlock] = rcptModifiersState

storage/sql/sql.go

@@ -18,13 +18,13 @@ import (
 	specialuse "github.com/emersion/go-imap-specialuse"
 	"github.com/emersion/go-imap/backend"
 	"github.com/emersion/go-message/textproto"
-	"github.com/emersion/go-smtp"
 	imapsql "github.com/foxcpp/go-imap-sql"
 	"github.com/foxcpp/maddy/address"
 	"github.com/foxcpp/maddy/auth"
 	"github.com/foxcpp/maddy/buffer"
 	"github.com/foxcpp/maddy/config"
 	"github.com/foxcpp/maddy/dns"
+	"github.com/foxcpp/maddy/exterrors"
 	"github.com/foxcpp/maddy/log"
 	"github.com/foxcpp/maddy/module"
 	"github.com/foxcpp/maddy/target"
@@ -65,10 +65,12 @@ func (d *delivery) AddRcpt(rcptTo string) error {
 		var err error
 		accountName, _, err = address.Split(rcptTo)
 		if err != nil {
-			return &smtp.SMTPError{
+			return &exterrors.SMTPError{
 				Code:         501,
-				EnhancedCode: smtp.EnhancedCode{5, 1, 3},
+				EnhancedCode: exterrors.EnhancedCode{5, 1, 3},
-				Message:      "Invalid recipient address: " + err.Error(),
+				Message:      "Invalid recipient address",
+				TargetName:   "sql",
+				Err:          err,
 			}
 		}
 	}
@@ -86,10 +88,12 @@ func (d *delivery) AddRcpt(rcptTo string) error {
 
 	if err := d.d.AddRcpt(strings.ToLower(accountName), userHeader); err != nil {
 		if err == imapsql.ErrUserDoesntExists || err == backend.ErrNoSuchMailbox {
-			return &smtp.SMTPError{
+			return &exterrors.SMTPError{
 				Code:         550,
-				EnhancedCode: smtp.EnhancedCode{5, 1, 1},
+				EnhancedCode: exterrors.EnhancedCode{5, 1, 1},
 				Message:      "User doesn't exist",
+				TargetName:   "sql",
+				Err:          err,
 			}
 		}
 		return err

target/smtp_downstream/sasl.go

@@ -2,8 +2,8 @@ package smtp_downstream
 
 import (
 	"github.com/emersion/go-sasl"
-	"github.com/emersion/go-smtp"
 	"github.com/foxcpp/maddy/config"
+	"github.com/foxcpp/maddy/exterrors"
 )
 
 type saslClientFactory func(downstreamUser, downstreamPass string) (sasl.Client, error)
@@ -29,10 +29,14 @@ func (u *Downstream) saslAuthDirective(m *config.Map, node *config.Node) (interf
 		return func(downstreamUser, downstreamPass string) (sasl.Client, error) {
 			if downstreamUser == "" || downstreamPass == "" {
 				u.log.Printf("client is not authenticated, can't forward credentials")
-				return nil, &smtp.SMTPError{
+				return nil, &exterrors.SMTPError{
 					Code:         530,
-					EnhancedCode: smtp.EnhancedCode{5, 7, 0},
+					EnhancedCode: exterrors.EnhancedCode{5, 7, 0},
 					Message:      "Authentication is required",
+					TargetName:   "smtp_downstream",
+					Misc: map[string]interface{}{
+						"reason": "Credentials forwarding is requested but the client is not authenticated",
+					},
 				}
 			}
 			// TODO: See if it is useful to support custom identity argument.