Make HttpSys client certificate behavior consistent #33586 (#34012)

Author: Tratcher

src/Servers/HttpSys/Directory.Build.props

@@ -0,0 +1,6 @@
+<Project>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory)..\, Directory.Build.props))\Directory.Build.props" />
+  <PropertyGroup>
+    <KestrelSharedSourceRoot>$(MSBuildThisFileDirectory)..\Kestrel\shared\</KestrelSharedSourceRoot>
+  </PropertyGroup>
+</Project>

src/Servers/HttpSys/HttpSysServer.slnf

@@ -8,10 +8,10 @@
       "src\\Hosting\\Server.Abstractions\\src\\Microsoft.AspNetCore.Hosting.Server.Abstractions.csproj",
       "src\\Http\\Authentication.Abstractions\\src\\Microsoft.AspNetCore.Authentication.Abstractions.csproj",
       "src\\Http\\Authentication.Core\\src\\Microsoft.AspNetCore.Authentication.Core.csproj",
+      "src\\Http\\Features\\src\\Microsoft.Extensions.Features.csproj",
       "src\\Http\\Headers\\src\\Microsoft.Net.Http.Headers.csproj",
       "src\\Http\\Http.Abstractions\\src\\Microsoft.AspNetCore.Http.Abstractions.csproj",
       "src\\Http\\Http.Extensions\\src\\Microsoft.AspNetCore.Http.Extensions.csproj",
-      "src\\Http\\Features\\src\\Microsoft.Extensions.Features.csproj",
       "src\\Http\\Http.Features\\src\\Microsoft.AspNetCore.Http.Features.csproj",
       "src\\Http\\Http\\src\\Microsoft.AspNetCore.Http.csproj",
       "src\\Http\\Metadata\\src\\Microsoft.AspNetCore.Metadata.csproj",
@@ -25,8 +25,8 @@
       "src\\Servers\\HttpSys\\src\\Microsoft.AspNetCore.Server.HttpSys.csproj",
       "src\\Servers\\HttpSys\\test\\FunctionalTests\\Microsoft.AspNetCore.Server.HttpSys.FunctionalTests.csproj",
       "src\\Servers\\HttpSys\\test\\Tests\\Microsoft.AspNetCore.Server.HttpSys.Tests.csproj",
-      "src\\Servers\\Kestrel\\Transport.Sockets\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj",
       "src\\Servers\\IIS\\IIS\\src\\Microsoft.AspNetCore.Server.IIS.csproj",
+      "src\\Servers\\Kestrel\\Transport.Sockets\\src\\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj",
       "src\\Testing\\src\\Microsoft.AspNetCore.Testing.csproj"
     ]
   }

src/Servers/HttpSys/src/RequestProcessing/RequestContext.FeatureCollection.cs

@@ -62,6 +62,7 @@ internal partial class RequestContext :
         private string? _connectionId;
         private string? _traceIdentitfier;
         private X509Certificate2? _clientCert;
+        private Task<X509Certificate2?>? _clientCertTask;
         private ClaimsPrincipal? _user;
         private Stream _responseStream = default!;
         private PipeWriter? _pipeWriter;
@@ -318,15 +319,10 @@ string IHttpConnectionFeature.ConnectionId
                 if (IsNotInitialized(Fields.ClientCertificate))
                 {
                     var method = Server.Options.ClientCertificateMethod;
-                    if (method == ClientCertificateMethod.AllowCertificate)
+                    if (method != ClientCertificateMethod.NoCertificate)
                     {
                         _clientCert = Request.ClientCertificate;
                     }
-                    else if (method == ClientCertificateMethod.AllowRenegotation)
-                    {
-                        _clientCert = Request.GetClientCertificateAsync().Result; // TODO: Sync over async;
-                    }
-                    // else if (method == ClientCertificateMethod.NoCertificate) // No-op
 
                     SetInitialized(Fields.ClientCertificate);
                 }
@@ -335,29 +331,34 @@ string IHttpConnectionFeature.ConnectionId
             set
             {
                 _clientCert = value;
+                _clientCertTask = Task.FromResult(value);
                 SetInitialized(Fields.ClientCertificate);
             }
         }
 
-        async Task<X509Certificate2?> ITlsConnectionFeature.GetClientCertificateAsync(CancellationToken cancellationToken)
+        Task<X509Certificate2?> ITlsConnectionFeature.GetClientCertificateAsync(CancellationToken cancellationToken)
         {
-            if (IsNotInitialized(Fields.ClientCertificate))
+            if (_clientCertTask != null)
             {
-                var method = Server.Options.ClientCertificateMethod;
-                if (method != ClientCertificateMethod.NoCertificate)
-                {
-                    // Check if a cert was already available on the connection.
-                    _clientCert = Request.ClientCertificate;
-                }
+                return _clientCertTask;
+            }
 
-                if (_clientCert == null && method == ClientCertificateMethod.AllowRenegotation)
-                {
-                    _clientCert = await Request.GetClientCertificateAsync(cancellationToken);
-                }
+            var tlsFeature = (ITlsConnectionFeature)this;
+            var clientCert = tlsFeature.ClientCertificate; // Lazy initialized
+            if (clientCert != null
+                || Server.Options.ClientCertificateMethod != ClientCertificateMethod.AllowRenegotation
+                // Delayed client cert negotiation is not allowed on HTTP/2.
+                || Request.ProtocolVersion >= HttpVersion.Version20)
+            {
+                return _clientCertTask = Task.FromResult(clientCert);
+            }
 
-                SetInitialized(Fields.ClientCertificate);
+            return _clientCertTask = GetCertificateAsync(cancellationToken);
+
+            async Task<X509Certificate2?> GetCertificateAsync(CancellationToken cancellation)
+            {
+                return _clientCert = await Request.GetClientCertificateAsync(cancellation);
             }
-            return _clientCert;
         }
 
         internal ITlsConnectionFeature? GetTlsConnectionFeature()

