-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
deps: remove go-smtp and go-sasl in favor of net/smtp #2467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e3cef10
69fe1bc
c211b67
63f5fd7
fd3f0dd
4bfa654
7898f1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,17 +5,83 @@ import ( | |||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||
| "net" | ||||||||||||||||||||||||||||||||
| "net/mail" | ||||||||||||||||||||||||||||||||
| "net/smtp" | ||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| sasl "github.com/emersion/go-sasl" | ||||||||||||||||||||||||||||||||
| smtp "github.com/emersion/go-smtp" | ||||||||||||||||||||||||||||||||
| "golang.org/x/xerrors" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| "github.com/future-architect/vuls/config" | ||||||||||||||||||||||||||||||||
| "github.com/future-architect/vuls/models" | ||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // plainAuth implements smtp.Auth for the PLAIN mechanism without | ||||||||||||||||||||||||||||||||
| // stdlib's TLS enforcement, preserving behavioral parity with the | ||||||||||||||||||||||||||||||||
| // previously used go-smtp library for TLSMode "None" configurations. | ||||||||||||||||||||||||||||||||
| type plainAuth struct { | ||||||||||||||||||||||||||||||||
| identity, username, password string | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||
|
Comment on lines
+25
to
+46
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // loginAuth implements smtp.Auth for the LOGIN mechanism. | ||||||||||||||||||||||||||||||||
| type loginAuth struct { | ||||||||||||||||||||||||||||||||
| username, password string | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| func (a *loginAuth) Start(server *smtp.ServerInfo) (string, []byte, error) { | ||||||||||||||||||||||||||||||||
| if !server.TLS { | ||||||||||||||||||||||||||||||||
| advertised := false | ||||||||||||||||||||||||||||||||
| for _, mech := range server.Auth { | ||||||||||||||||||||||||||||||||
| if strings.EqualFold(mech, "LOGIN") { | ||||||||||||||||||||||||||||||||
| advertised = true | ||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if !advertised { | ||||||||||||||||||||||||||||||||
| return "", nil, xerrors.New("unencrypted connection: LOGIN auth requires TLS or explicit server advertisement") | ||||||||||||||||||||||||||||||||
|
Comment on lines
+56
to
+64
|
||||||||||||||||||||||||||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plainAuthincludes ahostfield but it’s never read. Either remove it to reduce confusion, or use it for an explicit server name check (e.g., compare againstserver.Name/configured host) so the field has a purpose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Removed the unused
hostfield fromplainAuthstruct and the corresponding argument innewAuth. Fixed in 7898f1e.