feat: support STARTTLS on plaintext listeners#8
Merged
Conversation
Listeners with TLS config now take a mode field: "implicit" (default, unchanged behavior) wraps the connection in TLS immediately, while "starttls" accepts plaintext and upgrades in-session when the client issues STARTTLS (RFC 3207), as used on submission port 587. The previous STARTTLS path was dead code and broken in three ways: the parser required a CRLF the read loop had already stripped, main.rs never provided the session a TLS acceptor, and the upgrade duplicated the raw fd (unsafe from_raw_fd), creating two owners of the socket. - Add smtp::MaybeTlsStream, which upgrades plain->TLS in place via mem::replace; no unsafe, no recursive session handling. The acceptor travels with the stream, so each listener decides independently whether to offer STARTTLS. - Advertise 250-STARTTLS per stream capability; reject with 502 when unavailable or already on TLS, and 503 mid-transaction. - Discard bytes pipelined after the STARTTLS command before the handshake to prevent plaintext command injection, and fully reset the session afterwards (client must EHLO and authenticate again). - Bound the TLS handshake with cmd_timeout on both the STARTTLS upgrade and the implicit-TLS accept, so silent clients cannot hold connection permits indefinitely. - Fix parse of STARTTLS on CRLF-stripped lines; parameters still rejected. - Send a single 221 on QUIT: handle_client no longer writes a second Bye on clean termination (previously caused broken-pipe errors for fast-closing clients, and a stray 221 after timeout's 421). - Remove the now-unused SmtpServer::with_tls and the smtp crate's rustls/rustls-pemfile dependencies; update example configs and docs. Verified with cargo test (153 tests) and a live 10-case end-to-end run covering plain/implicit/STARTTLS listeners, injection, and timeouts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds STARTTLS support (RFC 3207). Listeners with TLS config now take a
modefield:implicit(default): TLS handshake before any SMTP traffic — existing behavior for port 465.starttls: connection starts in plaintext, server advertises250-STARTTLS, and the session upgrades to TLS when the client issuesSTARTTLS— for submission on port 587.What was broken
The STARTTLS path in the
smtpcrate was dead code (seePRODUCTION_HARDENING.mditem 8, now marked addressed):"STARTTLS\r\n", but the read loop strips CRLF before parsing, so real clients always got500 Syntax error.main.rsnever gave the session a TLS acceptor, so STARTTLS was never advertised.unsafe { from_raw_fd }, creating two owners of the same socket (double-close).Implementation
smtp::MaybeTlsStreamenum upgrades plain → TLS in place viamem::replace— nounsafe, no recursive session handling. The acceptor travels with the stream, so each listener independently decides whether to offer STARTTLS (different certs per listener keep working).502when unavailable or already on TLS,503mid-transaction.cmd_timeouton both the STARTTLS upgrade and the implicit-TLS accept, so clients that go silent mid-handshake can't holdmax_connectionspermits indefinitely.221(previouslyhandle_clientwrote a second Bye on clean exit, causing broken-pipe error logs for fast-closing clients).SmtpServer::with_tlsand the smtp crate'srustls/rustls-pemfiledeps; example configs and docs updated.Testing
cargo test: 153 tests pass; parser tests updated for the corrected STARTTLS contract. Clippy clean on touched code.cmd_timeout = "3s"), 10 cases, all passing:502on plain listeners and after upgrade;503mid-transactionSTARTTLSis provably discardedSTARTTLS(and on the implicit-TLS port) are dropped at the timeout221on QUIT, zero errors in server logsThe e2e checks live in a session-local script; porting them into
smtp/tests/as Rust integration tests is a possible follow-up.