Skip to content

make EstablishProxyTunnelAsync throw on failure status code from proxy #50763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 6, 2021
Merged

make EstablishProxyTunnelAsync throw on failure status code from proxy #50763

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a4d2fd
make EstablishProxyTunnelAsync throw on failure status code from prox…
Apr 5, 2021
d6f65c9
ensure tunnelResponse is disposed on exception
Apr 6, 2021
9c24969
Update src/libraries/System.Net.Http/src/Resources/Strings.resx
geoffkizer Apr 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Proxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,8 @@ public async Task AuthenticatedProxiedRequest_GetAsyncWithNoCreds_ProxyAuthentic
}
}

[OuterLoop("Uses external server")]
[Fact]
public async Task AuthenticatedProxyTunnelRequest_PostAsyncWithNoCreds_ProxyAuthenticationRequiredStatusCode()
public async Task AuthenticatedProxyTunnelRequest_PostAsyncWithNoCreds_Throws()
{
if (IsWinHttpHandler)
{
Expand All @@ -300,14 +299,13 @@ public async Task AuthenticatedProxyTunnelRequest_PostAsyncWithNoCreds_ProxyAuth
handler.Proxy = new WebProxy(proxyServer.Uri);
handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
using (HttpClient client = CreateHttpClient(handler))
using (HttpResponseMessage response = await client.PostAsync(Configuration.Http.SecureRemoteEchoServer, new StringContent(content)))
{
Assert.Equal(HttpStatusCode.ProxyAuthenticationRequired, response.StatusCode);
HttpRequestException e = await Assert.ThrowsAnyAsync<HttpRequestException>(async () => await client.PostAsync("https://nosuchhost.invalid", new StringContent(content)));
Assert.Contains("407", e.Message);
}
}
}


