275 Refactor SASL authentication handling and remove redundant tests - #282
Conversation
The ClientState struct is a plain data container with no business logic. Its tests only verified Go zero-values and struct field assignment, which are language features, not application logic.
Adds proper multi-step SASL authentication for LOGIN and PLAIN mechanisms by tracking auth state per connection: - AUTH LOGIN without initial response: sends Username prompt - CONT with username: stores decoded username, sends Password prompt - CONT with password: authenticates via existing auth API - AUTH PLAIN without initial response: sends CONT prompt, then authenticates - AUTH OAUTHBEARER/XOAUTH2 without initial response: handles CONT prompt State is cleaned up after authentication completes or fails.
manupawickramasinghe
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements multi-step SASL authentication by introducing an authState tracker and a CONT command handler. It completes the LOGIN mechanism implementation and updates PLAIN and OAuth mechanisms to support continuation flows, with corresponding integration test updates. Feedback was provided regarding a potential DoS vulnerability where the authStates map could grow indefinitely, and a suggestion was made to send a FAIL response for malformed CONT commands to prevent client hangs.
Aravinda-HWK
assigned
manupawickramasinghe and
maneesha-xyz and unassigned
maneesha-xyz
|
@manupawickramasinghe could you please resolve the Gemini comments, then we can start reviewing this PR. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…andling - Add maxAuthStatesPerConn limit (10) to prevent unbounded auth state map growth - Add DoS check in handleAuth rejecting new attempts when limit exceeded - Fix potential panic in handleCont when accessing parts[1] with len(parts) < 2 - Guard with length check before accessing parts[1] for FAIL response
c67c64a to
07263f8
Compare
|
@Aravinda-HWK The issue have been resolved |
|
@copilot Please let me know if this needs any additions |
|
Can you also run @Aravinda-HWK shall we add this to our ci. |
Sure, we should add this. |
|
|
d4c0303 to
2b5d708
Compare
|
LGTM. |
📌 Description
Implements SASL multi-step authentication support for PLAIN and LOGIN mechanisms with per-connection state tracking. Previously, LOGIN only returned "not fully implemented" and PLAIN continuation requests never completed authentication. #275
🔍 Changes Made
authStatestruct to track multi-step auth per connection (mechanism, step, username)handleCont()— dispatches CONTINUE commands to the correct handler based on stored auth statehandleLoginCont()— full two-step LOGIN flow: Username: prompt → Password: prompt → authenticatego 1.25.0andgolang.org/x/sync v0.20.0(were downgraded by bot commits)CONTcommand into the SASL command dispatcher✅ Checklist (Email System)
🧪 Testing Instructions
go test ./internal/sasl/... -v -count=1— 35 unit testsgo test ./test/integration/sasl/... -v -count=1— 10 integration testsAUTH <id> PLAIN service=smtp resp=<base64>AUTH <id> PLAIN service=smtp→CONT <id> <base64>AUTH <id> LOGIN service=smtp→CONT <id> <base64-user>→CONT <id> <base64-pass>📷 Screenshots / Logs
Environment
fix/restore-go-versionBuild & Vet
SASL Unit Tests — 35/35 PASS
SASL Integration Tests — 10/10 PASS
Full Test Suite
Pre-existing failures (unrelated to this change):
raven/internal/db— Fails on main too (test data setup issue)raven/internal/delivery/lmtp— Times out at 300s on main too (LMTP session test hang)