Skip to content
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.

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

Detect name resolution errors in QuicConnection.ConnectAsync #89095

Closed
antonfirsov opened this issue Jul 18, 2023 · 8 comments · Fixed by #89948
Closed

Detect name resolution errors in QuicConnection.ConnectAsync #89095

antonfirsov opened this issue Jul 18, 2023 · 8 comments · Fixed by #89948
Assignees
Milestone

Comments

@antonfirsov antonfirsov added this to the 8.0.0 milestone Jul 18, 2023
@antonfirsov antonfirsov self-assigned this Jul 18, 2023
@ghost

This comment was marked as off-topic.

@antonfirsov antonfirsov changed the title [HttpRequestError] Map QUIC errors to HttpRequestError if possible [HttpRequestError] Detect NameResolutionError with Quic Aug 2, 2023
@antonfirsov
Copy link
Member Author

antonfirsov commented Aug 2, 2023

Thinking that QuicException.TransportErrorCode would help identifying relevant error cases was a wrong assumption.

The actual problem here, similarly to #89096 is that we can't detect NameResolutionError. If QuicClientConnectionOptions.ClientAuthenticationOptions.TargetHost is different than the actual host, FinishConnectAsync will resolve the addresses, and in case it fails a SocketException(SocketError.HostNotFound) will correctly surface to the user. But this is not the case with HTTP/3 logic where we have host == QuicClientConnectionOptions.ClientAuthenticationOptions.TargetHost. In this case we leave the leave the name resolution to msquic, which will fail with the mysterious error code -2147013895 (0x80072af9) on Windows and QUIC_STATUS_INVALID_PARAMETER on Linux.

Quic repro
DnsEndPoint endPoint = new DnsEndPoint("BadHost", 443);

await Assert.ThrowsAsync<SocketException>(async () => await QuicConnection.ConnectAsync(new QuicClientConnectionOptions()
{
    MaxInboundBidirectionalStreams = 0,
    MaxInboundUnidirectionalStreams = 5,
    IdleTimeout = TimeSpan.FromMinutes(1),
    DefaultStreamErrorCode = 0x10c,
    DefaultCloseErrorCode = 0x100,
    RemoteEndPoint = endPoint,
    ClientAuthenticationOptions = new SslClientAuthenticationOptions()
    {
        TargetHost = "BadHost",
        EnabledSslProtocols = SslProtocols.Tls13,
        ApplicationProtocols = new Collections.Generic.List<SslApplicationProtocol> { SslApplicationProtocol.Http3 }
    }
}));

A robust fix would be to do the name resolution in S.N.Quic code, running a multi-connect attempt similar to Socket.'s DnsConnectAsync. Since there is no multi-connect in msquic anyways (microsoft/msquic#1181), it might be good enough to also take the first resolved address for the host == TargetHost case.

cc @karelz @ManickaP @CarnaViire, not sure how critical is this for 8.0, you may want to punt this.

@ghost
Copy link

ghost commented Aug 2, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

https://github.com/dotnet/runtime/pull/88974/files#r1265843373 needs a follow-up:

https://github.com/antonfirsov/runtime/blob/b54e8c72cb63de3549ae4bff268d88daa628404c/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs#L146

Author: antonfirsov
Assignees: antonfirsov
Labels:

area-System.Net.Http, area-System.Net.Quic

Milestone: 8.0.0

@antonfirsov antonfirsov changed the title [HttpRequestError] Detect NameResolutionError with Quic Detect name resolution errors in QuicConnection.ConnectAsync Aug 2, 2023
@ManickaP
Copy link
Member

ManickaP commented Aug 3, 2023

Seems like dupe of #82404

@antonfirsov
Copy link
Member Author

Sorry, I forgot about that one. In case we want a simple fix for the HttpRequestError problem, I wonder if we can get rid of the host.Equals(options.ClientAuthenticationOptions.TargetHost) special-casing, always resolving the name & setting a QUIC_PARAM_CONN_REMOTE_ADDRESS to the first address? Or do we expect problematic side-effects from doing so? If we are unwilling to do such a change for 8.0, let's close this issue as a dupe of #82404.

@ManickaP
Copy link
Member

ManickaP commented Aug 3, 2023

Yeah, I think we could do that, i.e. remove that part of the condition and do name resolution ourselves. I see no problem with that. @wfurt, just checking if you see any potential issues with that? If not, Anton go ahead.

@antonfirsov antonfirsov self-assigned this Aug 3, 2023
@antonfirsov
Copy link
Member Author

Tentatively re-assigned myself. Will bring it up in todays meeting, and file a PR if we have consensus. (If no, let's close it as a dupe.)

@wfurt
Copy link
Member

wfurt commented Aug 3, 2023

I think that should be ok. It is more work but we do it anyway and it possibly opens avenue to deal better with names that remove to multiple addresses.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 3, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 4, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants