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

@@ -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

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