From d83c1dc5916c8b26f50154666d90b0e1dc2e288c Mon Sep 17 00:00:00 2001
From: Alberto Bertogli <albertito@blitiri.com.ar>
Date: Sat, 23 May 2020 01:02:07 +0100
Subject: [PATCH] smtpsrv: Fix error code on transient authentication issues

When we can't authenticate due to a transient issue, for example if we
rely on Dovecot and it is not responding, we should use a differentiated
error code to avoid confusing users.

However, today we return the same error code as when the user enters the
wrong password, which could confuse users as their MUA might think their
credentials are no longer valid.

This patch fixes the issue by returning a differentiated error code in
that case, as per RFC 4954.

Thanks to Max Mazurov (fox.cpp@disroot.org) for reporting this problem.
---
 internal/smtpsrv/conn.go        |  3 +++
 internal/smtpsrv/server_test.go | 48 +++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/internal/smtpsrv/conn.go b/internal/smtpsrv/conn.go
index 611f4b9..eef67d4 100644
--- a/internal/smtpsrv/conn.go
+++ b/internal/smtpsrv/conn.go
@@ -1004,9 +1004,12 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
 		return 501, fmt.Sprintf("5.5.2 Error decoding AUTH response: %v", err)
 	}
 
+	// https://tools.ietf.org/html/rfc4954#section-6
 	authOk, err := c.authr.Authenticate(user, domain, passwd)
 	if err != nil {
 		c.tr.Errorf("error authenticating %q@%q: %v", user, domain, err)
+		maillog.Auth(c.conn.RemoteAddr(), user+"@"+domain, false)
+		return 454, "4.7.0 Temporary authentication failure"
 	}
 	if authOk {
 		c.authUser = user
diff --git a/internal/smtpsrv/server_test.go b/internal/smtpsrv/server_test.go
index 557b52a..381fada 100644
--- a/internal/smtpsrv/server_test.go
+++ b/internal/smtpsrv/server_test.go
@@ -200,6 +200,19 @@ func TestAuthOnSMTP(t *testing.T) {
 	sendEmailWithAuth(t, c, auth)
 }
 
+func TestBrokenAuth(t *testing.T) {
+	c := mustDial(t, ModeSubmission, true)
+	defer c.Close()
+
+	auth := smtp.PlainAuth("", "user@broken", "passwd", "127.0.0.1")
+	err := c.Auth(auth)
+	if err == nil {
+		t.Errorf("Broken auth succeeded")
+	} else if err.Error() != "454 4.7.0 Temporary authentication failure" {
+		t.Errorf("Broken auth returned unexpected error %q", err.Error())
+	}
+}
+
 func TestWrongMailParsing(t *testing.T) {
 	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
@@ -295,6 +308,24 @@ func TestTooManyRecipients(t *testing.T) {
 	}
 }
 
+func TestRcptFailsExistsCheck(t *testing.T) {
+	c := mustDial(t, ModeSMTP, true)
+	defer c.Close()
+
+	if err := c.Mail("from@localhost"); err != nil {
+		t.Fatalf("Mail: %v", err)
+	}
+
+	err := c.Rcpt("to@broken")
+	if err == nil {
+		t.Errorf("Accepted RCPT with broken Exists")
+	}
+	expect := "550 5.1.1 Destination address is unknown (user does not exist)"
+	if err.Error() != expect {
+		t.Errorf("RCPT returned unexpected error %q", err.Error())
+	}
+}
+
 var str1MiB string
 
 func sendLargeEmail(tb testing.TB, c *smtp.Client, sizeMiB int) error {
@@ -555,6 +586,20 @@ func waitForServer(addr string) error {
 	return fmt.Errorf("not reachable")
 }
 
+type brokenAuthBE struct{}
+
+func (b brokenAuthBE) Authenticate(user, password string) (bool, error) {
+	return false, fmt.Errorf("failed to auth")
+}
+
+func (b brokenAuthBE) Exists(user string) (bool, error) {
+	return false, fmt.Errorf("failed to check if user exists")
+}
+
+func (b brokenAuthBE) Reload() error {
+	return fmt.Errorf("failed to reload")
+}
+
 // realMain is the real main function, which returns the value to pass to
 // os.Exit(). We have to do this so we can use defer.
 func realMain(m *testing.M) int {
@@ -615,6 +660,9 @@ func realMain(m *testing.M) int {
 		s.AddDomain("localhost")
 		s.AddUserDB("localhost", udb)
 
+		s.AddDomain("broken")
+		s.authr.Register("broken", &brokenAuthBE{})
+
 		// Disable SPF lookups, to avoid leaking DNS queries.
 		disableSPFForTesting = true