deps: remove go-smtp and go-sasl in favor of net/smtp#2467
deps: remove go-smtp and go-sasl in favor of net/smtp#2467
Conversation
Replace emersion/go-smtp and emersion/go-sasl with stdlib net/smtp. Implement loginAuth for SMTP LOGIN authentication (~25 lines). Use tls.Dial + smtp.NewClient for SMTPS, smtp.Dial + StartTLS for STARTTLS. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Replaces third-party SMTP client/auth dependencies (emersion/go-smtp, emersion/go-sasl) with Go’s net/smtp, including a small self-implementation of the LOGIN auth mechanism.
Changes:
- Refactored SMTP connection logic to use
net/smtpfor SMTPS (implicit TLS), STARTTLS, and plain connections. - Added
loginAuthimplementingsmtp.Authfor theLOGINmechanism and corresponding unit tests. - Removed
emersion/go-smtp/emersion/go-saslfromgo.modandgo.sum.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| reporter/email.go | Migrates SMTP send/auth code to net/smtp, adds LOGIN auth implementation, and rewrites TLS dialing paths. |
| reporter/loginauth_test.go | Adds unit tests covering the new LOGIN auth implementation. |
| go.mod | Drops direct dependencies on emersion/go-smtp and emersion/go-sasl. |
| go.sum | Removes checksums for the removed SMTP/SASL dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (e *emailSender) dialTLS(addr string, tlsConfig *tls.Config) (*smtp.Client, error) { | ||
| conn, err := tls.Dial("tcp", addr, tlsConfig) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("Failed to create TLS connection to SMTP server: %w", err) | ||
| } | ||
| host, _, _ := net.SplitHostPort(addr) | ||
| c, err := smtp.NewClient(conn, host) | ||
| if err != nil { | ||
| _ = conn.Close() | ||
| return nil, xerrors.Errorf("Failed to create SMTP client over TLS: %w", err) | ||
| } |
There was a problem hiding this comment.
net.SplitHostPort(addr) errors are ignored; if parsing fails (e.g., unexpected addr format), host may be empty and the SMTP client will be initialized with an incorrect server name. Handle the error explicitly and fail fast (closing the TLS conn) or fall back to a known-good host value (e.g., the configured hostname) rather than proceeding with an empty/invalid host.
There was a problem hiding this comment.
Fixed in fd3f0dd. net.SplitHostPort error is now checked and returns an error with conn.Close() cleanup, consistent with the existing error-handling pattern in dialTLS.
reporter/email.go
Outdated
| username, password string | ||
| } | ||
|
|
||
| func (a *loginAuth) Start(_ *smtp.ServerInfo) (string, []byte, error) { |
There was a problem hiding this comment.
loginAuth currently ignores smtp.ServerInfo, so it can attempt LOGIN authentication even when the connection is not encrypted. This can leak credentials in cleartext for TLS mode "None" (or if STARTTLS wasn’t negotiated). Consider enforcing a minimum safety check similar to smtp.PlainAuth (e.g., require server.TLS == true unless connecting to localhost, and optionally validate server.Name against the expected host) and return an error otherwise.
| func (a *loginAuth) Start(_ *smtp.ServerInfo) (string, []byte, error) { | |
| func (a *loginAuth) Start(server *smtp.ServerInfo) (string, []byte, error) { | |
| if server == nil { | |
| return "", nil, errors.New("smtp: server info is required for LOGIN auth") | |
| } | |
| // Enforce a minimum safety check similar to smtp.PlainAuth: | |
| // require TLS unless we're connecting to localhost. | |
| if !server.TLS { | |
| host := server.Name | |
| if h, _, err := net.SplitHostPort(server.Name); err == nil { | |
| host = h | |
| } | |
| switch host { | |
| case "localhost", "127.0.0.1", "::1": | |
| // Allow insecure localhost connections. | |
| default: | |
| return "", nil, errors.New("smtp: refusing LOGIN authentication over unencrypted connection") | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in fd3f0dd. loginAuth.Start now checks server.TLS and, if the connection is not TLS, verifies that the server explicitly advertises LOGIN in its auth mechanisms. This mirrors the approach of smtp.PlainAuth in the stdlib, which also guards against sending credentials over unencrypted connections.
reporter/email.go
Outdated
| defer c.Close() | ||
| default: | ||
| return xerrors.New(`invalid TLS mode. accepts: ["", "None", "STARTTLS", "SMTPS"]`) | ||
| return errors.New(`invalid TLS mode. accepts: ["", "None", "STARTTLS", "SMTPS"]`) |
There was a problem hiding this comment.
This introduces a new stdlib errors import solely for this line while the rest of the function uses xerrors. To keep error construction/wrapping consistent and avoid an extra import in this file, consider using xerrors.New(...) (or a fmt.Errorf(...) without wrapping) here instead.
reporter/loginauth_test.go
Outdated
|
|
||
| func TestLoginAuthNext(t *testing.T) { | ||
| t.Parallel() | ||
| auth := &loginAuth{username: "user@example.com", password: "s3cret"} |
There was a problem hiding this comment.
auth is shared across subtests. While it’s currently safe because the subtests aren’t parallelized, this creates an easy footgun if someone later adds t.Parallel() inside t.Run. Creating a fresh loginAuth inside each subtest (or capturing a per-test copy) makes the tests more robust against future parallelization.
There was a problem hiding this comment.
Fixed in fd3f0dd. Restructured TestLoginAuthStart into subtests, each creating its own auth instance. Also added test cases for the new TLS enforcement logic (non-TLS with LOGIN advertised succeeds, non-TLS without LOGIN advertised fails).
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got, err := auth.Next([]byte(tt.fromServer), tt.more) | ||
| if err != nil { |
There was a problem hiding this comment.
auth is shared across subtests. While it’s currently safe because the subtests aren’t parallelized, this creates an easy footgun if someone later adds t.Parallel() inside t.Run. Creating a fresh loginAuth inside each subtest (or capturing a per-test copy) makes the tests more robust against future parallelization.
There was a problem hiding this comment.
Fixed in fd3f0dd. Moved auth creation inside each subtest body and added t.Parallel() to subtests, so each test case is fully independent.
- Handle net.SplitHostPort error in dialTLS instead of ignoring it - Add TLS enforcement to loginAuth.Start - Replace errors.New with xerrors.New for import consistency - Scope auth per-subtest in loginauth_test.go for t.Parallel safety Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| advertised := false | ||
| for _, mech := range server.Auth { | ||
| if mech == "LOGIN" { | ||
| advertised = true | ||
| break | ||
| } | ||
| } | ||
| if !advertised { | ||
| return "", nil, xerrors.New("unencrypted connection: LOGIN auth requires TLS or explicit server advertisement") |
There was a problem hiding this comment.
loginAuth.Start allows LOGIN auth on non-TLS connections as long as the server advertises "LOGIN". Advertising an auth mechanism doesn’t imply transport security, so this can still send credentials in cleartext (e.g., TLSMode "None" or auto-mode when STARTTLS isn’t available). Consider enforcing the same safety gate as smtp.PlainAuth (require server.TLS unless the server is localhost), or otherwise require an explicit user opt-in before permitting LOGIN over plaintext.
| advertised := false | |
| for _, mech := range server.Auth { | |
| if mech == "LOGIN" { | |
| advertised = true | |
| break | |
| } | |
| } | |
| if !advertised { | |
| return "", nil, xerrors.New("unencrypted connection: LOGIN auth requires TLS or explicit server advertisement") | |
| host, _, err := net.SplitHostPort(server.Name) | |
| if err != nil { | |
| host = server.Name | |
| } | |
| if host != "localhost" && host != "127.0.0.1" && host != "::1" { | |
| return "", nil, xerrors.New("unencrypted connection: LOGIN auth requires TLS when not connecting to localhost") |
There was a problem hiding this comment.
Thank you for the suggestion. The current loginAuth.Start already checks server.TLS and only falls through to non-TLS if the server explicitly advertises LOGIN. I have also added strings.EqualFold for case-insensitive mechanism matching (see comment 3 fix).
Regarding making it even stricter (fully blocking non-TLS like smtp.PlainAuth): this PR is focused on removing the go-smtp dependency with behavioral parity. The original go-smtp code did not enforce TLS checks on LOGIN auth. Changing security policy would be a separate concern for a follow-up PR.
That said, I have applied the same "TLS or explicit advertisement" guard pattern consistently to both plainAuth and loginAuth, which is a reasonable middle ground. Fixed in 4bfa654.
reporter/email.go
Outdated
| defer c.Close() | ||
|
|
||
| if ok, param := c.Extension("AUTH"); ok { | ||
| authList := strings.Split(param, " ") |
There was a problem hiding this comment.
Parsing the server’s AUTH mechanisms with strings.Split(param, " ") can yield empty entries if the server returns multiple spaces or tabs. Using strings.Fields(param) avoids that and is more robust to variations in EHLO formatting.
| authList := strings.Split(param, " ") | |
| authList := strings.Fields(param) |
There was a problem hiding this comment.
Good catch. Fixed: replaced strings.Split(param, " ") with strings.Fields(param) which handles multiple spaces and leading/trailing whitespace correctly. Fixed in 4bfa654.
| func (e *emailSender) newAuth(authList []string) smtp.Auth { | ||
| for _, v := range authList { | ||
| switch v { | ||
| case "PLAIN": | ||
| auth := sasl.NewPlainClient("", e.conf.User, e.conf.Password) | ||
| return auth | ||
| return smtp.PlainAuth("", e.conf.User, e.conf.Password, e.conf.SMTPAddr) | ||
| case "LOGIN": | ||
| auth := sasl.NewLoginClient(e.conf.User, e.conf.Password) | ||
| return auth | ||
| return &loginAuth{username: e.conf.User, password: e.conf.Password} | ||
| } |
There was a problem hiding this comment.
newAuth matches auth mechanism names case-sensitively ("PLAIN"/"LOGIN"), but SMTP extension tokens are case-insensitive and some servers may advertise these as e.g. "login". Consider normalizing (e.g., strings.ToUpper(v) or strings.EqualFold) before switching so auth negotiation doesn’t fail unexpectedly.
There was a problem hiding this comment.
Valid point - RFC 4954 specifies that SASL mechanism names are case-insensitive. Fixed: newAuth now uses strings.ToUpper(v) for the switch, and both loginAuth.Start and the new plainAuth.Start use strings.EqualFold when scanning server.Auth. Fixed in 4bfa654.
| case "PLAIN": | ||
| auth := sasl.NewPlainClient("", e.conf.User, e.conf.Password) | ||
| return auth | ||
| return smtp.PlainAuth("", e.conf.User, e.conf.Password, e.conf.SMTPAddr) | ||
| case "LOGIN": |
There was a problem hiding this comment.
Using smtp.PlainAuth here changes behavior for TLSMode "None" (or any plaintext connection): stdlib PlainAuth refuses to send credentials unless server.TLS is true (except localhost), so AUTH PLAIN may now fail even when users intentionally configured plaintext. If preserving the previous behavior is required, consider either (1) skipping auth on plaintext, (2) allowing plaintext auth only when TLSMode == "None" via a dedicated opt-in path, or (3) implementing an explicit insecure plain auth implementation for that mode.
There was a problem hiding this comment.
Excellent catch. You are right that stdlib smtp.PlainAuth refuses to send credentials on non-TLS connections (except localhost), which would break users who intentionally configured TLSMode: "None".
Fixed: replaced smtp.PlainAuth with a custom plainAuth implementation that mirrors the same guard pattern as loginAuth - it allows PLAIN auth on non-TLS connections if the server explicitly advertises the PLAIN mechanism. This preserves behavioral parity with the original go-smtp library that this PR replaces.
The custom plainAuth still constructs the SASL PLAIN response correctly (\x00username\x00password) and warns on non-TLS when the mechanism is not explicitly advertised. Fixed in 4bfa654.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| auth := &loginAuth{username: "user@example.com", password: "s3cret"} | ||
| got, err := auth.Next([]byte(tt.fromServer), tt.more) |
There was a problem hiding this comment.
In TestLoginAuthNext, the loop variable tt is captured by the subtest closure while each subtest calls t.Parallel(). In Go this can cause subtests to read the wrong tt values (flaky/incorrect tests). Rebind tt := tt (or pass tt as an argument) inside the loop before t.Run so each parallel subtest gets its own copy.
There was a problem hiding this comment.
Go 1.22+ (this project uses Go 1.26) fixed the loop variable capture issue, so tt := tt is no longer needed. No change made.
| type plainAuth struct { | ||
| identity, username, password, host string | ||
| } |
There was a problem hiding this comment.
plainAuth includes a host field but it’s never read. Either remove it to reduce confusion, or use it for an explicit server name check (e.g., compare against server.Name/configured host) so the field has a purpose.
There was a problem hiding this comment.
Good catch. Removed the unused host field from plainAuth struct and the corresponding argument in newAuth. Fixed in 7898f1e.
| func (a *plainAuth) Start(server *smtp.ServerInfo) (string, []byte, error) { | ||
| if !server.TLS { | ||
| advertised := false | ||
| for _, mech := range server.Auth { | ||
| if strings.EqualFold(mech, "PLAIN") { | ||
| advertised = true | ||
| break | ||
| } | ||
| } | ||
| if !advertised { | ||
| return "", nil, xerrors.New("unencrypted connection: PLAIN auth requires TLS or explicit server advertisement") | ||
| } | ||
| } | ||
| resp := []byte(a.identity + "\x00" + a.username + "\x00" + a.password) | ||
| return "PLAIN", resp, nil | ||
| } | ||
|
|
||
| func (a *plainAuth) Next(_ []byte, more bool) ([]byte, error) { | ||
| if more { | ||
| return nil, xerrors.New("unexpected server challenge") | ||
| } | ||
| return nil, nil |
There was a problem hiding this comment.
The new plainAuth behavior isn’t covered by tests (only loginAuth is). Since this auth implementation is security- and interoperability-sensitive, add unit tests for plainAuth.Start (TLS vs non-TLS + advertised mechanisms) and plainAuth.Next (unexpected challenge when more==true).
There was a problem hiding this comment.
Added TestPlainAuthStart and TestPlainAuthNext covering TLS/non-TLS paths, identity inclusion in response, and challenge handling. Fixed in 7898f1e.
reporter/email.go
Outdated
| case strings.Contains(prompt, "password"): | ||
| return []byte(a.password), nil | ||
| default: | ||
| return nil, fmt.Errorf("unexpected server challenge: %q", fromServer) |
There was a problem hiding this comment.
loginAuth.Next returns errors via fmt.Errorf while the rest of this file consistently uses xerrors.New/xerrors.Errorf. For consistency (and to keep error formatting/wrapping uniform), consider switching this to xerrors.Errorf.
| return nil, fmt.Errorf("unexpected server challenge: %q", fromServer) | |
| return nil, xerrors.Errorf("unexpected server challenge: %q", fromServer) |
There was a problem hiding this comment.
Switched loginAuth.Next from fmt.Errorf to xerrors.Errorf for consistency with the rest of the file. Fixed in 7898f1e.
- Remove unused host field from plainAuth struct - Switch loginAuth.Next error from fmt.Errorf to xerrors.Errorf - Add unit tests for plainAuth.Start and plainAuth.Next Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Why (motivation for removing this dependency)
emersion/go-smtpandemersion/go-saslare SMTP client libraries used for sending vulnerability report emailsreporter/email.gonet/smtpprovides equivalent SMTP client functionality (Dial, StartTLS, Auth, Mail, Rcpt, Data, Quit)net/smtphas built-inPlainAuth; only LOGIN auth needs a self-implementation (~25 lines)What (replacement details)
smtp.DialTLS()withtls.Dial("tcp", ...) + smtp.NewClient()for implicit TLS (port 465 / SMTPS mode)smtp.DialStartTLS()withsmtp.Dial() + c.StartTLS()for STARTTLS upgradesasl.NewPlainClient()withsmtp.PlainAuth()sasl.NewLoginClient()with self-implementedloginAuthstruct implementingsmtp.Authinterfacenilparams fromc.Mail()andc.Rcpt()calls (go-smtp accepted options, net/smtp doesn't)Changed files
reporter/email.gologinAuthtype, refactoredsendMail()withdialTLS()/dialStartTLS()helpers, replacednewSaslClient()withnewAuth()reporter/loginauth_test.gogo.mod/go.sumemersion/go-smtpandemersion/go-saslSafety (why this is safe)
net/smtpis the official Go stdlib SMTP client, used widely in productionTest plan
TestLoginAuthStart— verifies "LOGIN" mechanism returnedTestLoginAuthNext— 5 sub-tests: Username/Password prompts (mixed case), andmore=falsereturns nilTestLoginAuthNextUnexpectedChallenge— verifies error on unknown server challengego build ./cmd/...— passgo test ./...— all passGOEXPERIMENT=jsonv2 golangci-lint run ./...— 0 issuesReview hint (how to review efficiently)
reporter/email.golines 21-43 —loginAuthimplementation (the core self-impl, ~25 lines)reporter/email.golines 125-147 —dialTLS()anddialStartTLS()helpers (the TLS connection patterns)reporter/email.golines 149-195 —sendMail()rewrite (compare TLS mode switch with the original)reporter/loginauth_test.go— tests for the self-implementationgo.mod— confirm go-smtp and go-sasl removed🤖 Generated with Claude Code