Address PR feedback

dotnet · Aug 31, 2020 · a9436ea · a9436ea
1 parent 6f667fe
commit a9436ea
Show file tree

Hide file tree

Showing 4 changed files with 188 additions and 68 deletions.
diff --git a/src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs b/src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs
@@ -208,7 +208,8 @@ internal static bool TryUseHttps(this ListenOptions listenOptions)
         }
 
         /// <summary>
-        /// Configure Kestrel to use HTTPS.
+        /// Configure Kestrel to use HTTPS. This does not use default certificates or other defaults specified via config or
+        /// <see cref="KestrelServerOptions.ConfigureHttpsDefaults(Action{HttpsConnectionAdapterOptions})"/>.
         /// </summary>
         /// <param name="listenOptions">The <see cref="ListenOptions"/> to configure.</param>
         /// <param name="httpsOptions">Options to configure HTTPS.</param>
@@ -230,31 +231,33 @@ public static ListenOptions UseHttps(this ListenOptions listenOptions, HttpsConn
         }
 
         /// <summary>
-        /// Configure Kestrel to use HTTPS.
+        /// Configure Kestrel to use HTTPS. This does not use default certificates or other defaults specified via config or
+        /// <see cref="KestrelServerOptions.ConfigureHttpsDefaults(Action{HttpsConnectionAdapterOptions})"/>.
         /// </summary>
         /// <param name="listenOptions">The <see cref="ListenOptions"/> to configure.</param>
-        /// <param name="httpsOptionsCallback">Callback to configure HTTPS options.</param>
-        /// <param name="state">State for the <paramref name="httpsOptionsCallback"/>.</param>
+        /// <param name="serverOptionsSelectionCallback">Callback to configure HTTPS options.</param>
+        /// <param name="state">State for the <paramref name="serverOptionsSelectionCallback"/>.</param>
         /// <returns>The <see cref="ListenOptions"/>.</returns>
-        public static ListenOptions UseHttps(this ListenOptions listenOptions, ServerOptionsSelectionCallback httpsOptionsCallback, object state)
+        public static ListenOptions UseHttps(this ListenOptions listenOptions, ServerOptionsSelectionCallback serverOptionsSelectionCallback, object state)
         {
-            return listenOptions.UseHttps(httpsOptionsCallback, state, HttpsConnectionAdapterOptions.DefaultHandshakeTimeout);
+            return listenOptions.UseHttps(serverOptionsSelectionCallback, state, HttpsConnectionAdapterOptions.DefaultHandshakeTimeout);
         }
 
         /// <summary>
-        /// Configure Kestrel to use HTTPS.
+        /// Configure Kestrel to use HTTPS. This does not use default certificates or other defaults specified via config or
+        /// <see cref="KestrelServerOptions.ConfigureHttpsDefaults(Action{HttpsConnectionAdapterOptions})"/>.
         /// </summary>
         /// <param name="listenOptions">The <see cref="ListenOptions"/> to configure.</param>
-        /// <param name="httpsOptionsCallback">Callback to configure HTTPS options.</param>
-        /// <param name="state">State for the <paramref name="httpsOptionsCallback"/>.</param>
+        /// <param name="serverOptionsSelectionCallback">Callback to configure HTTPS options.</param>
+        /// <param name="state">State for the <paramref name="serverOptionsSelectionCallback"/>.</param>
         /// <param name="handshakeTimeout">Specifies the maximum amount of time allowed for the TLS/SSL handshake. This must be positive and finite.</param>
         /// <returns>The <see cref="ListenOptions"/>.</returns>
-        public static ListenOptions UseHttps(this ListenOptions listenOptions, ServerOptionsSelectionCallback httpsOptionsCallback, object state, TimeSpan handshakeTimeout)
+        public static ListenOptions UseHttps(this ListenOptions listenOptions, ServerOptionsSelectionCallback serverOptionsSelectionCallback, object state, TimeSpan handshakeTimeout)
         {
             // HttpsOptionsCallback is an internal delegate that is just the ServerOptionsSelectionCallback + a ConnectionContext parameter.
             // Given that ConnectionContext will eventually be replaced by System.Net.Connections, it doesn't make much sense to make the HttpsOptionsCallback delegate public.
             HttpsOptionsCallback adaptedCallback = (connection, stream, clientHelloInfo, state, cancellationToken) =>
-                httpsOptionsCallback(stream, clientHelloInfo, state, cancellationToken);
+                serverOptionsSelectionCallback(stream, clientHelloInfo, state, cancellationToken);
 
             return listenOptions.UseHttps(adaptedCallback, state, handshakeTimeout);
         }

diff --git a/src/Servers/Kestrel/samples/SampleApp/Startup.cs b/src/Servers/Kestrel/samples/SampleApp/Startup.cs
@@ -5,6 +5,7 @@
 using System.Diagnostics;
 using System.IO;
 using System.Net;
+using System.Net.Security;
 using System.Security.Authentication;
 using System.Security.Cryptography.X509Certificates;
 using System.Threading.Tasks;
@@ -109,15 +110,21 @@ public static Task Main(string[] args)
 
                             options.ListenAnyIP(basePort + 5, listenOptions =>
                             {
-                                listenOptions.UseHttps(httpsOptions =>
+                                var localhostCert = CertificateLoader.LoadFromStoreCert("localhost", "My", StoreLocation.CurrentUser, allowInvalid: true);
+
+                                listenOptions.UseHttps((stream, clientHelloInfo, state, cancellationToken) =>
                                 {
-                                    var localhostCert = CertificateLoader.LoadFromStoreCert("localhost", "My", StoreLocation.CurrentUser, allowInvalid: true);
-                                    httpsOptions.ServerCertificateSelector = (features, name) =>
+                                    // Here you would check the name, select an appropriate cert, and provide a fallback or fail for null names.
+                                    if (clientHelloInfo.ServerName != null && clientHelloInfo.ServerName != "localhost")
                                     {
-                                        // Here you would check the name, select an appropriate cert, and provide a fallback or fail for null names.
-                                        return localhostCert;
-                                    };
-                                });
+                                        throw new AuthenticationException($"The endpoint is not configured for sever name '{clientHelloInfo.ServerName}'.");
+                                    }
+
+                                    return new ValueTask<SslServerAuthenticationOptions>(new SslServerAuthenticationOptions
+                                    {
+                                        ServerCertificate = localhostCert
+                                    });
+                                }, state: null);
                             });
 
                             options

diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs
@@ -123,6 +123,42 @@ void ConfigureListenOptions(ListenOptions listenOptions)
             }
         }
 
+        [Fact]
+        public async Task HandshakeDetailsAreAvailableAfterAsyncCallback()
+        {
+            void ConfigureListenOptions(ListenOptions listenOptions)
+            {
+                listenOptions.UseHttps(async (stream, clientHelloInfo, state, cancellationToken) =>
+                {
+                    await Task.Yield();
+
+                    return new SslServerAuthenticationOptions
+                    {
+                        ServerCertificate = _x509Certificate2,
+                    };
+                }, state: null);
+            }
+
+            await using (var server = new TestServer(context =>
+            {
+                var tlsFeature = context.Features.Get<ITlsHandshakeFeature>();
+                Assert.NotNull(tlsFeature);
+                Assert.True(tlsFeature.Protocol > SslProtocols.None, "Protocol");
+                Assert.True(tlsFeature.CipherAlgorithm > CipherAlgorithmType.Null, "Cipher");
+                Assert.True(tlsFeature.CipherStrength > 0, "CipherStrength");
+                Assert.True(tlsFeature.HashAlgorithm >= HashAlgorithmType.None, "HashAlgorithm"); // May be None on Linux.
+                Assert.True(tlsFeature.HashStrength >= 0, "HashStrength"); // May be 0 for some algorithms
+                Assert.True(tlsFeature.KeyExchangeAlgorithm >= ExchangeAlgorithmType.None, "KeyExchangeAlgorithm"); // Maybe None on Windows 7
+                Assert.True(tlsFeature.KeyExchangeStrength >= 0, "KeyExchangeStrength"); // May be 0 on mac
+
+                return context.Response.WriteAsync("hello world");
+            }, new TestServiceContext(LoggerFactory), ConfigureListenOptions))
+            {
+                var result = await server.HttpClientSlim.GetStringAsync($"https://localhost:{server.Port}/", validateCertificate: false);
+                Assert.Equal("hello world", result);
+            }
+        }
+
         [Fact]
         public async Task RequireCertificateFailsWhenNoCertificate()
         {
@@ -166,22 +202,18 @@ void ConfigureListenOptions(ListenOptions listenOptions)
         }
 
         [Fact]
-        [QuarantinedTest("https://github.com/dotnet/runtime/issues/40402")]
-        public async Task ClientCertificateRequiredConfiguredInCallbackContinuesWhenNoCertificate()
+        public async Task AsyncCallbackSettingClientCertificateRequiredContinuesWhenNoCertificate()
         {
             void ConfigureListenOptions(ListenOptions listenOptions)
             {
-                listenOptions.UseHttps((connection, stream, clientHelloInfo, state, cancellationToken) =>
+                listenOptions.UseHttps((stream, clientHelloInfo, state, cancellationToken) =>
                     new ValueTask<SslServerAuthenticationOptions>(new SslServerAuthenticationOptions
                     {
                         ServerCertificate = _x509Certificate2,
-                        // From the API Docs: "Note that this is only a request --
-                        // if no certificate is provided, the server still accepts the connection request."
-                        // Not to mention this is equivalent to the test above.
                         ClientCertificateRequired = true,
                         RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true,
                         CertificateRevocationCheckMode = X509RevocationMode.NoCheck
-                    }), state: null, HttpsConnectionAdapterOptions.DefaultHandshakeTimeout);
+                    }), state: null);
             }
 
             await using (var server = new TestServer(context =>
@@ -255,6 +287,39 @@ void ConfigureListenOptions(ListenOptions listenOptions)
             }
         }
 
+        [Fact]
+        public async Task UsesProvidedAsyncCallback()
+        {
+            var selectorCalled = 0;
+            void ConfigureListenOptions(ListenOptions listenOptions)
+            {
+                listenOptions.UseHttps(async (stream, clientHelloInfo, state, cancellationToken) =>
+                {
+                    await Task.Yield();
+
+                    Assert.NotNull(stream);
+                    Assert.Equal("localhost", clientHelloInfo.ServerName);
+                    selectorCalled++;
+
+                    return new SslServerAuthenticationOptions
+                    {
+                        ServerCertificate = _x509Certificate2
+                    };
+                }, state: null);
+            }
+
+            await using (var server = new TestServer(context => Task.CompletedTask, new TestServiceContext(LoggerFactory), ConfigureListenOptions))
+            {
+                using (var connection = server.CreateConnection())
+                {
+                    var stream = OpenSslStream(connection.Stream);
+                    await stream.AuthenticateAsClientAsync("localhost");
+                    Assert.True(stream.RemoteCertificate.Equals(_x509Certificate2));
+                    Assert.Equal(1, selectorCalled);
+                }
+            }
+        }
+
         [Fact]
         public async Task UsesProvidedServerCertificateSelectorEachTime()
         {