[MM-17797] Add Timeout To SMTP Connection (#13251)

* model/config: add timeout setting to email settings

* services/mailservice: add timeout to connection

* app/server_app_adapter: show SMTP connection errors on log

* services/mailservice: add r/w deadline to smtp conn

* services/mailservice: add context timeout

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

app/server_app_adapters.go

@@ -123,7 +123,9 @@ func (s *Server) RunOldAppInitialization() error {
 		handlers: make(map[string]webSocketHandler),
 	}
 
-	mailservice.TestConnection(s.FakeApp().Config())
+	if err := mailservice.TestConnection(s.FakeApp().Config()); err != nil {
+		mlog.Error("Mail server connection test is failed: " + err.Message)
+	}
 
 	if _, err := url.ParseRequestURI(*s.FakeApp().Config().ServiceSettings.SiteURL); err != nil {
 		mlog.Error("SiteURL must be set. Some features will operate incorrectly if the SiteURL is not set. See documentation for details: http://about.mattermost.com/default-site-url")

model/config.go

@@ -1245,6 +1245,7 @@ type EmailSettings struct {
 	SMTPPassword                      *string `restricted:"true"`
 	SMTPServer                        *string `restricted:"true"`
 	SMTPPort                          *string `restricted:"true"`
+	SMTPServerTimeout                 *int
 	ConnectionSecurity                *string `restricted:"true"`
 	SendPushNotifications             *bool
 	PushNotificationServer            *string
@@ -1325,6 +1326,10 @@ func (s *EmailSettings) SetDefaults(isUpdate bool) {
 		s.SMTPPort = NewString("10025")
 	}
 
+	if s.SMTPServerTimeout == nil || *s.SMTPServerTimeout == 0 {
+		s.SMTPServerTimeout = NewInt(10)
+	}
+
 	if s.ConnectionSecurity == nil || *s.ConnectionSecurity == CONN_SECURITY_PLAIN {
 		s.ConnectionSecurity = NewString(CONN_SECURITY_NONE)
 	}

services/mailservice/mail.go

@@ -4,6 +4,7 @@
 package mailservice
 
 import (
+	"context"
 	"crypto/tls"
 	"errors"
 	"io"
@@ -54,6 +55,7 @@ type SmtpConnectionInfo struct {
 	SmtpServerName       string
 	SmtpServerHost       string
 	SmtpPort             string
+	SmtpServerTimeout    int
 	SkipCertVerification bool
 	ConnectionSecurity   string
 	Auth                 bool
@@ -115,18 +117,22 @@ func ConnectToSMTPServerAdvanced(connectionInfo *SmtpConnectionInfo) (net.Conn,
 	var err error
 
 	smtpAddress := connectionInfo.SmtpServerHost + ":" + connectionInfo.SmtpPort
+	dialer := &net.Dialer{
+		Timeout: time.Duration(connectionInfo.SmtpServerTimeout) * time.Second,
+	}
+
 	if connectionInfo.ConnectionSecurity == model.CONN_SECURITY_TLS {
 		tlsconfig := &tls.Config{
 			InsecureSkipVerify: connectionInfo.SkipCertVerification,
 			ServerName:         connectionInfo.SmtpServerName,
 		}
 
-		conn, err = tls.Dial("tcp", smtpAddress, tlsconfig)
+		conn, err = tls.DialWithDialer(dialer, "tcp", smtpAddress, tlsconfig)
 		if err != nil {
 			return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open_tls.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 	} else {
-		conn, err = net.Dial("tcp", smtpAddress)
+		conn, err = dialer.Dial("tcp", smtpAddress)
 		if err != nil {
 			return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
@@ -143,19 +149,39 @@ func ConnectToSMTPServer(config *model.Config) (net.Conn, *model.AppError) {
 			SmtpServerName:       *config.EmailSettings.SMTPServer,
 			SmtpServerHost:       *config.EmailSettings.SMTPServer,
 			SmtpPort:             *config.EmailSettings.SMTPPort,
+			SmtpServerTimeout:    *config.EmailSettings.SMTPServerTimeout,
 		},
 	)
 }
 
-func NewSMTPClientAdvanced(conn net.Conn, hostname string, connectionInfo *SmtpConnectionInfo) (*smtp.Client, *model.AppError) {
-	c, err := smtp.NewClient(conn, connectionInfo.SmtpServerName+":"+connectionInfo.SmtpPort)
-	if err != nil {
-		mlog.Error("Failed to open a connection to SMTP server", mlog.Err(err))
+func NewSMTPClientAdvanced(ctx context.Context, conn net.Conn, hostname string, connectionInfo *SmtpConnectionInfo) (*smtp.Client, *model.AppError) {
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	var c *smtp.Client
+	ec := make(chan error)
+	go func() {
+		var err error
+		c, err = smtp.NewClient(conn, connectionInfo.SmtpServerName+":"+connectionInfo.SmtpPort)
+		if err != nil {
+			ec <- err
+			return
+		}
+		cancel()
+	}()
+
+	select {
+	case <-ctx.Done():
+		err := ctx.Err()
+		if err != nil && err.Error() != "context canceled" {
+			return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open_tls.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
+	case err := <-ec:
 		return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.open_tls.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
 	if hostname != "" {
-		err = c.Hello(hostname)
+		err := c.Hello(hostname)
 		if err != nil {
 			mlog.Error("Failed to to set the HELO to SMTP server", mlog.Err(err))
 			return nil, model.NewAppError("SendMail", "utils.mail.connect_smtp.helo.app_error", nil, err.Error(), http.StatusInternalServerError)
@@ -171,15 +197,16 @@ func NewSMTPClientAdvanced(conn net.Conn, hostname string, connectionInfo *SmtpC
 	}
 
 	if connectionInfo.Auth {
-		if err = c.Auth(&authChooser{connectionInfo: connectionInfo}); err != nil {
+		if err := c.Auth(&authChooser{connectionInfo: connectionInfo}); err != nil {
 			return nil, model.NewAppError("SendMail", "utils.mail.new_client.auth.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 	}
 	return c, nil
 }
 
-func NewSMTPClient(conn net.Conn, config *model.Config) (*smtp.Client, *model.AppError) {
+func NewSMTPClient(ctx context.Context, conn net.Conn, config *model.Config) (*smtp.Client, *model.AppError) {
 	return NewSMTPClientAdvanced(
+		ctx,
 		conn,
 		utils.GetHostnameFromSiteURL(*config.ServiceSettings.SiteURL),
 		&SmtpConnectionInfo{
@@ -188,6 +215,7 @@ func NewSMTPClient(conn net.Conn, config *model.Config) (*smtp.Client, *model.Ap
 			SmtpServerName:       *config.EmailSettings.SMTPServer,
 			SmtpServerHost:       *config.EmailSettings.SMTPServer,
 			SmtpPort:             *config.EmailSettings.SMTPPort,
+			SmtpServerTimeout:    *config.EmailSettings.SMTPServerTimeout,
 			Auth:                 *config.EmailSettings.EnableSMTPAuth,
 			SmtpUsername:         *config.EmailSettings.SMTPUsername,
 			SmtpPassword:         *config.EmailSettings.SMTPPassword,
@@ -195,25 +223,31 @@ func NewSMTPClient(conn net.Conn, config *model.Config) (*smtp.Client, *model.Ap
 	)
 }
 
-func TestConnection(config *model.Config) {
+func TestConnection(config *model.Config) *model.AppError {
 	if !*config.EmailSettings.SendEmailNotifications {
-		return
+		return &model.AppError{Message: "SendEmailNotifications is not true"}
 	}
 
-	conn, err1 := ConnectToSMTPServer(config)
-	if err1 != nil {
-		mlog.Error("SMTP server settings do not appear to be configured properly", mlog.Err(err1))
-		return
+	conn, err := ConnectToSMTPServer(config)
+	if err != nil {
+		return &model.AppError{Message: "Could not connect to SMTP server, check SMTP server settings.", DetailedError: err.DetailedError}
 	}
 	defer conn.Close()
 
-	c, err2 := NewSMTPClient(conn, config)
-	if err2 != nil {
-		mlog.Error("SMTP server settings do not appear to be configured properly", mlog.Err(err2))
-		return
+	sec := *config.EmailSettings.SMTPServerTimeout
+
+	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(ctx, time.Duration(sec)*time.Second)
+	defer cancel()
+
+	c, err := NewSMTPClient(ctx, conn, config)
+	if err != nil {
+		return &model.AppError{Message: "Could not connect to SMTP server, check SMTP server settings."}
 	}
-	defer c.Quit()
-	defer c.Close()
+	c.Close()
+	c.Quit()
+
+	return nil
 }
 
 func SendMailWithEmbeddedFilesUsingConfig(to, subject, htmlBody string, embeddedFiles map[string]io.Reader, config *model.Config, enableComplianceFeatures bool) *model.AppError {
@@ -249,7 +283,13 @@ func sendMailUsingConfigAdvanced(mail mailData, config *model.Config, enableComp
 	}
 	defer conn.Close()
 
-	c, err := NewSMTPClient(conn, config)
+	sec := *config.EmailSettings.SMTPServerTimeout
+
+	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(ctx, time.Duration(sec)*time.Second)
+	defer cancel()
+
+	c, err := NewSMTPClient(ctx, conn, config)
 	if err != nil {
 		return err
 	}

services/mailservice/mail_test.go

@@ -5,10 +5,13 @@ package mailservice
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -31,7 +34,7 @@ func TestMailConnectionFromConfig(t *testing.T) {
 	conn, err := ConnectToSMTPServer(cfg)
 	require.Nil(t, err, "Should connect to the SMTP Server %v", err)
 
-	_, err = NewSMTPClient(conn, cfg)
+	_, err = NewSMTPClient(context.Background(), conn, cfg)
 
 	require.Nil(t, err, "Should get new SMTP client")
 
@@ -58,10 +61,12 @@ func TestMailConnectionAdvanced(t *testing.T) {
 			SmtpPort:             *cfg.EmailSettings.SMTPPort,
 		},
 	)
+	defer conn.Close()
 
 	require.Nil(t, err, "Should connect to the SMTP Server")
 
 	_, err2 := NewSMTPClientAdvanced(
+		context.Background(),
 		conn,
 		utils.GetHostnameFromSiteURL(*cfg.ServiceSettings.SiteURL),
 		&SmtpConnectionInfo{
@@ -73,12 +78,46 @@ func TestMailConnectionAdvanced(t *testing.T) {
 			Auth:                 *cfg.EmailSettings.EnableSMTPAuth,
 			SmtpUsername:         *cfg.EmailSettings.SMTPUsername,
 			SmtpPassword:         *cfg.EmailSettings.SMTPPassword,
+			SmtpServerTimeout:    1,
 		},
 	)
 
 	require.Nil(t, err2, "Should get new SMTP client")
 
-	_, err3 := ConnectToSMTPServerAdvanced(
+	l, err := net.Listen("tcp", "localhost:42356") // emulate nc -l 42356
+	require.Nil(t, err, "Should've open a network socket and listen")
+	defer l.Close()
+
+	connInfo := &SmtpConnectionInfo{
+		ConnectionSecurity:   *cfg.EmailSettings.ConnectionSecurity,
+		SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification,
+		SmtpServerName:       *cfg.EmailSettings.SMTPServer,
+		SmtpServerHost:       strings.Split(l.Addr().String(), ":")[0],
+		SmtpPort:             strings.Split(l.Addr().String(), ":")[1],
+		Auth:                 *cfg.EmailSettings.EnableSMTPAuth,
+		SmtpUsername:         *cfg.EmailSettings.SMTPUsername,
+		SmtpPassword:         *cfg.EmailSettings.SMTPPassword,
+		SmtpServerTimeout:    1,
+	}
+
+	conn, err = ConnectToSMTPServerAdvanced(connInfo)
+	defer conn.Close()
+	require.Nil(t, err, "Should connect to the SMTP Server")
+
+	ctx := context.Background()
+	ctx, cancel := context.WithTimeout(ctx, time.Second)
+	defer cancel()
+
+	_, err3 := NewSMTPClientAdvanced(
+		ctx,
+		conn,
+		utils.GetHostnameFromSiteURL(*cfg.ServiceSettings.SiteURL),
+		connInfo,
+	)
+
+	require.NotNil(t, err3, "Should get new SMTP client")
+
+	_, err4 := ConnectToSMTPServerAdvanced(
 		&SmtpConnectionInfo{
 			ConnectionSecurity:   *cfg.EmailSettings.ConnectionSecurity,
 			SkipCertVerification: *cfg.EmailSettings.SkipServerCertificateVerification,
@@ -87,7 +126,8 @@ func TestMailConnectionAdvanced(t *testing.T) {
 			SmtpPort:             "553",
 		},
 	)
-	require.NotNil(t, err3, "Should not connect to the SMTP Server")
+
+	require.NotNil(t, err4, "Should not connect to the SMTP Server")
 }
 
 func TestSendMailUsingConfig(t *testing.T) {