Improve based on pull request feedback

src/Microsoft.AspNet.Server.Kestrel.Https/ClientCertificateValidationCallback.cs

@@ -1,11 +0,0 @@
-// Copyright (c) .NET Foundation. All rights reserved.
-// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
-
-using System.Net.Security;
-using System.Security.Cryptography.X509Certificates;
-
-namespace Microsoft.AspNet.Server.Kestrel.Https
-{
-    public delegate bool ClientCertificateValidationCallback(
-        X509Certificate2 certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors);
-}

src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilter.cs

@@ -14,28 +14,25 @@ namespace Microsoft.AspNet.Server.Kestrel.Https
 {
     public class HttpsConnectionFilter : IConnectionFilter
     {
-        private readonly X509Certificate2 _serverCert;
+        private readonly HttpsConnectionFilterOptions _options;
-        private readonly ClientCertificateMode _clientCertMode;
-        private readonly ClientCertificateValidationCallback _clientValidationCallback;
         private readonly IConnectionFilter _previous;
-        private readonly SslProtocols _sslProtocols;
-        private X509Certificate2 _clientCert;
 
         public HttpsConnectionFilter(HttpsConnectionFilterOptions options, IConnectionFilter previous)
         {
-            if (options.ServerCertificate == null)
+            if (options == null)
             {
-                throw new ArgumentNullException(nameof(options.ServerCertificate));
+                throw new ArgumentNullException(nameof(options));
             }
             if (previous == null)
             {
                 throw new ArgumentNullException(nameof(previous));
             }
+            if (options.ServerCertificate == null)
+            {
+                throw new ArgumentException("The server certificate parameter is required.");
+            }
 
-            _serverCert = options.ServerCertificate;
+            _options = options;
-            _clientCertMode = options.ClientCertificateMode;
-            _clientValidationCallback = options.ClientCertificateValidation;
-            _sslProtocols = options.SslProtocols;
             _previous = previous;
         }
 
@@ -45,68 +42,76 @@ namespace Microsoft.AspNet.Server.Kestrel.Https
 
             if (string.Equals(context.Address.Scheme, "https", StringComparison.OrdinalIgnoreCase))
             {
+                X509Certificate2 clientCertificate = null;
                 SslStream sslStream;
-                if (_clientCertMode == ClientCertificateMode.NoCertificate)
+                if (_options.ClientCertificateMode == ClientCertificateMode.NoCertificate)
                 {
                     sslStream = new SslStream(context.Connection);
-                    await sslStream.AuthenticateAsServerAsync(_serverCert, clientCertificateRequired: false,
+                    await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, clientCertificateRequired: false,
-                        enabledSslProtocols: _sslProtocols, checkCertificateRevocation: false);
+                        enabledSslProtocols: _options.SslProtocols, checkCertificateRevocation: _options.CheckCertificateRevocation);
                 }
                 else
                 {
                     sslStream = new SslStream(context.Connection, leaveInnerStreamOpen: false,
                         userCertificateValidationCallback: (sender, certificate, chain, sslPolicyErrors) =>
                         {
-                            if (sslPolicyErrors.HasFlag(SslPolicyErrors.RemoteCertificateNotAvailable))
+                            Console.WriteLine("callback type: " + (certificate is X509Certificate2));
+                            if (certificate == null)
                             {
-                                return _clientCertMode != ClientCertificateMode.RequireCertificate;
+                                return _options.ClientCertificateMode != ClientCertificateMode.RequireCertificate;
                             }
 
-
+                            if (_options.ClientCertificateValidation == null)
-                            if (_clientValidationCallback == null)
                             {
                                 if (sslPolicyErrors != SslPolicyErrors.None)
                                 {
                                     return false;
                                 }
                             }
-#if DOTNET5_4
-                            // conversion X509Certificate to X509Certificate2 not supported
-                            // https://github.com/dotnet/corefx/issues/4510
-                            X509Certificate2 certificate2 = null;
-                            return false;
-#else
-                            X509Certificate2 certificate2 = certificate as X509Certificate2 ??
-                                                            new X509Certificate2(certificate);
 
-#endif
+                            X509Certificate2 certificate2 = certificate as X509Certificate2;
-                            if (_clientValidationCallback != null)
+                            if (certificate2 == null)
                             {
-                                if (!_clientValidationCallback(certificate2, chain, sslPolicyErrors))
+#if DOTNET5_4
+                                // conversion X509Certificate to X509Certificate2 not supported
+                                // https://github.com/dotnet/corefx/issues/4510
+                                return false;
+#else
+                                certificate2 = new X509Certificate2(certificate);
+#endif
+                            }
+
+                            if (_options.ClientCertificateValidation != null)
+                            {
+                                if (!_options.ClientCertificateValidation(certificate2, chain, sslPolicyErrors))
                                 {
                                     return false;
                                 }
                             }
 
-                            _clientCert = certificate2;
+                            clientCertificate = certificate2;
                             return true;
                         });
-                    await sslStream.AuthenticateAsServerAsync(_serverCert, clientCertificateRequired: true,
+                    await sslStream.AuthenticateAsServerAsync(_options.ServerCertificate, clientCertificateRequired: true,
-                        enabledSslProtocols: _sslProtocols, checkCertificateRevocation: false);
+                        enabledSslProtocols: _options.SslProtocols, checkCertificateRevocation: _options.CheckCertificateRevocation);
+                    Console.WriteLine("remote type: " + (sslStream.RemoteCertificate is X509Certificate2));
                 }
+
+                var previousPrepareRequest = context.PrepareRequest;
+                context.PrepareRequest = features =>
+                    {
+                        previousPrepareRequest?.Invoke(features);
+
+                        if (clientCertificate != null)
+                        {
+                            features.Set<ITlsConnectionFeature>(
+                                new TlsConnectionFeature {ClientCertificate = clientCertificate});
+                        }
+
+                        features.Get<IHttpRequestFeature>().Scheme = "https";
+                    };
                 context.Connection = sslStream;
             }
         }