[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsSubsystemForLinux))] // [ActiveIssue("https://github.com/dotnet/runtime/issues/18258")]
public async Task Proxy_SslProxyUnsupported_Throws()
{
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Http/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,4 +606,7 @@
<data name="net_http_synchronous_reads_not_supported" xml:space="preserve">
<value>Synchronous reads are not supported, use ReadAsync instead.</value>
</data>
<data name="net_http_proxy_tunnel_returned_failure_status_code" xml:space="preserve">
<value>The proxy tunnel request to proxy '{0} failed with status code '{1}'."</value>
</data>
</root>
10 changes: 1 addition & 9 deletions ...ies/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,7 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe
if (response.Headers.ConnectionClose.GetValueOrDefault())
{
// Server is closing the connection and asking us to authenticate on a new connection.
// expression returns null connection on error, was not able to use '!' for the expression
#pragma warning disable CS8600
(connection, response) = await connectionPool.CreateHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
#pragma warning restore CS8600
if (response != null)
{
return response;
}

connection = await connectionPool.CreateHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
connectionPool.IncrementConnectionCount();
connection!.Acquire();
isNewConnection = true;
Expand Down
106 changes: 36 additions & 70 deletions src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,20 +334,20 @@ public byte[] Http2AltSvcOriginUri
/// <summary>Object used to synchronize access to state in the pool.</summary>
private object SyncObj => _idleConnections;

private ValueTask<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>
private ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
GetConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
{
// Do not even attempt at getting/creating a connection if it's already obvious we cannot provided the one requested.
if (request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower)
{
if (request.Version.Major == 3 && !_http3Enabled)
{
return ValueTask.FromException<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>(
return ValueTask.FromException<(HttpConnectionBase connection, bool isNewConnection)>(
new HttpRequestException(SR.Format(SR.net_http_requested_version_not_enabled, request.Version, request.VersionPolicy, 3)));
}
if (request.Version.Major == 2 && !_http2Enabled)
{
return ValueTask.FromException<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>(
return ValueTask.FromException<(HttpConnectionBase connection, bool isNewConnection)>(
new HttpRequestException(SR.Format(SR.net_http_requested_version_not_enabled, request.Version, request.VersionPolicy, 2)));
}
}
Expand All @@ -365,7 +365,7 @@ public byte[] Http2AltSvcOriginUri
{
if (IsAltSvcBlocked(authority))
{
return ValueTask.FromException<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>(
return ValueTask.FromException<(HttpConnectionBase connection, bool isNewConnection)>(
new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 3)));
}

Expand All @@ -376,7 +376,7 @@ public byte[] Http2AltSvcOriginUri
// If we got here, we cannot provide HTTP/3 connection. Do not continue if downgrade is not allowed.
if (request.Version.Major >= 3 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower)
{
return ValueTask.FromException<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>(
return ValueTask.FromException<(HttpConnectionBase connection, bool isNewConnection)>(
new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 3)));
}

Expand All @@ -389,11 +389,11 @@ public byte[] Http2AltSvcOriginUri
// If we got here, we cannot provide HTTP/2 connection. Do not continue if downgrade is not allowed.
if (request.Version.Major >= 2 && request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower)
{
return ValueTask.FromException<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>(
return ValueTask.FromException<(HttpConnectionBase connection, bool isNewConnection)>(
new HttpRequestException(SR.Format(SR.net_http_requested_version_cannot_establish, request.Version, request.VersionPolicy, 2)));
}

return GetHttpConnectionAsync(request, async, cancellationToken);
return GetHttp11ConnectionAsync(request, async, cancellationToken);
}

private ValueTask<HttpConnection?> GetOrReserveHttp11ConnectionAsync(bool async, CancellationToken cancellationToken)
Expand Down Expand Up @@ -494,27 +494,21 @@ public byte[] Http2AltSvcOriginUri
}
}

private async ValueTask<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>
GetHttpConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
GetHttp11ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
{
HttpConnection? connection = await GetOrReserveHttp11ConnectionAsync(async, cancellationToken).ConfigureAwait(false);
if (connection != null)
{
return (connection, false, null);
return (connection, false);
}

if (NetEventSource.Log.IsEnabled()) Trace("Creating new connection for pool.");

try
{
HttpResponseMessage? failureResponse;
(connection, failureResponse) = await CreateHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
if (connection == null)
{
Debug.Assert(failureResponse != null);
DecrementConnectionCount();
}
return (connection, true, failureResponse);
connection = await CreateHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
return (connection, true);
}
catch
{
Expand All @@ -523,7 +517,7 @@ public byte[] Http2AltSvcOriginUri
}
}

private async ValueTask<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
GetHttp2ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
{
Debug.Assert(_kind == HttpConnectionKind.Https || _kind == HttpConnectionKind.SslProxyTunnel || _kind == HttpConnectionKind.Http);
Expand All @@ -536,7 +530,7 @@ public byte[] Http2AltSvcOriginUri
// Connection exists and it is still good to use.
if (NetEventSource.Log.IsEnabled()) Trace("Using existing HTTP2 connection.");
_usedSinceLastCleanup = true;
return (http2Connection, false, null);
return (http2Connection, false);
}

// Ensure that the connection creation semaphore is created
Expand Down Expand Up @@ -564,7 +558,7 @@ public byte[] Http2AltSvcOriginUri
http2Connection = GetExistingHttp2Connection();
if (http2Connection != null)
{
return (http2Connection, false, null);
return (http2Connection, false);
}

// Recheck if HTTP2 has been disabled by a previous attempt.
Expand All @@ -575,16 +569,9 @@ public byte[] Http2AltSvcOriginUri
Trace("Attempting new HTTP2 connection.");
}

HttpResponseMessage? failureResponse;

(socket, stream, transportContext, failureResponse) =
(socket, stream, transportContext) =
await ConnectAsync(request, async, cancellationToken).ConfigureAwait(false);

if (failureResponse != null)
{
return (null, true, failureResponse);
}

Debug.Assert(stream != null);

sslStream = stream as SslStream;
Expand All @@ -598,7 +585,7 @@ public byte[] Http2AltSvcOriginUri
Trace("New unencrypted HTTP2 connection established.");
}

