Fix #56: correct relay TLS negotiation (STARTTLS + opportunistic fallback)

The relay client forced implicit TLS even when TLS was not required, so
relaying to a plain-text server (e.g. Mailpit) failed with
"Cannot determine the frame size or a corrupted frame was received".

Root causes:
- RelayService.GetTargetConfiguration discarded the configured smart host
  (DefaultSmartHost/DomainRouting) and fabricated a config with
  UseTls = EnableTls (default true), forcing implicit TLS.
- SmtpRelayClient only supported implicit TLS (SMTPS); it never issued
  STARTTLS and ignored UseStartTls/RequireTls.
- The builder helpers mapped port 587 to implicit TLS (UseTls=true)
  instead of STARTTLS.

Changes:
- SmtpRelayClient: add UseStartTls/RequireTls/ValidateServerCertificate;
  implement opportunistic STARTTLS after EHLO with graceful plain-text
  fallback when not required, and enforcement when RequireTls is set.
  Honor the cancellation token during the TLS handshake. Restrict the
  "accept any certificate" shortcut to opportunistic STARTTLS only.
- RelayService.GetTargetConfiguration: match host:port against
  DefaultSmartHost, SmartHosts and DomainRouting before fabricating an
  MX default (implicit TLS only on port 465). Wire the new client
  options. Remove the buggy EnableSsl override on the bounce path.
- RelayBuilder/SmtpServerExtensions: map port 465 to implicit TLS and
  587/others to STARTTLS.

Co-authored-by: Felix Lebel <1714641+TheDeadCode@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 3 people

src/Zetian.Relay/Builder/RelayBuilder.cs

@@ -41,8 +41,8 @@ public RelayBuilder WithSmartHost(
                 Credentials = !string.IsNullOrEmpty(username)
                     ? new NetworkCredential(username, password)
                     : null,
-                UseTls = port is 465 or 587,
-                UseStartTls = port == 587
+                UseTls = port == 465,
+                UseStartTls = port != 465
             };
             return this;
         }
@@ -194,8 +194,8 @@ public RelayBuilder AddDomainRoute(
                 Credentials = !string.IsNullOrEmpty(username)
                     ? new NetworkCredential(username, password)
                     : null,
-                UseTls = port is 465 or 587,
-                UseStartTls = port == 587
+                UseTls = port == 465,
+                UseStartTls = port != 465
             };
             return this;
         }

src/Zetian.Relay/Client/SmtpRelayClient.cs

@@ -34,7 +34,33 @@ public class SmtpRelayClient(ILogger<SmtpRelayClient>? logger = null) : ISmtpCli
 
         public string Host { get; set; } = "localhost";
         public int Port { get; set; } = 25;
+
+        /// <summary>
+        /// Gets or sets whether to use implicit TLS (SMTPS) by negotiating TLS immediately on connect.
+        /// Typically used for port 465. For STARTTLS use <see cref="UseStartTls"/> instead.
+        /// </summary>
         public bool EnableSsl { get; set; }
+
+        /// <summary>
+        /// Gets or sets whether to attempt opportunistic STARTTLS after EHLO when the server advertises it.
+        /// Ignored when <see cref="EnableSsl"/> (implicit TLS) is enabled.
+        /// </summary>
+        public bool UseStartTls { get; set; } = true;
+
+        /// <summary>
+        /// Gets or sets whether TLS is mandatory. When true, the connection fails unless TLS is
+        /// established (via implicit TLS or STARTTLS). When false, TLS is opportunistic and the
+        /// client falls back to plain text if the server does not offer it.
+        /// </summary>
+        public bool RequireTls { get; set; }
+
+        /// <summary>
+        /// Gets or sets whether to validate the server certificate when TLS is required.
+        /// For opportunistic TLS (when <see cref="RequireTls"/> is false) certificate errors are
+        /// ignored, because encryption without authentication is still preferable to plain text.
+        /// </summary>
+        public bool ValidateServerCertificate { get; set; } = true;
+
         public SslProtocols SslProtocols { get; set; } = SslProtocols.Tls12 | SslProtocols.Tls13;
         public X509Certificate2? ClientCertificate { get; set; }
         public NetworkCredential? Credentials { get; set; }
@@ -62,10 +88,11 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default)
                 using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
                 cts.CancelAfter(Timeout);
 
-                await _tcpClient.ConnectAsync(Host, Port).ConfigureAwait(false);
+                await _tcpClient.ConnectAsync(Host, Port, cts.Token).ConfigureAwait(false);
 
                 _stream = _tcpClient.GetStream();
 
+                // Implicit TLS (SMTPS, e.g. port 465): negotiate TLS before the greeting.
                 if (EnableSsl)
                 {
                     await UpgradeToSslAsync(cts.Token).ConfigureAwait(false);
@@ -84,6 +111,28 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default)
                 // Send EHLO
                 await SendEhloAsync(cts.Token).ConfigureAwait(false);
 
