Skip to content

Commit 77fe36f

Browse files
KahbaziTratcherhalter73
authored
Remove connection-specific headeres for HTTP/2 and HTTP/3 (#24543)
* Remove connection-specific headeres for HTTP/2 and HTTP/3 * Log * Consolidate check * Apply suggestions from code review Co-authored-by: Stephen Halter <[email protected]> * Update src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs * Update src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs * Update src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs Co-authored-by: Chris R <[email protected]> Co-authored-by: Stephen Halter <[email protected]>
1 parent 3a942ff commit 77fe36f

File tree

11 files changed

+544
-272
lines changed

11 files changed

+544
-272
lines changed

‎src/Http/Headers/src/HeaderNames.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ public static class HeaderNames
6565
public static readonly string Pragma = "Pragma";
6666
public static readonly string ProxyAuthenticate = "Proxy-Authenticate";
6767
public static readonly string ProxyAuthorization = "Proxy-Authorization";
68+
public static readonly string ProxyConnection = "Proxy-Connection";
6869
public static readonly string Range = "Range";
6970
public static readonly string Referer = "Referer";
7071
public static readonly string RetryAfter = "Retry-After";

‎src/Http/Headers/src/PublicAPI.Unshipped.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
#nullable enable
22
Microsoft.Net.Http.Headers.MediaTypeHeaderValue.MatchesMediaType(Microsoft.Extensions.Primitives.StringSegment otherMediaType) -> bool
3-
3+
static readonly Microsoft.Net.Http.Headers.HeaderNames.ProxyConnection -> string!

‎src/Servers/Kestrel/Core/src/Internal/Http/HttpHeaders.Generated.cs

Lines changed: 404 additions & 270 deletions
Large diffs are not rendered by default.

‎src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,6 +1088,26 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
10881088
var hasConnection = responseHeaders.HasConnection;
10891089
var hasTransferEncoding = responseHeaders.HasTransferEncoding;
10901090

1091+
// We opt to remove the following headers from an HTTP/2+ response since their presence would be considered a protocol violation.
1092+
// This is done quietly because these headers are valid in other contexts and this saves the app from being broken by
1093+
// low level protocol details. Http.Sys also removes these headers silently.
1094+
//
1095+
// https://tools.ietf.org/html/rfc7540#section-8.1.2.2
1096+
// "This means that an intermediary transforming an HTTP/1.x message to HTTP/2 will need to remove any header fields
1097+
// nominated by the Connection header field, along with the Connection header field itself.
1098+
// Such intermediaries SHOULD also remove other connection-specific header fields, such as Keep-Alive,
1099+
// Proxy-Connection, Transfer-Encoding, and Upgrade, even if they are not nominated by the Connection header field."
1100+
//
1101+
// Http/3 has a similar requirement: https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-field-formatting-and-compre
1102+
if (_httpVersion > Http.HttpVersion.Http11 && responseHeaders.HasInvalidH2H3Headers)
1103+
{
1104+
responseHeaders.ClearInvalidH2H3Headers();
1105+
hasTransferEncoding = false;
1106+
hasConnection = false;
1107+
1108+
Log.InvalidResponseHeaderRemoved();
1109+
}
1110+
10911111
if (_keepAlive &&
10921112
hasConnection &&
10931113
(HttpHeaders.ParseConnection(responseHeaders.HeaderConnection) & ConnectionOptions.KeepAlive) == 0)

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/IKestrelTrace.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,5 +76,7 @@ internal interface IKestrelTrace : ILogger
7676
void Http2FrameSending(string connectionId, Http2Frame frame);
7777

7878
void Http2MaxConcurrentStreamsReached(string connectionId);
79+
80+
void InvalidResponseHeaderRemoved();
7981
}
8082
}

‎src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,10 @@ internal class KestrelTrace : IKestrelTrace
117117
LoggerMessage.Define<string>(LogLevel.Debug, new EventId(40, nameof(Http2MaxConcurrentStreamsReached)),
118118
@"Connection id ""{ConnectionId}"" reached the maximum number of concurrent HTTP/2 streams allowed.");
119119

120+
private static readonly Action<ILogger, Exception> _invalidResponseHeaderRemoved =
121+
LoggerMessage.Define(LogLevel.Warning, new EventId(41, nameof(InvalidResponseHeaderRemoved)),
122+
"One or more of the following response headers have been removed because they are invalid for HTTP/2 and HTTP/3 responses: 'Connection', 'Transfer-Encoding', 'Keep-Alive', 'Upgrade' and 'Proxy-Connection'.");
123+
120124
protected readonly ILogger _logger;
121125

122126
public KestrelTrace(ILogger logger)
@@ -295,6 +299,11 @@ public void Http2MaxConcurrentStreamsReached(string connectionId)
295299
_http2MaxConcurrentStreamsReached(_logger, connectionId, null);
296300
}
297301

302+
public void InvalidResponseHeaderRemoved()
303+
{
304+
_invalidResponseHeaderRemoved(_logger, null);
305+
}
306+
298307
public virtual void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
299308
=> _logger.Log(logLevel, eventId, state, exception, formatter);
300309

‎src/Servers/Kestrel/perf/Kestrel.Performance/Mocks/MockTrace.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,5 +56,6 @@ public void Http2ConnectionClosed(string connectionId, int highestOpenedStreamId
5656
public void Http2FrameReceived(string connectionId, Http2Frame frame) { }
5757
public void Http2FrameSending(string connectionId, Http2Frame frame) { }
5858
public void Http2MaxConcurrentStreamsReached(string connectionId) { }
59+
public void InvalidResponseHeaderRemoved() { }
5960
}
6061
}

