fix: support AUTH LOGIN for SMTP servers like Microsoft 365

[wangty371]

Summary

• Add custom LoginAuth implementation of smtp.Auth for the LOGIN mechanism
• Auto-detect auth mechanism from server EHLO response: prefer PLAIN, fall back to LOGIN
• Fixes 504 5.7.4 Unrecognized authentication type when using Microsoft Exchange Online (smtp.office365.com:587)

Closes #132

Root Cause

Go smtp.PlainAuth only supports AUTH PLAIN. Microsoft Exchange Online advertises AUTH LOGIN XOAUTH2 (no PLAIN), so authentication fails.

Changes

Single file: modules/base/common/service_email.go

1. loginAuth struct — implements smtp.Auth interface for the LOGIN mechanism
2. chooseAuth method — inspects server EHLO AUTH advertisement and picks PLAIN or LOGIN
3. dispatchSMTP — calls chooseAuth after STARTTLS instead of hardcoding smtp.PlainAuth

Test plan

• Verify go build and existing tests pass
• Test with smtp.office365.com:587 (AUTH LOGIN only) — should succeed
• Test with smtp.163.com:465 (AUTH PLAIN) — should continue to work
• Test with smtp.gmail.com:587 (supports both PLAIN and LOGIN) — should pick PLAIN

[Jerry-Xin]

This PR is in scope for octo-server, but the custom LOGIN auth implementation drops the transport-safety checks that smtp.PlainAuth previously enforced.

🔴 Blocking

• 🔴 Critical: loginAuth.Start sends credentials without verifying TLS or host identity. In modules/base/common/service_email.go, Start always returns "LOGIN" and allows net/smtp to continue. Unlike smtp.PlainAuth, it does not reject non-TLS connections and does not verify server.Name against the expected host. Because dispatchSMTP only upgrades with STARTTLS when the server advertises it, a server or downgrade attack that advertises AUTH LOGIN but no STARTTLS would now receive the username/password over plaintext. This is a security regression from the previous PlainAuth behavior. Fix by storing the expected host in loginAuth and mirroring PlainAuth’s checks: require server.TLS or localhost, and reject mismatched server.Name.

💬 Non-blocking

• 🟡 Warning: chooseAuth uses strings.Contains over the raw AUTH extension. This can mis-detect mechanisms if a token merely contains another token. Prefer strings.Fields plus exact case-insensitive token matching for PLAIN and LOGIN.

• 🔵 Suggestion: loginAuth.Next only accepts exact "Username:" and "Password:" challenges. AUTH LOGIN is not standardized and some servers vary casing or whitespace. Consider strings.TrimSpace plus strings.EqualFold to make this more robust, while still rejecting unknown challenges.

• 🟡 Warning: There are no tests covering auth mechanism selection or the new LOGIN flow. At minimum, add unit tests for PLAIN preference, LOGIN fallback, no-AUTH legacy fallback, and loginAuth.Start rejecting non-TLS non-localhost connections.

✅ Highlights

• The feature is well targeted to the reported Microsoft 365 issue.
• Choosing auth after STARTTLS is the right place to inspect the post-upgrade EHLO capabilities.

[lml2468]

[APPROVE] Clean fix. strings is already imported on main; the new code will compile. Note: CI (Build/Lint/Vet/Test) has not run — this is a fork PR and workflows require maintainer approval before they execute.

Verified

chooseAuth — EHLO-based auto-detection
Reads the server's advertised AUTH mechanisms via client.Extension("AUTH"). Preference order: PLAIN (widely supported) → LOGIN (Exchange Online fallback) → PLAIN default when no extension advertised (preserves legacy behavior for servers that don't advertise AUTH in EHLO). ✅

loginAuth — standard AUTH LOGIN implementation
Start returns the mechanism name "LOGIN" with no initial response (correct per AUTH LOGIN spec — client waits for server challenge). Next handles the two challenge → response steps: "Username:" → username bytes, "Password:" → password bytes. Any unexpected challenge short-circuits with a non-nil error (fail-closed). more=false returns nil, nil (authentication exchange complete, no trailing response). ✅

No credential logging or storage changes — loginAuth stores credentials the same way smtp.PlainAuth does (struct fields). ✅

Notes (non-blocking)

• Challenge matching is exact: "Username:" and "Password:" (capital U/P). Microsoft Exchange Online uses exactly these strings. Servers with lowercase or other variants would hit the default error branch — currently acceptable since the stated target is Exchange Online. If broader compat is needed later, a strings.EqualFold could be added.
• CI (Build/Lint/Vet/Test) was not triggered — please approve the fork workflow before merging.

[yujiawei]

Code Review — PR #133 (octo-server)

Summary

