[7.0] Close connections on client cert failures (#43958)

* Close connections on client cert failures #41369
* Update TlsConnectionFeature.cs
* Update src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs

Co-authored-by: Stephen Halter <halter73@gmail.com>

3 files changed, 78 insertions, 55 deletions

diff --git a/src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs b/src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs
index 30265afdd3..c9ce85ee56 100644
--- a/src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs
+++ b/src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs
@@ -4,6 +4,7 @@
 using System.Net.Security;
 using System.Security.Authentication;
 using System.Security.Cryptography.X509Certificates;
+using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Connections.Features;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Features;
@@ -13,6 +14,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
 internal sealed class TlsConnectionFeature : ITlsConnectionFeature, ITlsApplicationProtocolFeature, ITlsHandshakeFeature
 {
     private readonly SslStream _sslStream;
+    private readonly ConnectionContext _context;
     private X509Certificate2? _clientCert;
     private ReadOnlyMemory<byte>? _applicationProtocol;
     private SslProtocols? _protocol;
@@ -24,14 +26,19 @@ internal sealed class TlsConnectionFeature : ITlsConnectionFeature, ITlsApplicat
     private int? _keyExchangeStrength;
     private Task<X509Certificate2?>? _clientCertTask;
 
-    public TlsConnectionFeature(SslStream sslStream)
+    public TlsConnectionFeature(SslStream sslStream, ConnectionContext context)
     {
         if (sslStream is null)
         {
             throw new ArgumentNullException(nameof(sslStream));
         }
+        if (context is null)
+        {
+            throw new ArgumentNullException(nameof(context));
+        }
 
         _sslStream = sslStream;
+        _context = context;
     }
 
     internal bool AllowDelayedClientCertificateNegotation { get; set; }
@@ -122,7 +129,20 @@ internal sealed class TlsConnectionFeature : ITlsConnectionFeature, ITlsApplicat
 
     private async Task<X509Certificate2?> GetClientCertificateAsyncCore(CancellationToken cancellationToken)
     {
-        await _sslStream.NegotiateClientCertificateAsync(cancellationToken);
+        try
+        {
+            await _sslStream.NegotiateClientCertificateAsync(cancellationToken);
+        }
+        catch
+        {
+            // We can't tell which exceptions are fatal or recoverable. Consider them all recoverable only given a new connection
+            // and close the connection gracefully to avoid over-caching and affecting future requests on this connection.
+            // This allows recovery by starting a new connection. The close is graceful to allow the server to
+            // send an error response like 401. https://github.com/dotnet/aspnetcore/issues/41369
+            _context.Features.Get<IConnectionLifetimeNotificationFeature>()?.RequestClose();
+            throw;
+        }
+
         return ClientCertificate;
     }
 
diff --git a/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs b/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
index 7b21c081fa..c011b44fe7 100644
--- a/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
+++ b/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
@@ -141,7 +141,7 @@ internal sealed class HttpsConnectionMiddleware
             context.Features.Get<IMemoryPoolFeature>()?.MemoryPool ?? MemoryPool<byte>.Shared);
         var sslStream = sslDuplexPipe.Stream;
 
-        var feature = new Core.Internal.TlsConnectionFeature(sslStream);
+        var feature = new Core.Internal.TlsConnectionFeature(sslStream, context);
         // Set the mode if options were used. If the callback is used it will set the mode later.
         feature.AllowDelayedClientCertificateNegotation =
             _options?.ClientCertificateMode == ClientCertificateMode.DelayCertificate;
diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs
index e0ec716e8f..e52f8e1d66 100644
--- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs
+++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs
@@ -539,7 +539,6 @@ public class HttpsConnectionMiddlewareTests : LoggedTest
             listenOptions.UseHttps(options =>
             {
                 options.ServerCertificate = _x509Certificate2;
-                options.SslProtocols = SslProtocols.Tls12; // Linux doesn't support renegotiate on TLS1.3 yet. https://github.com/dotnet/runtime/issues/55757
                 options.ClientCertificateMode = ClientCertificateMode.DelayCertificate;
                 options.AllowAnyClientCertificate();
             });
@@ -626,7 +625,6 @@ public class HttpsConnectionMiddlewareTests : LoggedTest
                     return ValueTask.FromResult(new SslServerAuthenticationOptions()
                     {
                         ServerCertificate = _x509Certificate2,
-                        EnabledSslProtocols = SslProtocols.Tls12, // Linux doesn't support renegotiate on TLS1.3 yet. https://github.com/dotnet/runtime/issues/55757
                         ClientCertificateRequired = false,
                         RemoteCertificateValidationCallback = (_, _, _, _) => true,
                     });
@@ -670,7 +668,6 @@ public class HttpsConnectionMiddlewareTests : LoggedTest
             listenOptions.UseHttps(options =>
             {
                 options.ServerCertificate = _x509Certificate2;
-                options.SslProtocols = SslProtocols.Tls12; // Linux doesn't support renegotiate on TLS1.3 yet. https://github.com/dotnet/runtime/issues/55757
                 options.ClientCertificateMode = ClientCertificateMode.DelayCertificate;
                 options.AllowAnyClientCertificate();
             });
@@ -786,60 +783,13 @@ public class HttpsConnectionMiddlewareTests : LoggedTest
     // then the connection is aborted.
     [OSSkipCondition(OperatingSystems.MacOSX, SkipReason = "Missing platform support.")]
     [SkipOnHelix("https://github.com/dotnet/aspnetcore/issues/33566#issuecomment-892031659", Queues = HelixConstants.RedhatAmd64)] // Outdated OpenSSL client
-    public async Task RenegotiateForClientCertificateOnPostWithoutBufferingThrows_TLS12()
+    public async Task RenegotiateForClientCertificateOnPostWithoutBufferingThrows()
     {
         void ConfigureListenOptions(ListenOptions listenOptions)
         {
             listenOptions.Protocols = HttpProtocols.Http1;
             listenOptions.UseHttps(options =>
             {
-                options.SslProtocols = SslProtocols.Tls12;
-                options.ServerCertificate = _x509Certificate2;
-                options.ClientCertificateMode = ClientCertificateMode.DelayCertificate;
-                options.AllowAnyClientCertificate();
-            });
-        }
-
-        // Under 4kb can sometimes work because it fits into Kestrel's header parsing buffer.
-        var expectedBody = new string('a', 1024 * 4);
-
-        await using var server = new TestServer(async context =>
-        {
-            var tlsFeature = context.Features.Get<ITlsConnectionFeature>();
-            Assert.NotNull(tlsFeature);
-            Assert.Null(tlsFeature.ClientCertificate);
-            Assert.Null(context.Connection.ClientCertificate);
-
-            var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => context.Connection.GetClientCertificateAsync());
-            Assert.Equal("Client stream needs to be drained before renegotiation.", ex.Message);
-            Assert.Null(tlsFeature.ClientCertificate);
-            Assert.Null(context.Connection.ClientCertificate);
-        }, new TestServiceContext(LoggerFactory), ConfigureListenOptions);
-
-        using var connection = server.CreateConnection();
-        // SslStream is used to ensure the certificate is actually passed to the server
-        // HttpClient might not send the certificate because it is invalid or it doesn't match any
-        // of the certificate authorities sent by the server in the SSL handshake.
-        // Use a random host name to avoid the TLS session resumption cache.
-        var stream = OpenSslStreamWithCert(connection.Stream);
-        await stream.AuthenticateAsClientAsync(Guid.NewGuid().ToString());
-        await AssertConnectionResult(stream, true, expectedBody);
-    }
-
-    [ConditionalFact]
-    // TLS 1.3 uses a new client cert negotiation extension that doesn't cause the connection to abort
-    // for this error.
-    [MinimumOSVersion(OperatingSystems.Windows, "10.0.20145")] // Needs a preview version with TLS 1.3 enabled.
-    [OSSkipCondition(OperatingSystems.MacOSX | OperatingSystems.Linux, SkipReason = "https://github.com/dotnet/runtime/issues/55757")]
-    [SkipOnHelix("https://github.com/dotnet/aspnetcore/issues/33566#issuecomment-892031659", Queues = HelixConstants.RedhatAmd64)] // Outdated OpenSSL client
-    public async Task RenegotiateForClientCertificateOnPostWithoutBufferingThrows_TLS13()
-    {
-        void ConfigureListenOptions(ListenOptions listenOptions)
-        {
-            listenOptions.Protocols = HttpProtocols.Http1;
-            listenOptions.UseHttps(options =>
-            {
-                options.SslProtocols = SslProtocols.Tls13;
                 options.ServerCertificate = _x509Certificate2;
                 options.ClientCertificateMode = ClientCertificateMode.DelayCertificate;
                 options.AllowAnyClientCertificate();
@@ -977,7 +927,6 @@ public class HttpsConnectionMiddlewareTests : LoggedTest
             listenOptions.UseHttps(options =>
             {
                 options.ServerCertificate = _x509Certificate2;
-                options.SslProtocols = SslProtocols.Tls12; // Linux doesn't support renegotiate on TLS1.3 yet. https://github.com/dotnet/runtime/issues/55757
                 options.ClientCertificateMode = ClientCertificateMode.DelayCertificate;
                 options.AllowAnyClientCertificate();
             });
@@ -1013,6 +962,60 @@ public class HttpsConnectionMiddlewareTests : LoggedTest
         await AssertConnectionResult(stream, true, expectedBody);
     }
 
+    [ConditionalFact]
+    [OSSkipCondition(OperatingSystems.MacOSX, SkipReason = "Missing platform support.")]
+    [SkipOnHelix("https://github.com/dotnet/aspnetcore/issues/33566#issuecomment-892031659", Queues = HelixConstants.RedhatAmd64)] // Outdated OpenSSL client
+    public async Task RenegotationFailureCausesConnectionClose()
+    {
+        void ConfigureListenOptions(ListenOptions listenOptions)
+        {
+            listenOptions.Protocols = HttpProtocols.Http1;
+            listenOptions.UseHttps(options =>
+            {
+                options.ServerCertificate = _x509Certificate2;
+                options.ClientCertificateMode = ClientCertificateMode.DelayCertificate;
+                options.AllowAnyClientCertificate();
+            });
+        }
+
+        var expectedBody = new string('a', 1024 * 4);
+
+        await using var server = new TestServer(async context =>
+        {
+            var tlsFeature = context.Features.Get<ITlsConnectionFeature>();
+            Assert.NotNull(tlsFeature);
+            Assert.Null(tlsFeature.ClientCertificate);
+            Assert.Null(context.Connection.ClientCertificate);
+
+            // Request the client cert while there's still body data in the buffers
+            var ioe = await Assert.ThrowsAsync<InvalidOperationException>(() => context.Connection.GetClientCertificateAsync());
+            Assert.Equal("Client stream needs to be drained before renegotiation.", ioe.Message);
+
+            context.Response.ContentLength = 11;
+            await context.Response.WriteAsync("hello world");
+
+        }, new TestServiceContext(LoggerFactory), ConfigureListenOptions);
+
+        using var connection = server.CreateConnection();
+        // SslStream is used to ensure the certificate is actually passed to the server
+        // HttpClient might not send the certificate because it is invalid or it doesn't match any
+        // of the certificate authorities sent by the server in the SSL handshake.
+        // Use a random host name to avoid the TLS session resumption cache.
+        var stream = OpenSslStreamWithCert(connection.Stream);
+        await stream.AuthenticateAsClientAsync(Guid.NewGuid().ToString());
+
+        var request = Encoding.UTF8.GetBytes($"POST / HTTP/1.1\r\nHost: localhost\r\nContent-Length: {expectedBody.Length}\r\n\r\n{expectedBody}");
+        await stream.WriteAsync(request, 0, request.Length).DefaultTimeout();
+        var reader = new StreamReader(stream);
+        Assert.Equal("HTTP/1.1 200 OK", await reader.ReadLineAsync().DefaultTimeout());
+        Assert.Equal("Content-Length: 11", await reader.ReadLineAsync().DefaultTimeout());
+        Assert.Equal("Connection: close", await reader.ReadLineAsync().DefaultTimeout());
+        Assert.StartsWith("Date: ", await reader.ReadLineAsync().DefaultTimeout());
+        Assert.Equal("", await reader.ReadLineAsync().DefaultTimeout());
+        Assert.Equal("hello world", await reader.ReadLineAsync().DefaultTimeout());
+        Assert.Null(await reader.ReadLineAsync().DefaultTimeout());
+    }
+
     [Fact]
     public async Task HttpsSchemePassedToRequestFeature()
     {