return (http2Connection, true, null);
return (http2Connection, true);
}

Debug.Assert(sslStream != null);
Expand All @@ -620,7 +607,7 @@ public byte[] Http2AltSvcOriginUri
Trace("New HTTP2 connection established.");
}

return (http2Connection, true, null);
return (http2Connection, true);
}
}
}
Expand Down Expand Up @@ -667,7 +654,7 @@ public byte[] Http2AltSvcOriginUri

if (canUse)
{
return (await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), true, null);
return (await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), true);
}
else
{
Expand All @@ -681,7 +668,7 @@ public byte[] Http2AltSvcOriginUri
}

// If we reach this point, it means we need to fall back to a (new or existing) HTTP/1.1 connection.
return await GetHttpConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
return await GetHttp11ConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
}

private Http2Connection? GetExistingHttp2Connection()
Expand Down Expand Up @@ -732,7 +719,7 @@ private void AddHttp2Connection(Http2Connection newConnection)
}
}

private async ValueTask<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>
private async ValueTask<(HttpConnectionBase connection, bool isNewConnection)>
GetHttp3ConnectionAsync(HttpRequestMessage request, HttpAuthority authority, CancellationToken cancellationToken)
{
Debug.Assert(_kind == HttpConnectionKind.Https);
Expand All @@ -755,7 +742,7 @@ private void AddHttp2Connection(Http2Connection newConnection)
// Connection exists and it is still good to use.
if (NetEventSource.Log.IsEnabled()) Trace("Using existing HTTP3 connection.");
_usedSinceLastCleanup = true;
return (http3Connection, false, null);
return (http3Connection, false);
}
}

Expand Down Expand Up @@ -783,7 +770,7 @@ private void AddHttp2Connection(Http2Connection newConnection)
Trace("Using existing HTTP3 connection.");
}

return (_http3Connection, false, null);
return (_http3Connection, false);
}

if (NetEventSource.Log.IsEnabled())
Expand Down Expand Up @@ -820,7 +807,7 @@ private void AddHttp2Connection(Http2Connection newConnection)
Trace("New HTTP3 connection established.");
}

return (http3Connection, true, null);
return (http3Connection, true);
}
finally
{
Expand All @@ -834,14 +821,7 @@ public async ValueTask<HttpResponseMessage> SendWithRetryAsync(HttpRequestMessag
{
// Loop on connection failures and retry if possible.

(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse) = await GetConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);
if (failureResponse != null)
{
// Proxy tunnel failure; return proxy response
Debug.Assert(isNewConnection);
Debug.Assert(connection == null);
return failureResponse;
}
(HttpConnectionBase connection, bool isNewConnection) = await GetConnectionAsync(request, async, cancellationToken).ConfigureAwait(false);

HttpResponseMessage response;

Expand Down Expand Up @@ -1201,7 +1181,7 @@ public ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool
return SendWithProxyAuthAsync(request, async, doRequestAuth, cancellationToken);
}

private async ValueTask<(Socket?, Stream?, TransportContext?, HttpResponseMessage?)> ConnectAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
private async ValueTask<(Socket?, Stream, TransportContext?)> ConnectAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
{
// If a non-infinite connect timeout has been set, create and use a new CancellationToken that will be canceled
// when either the original token is canceled or a connect timeout occurs.
Expand Down Expand Up @@ -1232,14 +1212,7 @@ public ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool

case HttpConnectionKind.ProxyTunnel:
case HttpConnectionKind.SslProxyTunnel:
HttpResponseMessage? response;
(stream, response) = await EstablishProxyTunnelAsync(async, request.HasHeaders ? request.Headers : null, cancellationToken).ConfigureAwait(false);
if (response != null)
{
// Return non-success response from proxy.
response.RequestMessage = request;
return (null, null, null, response);
}
stream = await EstablishProxyTunnelAsync(async, request.HasHeaders ? request.Headers : null, cancellationToken).ConfigureAwait(false);
break;
}

Expand All @@ -1259,7 +1232,7 @@ public ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool
stream = sslStream;
}

