Skip to content

fix(mailer): support SMTP AUTH LOGIN when PLAIN is unavailable - #100

Merged
shockalotti merged 6 commits into
Calnode:mainfrom
KitKat31337:feat/smtp-auth-login
Sep 26, 2026
Merged

shockalotti merged 6 commits into
Calnode:mainfrom
KitKat31337:feat/smtp-auth-login

Conversation

@KitKat31337

Copy link
Copy Markdown
Contributor

Problem

Calnode currently uses smtp.PlainAuth for SMTP credentials. During a connection to smtp.office365.com:587, Microsoft advertised AUTH LOGIN XOAUTH2 after STARTTLS, with no PLAIN option. Calnode therefore could not use the advertised password authentication method.

Change

  • Select AUTH PLAIN when the server offers it.
  • Fall back to AUTH LOGIN when PLAIN is unavailable.
  • Require TLS before sending credentials with LOGIN.
  • Return a clear error when the server offers neither supported method.
  • Preserve the existing behavior of omitting the server’s full authentication response from Calnode logs.

Testing

Added tests for the Office 365 capability list, PLAIN preference, unsupported methods, LOGIN challenge responses, and the TLS requirement.

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA. ✅
Posted by the CLA Assistant Lite bot.

@KitKat31337

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Sep 25, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No blocking issues — the mechanism selection and the AUTH LOGIN exchange are correct. One coverage suggestion and a small test nit below.

Reviewed changes

  • Advertised-mechanism selection — (*SMTP).authForMethods reads the server's advertised AUTH list from c.Extension("AUTH") and returns PLAIN when offered, else LOGIN, else a clear unsupported-mechanism error, replacing the hardcoded smtp.PlainAuth.
  • AUTH LOGIN implementation — a new loginAuth smtp.Auth answers the two challenges with username then password and hard-requires TLS via Start.
  • Tests + docs — unit tests cover the selection preference and the loginAuth state machine; docs/ARCHITECTURE.md documents the rule.

I verified the load-bearing assumptions against Go's net/smtp: StartTLS re-issues EHLO after the handshake (so the AUTH advertisement is post-TLS), NewClient sets tls=true for implicit-TLS connections, and Client.Auth drives a no-initial-response mechanism as AUTH LOGIN → 334 → username → 334 → password → 235 → Next(..., false). go build, go vet and go test ./internal/mailer/ all pass.

ℹ️ The LOGIN exchange is only tested in isolation, never through net/smtp

TestLoginAuthRequiresTLSAndAnswersChallenges calls loginAuth directly, so it pins the state machine but not the contract the PR actually depends on: that net/smtp's Client.Auth sends no initial response, calls Next(more=true) exactly twice, then calls Next(..., false) on the 235. A regression in that interaction (e.g. an accidental initial response, or an extra challenge) would ship undetected.

Technical details
# Add an integration test for the AUTH LOGIN exchange

## Affected sites
- `internal/mailer/smtp_test.go:38-59` — `TestLoginAuthRequiresTLSAndAnswersChallenges` exercises `loginAuth` directly; the `Send` → `c.Extension("AUTH")` → `authForMethods` → `c.Auth` path is never run.
- `internal/mailer/smtp.go:198-209` — the selection and `c.Auth` call site.

## Required outcome
- A test using a local listener (mirroring `TestSMTP_Send_timesOutOnUnresponsiveServer`) that speaks enough SMTP to advertise `AUTH LOGIN`, issue the two 334 challenges, verify the base64 username/password, return 235, then accept `MAIL FROM`/`RCPT TO`/`DATA`, asserting `Send` returns nil.

## Open questions for the human
- Whether a fuller fake SMTP server is worth the maintenance cost versus leaning on the stdlib contract.

ℹ️ Nitpicks

  • internal/mailer/smtp_test.go:30 — the PLAIN case only asserts the result is not *loginAuth, so an authForMethods that returned nil, nil for a PLAIN-capable server would pass. Assert positively, e.g. call auth.Start(&smtp.ServerInfo{Name: s.host, TLS: true}) and check the mechanism is "PLAIN".
  • docs/ARCHITECTURE.md:648 — reads well; no CHANGELOG.md entry under [Unreleased], though the repo applies that convention inconsistently.

Pullfrog  | Fix it ➔ | View workflow run | Using opencode-go/deepseek-v4.1-flash | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Locked down the PLAIN selection assertion — the LOGIN PLAIN case in TestSMTPAuthForMethods now calls auth.Start(&smtp.ServerInfo{Name: s.host, TLS: true}) and requires mechanism "PLAIN", closing the prior hole where any non-*loginAuth result (including nil) would pass.
  • Added the SMTP AUTH LOGIN changelog entry — an [Unreleased] → ### Fixed note describing the PLAIN-then-LOGIN selection, the TLS requirement, and the unsupported-method error.

go test ./internal/mailer/ passes.

Pullfrog  | View workflow run | Using opencode-go/deepseek-v4.1-flash | 𝕏

@shockalotti
shockalotti merged commit 9f15dfe into Calnode:main Sep 26, 2026
2 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 26, 2026
@shockalotti

Copy link
Copy Markdown
Contributor

Quick follow-up now that this is on main: could you pull latest, re-run your Office 365 manual test, and confirm it's still green? CI doesn't cover a live LOGIN-only server, and you're the one with access to the real thing — your confirmation is the actual end-to-end proof here. If anything looks off, reopen or ping me and I'll dig in. Thanks again for the contribution.

@shockalotti

Copy link
Copy Markdown
Contributor

@pullfrog hi

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants