Clone Windows Identity in LongPolling connections (#2985)

Author: BrennanConroy

build/dependencies.props

@@ -69,6 +69,7 @@
     <SystemReactiveLinqPackageVersion>3.1.1</SystemReactiveLinqPackageVersion>
     <SystemReflectionEmitPackageVersion>4.3.0</SystemReflectionEmitPackageVersion>
     <SystemRuntimeCompilerServicesUnsafePackageVersion>4.5.1</SystemRuntimeCompilerServicesUnsafePackageVersion>
+    <SystemSecurityPrincipalWindowsPackageVersion>4.5.0</SystemSecurityPrincipalWindowsPackageVersion>
     <SystemThreadingChannelsPackageVersion>4.5.0</SystemThreadingChannelsPackageVersion>
     <SystemThreadingTasksExtensionsPackageVersion>4.5.1</SystemThreadingTasksExtensionsPackageVersion>
     <XunitAssertPackageVersion>2.3.1</XunitAssertPackageVersion>

src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionContext.cs

@@ -6,6 +6,7 @@
 using System.Collections.Generic;
 using System.IO.Pipelines;
 using System.Security.Claims;
+using System.Security.Principal;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
@@ -208,6 +209,14 @@ public async Task DisposeAsync(bool closeGracefully = false)
                 Cancellation?.Dispose();
 
                 Cancellation = null;
+
+                if (User != null && User.Identity is WindowsIdentity)
+                {
+                    foreach (var identity in User.Identities)
+                    {
+                        (identity as IDisposable)?.Dispose();
+                    }
+                }
             }
 
             await disposeTask;

src/Microsoft.AspNetCore.Http.Connections/Internal/HttpConnectionDispatcher.cs

@@ -7,11 +7,11 @@
 using System.Diagnostics;
 using System.IO;
 using System.IO.Pipelines;
+using System.Security.Claims;
+using System.Security.Principal;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
-using Microsoft.AspNetCore.Connections.Features;
-using Microsoft.AspNetCore.Http.Connections.Features;
 using Microsoft.AspNetCore.Http.Connections.Internal.Transports;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Internal;
@@ -608,9 +608,6 @@ private async Task<bool> EnsureConnectionStateAsync(HttpConnectionContext connec
                 return false;
             }
 
-            // Setup the connection state from the http context
-            connection.User = context.User;
-
             // Configure transport-specific features.
             if (transportType == HttpTransportType.LongPolling)
             {
@@ -630,21 +627,49 @@ private async Task<bool> EnsureConnectionStateAsync(HttpConnectionContext connec
                 {
                     // Set the request trace identifier to the current http request handling the poll
                     existing.TraceIdentifier = context.TraceIdentifier;
-                    existing.User = context.User;
+
+                    // Don't copy the identity if it's a windows identity
+                    // We specifically clone the identity on first poll if it's a windows identity
+                    // If we swapped the new User here we'd have to dispose the old identities which could race with the application
+                    // trying to access the identity.
+                    if (context.User.Identity is WindowsIdentity)
+                    {
+                        existing.User = context.User;
+                    }
                 }
             }
             else
             {
                 connection.HttpContext = context;
             }
 
+            // Setup the connection state from the http context
+            connection.User = connection.HttpContext.User;
+
             // Set the Connection ID on the logging scope so that logs from now on will have the
             // Connection ID metadata set.
             logScope.ConnectionId = connection.ConnectionId;
 
             return true;
         }
 
+        private static void CloneUser(HttpContext newContext, HttpContext oldContext)
+        {
+            if (oldContext.User.Identity is WindowsIdentity)
+            {
+                newContext.User = new ClaimsPrincipal();
+
+                foreach (var identity in oldContext.User.Identities)
+                {
+                    newContext.User.AddIdentity(identity.Clone());
+                }
+            }
+            else
+            {
+                newContext.User = oldContext.User;
+            }
+        }
+
         private static HttpContext CloneHttpContext(HttpContext context)
         {
             // The reason we're copying the base features instead of the HttpContext properties is
@@ -692,7 +717,8 @@ private static HttpContext CloneHttpContext(HttpContext context)
 
             var newHttpContext = new DefaultHttpContext(features);
             newHttpContext.TraceIdentifier = context.TraceIdentifier;
-            newHttpContext.User = context.User;
+
+            CloneUser(newHttpContext, context);
 
             // Making request services function property could be tricky and expensive as it would require
             // DI scope per connection. It would also mean that services resolved in middleware leading up to here

src/Microsoft.AspNetCore.Http.Connections/Microsoft.AspNetCore.Http.Connections.csproj

@@ -29,6 +29,7 @@
     <PackageReference Include="Microsoft.Extensions.WebEncoders.Sources" Version="$(MicrosoftExtensionsWebEncodersSourcesPackageVersion)" PrivateAssets="All" />
     <PackageReference Include="Microsoft.Extensions.ValueStopwatch.Sources" Version="$(MicrosoftExtensionsValueStopwatchSourcesPackageVersion)" PrivateAssets="All" />
     <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonPackageVersion)" />
+    <PackageReference Include="System.Security.Principal.Windows" Version="$(SystemSecurityPrincipalWindowsPackageVersion)" />
   </ItemGroup>
 
 </Project>