-
-        public void PrepareRequest(IFeatureCollection features)
-        {
-            _previous.PrepareRequest(features);
-
-            if (_clientCert != null)
-            {
-                features.Set<ITlsConnectionFeature>(
-                    new TlsConnectionFeature { ClientCertificate = _clientCert });
-            }
-        }
     }
 }

src/Microsoft.AspNet.Server.Kestrel.Https/HttpsConnectionFilterOptions.cs

@@ -1,6 +1,8 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
+using System.Net.Security;
 using System.Security.Authentication;
 using System.Security.Cryptography.X509Certificates;
 
@@ -16,7 +18,8 @@ namespace Microsoft.AspNet.Server.Kestrel.Https
 
         public X509Certificate2 ServerCertificate { get; set; }
         public ClientCertificateMode ClientCertificateMode { get; set; }
-        public ClientCertificateValidationCallback ClientCertificateValidation { get; set; }
+        public Func<X509Certificate2, X509Chain, SslPolicyErrors, bool> ClientCertificateValidation { get; set; }
         public SslProtocols SslProtocols { get; set; }
+        public bool CheckCertificateRevocation { get; set; }
     }
 }

src/Microsoft.AspNet.Server.Kestrel/Filter/ConnectionFilterContext.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.IO;
 using Microsoft.AspNet.Http.Features;
 
@@ -10,5 +11,6 @@ namespace Microsoft.AspNet.Server.Kestrel.Filter
     {
         public ServerAddress Address { get; set; }
         public Stream Connection { get; set; }
+        public Action<IFeatureCollection> PrepareRequest { get; set; }
     }
 }

src/Microsoft.AspNet.Server.Kestrel/Filter/IConnectionFilter.cs

@@ -9,6 +9,5 @@ namespace Microsoft.AspNet.Server.Kestrel.Filter
     public interface IConnectionFilter
     {
         Task OnConnection(ConnectionFilterContext context);
-        void PrepareRequest(IFeatureCollection features);
     }
 }

src/Microsoft.AspNet.Server.Kestrel/Filter/NoOpConnectionFilter.cs

@@ -13,8 +13,5 @@ namespace Microsoft.AspNet.Server.Kestrel.Filter
         {
             return TaskUtilities.CompletedTask;
         }
-
-        public void PrepareRequest(IFeatureCollection features)
-        {}
     }
 }

