From 771574b6529f00791bbb6e1ba54edf262444fd3f Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Tue, 10 Mar 2020 18:34:31 +0300 Subject: [PATCH] [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 --- app/server_app_adapters.go | 4 +- model/config.go | 5 ++ services/mailservice/mail.go | 84 +++++++++++++++++++++++-------- services/mailservice/mail_test.go | 46 +++++++++++++++-- 4 files changed, 113 insertions(+), 26 deletions(-) diff --git a/app/server_app_adapters.go b/app/server_app_adapters.go index 758bdf94f2..f28ef26bb5 100644 --- a/app/server_app_adapters.go +++ b/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") diff --git a/model/config.go b/model/config.go index b4c4346479..f5b7b9a67a 100644 --- a/model/config.go +++ b/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) } diff --git a/services/mailservice/mail.go b/services/mailservice/mail.go index f7e003cb5b..3e392cb759 100644 --- a/services/mailservice/mail.go +++ b/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 } diff --git a/services/mailservice/mail_test.go b/services/mailservice/mail_test.go index cd22c5495a..365758e035 100644 --- a/services/mailservice/mail_test.go +++ b/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) {