[release/7.0] [SignalR] Avoid Http2 when negotiate auth is used (#46013)

* [SignalR] Avoid Http2 when negotiate auth is used * build * fb * Update src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs Co-authored-by: Brennan <[email protected]>
dotnet · Jan 12, 2023 · 25534d3 · 25534d3
1 parent 605e6a3
commit 25534d3
Show file tree

Hide file tree

Showing 10 changed files with 196 additions and 34 deletions.
diff --git a/src/SignalR/SignalR.slnf b/src/SignalR/SignalR.slnf
@@ -33,6 +33,7 @@
       "src\\Security\\Authentication\\Cookies\\src\\Microsoft.AspNetCore.Authentication.Cookies.csproj",
       "src\\Security\\Authentication\\Core\\src\\Microsoft.AspNetCore.Authentication.csproj",
       "src\\Security\\Authentication\\JwtBearer\\src\\Microsoft.AspNetCore.Authentication.JwtBearer.csproj",
+      "src\\Security\\Authentication\\Negotiate\\src\\Microsoft.AspNetCore.Authentication.Negotiate.csproj",
       "src\\Security\\Authorization\\Core\\src\\Microsoft.AspNetCore.Authorization.csproj",
       "src\\Security\\Authorization\\Policy\\src\\Microsoft.AspNetCore.Authorization.Policy.csproj",
       "src\\Servers\\Connections.Abstractions\\src\\Microsoft.AspNetCore.Connections.Abstractions.csproj",

diff --git a/src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs b/src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.cs
@@ -1789,6 +1789,110 @@ public async Task WebSocketsCanConnectOverHttp2()
         Assert.Contains(TestSink.Writes, context => context.Message.Contains("Request finished HTTP/2 CONNECT"));
     }
 
+    [ConditionalTheory]
+    [MemberData(nameof(TransportTypes))]
+    // Negotiate auth on non-windows requires a lot of setup which is out of scope for these tests
+    [OSSkipCondition(OperatingSystems.MacOSX | OperatingSystems.Linux)]
+    public async Task TransportFallsbackFromHttp2WhenUsingCredentials(HttpTransportType httpTransportType)
+    {
+        await using (var server = await StartServer<Startup>(configureKestrelServerOptions: o =>
+        {
+            o.ConfigureEndpointDefaults(o2 =>
+            {
+                o2.Protocols = Server.Kestrel.Core.HttpProtocols.Http1;
+                o2.UseHttps();
+            });
+            o.ConfigureHttpsDefaults(httpsOptions =>
+            {
+                httpsOptions.ServerCertificate = TestCertificateHelper.GetTestCert();
+            });
+        }))
+        {
+            var hubConnection = new HubConnectionBuilder()
+                .WithLoggerFactory(LoggerFactory)
+                .WithUrl(server.Url + "/windowsauthhub", httpTransportType, options =>
+                {
+                    options.HttpMessageHandlerFactory = h =>
+                    {
+                        ((HttpClientHandler)h).ServerCertificateCustomValidationCallback = (_, _, _, _) => true;
+                        return h;
+                    };
+                    options.WebSocketConfiguration = o =>
+                    {
+                        o.RemoteCertificateValidationCallback = (_, _, _, _) => true;
+                        o.HttpVersion = HttpVersion.Version20;
+                    };
+                    options.UseDefaultCredentials = true;
+                })
+                .Build();
+            try
+            {
+                await hubConnection.StartAsync().DefaultTimeout();
+                var echoResponse = await hubConnection.InvokeAsync<string>(nameof(HubWithAuthorization2.Echo), "Foo").DefaultTimeout();
+                Assert.Equal("Foo", echoResponse);
+            }
+            catch (Exception ex)
+            {
+                LoggerFactory.CreateLogger<HubConnectionTests>().LogError(ex, "{ExceptionType} from test", ex.GetType().FullName);
+                throw;
+            }
+            finally
+            {
+                await hubConnection.DisposeAsync().DefaultTimeout();
+            }
+        }
+
+        // Check that HTTP/1.1 was used instead of the configured HTTP/2 since Windows Auth is being used
+        Assert.Contains(TestSink.Writes, context => context.Message.Contains("Request starting HTTP/1.1 POST"));
+        Assert.Contains(TestSink.Writes, context => context.Message.Contains("Request starting HTTP/1.1 GET"));
+        Assert.Contains(TestSink.Writes, context => context.Message.Contains("Request finished HTTP/1.1 GET"));
+    }
+
+    [ConditionalFact]
+    [WebSocketsSupportedCondition]
+    // Negotiate auth on non-windows requires a lot of setup which is out of scope for these tests
+    [OSSkipCondition(OperatingSystems.MacOSX | OperatingSystems.Linux)]
+    public async Task WebSocketsFailsWhenHttp1NotAllowedAndUsingCredentials()
+    {
+        await using (var server = await StartServer<Startup>(context => context.EventId.Name == "ErrorStartingTransport",
+            configureKestrelServerOptions: o =>
+        {
+            o.ConfigureEndpointDefaults(o2 =>
+            {
+                o2.Protocols = Server.Kestrel.Core.HttpProtocols.Http1AndHttp2;
+                o2.UseHttps();
+            });
+            o.ConfigureHttpsDefaults(httpsOptions =>
+            {
+                httpsOptions.ServerCertificate = TestCertificateHelper.GetTestCert();
+            });
+        }))
+        {
+            var hubConnection = new HubConnectionBuilder()
+                .WithLoggerFactory(LoggerFactory)
+                .WithUrl(server.Url + "/windowsauthhub", HttpTransportType.WebSockets, options =>
+                {
+                    options.HttpMessageHandlerFactory = h =>
+                    {
+                        ((HttpClientHandler)h).ServerCertificateCustomValidationCallback = (_, _, _, _) => true;
+                        return h;
+                    };
+                    options.WebSocketConfiguration = o =>
+                    {
+                        o.RemoteCertificateValidationCallback = (_, _, _, _) => true;
+                        o.HttpVersion = HttpVersion.Version20;
+                        o.HttpVersionPolicy = HttpVersionPolicy.RequestVersionExact;
+                    };
+                    options.UseDefaultCredentials = true;
+                })
+                .Build();
+
+            var ex = await Assert.ThrowsAsync<AggregateException>(() => hubConnection.StartAsync().DefaultTimeout());
+            Assert.Contains("Negotiate Authentication doesn't work with HTTP/2 or higher.", ex.Message);
+            await hubConnection.DisposeAsync().DefaultTimeout();
+        }
+    }
+
     [ConditionalFact]
     [WebSocketsSupportedCondition]
     [OSSkipCondition(OperatingSystems.MacOSX, SkipReason = "HTTP/2 over TLS is not supported on macOS due to missing ALPN support.")]

diff --git a/...rp/Client/test/FunctionalTests/Microsoft.AspNetCore.SignalR.Client.FunctionalTests.csproj b/...rp/Client/test/FunctionalTests/Microsoft.AspNetCore.SignalR.Client.FunctionalTests.csproj
@@ -7,6 +7,7 @@
     <ProjectReference Include="$(SignalRTestUtilsProject)" />
 
     <Reference Include="Microsoft.AspNetCore.Authentication.JwtBearer" />
+    <Reference Include="Microsoft.AspNetCore.Authentication.Negotiate" />
     <Reference Include="Microsoft.AspNetCore.Diagnostics" />
     <Reference Include="Microsoft.AspNetCore.Http" />
     <Reference Include="Microsoft.AspNetCore.SignalR.Client" />

diff --git a/src/SignalR/clients/csharp/Client/test/FunctionalTests/Startup.cs b/src/SignalR/clients/csharp/Client/test/FunctionalTests/Startup.cs
@@ -6,6 +6,7 @@
 using System.IO;
 using System.Security.Claims;
 using Microsoft.AspNetCore.Authentication.JwtBearer;
+using Microsoft.AspNetCore.Authentication.Negotiate;
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.DataProtection;
@@ -36,6 +37,11 @@ public void ConfigureServices(IServiceCollection services)
                 policy.AddAuthenticationSchemes(JwtBearerDefaults.AuthenticationScheme);
                 policy.RequireClaim(ClaimTypes.NameIdentifier);
             });
+            options.AddPolicy(NegotiateDefaults.AuthenticationScheme, policy =>
+            {
+                policy.AddAuthenticationSchemes(NegotiateDefaults.AuthenticationScheme);
+                policy.RequireClaim(ClaimTypes.Name);
+            });
         });
 
         services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
@@ -51,6 +57,7 @@ public void ConfigureServices(IServiceCollection services)
                             IssuerSigningKey = SecurityKey
                         };
                 });
+        services.AddAuthentication(NegotiateDefaults.AuthenticationScheme).AddNegotiate();
 
         // Since tests run in parallel, it's possible multiple servers will startup and read files being written by another test
         // Use a unique directory per server to avoid this collision
@@ -85,6 +92,8 @@ public void Configure(IApplicationBuilder app)
             endpoints.MapHub<HubWithAuthorization>("/authorizedhub");
             endpoints.MapHub<HubWithAuthorization2>("/authorizedhub2")
                   .RequireAuthorization(new AuthorizeAttribute(JwtBearerDefaults.AuthenticationScheme));
+            endpoints.MapHub<HubWithAuthorization2>("/windowsauthhub")
+                  .RequireAuthorization(new AuthorizeAttribute(NegotiateDefaults.AuthenticationScheme));
 
             endpoints.MapHub<TestHub>("/default-nowebsockets", options => options.Transports = HttpTransportType.LongPolling | HttpTransportType.ServerSentEvents);
 

diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs
@@ -462,10 +462,6 @@ private async Task<NegotiationResponse> NegotiateAsync(Uri url, HttpClient httpC
 
             using (var request = new HttpRequestMessage(HttpMethod.Post, uri))
             {
-#if NETSTANDARD2_1_OR_GREATER || NET7_0_OR_GREATER
-                // HttpClient gracefully falls back to HTTP/1.1, so it's fine to set the preferred version to a higher version
-                request.Version = HttpVersion.Version20;
-#endif
 #if NET5_0_OR_GREATER
                 request.Options.Set(new HttpRequestOptionsKey<bool>("IsNegotiate"), true);
 #else
@@ -542,6 +538,7 @@ private HttpClient CreateHttpClient()
         HttpMessageHandler httpMessageHandler = httpClientHandler;
 
         var isBrowser = OperatingSystem.IsBrowser();
+        var allowHttp2 = true;
 
         if (_httpConnectionOptions != null)
         {
@@ -579,11 +576,17 @@ private HttpClient CreateHttpClient()
                 if (_httpConnectionOptions.UseDefaultCredentials != null)
                 {
                     httpClientHandler.UseDefaultCredentials = _httpConnectionOptions.UseDefaultCredentials.Value;
+                    // Negotiate Auth isn't supported over HTTP/2 and HttpClient does not gracefully fallback to HTTP/1.1 in that case
+                    // https://github.com/dotnet/runtime/issues/1582
+                    allowHttp2 = !_httpConnectionOptions.UseDefaultCredentials.Value;
                 }
 
                 if (_httpConnectionOptions.Credentials != null)
                 {
                     httpClientHandler.Credentials = _httpConnectionOptions.Credentials;
+                    // Negotiate Auth isn't supported over HTTP/2 and HttpClient does not gracefully fallback to HTTP/1.1 in that case
+                    // https://github.com/dotnet/runtime/issues/1582
+                    allowHttp2 = false;
                 }
             }
 
@@ -604,6 +607,11 @@ private HttpClient CreateHttpClient()
         // Wrap message handler after HttpMessageHandlerFactory to ensure not overridden
         httpMessageHandler = new LoggingHttpMessageHandler(httpMessageHandler, _loggerFactory);
 
+        if (allowHttp2)
+        {
+            httpMessageHandler = new Http2HttpMessageHandler(httpMessageHandler);
+        }
+
         var httpClient = new HttpClient(httpMessageHandler);
         httpClient.Timeout = HttpClientTimeout;
 

diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Http2HttpMessageHandler.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Http2HttpMessageHandler.cs
@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Net;
+using System.Net.Http;
+using System.Runtime.InteropServices;
+using System.Text;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace Microsoft.AspNetCore.Http.Connections.Client.Internal;
+
+internal class Http2HttpMessageHandler : DelegatingHandler
+{
+    public Http2HttpMessageHandler(HttpMessageHandler inner) : base(inner) { }
+
+    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+    {
+#if NETSTANDARD2_1_OR_GREATER || NET7_0_OR_GREATER
+        // Check just in case HttpRequestMessage defaults to 3 or higher for some reason
+        if (request.Version == HttpVersion.Version11)
+        {
+            // HttpClient gracefully falls back to HTTP/1.1,
+            // so it's fine to set the preferred version to a higher version
+            request.Version = HttpVersion.Version20;
+        }
+#endif
+
+        return base.SendAsync(request, cancellationToken);
+    }
+}
diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/LongPollingTransport.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/LongPollingTransport.cs
@@ -50,12 +50,7 @@ public async Task StartAsync(Uri url, TransferFormat transferFormat, Cancellatio
 
         // Make initial long polling request
         // Server uses first long polling request to finish initializing connection and it returns without data
-        var request = new HttpRequestMessage(HttpMethod.Get, url)
-        {
-#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP
-            Version = HttpVersion.Version20,
-#endif
-        };
+        var request = new HttpRequestMessage(HttpMethod.Get, url);
         using (var response = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false))
         {
             response.EnsureSuccessStatusCode();
@@ -153,12 +148,7 @@ private async Task Poll(Uri pollUrl, CancellationToken cancellationToken)
         {
             while (!cancellationToken.IsCancellationRequested)
             {
-                var request = new HttpRequestMessage(HttpMethod.Get, pollUrl)
-                {
-#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP
-                    Version = HttpVersion.Version20,
-#endif
-                };
+                var request = new HttpRequestMessage(HttpMethod.Get, pollUrl);
 
                 HttpResponseMessage response;
 
@@ -237,12 +227,7 @@ private async Task SendDeleteRequest(Uri url)
         try
         {
             Log.SendingDeleteRequest(_logger, url);
-            var request = new HttpRequestMessage(HttpMethod.Delete, url)
-            {
-#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP
-                Version = HttpVersion.Version20,
-#endif
-            };
+            var request = new HttpRequestMessage(HttpMethod.Delete, url);
             var response = await _httpClient.SendAsync(request).ConfigureAwait(false);
 
             if (response.StatusCode == HttpStatusCode.NotFound)

diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/SendUtils.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/SendUtils.cs
@@ -41,11 +41,6 @@ public static async Task SendMessages(Uri sendUrl, IDuplexPipe application, Http
                         // Send them in a single post
                         var request = new HttpRequestMessage(HttpMethod.Post, sendUrl);
 
-#if NETSTANDARD2_1_OR_GREATER || NET7_0_OR_GREATER
-                        // HttpClient gracefully falls back to HTTP/1.1, so it's fine to set the preferred version to a higher version
-                        request.Version = HttpVersion.Version20;
-#endif
-
                         request.Content = new ReadOnlySequenceContent(buffer);
 
                         // ResponseHeadersRead instructs SendAsync to return once headers are read

diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/ServerSentEventsTransport.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/ServerSentEventsTransport.cs
@@ -55,12 +55,7 @@ public async Task StartAsync(Uri url, TransferFormat transferFormat, Cancellatio
 
         Log.StartTransport(_logger, transferFormat);
 
-        var request = new HttpRequestMessage(HttpMethod.Get, url)
-        {
-#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP
-            Version = HttpVersion.Version20,
-#endif
-        };
+        var request = new HttpRequestMessage(HttpMethod.Get, url);
         request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("text/event-stream"));
 
         HttpResponseMessage? response = null;

diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs
@@ -91,6 +91,10 @@ private async ValueTask<WebSocket> DefaultWebSocketFactory(WebSocketConnectionCo
                 }
             }
 
+#if NET7_0_OR_GREATER
+            var allowHttp2 = true;
+#endif
+
             if (!isBrowser)
             {
                 if (context.Options.Cookies != null)
@@ -106,6 +110,11 @@ private async ValueTask<WebSocket> DefaultWebSocketFactory(WebSocketConnectionCo
                 if (context.Options.Credentials != null)
                 {
                     webSocket.Options.Credentials = context.Options.Credentials;
+                    // Negotiate Auth isn't supported over HTTP/2 and HttpClient does not gracefully fallback to HTTP/1.1 in that case
+                    // https://github.com/dotnet/runtime/issues/1582
+#if NET7_0_OR_GREATER
+                    allowHttp2 = false;
+#endif
                 }
 
                 var originalProxy = webSocket.Options.Proxy;
@@ -117,12 +126,20 @@ private async ValueTask<WebSocket> DefaultWebSocketFactory(WebSocketConnectionCo
                 if (context.Options.UseDefaultCredentials != null)
                 {
                     webSocket.Options.UseDefaultCredentials = context.Options.UseDefaultCredentials.Value;
+                    if (context.Options.UseDefaultCredentials.Value)
+                    {
+                        // Negotiate Auth isn't supported over HTTP/2 and HttpClient does not gracefully fallback to HTTP/1.1 in that case
+                        // https://github.com/dotnet/runtime/issues/1582
+#if NET7_0_OR_GREATER
+                        allowHttp2 = false;
+#endif
+                    }
                 }
 
                 context.Options.WebSocketConfiguration?.Invoke(webSocket.Options);
 
 #if NET7_0_OR_GREATER
-                if (webSocket.Options.HttpVersion >= HttpVersion.Version20)
+                if (webSocket.Options.HttpVersion >= HttpVersion.Version20 && allowHttp2)
                 {
                     // Reset options we set on the users' behalf since they are already on the HttpClient that we're passing to ConnectAsync
                     // And ConnectAsync will throw if these options are set on the ClientWebSocketOptions
@@ -148,6 +165,19 @@ private async ValueTask<WebSocket> DefaultWebSocketFactory(WebSocketConnectionCo
                     }
                 }
 
+                if (!allowHttp2 && webSocket.Options.HttpVersion >= HttpVersion.Version20)
+                {
+                    // We shouldn't fallback to HTTP/1.1 if the user explicitly states
+                    if (webSocket.Options.HttpVersionPolicy == HttpVersionPolicy.RequestVersionOrLower)
+                    {
+                        webSocket.Options.HttpVersion = HttpVersion.Version11;
+                    }
+                    else
+                    {
+                        throw new InvalidOperationException("Negotiate Authentication doesn't work with HTTP/2 or higher.");
+                    }
+                }
+
                 static bool IsX509CertificateCollectionEqual(X509CertificateCollection? left, X509CertificateCollection? right)
                 {
                     var leftCount = left?.Count ?? 0;