src/Microsoft.AspNet.Server.Kestrel/Http/Connection.cs

@@ -177,7 +177,7 @@ namespace Microsoft.AspNet.Server.Kestrel.Http
 
         private Frame CreateFrame()
         {
-            return new Frame(this, _remoteEndPoint, _localEndPoint, ConnectionFilter);
+            return new Frame(this, _remoteEndPoint, _localEndPoint, _filterContext?.PrepareRequest);
         }
 
         void IConnectionControl.Pause()

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

@@ -56,22 +56,22 @@ namespace Microsoft.AspNet.Server.Kestrel.Http
 
         private readonly IPEndPoint _localEndPoint;
         private readonly IPEndPoint _remoteEndPoint;
-        private readonly IConnectionFilter _connectionFilter;
+        private readonly Action<IFeatureCollection> _prepareRequest;
 
         public Frame(ConnectionContext context)
-            : this(context, remoteEndPoint: null, localEndPoint: null, connectionFilter: null)
+            : this(context, remoteEndPoint: null, localEndPoint: null, prepareRequest: null)
         {
         }
 
         public Frame(ConnectionContext context,
                      IPEndPoint remoteEndPoint,
                      IPEndPoint localEndPoint,
-                     IConnectionFilter connectionFilter)
+                     Action<IFeatureCollection> prepareRequest)
             : base(context)
         {
             _remoteEndPoint = remoteEndPoint;
             _localEndPoint = localEndPoint;
-            _connectionFilter = connectionFilter;
+            _prepareRequest = prepareRequest;
 
             FrameControl = this;
             Reset();

test/Microsoft.AspNet.Server.KestrelTests/ConnectionFilterTests.cs

@@ -95,9 +95,6 @@ namespace Microsoft.AspNet.Server.KestrelTests
                 return _empty;
             }
 
-            public void PrepareRequest(IFeatureCollection frame)
-            {}
-
             public int BytesRead => _rewritingStream.BytesRead;
         }
 
@@ -113,9 +110,6 @@ namespace Microsoft.AspNet.Server.KestrelTests
 
                 context.Connection = new RewritingStream(oldConnection);
             }
-
-            public void PrepareRequest(IFeatureCollection frame)
-            {}
         }
 
         private class RewritingStream : Stream

test/Microsoft.AspNet.Server.KestrelTests/HttpsConnectionFilterTests.cs

@@ -122,7 +122,7 @@ namespace Microsoft.AspNet.Server.KestrelTests
 
                 using (var server = new TestServer(App, serviceContext, serverAddress))
                 {
-                    using (var client = new HttpClient())
+                    using (var client = new HttpClient(handler))
                     {
                         await Assert.ThrowsAnyAsync<Exception>(
                             () => client.GetAsync(serverAddress));
@@ -137,10 +137,6 @@ namespace Microsoft.AspNet.Server.KestrelTests
             }
         }
 
-        // https://github.com/dotnet/corefx/issues/4512
-        // WinHttpHandler throws an Exception (ERROR_INTERNET_SECURE_FAILURE)
-#if DNX451
-
         // https://github.com/aspnet/KestrelHttpServer/issues/240
         // This test currently fails on mono because of an issue with SslStream.
         [ConditionalFact]
@@ -181,7 +177,7 @@ namespace Microsoft.AspNet.Server.KestrelTests
 
                 using (var server = new TestServer(app, serviceContext, serverAddress))
                 {
-                    using (var client = new HttpClient())
+                    using (var client = new HttpClient(handler))
                     {
                         var result = await client.GetAsync(serverAddress);
 
@@ -196,11 +192,6 @@ namespace Microsoft.AspNet.Server.KestrelTests
 #endif
             }
         }
-#endif
-
-        // https://github.com/dotnet/corefx/issues/4510
-        // Can't convert X509Certificate to X509Certificate2 in HttpsConnectionFilter
-#if DNX451
 
         // https://github.com/aspnet/KestrelHttpServer/issues/240
         // This test currently fails on mono because of an issue with SslStream.
@@ -269,7 +260,6 @@ namespace Microsoft.AspNet.Server.KestrelTests
 #endif
     }
 }
-#endif
 
     }
 }