From 6008034da4a89dc4b734d1184535bfa572cd41f9 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 3 Aug 2023 16:57:06 +0200 Subject: [PATCH 1/6] map TryAgain to NameResolutionError --- .../System/Net/Http/SocketsHttpHandler/ConnectHelper.cs | 8 ++++++-- .../tests/FunctionalTests/SocketsHttpHandlerTest.cs | 6 ++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs index 8bf0f39e0f63a9..d8d43a4d250ea1 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs @@ -143,13 +143,17 @@ internal static Exception CreateWrappedException(Exception exception, string hos static HttpRequestError DeduceError(Exception exception) { - // TODO: Deduce quic errors from QuicException.TransportErrorCode once https://github.com/dotnet/runtime/issues/87262 is implemented. if (exception is AuthenticationException) { return HttpRequestError.SecureConnectionError; } - if (exception is SocketException socketException && socketException.SocketErrorCode == SocketError.HostNotFound) + // Resolving a non-existent hostname often leads to EAI_AGAIN/TryAgain on Linux, indicating a non-authoritative failure, eg. timeout. + // Getting EAGAIN/TryAgain from a TCP connect() is not possible on Windows or Mac according to the docs and indicates lack of kernel resources on Linux, + // which should be a very rare error in practice. As a result, mapping SocketError.TryAgain to HttpRequestError.NameResolutionError + // leads to a more reliable distinction between NameResolutionError and ConnectionError. + if (exception is SocketException socketException && + socketException.SocketErrorCode is SocketError.HostNotFound or SocketError.TryAgain) { return HttpRequestError.NameResolutionError; } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index e07a193c52eb06..279882b5cb153c 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -4376,10 +4376,8 @@ public async Task NameResolutionError() }; HttpRequestException ex = await Assert.ThrowsAsync(() => client.SendAsync(message)); - - // TODO: Some platforms fail to detect NameResolutionError reliably, we should investigate this. - // Also, System.Net.Quic does not report DNS resolution errors yet. - Assert.True(ex.HttpRequestError is HttpRequestError.NameResolutionError or HttpRequestError.ConnectionError); + Assert.Equal(HttpRequestError.NameResolutionError, ex.HttpRequestError); + Assert.IsType(ex.InnerException); } [Fact] From 53a810dc180e6996bc3edb26f22a2ccbd7875b67 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 3 Aug 2023 17:27:12 +0200 Subject: [PATCH 2/6] Quic: always resolve DNS in managed code --- .../src/System/Net/Quic/QuicConnection.cs | 45 ++++++------------- .../FunctionalTests/QuicConnectionTests.cs | 17 ++++--- .../tests/FunctionalTests/QuicTestBase.cs | 11 ++--- 3 files changed, 30 insertions(+), 43 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index f8964b517e7820..66823000bae4e1 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -258,47 +258,30 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, { throw new ArgumentException(SR.Format(SR.net_quic_unsupported_endpoint_type, options.RemoteEndPoint.GetType()), nameof(options)); } - int addressFamily = QUIC_ADDRESS_FAMILY_UNSPEC; - // RemoteEndPoint is either IPEndPoint or DnsEndPoint containing IPAddress string. - // --> Set the IP directly, no name resolution needed. - if (address is not null) + if (address is null) { - QuicAddr quicAddress = new IPEndPoint(address, port).ToQuicAddr(); - MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_REMOTE_ADDRESS, quicAddress); - } - // RemoteEndPoint is DnsEndPoint containing hostname that is different from requested SNI. - // --> Resolve the hostname and set the IP directly, use requested SNI in ConnectionStart. - else if (host is not null && - !host.Equals(options.ClientAuthenticationOptions.TargetHost, StringComparison.OrdinalIgnoreCase)) - { - IPAddress[] addresses = await Dns.GetHostAddressesAsync(host!, cancellationToken).ConfigureAwait(false); + Debug.Assert(host is not null); + + // Given just a ServerName to connect to, msquic would also use the first address after the resolution + // (https://github.com/microsoft/msquic/issues/1181) and it would not return a well-known error code + // for resolution failures we could rely on. By doing the resolution in managed code, we can guarantee + // that a SocketException will surface to the user if the name resolution fails. + IPAddress[] addresses = await Dns.GetHostAddressesAsync(host, cancellationToken).ConfigureAwait(false); cancellationToken.ThrowIfCancellationRequested(); if (addresses.Length == 0) { throw new SocketException((int)SocketError.HostNotFound); } - - QuicAddr quicAddress = new IPEndPoint(addresses[0], port).ToQuicAddr(); - MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_REMOTE_ADDRESS, quicAddress); - } - // RemoteEndPoint is DnsEndPoint containing hostname that is the same as the requested SNI. - // --> Let MsQuic resolve the hostname/SNI, give address family hint is specified in DnsEndPoint. - else - { - if (options.RemoteEndPoint.AddressFamily == AddressFamily.InterNetwork) - { - addressFamily = QUIC_ADDRESS_FAMILY_INET; - } - if (options.RemoteEndPoint.AddressFamily == AddressFamily.InterNetworkV6) - { - addressFamily = QUIC_ADDRESS_FAMILY_INET6; - } + address = addresses[0]; } + QuicAddr quicAddress = new IPEndPoint(address, port).ToQuicAddr(); + MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_REMOTE_ADDRESS, quicAddress); + if (options.LocalEndPoint is not null) { - QuicAddr quicAddress = options.LocalEndPoint.ToQuicAddr(); + quicAddress = options.LocalEndPoint.ToQuicAddr(); MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_LOCAL_ADDRESS, quicAddress); } @@ -326,7 +309,7 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ConnectionStart( _handle, _configuration, - (ushort)addressFamily, + (ushort)quicAddress.Family, (sbyte*)targetHostPtr, (ushort)port), "ConnectionStart failed"); diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index e633ae41026276..7276047cb7df25 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -18,10 +18,12 @@ public sealed class QuicConnectionTests : QuicTestBase public QuicConnectionTests(ITestOutputHelper output) : base(output) { } - [Fact] - public async Task TestConnect() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task TestConnect(bool ipv6) { - await using QuicListener listener = await CreateQuicListener(); + await using QuicListener listener = await CreateQuicListener(ipv6 ? IPAddress.IPv6Loopback : IPAddress.Loopback); var options = CreateQuicClientOptions(listener.LocalEndPoint); ValueTask connectTask = CreateQuicConnection(options); @@ -369,9 +371,10 @@ await Task.WhenAll( }).WaitAsync(TimeSpan.FromSeconds(5))); } - [Fact] - [OuterLoop("Uses external servers")] - public async Task ConnectAsync_InvalidName_ThrowsSocketException() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task ConnectAsync_InvalidName_ThrowsSocketException(bool sameTargetHost) { string name = $"{Guid.NewGuid().ToString("N")}.microsoft.com."; var options = new QuicClientConnectionOptions() @@ -379,7 +382,7 @@ public async Task ConnectAsync_InvalidName_ThrowsSocketException() DefaultStreamErrorCode = DefaultStreamErrorCodeClient, DefaultCloseErrorCode = DefaultCloseErrorCodeClient, RemoteEndPoint = new DnsEndPoint(name, 10000), - ClientAuthenticationOptions = GetSslClientAuthenticationOptions() + ClientAuthenticationOptions = GetSslClientAuthenticationOptions(sameTargetHost ? name : "localhost") }; SocketException ex = await Assert.ThrowsAsync(() => QuicConnection.ConnectAsync(options).AsTask()); diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs index 149ac5bc2671e3..b6d7cd6c17e6e5 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs @@ -108,13 +108,13 @@ public SslServerAuthenticationOptions GetSslServerAuthenticationOptions() }; } - public SslClientAuthenticationOptions GetSslClientAuthenticationOptions() + public SslClientAuthenticationOptions GetSslClientAuthenticationOptions(string targetHost = "localhost") { return new SslClientAuthenticationOptions() { ApplicationProtocols = new List() { ApplicationProtocol }, RemoteCertificateValidationCallback = RemoteCertificateValidationCallback, - TargetHost = "localhost" + TargetHost = targetHost }; } @@ -140,8 +140,9 @@ internal ValueTask CreateQuicConnection(QuicClientConnectionOpti return QuicConnection.ConnectAsync(clientOptions); } - internal QuicListenerOptions CreateQuicListenerOptions() + internal QuicListenerOptions CreateQuicListenerOptions(IPAddress address = null) { + address ??= IPAddress.Loopback; return new QuicListenerOptions() { ListenEndPoint = new IPEndPoint(IPAddress.Loopback, 0), @@ -150,9 +151,9 @@ internal QuicListenerOptions CreateQuicListenerOptions() }; } - internal ValueTask CreateQuicListener(int MaxInboundUnidirectionalStreams = 100, int MaxInboundBidirectionalStreams = 100) + internal ValueTask CreateQuicListener(IPAddress address = null) { - var options = CreateQuicListenerOptions(); + var options = CreateQuicListenerOptions(address); return CreateQuicListener(options); } From 639174beb58da1795943b5aca0214f2be261487d Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 3 Aug 2023 19:16:29 +0200 Subject: [PATCH 3/6] oops --- .../System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs index b6d7cd6c17e6e5..7388fcbeda8040 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestBase.cs @@ -145,7 +145,7 @@ internal QuicListenerOptions CreateQuicListenerOptions(IPAddress address = null) address ??= IPAddress.Loopback; return new QuicListenerOptions() { - ListenEndPoint = new IPEndPoint(IPAddress.Loopback, 0), + ListenEndPoint = new IPEndPoint(address, 0), ApplicationProtocols = new List() { ApplicationProtocol }, ConnectionOptionsCallback = (_, _, _) => ValueTask.FromResult(CreateQuicServerOptions()) }; From 6eebbda8ff71b61d58e17348c6cc42705fe86aa9 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 3 Aug 2023 19:30:49 +0200 Subject: [PATCH 4/6] ignore ScopId when comparing ipv6 endpoints in tests --- .../FunctionalTests/QuicConnectionTests.cs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index 7276047cb7df25..6d04d3df863d9a 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Net.Sockets; using System.Security.Cryptography.X509Certificates; using System.Threading; @@ -33,15 +35,27 @@ public async Task TestConnect(bool ipv6) await using QuicConnection serverConnection = acceptTask.Result; await using QuicConnection clientConnection = connectTask.Result; - Assert.Equal(listener.LocalEndPoint, serverConnection.LocalEndPoint); - Assert.Equal(listener.LocalEndPoint, clientConnection.RemoteEndPoint); - Assert.Equal(clientConnection.LocalEndPoint, serverConnection.RemoteEndPoint); + IgnoreScopeIdIPEndpointComparer endPointComparer = new(); + Assert.Equal(listener.LocalEndPoint, serverConnection.LocalEndPoint, endPointComparer); + Assert.Equal(listener.LocalEndPoint, clientConnection.RemoteEndPoint, endPointComparer); + Assert.Equal(clientConnection.LocalEndPoint, serverConnection.RemoteEndPoint, endPointComparer); Assert.Equal(ApplicationProtocol.ToString(), clientConnection.NegotiatedApplicationProtocol.ToString()); Assert.Equal(ApplicationProtocol.ToString(), serverConnection.NegotiatedApplicationProtocol.ToString()); Assert.Equal(options.ClientAuthenticationOptions.TargetHost, clientConnection.TargetHostName); Assert.Equal(options.ClientAuthenticationOptions.TargetHost, serverConnection.TargetHostName); } + private class IgnoreScopeIdIPEndpointComparer : IEqualityComparer + { + public bool Equals(IPEndPoint x, IPEndPoint y) + { + byte[] xBytes = x.Address.GetAddressBytes(); + byte[] yBytes = y.Address.GetAddressBytes(); + return xBytes.AsSpan().SequenceEqual(yBytes) && x.Port == y.Port; + } + public int GetHashCode([DisallowNull] IPEndPoint obj) => obj.Port; + } + private static async Task OpenAndUseStreamAsync(QuicConnection c) { QuicStream s = await c.OpenOutboundStreamAsync(QuicStreamType.Bidirectional); From 11488d8025c8e48221c34d66356dd4115bf2ac59 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 3 Aug 2023 19:44:24 +0200 Subject: [PATCH 5/6] do not reuse QuicAddr --- .../src/System/Net/Quic/QuicConnection.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 66823000bae4e1..14de40dcdca8a0 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -276,13 +276,13 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, address = addresses[0]; } - QuicAddr quicAddress = new IPEndPoint(address, port).ToQuicAddr(); - MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_REMOTE_ADDRESS, quicAddress); + QuicAddr remoteQuicAddress = new IPEndPoint(address, port).ToQuicAddr(); + MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_REMOTE_ADDRESS, remoteQuicAddress); if (options.LocalEndPoint is not null) { - quicAddress = options.LocalEndPoint.ToQuicAddr(); - MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_LOCAL_ADDRESS, quicAddress); + QuicAddr localQuicAddress = options.LocalEndPoint.ToQuicAddr(); + MsQuicHelpers.SetMsQuicParameter(_handle, QUIC_PARAM_CONN_LOCAL_ADDRESS, localQuicAddress); } // RFC 6066 forbids IP literals @@ -309,7 +309,7 @@ private async ValueTask FinishConnectAsync(QuicClientConnectionOptions options, ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.ConnectionStart( _handle, _configuration, - (ushort)quicAddress.Family, + (ushort)remoteQuicAddress.Family, (sbyte*)targetHostPtr, (ushort)port), "ConnectionStart failed"); From 6859a7562c85289c7fcf99cc2b507613e05ab185 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Thu, 3 Aug 2023 23:21:02 +0200 Subject: [PATCH 6/6] Skip NameResolutionError() test on Windows7 --- .../tests/FunctionalTests/SocketsHttpHandlerTest.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 279882b5cb153c..b7dfcdce83c25b 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -4365,7 +4365,8 @@ protected SocketsHttpHandler_HttpRequestErrorTest(ITestOutputHelper output) : ba { } - [Fact] + // On Windows7 DNS may return SocketError.NoData (WSANO_DATA), which we currently don't map to NameResolutionError. + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] public async Task NameResolutionError() { using HttpClient client = CreateHttpClient();