The intent (AUTH LOGIN fallback for Microsoft Exchange Online) is correct and the chosen approach — server-advertisement-driven mechanism selection plus a tiny smtp.Auth LOGIN implementation — is the right shape. Build passes; behavior for existing PLAIN servers is unchanged.

However, the new loginAuth.Start drops a security guard that the stdlib smtp.PlainAuth enforces, opening a credential-leak path under STARTTLS-stripping conditions. That regression should be fixed before merging.

Verdict

CHANGES_REQUESTED — one P1 security regression, plus two robustness suggestions (P2) and a missing-tests note (P2).

P1 — loginAuth.Start silently sends credentials over plaintext

modules/base/common/service_email.go:375-377

func (a *loginAuth) Start(server *smtp.ServerInfo) (string, []byte, error) {
    return "LOGIN", nil, nil
}

Stdlib smtp.PlainAuth.Start refuses to send credentials unless server.TLS is true (or host is localhost):

PlainAuth will only send the credentials if the connection is using TLS or is connected to localhost. Otherwise authentication will fail with an error, without sending the credentials.

The new LoginAuth has no such check, so the combined flow in dispatchSMTP is weaker than the pre-PR behavior:

1. dispatchSMTP (service_email.go:297-303) only runs STARTTLS when the server advertises the extension. If the extension is absent, the connection stays plaintext.
2. chooseAuth (service_email.go:316-328) then picks LoginAuth whenever the server advertises only LOGIN.
3. loginAuth.Start returns the mechanism unconditionally, so client.Auth proceeds to write the base64'd username and password over the cleartext socket.

Concrete attack: an on-path attacker on port 587 strips STARTTLS from the EHLO response and replaces the AUTH list with AUTH LOGIN. Pre-PR, smtp.PlainAuth would have aborted with "unencrypted connection". Post-PR, the credentials are exfiltrated. Microsoft 365 itself always requires TLS, so the regression doesn't bite in the happy path, but it is a real downgrade-attack surface that we previously had defense-in-depth against — and this PR is exactly where the guard needs to be re-added.

Suggested fix (mirrors stdlib plainAuth.Start, including a localhost carve-out for tests):

func (a *loginAuth) Start(server *smtp.ServerInfo) (string, []byte, error) {
    if !server.TLS && !isLocalhost(server.Name) {
        return "", nil, errors.New("unencrypted connection")
    }
    return "LOGIN", nil, nil
}

func isLocalhost(name string) bool {
    return name == "localhost" || name == "127.0.0.1" || name == "::1"
}

(Optional, but worth considering separately: hardening dispatchSMTP to require STARTTLS on submission ports — i.e. error out if STARTTLS is unavailable on 587 — would close the broader downgrade hole. Out of scope for this PR; flagging for follow-up.)

P2 — Prompt matching uses exact equality

modules/base/common/service_email.go:383-390

switch string(fromServer) {
case "Username:":
    return []byte(a.username), nil
case "Password:":
    return []byte(a.password), nil
default:
    return nil, fmt.Errorf("unexpected server challenge: %s", fromServer)
}

Username: / Password: are what Microsoft 365 and most modern servers send (the stated target), so this works today. But the AUTH LOGIN prompts are server-defined — older Exchange variants, some appliances, and a few corporate gateways send User Name, username, or Password\x00. A small relaxation makes this resilient without giving anything up:

prompt := strings.ToLower(strings.TrimRight(string(fromServer), ": \x00"))
switch prompt {
case "username", "user name", "user":
    return []byte(a.username), nil
case "password", "pass":
    return []byte(a.password), nil
default:
    return nil, fmt.Errorf("unexpected server challenge: %q", fromServer)
}

Nit but worth doing while the code is open.

P2 — chooseAuth uses substring match on the AUTH advertisement

modules/base/common/service_email.go:317-324

