some small proxy-related fixes (#50770)

* add tests

* add and use DoProxyAuth

* don't apply max connections to ProxyConnect connections

* make DoProxyAuth a property

* disable WinHttpHandler for new tests

* improve test robustness
Co-authored-by: NGeoffrey Kizer <geoffrek@windows.microsoft.com>

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Proxy.cs

@@ -320,21 +320,129 @@ public async Task Proxy_SslProxyUnsupported_Throws()
             }
         }
 
-        [OuterLoop("Uses external server")]
         [Fact]
-        public async Task Proxy_SendSecureRequestThruProxy_ConnectTunnelUsed()
+        public async Task ProxyTunnelRequest_GetAsync_Success()
         {
+            if (IsWinHttpHandler)
+            {
+                return;
+            }
+
+            const string Content = "Hello world";
+
             using (LoopbackProxyServer proxyServer = LoopbackProxyServer.Create())
             {
                 HttpClientHandler handler = CreateHttpClientHandler();
                 handler.Proxy = new WebProxy(proxyServer.Uri);
+                handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
                 using (HttpClient client = CreateHttpClient(handler))
-                using (HttpResponseMessage response = await client.GetAsync(Configuration.Http.SecureRemoteEchoServer))
                 {
-                    Assert.Equal(HttpStatusCode.OK, response.StatusCode);
-                    _output.WriteLine($"Proxy request line: {proxyServer.Requests[0].RequestLine}");
-                    Assert.Contains("CONNECT", proxyServer.Requests[0].RequestLine);
+                    var options = new LoopbackServer.Options { UseSsl = true };
+                    await LoopbackServer.CreateServerAsync(async (server, uri) =>
+                    {
+                        Assert.Equal(proxyServer.Uri, handler.Proxy.GetProxy(uri));
+                        
+                        Task<HttpResponseMessage> clientTask = client.GetAsync(uri);
+                        await server.AcceptConnectionSendResponseAndCloseAsync(content: Content);
+                        using (var response = await clientTask)
+                        {
+                            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+                            Assert.Equal(Content, await response.Content.ReadAsStringAsync());
+                        }
+                    }, options);
+                }
+
+                Assert.Contains("CONNECT", proxyServer.Requests[0].RequestLine);
+            }
+        }
+
+        [Fact]
+        public async Task ProxyTunnelRequest_MaxConnectionsSetButDoesNotApplyToProxyConnect_Success()
+        {
+            if (IsWinHttpHandler)
+            {
+                return;
+            }
+
+            const string Content = "Hello world";
+
+            using (LoopbackProxyServer proxyServer = LoopbackProxyServer.Create())
+            {
+                HttpClientHandler handler = CreateHttpClientHandler();
+                handler.Proxy = new WebProxy(proxyServer.Uri);
+                handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
+                handler.MaxConnectionsPerServer = 1;
+                using (HttpClient client = CreateHttpClient(handler))
+                {
+                    var options = new LoopbackServer.Options { UseSsl = true };
+                    await LoopbackServer.CreateServerAsync(async (server1, uri1) =>
+                    {
+                        await LoopbackServer.CreateServerAsync(async (server2, uri2) =>
+                        {
+                            Assert.Equal(proxyServer.Uri, handler.Proxy.GetProxy(uri1));
+                            Assert.Equal(proxyServer.Uri, handler.Proxy.GetProxy(uri2));
+
+                            Task<HttpResponseMessage> clientTask1 = client.GetAsync(uri1);
+                            Task<HttpResponseMessage> clientTask2 = client.GetAsync(uri2);
+                            await server1.AcceptConnectionAsync(async connection1 =>
+                            {
+                                await server2.AcceptConnectionAsync(async connection2 =>
+                                {
+                                    await connection1.HandleRequestAsync(content: Content);
+                                    await connection2.HandleRequestAsync(content: Content);
+                                });
+                            });
+
+                            using (var response1 = await clientTask1)
+                            {
+                                Assert.Equal(HttpStatusCode.OK, response1.StatusCode);
+                                Assert.Equal(Content, await response1.Content.ReadAsStringAsync());
+                            }
+
+                            using (var response2 = await clientTask2)
+                            {
+                                Assert.Equal(HttpStatusCode.OK, response2.StatusCode);
+                                Assert.Equal(Content, await response2.Content.ReadAsStringAsync());
+                            }
+                        }, options);
+                    }, options);
+                }
+
+                Assert.Contains("CONNECT", proxyServer.Requests[0].RequestLine);
+                Assert.Contains("CONNECT", proxyServer.Requests[1].RequestLine);
+            }
+        }
+
+        [Fact]
+        public async Task ProxyTunnelRequest_OriginServerSendsProxyAuthChallenge_NoProxyAuthPerformed()
+        {
+            if (IsWinHttpHandler)
+            {
+                return;
+            }
+
+            using (LoopbackProxyServer proxyServer = LoopbackProxyServer.Create())
+            {
+                HttpClientHandler handler = CreateHttpClientHandler();
+                handler.Proxy = new WebProxy(proxyServer.Uri) { Credentials = ConstructCredentials(new NetworkCredential("username", "password"), proxyServer.Uri, BasicAuth, true) };
+                handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
+                using (HttpClient client = CreateHttpClient(handler))
+                {
+                    var options = new LoopbackServer.Options { UseSsl = true };
+                    await LoopbackServer.CreateServerAsync(async (server, uri) =>
+                    {
+                        Assert.Equal(proxyServer.Uri, handler.Proxy.GetProxy(uri));
+
+                        Task<HttpResponseMessage> clientTask = client.GetAsync(uri);
+                        await server.AcceptConnectionSendResponseAndCloseAsync(statusCode: HttpStatusCode.ProxyAuthenticationRequired, additionalHeaders: "Proxy-Authenticate: Basic");
+                        using (var response = await clientTask)
+                        {
+                            Assert.Equal(HttpStatusCode.ProxyAuthenticationRequired, response.StatusCode);
+                        }
+                    }, options);
                 }
+
+                Assert.Contains("CONNECT", proxyServer.Requests[0].RequestLine);
             }
         }
 
