Skip to content

Fix #56#57

Closed
TheDeadCode wants to merge 1 commit into
Taiizor:developfrom
TheDeadCode:develop
Closed

Fix #56#57
TheDeadCode wants to merge 1 commit into
Taiizor:developfrom
TheDeadCode:develop

Conversation

@TheDeadCode
Copy link
Copy Markdown
Contributor

Fix for #56. Will now defer to STARTTLS is available. If not, will forcibly fail (Like previous behaviour)

…useful for server-to-server connections, when acting as an MTA.
@Taiizor
Copy link
Copy Markdown
Owner

Taiizor commented Jun 3, 2026

Thanks for digging into this, @TheDeadCode — the diagnosis is spot on, and this does resolve the immediate symptom (relaying to a plain-text server like Mailpit). I especially liked passing the cancellation token into TcpClient.ConnectAsync; I've carried that idea over.

After reviewing, I opened #58 which builds on your work (you're credited as co-author) and takes a slightly different, more complete approach. A few gaps I wanted to flag here for context:

  1. Failed-handshake "dance" on the common case. The model is "try implicit TLS → catch → reconnect plain text → STARTTLS". Because the builder maps port 587 to UseTls = true, every 587 submission delivery does an implicit handshake that is guaranteed to fail, then tears down and reconnects. Two TCP connections + one failed handshake per delivery for the most common configuration.

  2. RequireTls is still never consulted. The PR description says it will "forcibly fail" when STARTTLS is unavailable, but in practice UpgradeToStartTlsAsync silently continues in plain text if the STARTTLS reply isn't successful, and the STARTTLS block is skipped entirely when the server doesn't advertise it — so there's no way to enforce TLS.

  3. UseStartTls stays dead. STARTTLS is gated on EnableSsl == true, so a host with UseTls = false never attempts opportunistic STARTTLS even when the server advertises it.

  4. GetTargetConfiguration only matches DefaultSmartHost. DomainRouting hosts still fall through to the fabricated config (UseTls = EnableTls).

  5. Root mapping bug. RelayBuilder/SmtpServerExtensions mark port 587 as implicit TLS (UseTls = port is 465 or 587); 587 is a STARTTLS submission port. (SmartHostExample.cs already had it right: UseTls = port == 465.)

#58 makes STARTTLS a first-class path (no reconnect dance), honors RequireTls/UseStartTls, also matches DomainRouting, fixes the 465/587 mapping, and adds opportunistic-vs-required certificate handling. It's verified end-to-end against fake plain-text and STARTTLS servers across net6.0–net10.0.

Either way, thank you for the contribution and the clear repro — it made this easy to track down. 🙏

@Taiizor
Copy link
Copy Markdown
Owner

Taiizor commented Jun 3, 2026

Closing in favor of #58, which builds on this (you're credited as co-author, @TheDeadCode) with a more complete fix — first-class STARTTLS, RequireTls/UseStartTls support, DomainRouting matching, and the 465/587 port-mapping fix. Thanks again for the diagnosis and repro! 🙏

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Relay fails when RequireTls is false and EnableTls is true

2 participants