From 211650af8d8dc951141917d55aae3eedbcce81ac Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Mon, 7 Sep 2026 04:19:42 -0700 Subject: [PATCH] feat: send through every kind of SMTP server (issues #359-#361): negotiate the sign-in method from what the server advertises, preferring CRAM-MD5 then LOGIN then PLAIN, because sending AUTH PLAIN blind was refused by every server that offers only LOGIN, which is Microsoft 365 relays and most appliance relays, and that refusal was reported to the mailbox's owner as a wrong password and deactivated the account; put a deadline on the whole SMTP conversation so a peer that stops answering without closing the connection can no longer park a send goroutine forever, which only the dial was protected against; classify a refusal by its reply code so a permanent 5xx on the sender, the recipient or the message is reported as the rejection it is and not retried four times as though the server were offline, while a 4xx still retries; announce the sender's own domain in EHLO rather than net/smtp's localhost, which relays read as a spam signal; and share one AUTH LOGIN implementation with the notification mailer instead of keeping two copies of the code that handles credentials --- .../docs/development/troubleshooting.mdx | 2 + docs/content/docs/guides/mailboxes.mdx | 2 + internal/app/worker/health_record.go | 1 + internal/app/worker/wmail/send.go | 23 +- internal/client/smtpimap/smtp/auth.go | 105 +++++ internal/client/smtpimap/smtp/client.go | 83 +++- internal/client/smtpimap/smtp/replycode.go | 31 ++ internal/client/smtpimap/smtp/server_test.go | 389 ++++++++++++++++++ internal/email/smtp.go | 17 +- internal/errx/email.go | 26 ++ internal/notify/smtp.go | 58 +-- 11 files changed, 674 insertions(+), 63 deletions(-) create mode 100644 internal/client/smtpimap/smtp/auth.go create mode 100644 internal/client/smtpimap/smtp/replycode.go create mode 100644 internal/client/smtpimap/smtp/server_test.go diff --git a/docs/content/docs/development/troubleshooting.mdx b/docs/content/docs/development/troubleshooting.mdx index 6f14e24a..0cc95ace 100644 --- a/docs/content/docs/development/troubleshooting.mdx +++ b/docs/content/docs/development/troubleshooting.mdx @@ -78,6 +78,8 @@ Newer builds return the invite-only refusal with its own machine code, `registra | "No mailbox workers are available" when connecting a mailbox | No worker has a heartbeat inside the last 10 minutes. Check `make status` shows `worker` running and `make logs worker` is clean. An empty `ENCRYPTED_KEYS_BACKEND_URL` or worker token lets a worker start and never register, silently | | Connecting a mailbox fails with `SERVER_UNREACHABLE` on a reachable host | The security setting does not match the server. A server expecting STARTTLS looks unreachable to a client attempting implicit TLS, and vice versa. Any port from 1 to 65535 is accepted, so the port alone no longer decides: set **Security** to SSL / TLS for a server that is encrypted from the first byte (usually SMTP `465`, IMAP `993`) and STARTTLS for one that upgrades in place (usually SMTP `587` or `2525`, IMAP `143`) | | A mailbox stalls after about an hour | The worker is missing `BOX_GOOGLE_*` or `BOX_OUTLOOK_*`. The backend starts the OAuth flow but each worker refreshes the token. Set them and restart the worker | +| Sending fails with an authentication error but the password is right | Warmbly negotiates the sign-in method from what the server advertises. If the mailbox reports `AUTH_UNSUPPORTED`, the server offers only mechanisms Warmbly does not implement, such as NTLM or GSSAPI; an app password, or the provider's documented SMTP host, usually offers a standard one | +| A send is refused and not retried | A `SEND_REJECTED` or `RECIPIENT_REJECTED` error means the receiving server answered with a permanent `5xx`, so retrying cannot deliver the message and would only spend the mailbox's daily budget. The server's own words are in the error. A temporary `4xx` is retried automatically and reported as a connection problem | | An IMAP mailbox connects but no mail ever arrives | Check the sync card in the mailbox drawer for a folder count. Zero folders on a reachable server means the server refused `STATUS` for every folder; the worker log names each one it skipped. A connected mailbox whose inbox is genuinely empty is normal | | An IMAP mailbox stopped syncing and the log is quiet | Sessions dropped by the server, or by a firewall that removed the mapping without closing the connection, are re-dialed on the next pass, and every pass that cannot reach the server is retried on a widening interval up to five minutes. If a mailbox is still stuck, `make logs worker` shows the folder cursors; a mailbox held by the sync budget says so in its drawer instead | | A folder is missing from the unibox | Up to `100` folders per mailbox are synced (**Instance settings > Limits**). Past that, the inbox and the special folders are kept and the rest follow the server's order, with a warning in the drawer naming how many were left out. Gmail's All Mail, Starred and Important are label views over other folders and are deliberately never synced. A folder can also be skipped when the mail server gives it the same internal id (`UIDVALIDITY`) as another folder, which happens on servers that derive that id from the creation time; the drawer says so, and renaming or recreating the folder gives it a new one | diff --git a/docs/content/docs/guides/mailboxes.mdx b/docs/content/docs/guides/mailboxes.mdx index ede79077..c3a427ba 100644 --- a/docs/content/docs/guides/mailboxes.mdx +++ b/docs/content/docs/guides/mailboxes.mdx @@ -39,6 +39,8 @@ When a mail server goes down or stops answering, the mailbox is not deactivated. With two-factor authentication on, generate an app password in your provider's security settings and use that. +Warmbly signs in with whichever method your server offers, preferring CRAM-MD5, then LOGIN, then PLAIN. Servers that accept only one of these, which includes Microsoft 365 relays and most appliance relays, work without any setting to change. + Credentials and both connections are validated when you add the account, so wrong settings fail immediately rather than silently at send time. Tokens and credentials are sealed with envelope encryption before they touch storage. diff --git a/internal/app/worker/health_record.go b/internal/app/worker/health_record.go index 4eec4493..31caaf09 100644 --- a/internal/app/worker/health_record.go +++ b/internal/app/worker/health_record.go @@ -44,6 +44,7 @@ func (s *WorkerService) recordSendOutcome(result *wmail.SendResult) { errx.MailErrorCodeQuotaExceeded: s.RecordRateLimitError() case errx.MailErrorCodeRecipientRejected, + errx.MailErrorCodeSendRejected, errx.MailErrorCodeAccountSuspended, // Refused on the sending domain's authentication: a hard rejection // that stays hard until DNS is fixed, so it must not be counted as a diff --git a/internal/app/worker/wmail/send.go b/internal/app/worker/wmail/send.go index a8c64873..5cad8902 100644 --- a/internal/app/worker/wmail/send.go +++ b/internal/app/worker/wmail/send.go @@ -108,6 +108,21 @@ type SendResult struct { const maxSendRetries = 3 +// permanentSendFailure is a refusal no retry can turn into a delivery: the +// receiving server rejected the sender, the recipient or the message itself +// with a 5xx. Named codes rather than the resolve method, because several +// warnings carry a non-retry method while still being worth another attempt. +func permanentSendFailure(err *errx.MailError) bool { + if err == nil { + return false + } + switch err.Code { + case errx.MailErrorCodeSendRejected, errx.MailErrorCodeRecipientRejected: + return true + } + return false +} + // Send attempts to send an email with retry for transient failures func (w *WMail) Send(ctx context.Context, req *SendRequest) *SendResult { // For warmup emails, ensure HTML is empty @@ -141,10 +156,16 @@ func (w *WMail) Send(ctx context.Context, req *SendRequest) *SendResult { return result } - // Don't retry critical/auth errors - only transient ones + // Don't retry critical/auth errors - only transient ones. if result.Error != nil && result.Error.Type == errx.MailErrorCritical { return result } + // A permanent refusal is not critical but is just as final: the + // server answered with a 5xx, so it will answer the same way next + // time and another attempt only spends the mailbox's daily budget. + if permanentSendFailure(result.Error) { + return result + } if attempt < maxSendRetries { backoff := time.Duration(1<= 0 && at+1 < len(address) { + return address[at+1:] + } + return "" +} + type Client struct { FirstName string LastName string @@ -34,6 +49,11 @@ type Client struct { Credentials *models.Service Oauth2 *models.Oauth2Service + // plaintext skips the STARTTLS requirement. Only the tests set it, to + // talk to an in-process server; no product path reaches it, because SMTP + // AUTH in the clear would put the mailbox password on the wire. + plaintext bool + // BindIP optionally pins outbound TCP to a specific local source address. // When nil, WORKER_BIND_IP is consulted; when still unset, the OS default // route is used. @@ -235,9 +255,18 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b addr := fmt.Sprintf("%s:%d", host, port) tlsConf := &tls.Config{ ServerName: host, - InsecureSkipVerify: netbind.InsecureTLS(), + InsecureSkipVerify: netbind.InsecureTLS(), //nolint:gosec // MAIL_TLS_INSECURE, local dev only + MinVersion: tls.VersionTLS12, } + // Everything after the dial gets a deadline. net/smtp sets none of its + // own and only the connect was bounded, so a peer that stopped answering + // without closing the connection parked the send goroutine forever: the + // greeting, AUTH, each RCPT and the wait for the server's verdict at the + // end of DATA all block with nothing to fail them. + ctx, cancel := context.WithTimeout(ctx, sendTimeout) + defer cancel() + var conn net.Conn var err error // Implicit TLS (SMTPS) means the server speaks TLS from the first byte, so @@ -253,6 +282,9 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b return errx.ErrMailServerUnreachable } defer conn.Close() + if deadline, ok := ctx.Deadline(); ok { + _ = conn.SetDeadline(deadline) + } // Use the resolved host: c.Credentials is nil for OAuth2-configured clients. client, err := smtp.NewClient(conn, host) @@ -261,6 +293,14 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b } defer client.Quit() + // Announce the sender's own domain. net/smtp says "localhost" when left + // alone, which relays read as a spam signal. + if name := ehloName(c.Email); name != "" { + if err := client.Hello(name); err != nil { + return errx.ErrMailServerUnreachable + } + } + // TLS is mandatory. The MAIL_TLS_INSECURE dev knob additionally allows a // server with no STARTTLS at all (the local mailpit sink) — never taken in // production, where the env var is unset. @@ -269,7 +309,7 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b if err := client.StartTLS(tlsConf); err != nil { return errx.ErrMailServerUnreachable } - } else if !netbind.InsecureTLS() { + } else if !netbind.InsecureTLS() && !c.plaintext { return errx.ErrMailServerUnreachable } } @@ -277,9 +317,24 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b // --- Auth --- switch c.AuthType { case models.AuthPlain: - auth := smtp.PlainAuth("", c.Credentials.Username, c.Credentials.Password, c.Credentials.Host) - if err := client.Auth(auth); err != nil { - return errx.ErrMailInvalidCredentials + // Negotiated, not assumed: a server that advertises only LOGIN + // rejects a blind AUTH PLAIN, and that rejection reads exactly like a + // wrong password, so the mailbox was deactivated over credentials + // that were correct. + auth, aerr := NegotiateAuth(client, c.Credentials.Username, c.Credentials.Password, c.Credentials.Host) + if aerr != nil { + return errx.ErrMailAuthUnsupported + } + if auth != nil { + if err := client.Auth(auth); err != nil { + // A 4xx is the server saying "not now" (rate-limited AUTH, a + // backend it cannot reach); only a 5xx means the credentials + // themselves are refused. + if !permanentReply(err) { + return errx.ErrMailServerUnreachable + } + return errx.ErrMailInvalidCredentials + } } case models.AuthOAuth2: tk, err := c.Oauth2.Token.Token() @@ -300,6 +355,15 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b } if err := client.Mail(from); err != nil { + // A refused MAIL FROM is how a blocked sender, an over-quota mailbox + // and a relay-denied policy arrive. Retrying a 5xx never succeeds and + // reports an outage that is not happening. + if permanentReply(err) { + if isDomainAuthRejection(err) { + return errx.ErrMailDomainAuthRejected + } + return errx.ErrMailSendRejected(err.Error()) + } return errx.ErrMailServerUnreachable } for _, r := range to { @@ -311,7 +375,11 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b } // A refused RCPT is a recipient problem (bad address, policy // rejection), not a dead server; classifying it as unreachable - // hid rejections from bounce accounting. + // hid rejections from bounce accounting. A 4xx is greylisting or + // a busy server, which is worth another attempt. + if !permanentReply(err) { + return errx.ErrMailServerUnreachable + } return errx.ErrMailRecipientRejected } } @@ -328,6 +396,9 @@ func (c *Client) sendRaw(ctx context.Context, from string, to []string, data []b if isDomainAuthRejection(err) { return errx.ErrMailDomainAuthRejected } + if permanentReply(err) { + return errx.ErrMailSendRejected(err.Error()) + } return errx.ErrMailServerUnreachable } diff --git a/internal/client/smtpimap/smtp/replycode.go b/internal/client/smtpimap/smtp/replycode.go new file mode 100644 index 00000000..96a17ddd --- /dev/null +++ b/internal/client/smtpimap/smtp/replycode.go @@ -0,0 +1,31 @@ +package smtp + +import ( + "errors" + "net/textproto" +) + +// replyCode is the three-digit status the server answered a command with, or +// 0 when the failure was not a reply at all (a dropped connection, a timeout, +// a TLS error). net/smtp surfaces every reply as a *textproto.Error. +func replyCode(err error) int { + var proto *textproto.Error + if errors.As(err, &proto) { + return proto.Code + } + return 0 +} + +// permanentReply reports whether the server refused for good. 5xx means it +// will refuse the same message again, so retrying wastes the mailbox's daily +// budget and delays the campaign; 4xx explicitly invites a retry, and a +// failure with no reply code at all is the transport, which is also worth +// retrying. +// +// Reading the code is what lets a blocked sender or an over-quota mailbox be +// reported as what it is instead of as "the server may be offline", which is +// what every post-connection failure used to be called. +func permanentReply(err error) bool { + code := replyCode(err) + return code >= 500 && code < 600 +} diff --git a/internal/client/smtpimap/smtp/server_test.go b/internal/client/smtpimap/smtp/server_test.go new file mode 100644 index 00000000..ab770257 --- /dev/null +++ b/internal/client/smtpimap/smtp/server_test.go @@ -0,0 +1,389 @@ +package smtp + +import ( + "bufio" + "context" + "encoding/base64" + "fmt" + "net" + "strings" + "sync" + "testing" + "time" + + "github.com/warmbly/warmbly/internal/models" +) + +// fakeServer is a hand-rolled SMTP server, because the mechanisms that matter +// here are the ones a real server chooses to advertise and net/smtp ships no +// server to configure. It speaks only enough to get a message accepted. +type fakeServer struct { + ln net.Listener + // authAdvertised is the AUTH parameter list, e.g. "LOGIN" or "PLAIN LOGIN". + // Empty advertises no AUTH extension at all. + authAdvertised string + // reject, when set, is the reply given to the command whose verb it is + // keyed by ("MAIL", "RCPT", "AUTH", "DATA-END"). + reject map[string]string + + mu sync.Mutex + // seen records the commands the client actually sent, so a test can prove + // which mechanism was chosen rather than only that the send succeeded. + seen []string + // credentials are what the client supplied, decoded. + credentials []string +} + +func newFakeServer(t *testing.T, authAdvertised string) *fakeServer { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + s := &fakeServer{ln: ln, authAdvertised: authAdvertised, reject: map[string]string{}} + t.Cleanup(func() { _ = ln.Close() }) + go s.serve() + return s +} + +func (s *fakeServer) addr() (string, int) { + host, port, _ := net.SplitHostPort(s.ln.Addr().String()) + p := 0 + for _, r := range port { + p = p*10 + int(r-'0') + } + return host, p +} + +func (s *fakeServer) record(cmd string) { + s.mu.Lock() + s.seen = append(s.seen, cmd) + s.mu.Unlock() +} + +func (s *fakeServer) sawPrefix(prefix string) bool { + s.mu.Lock() + defer s.mu.Unlock() + for _, c := range s.seen { + if strings.HasPrefix(strings.ToUpper(c), strings.ToUpper(prefix)) { + return true + } + } + return false +} + +func (s *fakeServer) serve() { + for { + conn, err := s.ln.Accept() + if err != nil { + return + } + go s.session(conn) + } +} + +func (s *fakeServer) session(conn net.Conn) { + defer func() { _ = conn.Close() }() + w := func(format string, a ...any) { _, _ = fmt.Fprintf(conn, format+"\r\n", a...) } + br := bufio.NewReader(conn) + + w("220 fake ESMTP") + inData := false + for { + line, err := br.ReadString('\n') + if err != nil { + return + } + line = strings.TrimRight(line, "\r\n") + + if inData { + if line == "." { + inData = false + if reply, bad := s.reject["DATA-END"]; bad { + w("%s", reply) + continue + } + w("250 2.0.0 accepted") + } + continue + } + + s.record(line) + verb := strings.ToUpper(line) + switch { + case strings.HasPrefix(verb, "EHLO"): + w("250-fake greets you") + if s.authAdvertised != "" { + w("250-AUTH %s", s.authAdvertised) + } + w("250 8BITMIME") + case strings.HasPrefix(verb, "HELO"): + w("250 fake") + case strings.HasPrefix(verb, "AUTH LOGIN"): + if reply, bad := s.reject["AUTH"]; bad { + w("%s", reply) + continue + } + w("334 %s", base64.StdEncoding.EncodeToString([]byte("Username:"))) + user, _ := br.ReadString('\n') + s.decode(strings.TrimSpace(user)) + w("334 %s", base64.StdEncoding.EncodeToString([]byte("Password:"))) + pass, _ := br.ReadString('\n') + s.decode(strings.TrimSpace(pass)) + w("235 2.7.0 authenticated") + case strings.HasPrefix(verb, "AUTH PLAIN"): + if reply, bad := s.reject["AUTH"]; bad { + w("%s", reply) + continue + } + w("235 2.7.0 authenticated") + case strings.HasPrefix(verb, "AUTH"): + // A mechanism this server does not implement. + w("504 5.5.4 unrecognized authentication type") + case strings.HasPrefix(verb, "MAIL"): + if reply, bad := s.reject["MAIL"]; bad { + w("%s", reply) + continue + } + w("250 2.1.0 ok") + case strings.HasPrefix(verb, "RCPT"): + if reply, bad := s.reject["RCPT"]; bad { + w("%s", reply) + continue + } + w("250 2.1.5 ok") + case strings.HasPrefix(verb, "DATA"): + inData = true + w("354 go ahead") + case strings.HasPrefix(verb, "QUIT"): + w("221 2.0.0 bye") + return + case strings.HasPrefix(verb, "RSET"), strings.HasPrefix(verb, "NOOP"): + w("250 2.0.0 ok") + default: + w("500 5.5.1 unrecognized") + } + } +} + +func (s *fakeServer) decode(b64 string) { + raw, err := base64.StdEncoding.DecodeString(b64) + if err != nil { + return + } + s.mu.Lock() + s.credentials = append(s.credentials, string(raw)) + s.mu.Unlock() +} + +// A server that advertises only LOGIN is the case that could not send at all: +// the client sent AUTH PLAIN blind, the server refused, and the refusal was +// reported to the mailbox's owner as a wrong password. +func TestNegotiateAuthPrefersWhatTheServerAdvertises(t *testing.T) { + for _, tc := range []struct { + advertised string + wantCmd string + }{ + {"LOGIN", "AUTH LOGIN"}, + {"PLAIN", "AUTH PLAIN"}, + // Both offered: LOGIN, because a server that speaks only one of the + // two speaks LOGIN, and preferring it costs nothing here. + {"PLAIN LOGIN", "AUTH LOGIN"}, + {"CRAM-MD5 PLAIN LOGIN", "AUTH CRAM-MD5"}, + } { + srv := newFakeServer(t, tc.advertised) + host, port := srv.addr() + c := newTestClient(host, port) + + err := c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")) + if tc.advertised == "CRAM-MD5 PLAIN LOGIN" { + // The fake does not implement the CRAM-MD5 exchange; what matters + // is that the client asked for it. + if !srv.sawPrefix(tc.wantCmd) { + t.Errorf("advertised %q: client never sent %q", tc.advertised, tc.wantCmd) + } + continue + } + if err != nil { + t.Errorf("advertised %q: send failed: %v", tc.advertised, err.Message) + } + if !srv.sawPrefix(tc.wantCmd) { + t.Errorf("advertised %q: client never sent %q, sent %v", tc.advertised, tc.wantCmd, srv.seen) + } + } +} + +// The LOGIN exchange has to hand over the real credentials, not just pick the +// mechanism. +func TestLoginAuthSendsTheCredentials(t *testing.T) { + srv := newFakeServer(t, "LOGIN") + host, port := srv.addr() + c := newTestClient(host, port) + + if err := c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")); err != nil { + t.Fatalf("send: %v", err.Message) + } + srv.mu.Lock() + got := append([]string(nil), srv.credentials...) + srv.mu.Unlock() + if len(got) != 2 || got[0] != "user@warmbly.test" || got[1] != "hunter2" { + t.Fatalf("server received %q, want the username then the password", got) + } +} + +// A server advertising no AUTH wants none. Refusing to send would break the +// local development sink and any relay that authorizes by IP. +func TestSendWithoutAuthExtension(t *testing.T) { + srv := newFakeServer(t, "") + host, port := srv.addr() + c := newTestClient(host, port) + + if err := c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")); err != nil { + t.Fatalf("send to a server with no AUTH: %v", err.Message) + } + if srv.sawPrefix("AUTH") { + t.Error("client authenticated against a server that advertises no AUTH") + } +} + +// The sender's own domain goes in EHLO. net/smtp says "localhost" when left +// alone, which relays read as a spam signal. +func TestEHLOAnnouncesTheSenderDomain(t *testing.T) { + srv := newFakeServer(t, "LOGIN") + host, port := srv.addr() + c := newTestClient(host, port) + + _ = c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")) + if !srv.sawPrefix("EHLO warmbly.test") { + t.Errorf("EHLO did not announce the sender domain: %v", srv.seen) + } +} + +// A 5xx is the server's final answer. Retrying it cannot deliver the message +// and spends the mailbox's daily budget, so it must be distinguishable from +// an outage, which is what every one of these used to be reported as. +func TestPermanentRefusalsAreNotReportedAsAnOutage(t *testing.T) { + for _, tc := range []struct { + name string + at string + reply string + wantCode string + }{ + {"sender blocked", "MAIL", "550 5.7.1 sender denied", "SEND_REJECTED"}, + {"recipient unknown", "RCPT", "550 5.1.1 no such user", "RECIPIENT_REJECTED"}, + {"message refused", "DATA-END", "554 5.7.1 message rejected", "SEND_REJECTED"}, + // A 4xx invites a retry, so it stays an outage-shaped error. + {"sender throttled", "MAIL", "451 4.7.1 try again later", "SERVER_UNREACHABLE"}, + {"recipient greylisted", "RCPT", "450 4.2.0 greylisted", "SERVER_UNREACHABLE"}, + {"message deferred", "DATA-END", "451 4.3.0 try later", "SERVER_UNREACHABLE"}, + } { + srv := newFakeServer(t, "LOGIN") + srv.reject[tc.at] = tc.reply + host, port := srv.addr() + c := newTestClient(host, port) + + err := c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")) + if err == nil { + t.Errorf("%s: send succeeded against %q", tc.name, tc.reply) + continue + } + if string(err.Code) != tc.wantCode { + t.Errorf("%s (%q): code = %q, want %q", tc.name, tc.reply, err.Code, tc.wantCode) + } + } +} + +// A refused AUTH is only a credentials problem when the server says so with a +// 5xx. A 4xx is the server declining for now, and deactivating the mailbox +// over it tells the owner their password is wrong when it is not. +func TestTransientAuthFailureIsNotACredentialsProblem(t *testing.T) { + srv := newFakeServer(t, "LOGIN") + srv.reject["AUTH"] = "454 4.7.0 temporary authentication failure" + host, port := srv.addr() + c := newTestClient(host, port) + + err := c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")) + if err == nil { + t.Fatal("send succeeded despite a refused AUTH") + } + if string(err.Code) != "SERVER_UNREACHABLE" { + t.Errorf("code = %q, want the transient classification", err.Code) + } +} + +// A server whose advertised mechanisms we do not implement should say so, +// rather than reporting the password as wrong. +func TestUnsupportedAuthMechanismIsItsOwnError(t *testing.T) { + srv := newFakeServer(t, "GSSAPI NTLM") + host, port := srv.addr() + c := newTestClient(host, port) + + err := c.sendRaw(t.Context(), "sender@warmbly.test", []string{"to@example.test"}, []byte("Subject: hi\r\n\r\nbody\r\n")) + if err == nil { + t.Fatal("send succeeded against a server offering no mechanism we speak") + } + if string(err.Code) != "AUTH_UNSUPPORTED" { + t.Errorf("code = %q, want AUTH_UNSUPPORTED", err.Code) + } +} + +// A peer that accepts the connection and then says nothing must not hold the +// send goroutine forever. Only the dial was bounded before, and net/smtp sets +// no deadline of its own, so a dropped NAT mapping parked the send. +func TestSendTimesOutOnASilentPeer(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + defer func() { _ = ln.Close() }() + go func() { + conn, err := ln.Accept() + if err != nil { + return + } + defer func() { _ = conn.Close() }() + select {} + }() + + host, portStr, _ := net.SplitHostPort(ln.Addr().String()) + port := 0 + for _, r := range portStr { + port = port*10 + int(r-'0') + } + c := newTestClient(host, port) + + ctx, cancel := contextWithDeadline(t, 3*time.Second) + defer cancel() + done := make(chan struct{}) + go func() { + _ = c.sendRaw(ctx, "sender@warmbly.test", []string{"to@example.test"}, []byte("body")) + close(done) + }() + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("a send against a peer that never answers did not return: the goroutine is parked forever") + } +} + +// newTestClient points a client at the in-process fake, which speaks no TLS. +func newTestClient(host string, port int) *Client { + return &Client{ + FirstName: "Test", + Email: "sender@warmbly.test", + AuthType: models.AuthPlain, + Credentials: &models.Service{ + Username: "user@warmbly.test", + Password: "hunter2", + Host: host, + Port: port, + Security: models.MailSecurityStartTLS, + }, + plaintext: true, + } +} + +func contextWithDeadline(t *testing.T, d time.Duration) (context.Context, context.CancelFunc) { + t.Helper() + return context.WithTimeout(t.Context(), d) +} diff --git a/internal/email/smtp.go b/internal/email/smtp.go index 446b1312..73318a83 100644 --- a/internal/email/smtp.go +++ b/internal/email/smtp.go @@ -8,6 +8,7 @@ import ( "net/smtp" "github.com/warmbly/warmbly/internal/client/netbind" + wsmtp "github.com/warmbly/warmbly/internal/client/smtpimap/smtp" "github.com/warmbly/warmbly/internal/models" ) @@ -22,7 +23,8 @@ func VerifySMTP(ctx context.Context, host string, port int, user, pass, security // knob for the local self-signed sandbox, never set in production. tlsConf := &tls.Config{ ServerName: host, - InsecureSkipVerify: netbind.InsecureTLS(), + InsecureSkipVerify: netbind.InsecureTLS(), //nolint:gosec // MAIL_TLS_INSECURE, local dev only + MinVersion: tls.VersionTLS12, } var conn net.Conn @@ -62,7 +64,18 @@ func VerifySMTP(ctx context.Context, host string, port int, user, pass, security } } - auth := smtp.PlainAuth("", user, pass, host) + // Negotiated from what the server advertised, like the send path: a + // server that offers only LOGIN refuses a blind AUTH PLAIN, and probing + // with PLAIN alone rejected mailboxes whose credentials were correct. + auth, aerr := wsmtp.NegotiateAuth(c, user, pass, host) + if aerr != nil { + return false + } + if auth == nil { + // No AUTH offered at all: nothing to verify, and the send path will + // not authenticate either. + return true + } done := make(chan error, 1) go func() { done <- c.Auth(auth) }() diff --git a/internal/errx/email.go b/internal/errx/email.go index b33a401a..0b7ea16f 100644 --- a/internal/errx/email.go +++ b/internal/errx/email.go @@ -62,6 +62,14 @@ const ( MailErrorCodeSyncFairUse MailErrorCode = "SYNC_FAIR_USE" MailErrorCodeSendingTooFast MailErrorCode = "SENDING_TOO_FAST" MailErrorCodeRecipientRejected MailErrorCode = "RECIPIENT_REJECTED" + // MailErrorCodeSendRejected is the receiving server refusing the message + // or the sender for good (a 5xx on MAIL FROM or at the end of DATA). + // Distinct from RECIPIENT_REJECTED, which is one address, and from + // SERVER_UNREACHABLE, which is worth retrying. + MailErrorCodeSendRejected MailErrorCode = "SEND_REJECTED" + // MailErrorCodeAuthUnsupported is a server whose advertised + // authentication mechanisms we do not implement. + MailErrorCodeAuthUnsupported MailErrorCode = "AUTH_UNSUPPORTED" // MailErrorCodeDomainAuthRejected is the receiving side refusing the mail // because the SENDING DOMAIN failed its authentication bar (Outlook's // 5.7.515, Gmail's 5.7.26). Not a dead server and not a bad recipient: @@ -207,6 +215,18 @@ var ( "The recipient email address was rejected by the mail server.", MailErrorResolveMethodNone, ) + // ErrMailSendRejected is a permanent refusal of the message itself. Not + // retried: a 5xx means the server will answer the same way next time, so + // another attempt only spends the mailbox's daily budget. + ErrMailSendRejected = func(detail string) *MailError { + return MError(MailErrorWarning, MailErrorCodeSendRejected, fmt.Sprintf("The receiving mail server refused this message: %s", detail), MailErrorResolveMethodNone) + } + ErrMailAuthUnsupported = MError( + MailErrorCritical, + MailErrorCodeAuthUnsupported, + "This mail server asks for a sign-in method Warmbly does not support. Check the server's documentation for an app password or an alternative SMTP host.", + MailErrorResolveMethodReload, + ) ErrMailDomainAuthRejected = MError( MailErrorCritical, MailErrorCodeDomainAuthRejected, @@ -278,6 +298,12 @@ func (e *MailError) GetUserErrorInfo() UserErrorInfo { case MailErrorCodeAccountSuspended: info.Title = "Account Suspended" info.ActionRequired = "Contact your email provider to resolve this issue" + case MailErrorCodeSendRejected: + info.Title = "Message refused" + info.ActionRequired = "The receiving server rejected this message outright. The reason it gave is in the message above." + case MailErrorCodeAuthUnsupported: + info.Title = "Sign-in method not supported" + info.ActionRequired = "This server asks for an authentication method Warmbly does not support. An app password, or the provider's documented SMTP host, usually works." case MailErrorCodeRecipientRejected: info.Title = "Recipient Rejected" info.ActionRequired = "The recipient address was not accepted" diff --git a/internal/notify/smtp.go b/internal/notify/smtp.go index d7e50d75..8722237a 100644 --- a/internal/notify/smtp.go +++ b/internal/notify/smtp.go @@ -12,6 +12,7 @@ import ( "time" "github.com/getsentry/sentry-go" + wsmtp "github.com/warmbly/warmbly/internal/client/smtpimap/smtp" "github.com/warmbly/warmbly/internal/config" ) @@ -171,7 +172,7 @@ func (s *smtpEmailNotificationService) authenticate(client *smtp.Client) error { // Credentials only travel over an encrypted link. Loopback is exempt so a // sidecar relay on the same host still works. - if _, isTLS := client.TLSConnectionState(); !isTLS && !isLoopback(s.cfg.Host) { + if _, isTLS := client.TLSConnectionState(); !isTLS && !wsmtp.IsLoopbackHost(s.cfg.Host) { return ErrSMTPCleartextAuth } @@ -193,7 +194,7 @@ func (s *smtpEmailNotificationService) authMechanism(client *smtp.Client) (smtp. case config.SMTPAuthPlain: return smtp.PlainAuth("", s.cfg.Username, s.cfg.Password, s.cfg.Host), nil case config.SMTPAuthLogin: - return newLoginAuth(s.cfg.Username, s.cfg.Password, s.cfg.Host), nil + return wsmtp.NewLoginAuth(s.cfg.Username, s.cfg.Password, s.cfg.Host), nil case config.SMTPAuthCRAMMD5: return smtp.CRAMMD5Auth(s.cfg.Username, s.cfg.Password), nil } @@ -210,7 +211,7 @@ func (s *smtpEmailNotificationService) authMechanism(client *smtp.Client) (smtp. case strings.Contains(mechs, "CRAM-MD5"): return smtp.CRAMMD5Auth(s.cfg.Username, s.cfg.Password), nil case strings.Contains(mechs, "LOGIN"): - return newLoginAuth(s.cfg.Username, s.cfg.Password, s.cfg.Host), nil + return wsmtp.NewLoginAuth(s.cfg.Username, s.cfg.Password, s.cfg.Host), nil case strings.Contains(mechs, "PLAIN"): return smtp.PlainAuth("", s.cfg.Username, s.cfg.Password, s.cfg.Host), nil default: @@ -229,54 +230,3 @@ func (s *smtpEmailNotificationService) ehloName() string { } return "" } - -func isLoopback(host string) bool { - if host == "localhost" { - return true - } - if ip := net.ParseIP(host); ip != nil { - return ip.IsLoopback() - } - return false -} - -// loginAuth implements the non-standard but widely required AUTH LOGIN -// mechanism, which net/smtp does not ship. Exchange Online and many appliance -// relays advertise it exclusively. -type loginAuth struct { - username string - password string - host string -} - -func newLoginAuth(username, password, host string) smtp.Auth { - return &loginAuth{username: username, password: password, host: host} -} - -func (a *loginAuth) Start(server *smtp.ServerInfo) (string, []byte, error) { - if !server.TLS && !isLoopback(server.Name) { - return "", nil, ErrSMTPCleartextAuth - } - return "LOGIN", nil, nil -} - -func (a *loginAuth) Next(fromServer []byte, more bool) ([]byte, error) { - if !more { - return nil, nil - } - // Servers vary in how they word the prompts, so match on the decoded - // challenge rather than expecting an exact string. - switch strings.ToLower(string(fromServer)) { - case "username:", "user name": - return []byte(a.username), nil - case "password:": - return []byte(a.password), nil - } - if strings.Contains(strings.ToLower(string(fromServer)), "user") { - return []byte(a.username), nil - } - if strings.Contains(strings.ToLower(string(fromServer)), "pass") { - return []byte(a.password), nil - } - return nil, fmt.Errorf("smtp: unexpected LOGIN challenge %q", string(fromServer)) -}