strings.Contains(mechs, "PLAIN") and strings.Contains(mechs, "LOGIN") will match unintended tokens (e.g. a hypothetical XOAUTH2-PLAIN, or a server advertising LOGIN-MD5 only — Go has no such auth so we'd then attempt LOGIN and fail). Token-aware matching is one line:

tokens := strings.Fields(strings.ToUpper(authStr))
has := func(m string) bool {
    for _, t := range tokens { if t == m { return true } }
    return false
}
if has("PLAIN") { ... }
if has("LOGIN") { ... }

Low impact in practice; flagging because it's cheap.

P2 — No unit tests for the new auth path

modules/base/common/service_email_envelope_test.go already covers the envelope helpers; the AUTH mechanism selection and loginAuth state machine are completely untested. Both are pure functions with no network dependency and trivially testable:

• chooseAuth with a fake *smtp.Client (or refactor to take an interface) for cases: AUTH not advertised, AUTH PLAIN LOGIN, AUTH LOGIN XOAUTH2 (the actual M365 string), AUTH XOAUTH2 only.
• loginAuth.Start with server.TLS=false (must error after fix) and server.TLS=true (must succeed).
• loginAuth.Next for Username:, Password:, unknown challenge (must error), and more=false (must return nil).

Without these, the next change in this area is one regression away from a silent prod break — same shape of bug as the "missing QUIT" regression already memorialized in the runSMTPTransaction comment.

Things that look good

• Reusing chooseAuth after STARTTLS (rather than before EHLO) is correct — extensions are only authoritative after the encrypted EHLO.
• Default fallback (return smtp.PlainAuth(...) when AUTH isn't advertised) preserves legacy behavior for servers that don't advertise extensions; matches the comment.
• loginAuth.Next correctly returns (nil, nil) when more is false, so a server that finishes the exchange with a non-prompt 235 line won't trip the default error case.
• Single-file change, minimal blast radius, clear PR description with linked issue.

Human-review asks (security-sensitive)

Flagging for the human reviewer:

1. Confirm the P1 fix above lands before merge — this changes the credential-exposure posture, not just functionality.
2. Worth deciding policy: should dispatchSMTP hard-require STARTTLS on submission ports (587) instead of silently proceeding plaintext when the extension isn't advertised? Today's PR doesn't change that posture, but the new auth path makes the question more pointed.
3. Manual smoke test against smtp.office365.com:587 with a real M365 mailbox is still the only end-to-end signal that the LOGIN exchange actually works — recommend running it before merge.

[yujiawei]

Code Review — PR #133 (octo-server)

Summary

Adds a custom LoginAuth (SMTP AUTH LOGIN SASL mechanism) and an EHLO-driven
chooseAuth selector to dispatchSMTP, so the email service can authenticate
against servers (notably Microsoft Exchange Online / smtp.office365.com:587)
that advertise AUTH LOGIN but not AUTH PLAIN. PLAIN remains preferred when
offered; LOGIN is the explicit fallback.

The implementation mirrors the Go stdlib PlainAuth security model (TLS-only,
host-name check, localhost carve-out for tests), and the follow-up commit
17c5728 addresses prior review feedback (TLS guard added, robust prompt
matching, unit tests).

I checked out the head SHA, ran go build ./... (clean) and the new
TestLoginAuth_* / TestIsLocalhost tests (all pass).

Verdict

No P0/P1 blockers. The security-sensitive surface (credential transmission)
is handled correctly. Approving with optional polish notes below.

1. Verification

Item	Status	Evidence
Build clean at head	✅	go build ./... from 17c5728, no output
New unit tests pass	✅	go test ./modules/base/common -run 'TestLoginAuth|TestIsLocalhost' -count=1 → ok
go vet clean for touched package	✅	go vet ./modules/base/common/... clean
TLS-only credential transmission for LOGIN	✅	service_email.go:386 — Start rejects when !server.TLS && !isLocalhost(...)
Host-name pinning prevents credential leak after redirect	✅	service_email.go:389 — server.Name != a.host → error
EHLO inspection happens after STARTTLS	✅	chooseAuth is called at service_email.go:308, i.e. after the client.StartTLS(...) block at service_email.go:298-302. This is required for Microsoft 365 which only advertises AUTH LOGIN XOAUTH2 post-TLS.
Mechanism token matching is case- and whitespace-tolerant	✅	service_email.go:318 strings.Fields(strings.ToUpper(authStr)) plus tokenized exact match in hasMech
PLAIN preferred over LOGIN when both offered	✅	service_email.go:327-332 checks PLAIN first
Default fallback when AUTH not advertised	✅	service_email.go:335 — preserves prior behavior; will surface a clear SMTP认证失败 error if the server truly rejects PLAIN
Port 465 (implicit TLS) still works with LOGIN	✅	Go's smtp.NewClient sets c.tls = true when passed a *tls.Conn (it does the _, c.tls = conn.(*tls.Conn) assertion internally), so server.TLS is true in the SASL Start call even on the 465 path that never invokes client.StartTLS. The TLS guard does not regress 465 connectivity.
No CRLF / header injection regression	✅	Username / password are sent as SASL payloads inside Client.Auth, not as message headers; existing sanitizeHeader path for envelope fields is untouched

2. Findings

P2 — chooseAuth doesn't need a receiver (nit)

chooseAuth at modules/base/common/service_email.go:316 uses no field of
*EmailService. Making it a package-level function (or a method on
*smtp.Client's caller helper) would make it trivially unit-testable without
spinning up an EmailService. Non-blocking — current placement reads fine.

P2 — Consider trimming = from token (defensive, optional)

Real-world EHLO responses sometimes use AUTH=LOGIN PLAIN (legacy Outlook /
Exchange variants pre-RFC alignment) in addition to the canonical
AUTH LOGIN PLAIN. Go's Client.Extension returns the raw params after the
extension name, but I checked net/smtp and it already strips the leading
= for the AUTH extension specifically (extAuth/auth normalization in
hello()), so the current strings.Fields(strings.ToUpper(authStr)) is
sufficient. Calling this out as a "checked, not a problem" rather than a
fix request — no change needed.

P2 — chooseAuth has no direct unit test (nit)

The loginAuth SASL state machine has solid coverage in
service_email_login_auth_test.go, but chooseAuth's selection logic
(PLAIN preferred, LOGIN fallback, default-on-missing) is only exercised
end-to-end. Mocking *smtp.Client is genuinely awkward in Go, so I'm not
asking for it — but if you ever refactor to a pure helper that takes the
EHLO AUTH string directly (e.g. pickAuthFromEHLO(authStr string) string),
the unit test becomes a one-liner. Worth holding in mind for the next pass.

P3 — Comment language drift (nit)

The file is predominantly Chinese inline comments; new code is documented in
English. Both are clear; just calling out the stylistic mix.

3. Security review (PR labelled security_sensitive)

• Credential confidentiality: LOGIN, like PLAIN, is base64-only — no
  improvement over PLAIN cryptographically, but Microsoft 365 mandates it.
  The TLS guard at service_email.go:386 ensures credentials are never sent
  over an unencrypted channel except to localhost (test carve-out matching
  Go stdlib PlainAuth).
• MITM / DNS-rebind hardening: server.Name != a.host check at
  service_email.go:389 mirrors stdlib PlainAuth. Combined with TLS
  ServerName pinning in dispatchSMTP (tls.Config{ServerName: host} at
  service_email.go:273 and :299), the auth target is bound to the same
  hostname used for cert validation.
• No new logging of credentials: verified — loginAuth.Next only returns
  raw bytes back to the SMTP client; no s.Log / zap call exposes
  password material.
• Localhost carve-out scope: isLocalhost matches localhost, 127.0.0.1,
  ::1. Tight enough — does not match 127.0.0.2 or other loopback range
  IPs, but that's consistent with stdlib behavior and acceptable for the
  test-only use case.
• Default-to-PLAIN fallback when AUTH unannounced: preserves prior
  behavior; will not silently downgrade an in-flight LOGIN session. If a
  server suddenly stops advertising AUTH, the user will see
  SMTP认证失败: ... — visible failure, not silent compromise.

I have no blocking security concerns. The human-review requirement
(needs-human-review label) was applied by the dispatcher because of
keyword routing; the substance of the change is a narrow, well-scoped
credential-path fix.

4. Suggestions (non-blocking)

1. If you do another pass, consider extracting pickAuthFromEHLO(authStr) string so the selection logic is unit-testable without a real SMTP
   client.
2. Consider promoting LoginAuth to a small helper package (e.g.
   internal/smtpauth) the day a second caller needs it. Today the only
   user is EmailService, so it belongs where it is.

5. Additional findings outside the diff

None. I scanned the surrounding dispatchSMTP / runSMTPTransaction paths
for related credential or TLS handling regressions and found nothing the PR
introduced or missed.

[Jerry-Xin]

Summary: The PR is relevant to octo-server’s SMTP email delivery path and the implementation correctly adds LOGIN fallback without changing the existing PLAIN-first behavior.

💬 Non-blocking

• 🟡 Warning: modules/base/common/service_email_login_auth_test.go:10 tests loginAuth directly, but there is no protocol-level test proving chooseAuth selects LOGIN from an EHLO AUTH LOGIN XOAUTH2 response and works through smtp.Client.Auth with base64 challenges. A small fake SMTP server test would better pin the actual Microsoft 365 regression path.
• 🔵 Suggestion: modules/base/common/service_email.go:399 only accepts decoded prompts beginning with username or password. That should work for common LOGIN servers, but a step-based fallback could make this more tolerant of servers that send slightly different challenge labels.

✅ Highlights

• ✅ modules/base/common/service_email.go:297 chooses auth after STARTTLS, so the post-TLS EHLO capabilities are used.
• ✅ modules/base/common/service_email.go:385 mirrors smtp.PlainAuth’s TLS/host checks, avoiding credential exposure on plaintext connections.
• ✅ Focused tests pass: go test ./modules/base/common.
• ⚠️ go test ./modules/common could not complete in this environment because it requires MySQL at 127.0.0.1:3306; this is not attributable to this PR.

[wangty371]

All three reviewers have approved and review feedback has been addressed. Ready to merge when convenient, thanks!