‎src/Servers/Kestrel/shared/KnownHeaders.cs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
using System.Linq;
99
using System.Net.Http.HPack;
1010
using System.Text;
11-
using Microsoft.Net.Http.Headers;
1211

1312
namespace CodeGenerator
1413
{
@@ -17,6 +16,7 @@ public class KnownHeaders
1716
public readonly static KnownHeader[] RequestHeaders;
1817
public readonly static KnownHeader[] ResponseHeaders;
1918
public readonly static KnownHeader[] ResponseTrailers;
19+
public readonly static long InvalidH2H3ResponseHeadersBits;
2020

2121
static KnownHeaders()
2222
{
@@ -159,6 +159,7 @@ static KnownHeaders()
159159
"ETag",
160160
"Location",
161161
"Proxy-Authenticate",
162+
"Proxy-Connection",
162163
"Retry-After",
163164
"Server",
164165
"Set-Cookie",
@@ -198,6 +199,20 @@ static KnownHeaders()
198199
PrimaryHeader = responsePrimaryHeaders.Contains(header)
199200
})
200201
.ToArray();
202+
203+
var invalidH2H3ResponseHeaders = new[]
204+
{
205+
"Connection",
206+
"Transfer-Encoding",
207+
"Keep-Alive",
208+
"Upgrade",
209+
"Proxy-Connection"
210+
};
211+
212+
InvalidH2H3ResponseHeadersBits = ResponseHeaders
213+
.Where(header => invalidH2H3ResponseHeaders.Contains(header.Name))
214+
.Select(header => 1L << header.Index)
215+
.Aggregate((a, b) => a | b);
201216
}
202217

203218
static string Each<T>(IEnumerable<T> values, Func<T, string> formatter)
@@ -1015,6 +1030,11 @@ protected override bool CopyToFast(KeyValuePair<string, StringValues>[] array, i
10151030
return true;
10161031
}}
10171032
{(loop.ClassName == "HttpResponseHeaders" ? $@"
1033+
internal bool HasInvalidH2H3Headers => (_bits & {InvalidH2H3ResponseHeadersBits}) != 0;
1034+
internal void ClearInvalidH2H3Headers()
1035+
{{
1036+
_bits &= ~{InvalidH2H3ResponseHeadersBits};
1037+
}}
10181038
internal unsafe void CopyToFast(ref BufferWriter<PipeWriter> output)
10191039
{{
10201040
var tempBits = (ulong)_bits | (_contentLength.HasValue ? {"0x" + (1L << 63).ToString("x" , CultureInfo.InvariantCulture)}L : 0);

‎src/Servers/Kestrel/shared/test/CompositeKestrelTrace.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,5 +235,11 @@ public void Http2MaxConcurrentStreamsReached(string connectionId)
235235
_trace1.Http2MaxConcurrentStreamsReached(connectionId);
236236
_trace2.Http2MaxConcurrentStreamsReached(connectionId);
237237
}
238+
239+
public void InvalidResponseHeaderRemoved()
240+
{
241+
_trace1.InvalidResponseHeaderRemoved();
242+
_trace2.InvalidResponseHeaderRemoved();
243+
}
238244
}
239245
}

‎src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4785,5 +4785,48 @@ await WaitForConnectionErrorAsync<Http2ConnectionErrorException>(
47854785
expectedErrorCode: Http2ErrorCode.PROTOCOL_ERROR,
47864786
expectedErrorMessage: CoreStrings.BadRequest_MalformedRequestInvalidHeaders);
47874787
}
4788+
4789+
[Fact]
4790+
public async Task RemoveConnectionSpecificHeaders()
4791+
{
4792+
await InitializeConnectionAsync(async context =>
4793+
{
4794+
var response = context.Response;
4795+
4796+
response.Headers.Add(HeaderNames.TransferEncoding, "chunked");
4797+
response.Headers.Add(HeaderNames.Upgrade, "websocket");
4798+
response.Headers.Add(HeaderNames.Connection, "Keep-Alive");
4799+
response.Headers.Add(HeaderNames.KeepAlive, "timeout=5, max=1000");
4800+
response.Headers.Add(HeaderNames.ProxyConnection, "keep-alive");
4801+
await response.WriteAsync("hello, world");
4802+
});
4803+
4804+
await StartStreamAsync(1, _browserRequestHeaders, endStream: true);
4805+
4806+
var headersFrame = await ExpectAsync(Http2FrameType.HEADERS,
4807+
withLength: 32,
4808+
withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS,
4809+
withStreamId: 1);
4810+
var dataFrame1 = await ExpectAsync(Http2FrameType.DATA,
4811+
withLength: 12,
4812+
withFlags: (byte)Http2DataFrameFlags.NONE,
4813+
withStreamId: 1);
4814+
await ExpectAsync(Http2FrameType.DATA,
4815+
withLength: 0,
4816+
withFlags: (byte)Http2DataFrameFlags.END_STREAM,
4817+
withStreamId: 1);
4818+
4819+
await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false);
4820+
4821+
_hpackDecoder.Decode(headersFrame.PayloadSequence, endHeaders: false, handler: this);
4822+
4823+
Assert.Equal(2, _decodedHeaders.Count);
4824+
Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase);
4825+
Assert.Equal("200", _decodedHeaders[HeaderNames.Status]);
4826+
4827+
Assert.True(_helloWorldBytes.AsSpan().SequenceEqual(dataFrame1.PayloadSequence.ToArray()));
4828+
4829+
Assert.Contains(LogMessages, m => m.Message.Equals("One or more of the following response headers have been removed because they are invalid for HTTP/2 and HTTP/3 responses: 'Connection', 'Transfer-Encoding', 'Keep-Alive', 'Upgrade' and 'Proxy-Connection'."));
4830+
}
47884831
}
47894832
}

0 commit comments

Comments
 (0)