From 152b33e4fa35fa5aca7d18eb916c2e1d2fc8a4e3 Mon Sep 17 00:00:00 2001 From: James Elliott Date: Tue, 28 Jan 2020 15:00:43 +1100 Subject: [PATCH] [FIX] SMTP Notifier Unhandled Error Conditions (#585) - Only attempt to close the connection once it's established. - Defer the client Quit/Close so that it always executes at the end. - Fixes #585 --- internal/notification/smtp_notifier.go | 68 ++++++++++++-------------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/internal/notification/smtp_notifier.go b/internal/notification/smtp_notifier.go index 2cfd75850..f99c548ce 100644 --- a/internal/notification/smtp_notifier.go +++ b/internal/notification/smtp_notifier.go @@ -14,7 +14,7 @@ import ( log "github.com/sirupsen/logrus" ) -// SMTPNotifier a notifier to send emails to SMTP servers +// SMTPNotifier a notifier to send emails to SMTP servers. type SMTPNotifier struct { username string password string @@ -29,7 +29,7 @@ type SMTPNotifier struct { tlsConfig *tls.Config } -// NewSMTPNotifier create an SMTPNotifier targeting a given address +// NewSMTPNotifier create an SMTPNotifier targeting a given address. func NewSMTPNotifier(configuration schema.SMTPNotifierConfiguration) *SMTPNotifier { notifier := &SMTPNotifier{ username: configuration.Username, @@ -47,15 +47,15 @@ func NewSMTPNotifier(configuration schema.SMTPNotifierConfiguration) *SMTPNotifi } func (n *SMTPNotifier) initializeTLSConfig() { - // Do not allow users to disable verification of certs if they have also set a trusted cert that was loaded - // The second part of this check happens in the Configure Cert Pool code block + // Do not allow users to disable verification of certs if they have also set a trusted cert that was loaded. + // The second part of this check happens in the Configure Cert Pool code block. log.Debug("Notifier SMTP client initializing TLS configuration") insecureSkipVerify := false if n.disableVerifyCert { insecureSkipVerify = true } - //Configure Cert Pool + //Configure Cert Pool. certPool, err := x509.SystemCertPool() if err != nil || certPool == nil { certPool = x509.NewCertPool() @@ -89,9 +89,9 @@ func (n *SMTPNotifier) initializeTLSConfig() { } } -// Do startTLS if available (some servers only provide the auth extension after, and encryption is preferred) +// Do startTLS if available (some servers only provide the auth extension after, and encryption is preferred). func (n *SMTPNotifier) startTLS() (bool, error) { - // Only start if not already encrypted + // Only start if not already encrypted. if _, ok := n.client.TLSConnectionState(); ok { log.Debugf("Notifier SMTP connection is already encrypted, skipping STARTTLS") return ok, nil @@ -115,16 +115,16 @@ func (n *SMTPNotifier) startTLS() (bool, error) { return ok, nil } -// Attempt Authentication +// Attempt Authentication. func (n *SMTPNotifier) auth() (bool, error) { - // Attempt AUTH if password is specified only + // Attempt AUTH if password is specified only. if n.password != "" { _, ok := n.client.TLSConnectionState() if !ok { return false, errors.New("Notifier SMTP client does not support authentication over plain text and the connection is currently plain text") } - // Check the server supports AUTH, and get the mechanisms + // Check the server supports AUTH, and get the mechanisms. ok, m := n.client.Extension("AUTH") if ok { log.Debugf("Notifier SMTP server supports authentication with the following mechanisms: %s", m) @@ -140,12 +140,12 @@ func (n *SMTPNotifier) auth() (bool, error) { log.Debug("Notifier SMTP client attempting AUTH LOGIN with server") } - // Throw error since AUTH extension is not supported + // Throw error since AUTH extension is not supported. if auth == nil { return false, fmt.Errorf("notifier SMTP server does not advertise a AUTH mechanism that are supported by Authelia (PLAIN or LOGIN are supported, but server advertised %s mechanisms)", m) } - // Authenticate + // Authenticate. err := n.client.Auth(auth) if err != nil { return false, err @@ -196,7 +196,7 @@ func (n *SMTPNotifier) compose(recipient, subject, body string) error { return nil } -// Dial the SMTP server with the SMTPNotifier config +// Dial the SMTP server with the SMTPNotifier config. func (n *SMTPNotifier) dial() error { log.Debugf("Notifier SMTP client attempting connection to %s", n.address) client, err := smtp.Dial(n.address) @@ -208,50 +208,46 @@ func (n *SMTPNotifier) dial() error { return nil } +// Closes the connection properly. +func (n *SMTPNotifier) cleanup() { + err := n.client.Quit() + if err != nil { + log.Warnf("Notifier SMTP client encountered error during cleanup: %s", err) + } +} + // Send an email func (n *SMTPNotifier) Send(recipient, subject, body string) error { - err := n.dial() - if err != nil { - _ = n.client.Close() + if err := n.dial(); err != nil { return err } - _, err = n.startTLS() - if err != nil { - _ = n.client.Close() + // Always execute QUIT at the end once we're connected. + defer n.cleanup() + + // Start TLS and then Authenticate. + if _, err := n.startTLS(); err != nil { + return err + } + if _, err := n.auth(); err != nil { return err } - _, err = n.auth() - if err != nil { - _ = n.client.Close() - return err - } - - // Set the sender and recipient first + // Set the sender and recipient first. if err := n.client.Mail(n.sender); err != nil { log.Debugf("Notifier SMTP failed while sending MAIL FROM (using sender) with error: %s", err) - _ = n.client.Close() return err } if err := n.client.Rcpt(recipient); err != nil { log.Debugf("Notifier SMTP failed while sending RCPT TO (using recipient) with error: %s", err) - _ = n.client.Close() return err } - // compose and send the email body + // Compose and send the email body to the server. if err := n.compose(recipient, subject, body); err != nil { - _ = n.client.Close() return err } - // Send the QUIT command and close the connection - err = n.client.Quit() - if err != nil { - _ = n.client.Close() - return err - } log.Debug("Notifier SMTP client successfully sent email") return nil }