diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs index a87555145fbae8..372fd2b06ce932 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpClient.cs @@ -43,6 +43,7 @@ public class SmtpClient : IDisposable private bool _inCall; private bool _timedOut; private string? _targetName; + private const string DefaultTargetNamePrefix = "SMTPSVC/"; private SmtpDeliveryMethod _deliveryMethod = SmtpDeliveryMethod.Network; private SmtpDeliveryFormat _deliveryFormat = SmtpDeliveryFormat.SevenBit; // Non-EAI default private string? _pickupDirectoryLocation; @@ -103,7 +104,7 @@ private void Initialize() _port = DefaultPort; } - _targetName ??= "SMTPSVC/" + _host; + _targetName ??= DefaultTargetNamePrefix + _host; if (_clientDomain == null) { @@ -162,8 +163,20 @@ public string? Host if (value != _host) { + // If TargetName is still the default derived from the current host, keep it in + // sync with the new host so Negotiate/NTLM authentication uses the correct SPN. + // A TargetName explicitly set by the caller (not matching the default) is left + // untouched. + if (_targetName == DefaultTargetNamePrefix + _host) + { + _targetName = DefaultTargetNamePrefix + value; + } + _host = value; _servicePoint = null; + // The cached connection targets the previous host, so invalidate it to force + // a new connection to be established on the next send. + _transport.InvalidateCachedConnection(); } } } @@ -187,6 +200,9 @@ public int Port { _port = value; _servicePoint = null; + // The cached connection targets the previous port, so invalidate it to force + // a new connection to be established on the next send. + _transport.InvalidateCachedConnection(); } } } @@ -316,6 +332,11 @@ public bool EnableSsl } set { + if (_inCall) + { + throw new InvalidOperationException(SR.SmtpInvalidOperationDuringSend); + } + _transport.EnableSsl = value; } } @@ -334,7 +355,21 @@ public X509CertificateCollection ClientCertificates public string? TargetName { get { return _targetName; } - set { _targetName = value; } + set + { + if (_inCall) + { + throw new InvalidOperationException(SR.SmtpInvalidOperationDuringSend); + } + + if (value != _targetName) + { + _targetName = value; + // The target name is the SPN used during authentication, so invalidate any + // cached connection to force a new one on the next send. + _transport.InvalidateCachedConnection(); + } + } } private bool ServerSupportsEai diff --git a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs index 6d678d0f5800ac..19d01f8af9f838 100644 --- a/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs +++ b/src/libraries/System.Net.Mail/src/System/Net/Mail/SmtpTransport.cs @@ -20,6 +20,10 @@ internal sealed class SmtpTransport private readonly SmtpClient _client; private ICredentialsByHost? _credentials; private bool _shouldAbort; + // Written (set true) from property setters without holding the transport lock and read + // from IsConnected on the send path, so it is volatile to make an invalidating + // configuration change reliably observable across threads without widening locking. + private volatile bool _stale; private bool _enableSsl; @@ -43,7 +47,11 @@ internal ICredentialsByHost? Credentials } set { - _credentials = value; + if (!ReferenceEquals(value, _credentials)) + { + _credentials = value; + InvalidateCachedConnection(); + } } } @@ -51,7 +59,7 @@ internal bool IsConnected { get { - return _connection != null && _connection.IsConnected; + return _connection != null && _connection.IsConnected && !_stale; } } @@ -63,7 +71,11 @@ internal bool EnableSsl } set { - _enableSsl = value; + if (value != _enableSsl) + { + _enableSsl = value; + InvalidateCachedConnection(); + } } } @@ -89,6 +101,15 @@ internal Task GetConnectionAsync(string host, int port, Cancellation { lock (this) { + // Abort any previously cached connection (for example one that became stale after a + // configuration change, or one whose connect attempt failed) so its socket is not + // leaked. Abort() only force-closes the socket without any network round-trip, so it + // is safe to run under the lock and does not block this async send path. A graceful + // QUIT is unnecessary for a connection we are discarding. Sends are serialized by + // SmtpClient._inCall, so no other GetConnectionAsync can run concurrently here. + _connection?.Abort(); + _stale = false; + _connection = new SmtpConnection(this, _client, _credentials, _authenticationModules); if (_shouldAbort) { @@ -147,6 +168,15 @@ internal void ReleaseConnection() _connection?.ReleaseConnection(); } + // Marks any cached connection as stale without performing blocking work. The connection is + // aborted and replaced the next time one is established (see GetConnectionAsync). This is + // called when a property that affects how the connection is established (host, port, + // credentials, SSL settings, target name) changes. + internal void InvalidateCachedConnection() + { + _stale = true; + } + internal void Abort() { lock (this) diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs index 5dfa223a8ca8c2..743cef1574e3ef 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientConnectionTest.cs @@ -9,6 +9,13 @@ namespace System.Net.Mail.Tests { + public enum ConnectionAffectingProperty + { + Host, + Credentials, + TargetName, + } + public abstract class SmtpClientConnectionTest : LoopbackServerTestBase where TSendMethod : ISendMethodProvider { @@ -84,6 +91,77 @@ public async Task EHelloNotRecognized_RestartWithHello() await SendMail(new MailMessage("mono@novell.com", "everyone@novell.com", "introduction", "hello")); Assert.True(helloReceived, "HELO command was not received."); } + + [Fact] + public async Task ChangingPort_DoesNotReuseConnectionToPreviousServer() + { + using LoopbackSmtpServer server2 = new LoopbackSmtpServer(Output); + + await SendMail(new MailMessage("first@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal("", Server.MailFrom); + + // Point the client at a different server. The cached connection to the original + // server must be dropped so the next message is delivered to the new server. + Smtp.Port = server2.Port; + + await SendMail(new MailMessage("second@example.com", "everyone@novell.com", "introduction", "hello")); + + Assert.Equal("", server2.MailFrom); + Assert.Equal(1, server2.ConnectionCount); + + // The original server must not have received the second message. + Assert.Equal("", Server.MailFrom); + } + + [Theory] + [InlineData(ConnectionAffectingProperty.Host)] + [InlineData(ConnectionAffectingProperty.Credentials)] + [InlineData(ConnectionAffectingProperty.TargetName)] + public async Task ChangingConnectionProperty_EstablishesNewConnection(ConnectionAffectingProperty property) + { + Server.ReceiveMultipleConnections = true; + + await SendMail(new MailMessage("first@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal(1, Server.ConnectionCount); + Assert.Equal("", Server.MailFrom); + + // Change a property that affects how the connection is established. The cached + // connection must be invalidated and a new one established on the next send. + switch (property) + { + case ConnectionAffectingProperty.Host: + // A different value that still resolves to the loopback server. The default + // TargetName should follow the host so authentication uses the correct SPN. + Assert.Equal("SMTPSVC/localhost", Smtp.TargetName); + Smtp.Host = "127.0.0.1"; + Assert.Equal("SMTPSVC/127.0.0.1", Smtp.TargetName); + break; + case ConnectionAffectingProperty.Credentials: + Smtp.Credentials = new NetworkCredential("foo", "bar"); + break; + case ConnectionAffectingProperty.TargetName: + Smtp.TargetName = "SMTPSVC/example.com"; + break; + } + + await SendMail(new MailMessage("second@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal(2, Server.ConnectionCount); + Assert.Equal("", Server.MailFrom); + } + + [Fact] + public async Task ChangingHost_PreservesExplicitlySetTargetName() + { + // A TargetName explicitly set by the caller must not be overwritten when the host + // changes, even though the default (host-derived) TargetName does follow the host. + Smtp.TargetName = "SMTPSVC/explicit.example.com"; + + await SendMail(new MailMessage("first@example.com", "everyone@novell.com", "introduction", "hello")); + Assert.Equal("SMTPSVC/explicit.example.com", Smtp.TargetName); + + Smtp.Host = "127.0.0.1"; + Assert.Equal("SMTPSVC/explicit.example.com", Smtp.TargetName); + } } public class SmtpClientConnectionTest_Send : SmtpClientConnectionTest diff --git a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs index 0ca6393f1f6cab..262a01e06f7919 100644 --- a/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs +++ b/src/libraries/System.Net.Mail/tests/Functional/SmtpClientTlsTest.cs @@ -185,6 +185,27 @@ public async Task ClientCertificateRequired_Sent() Assert.Equal(clientCert, receivedClientCert); } + [ActiveIssue("https://github.com/dotnet/runtime/issues/120959", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot), nameof(PlatformDetection.IsAndroid))] + [Fact] + public async Task EnableSsl_ChangedAfterConnect_EstablishesNewEncryptedConnection() + { + Server.ReceiveMultipleConnections = true; + _serverCertValidationCallback = (cert, chain, errors) => true; + + // First send happens over a plaintext connection. + await SendMail(new MailMessage("foo@example.com", "bar@example.com", "hello", "howdydoo")); + Assert.Equal(1, Server.ConnectionCount); + Assert.False(Server.IsEncrypted, "First connection should not be encrypted."); + + // Enabling SSL must invalidate the cached plaintext connection so the next send + // establishes a new, encrypted connection instead of reusing the old one. + Smtp.EnableSsl = true; + + await SendMail(new MailMessage("foo@example.com", "bar@example.com", "hello", "howdydoo")); + Assert.Equal(2, Server.ConnectionCount); + Assert.True(Server.IsEncrypted, "Second connection should be encrypted after enabling SSL."); + } + private bool ServerCertValidationCallback(object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) { if (_serverCertValidationCallback != null)