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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

HTTP/3 input validation and connection abort #29665

Merged
10 commits merged into from
Mar 17, 2021
Merged

HTTP/3 input validation and connection abort #29665

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
58f0725
Connection error for invalid HTTP/3 frame on request stream
JamesNK Jan 27, 2021
d157b35
Update src/Servers/Kestrel/Core/src/CoreStrings.resx
JamesNK Mar 6, 2021
17a98cd
Write frame types and errors codes in logs as per spec
JamesNK Mar 6, 2021
31373aa
Clean up
JamesNK Mar 6, 2021
ff298ab
Add HTTP/3 connection exception with code
JamesNK Mar 7, 2021
b38e627
Connection abort
JamesNK Mar 7, 2021
36598cd
More abort connection stuff
JamesNK Mar 8, 2021
701c04e
More test
JamesNK Mar 8, 2021
5e5a3f6
More tests
JamesNK Mar 8, 2021
0ceb9fd
PR feedback
JamesNK Mar 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/Servers/Kestrel/Core/src/CoreStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,9 +654,30 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
<value>An error occurred after the response headers were sent, a reset is being sent.</value>
</data>
<data name="Http3StreamErrorDataReceivedBeforeHeaders" xml:space="preserve">
<value>The client sent a DATA frame before the HEADERS frame.</value>
<value>The client sent a DATA frame to a request stream before the HEADERS frame.</value>
</data>
<data name="Http3StreamErrorFrameReceivedAfterTrailers" xml:space="preserve">
<value>The client sent a {frameType} frame after trailing HEADERS.</value>
</data>
</root>
<data name="Http3ErrorUnsupportedFrameOnRequestStream" xml:space="preserve">
<value>The client sent a {frameType} frame to a request stream which isn't supported.</value>
</data>
<data name="Http3ErrorUnsupportedFrameOnServer" xml:space="preserve">
<value>The client sent a {frameType} frame to the server which isn't supported.</value>
</data>
<data name="Http3ErrorUnsupportedFrameOnControlStream" xml:space="preserve">
<value>The client sent a {frameType} frame to a control stream which isn't supported.</value>
</data>
<data name="Http3ErrorControlStreamMultipleSettingsFrames" xml:space="preserve">
<value>The client sent a SETTINGS frame to a control stream that already has settings.</value>
</data>
<data name="Http3ErrorControlStreamFrameReceivedBeforeSettings" xml:space="preserve">
<value>The client sent a {frameType} frame to a control stream before the SETTINGS frame.</value>
</data>
<data name="Http3ErrorControlStreamReservedSetting" xml:space="preserve">
<value>The client sent a reserved setting identifier: {identifier}</value>
</data>
<data name="Http3ControlStreamErrorMultipleInboundStreams" xml:space="preserve">
<value>The client created multiple inbound {streamType} streams for the connection.</value>
</data>
</root>
6 changes: 5 additions & 1 deletion src/Servers/Kestrel/Core/src/Internal/Http3/Frames/Http3RawFrame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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}";
}
}
}
15 changes: 9 additions & 6 deletions src/Servers/Kestrel/Core/src/Internal/Http3/Http3Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -49,6 +49,8 @@ public Http3Connection(Http3ConnectionContext context)
_timeoutControl = new TimeoutControl(this);
_context.TimeoutControl ??= _timeoutControl;

_errorCodeFeature = context.ConnectionFeatures.Get<IProtocolErrorCodeFeature>()!;
Copy link
Contributor

Choose a reason for hiding this comment

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

I hated this approach when I added it. Maybe better to expose an exception type in the transport that has an error code on it? Then when we call abort, the error code will be set.

Copy link
Member Author

Choose a reason for hiding this comment

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

The exception would have to be in connection abstractions so Kestrel can throw it and the QUIC transport can read it.

Looking at where IProtocolErrorCodeFeature is used, ConnectionAbortedException would be a good place to add a long? ProtocolError { get; } property. How come a feature was used instead?

@davidfowl

Copy link
Member

@halter73 halter73 Mar 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I also prefer the protocol error being an argument to abort rather than a property that's set beforehand. What if we made a QuicConnectionAbortedException with a ProtocolError property and do an as check in QuicConnectionContext.Abort(ConnectionAbortedException)? I think I prefer that slightly to just adding the property directly to ConnectionAbortedException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because I didn't want to modify ConnectionAbortedException I believe at the time.


var httpLimits = context.ServiceContext.ServerOptions.Limits;

_serverSettings.HeaderTableSize = (uint)httpLimits.Http3.HeaderTableSize;
Expand Down Expand Up @@ -76,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<TContext>(IHttpApplication<TContext> httpApplication) where TContext : notnull
{
Expand Down Expand Up @@ -163,7 +166,7 @@ private bool TryClose()
return false;
}

public void Abort(ConnectionAbortedException ex)
public void Abort(ConnectionAbortedException ex, Http3ErrorCode errorCode)
{
bool previousState;

Expand All @@ -180,6 +183,7 @@ public void Abort(ConnectionAbortedException ex)
SendGoAway(_highestOpenedStreamId);
}

_errorCodeFeature.Error = (long)errorCode;
_multiplexedContext.Abort(ex);
}
}
Expand Down Expand Up @@ -326,9 +330,8 @@ internal async Task InnerProcessStreamsAsync<TContext>(IHttpApplication<TContext
Log.RequestProcessingError(_context.ConnectionId, ex);
error = ex;
}
catch (Http3ConnectionException ex)
catch (Http3ConnectionErrorException ex)
{
// TODO Connection error code?
Log.Http3ConnectionError(_context.ConnectionId, ex);
error = ex;
}
Expand All @@ -352,7 +355,7 @@ internal async Task InnerProcessStreamsAsync<TContext>(IHttpApplication<TContext

foreach (var stream in _streams.Values)
{
stream.Abort(connectionError);
stream.Abort(connectionError, (Http3ErrorCode)_errorCodeFeature.Error);
}

while (_activeRequestCount > 0)
Expand All @@ -364,7 +367,7 @@ internal async Task InnerProcessStreamsAsync<TContext>(IHttpApplication<TContext
}
catch
{
Abort(connectionError);
Abort(connectionError, Http3ErrorCode.NoError);
throw;
}
}
Expand Down
19 changes: 19 additions & 0 deletions src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionErrorException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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;
using System.Net.Http;

namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http3
{
internal class Http3ConnectionErrorException : Exception
{
public Http3ConnectionErrorException(string message, Http3ErrorCode errorCode)
: base($"HTTP/3 connection error ({errorCode}): {message}")
{
ErrorCode = errorCode;
}

public Http3ErrorCode ErrorCode { get; }
}
}
28 changes: 0 additions & 28 deletions src/Servers/Kestrel/Core/src/Internal/Http3/Http3ConnectionException.cs
View file
Open in desktop

This file was deleted.

Loading