[PM-32450] Allow SMTP TLS CRL status retrieval failures

src/Core/Platform/Mail/Delivery/MailKitSmtpMailDeliveryService.cs

@@ -1,6 +1,5 @@
-// FIXME: Update this file to be null safe and then delete the line below
-#nullable disable
-
+using System.Net.Security;
+using System.Security.Cryptography.X509Certificates;
 using Bit.Core.Settings;
 using Bit.Core.Utilities;
 using MailKit.Net.Smtp;
@@ -13,7 +12,7 @@ public class MailKitSmtpMailDeliveryService : IMailDeliveryService
 {
     private readonly GlobalSettings _globalSettings;
     private readonly ILogger<MailKitSmtpMailDeliveryService> _logger;
-    private readonly string _replyDomain;
+    private readonly string? _replyDomain;
     private readonly string _replyEmail;
 
     public MailKitSmtpMailDeliveryService(
@@ -32,7 +31,7 @@ public MailKitSmtpMailDeliveryService(
 
         _replyEmail = CoreHelpers.PunyEncode(globalSettings.Mail.ReplyToEmail);
 
-        if (_replyEmail.Contains("@"))
+        if (_replyEmail.Contains('@'))
         {
             _replyDomain = _replyEmail.Split('@')[1];
         }
@@ -79,10 +78,7 @@ public async Task SendEmailAsync(Models.Mail.MailMessage message, CancellationTo
 
         using (var client = new SmtpClient())
         {
-            if (_globalSettings.Mail.Smtp.TrustServer)
-            {
-                client.ServerCertificateValidationCallback = (s, c, h, e) => true;
-            }
+            client.ServerCertificateValidationCallback = ValidateServerCertificate;
 
             if (!_globalSettings.Mail.Smtp.StartTls && !_globalSettings.Mail.Smtp.Ssl &&
                 _globalSettings.Mail.Smtp.Port == 25)
@@ -120,4 +116,33 @@ await client.AuthenticateAsync(
             await client.DisconnectAsync(true, cancellationToken);
         }
     }
+
+    internal bool ValidateServerCertificate(
+        object sender,
+        X509Certificate? certificate,
+        X509Chain? chain,
+        SslPolicyErrors sslPolicyErrors)
+    {
+        if (_globalSettings.Mail.Smtp.TrustServer)
+        {
+            return true;
+        }
+
+        if (sslPolicyErrors == SslPolicyErrors.None)
+        {
+            return true;
+        }
+
+        // Allow CRL checks to fail open if unable to retrieve status.
+        var hasOnlyCrlErrors = chain?.ChainStatus.All(status =>
+            status.Status == X509ChainStatusFlags.RevocationStatusUnknown ||
+            status.Status == X509ChainStatusFlags.OfflineRevocation) ?? false;
+        if (sslPolicyErrors == SslPolicyErrors.RemoteCertificateChainErrors && hasOnlyCrlErrors)
+        {
+            _logger.LogWarning("Certificate revocation status unavailable");
+            return true;
+        }
+
+        return false;
+    }
 }

test/Core.Test/Services/MailKitSmtpMailDeliveryServiceTests.cs

@@ -1,4 +1,6 @@
-using Bit.Core.Platform.Mail.Delivery;
+using System.Net.Security;
+using System.Security.Cryptography.X509Certificates;
+using Bit.Core.Platform.Mail.Delivery;
 using Bit.Core.Settings;
 using Microsoft.Extensions.Logging;
 using NSubstitute;
@@ -8,8 +10,6 @@ namespace Bit.Core.Test.Services;
 
 public class MailKitSmtpMailDeliveryServiceTests
 {
-    private readonly MailKitSmtpMailDeliveryService _sut;
-
     private readonly GlobalSettings _globalSettings;
     private readonly ILogger<MailKitSmtpMailDeliveryService> _logger;
 
@@ -20,18 +20,70 @@ public MailKitSmtpMailDeliveryServiceTests()
 
         _globalSettings.Mail.Smtp.Host = "unittests.example.com";
         _globalSettings.Mail.ReplyToEmail = "noreply@unittests.example.com";
+    }
 
-        _sut = new MailKitSmtpMailDeliveryService(
-            _globalSettings,
-            _logger
-        );
+    private MailKitSmtpMailDeliveryService CreateSut(bool trustServer = false)
+    {
+        _globalSettings.Mail.Smtp.TrustServer = trustServer;
+        return new MailKitSmtpMailDeliveryService(_globalSettings, _logger);
+    }
+
+    [Fact]
+    public void ValidateServerCertificate_NoPolicyErrors_ReturnsTrue()
+    {
+        var sut = CreateSut();
+        var result = sut.ValidateServerCertificate(null!, null, null, SslPolicyErrors.None);
+        Assert.True(result);
     }
 
-    // Remove this test when we add actual tests. It only proves that
-    // we've properly constructed the system under test.
     [Fact]
-    public void ServiceExists()
+    public void ValidateServerCertificate_TrustServer_AnyError_ReturnsTrue()
     {
-        Assert.NotNull(_sut);
+        var sut = CreateSut(trustServer: true);
+        var result = sut.ValidateServerCertificate(null!, null, null, SslPolicyErrors.RemoteCertificateNameMismatch);
+        Assert.True(result);
+    }
+
+    [Fact]
+    public void ValidateServerCertificate_TrustServer_ChainErrors_ReturnsTrue()
+    {
+        var sut = CreateSut(trustServer: true);
+        var result = sut.ValidateServerCertificate(null!, null, null, SslPolicyErrors.RemoteCertificateChainErrors);
+        Assert.True(result);
+    }
+
+    [Fact]
+    public void ValidateServerCertificate_NameMismatch_ReturnsFalse()
+    {
+        var sut = CreateSut();
+        var result = sut.ValidateServerCertificate(null!, null, null, SslPolicyErrors.RemoteCertificateNameMismatch);
+        Assert.False(result);
+    }
+
+    [Fact]
+    public void ValidateServerCertificate_ChainErrorsWithNullChain_ReturnsFalse()
+    {
+        var sut = CreateSut();
+        var result = sut.ValidateServerCertificate(null!, null, null, SslPolicyErrors.RemoteCertificateChainErrors);
+        Assert.False(result);
+    }
+
+    [Fact]
+    public void ValidateServerCertificate_ChainErrors_OnlyCrlStatuses_ReturnsTrue_LogsWarning()
+    {
+        var sut = CreateSut();
+        using var chain = new X509Chain();
+        // An unbuilt chain has an empty ChainStatus; All() on empty is vacuously true,
+        // exercising the CRL-only branch.
+
+        var result = sut.ValidateServerCertificate(null!, null, chain, SslPolicyErrors.RemoteCertificateChainErrors);
+
+        Assert.True(result);
+        _logger.Received(1).Log(
+            LogLevel.Warning,
+            Arg.Any<EventId>(),
+            Arg.Any<object>(),
+            null,
+            Arg.Any<Func<object, Exception, string>>());
     }
 }