From 547f78dbee8fb1777b4a96350b845c702f5986eb Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Wed, 2 Oct 2024 12:37:54 +0200 Subject: [PATCH 1/4] Enhance SMTP LOGIN auth and add comprehensive tests Refactored SMTP LOGIN auth to improve compatibility with various server responses, consolidating error handling and response steps. Added extensive tests to verify successful and failed authentication across different server configurations. --- client_test.go | 158 +++++++++++++++++++++++++++++++++++++++++++++ smtp/auth_login.go | 65 ++++++++----------- 2 files changed, 185 insertions(+), 38 deletions(-) diff --git a/client_test.go b/client_test.go index 481b9be..35fca38 100644 --- a/client_test.go +++ b/client_test.go @@ -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("toni@tester.com"), + 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("toni@tester.com"), + 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 .") for { diff --git a/smtp/auth_login.go b/smtp/auth_login.go index aa80223..e25b3e6 100644 --- a/smtp/auth_login.go +++ b/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,57 +9,42 @@ 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. @@ -67,7 +52,7 @@ func (a *loginAuth) Start(server *ServerInfo) (string, []byte, error) { // 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") @@ -75,12 +60,16 @@ func (a *loginAuth) Start(server *ServerInfo) (string, []byte, error) { 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)) From 93752280aaa1ec6b0ae39e97858e908dc78acd7a Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Wed, 2 Oct 2024 12:54:32 +0200 Subject: [PATCH 2/4] Update smtp_test.go to add more authentication test cases Enhanced the LoginAuth test coverage by adding new scenarios with different sequences and invalid cases. This ensures more robust validation and better handling of edge cases in authentication testing. --- smtp/smtp_test.go | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/smtp/smtp_test.go b/smtp/smtp_test.go index 6df6eeb..d5b02a7 100644 --- a/smtp/smtp_test.go +++ b/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"), From 9d70283af976b6db4d0a5734d0aad1d58b8f45e7 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Wed, 2 Oct 2024 13:09:55 +0200 Subject: [PATCH 3/4] Reset response step in AUTH LOGIN initialization The addition of `a.respStep = 0` resets the response step counter at the beginning of the AUTH LOGIN process. This ensures that the state starts correctly and avoids potential issues related to residual values from previous authentications. --- smtp/auth_login.go | 1 + 1 file changed, 1 insertion(+) diff --git a/smtp/auth_login.go b/smtp/auth_login.go index e25b3e6..715861c 100644 --- a/smtp/auth_login.go +++ b/smtp/auth_login.go @@ -57,6 +57,7 @@ func (a *loginAuth) Start(server *ServerInfo) (string, []byte, error) { if server.Name != a.host { return "", nil, errors.New("wrong host name") } + a.respStep = 0 return "LOGIN", nil, nil } From 761e20504969e3f0e766333853d1361b0abd1387 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Wed, 2 Oct 2024 13:10:10 +0200 Subject: [PATCH 4/4] Fix client connection test error handling Changed variable assignment in the test to fix error handling. This ensures the error is properly caught and reported during the client connection process. --- client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_test.go b/client_test.go index 35fca38..53174f2 100644 --- a/client_test.go +++ b/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 }