Self-review

Signed-off-by: Danny Kopping <danny@coder.com>
coder · dannykopping · Jul 19, 2024 · Jul 12, 2024 · Jul 15, 2024 · Jul 16, 2024
commit cf9312973315d7052fceb913fa94d8230b7a805b
diff --git a/coderd/notifications/dispatch/smtp.go b/coderd/notifications/dispatch/smtp.go
@@ -7,7 +7,6 @@ import (
 	"crypto/x509"
 	_ "embed"
 	"fmt"
-	"io"
 	"mime/multipart"
 	"mime/quotedprintable"
 	"net"
@@ -16,6 +15,7 @@ import (
 	"os"
 	"slices"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/emersion/go-sasl"
@@ -45,9 +45,11 @@ var (
 	plainTemplate string
 )
 
+var loginWarnOnce sync.Once
+
 // SMTPHandler is responsible for dispatching notification messages via SMTP.
 // NOTE: auth and TLS is currently *not* enabled in this initial thin slice.
-// TODO: implement DKIM/SPF/DMARC: https://github.com/emersion/go-msgauth
+// TODO: implement DKIM/SPF/DMARC? https://github.com/emersion/go-msgauth
 type SMTPHandler struct {
 	cfg codersdk.NotificationsEmailConfig
 	log slog.Logger
@@ -116,6 +118,7 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
 		}
 
 		// TODO: reuse client across dispatches (if possible).
+		// Create an SMTP client for communication with the smarthost.
 		c, err := s.client(ctx, smarthost, smarthostPort)
 		if err != nil {
 			return true, xerrors.Errorf("SMTP client creation: %w", err)
@@ -129,11 +132,14 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
 		}()
 
 		// Check for authentication capabilities.
-		if ok, mech := c.Extension("AUTH"); ok {
-			auth, err := s.auth(ctx, mech)
+		if ok, avail := c.Extension("AUTH"); ok {
+			// Ensure the auth mechanisms available are ones we can use.
+			auth, err := s.auth(ctx, avail)
 			if err != nil {
 				return true, xerrors.Errorf("determine auth mechanism: %w", err)
 			}
+
+			// If so, use the auth mechanism to authenticate.
 			if auth != nil {
 				if err := c.Auth(auth); err != nil {
 					return true, xerrors.Errorf("%T auth: %w", auth, err)
@@ -247,6 +253,7 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
 	}
 }
 
+// client creates an SMTP client capable of communicating over a plain or TLS-encrypted connection.
 func (s *SMTPHandler) client(ctx context.Context, host string, port string) (*smtp.Client, error) {
 	var (
 		c    *smtp.Client
@@ -367,7 +374,7 @@ func (s *SMTPHandler) loadCAFile() (*x509.CertPool, error) {
 		return nil, nil
 	}
 
-	ca, err := s.loadFile(s.cfg.TLS.CAFile.String())
+	ca, err := os.ReadFile(s.cfg.TLS.CAFile.String())
 	if err != nil {
 		return nil, xerrors.Errorf("load CA file: %w", err)
 	}
@@ -385,11 +392,11 @@ func (s *SMTPHandler) loadCertificate() (*tls.Certificate, error) {
 		return nil, nil
 	}
 
-	cert, err := s.loadFile(s.cfg.TLS.CertFile.Value())
+	cert, err := os.ReadFile(s.cfg.TLS.CertFile.Value())
 	if err != nil {
 		return nil, xerrors.Errorf("load cert: %w", err)
 	}
-	key, err := s.loadFile(s.cfg.TLS.KeyFile.String())
+	key, err := os.ReadFile(s.cfg.TLS.KeyFile.String())
 	if err != nil {
 		return nil, xerrors.Errorf("load key: %w", err)
 	}
@@ -402,20 +409,6 @@ func (s *SMTPHandler) loadCertificate() (*tls.Certificate, error) {
 	return &pair, nil
 }
 
-func (s *SMTPHandler) loadFile(path string) ([]byte, error) {
-	f, err := os.OpenFile(path, os.O_RDONLY, 0o644)
-	if err != nil {
-		return nil, err
-	}
-
-	out, err := io.ReadAll(f)
-	if err != nil {
-		return nil, err
-	}
-
-	return out, nil
-}
-
 // auth returns a value which implements the smtp.Auth based on the available auth mechanisms.
 func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, error) {
 	username := s.cfg.Auth.Username.String()
@@ -431,7 +424,7 @@ func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, erro
 				continue
 			}
 			if password == "" {
-				errs = multierror.Append(errs, xerrors.New("cannot use PLAIN auth, password not defined")) // TODO: show env/flag here
+				errs = multierror.Append(errs, xerrors.New("cannot use PLAIN auth, password not defined (see CODER_NOTIFICATIONS_EMAIL_AUTH_PASSWORD)"))
 				continue
 			}
 
@@ -442,15 +435,18 @@ func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, erro
 				continue
 			}
 
-			s.log.Warn(ctx, "LOGIN auth is obsolete and should be avoided (use PLAIN instead): https://www.ietf.org/archive/id/draft-murchison-sasl-login-00.txt")
+			// Warn that LOGIN is obsolete, but don't do it every time we dispatch a notification.
+			loginWarnOnce.Do(func() {
+				s.log.Warn(ctx, "LOGIN auth is obsolete and should be avoided (use PLAIN instead): https://www.ietf.org/archive/id/draft-murchison-sasl-login-00.txt")
+			})
 
 			password, err := s.password()
 			if err != nil {
 				errs = multierror.Append(errs, err)
 				continue
 			}
 			if password == "" {
-				errs = multierror.Append(errs, xerrors.New("cannot use LOGIN auth, password not defined")) // TODO: show env/flag here
+				errs = multierror.Append(errs, xerrors.New("cannot use LOGIN auth, password not defined (see CODER_NOTIFICATIONS_EMAIL_AUTH_PASSWORD)"))
 				continue
 			}
 

diff --git a/coderd/notifications/dispatch/smtp_test.go b/coderd/notifications/dispatch/smtp_test.go
@@ -43,6 +43,10 @@ func TestSMTP(t *testing.T) {
 
 		subject = "This is the subject"
 		body    = "This is the body"
+
+		caFile   = "fixtures/ca.crt"
+		certFile = "fixtures/server.crt"
+		keyFile  = "fixtures/server.key"
 	)
 
 	logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true, IgnoredErrorIs: []error{}}).Leveled(slog.LevelDebug)
@@ -195,6 +199,7 @@ func TestSMTP(t *testing.T) {
 			retryable:        false,
 		},
 		{
+			// No auth, no problem!
 			name:      "No auth mechanisms supported, none configured",
 			authMechs: []string{},
 			cfg: codersdk.NotificationsEmailConfig{
@@ -209,17 +214,14 @@ func TestSMTP(t *testing.T) {
 		 */
 		{
 			// TLS is forced but certificate used by mock server is untrusted.
-			name:   "TLS conn fails: x509 untrusted",
-			useTLS: true,
-			cfg: codersdk.NotificationsEmailConfig{
-				ForceTLS: true,
-			},
+			name:        "TLS: x509 untrusted",
+			useTLS:      true,
 			expectedErr: "certificate is not trusted",
 			retryable:   true,
 		},
 		{
 			// TLS is forced and self-signed certificate used by mock server is not verified.
-			name:   "TLS conn succeeds: x509 untrusted ignored",
+			name:   "TLS: x509 untrusted ignored",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
 				Hello:    hello,
@@ -234,12 +236,11 @@ func TestSMTP(t *testing.T) {
 		{
 			// TLS is forced and STARTTLS is configured, but STARTTLS cannot be used by TLS connections.
 			// STARTTLS should be disabled and connection should succeed.
-			name:   "TLS conn succeeds: STARTTLS is ignored",
+			name:   "TLS: STARTTLS is ignored",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
-				Hello:    hello,
-				From:     from,
-				ForceTLS: true,
+				Hello: hello,
+				From:  from,
 				TLS: codersdk.NotificationsEmailTLSConfig{
 					InsecureSkipVerify: true,
 					StartTLS:           true,
@@ -249,7 +250,7 @@ func TestSMTP(t *testing.T) {
 		},
 		{
 			// Plain connection is established and upgraded via STARTTLS, but certificate is untrusted.
-			name:   "TLS conn fails: STARTTLS untrusted",
+			name:   "TLS: STARTTLS untrusted",
 			useTLS: false,
 			cfg: codersdk.NotificationsEmailConfig{
 				TLS: codersdk.NotificationsEmailTLSConfig{
@@ -263,7 +264,7 @@ func TestSMTP(t *testing.T) {
 		},
 		{
 			// Plain connection is established and upgraded via STARTTLS, certificate is not verified.
-			name:   "TLS conn succeeds: STARTTLS",
+			name:   "TLS: STARTTLS",
 			useTLS: false,
 			cfg: codersdk.NotificationsEmailConfig{
 				Hello: hello,
@@ -278,73 +279,68 @@ func TestSMTP(t *testing.T) {
 		},
 		{
 			// TLS connection using self-signed certificate.
-			name:   "TLS conn succeeds: self-signed",
+			name:   "TLS: self-signed",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
 				Hello: hello,
 				From:  from,
 				TLS: codersdk.NotificationsEmailTLSConfig{
-					CAFile:   "fixtures/ca.crt",
-					CertFile: "fixtures/server.crt",
-					KeyFile:  "fixtures/server.key",
+					CAFile:   caFile,
+					CertFile: certFile,
+					KeyFile:  keyFile,
 				},
-				ForceTLS: true,
 			},
 			toAddrs: []string{to},
 		},
 		{
 			// TLS connection using self-signed certificate & specifying the DNS name configured in the certificate.
-			name:   "TLS conn succeeds: self-signed + SNI",
+			name:   "TLS: self-signed + SNI",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
 				Hello: hello,
 				From:  from,
 				TLS: codersdk.NotificationsEmailTLSConfig{
 					ServerName: "myserver.local",
-					CAFile:     "fixtures/ca.crt",
-					CertFile:   "fixtures/server.crt",
-					KeyFile:    "fixtures/server.key",
+					CAFile:     caFile,
+					CertFile:   certFile,
+					KeyFile:    keyFile,
 				},
-				ForceTLS: true,
 			},
 			toAddrs: []string{to},
 		},
 		{
-			name:   "TLS conn fails: load CA",
+			name:   "TLS: load CA",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
 				TLS: codersdk.NotificationsEmailTLSConfig{
 					CAFile: "nope.crt",
 				},
-				ForceTLS: true,
 			},
 			expectedErr: "open nope.crt: no such file or directory",
 			retryable:   true,
 		},
 		{
-			name:   "TLS conn fails: load cert",
+			name:   "TLS: load cert",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
 				TLS: codersdk.NotificationsEmailTLSConfig{
-					CAFile:   "fixtures/ca.crt",
+					CAFile:   caFile,
 					CertFile: "fixtures/nope.cert",
-					KeyFile:  "fixtures/server.key",
+					KeyFile:  keyFile,
 				},
-				ForceTLS: true,
 			},
 			expectedErr: "open fixtures/nope.cert: no such file or directory",
 			retryable:   true,
 		},
 		{
-			name:   "TLS conn fails: load cert key",
+			name:   "TLS: load cert key",
 			useTLS: true,
 			cfg: codersdk.NotificationsEmailConfig{
 				TLS: codersdk.NotificationsEmailTLSConfig{
-					CAFile:   "fixtures/ca.crt",
-					CertFile: "fixtures/server.crt",
+					CAFile:   caFile,
+					CertFile: certFile,
 					KeyFile:  "fixtures/nope.key",
 				},
-				ForceTLS: true,
 			},
 			expectedErr: "open fixtures/nope.key: no such file or directory",
 			retryable:   true,
@@ -365,11 +361,10 @@ func TestSMTP(t *testing.T) {
 					Password: password,
 				},
 				TLS: codersdk.NotificationsEmailTLSConfig{
-					CAFile:   "fixtures/ca.crt",
-					CertFile: "fixtures/server.crt",
-					KeyFile:  "fixtures/server.key",
+					CAFile:   caFile,
+					CertFile: certFile,
+					KeyFile:  keyFile,
 				},
-				ForceTLS: true,
 			},
 			toAddrs:          []string{to},
 			expectedAuthMeth: sasl.Plain,
@@ -382,6 +377,8 @@ func TestSMTP(t *testing.T) {
 
 			ctx := testutil.Context(t, testutil.WaitShort)
 
+			tc.cfg.ForceTLS = serpent.Bool(tc.useTLS)
+
 			backend := NewBackend(Config{
 				AuthMechanisms: tc.authMechs,
 
@@ -394,7 +391,6 @@ func TestSMTP(t *testing.T) {
 			srv, listen, err := createMockSMTPServer(backend, tc.useTLS)
 			require.NoError(t, err)
 			t.Cleanup(func() {
-				_ = listen.Close()
 				// We expect that the server has already been closed in the test
 				assert.ErrorIs(t, srv.Shutdown(ctx), smtp.ErrServerClosed)
 			})

diff --git a/coderd/notifications/dispatch/smtp_util_test.go b/coderd/notifications/dispatch/smtp_util_test.go
@@ -34,7 +34,6 @@ type Message struct {
 	Subject, Contents            string   // Content
 }
 
-// The Backend implements SMTP server methods.
 type Backend struct {
 	cfg Config
 
@@ -61,7 +60,6 @@ func (b *Backend) Reset() {
 	b.lastMsg = nil
 }
 
-// A Session is returned after successful login.
 type Session struct {
 	conn    *smtp.Conn
 	backend *Backend

diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -508,18 +508,18 @@ type NotificationsEmailConfig struct {
 	Auth NotificationsEmailAuthConfig `json:"auth" typescript:",notnull"`
 	// TLS details.
 	TLS NotificationsEmailTLSConfig `json:"tls" typescript:",notnull"`
-	// ForceTLS caused a TLS connection to be attempted.
+	// ForceTLS causes a TLS connection to be attempted.
 	ForceTLS serpent.Bool `json:"force_tls" typescript:",notnull"`
 }
 
 type NotificationsEmailAuthConfig struct {
-	// Identity used for PLAIN auth.
+	// Identity for PLAIN auth.
 	Identity serpent.String `json:"identity" typescript:",notnull"`
-	// Username for LOGIN/PLAIN auth; authentication is disabled if this is left blank.
+	// Username for LOGIN/PLAIN auth.
 	Username serpent.String `json:"username" typescript:",notnull"`
-	// Password to use for LOGIN/PLAIN auth.
+	// Password for LOGIN/PLAIN auth.
 	Password serpent.String `json:"password" typescript:",notnull"`
-	// File from which to load the password to use for LOGIN/PLAIN auth.
+	// File from which to load the password for LOGIN/PLAIN auth.
 	PasswordFile serpent.String `json:"password_file" typescript:",notnull"`
 }
 
@@ -528,6 +528,7 @@ func (c *NotificationsEmailAuthConfig) Empty() bool {
 }
 
 type NotificationsEmailTLSConfig struct {
+	// StartTLS attempts to upgrade plain connections to TLS.
 	StartTLS serpent.Bool `json:"start_tls" typescript:",notnull"`
 	// ServerName to verify the hostname for the targets.
 	ServerName serpent.String `json:"server_name" typescript:",notnull"`