src/Servers/HttpSys/test/FunctionalTests/HttpsTests.cs

@@ -21,6 +21,8 @@ namespace Microsoft.AspNetCore.Server.HttpSys
 {
     public class HttpsTests
     {
+        private static readonly X509Certificate2 _x509Certificate2 = TestResources.GetTestCertificate("eku.client.pfx");
+
         [ConditionalFact]
         public async Task Https_200OK_Success()
         {
@@ -66,38 +68,61 @@ public async Task Https_EchoHelloWorld_Success()
             }
         }
 
-        [ConditionalFact]
-        public async Task Https_ClientCertNotSent_ClientCertNotPresent()
+        [ConditionalTheory]
+        [InlineData(ClientCertificateMethod.NoCertificate)]
+        [InlineData(ClientCertificateMethod.AllowCertificate)]
+        [InlineData(ClientCertificateMethod.AllowRenegotation)]
+        public async Task Https_ClientCertNotSent_ClientCertNotPresent(ClientCertificateMethod clientCertificateMethod)
         {
-            using (Utilities.CreateDynamicHttpsServer(out var address, async httpContext =>
+            using (Utilities.CreateDynamicHttpsServer("", out var root, out var address, options =>
+            {
+                options.ClientCertificateMethod = clientCertificateMethod;
+            },
+            async httpContext =>
             {
                 var tls = httpContext.Features.Get<ITlsConnectionFeature>();
                 Assert.NotNull(tls);
+                Assert.Null(tls.ClientCertificate);
                 var cert = await tls.GetClientCertificateAsync(CancellationToken.None);
                 Assert.Null(cert);
                 Assert.Null(tls.ClientCertificate);
             }))
             {
-                string response = await SendRequestAsync(address);
+                var response = await SendRequestAsync(address);
                 Assert.Equal(string.Empty, response);
             }
         }
 
-        [ConditionalFact(Skip = "Manual test only, client certs are not always available.")]
-        public async Task Https_ClientCertRequested_ClientCertPresent()
+        [ConditionalTheory]
+        [InlineData(ClientCertificateMethod.NoCertificate)]
+        [InlineData(ClientCertificateMethod.AllowCertificate)]
+        [InlineData(ClientCertificateMethod.AllowRenegotation)]
+        public async Task Https_ClientCertRequested_ClientCertPresent(ClientCertificateMethod clientCertificateMethod)
         {
-            using (Utilities.CreateDynamicHttpsServer(out var address, async httpContext =>
+            using (Utilities.CreateDynamicHttpsServer("", out var root, out var address, options =>
+            {
+                options.ClientCertificateMethod = clientCertificateMethod;
+            },
+            async httpContext =>
             {
                 var tls = httpContext.Features.Get<ITlsConnectionFeature>();
                 Assert.NotNull(tls);
+                Assert.Null(tls.ClientCertificate);
                 var cert = await tls.GetClientCertificateAsync(CancellationToken.None);
-                Assert.NotNull(cert);
-                Assert.NotNull(tls.ClientCertificate);
+                if (clientCertificateMethod == ClientCertificateMethod.AllowRenegotation)
+                {
+                    Assert.NotNull(cert);
+                    Assert.NotNull(tls.ClientCertificate);
+                }
+                else
+                {
+                    Assert.Null(cert);
+                    Assert.Null(tls.ClientCertificate);
+                }
             }))
             {
-                X509Certificate2 cert = FindClientCert();
-                Assert.NotNull(cert);
-                string response = await SendRequestAsync(address, cert);
+                Assert.NotNull(_x509Certificate2);
+                var response = await SendRequestAsync(address, _x509Certificate2);
                 Assert.Equal(string.Empty, response);
             }
         }

src/Servers/HttpSys/test/FunctionalTests/Microsoft.AspNetCore.Server.HttpSys.FunctionalTests.csproj

@@ -15,6 +15,8 @@
     <Compile Include="$(SharedSourceRoot)ValueTaskExtensions\**\*.cs" LinkBase="Shared\" />
     <Compile Remove="$(SharedSourceRoot)ServerInfrastructure\DuplexPipe.cs" />
     <Compile Remove="$(SharedSourceRoot)ServerInfrastructure\StringUtilities.cs" />
+    <Compile Include="$(KestrelSharedSourceRoot)test\TestResources.cs" LinkBase="shared" />
+    <Content Include="$(KestrelSharedSourceRoot)test\TestCertificates\*.pfx" LinkBase="shared\TestCertificates" CopyToOutputDirectory="PreserveNewest" />
   </ItemGroup>
 
   <ItemGroup>

src/Servers/Kestrel/Core/src/Internal/TlsConnectionFeature.cs

@@ -46,7 +46,11 @@ public X509Certificate2? ClientCertificate
             {
                 return _clientCert ??= ConvertToX509Certificate2(_sslStream.RemoteCertificate);
             }
-            set => _clientCert = value;
+            set
+            {
+                _clientCert = value;
+                _clientCertTask = Task.FromResult(value);
+            }
         }
 
         // Used for event source, not part of any of the feature interfaces.