@@ -373,7 +481,6 @@ public async Task ProxyAuth_Digest_Succeeds()
                     Assert.Equal(HttpStatusCode.OK, clientTask.Result.StatusCode);
                 }
             }, options);
-
         }
 
         [Fact]

src/libraries/Common/tests/System/Net/Http/LoopbackProxyServer.cs

@@ -143,7 +143,12 @@ private async Task<bool> ProcessRequest(Socket clientSocket, StreamReader reader
             }
 
             var request = new ReceivedRequest();
-            _requests.Add(request);
+
+            // Avoid concurrent writes to the request list
+            lock (_requests)
+            {
+                _requests.Add(request);
+            }
 
             request.RequestLine = line;
             string[] requestTokens = request.RequestLine.Split(' ');

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

@@ -105,13 +105,12 @@ internal sealed class HttpConnectionPool : IDisposable
         /// <param name="port">The port with which this pool is associated.</param>
         /// <param name="sslHostName">The SSL host with which this pool is associated.</param>
         /// <param name="proxyUri">The proxy this pool targets (optional).</param>
-        /// <param name="maxConnections">The maximum number of connections allowed to be associated with the pool at any given time.</param>
-        public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionKind kind, string? host, int port, string? sslHostName, Uri? proxyUri, int maxConnections)
+        public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionKind kind, string? host, int port, string? sslHostName, Uri? proxyUri)
         {
             _poolManager = poolManager;
             _kind = kind;
             _proxyUri = proxyUri;
-            _maxConnections = maxConnections;
+            _maxConnections = Settings._maxConnectionsPerServer;
 
             if (host != null)
             {
@@ -174,6 +173,11 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK
                     Debug.Assert(sslHostName == null);
                     Debug.Assert(proxyUri != null);
 
+                    // Don't enforce the max connections limit on proxy tunnels; this would mean that connections to different origin servers
+                    // would compete for the same limited number of connections.
+                    // We will still enforce this limit on the user of the tunnel (i.e. ProxyTunnel or SslProxyTunnel).
+                    _maxConnections = int.MaxValue;
+
                     _http2Enabled = false;
                     _http3Enabled = false;
                     break;
@@ -290,7 +294,6 @@ private static SslClientAuthenticationOptions ConstructSslOptions(HttpConnection
         public HttpConnectionSettings Settings => _poolManager.Settings;
         public HttpConnectionKind Kind => _kind;
         public bool IsSecure => _kind == HttpConnectionKind.Https || _kind == HttpConnectionKind.SslProxyTunnel;
-        public bool AnyProxyKind => (_proxyUri != null);
         public Uri? ProxyUri => _proxyUri;
         public ICredentials? ProxyCredentials => _poolManager.ProxyCredentials;
         public byte[]? HostHeaderValueBytes => _hostHeaderValueBytes;
@@ -1149,9 +1152,11 @@ public async Task<HttpResponseMessage> SendWithNtConnectionAuthAsync(HttpConnect
             }
         }
 
+        private bool DoProxyAuth => (_kind == HttpConnectionKind.Proxy || _kind == HttpConnectionKind.ProxyConnect);
+
         public Task<HttpResponseMessage> SendWithNtProxyAuthAsync(HttpConnection connection, HttpRequestMessage request, bool async, CancellationToken cancellationToken)
         {
-            if (AnyProxyKind && ProxyCredentials != null)
+            if (DoProxyAuth && ProxyCredentials is not null)
             {
                 return AuthenticationHelper.SendWithNtProxyAuthAsync(request, ProxyUri!, async, ProxyCredentials, connection, this, cancellationToken);
             }
@@ -1159,13 +1164,11 @@ public Task<HttpResponseMessage> SendWithNtProxyAuthAsync(HttpConnection connect
             return connection.SendAsync(request, async, cancellationToken);
         }
 
-
         public ValueTask<HttpResponseMessage> SendWithProxyAuthAsync(HttpRequestMessage request, bool async, bool doRequestAuth, CancellationToken cancellationToken)
         {
-            if ((_kind == HttpConnectionKind.Proxy || _kind == HttpConnectionKind.ProxyConnect) &&
-                _poolManager.ProxyCredentials != null)
+            if (DoProxyAuth && ProxyCredentials is not null)
             {
-                return AuthenticationHelper.SendWithProxyAuthAsync(request, _proxyUri!, async, _poolManager.ProxyCredentials, doRequestAuth, this, cancellationToken);
+                return AuthenticationHelper.SendWithProxyAuthAsync(request, _proxyUri!, async, ProxyCredentials, doRequestAuth, this, cancellationToken);
             }
 
             return SendWithRetryAsync(request, async, doRequestAuth, cancellationToken);

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs

@@ -39,9 +39,7 @@ internal sealed class HttpConnectionPoolManager : IDisposable
         private readonly Timer? _cleaningTimer;
         /// <summary>Heart beat timer currently used for Http2 ping only.</summary>
         private readonly Timer? _heartBeatTimer;
-        /// <summary>The maximum number of connections allowed per pool. <see cref="int.MaxValue"/> indicates unlimited.</summary>
-        private readonly int _maxConnectionsPerServer;
-        // Temporary
+
         private readonly HttpConnectionSettings _settings;
         private readonly IWebProxy? _proxy;
         private readonly ICredentials? _proxyCredentials;
@@ -60,7 +58,6 @@ internal sealed class HttpConnectionPoolManager : IDisposable
         public HttpConnectionPoolManager(HttpConnectionSettings settings)
         {
             _settings = settings;
-            _maxConnectionsPerServer = settings._maxConnectionsPerServer;
             _pools = new ConcurrentDictionary<HttpConnectionKey, HttpConnectionPool>();
 
             // As an optimization, we can sometimes avoid the overheads associated with
@@ -321,7 +318,7 @@ public ValueTask<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request,
             HttpConnectionPool? pool;
             while (!_pools.TryGetValue(key, out pool))
             {
-                pool = new HttpConnectionPool(this, key.Kind, key.Host, key.Port, key.SslHostName, key.ProxyUri, _maxConnectionsPerServer);
+                pool = new HttpConnectionPool(this, key.Kind, key.Host, key.Port, key.SslHostName, key.ProxyUri);
 
                 if (_cleaningTimer == null)
                 {