Merge pull request #312 from wneessen/bug/311_smtp-auth-login-should-…

…follow-ietf-draft-more-closely

Enhance SMTP LOGIN auth and add comprehensive tests

client_test.go

@@ -620,7 +620,7 @@ func TestClient_DialWithContext(t *testing.T) {
 		t.Skipf("failed to create test client: %s. Skipping tests", err)
 	}
 	ctx := context.Background()
-	if err := c.DialWithContext(ctx); err != nil {
+	if err = c.DialWithContext(ctx); err != nil {
 		t.Errorf("failed to dial with context: %s", err)
 		return
 	}
@@ -1872,6 +1872,119 @@ func TestClient_AuthSCRAMSHAX(t *testing.T) {
 	}
 }
 
+func TestClient_AuthLoginSuccess(t *testing.T) {
+	tests := []struct {
+		name       string
+		featureSet string
+	}{
+		{"default", "250-AUTH LOGIN\r\n250-X-DEFAULT-LOGIN\r\n250-8BITMIME\r\n250-DSN\r\n250 SMTPUTF8"},
+		{"mox server", "250-AUTH LOGIN\r\n250-X-MOX-LOGIN\r\n250-8BITMIME\r\n250-DSN\r\n250 SMTPUTF8"},
+		{"null byte", "250-AUTH LOGIN\r\n250-X-NULLBYTE-LOGIN\r\n250-8BITMIME\r\n250-DSN\r\n250 SMTPUTF8"},
+		{"bogus responses", "250-AUTH LOGIN\r\n250-X-BOGUS-LOGIN\r\n250-8BITMIME\r\n250-DSN\r\n250 SMTPUTF8"},
+		{"empty responses", "250-AUTH LOGIN\r\n250-X-EMPTY-LOGIN\r\n250-8BITMIME\r\n250-DSN\r\n250 SMTPUTF8"},
+	}
+
+	for i, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			serverPort := TestServerPortBase + 40 + i
+			go func() {
+				if err := simpleSMTPServer(ctx, tt.featureSet, true, serverPort); err != nil {
+					t.Errorf("failed to start test server: %s", err)
+					return
+				}
+			}()
+			time.Sleep(time.Millisecond * 300)
+
+			client, err := NewClient(TestServerAddr,
+				WithPort(serverPort),
+				WithTLSPortPolicy(NoTLS),
+				WithSMTPAuth(SMTPAuthLogin),
+				WithUsername("[email protected]"),
+				WithPassword("V3ryS3cr3t+"))
+			if err != nil {
+				t.Errorf("unable to create new client: %s", err)
+				return
+			}
+			if err = client.DialWithContext(context.Background()); err != nil {
+				t.Errorf("failed to dial to test server: %s", err)
+			}
+			if err = client.Close(); err != nil {
+				t.Errorf("failed to close server connection: %s", err)
+			}
+		})
+	}
+}
+
+func TestClient_AuthLoginFail(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	serverPort := TestServerPortBase + 50
+	featureSet := "250-AUTH LOGIN\r\n250-X-DEFAULT-LOGIN\r\n250-8BITMIME\r\n250-DSN\r\n250 SMTPUTF8"
+	go func() {
+		if err := simpleSMTPServer(ctx, featureSet, true, serverPort); err != nil {
+			t.Errorf("failed to start test server: %s", err)
+			return
+		}
+	}()
+	time.Sleep(time.Millisecond * 300)
+
+	client, err := NewClient(TestServerAddr,
+		WithPort(serverPort),
+		WithTLSPortPolicy(NoTLS),
+		WithSMTPAuth(SMTPAuthLogin),
+		WithUsername("[email protected]"),
+		WithPassword("InvalidPassword"))
+	if err != nil {
+		t.Errorf("unable to create new client: %s", err)
+		return
+	}
+	if err = client.DialWithContext(context.Background()); err == nil {
+		t.Error("expected to fail to dial to test server, but it succeeded")
+	}
+}
+
+func TestClient_AuthLoginFail_noTLS(t *testing.T) {
+	if os.Getenv("TEST_SKIP_ONLINE") != "" {
+		t.Skipf("env variable TEST_SKIP_ONLINE is set. Skipping online tests")
+	}
+	th := os.Getenv("TEST_HOST")
+	if th == "" {
+		t.Skipf("no host set. Skipping online tests")
+	}
+	tp := 587
+	if tps := os.Getenv("TEST_PORT"); tps != "" {
+		tpi, err := strconv.Atoi(tps)
+		if err == nil {
+			tp = tpi
+		}
+	}
+	client, err := NewClient(th, WithPort(tp), WithSMTPAuth(SMTPAuthLogin), WithTLSPolicy(NoTLS))
+	if err != nil {
+		t.Errorf("failed to create new client: %s", err)
+	}
+	u := os.Getenv("TEST_SMTPAUTH_USER")
+	if u != "" {
+		client.SetUsername(u)
+	}
+	p := os.Getenv("TEST_SMTPAUTH_PASS")
+	if p != "" {
+		client.SetPassword(p)
+	}
+	// We don't want to log authentication data in tests
+	client.SetDebugLog(false)
+
+	if err = client.DialWithContext(context.Background()); err == nil {
+		t.Error("expected to fail to dial to test server, but it succeeded")
+	}
+	if !errors.Is(err, smtp.ErrUnencrypted) {
+		t.Errorf("expected error to be %s, but got %s", smtp.ErrUnencrypted, err)
+	}
+}
+
 func TestClient_AuthSCRAMSHAX_fail(t *testing.T) {
 	if os.Getenv("TEST_ONLINE_SCRAM") == "" {
 		t.Skipf("TEST_ONLINE_SCRAM is not set. Skipping online SCRAM tests")
@@ -2491,6 +2604,51 @@ func handleTestServerConnection(connection net.Conn, featureSet string, failRese
 				break
 			}
 			_ = writeLine("235 2.7.0 Authentication successful")
+		case strings.HasPrefix(data, "AUTH LOGIN"):
+			var username, password string
+			userResp := "VXNlcm5hbWU6"
+			passResp := "UGFzc3dvcmQ6"
+			if strings.Contains(featureSet, "250-X-MOX-LOGIN") {
+				userResp = ""
+				passResp = "UGFzc3dvcmQ="
+			}
+			if strings.Contains(featureSet, "250-X-NULLBYTE-LOGIN") {
+				userResp = "VXNlciBuYW1lAA=="
+				passResp = "UGFzc3dvcmQA"
+			}
+			if strings.Contains(featureSet, "250-X-BOGUS-LOGIN") {
+				userResp = "Qm9ndXM="
+				passResp = "Qm9ndXM="
+			}
+			if strings.Contains(featureSet, "250-X-EMPTY-LOGIN") {
+				userResp = ""
+				passResp = ""
+			}
+			_ = writeLine("334 " + userResp)
+
+			ddata, derr := reader.ReadString('\n')
+			if derr != nil {
+				fmt.Printf("failed to read username data from connection: %s\n", derr)
+				break
+			}
+			ddata = strings.TrimSpace(ddata)
+			username = ddata
+			_ = writeLine("334 " + passResp)
+
+			ddata, derr = reader.ReadString('\n')
+			if derr != nil {
+				fmt.Printf("failed to read password data from connection: %s\n", derr)
+				break
+			}
+			ddata = strings.TrimSpace(ddata)
+			password = ddata
+
+			if !strings.EqualFold(username, "dG9uaUB0ZXN0ZXIuY29t") ||
+				!strings.EqualFold(password, "VjNyeVMzY3IzdCs=") {
+				_ = writeLine("535 5.7.8 Error: authentication failed")
+				break
+			}
+			_ = writeLine("235 2.7.0 Authentication successful")
 		case strings.EqualFold(data, "DATA"):
 			_ = writeLine("354 End data with <CR><LF>.<CR><LF>")
 			for {

smtp/auth_login.go

@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: Copyright (c) 2022-2023 The go-mail Authors
+// SPDX-FileCopyrightText: Copyright (c) 2022-2024 The go-mail Authors
 //
 // SPDX-License-Identifier: MIT
 
@@ -9,78 +9,68 @@ import (
 	"fmt"
 )
 
+// ErrUnencrypted is an error indicating that the connection is not encrypted.
+var ErrUnencrypted = errors.New("unencrypted connection")
+
 // loginAuth is the type that satisfies the Auth interface for the "SMTP LOGIN" auth
 type loginAuth struct {
 	username, password string
 	host               string
+	respStep           uint8
 }
 
-const (
-	// LoginXUsernameChallenge represents the Username Challenge response sent by the SMTP server per the AUTH LOGIN
-	// extension.
-	//
-	// See: https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-xlogin/.
-	LoginXUsernameChallenge      = "Username:"
-	LoginXUsernameLowerChallenge = "username:"
-
-	// LoginXPasswordChallenge represents the Password Challenge response sent by the SMTP server per the AUTH LOGIN
-	// extension.
-	//
-	//	See: https://learn.microsoft.com/en-us/openspecs/exchange_server_protocols/ms-xlogin/.
-	LoginXPasswordChallenge      = "Password:"
-	LoginXPasswordLowerChallenge = "password:"
-
-	// LoginXDraftUsernameChallenge represents the Username Challenge response sent by the SMTP server per the IETF
-	// draft AUTH LOGIN extension. It should be noted this extension is an expired draft which was never formally
-	// published and was deprecated in favor of the AUTH PLAIN extension.
-	//
-	// See: https://datatracker.ietf.org/doc/html/draft-murchison-sasl-login-00.
-	LoginXDraftUsernameChallenge = "User Name\x00"
-
-	// LoginXDraftPasswordChallenge represents the Password Challenge response sent by the SMTP server per the IETF
-	// draft AUTH LOGIN extension. It should be noted this extension is an expired draft which was never formally
-	// published and was deprecated in favor of the AUTH PLAIN extension.
-	//
-	// See: https://datatracker.ietf.org/doc/html/draft-murchison-sasl-login-00.
-	LoginXDraftPasswordChallenge = "Password\x00"
-)
-
 // LoginAuth returns an [Auth] that implements the LOGIN authentication
 // mechanism as it is used by MS Outlook. The Auth works similar to PLAIN
 // but instead of sending all in one response, the login is handled within
 // 3 steps:
-// - Sending AUTH LOGIN (server responds with "Username:")
-// - Sending the username (server responds with "Password:")
+// - Sending AUTH LOGIN (server might responds with "Username:")
+// - Sending the username (server might responds with "Password:")
 // - Sending the password (server authenticates)
+// This is the common approach as specified by Microsoft in their MS-XLOGIN spec.
+// See: https://msopenspecs.azureedge.net/files/MS-XLOGIN/%5bMS-XLOGIN%5d.pdf
+// Yet, there is also an old IETF draft for SMTP AUTH LOGIN that states for clients:
+// "The contents of both challenges SHOULD be ignored.".
+// See: https://datatracker.ietf.org/doc/html/draft-murchison-sasl-login-00
+// Since there is no official standard RFC and we've seen different implementations
+// of this mechanism (sending "Username:", "Username", "username", "User name", etc.)
+// we follow the IETF-Draft and ignore any server challange to allow compatiblity
+// with most mail servers/providers.
 //
 // LoginAuth will only send the credentials if the connection is using TLS
 // or is connected to localhost. Otherwise authentication will fail with an
 // error, without sending the credentials.
 func LoginAuth(username, password, host string) Auth {
-	return &loginAuth{username, password, host}
+	return &loginAuth{username, password, host, 0}
 }
 
+// Start begins the SMTP authentication process by validating server's TLS status and hostname.
+// Returns "LOGIN" on success.
 func (a *loginAuth) Start(server *ServerInfo) (string, []byte, error) {
 	// Must have TLS, or else localhost server.
 	// Note: If TLS is not true, then we can't trust ANYTHING in ServerInfo.
 	// In particular, it doesn't matter if the server advertises LOGIN auth.
 	// That might just be the attacker saying
 	// "it's ok, you can trust me with your password."
 	if !server.TLS && !isLocalhost(server.Name) {
-		return "", nil, errors.New("unencrypted connection")
+		return "", nil, ErrUnencrypted
 	}
 	if server.Name != a.host {
 		return "", nil, errors.New("wrong host name")
 	}
+	a.respStep = 0
 	return "LOGIN", nil, nil
 }
 
+// Next processes responses from the server during the SMTP authentication exchange, sending the
+// username and password.
 func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) {
 	if more {
-		switch string(fromServer) {
-		case LoginXUsernameChallenge, LoginXUsernameLowerChallenge, LoginXDraftUsernameChallenge:
+		switch a.respStep {
+		case 0:
+			a.respStep++
 			return []byte(a.username), nil
-		case LoginXPasswordChallenge, LoginXPasswordLowerChallenge, LoginXDraftPasswordChallenge:
+		case 1:
+			a.respStep++
 			return []byte(a.password), nil
 		default:
 			return nil, fmt.Errorf("unexpected server response: %s", string(fromServer))

smtp/smtp_test.go

@@ -57,10 +57,31 @@ var authTests = []authTest{
 	},
 	{
 		LoginAuth("user", "pass", "testserver"),
-		[]string{"Username:", "Password:", "User Name\x00", "Password\x00", "Invalid:"},
+		[]string{"Username:", "Password:"},
 		"LOGIN",
-		[]string{"", "user", "pass", "user", "pass", ""},
-		[]bool{false, false, false, false, true},
+		[]string{"", "user", "pass"},
+		[]bool{false, false},
+	},
+	{
+		LoginAuth("user", "pass", "testserver"),
+		[]string{"User Name\x00", "Password\x00"},
+		"LOGIN",
+		[]string{"", "user", "pass"},
+		[]bool{false, false},
+	},
+	{
+		LoginAuth("user", "pass", "testserver"),
+		[]string{"Invalid", "Invalid:"},
+		"LOGIN",
+		[]string{"", "user", "pass"},
+		[]bool{false, false},
+	},
+	{
+		LoginAuth("user", "pass", "testserver"),
+		[]string{"Invalid", "Invalid:", "Too many"},
+		"LOGIN",
+		[]string{"", "user", "pass", ""},
+		[]bool{false, false, true},
 	},
 	{
 		CRAMMD5Auth("user", "pass"),