From 58f07255acb7b68bc5e682292df2eb30584e110f Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 27 Jan 2021 19:48:03 +1300 Subject: [PATCH 01/10] Connection error for invalid HTTP/3 frame on request stream --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 6 ++++ .../Http3/Frames/Http3RawFrame.Data.cs | 12 ++++++++ .../src/Internal/Http3/Http3ControlStream.cs | 1 - .../Core/src/Internal/Http3/Http3Stream.cs | 11 +++---- .../Http3/Http3StreamTests.cs | 29 +++++++++++++++++++ .../Http3/Http3TestBase.cs | 17 +++++------ 6 files changed, 61 insertions(+), 15 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index 95dacfea4c3e..909b131a6b32 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -659,4 +659,10 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a {frameType} frame after trailing HEADERS. + + The client sent a {frameType} frame to a request stream which isn't supported. + + + The client sent a {frameType} frame to a the server which isn't supported. + \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs index b852ed8b5ba1..946d303716bb 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs @@ -10,5 +10,17 @@ public void PrepareData() Length = 0; Type = Http3FrameType.Data; } + + public string FormattedType => Type switch + { + Http3FrameType.Data => "DATA", + Http3FrameType.Headers => "HEADERS", + Http3FrameType.CancelPush => "CANCEL_PUSH", + Http3FrameType.Settings => "SETTINGS", + Http3FrameType.PushPromise => "PUSH_PROMISE", + Http3FrameType.GoAway => "GO_AWAY", + Http3FrameType.MaxPushId => "MAX_PUSH_ID", + _ => Type.ToString() + }; } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index d08eb6efa272..981ac6c7131e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -245,7 +245,6 @@ private ValueTask ProcessHttp3ControlStream(in ReadOnlySequence payload) { case Http3FrameType.Data: case Http3FrameType.Headers: - case Http3FrameType.DuplicatePush: case Http3FrameType.PushPromise: throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); case Http3FrameType.Settings: diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 6e65e4b18999..51a366c1efdd 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -446,14 +446,15 @@ private Task ProcessHttp3Stream(IHttpApplication application return ProcessDataFrameAsync(payload); case Http3FrameType.Headers: return ProcessHeadersFrameAsync(application, payload); - // need to be on control stream - case Http3FrameType.DuplicatePush: - case Http3FrameType.PushPromise: case Http3FrameType.Settings: - case Http3FrameType.GoAway: case Http3FrameType.CancelPush: + case Http3FrameType.GoAway: case Http3FrameType.MaxPushId: - throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); + // These frames need to be on a control stream + throw new Http3StreamErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); + case Http3FrameType.PushPromise: + // The server should never receive push promise + throw new Http3StreamErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnServer(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); default: return ProcessUnknownFrameAsync(); } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index b05114e7d204..7f75629743c3 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -1727,5 +1727,34 @@ public async Task TrailersWithoutEndingStream_ErrorAccessingTrailers() var ex = await Assert.ThrowsAsync(() => readTrailersTcs.Task).DefaultTimeout(); Assert.Equal("The request trailers are not available yet. They may not be available until the full request body is read.", ex.Message); } + + [Theory] + [InlineData(nameof(Http3FrameType.MaxPushId))] + [InlineData(nameof(Http3FrameType.Settings))] + [InlineData(nameof(Http3FrameType.CancelPush))] + [InlineData(nameof(Http3FrameType.GoAway))] + public async Task UnexpectedRequestFrame(string frameType) + { + var requestStream = await InitializeConnectionAndStreamsAsync(_echoApplication); + + var frame = new Http3RawFrame(); + frame.Type = Enum.Parse(frameType); + await requestStream.SendFrameAsync(frame, Memory.Empty); + + await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(frame.FormattedType)); + } + + [Theory] + [InlineData(nameof(Http3FrameType.PushPromise))] + public async Task UnexpectedServerFrame(string frameType) + { + var requestStream = await InitializeConnectionAndStreamsAsync(_echoApplication); + + var frame = new Http3RawFrame(); + frame.Type = Enum.Parse(frameType); + await requestStream.SendFrameAsync(frame, Memory.Empty); + + await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnServer(frame.FormattedType)); + } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs index ea4338926c3f..8afc8a7efae2 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs @@ -369,24 +369,23 @@ public async Task SendHeadersAsync(IEnumerable data, bool endStream = false) { - var outputWriter = _pair.Application.Output; var frame = new Http3RawFrame(); frame.PrepareData(); + await SendFrameAsync(frame, data, endStream); + } + + internal async Task SendFrameAsync(Http3RawFrame frame, Memory data, bool endStream = false) + { + var outputWriter = _pair.Application.Output; frame.Length = data.Length; Http3FrameWriter.WriteHeader(frame, outputWriter); await SendAsync(data.Span); From d157b35d7dc8d969c3abf90b97d4353c983ada65 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 7 Mar 2021 09:07:30 +1300 Subject: [PATCH 02/10] Update src/Servers/Kestrel/Core/src/CoreStrings.resx --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index 909b131a6b32..444678641a86 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -663,6 +663,6 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a {frameType} frame to a request stream which isn't supported. - The client sent a {frameType} frame to a the server which isn't supported. + The client sent a {frameType} frame to the server which isn't supported. - \ No newline at end of file + From 17a98cd45f18d5875762ae78a211f0f26f1a61b9 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 7 Mar 2021 10:26:02 +1300 Subject: [PATCH 03/10] Write frame types and errors codes in logs as per spec --- .../Http3/Frames/Http3RawFrame.Data.cs | 12 ----- .../Internal/Http3/Frames/Http3RawFrame.cs | 6 ++- .../src/Internal/Http3/Http3Formatting.cs | 49 +++++++++++++++++++ .../Core/src/Internal/Http3/Http3Stream.cs | 4 +- .../Internal/Infrastructure/KestrelTrace.cs | 27 ++++++---- .../Http3/Http3StreamTests.cs | 25 +++++++--- 6 files changed, 93 insertions(+), 30 deletions(-) create mode 100644 src/Servers/Kestrel/Core/src/Internal/Http3/Http3Formatting.cs diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs index 946d303716bb..b852ed8b5ba1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.Data.cs @@ -10,17 +10,5 @@ public void PrepareData() Length = 0; Type = Http3FrameType.Data; } - - public string FormattedType => Type switch - { - Http3FrameType.Data => "DATA", - Http3FrameType.Headers => "HEADERS", - Http3FrameType.CancelPush => "CANCEL_PUSH", - Http3FrameType.Settings => "SETTINGS", - Http3FrameType.PushPromise => "PUSH_PROMISE", - Http3FrameType.GoAway => "GO_AWAY", - Http3FrameType.MaxPushId => "MAX_PUSH_ID", - _ => Type.ToString() - }; } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.cs index f174f4b3266a..bdf9cacc1cd9 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.cs @@ -1,6 +1,8 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3; + namespace System.Net.Http { internal partial class Http3RawFrame @@ -9,9 +11,11 @@ internal partial class Http3RawFrame public Http3FrameType Type { get; internal set; } + public string FormattedType => Http3Formatting.ToFormattedType(Type); + public override string ToString() { - return $"{Type} Length: {Length}"; + return $"{FormattedType} Length: {Length}"; } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Formatting.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Formatting.cs new file mode 100644 index 000000000000..91ac629805a7 --- /dev/null +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Formatting.cs @@ -0,0 +1,49 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Net.Http; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3 +{ + internal static class Http3Formatting + { + public static string ToFormattedType(Http3FrameType type) + { + return type switch + { + Http3FrameType.Data => "DATA", + Http3FrameType.Headers => "HEADERS", + Http3FrameType.CancelPush => "CANCEL_PUSH", + Http3FrameType.Settings => "SETTINGS", + Http3FrameType.PushPromise => "PUSH_PROMISE", + Http3FrameType.GoAway => "GO_AWAY", + Http3FrameType.MaxPushId => "MAX_PUSH_ID", + _ => type.ToString() + }; + } + + public static string ToFormattedErrorCode(Http3ErrorCode errorCode) + { + return errorCode switch + { + Http3ErrorCode.NoError => "H3_NO_ERROR", + Http3ErrorCode.ProtocolError => "H3_GENERAL_PROTOCOL_ERROR", + Http3ErrorCode.InternalError => "H3_INTERNAL_ERROR", + Http3ErrorCode.StreamCreationError => "H3_STREAM_CREATION_ERROR", + Http3ErrorCode.ClosedCriticalStream => "H3_CLOSED_CRITICAL_STREAM", + Http3ErrorCode.UnexpectedFrame => "H3_FRAME_UNEXPECTED", + Http3ErrorCode.FrameError => "H3_FRAME_ERROR", + Http3ErrorCode.ExcessiveLoad => "H3_EXCESSIVE_LOAD", + Http3ErrorCode.IdError => "H3_ID_ERROR", + Http3ErrorCode.SettingsError => "H3_SETTINGS_ERROR", + Http3ErrorCode.MissingSettings => "H3_MISSING_SETTINGS", + Http3ErrorCode.RequestRejected => "H3_REQUEST_REJECTED", + Http3ErrorCode.RequestCancelled => "H3_REQUEST_CANCELLED", + Http3ErrorCode.RequestIncomplete => "H3_REQUEST_INCOMPLETE", + Http3ErrorCode.ConnectError => "H3_CONNECT_ERROR", + Http3ErrorCode.VersionFallback => "H3_VERSION_FALLBACK", + _ => errorCode.ToString() + }; + } + } +} diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 51a366c1efdd..55c4e8b6bd62 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -472,7 +472,7 @@ private Task ProcessHeadersFrameAsync(IHttpApplication appli // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1 if (_requestHeaderParsingState == RequestHeaderParsingState.Trailers) { - throw new Http3StreamErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3FrameType.Headers), Http3ErrorCode.UnexpectedFrame); + throw new Http3StreamErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Headers)), Http3ErrorCode.UnexpectedFrame); } if (_requestHeaderParsingState == RequestHeaderParsingState.Headers) @@ -522,7 +522,7 @@ private Task ProcessDataFrameAsync(in ReadOnlySequence payload) // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1 if (_requestHeaderParsingState == RequestHeaderParsingState.Trailers) { - throw new Http3StreamErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3FrameType.Data), Http3ErrorCode.UnexpectedFrame); + throw new Http3StreamErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Data))), Http3ErrorCode.UnexpectedFrame); } if (InputRemaining.HasValue) diff --git a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs index 8260d6da80f7..11f553c24863 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs @@ -134,16 +134,16 @@ internal class KestrelTrace : IKestrelTrace LoggerMessage.Define(LogLevel.Debug, new EventId(44, "Http3ConnectionClosed"), @"Connection id ""{ConnectionId}"" is closed. The last processed stream ID was {HighestOpenedStreamId}."); - private static readonly Action _http3StreamAbort = - LoggerMessage.Define(LogLevel.Debug, new EventId(45, "Http3StreamAbort"), + private static readonly Action _http3StreamAbort = + LoggerMessage.Define(LogLevel.Debug, new EventId(45, "Http3StreamAbort"), @"Trace id ""{TraceIdentifier}"": HTTP/3 stream error ""{error}"". An abort is being sent to the stream."); - private static readonly Action _http3FrameReceived = - LoggerMessage.Define(LogLevel.Trace, new EventId(46, "Http3FrameReceived"), + private static readonly Action _http3FrameReceived = + LoggerMessage.Define(LogLevel.Trace, new EventId(46, "Http3FrameReceived"), @"Connection id ""{ConnectionId}"" received {type} frame for stream ID {id} with length {length}."); - private static readonly Action _http3FrameSending = - LoggerMessage.Define(LogLevel.Trace, new EventId(47, "Http3FrameSending"), + private static readonly Action _http3FrameSending = + LoggerMessage.Define(LogLevel.Trace, new EventId(47, "Http3FrameSending"), @"Connection id ""{ConnectionId}"" sending {type} frame for stream ID {id} with length {length}."); protected readonly ILogger _logger; @@ -346,17 +346,26 @@ public void Http3ConnectionClosed(string connectionId, long highestOpenedStreamI public void Http3StreamAbort(string traceIdentifier, Http3ErrorCode error, ConnectionAbortedException abortReason) { - _http3StreamAbort(_logger, traceIdentifier, error, abortReason); + if (_logger.IsEnabled(LogLevel.Debug)) + { + _http3StreamAbort(_logger, traceIdentifier, Http3Formatting.ToFormattedErrorCode(error), abortReason); + } } public void Http3FrameReceived(string connectionId, long streamId, Http3RawFrame frame) { - _http3FrameReceived(_logger, connectionId, frame.Type, streamId, frame.Length, null); + if (_logger.IsEnabled(LogLevel.Trace)) + { + _http3FrameReceived(_logger, connectionId, Http3Formatting.ToFormattedType(frame.Type), streamId, frame.Length, null); + } } public void Http3FrameSending(string connectionId, long streamId, Http3RawFrame frame) { - _http3FrameSending(_logger, connectionId, frame.Type, streamId, frame.Length, null); + if (_logger.IsEnabled(LogLevel.Trace)) + { + _http3FrameSending(_logger, connectionId, Http3Formatting.ToFormattedType(frame.Type), streamId, frame.Length, null); + } } public virtual void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index 7f75629743c3..f8d5ca23079d 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -11,6 +11,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Internal; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3; using Microsoft.AspNetCore.Testing; using Microsoft.Net.Http.Headers; using Xunit; @@ -1541,7 +1542,9 @@ public async Task ResetAfterCompleteAsync_GETWithResponseBodyAndTrailers_ResetsA var decodedTrailers = await requestStream.ExpectHeadersAsync(); Assert.Equal("Custom Value", decodedTrailers["CustomName"]); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.NoError, expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code NoError."); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.NoError, + expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code NoError."); clientTcs.SetResult(0); await appTcs.Task; @@ -1609,7 +1612,9 @@ public async Task ResetAfterCompleteAsync_POSTWithResponseBodyAndTrailers_Reques var decodedTrailers = await requestStream.ExpectHeadersAsync(); Assert.Equal("Custom Value", decodedTrailers["CustomName"]); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.NoError, expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code NoError."); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.NoError, + expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code NoError."); clientTcs.SetResult(0); await appTcs.Task; @@ -1622,7 +1627,9 @@ public async Task DataBeforeHeaders_UnexpectedFrameError() await requestStream.SendDataAsync(Encoding.UTF8.GetBytes("This is invalid.")); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: "The client sent a DATA frame before the HEADERS frame."); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.UnexpectedFrame, + expectedErrorMessage: "The client sent a DATA frame before the HEADERS frame."); } [Fact] @@ -1680,7 +1687,9 @@ public async Task FrameAfterTrailers_UnexpectedFrameError() await requestStream.SendHeadersAsync(trailers, endStream: false); await requestStream.SendDataAsync(Encoding.UTF8.GetBytes("This is invalid.")); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: "The client sent a Data frame after trailing HEADERS."); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.UnexpectedFrame, + expectedErrorMessage: CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Data))); } [Fact] @@ -1741,7 +1750,9 @@ public async Task UnexpectedRequestFrame(string frameType) frame.Type = Enum.Parse(frameType); await requestStream.SendFrameAsync(frame, Memory.Empty); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(frame.FormattedType)); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.UnexpectedFrame, + expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(frame.FormattedType)); } [Theory] @@ -1754,7 +1765,9 @@ public async Task UnexpectedServerFrame(string frameType) frame.Type = Enum.Parse(frameType); await requestStream.SendFrameAsync(frame, Memory.Empty); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnServer(frame.FormattedType)); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.UnexpectedFrame, + expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnServer(frame.FormattedType)); } } } From 31373aa225896c8c42e0471053a76ea0121d9eb0 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 7 Mar 2021 10:33:12 +1300 Subject: [PATCH 04/10] Clean up --- .../src/Internal/Http3/Http3Stream.FeatureCollection.cs | 3 ++- .../InMemory.FunctionalTests/Http3/Http3StreamTests.cs | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.FeatureCollection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.FeatureCollection.cs index 35a0d2e1b383..90cc3b151e43 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.FeatureCollection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.FeatureCollection.cs @@ -58,7 +58,8 @@ IHeaderDictionary IHttpResponseTrailersFeature.Trailers void IHttpResetFeature.Reset(int errorCode) { - var abortReason = new ConnectionAbortedException(CoreStrings.FormatHttp3StreamResetByApplication((Http3ErrorCode)errorCode)); + var message = CoreStrings.FormatHttp3StreamResetByApplication(Http3Formatting.ToFormattedErrorCode((Http3ErrorCode)errorCode)); + var abortReason = new ConnectionAbortedException(message); ApplicationAbort(abortReason, (Http3ErrorCode)errorCode); } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index f8d5ca23079d..cf2d149c9484 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -761,7 +761,9 @@ public async Task ResetStream_ReturnStreamError() var doneWithHeaders = await requestStream.SendHeadersAsync(headers, endStream: true); - await requestStream.WaitForStreamErrorAsync(Http3ErrorCode.RequestCancelled, CoreStrings.FormatHttp3StreamResetByApplication(Http3ErrorCode.RequestCancelled)); + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.RequestCancelled, + CoreStrings.FormatHttp3StreamResetByApplication(Http3Formatting.ToFormattedErrorCode(Http3ErrorCode.RequestCancelled))); } [Fact] @@ -1544,7 +1546,7 @@ public async Task ResetAfterCompleteAsync_GETWithResponseBodyAndTrailers_ResetsA await requestStream.WaitForStreamErrorAsync( Http3ErrorCode.NoError, - expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code NoError."); + expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code H3_NO_ERROR."); clientTcs.SetResult(0); await appTcs.Task; @@ -1614,7 +1616,7 @@ public async Task ResetAfterCompleteAsync_POSTWithResponseBodyAndTrailers_Reques await requestStream.WaitForStreamErrorAsync( Http3ErrorCode.NoError, - expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code NoError."); + expectedErrorMessage: "The HTTP/3 stream was reset by the application with error code H3_NO_ERROR."); clientTcs.SetResult(0); await appTcs.Task; From ff298ab8beb46e13daf6bcc7629505b7f593d614 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 7 Mar 2021 16:55:01 +1300 Subject: [PATCH 05/10] Add HTTP/3 connection exception with code --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 11 +++- .../src/Internal/Http3/Http3Connection.cs | 3 +- .../Http3/Http3ConnectionErrorException.cs | 19 ++++++ .../Http3/Http3ConnectionException.cs | 28 -------- .../src/Internal/Http3/Http3ControlStream.cs | 65 ++++++++++++------- .../Core/src/Internal/Http3/Http3Stream.cs | 19 ++++-- .../Http3/Http3StreamErrorException.cs | 2 +- .../Internal/Infrastructure/IKestrelTrace.cs | 2 +- .../Internal/Infrastructure/KestrelTrace.cs | 2 +- .../perf/Microbenchmarks/Mocks/MockTrace.cs | 2 +- .../shared/test/CompositeKestrelTrace.cs | 2 +- .../Http3/Http3StreamTests.cs | 2 +- 12 files changed, 90 insertions(+), 67 deletions(-) create mode 100644 src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionErrorException.cs delete mode 100644 src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionException.cs diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index 444678641a86..a747a795fd0f 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -654,7 +654,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l An error occurred after the response headers were sent, a reset is being sent. - The client sent a DATA frame before the HEADERS frame. + The client sent a DATA frame to a request stream before the HEADERS frame. The client sent a {frameType} frame after trailing HEADERS. @@ -665,4 +665,13 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a {frameType} frame to the server which isn't supported. + + The client sent a {frameType} frame to a control stream which isn't supported. + + + The client sent a SETTINGS frame to a control stream that already has settings. + + + The client sent a {frameType} frame to a control stream before the SETTINGS frame. + diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 5088438ace97..051885ff7516 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -326,9 +326,8 @@ internal async Task InnerProcessStreamsAsync(IHttpApplication(IHttpApplication appli { if (!_http3Connection.SetInboundControlStream(this)) { - // TODO propagate these errors to connection. - throw new Http3ConnectionException("HTTP_STREAM_CREATION_ERROR"); + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-6.2.1 + throw new Http3ConnectionErrorException("Only one inbound control stream per peer is permitted.", Http3ErrorCode.StreamCreationError); } await HandleControlStream(); @@ -165,7 +165,8 @@ public async Task ProcessRequestAsync(IHttpApplication appli { if (!_http3Connection.SetInboundEncoderStream(this)) { - throw new Http3ConnectionException("HTTP_STREAM_CREATION_ERROR"); + // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-4.2 + throw new Http3ConnectionErrorException("Only one inbound encoder stream per peer is permitted.", Http3ErrorCode.StreamCreationError); } await HandleEncodingDecodingTask(); @@ -174,7 +175,8 @@ public async Task ProcessRequestAsync(IHttpApplication appli { if (!_http3Connection.SetInboundDecoderStream(this)) { - throw new Http3ConnectionException("HTTP_STREAM_CREATION_ERROR"); + // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-4.2 + throw new Http3ConnectionErrorException("Only one inbound decoder stream per peer is permitted.", Http3ErrorCode.StreamCreationError); } await HandleEncodingDecodingTask(); } @@ -238,25 +240,23 @@ private async ValueTask HandleEncodingDecodingTask() private ValueTask ProcessHttp3ControlStream(in ReadOnlySequence payload) { - // Two things: - // settings must be sent as the first frame of each control stream by each peer - // Can't send more than two settings frames. switch (_incomingFrame.Type) { case Http3FrameType.Data: case Http3FrameType.Headers: case Http3FrameType.PushPromise: - throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-7.2 + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnControlStream(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); case Http3FrameType.Settings: return ProcessSettingsFrameAsync(payload); case Http3FrameType.GoAway: - return ProcessGoAwayFrameAsync(payload); + return ProcessGoAwayFrameAsync(); case Http3FrameType.CancelPush: return ProcessCancelPushFrameAsync(); case Http3FrameType.MaxPushId: return ProcessMaxPushIdFrameAsync(); default: - return ProcessUnknownFrameAsync(); + return ProcessUnknownFrameAsync(_incomingFrame.Type); } } @@ -264,7 +264,8 @@ private ValueTask ProcessSettingsFrameAsync(ReadOnlySequence payload) { if (_haveReceivedSettingsFrame) { - throw new Http3ConnectionException("H3_SETTINGS_ERROR"); + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-settings + throw new Http3ConnectionErrorException(CoreStrings.Http3ErrorControlStreamMultipleSettingsFrames, Http3ErrorCode.UnexpectedFrame); } _haveReceivedSettingsFrame = true; @@ -313,39 +314,53 @@ private void ProcessSetting(long id, long value) } } - private ValueTask ProcessGoAwayFrameAsync(ReadOnlySequence payload) + private ValueTask ProcessGoAwayFrameAsync() { - throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); + EnsureSettingsFrame(Http3FrameType.GoAway); + + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-goaway + // PUSH is not implemented so nothing to do. + + // TODO: Double check the connection remains open. + return default; } private ValueTask ProcessCancelPushFrameAsync() { - if (!_haveReceivedSettingsFrame) - { - throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); - } + EnsureSettingsFrame(Http3FrameType.CancelPush); + + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-cancel_push + // PUSH is not implemented so nothing to do. return default; } private ValueTask ProcessMaxPushIdFrameAsync() { - if (!_haveReceivedSettingsFrame) - { - throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); - } + EnsureSettingsFrame(Http3FrameType.MaxPushId); + + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-cancel_push + // PUSH is not implemented so nothing to do. return default; } - private ValueTask ProcessUnknownFrameAsync() + private ValueTask ProcessUnknownFrameAsync(Http3FrameType frameType) + { + EnsureSettingsFrame(frameType); + + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-9 + // Unknown frames must be explicitly ignored. + return default; + } + + private void EnsureSettingsFrame(Http3FrameType frameType) { if (!_haveReceivedSettingsFrame) { - throw new Http3ConnectionException("HTTP_FRAME_UNEXPECTED"); + var message = CoreStrings.FormatHttp3ErrorControlStreamFrameReceivedBeforeSettings(Http3Formatting.ToFormattedType(frameType)); + throw new Http3ConnectionErrorException(message, Http3ErrorCode.MissingSettings); } - - return default; } public void StopProcessingNextRequest() diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 55c4e8b6bd62..3431e52e9d31 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -387,6 +387,12 @@ public async Task ProcessRequestAsync(IHttpApplication appli error = ex; Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); } + catch (Http3ConnectionErrorException ex) + { + // TODO: Abort overall connection + error = ex; + Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); + } catch (Exception ex) { error = ex; @@ -450,11 +456,12 @@ private Task ProcessHttp3Stream(IHttpApplication application case Http3FrameType.CancelPush: case Http3FrameType.GoAway: case Http3FrameType.MaxPushId: + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-7.2.4 // These frames need to be on a control stream - throw new Http3StreamErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); case Http3FrameType.PushPromise: // The server should never receive push promise - throw new Http3StreamErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnServer(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3ErrorUnsupportedFrameOnServer(_incomingFrame.FormattedType), Http3ErrorCode.UnexpectedFrame); default: return ProcessUnknownFrameAsync(); } @@ -462,6 +469,7 @@ private Task ProcessHttp3Stream(IHttpApplication application private Task ProcessUnknownFrameAsync() { + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-9 // Unknown frames must be explicitly ignored. return Task.CompletedTask; } @@ -472,7 +480,7 @@ private Task ProcessHeadersFrameAsync(IHttpApplication appli // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1 if (_requestHeaderParsingState == RequestHeaderParsingState.Trailers) { - throw new Http3StreamErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Headers)), Http3ErrorCode.UnexpectedFrame); + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Headers)), Http3ErrorCode.UnexpectedFrame); } if (_requestHeaderParsingState == RequestHeaderParsingState.Headers) @@ -515,14 +523,15 @@ private Task ProcessDataFrameAsync(in ReadOnlySequence payload) // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1 if (_requestHeaderParsingState == RequestHeaderParsingState.Ready) { - throw new Http3StreamErrorException(CoreStrings.Http3StreamErrorDataReceivedBeforeHeaders, Http3ErrorCode.UnexpectedFrame); + throw new Http3ConnectionErrorException(CoreStrings.Http3StreamErrorDataReceivedBeforeHeaders, Http3ErrorCode.UnexpectedFrame); } // DATA frame after trailing headers is invalid. // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-4.1 if (_requestHeaderParsingState == RequestHeaderParsingState.Trailers) { - throw new Http3StreamErrorException(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Data))), Http3ErrorCode.UnexpectedFrame); + var message = CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(CoreStrings.FormatHttp3StreamErrorFrameReceivedAfterTrailers(Http3Formatting.ToFormattedType(Http3FrameType.Data))); + throw new Http3ConnectionErrorException(message, Http3ErrorCode.UnexpectedFrame); } if (InputRemaining.HasValue) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3StreamErrorException.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3StreamErrorException.cs index 8edccac290bb..6c40b315b39d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3StreamErrorException.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3StreamErrorException.cs @@ -6,7 +6,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3 { - class Http3StreamErrorException : Exception + internal class Http3StreamErrorException : Exception { public Http3StreamErrorException(string message, Http3ErrorCode errorCode) : base($"HTTP/3 stream error ({errorCode}): {message}") diff --git a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/IKestrelTrace.cs b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/IKestrelTrace.cs index 7b0829ab9552..34356e071370 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/IKestrelTrace.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/IKestrelTrace.cs @@ -81,7 +81,7 @@ internal interface IKestrelTrace : ILogger void InvalidResponseHeaderRemoved(); - void Http3ConnectionError(string connectionId, Http3ConnectionException ex); + void Http3ConnectionError(string connectionId, Http3ConnectionErrorException ex); void Http3ConnectionClosing(string connectionId); diff --git a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs index 11f553c24863..82182456580a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs @@ -329,7 +329,7 @@ public void InvalidResponseHeaderRemoved() _invalidResponseHeaderRemoved(_logger, null); } - public void Http3ConnectionError(string connectionId, Http3ConnectionException ex) + public void Http3ConnectionError(string connectionId, Http3ConnectionErrorException ex) { _http3ConnectionError(_logger, connectionId, ex); } diff --git a/src/Servers/Kestrel/perf/Microbenchmarks/Mocks/MockTrace.cs b/src/Servers/Kestrel/perf/Microbenchmarks/Mocks/MockTrace.cs index 887980770300..2528dd7b6fc7 100644 --- a/src/Servers/Kestrel/perf/Microbenchmarks/Mocks/MockTrace.cs +++ b/src/Servers/Kestrel/perf/Microbenchmarks/Mocks/MockTrace.cs @@ -60,7 +60,7 @@ public void Http2FrameReceived(string connectionId, Http2Frame frame) { } public void Http2FrameSending(string connectionId, Http2Frame frame) { } public void Http2MaxConcurrentStreamsReached(string connectionId) { } public void InvalidResponseHeaderRemoved() { } - public void Http3ConnectionError(string connectionId, Http3ConnectionException ex) { } + public void Http3ConnectionError(string connectionId, Http3ConnectionErrorException ex) { } public void Http3ConnectionClosing(string connectionId) { } public void Http3ConnectionClosed(string connectionId, long highestOpenedStreamId) { } public void Http3StreamAbort(string traceIdentifier, Http3ErrorCode error, ConnectionAbortedException abortReason) { } diff --git a/src/Servers/Kestrel/shared/test/CompositeKestrelTrace.cs b/src/Servers/Kestrel/shared/test/CompositeKestrelTrace.cs index 3f5271b4ec0c..ef99df0548c6 100644 --- a/src/Servers/Kestrel/shared/test/CompositeKestrelTrace.cs +++ b/src/Servers/Kestrel/shared/test/CompositeKestrelTrace.cs @@ -245,7 +245,7 @@ public void InvalidResponseHeaderRemoved() _trace2.InvalidResponseHeaderRemoved(); } - public void Http3ConnectionError(string connectionId, Http3ConnectionException ex) + public void Http3ConnectionError(string connectionId, Http3ConnectionErrorException ex) { _trace1.Http3ConnectionError(connectionId, ex); _trace2.Http3ConnectionError(connectionId, ex); diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index cf2d149c9484..2f5b24b2bc09 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -1631,7 +1631,7 @@ public async Task DataBeforeHeaders_UnexpectedFrameError() await requestStream.WaitForStreamErrorAsync( Http3ErrorCode.UnexpectedFrame, - expectedErrorMessage: "The client sent a DATA frame before the HEADERS frame."); + expectedErrorMessage: CoreStrings.Http3StreamErrorDataReceivedBeforeHeaders); } [Fact] From b38e627dec5fe6eb891923a79982590a9638d319 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 7 Mar 2021 22:36:15 +1300 Subject: [PATCH 06/10] Connection abort --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 3 + .../src/Internal/Http3/Http3Connection.cs | 9 ++- .../src/Internal/Http3/Http3ControlStream.cs | 16 +++- .../src/Internal/Http3/Http3PeerSettings.cs | 2 +- .../src/Internal/Http3/Http3SettingType.cs | 7 +- .../Http3/Http3ConnectionTests.cs | 32 ++++++-- .../Http3/Http3TestBase.cs | 74 ++++++++++++++----- 7 files changed, 111 insertions(+), 32 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index a747a795fd0f..f21ffc5e82e2 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -674,4 +674,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a {frameType} frame to a control stream before the SETTINGS frame. + + The client sent a reserved setting {identifier}. + diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 051885ff7516..4c4b478f5abb 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -39,7 +39,7 @@ internal class Http3Connection : ITimeoutHandler private readonly Http3PeerSettings _serverSettings = new Http3PeerSettings(); private readonly StreamCloseAwaitable _streamCompletionAwaitable = new StreamCloseAwaitable(); - + private readonly IProtocolErrorCodeFeature _errorCodeFeature; public Http3Connection(Http3ConnectionContext context) { @@ -49,6 +49,8 @@ public Http3Connection(Http3ConnectionContext context) _timeoutControl = new TimeoutControl(this); _context.TimeoutControl ??= _timeoutControl; + _errorCodeFeature = context.ConnectionFeatures.Get()!; + var httpLimits = context.ServiceContext.ServerOptions.Limits; _serverSettings.HeaderTableSize = (uint)httpLimits.Http3.HeaderTableSize; @@ -163,7 +165,7 @@ private bool TryClose() return false; } - public void Abort(ConnectionAbortedException ex) + public void Abort(ConnectionAbortedException ex, Http3ErrorCode errorCode) { bool previousState; @@ -180,6 +182,7 @@ public void Abort(ConnectionAbortedException ex) SendGoAway(_highestOpenedStreamId); } + _errorCodeFeature.Error = (long)errorCode; _multiplexedContext.Abort(ex); } } @@ -363,7 +366,7 @@ internal async Task InnerProcessStreamsAsync(IHttpApplication GetNonProtocolDefaults() if (MaxRequestHeaderFieldSize != DefaultMaxRequestHeaderFieldSize) { - list.Add(new Http3PeerSetting(Http3SettingType.MaxHeaderListSize, MaxRequestHeaderFieldSize)); + list.Add(new Http3PeerSetting(Http3SettingType.MaxFieldSectionSize, MaxRequestHeaderFieldSize)); } return list; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3SettingType.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3SettingType.cs index c90b2885c215..f7d971d2d0f0 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3SettingType.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3SettingType.cs @@ -5,11 +5,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3 { internal enum Http3SettingType : long { + // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-5 QPackMaxTableCapacity = 0x1, /// - /// SETTINGS_MAX_HEADER_LIST_SIZE, default is unlimited. + /// SETTINGS_MAX_FIELD_SECTION_SIZE, default is unlimited. + /// https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-5 /// - MaxHeaderListSize = 0x6, + MaxFieldSectionSize = 0x6, + // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-5 QPackBlockedStreams = 0x7 } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index ef03fbb55958..dd0d49219e34 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -3,15 +3,11 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Net.Http; using System.Text; using System.Threading.Tasks; using Microsoft.AspNetCore.Connections; -using Microsoft.AspNetCore.Connections.Features; -using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.Features; -using Microsoft.AspNetCore.Testing; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3; using Microsoft.Net.Http.Headers; using Xunit; @@ -27,9 +23,12 @@ public async Task GoAwayReceived() var outboundcontrolStream = await CreateControlStream(); var inboundControlStream = await GetInboundControlStream(); - Connection.Abort(new ConnectionAbortedException()); + Connection.Abort(new ConnectionAbortedException(), Http3ErrorCode.NoError); await _closedStateReached.Task.DefaultTimeout(); - await WaitForConnectionErrorAsync(ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, expectedErrorCode: 0); + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: true, + expectedLastStreamId: 0, + expectedErrorCode: Http3ErrorCode.NoError); } [Fact] @@ -83,5 +82,24 @@ public async Task GracefulServerShutdownSendsGoawayClosesConnection() MultiplexedConnectionContext.ConnectionClosingCts.Cancel(); Assert.Null(await MultiplexedConnectionContext.AcceptAsync().DefaultTimeout()); } + + [Fact] + public async Task SETTINGS_ReservedSettingSent_ConnectionError() + { + await InitializeConnectionAsync(_echoApplication); + + var outboundcontrolStream = await CreateControlStream(); + await outboundcontrolStream.SendSettingsAsync(new List + { + new Http3PeerSetting(0x0, 0) // reserved value + }); + + await GetInboundControlStream(); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: true, + expectedLastStreamId: 0, + expectedErrorCode: Http3ErrorCode.SettingsError); + } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs index 8afc8a7efae2..5490274bc255 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs @@ -149,10 +149,12 @@ internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAwayFrames, long } } - VerifyGoAway(frame, expectedLastStreamId, expectedErrorCode); + Assert.Equal((long)expectedErrorCode, MultiplexedConnectionContext.Error); + + VerifyGoAway(frame, expectedLastStreamId); } - internal void VerifyGoAway(Http3FrameWithPayload frame, long expectedLastStreamId, Http3ErrorCode expectedErrorCode) + internal void VerifyGoAway(Http3FrameWithPayload frame, long expectedLastStreamId) { Assert.Equal(Http3FrameType.GoAway, frame.Type); var payload = frame.Payload; @@ -329,6 +331,19 @@ internal async Task ReceiveFrameAsync() } } } + + internal async Task SendFrameAsync(Http3RawFrame frame, Memory data, bool endStream = false) + { + var outputWriter = _pair.Application.Output; + frame.Length = data.Length; + Http3FrameWriter.WriteHeader(frame, outputWriter); + await SendAsync(data.Span); + + if (endStream) + { + await _pair.Application.Output.CompleteAsync(); + } + } } internal class Http3RequestStream : Http3StreamBase, IHttpHeadersHandler, IProtocolErrorCodeFeature @@ -383,19 +398,6 @@ internal async Task SendDataAsync(Memory data, bool endStream = false) await SendFrameAsync(frame, data, endStream); } - internal async Task SendFrameAsync(Http3RawFrame frame, Memory data, bool endStream = false) - { - var outputWriter = _pair.Application.Output; - frame.Length = data.Length; - Http3FrameWriter.WriteHeader(frame, outputWriter); - await SendAsync(data.Span); - - if (endStream) - { - await _pair.Application.Output.CompleteAsync(); - } - } - internal async Task> ExpectHeadersAsync() { var http3WithPayload = await ReceiveFrameAsync(); @@ -483,7 +485,7 @@ public Http3ControlStream(Http3TestBase testBase) var inputPipeOptions = GetInputPipeOptions(_testBase._serviceContext, _testBase._memoryPool, PipeScheduler.ThreadPool); var outputPipeOptions = GetOutputPipeOptions(_testBase._serviceContext, _testBase._memoryPool, PipeScheduler.ThreadPool); _pair = DuplexPipe.CreateConnectionPair(inputPipeOptions, outputPipeOptions); - StreamContext = new TestStreamContext(canRead: false, canWrite: true, _pair, this); + StreamContext = new TestStreamContext(canRead: true, canWrite: false, _pair, this); } public Http3ControlStream(ConnectionContext streamContext) @@ -507,6 +509,41 @@ void WriteSpan(PipeWriter pw) await FlushAsync(writableBuffer); } + internal async Task SendSettingsAsync(List settings, bool endStream = false) + { + var frame = new Http3RawFrame(); + frame.PrepareSettings(); + + var settingsLength = CalculateSettingsSize(settings); + var buffer = new byte[settingsLength]; + WriteSettings(settings, buffer); + + await SendFrameAsync(frame, buffer, endStream); + } + + internal static int CalculateSettingsSize(List settings) + { + var length = 0; + foreach (var setting in settings) + { + length += VariableLengthIntegerHelper.GetByteCount((long)setting.Parameter); + length += VariableLengthIntegerHelper.GetByteCount(setting.Value); + } + return length; + } + + internal static void WriteSettings(List settings, Span destination) + { + foreach (var setting in settings) + { + var parameterLength = VariableLengthIntegerHelper.WriteInteger(destination, (long)setting.Parameter); + destination = destination.Slice(parameterLength); + + var valueLength = VariableLengthIntegerHelper.WriteInteger(destination, (long)setting.Value); + destination = destination.Slice(valueLength); + } + } + public async ValueTask TryReadStreamIdAsync() { while (true) @@ -540,7 +577,7 @@ public async ValueTask TryReadStreamIdAsync() } } - public class TestMultiplexedConnectionContext : MultiplexedConnectionContext, IConnectionLifetimeNotificationFeature, IConnectionLifetimeFeature, IConnectionHeartbeatFeature + public class TestMultiplexedConnectionContext : MultiplexedConnectionContext, IConnectionLifetimeNotificationFeature, IConnectionLifetimeFeature, IConnectionHeartbeatFeature, IProtocolErrorCodeFeature { public readonly Channel ToServerAcceptQueue = Channel.CreateUnbounded(new UnboundedChannelOptions { @@ -562,6 +599,7 @@ public TestMultiplexedConnectionContext(Http3TestBase testBase) Features = new FeatureCollection(); Features.Set(this); Features.Set(this); + Features.Set(this); ConnectionClosedRequested = ConnectionClosingCts.Token; } @@ -575,6 +613,8 @@ public TestMultiplexedConnectionContext(Http3TestBase testBase) public CancellationTokenSource ConnectionClosingCts { get; set; } = new CancellationTokenSource(); + public long Error { get; set; } + public override void Abort() { Abort(new ConnectionAbortedException()); From 36598cdad8c31937f9c55f18e2996b1d3a1e3a26 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Mon, 8 Mar 2021 13:19:37 +1300 Subject: [PATCH 07/10] More abort connection stuff --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 4 +-- .../src/Internal/Http3/Http3Connection.cs | 3 +- .../Http3/Http3ConnectionErrorException.cs | 2 +- .../src/Internal/Http3/Http3ControlStream.cs | 5 +++ .../Core/src/Internal/Http3/Http3Stream.cs | 8 +++-- .../Http3/Http3ConnectionTests.cs | 33 +++++++------------ .../Http3/Http3StreamTests.cs | 6 ++++ .../Http3/Http3TestBase.cs | 31 +++++++++++++---- 8 files changed, 59 insertions(+), 33 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index f21ffc5e82e2..dfecd940d27a 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -675,6 +675,6 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a {frameType} frame to a control stream before the SETTINGS frame. - The client sent a reserved setting {identifier}. + The client sent a reserved setting identifier: {identifier} - + \ No newline at end of file diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs index 4c4b478f5abb..343637bd41e1 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs @@ -78,6 +78,7 @@ internal long HighestStreamId public Http3ControlStream? ControlStream { get; set; } public Http3ControlStream? EncoderStream { get; set; } public Http3ControlStream? DecoderStream { get; set; } + public string ConnectionId => _context.ConnectionId; public async Task ProcessStreamsAsync(IHttpApplication httpApplication) where TContext : notnull { @@ -354,7 +355,7 @@ internal async Task InnerProcessStreamsAsync(IHttpApplication 0) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionErrorException.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionErrorException.cs index 8f2161880ca0..c7a63a926d12 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionErrorException.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionErrorException.cs @@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3 internal class Http3ConnectionErrorException : Exception { public Http3ConnectionErrorException(string message, Http3ErrorCode errorCode) - : base($"HTTP/3 stream error ({errorCode}): {message}") + : base($"HTTP/3 connection error ({errorCode}): {message}") { ErrorCode = errorCode; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index 7d4fa9d05520..395bfdeb2482 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -28,6 +28,7 @@ internal abstract class Http3ControlStream : IThreadPoolWorkItem private readonly Http3StreamContext _context; private readonly Http3PeerSettings _serverPeerSettings; private readonly IStreamIdFeature _streamIdFeature; + private readonly IProtocolErrorCodeFeature _protocolErrorCodeFeature; private readonly Http3RawFrame _incomingFrame = new Http3RawFrame(); private volatile int _isClosed; private int _gracefulCloseInitiator; @@ -41,6 +42,7 @@ public Http3ControlStream(Http3Connection http3Connection, Http3StreamContext co _context = context; _serverPeerSettings = context.ServerSettings; _streamIdFeature = context.ConnectionFeatures.Get()!; + _protocolErrorCodeFeature = context.ConnectionFeatures.Get()!; _frameWriter = new Http3FrameWriter( context.Transport.Output, @@ -217,6 +219,9 @@ private async Task HandleControlStream() } catch (Http3ConnectionErrorException ex) { + _protocolErrorCodeFeature.Error = (long)ex.ErrorCode; + + Log.Http3ConnectionError(_http3Connection.ConnectionId, ex); _http3Connection.Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); } finally diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 3431e52e9d31..fa269864302c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -389,9 +389,13 @@ public async Task ProcessRequestAsync(IHttpApplication appli } catch (Http3ConnectionErrorException ex) { - // TODO: Abort overall connection error = ex; - Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); + _errorCodeFeature.Error = (long)ex.ErrorCode; + + Log.Http3ConnectionError(_http3Connection.ConnectionId, ex); + _http3Connection.Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); + + // TODO: HTTP/3 stream will be aborted by connection. Check this is correct. } catch (Exception ex) { diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index dd0d49219e34..aff5a0c5a731 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Net.Http; using System.Text; using System.Threading.Tasks; @@ -15,22 +16,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { public class Http3ConnectionTests : Http3TestBase { - [Fact] - public async Task GoAwayReceived() - { - await InitializeConnectionAsync(_echoApplication); - - var outboundcontrolStream = await CreateControlStream(); - var inboundControlStream = await GetInboundControlStream(); - - Connection.Abort(new ConnectionAbortedException(), Http3ErrorCode.NoError); - await _closedStateReached.Task.DefaultTimeout(); - await WaitForConnectionErrorAsync( - ignoreNonGoAwayFrames: true, - expectedLastStreamId: 0, - expectedErrorCode: Http3ErrorCode.NoError); - } - [Fact] public async Task CreateRequestStream_RequestCompleted_Disposed() { @@ -83,23 +68,29 @@ public async Task GracefulServerShutdownSendsGoawayClosesConnection() Assert.Null(await MultiplexedConnectionContext.AcceptAsync().DefaultTimeout()); } - [Fact] - public async Task SETTINGS_ReservedSettingSent_ConnectionError() + [Theory] + [InlineData(0x0)] + [InlineData(0x2)] + [InlineData(0x3)] + [InlineData(0x4)] + [InlineData(0x5)] + public async Task SETTINGS_ReservedSettingSent_ConnectionError(long settingIdentifier) { await InitializeConnectionAsync(_echoApplication); var outboundcontrolStream = await CreateControlStream(); await outboundcontrolStream.SendSettingsAsync(new List { - new Http3PeerSetting(0x0, 0) // reserved value + new Http3PeerSetting((Internal.Http3.Http3SettingType) settingIdentifier, 0) // reserved value }); await GetInboundControlStream(); - await WaitForConnectionErrorAsync( + await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, - expectedErrorCode: Http3ErrorCode.SettingsError); + expectedErrorCode: Http3ErrorCode.SettingsError, + expectedErrorMessage: $"HTTP/3 connection error (SettingsError): The client sent a reserved setting identifier: 0x{settingIdentifier.ToString("X", CultureInfo.InvariantCulture)}"); } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index 2f5b24b2bc09..a7fce40ae41f 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -1755,6 +1755,12 @@ public async Task UnexpectedRequestFrame(string frameType) await requestStream.WaitForStreamErrorAsync( Http3ErrorCode.UnexpectedFrame, expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(frame.FormattedType)); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: true, + expectedLastStreamId: 0, + expectedErrorCode: Http3ErrorCode.UnexpectedFrame, + expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnRequestStream(frame.FormattedType)); } [Theory] diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs index 5490274bc255..a3a287d07911 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs @@ -137,7 +137,8 @@ internal async ValueTask GetInboundControlStream() return null; } - internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAwayFrames, long expectedLastStreamId, Http3ErrorCode expectedErrorCode) + internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAwayFrames, long expectedLastStreamId, Http3ErrorCode expectedErrorCode, params string[] expectedErrorMessage) + where TException : Exception { var frame = await _inboundControlStream.ReceiveFrameAsync(); @@ -149,9 +150,16 @@ internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAwayFrames, long } } + VerifyGoAway(frame, expectedLastStreamId); + Assert.Equal((long)expectedErrorCode, MultiplexedConnectionContext.Error); - VerifyGoAway(frame, expectedLastStreamId); + if (expectedErrorMessage?.Length > 0) + { + var message = Assert.Single(LogMessages, m => m.Exception is TException); + + Assert.Contains(expectedErrorMessage, expected => message.Exception.Message.Contains(expected)); + } } internal void VerifyGoAway(Http3FrameWithPayload frame, long expectedLastStreamId) @@ -181,6 +189,8 @@ internal async ValueTask InitializeConnectionAndStreamsAsync OutboundControlStream = await CreateControlStream(); + await GetInboundControlStream(); + return await CreateRequestStream(); } @@ -447,12 +457,21 @@ internal async Task WaitForStreamErrorAsync(Http3ErrorCode protocolError, string _testBase.Logger.LogTrace("Input is completed"); Assert.True(readResult.IsCompleted); - Assert.Equal((long)protocolError, Error); - - if (expectedErrorMessage != null) + //try { - Assert.Contains(_testBase.LogMessages, m => m.Exception?.Message.Contains(expectedErrorMessage) ?? false); + Assert.Equal(protocolError, (Http3ErrorCode)Error); + + if (expectedErrorMessage != null) + { + Assert.Contains(_testBase.LogMessages, m => m.Exception?.Message.Contains(expectedErrorMessage) ?? false); + } } + //catch (Exception ex) + //{ + // Debugger.Launch(); + // string s = ex.Message; + // throw; + //} } } From 701c04ef2160b6fbbaf81d2ca26341d910f34081 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Mon, 8 Mar 2021 13:51:07 +1300 Subject: [PATCH 08/10] More test --- src/Servers/Kestrel/Core/src/CoreStrings.resx | 5 +- .../src/Internal/Http3/Http3ControlStream.cs | 62 +++++++++++-------- .../Http3/Http3ConnectionTests.cs | 20 +++++- .../Http3/Http3TestBase.cs | 37 ++++++----- 4 files changed, 80 insertions(+), 44 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/CoreStrings.resx b/src/Servers/Kestrel/Core/src/CoreStrings.resx index dfecd940d27a..0951807eede5 100644 --- a/src/Servers/Kestrel/Core/src/CoreStrings.resx +++ b/src/Servers/Kestrel/Core/src/CoreStrings.resx @@ -677,4 +677,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The client sent a reserved setting identifier: {identifier} - \ No newline at end of file + + The client created multiple inbound {streamType} streams for the connection. + + diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs index 395bfdeb2482..39dce967bde6 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3ControlStream.cs @@ -147,45 +147,53 @@ private async ValueTask TryReadStreamIdAsync() public async Task ProcessRequestAsync(IHttpApplication application) where TContext : notnull { - var streamType = await TryReadStreamIdAsync(); - - if (streamType == -1) + try { - return; - } + var streamType = await TryReadStreamIdAsync(); - if (streamType == ControlStream) - { - if (!_http3Connection.SetInboundControlStream(this)) + if (streamType == -1) { - // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-6.2.1 - throw new Http3ConnectionErrorException("Only one inbound control stream per peer is permitted.", Http3ErrorCode.StreamCreationError); + return; } - await HandleControlStream(); - } - else if (streamType == EncoderStream) - { - if (!_http3Connection.SetInboundEncoderStream(this)) + if (streamType == ControlStream) { - // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-4.2 - throw new Http3ConnectionErrorException("Only one inbound encoder stream per peer is permitted.", Http3ErrorCode.StreamCreationError); + if (!_http3Connection.SetInboundControlStream(this)) + { + // https://quicwg.org/base-drafts/draft-ietf-quic-http.html#section-6.2.1 + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3ControlStreamErrorMultipleInboundStreams("control"), Http3ErrorCode.StreamCreationError); + } + + await HandleControlStream(); } + else if (streamType == EncoderStream) + { + if (!_http3Connection.SetInboundEncoderStream(this)) + { + // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-4.2 + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3ControlStreamErrorMultipleInboundStreams("encoder"), Http3ErrorCode.StreamCreationError); + } - await HandleEncodingDecodingTask(); - } - else if (streamType == DecoderStream) - { - if (!_http3Connection.SetInboundDecoderStream(this)) + await HandleEncodingDecodingTask(); + } + else if (streamType == DecoderStream) + { + if (!_http3Connection.SetInboundDecoderStream(this)) + { + // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-4.2 + throw new Http3ConnectionErrorException(CoreStrings.FormatHttp3ControlStreamErrorMultipleInboundStreams("decoder"), Http3ErrorCode.StreamCreationError); + } + await HandleEncodingDecodingTask(); + } + else { - // https://quicwg.org/base-drafts/draft-ietf-quic-qpack.html#section-4.2 - throw new Http3ConnectionErrorException("Only one inbound decoder stream per peer is permitted.", Http3ErrorCode.StreamCreationError); + // TODO Close the control stream as it's unexpected. } - await HandleEncodingDecodingTask(); } - else + catch (Http3ConnectionErrorException ex) { - // TODO Close the control stream as it's unexpected. + Log.Http3ConnectionError(_http3Connection.ConnectionId, ex); + _http3Connection.Abort(new ConnectionAbortedException(ex.Message, ex), ex.ErrorCode); } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index aff5a0c5a731..83aee7d0a529 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -90,7 +90,25 @@ await WaitForConnectionErrorAsync( ignoreNonGoAwayFrames: true, expectedLastStreamId: 0, expectedErrorCode: Http3ErrorCode.SettingsError, - expectedErrorMessage: $"HTTP/3 connection error (SettingsError): The client sent a reserved setting identifier: 0x{settingIdentifier.ToString("X", CultureInfo.InvariantCulture)}"); + expectedErrorMessage: CoreStrings.FormatHttp3ErrorControlStreamReservedSetting($"0x{settingIdentifier.ToString("X", CultureInfo.InvariantCulture)}")); + } + + [Theory] + [InlineData(0, "control")] + [InlineData(2, "encoder")] + [InlineData(3, "decoder")] + public async Task InboundStreams_CreateMultiple_ConnectionError(int streamId, string name) + { + await InitializeConnectionAsync(_noopApplication); + + await CreateControlStream(streamId); + await CreateControlStream(streamId); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: true, + expectedLastStreamId: 0, + expectedErrorCode: Http3ErrorCode.StreamCreationError, + expectedErrorMessage: CoreStrings.FormatHttp3ControlStreamErrorMultipleInboundStreams(name)); } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs index a3a287d07911..672ac5fbd3e8 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs @@ -121,20 +121,27 @@ internal async ValueTask GetInboundControlStream() { if (_inboundControlStream == null) { - var reader = MultiplexedConnectionContext.ToClientAcceptQueue.Reader; - while (await reader.WaitToReadAsync()) + return await CreateNewInboundControlStream(); + } + + return null; + } + + internal async ValueTask CreateNewInboundControlStream() + { + var reader = MultiplexedConnectionContext.ToClientAcceptQueue.Reader; + while (await reader.WaitToReadAsync()) + { + while (reader.TryRead(out var stream)) { - while (reader.TryRead(out var stream)) - { - _inboundControlStream = stream; - var streamId = await stream.TryReadStreamIdAsync(); - Debug.Assert(streamId == 0, "StreamId sent that was non-zero, which isn't handled by tests"); - return _inboundControlStream; - } + _inboundControlStream = stream; + var streamId = await stream.TryReadStreamIdAsync(); + Debug.Assert(streamId == 0, "StreamId sent that was non-zero, which isn't handled by tests"); + return _inboundControlStream; } - } - - return null; + } + + throw new InvalidOperationException("Should never reach here."); } internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAwayFrames, long expectedLastStreamId, Http3ErrorCode expectedErrorCode, params string[] expectedErrorMessage) @@ -152,7 +159,7 @@ internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAway VerifyGoAway(frame, expectedLastStreamId); - Assert.Equal((long)expectedErrorCode, MultiplexedConnectionContext.Error); + Assert.Equal((Http3ErrorCode)expectedErrorCode, (Http3ErrorCode)MultiplexedConnectionContext.Error); if (expectedErrorMessage?.Length > 0) { @@ -180,6 +187,8 @@ protected async Task InitializeConnectionAsync(RequestDelegate application) // Skip all heartbeat and lifetime notification feature registrations. _connectionTask = Connection.ProcessStreamsAsync(new DummyApplication(application)); + await GetInboundControlStream(); + await Task.CompletedTask; } @@ -189,8 +198,6 @@ internal async ValueTask InitializeConnectionAndStreamsAsync OutboundControlStream = await CreateControlStream(); - await GetInboundControlStream(); - return await CreateRequestStream(); } From 5e5a3f6af465e5498b930a13b406026c3c39d052 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Mon, 8 Mar 2021 14:13:18 +1300 Subject: [PATCH 09/10] More tests --- .../Http3/Http3ConnectionTests.cs | 21 +++++++++ .../Http3/Http3TestBase.cs | 44 ++++++------------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs index 83aee7d0a529..19bf3cb2e936 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3ConnectionTests.cs @@ -110,5 +110,26 @@ await WaitForConnectionErrorAsync( expectedErrorCode: Http3ErrorCode.StreamCreationError, expectedErrorMessage: CoreStrings.FormatHttp3ControlStreamErrorMultipleInboundStreams(name)); } + + [Theory] + [InlineData(nameof(Http3FrameType.Data))] + [InlineData(nameof(Http3FrameType.Headers))] + [InlineData(nameof(Http3FrameType.PushPromise))] + public async Task ControlStream_UnexpectedFrameType_ConnectionError(string frameType) + { + await InitializeConnectionAsync(_noopApplication); + + var controlStream = await CreateControlStream(); + + var frame = new Http3RawFrame(); + frame.Type = Enum.Parse(frameType); + await controlStream.SendFrameAsync(frame, Memory.Empty); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: true, + expectedLastStreamId: 0, + expectedErrorCode: Http3ErrorCode.UnexpectedFrame, + expectedErrorMessage: CoreStrings.FormatHttp3ErrorUnsupportedFrameOnControlStream(Http3Formatting.ToFormattedType(frame.Type))); + } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs index 672ac5fbd3e8..f909a4f29a9f 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs @@ -121,27 +121,20 @@ internal async ValueTask GetInboundControlStream() { if (_inboundControlStream == null) { - return await CreateNewInboundControlStream(); - } - - return null; - } - - internal async ValueTask CreateNewInboundControlStream() - { - var reader = MultiplexedConnectionContext.ToClientAcceptQueue.Reader; - while (await reader.WaitToReadAsync()) - { - while (reader.TryRead(out var stream)) + var reader = MultiplexedConnectionContext.ToClientAcceptQueue.Reader; + while (await reader.WaitToReadAsync()) { - _inboundControlStream = stream; - var streamId = await stream.TryReadStreamIdAsync(); - Debug.Assert(streamId == 0, "StreamId sent that was non-zero, which isn't handled by tests"); - return _inboundControlStream; + while (reader.TryRead(out var stream)) + { + _inboundControlStream = stream; + var streamId = await stream.TryReadStreamIdAsync(); + Debug.Assert(streamId == 0, "StreamId sent that was non-zero, which isn't handled by tests"); + return _inboundControlStream; + } } } - throw new InvalidOperationException("Should never reach here."); + return null; } internal async Task WaitForConnectionErrorAsync(bool ignoreNonGoAwayFrames, long expectedLastStreamId, Http3ErrorCode expectedErrorCode, params string[] expectedErrorMessage) @@ -464,21 +457,12 @@ internal async Task WaitForStreamErrorAsync(Http3ErrorCode protocolError, string _testBase.Logger.LogTrace("Input is completed"); Assert.True(readResult.IsCompleted); - //try - { - Assert.Equal(protocolError, (Http3ErrorCode)Error); + Assert.Equal(protocolError, (Http3ErrorCode)Error); - if (expectedErrorMessage != null) - { - Assert.Contains(_testBase.LogMessages, m => m.Exception?.Message.Contains(expectedErrorMessage) ?? false); - } + if (expectedErrorMessage != null) + { + Assert.Contains(_testBase.LogMessages, m => m.Exception?.Message.Contains(expectedErrorMessage) ?? false); } - //catch (Exception ex) - //{ - // Debugger.Launch(); - // string s = ex.Message; - // throw; - //} } } From 0ceb9fd1d9661eca512123da8c5717baa0befe91 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 17 Mar 2021 15:20:02 +1300 Subject: [PATCH 10/10] PR feedback --- .../test/InMemory.FunctionalTests/Http3/Http3TestBase.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs index f909a4f29a9f..44ce3ac90d39 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3TestBase.cs @@ -389,13 +389,11 @@ public Http3RequestStream(Http3TestBase testBase, Http3Connection connection) public async Task SendHeadersAsync(IEnumerable> headers, bool endStream = false) { - var outputWriter = _pair.Application.Output; var frame = new Http3RawFrame(); frame.PrepareHeaders(); var buffer = _headerEncodingBuffer.AsMemory(); var done = _qpackEncoder.BeginEncode(headers, buffer.Span, out var length); - // TODO may want to modify behavior of input frames to mock different client behavior (client can send anything). await SendFrameAsync(frame, buffer.Slice(0, length), endStream); return done;