+                // Opportunistic / required STARTTLS upgrade (only when not already using implicit TLS).
+                if (!EnableSsl && (UseStartTls || RequireTls))
+                {
+                    bool serverSupportsStartTls = _serverCapabilities?.ContainsKey("STARTTLS") == true;
+
+                    if (serverSupportsStartTls)
+                    {
+                        await StartTlsAsync(cts.Token).ConfigureAwait(false);
+                    }
+                    else if (RequireTls)
+                    {
+                        throw new InvalidOperationException(
+                            $"TLS is required but server {Host}:{Port} does not advertise STARTTLS");
+                    }
+                    else
+                    {
+                        _logger.LogWarning(
+                            "Server {Host}:{Port} does not support STARTTLS; continuing without encryption",
+                            Host, Port);
+                    }
+                }
+
                 _logger.LogInformation("Connected to {Host}:{Port}", Host, Port);
             }
             catch (Exception ex)
@@ -386,16 +435,58 @@ private async Task SendEhloAsync(CancellationToken cancellationToken)
 
         private async Task UpgradeToSslAsync(CancellationToken cancellationToken)
         {
-            SslStream sslStream = new(_stream, false, ValidateServerCertificate);
+            SslStream sslStream = new(_stream!, false, OnCertificateValidation);
+
+            SslClientAuthenticationOptions options = new()
+            {
+                TargetHost = Host,
+                EnabledSslProtocols = SslProtocols,
+                CertificateRevocationCheckMode = X509RevocationMode.Online
+            };
 
-            await sslStream.AuthenticateAsClientAsync(
-                Host,
-                ClientCertificate != null ? [ClientCertificate] : null,
-                SslProtocols,
-                true).ConfigureAwait(false);
+            if (ClientCertificate != null)
+            {
+                options.ClientCertificates = new X509CertificateCollection { ClientCertificate };
+            }
+
+            // Pass the cancellation token so a stalled handshake honors the connection timeout.
+            await sslStream.AuthenticateAsClientAsync(options, cancellationToken).ConfigureAwait(false);
 
             _stream = sslStream;
-            _logger.LogDebug("SSL/TLS connection established");
+            _logger.LogDebug("SSL/TLS connection established with {Host}:{Port}", Host, Port);
+        }
+
+        private async Task StartTlsAsync(CancellationToken cancellationToken)
+        {
+            await SendCommandAsync("STARTTLS", cancellationToken).ConfigureAwait(false);
+            SmtpResponse response = await ReadResponseAsync(cancellationToken).ConfigureAwait(false);
+
+            // RFC 3207: a 220 reply means the server is ready to negotiate TLS.
+            if (response.Code != 220)
+            {
+                if (RequireTls)
+                {
+                    throw new InvalidOperationException(
+                        $"STARTTLS command rejected by {Host}:{Port}: {response.Code} {response.Message}");
+                }
+
+                _logger.LogWarning(
+                    "STARTTLS rejected by {Host}:{Port} ({Code} {Message}); continuing without encryption",
+                    Host, Port, response.Code, response.Message);
+                return;
+            }
+
+            await UpgradeToSslAsync(cancellationToken).ConfigureAwait(false);
+
+            // The reader/writer must be rebuilt on top of the now-encrypted stream. The old ones
+            // are intentionally not disposed (that would close the underlying socket). Per RFC 3207
+            // the server must not transmit anything after its "220" until the TLS handshake, so the
+            // discarded plain-text reader cannot have buffered any post-handshake bytes.
+            _reader = new StreamReader(_stream!, Encoding.ASCII);
+            _writer = new StreamWriter(_stream!, Encoding.ASCII) { AutoFlush = true };
+
+            // RFC 3207: the client must re-issue EHLO over the secured channel.
+            await SendEhloAsync(cancellationToken).ConfigureAwait(false);
         }
 
         private async Task AuthPlainAsync(CancellationToken cancellationToken)
@@ -548,7 +639,7 @@ private async Task<SmtpResponse> ReadMultilineResponseAsync(CancellationToken ca
             return new SmtpResponse(code, [.. lines]);
         }
 
-        private bool ValidateServerCertificate(
+        private bool OnCertificateValidation(
             object sender,
             X509Certificate? certificate,
             X509Chain? chain,
@@ -559,9 +650,31 @@ private bool ValidateServerCertificate(
                 return true;
             }
 
-            _logger.LogWarning("SSL certificate validation error: {Errors}", sslPolicyErrors);
+            // Opportunistic STARTTLS only: when we upgraded an otherwise plain-text connection
+            // and TLS is not required, encryption without authentication is still better than
+            // falling back to plain text, so accept the certificate but warn. This must NOT apply
+            // to implicit TLS (EnableSsl), where there is no plain-text fallback and silently
+            // accepting any certificate would mask a man-in-the-middle.
+            if (!EnableSsl && !RequireTls)
+            {
+                _logger.LogWarning(
+                    "Ignoring SSL certificate error ({Errors}) for opportunistic TLS to {Host}:{Port}",
+                    sslPolicyErrors, Host, Port);
+                return true;
+            }
+
+            // Implicit TLS or required TLS: honor the configured certificate validation policy.
+            if (!ValidateServerCertificate)
+            {
+                _logger.LogWarning(
+                    "SSL certificate error ({Errors}) ignored by configuration for {Host}:{Port}",
+                    sslPolicyErrors, Host, Port);
+                return true;
+            }
 
-            // You might want to make this configurable
+            _logger.LogWarning(
+                "SSL certificate validation failed ({Errors}) for {Host}:{Port}",
+                sslPolicyErrors, Host, Port);
             return false;
         }
 

src/Zetian.Relay/Extensions/SmtpServerExtensions.cs

@@ -81,8 +81,8 @@ public static ISmtpServer AddSmartHost(
                 Host = host,
                 Port = port,
                 Credentials = credentials,
-                UseTls = port is 465 or 587,
-                UseStartTls = port == 587
+                UseTls = port == 465,
+                UseStartTls = port != 465
             };
 
             // This is a simplified approach - in production you'd modify the configuration

src/Zetian.Relay/Services/RelayService.cs

@@ -417,21 +417,43 @@ private async Task<bool> CanRelayAsync(ISmtpSession session, ISmtpMessage messag
                 string host = parts[0];
                 int port = parts.Length > 1 && int.TryParse(parts[1], out int p) ? p : 25;
 
-                // Find matching configuration
+                // Prefer an explicitly configured smart host so its TLS, credential and timeout
+                // settings are honored instead of being overwritten by global defaults.
+                if (Configuration.DefaultSmartHost != null &&
+                    string.Equals(Configuration.DefaultSmartHost.Host, host, StringComparison.OrdinalIgnoreCase) &&
+                    Configuration.DefaultSmartHost.Port == port)
+                {
+                    return Configuration.DefaultSmartHost;
+                }
+
+                // Find matching failover smart host
                 SmartHostConfiguration? config = Configuration.SmartHosts
-                    .FirstOrDefault(sh => sh.Host == host && sh.Port == port && sh.Enabled);
+                    .FirstOrDefault(sh => string.Equals(sh.Host, host, StringComparison.OrdinalIgnoreCase) &&
+                                          sh.Port == port && sh.Enabled);
+
+                if (config != null)
+                {
+                    return config;
+                }
+
+                // Find matching domain-routing smart host
+                config = Configuration.DomainRouting.Values
+                    .FirstOrDefault(sh => string.Equals(sh.Host, host, StringComparison.OrdinalIgnoreCase) &&
+                                          sh.Port == port);
 
                 if (config != null)
                 {
                     return config;
                 }
 
-                // Create default configuration
+                // No explicit configuration (e.g. an MX-routed host): build a sensible default.
+                // Use implicit TLS only for the SMTPS port (465); otherwise rely on opportunistic
+                // STARTTLS so delivery falls back to plain text when the server offers no TLS.
                 return new SmartHostConfiguration
                 {
                     Host = host,
                     Port = port,
-                    UseTls = Configuration.EnableTls,
+                    UseTls = port == 465,
                     UseStartTls = Configuration.EnableTls,
                     ConnectionTimeout = Configuration.ConnectionTimeout
                 };
@@ -450,7 +472,11 @@ private ISmtpClient GetOrCreateClient(SmartHostConfiguration config)
                 {
                     Host = config.Host,
                     Port = config.Port,
-                    EnableSsl = config.UseTls,
+                    EnableSsl = config.UseTls,                                   // implicit TLS (SMTPS)
+                    UseStartTls = config.UseStartTls,                            // opportunistic STARTTLS
+                    RequireTls = Configuration.RequireTls,                       // TLS requirement policy
+                    SslProtocols = Configuration.SslProtocols,
+                    ValidateServerCertificate = Configuration.ValidateServerCertificate,
                     Credentials = config.Credentials,
                     LocalDomain = Configuration.LocalDomain,
                     Timeout = config.ConnectionTimeout
@@ -523,15 +549,10 @@ private async Task SendBounceMessageAsync(IRelayMessage message, string error)
                 {
                     ISmtpClient client = GetOrCreateClient(Configuration.DefaultSmartHost);
 
-                    // Connect if not connected
+                    // Connect if not connected. The client is already configured with the smart
+                    // host's TLS, credential and timeout settings by GetOrCreateClient.
                     if (!client.IsConnected)
                     {
-                        // Set connection parameters
-                        client.Host = Configuration.DefaultSmartHost.Host;
-                        client.Port = Configuration.DefaultSmartHost.Port;
-                        client.EnableSsl = Configuration.DefaultSmartHost.UseTls || Configuration.DefaultSmartHost.UseStartTls;
-                        client.Credentials = Configuration.DefaultSmartHost.Credentials;
-
                         await client.ConnectAsync().ConfigureAwait(false);
 
                         // Authenticate if credentials provided