return (socket, stream, transportContext, null);
return (socket, stream, transportContext);
}
finally
{
Expand Down Expand Up @@ -1323,17 +1296,12 @@ public ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool
}
}

internal async ValueTask<(HttpConnection?, HttpResponseMessage?)> CreateHttp11ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
internal async ValueTask<HttpConnection> CreateHttp11ConnectionAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
{
(Socket? socket, Stream? stream, TransportContext? transportContext, HttpResponseMessage? failureResponse) =
(Socket? socket, Stream? stream, TransportContext? transportContext) =
await ConnectAsync(request, async, cancellationToken).ConfigureAwait(false);

if (failureResponse != null)
{
return (null, failureResponse);
}

return (await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false), null);
return await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false);
}

private SslClientAuthenticationOptions GetSslOptionsForRequest(HttpRequestMessage request)
Expand Down Expand Up @@ -1414,11 +1382,10 @@ private async ValueTask<Http2Connection> ConstructHttp2ConnectionAsync(Stream st
return http2Connection;
}


// Returns the established stream or an HttpResponseMessage from the proxy indicating failure.
private async ValueTask<(Stream?, HttpResponseMessage?)> EstablishProxyTunnelAsync(bool async, HttpRequestHeaders? headers, CancellationToken cancellationToken)
private async ValueTask<Stream> EstablishProxyTunnelAsync(bool async, HttpRequestHeaders? headers, CancellationToken cancellationToken)
{
Debug.Assert(_originAuthority != null);

// Send a CONNECT request to the proxy server to establish a tunnel.
HttpRequestMessage tunnelRequest = new HttpRequestMessage(HttpMethod.Connect, _proxyUri);
tunnelRequest.Headers.Host = $"{_originAuthority.IdnHost}:{_originAuthority.Port}"; // This specifies destination host/port to connect to
Expand All @@ -1432,12 +1399,11 @@ private async ValueTask<Http2Connection> ConstructHttp2ConnectionAsync(Stream st

if (tunnelResponse.StatusCode != HttpStatusCode.OK)
{
return (null, tunnelResponse);
tunnelResponse.Dispose();
throw new HttpRequestException(SR.Format(SR.net_http_proxy_tunnel_returned_failure_status_code, _proxyUri, (int)tunnelResponse.StatusCode));
}

Stream stream = tunnelResponse.Content.ReadAsStream(cancellationToken);

return (stream, null);
return tunnelResponse.Content.ReadAsStream(cancellationToken);
Copy link
Member

@stephentoub stephentoub Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this ReadAsStream gets canceled (which I think would only happen if cancellation were already requested when it's called, since it's otherwise synchronous and immediate), do we need to Dispose of tunnelResponse?

Copy link
Contributor Author

@geoffkizer geoffkizer Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should, yeah. I'll add some logic for this.

Copy link
Contributor Author

@geoffkizer geoffkizer Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose technically we should be using the async overload here as well, at least when it is async...

Copy link
Member

@stephentoub stephentoub Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but we'd changed these to use the sync one as a) we own the response content and know its behavior and b) the async one results in an unnecessary allocation. From my perspective, there should never have been a ReadAsStreamAsync... it's never async :)

Copy link
Contributor Author

@geoffkizer geoffkizer Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we obsolete it, then?

(And why does the sync version take a CancellationToken?)

Copy link
Member

@stephentoub stephentoub Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we obsolete it, then?

Everyone uses it. I think it'd be too painful, and in reality it's only negatively impactful if you're hyperoptimizing. I think we have bigger fish to fry... cough...headers...cough.

And why does the sync version take a CancellationToken?

Good question. I'm 99% sure I commented on it being unnecessary. I'd need to look at the code, but if memory serves technically the base implementation could actually do I/O (making the XxAsync version still relevant) if it's not overridden and delegates to the CreateContentReadStream{Async} implementation.

}

/// <summary>Enqueues a waiter to the waiters list.</summary>
Expand Down