Skip to content

Commit cf93129

Browse files
dannykoppingdannykopping
committed
Self-review
Signed-off-by: Danny Kopping <danny@coder.com>
1 parent 379cf22 commit cf93129

File tree

4 files changed

+59
-68
lines changed

4 files changed

+59
-68
lines changed

‎coderd/notifications/dispatch/smtp.go

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"crypto/x509"
88
_ "embed"
99
"fmt"
10-
"io"
1110
"mime/multipart"
1211
"mime/quotedprintable"
1312
"net"
@@ -16,6 +15,7 @@ import (
1615
"os"
1716
"slices"
1817
"strings"
18+
"sync"
1919
"time"
2020

2121
"github.com/emersion/go-sasl"
@@ -45,9 +45,11 @@ var (
4545
plainTemplate string
4646
)
4747

48+
var loginWarnOnce sync.Once
49+
4850
// SMTPHandler is responsible for dispatching notification messages via SMTP.
4951
// NOTE: auth and TLS is currently *not* enabled in this initial thin slice.
50-
// TODO: implement DKIM/SPF/DMARC: https://github.com/emersion/go-msgauth
52+
// TODO: implement DKIM/SPF/DMARC? https://github.com/emersion/go-msgauth
5153
type SMTPHandler struct {
5254
cfg codersdk.NotificationsEmailConfig
5355
log slog.Logger
@@ -116,6 +118,7 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
116118
}
117119

118120
// TODO: reuse client across dispatches (if possible).
121+
// Create an SMTP client for communication with the smarthost.
119122
c, err := s.client(ctx, smarthost, smarthostPort)
120123
if err != nil {
121124
return true, xerrors.Errorf("SMTP client creation: %w", err)
@@ -129,11 +132,14 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
129132
}()
130133

131134
// Check for authentication capabilities.
132-
if ok, mech := c.Extension("AUTH"); ok {
133-
auth, err := s.auth(ctx, mech)
135+
if ok, avail := c.Extension("AUTH"); ok {
136+
// Ensure the auth mechanisms available are ones we can use.
137+
auth, err := s.auth(ctx, avail)
134138
if err != nil {
135139
return true, xerrors.Errorf("determine auth mechanism: %w", err)
136140
}
141+
142+
// If so, use the auth mechanism to authenticate.
137143
if auth != nil {
138144
if err := c.Auth(auth); err != nil {
139145
return true, xerrors.Errorf("%T auth: %w", auth, err)
@@ -247,6 +253,7 @@ func (s *SMTPHandler) dispatch(subject, htmlBody, plainBody, to string) Delivery
247253
}
248254
}
249255

256+
// client creates an SMTP client capable of communicating over a plain or TLS-encrypted connection.
250257
func (s *SMTPHandler) client(ctx context.Context, host string, port string) (*smtp.Client, error) {
251258
var (
252259
c *smtp.Client
@@ -367,7 +374,7 @@ func (s *SMTPHandler) loadCAFile() (*x509.CertPool, error) {
367374
return nil, nil
368375
}
369376

370-
ca, err := s.loadFile(s.cfg.TLS.CAFile.String())
377+
ca, err := os.ReadFile(s.cfg.TLS.CAFile.String())
371378
if err != nil {
372379
return nil, xerrors.Errorf("load CA file: %w", err)
373380
}
@@ -385,11 +392,11 @@ func (s *SMTPHandler) loadCertificate() (*tls.Certificate, error) {
385392
return nil, nil
386393
}
387394

388-
cert, err := s.loadFile(s.cfg.TLS.CertFile.Value())
395+
cert, err := os.ReadFile(s.cfg.TLS.CertFile.Value())
389396
if err != nil {
390397
return nil, xerrors.Errorf("load cert: %w", err)
391398
}
392-
key, err := s.loadFile(s.cfg.TLS.KeyFile.String())
399+
key, err := os.ReadFile(s.cfg.TLS.KeyFile.String())
393400
if err != nil {
394401
return nil, xerrors.Errorf("load key: %w", err)
395402
}
@@ -402,20 +409,6 @@ func (s *SMTPHandler) loadCertificate() (*tls.Certificate, error) {
402409
return &pair, nil
403410
}
404411

405-
func (s *SMTPHandler) loadFile(path string) ([]byte, error) {
406-
f, err := os.OpenFile(path, os.O_RDONLY, 0o644)
407-
if err != nil {
408-
return nil, err
409-
}
410-
411-
out, err := io.ReadAll(f)
412-
if err != nil {
413-
return nil, err
414-
}
415-
416-
return out, nil
417-
}
418-
419412
// auth returns a value which implements the smtp.Auth based on the available auth mechanisms.
420413
func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, error) {
421414
username := s.cfg.Auth.Username.String()
@@ -431,7 +424,7 @@ func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, erro
431424
continue
432425
}
433426
if password == "" {
434-
errs = multierror.Append(errs, xerrors.New("cannot use PLAIN auth, password not defined")) // TODO: show env/flag here
427+
errs = multierror.Append(errs, xerrors.New("cannot use PLAIN auth, password not defined (see CODER_NOTIFICATIONS_EMAIL_AUTH_PASSWORD)"))
435428
continue
436429
}
437430

@@ -442,15 +435,18 @@ func (s *SMTPHandler) auth(ctx context.Context, mechs string) (sasl.Client, erro
442435
continue
443436
}
444437

445-
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")
438+
// Warn that LOGIN is obsolete, but don't do it every time we dispatch a notification.
439+
loginWarnOnce.Do(func() {
440+
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")
441+
})
446442

447443
password, err := s.password()
448444
if err != nil {
449445
errs = multierror.Append(errs, err)
450446
continue
451447
}
452448
if password == "" {
453-
errs = multierror.Append(errs, xerrors.New("cannot use LOGIN auth, password not defined")) // TODO: show env/flag here
449+
errs = multierror.Append(errs, xerrors.New("cannot use LOGIN auth, password not defined (see CODER_NOTIFICATIONS_EMAIL_AUTH_PASSWORD)"))
454450
continue
455451
}
456452

‎coderd/notifications/dispatch/smtp_test.go

Lines changed: 33 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ func TestSMTP(t *testing.T) {
4343

4444
subject = "This is the subject"
4545
body = "This is the body"
46+
47+
caFile = "fixtures/ca.crt"
48+
certFile = "fixtures/server.crt"
49+
keyFile = "fixtures/server.key"
4650
)
4751

4852
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true, IgnoredErrorIs: []error{}}).Leveled(slog.LevelDebug)
@@ -195,6 +199,7 @@ func TestSMTP(t *testing.T) {
195199
retryable: false,
196200
},
197201
{
202+
// No auth, no problem!
198203
name: "No auth mechanisms supported, none configured",
199204
authMechs: []string{},
200205
cfg: codersdk.NotificationsEmailConfig{
@@ -209,17 +214,14 @@ func TestSMTP(t *testing.T) {
209214
*/
210215
{
211216
// TLS is forced but certificate used by mock server is untrusted.
212-
name: "TLS conn fails: x509 untrusted",
213-
useTLS: true,
214-
cfg: codersdk.NotificationsEmailConfig{
215-
ForceTLS: true,
216-
},
217+
name: "TLS: x509 untrusted",
218+
useTLS: true,
217219
expectedErr: "certificate is not trusted",
218220
retryable: true,
219221
},
220222
{
221223
// TLS is forced and self-signed certificate used by mock server is not verified.
222-
name: "TLS conn succeeds: x509 untrusted ignored",
224+
name: "TLS: x509 untrusted ignored",
223225
useTLS: true,
224226
cfg: codersdk.NotificationsEmailConfig{
225227
Hello: hello,
@@ -234,12 +236,11 @@ func TestSMTP(t *testing.T) {
234236
{
235237
// TLS is forced and STARTTLS is configured, but STARTTLS cannot be used by TLS connections.
236238
// STARTTLS should be disabled and connection should succeed.
237-
name: "TLS conn succeeds: STARTTLS is ignored",
239+
name: "TLS: STARTTLS is ignored",
238240
useTLS: true,
239241
cfg: codersdk.NotificationsEmailConfig{
240-
Hello: hello,
241-
From: from,
242-
ForceTLS: true,
242+
Hello: hello,
243+
From: from,
243244
TLS: codersdk.NotificationsEmailTLSConfig{
244245
InsecureSkipVerify: true,
245246
StartTLS: true,
@@ -249,7 +250,7 @@ func TestSMTP(t *testing.T) {
249250
},
250251
{
251252
// Plain connection is established and upgraded via STARTTLS, but certificate is untrusted.
252-
name: "TLS conn fails: STARTTLS untrusted",
253+
name: "TLS: STARTTLS untrusted",
253254
useTLS: false,
254255
cfg: codersdk.NotificationsEmailConfig{
255256
TLS: codersdk.NotificationsEmailTLSConfig{
@@ -263,7 +264,7 @@ func TestSMTP(t *testing.T) {
263264
},
264265
{
265266
// Plain connection is established and upgraded via STARTTLS, certificate is not verified.
266-
name: "TLS conn succeeds: STARTTLS",
267+
name: "TLS: STARTTLS",
267268
useTLS: false,
268269
cfg: codersdk.NotificationsEmailConfig{
269270
Hello: hello,
@@ -278,73 +279,68 @@ func TestSMTP(t *testing.T) {
278279
},
279280
{
280281
// TLS connection using self-signed certificate.
281-
name: "TLS conn succeeds: self-signed",
282+
name: "TLS: self-signed",
282283
useTLS: true,
283284
cfg: codersdk.NotificationsEmailConfig{
284285
Hello: hello,
285286
From: from,
286287
TLS: codersdk.NotificationsEmailTLSConfig{
287-
CAFile: "fixtures/ca.crt",
288-
CertFile: "fixtures/server.crt",
289-
KeyFile: "fixtures/server.key",
288+
CAFile: caFile,
289+
CertFile: certFile,
290+
KeyFile: keyFile,
290291
},
291-
ForceTLS: true,
292292
},
293293
toAddrs: []string{to},
294294
},
295295
{
296296
// TLS connection using self-signed certificate & specifying the DNS name configured in the certificate.
297-
name: "TLS conn succeeds: self-signed + SNI",
297+
name: "TLS: self-signed + SNI",
298298
useTLS: true,
299299
cfg: codersdk.NotificationsEmailConfig{
300300
Hello: hello,
301301
From: from,
302302
TLS: codersdk.NotificationsEmailTLSConfig{
303303
ServerName: "myserver.local",
304-
CAFile: "fixtures/ca.crt",
305-
CertFile: "fixtures/server.crt",
306-
KeyFile: "fixtures/server.key",
304+
CAFile: caFile,
305+
CertFile: certFile,
306+
KeyFile: keyFile,
307307
},
308-
ForceTLS: true,
309308
},
310309
toAddrs: []string{to},
311310
},
312311
{
313-
name: "TLS conn fails: load CA",
312+
name: "TLS: load CA",
314313
useTLS: true,
315314
cfg: codersdk.NotificationsEmailConfig{
316315
TLS: codersdk.NotificationsEmailTLSConfig{
317316
CAFile: "nope.crt",
318317
},
319-
ForceTLS: true,
320318
},
321319
expectedErr: "open nope.crt: no such file or directory",
322320
retryable: true,
323321
},
324322
{
325-
name: "TLS conn fails: load cert",
323+
name: "TLS: load cert",
326324
useTLS: true,
327325
cfg: codersdk.NotificationsEmailConfig{
328326
TLS: codersdk.NotificationsEmailTLSConfig{
329-
CAFile: "fixtures/ca.crt",
327+
CAFile: caFile,
330328
CertFile: "fixtures/nope.cert",
331-
KeyFile: "fixtures/server.key",
329+
KeyFile: keyFile,
332330
},
333-
ForceTLS: true,
334331
},
335332
expectedErr: "open fixtures/nope.cert: no such file or directory",
336333
retryable: true,
337334
},
338335
{
339-
name: "TLS conn fails: load cert key",
336+
name: "TLS: load cert key",
340337
useTLS: true,
341338
cfg: codersdk.NotificationsEmailConfig{
342339
TLS: codersdk.NotificationsEmailTLSConfig{
343-
CAFile: "fixtures/ca.crt",
344-
CertFile: "fixtures/server.crt",
340+
CAFile: caFile,
341+
CertFile: certFile,
345342
KeyFile: "fixtures/nope.key",
346343
},
347-
ForceTLS: true,
348344
},
349345
expectedErr: "open fixtures/nope.key: no such file or directory",
350346
retryable: true,
@@ -365,11 +361,10 @@ func TestSMTP(t *testing.T) {
365361
Password: password,
366362
},
367363
TLS: codersdk.NotificationsEmailTLSConfig{
368-
CAFile: "fixtures/ca.crt",
369-
CertFile: "fixtures/server.crt",
370-
KeyFile: "fixtures/server.key",
364+
CAFile: caFile,
365+
CertFile: certFile,
366+
KeyFile: keyFile,
371367
},
372-
ForceTLS: true,
373368
},
374369
toAddrs: []string{to},
375370
expectedAuthMeth: sasl.Plain,
@@ -382,6 +377,8 @@ func TestSMTP(t *testing.T) {
382377

383378
ctx := testutil.Context(t, testutil.WaitShort)
384379

380+
tc.cfg.ForceTLS = serpent.Bool(tc.useTLS)
381+
385382
backend := NewBackend(Config{
386383
AuthMechanisms: tc.authMechs,
387384

@@ -394,7 +391,6 @@ func TestSMTP(t *testing.T) {
394391
srv, listen, err := createMockSMTPServer(backend, tc.useTLS)
395392
require.NoError(t, err)
396393
t.Cleanup(func() {
397-
_ = listen.Close()
398394
// We expect that the server has already been closed in the test
399395
assert.ErrorIs(t, srv.Shutdown(ctx), smtp.ErrServerClosed)
400396
})

‎coderd/notifications/dispatch/smtp_util_test.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ type Message struct {
3434
Subject, Contents string // Content
3535
}
3636

37-
// The Backend implements SMTP server methods.
3837
type Backend struct {
3938
cfg Config
4039

@@ -61,7 +60,6 @@ func (b *Backend) Reset() {
6160
b.lastMsg = nil
6261
}
6362

64-
// A Session is returned after successful login.
6563
type Session struct {
6664
conn *smtp.Conn
6765
backend *Backend

‎codersdk/deployment.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -508,18 +508,18 @@ type NotificationsEmailConfig struct {
508508
Auth NotificationsEmailAuthConfig `json:"auth" typescript:",notnull"`
509509
// TLS details.
510510
TLS NotificationsEmailTLSConfig `json:"tls" typescript:",notnull"`
511-
// ForceTLS caused a TLS connection to be attempted.
511+
// ForceTLS causes a TLS connection to be attempted.
512512
ForceTLS serpent.Bool `json:"force_tls" typescript:",notnull"`
513513
}
514514

515515
type NotificationsEmailAuthConfig struct {
516-
// Identity used for PLAIN auth.
516+
// Identity for PLAIN auth.
517517
Identity serpent.String `json:"identity" typescript:",notnull"`
518-
// Username for LOGIN/PLAIN auth; authentication is disabled if this is left blank.
518+
// Username for LOGIN/PLAIN auth.
519519
Username serpent.String `json:"username" typescript:",notnull"`
520-
// Password to use for LOGIN/PLAIN auth.
520+
// Password for LOGIN/PLAIN auth.
521521
Password serpent.String `json:"password" typescript:",notnull"`
522-
// File from which to load the password to use for LOGIN/PLAIN auth.
522+
// File from which to load the password for LOGIN/PLAIN auth.
523523
PasswordFile serpent.String `json:"password_file" typescript:",notnull"`
524524
}
525525

@@ -528,6 +528,7 @@ func (c *NotificationsEmailAuthConfig) Empty() bool {
528528
}
529529

530530
type NotificationsEmailTLSConfig struct {
531+
// StartTLS attempts to upgrade plain connections to TLS.
531532
StartTLS serpent.Bool `json:"start_tls" typescript:",notnull"`
532533
// ServerName to verify the hostname for the targets.
533534
ServerName serpent.String `json:"server_name" typescript:",notnull"`

0 commit comments

Comments
 (0)