Add a request body size limit #1877

halter73 · 2017-06-01T00:35:42Z

Add IHttpMaxRequestBodySizeFeature
Improve TestConnection.Receive timeout errors

davidfowl · 2017-06-01T00:41:48Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServerLimits.cs

@@ -20,6 +20,9 @@ public class KestrelServerLimits
        // Matches the default large_client_header_buffers in nginx.
        private int _maxRequestHeadersTotalSize = 32 * 1024;

+        // Disabled by default.
+        private long? _maxRequestBodySize = null;


good default 😉

Tratcher · 2017-06-01T03:45:56Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/IHttpMaxRequestBodySizeFeature.cs

+    /// <summary>
+    /// 
+    /// </summary>
+    public interface IHttpMaxRequestBodySizeFeature


This needs to move to HttpAbstractions, it's not Kestrel specific.

Tratcher · 2017-06-01T03:54:13Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

+            {
+                if (_contentLength > _context.MaxRequestBodySize)
+                {
+                    _context.RejectRequest(RequestRejectionReason.RequestBodyTooLarge);


This method is really misleading, it just throws an exception, it doesn't change any state.

This is typical kestrel code though.

I've been misled by this too. At one point, someone compared this to explicit throws (e.g. throw MethodThatCreatesException()) and concluded that it doesn't inline as well, enough that it hurts perf. I think that was @pakrym or @benaadams

I was tempted to change the name of RejectRequest to ThrowBadHttpRequestException as part of this change. I could still do that if we're in agreement that it makes this and all the other calling code at least slightly more readable.

Renaming to ThrowXxx probably makes sense to indicate control is not returned to caller.

Tratcher · 2017-06-01T03:56:09Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

+            if (!_context.HasStartedConsumingRequestBody)
+            {
+                _context.HasStartedConsumingRequestBody = true;
+                OnReadStart();


What happens on the second call to ReadAsync if the first one throws here and is caught by the app? You need to reverse these lines.

Start was cleaner tbh. I understand why it was removed but it meant we avoid this lazy initialize once logic.

Yep. I definitely understand avoiding lazy initialization is cleaner. It's not even too inefficient to greedily start the pump now that the "max" size of the body pipe is 1 byte.

The problem is that the lazy init logic was required for the request feature to work nicely with Content-Length bodies, and if you are going to do some check only on the first read anyway, it's no longer cleaner to greedily start the pump.

Doesn't look like this got fixed.

Tratcher · 2017-06-01T04:02:52Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

                {
                    if (ch1 == ';')
                    {
                        consumed = reader.Cursor;
                        examined = reader.Cursor;

+                        AddAndCheckConsumedBytes(reader.ConsumedBytes);


Counting the framing is going to cause strange inconsistencies between Chunked, Content-Length, and HttpSysServer. @shirhatti I assume IIS does not count the chunked framing because it doesn't even see it, Http.Sys removes it, right?

Tratcher · 2017-06-01T04:04:01Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

                do
                {
                    ReadCursor extensionCursor;
                    if (ReadCursorOperations.Seek(buffer.Start, buffer.End, out extensionCursor, ByteCR) == -1)
                    {
                        // End marker not found yet
+                        CheckExaminedBytes(buffer.Length);


This is the wrong limit to apply to extensions. Let's review offline.

It's super trivial to change this to just count the "data" bytes if that's what we decide. I decided to do the more difficult thing first.

I'm not too concerned about breaking any apps by counting extension bytes towards the request body size limit since chunk extensions are so rare. Counting the framing bytes might make Kestrel behave differently that HttpSysServer, but the framing can easily take up to ~92% of the uploaded bytes without any chunk extensions or trailing headers. We can discuss offline how much we care about that.

Tratcher · 2017-06-01T04:06:56Z

test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/MessageBodyTests.cs

+                input.Add("80000000\r\n");
+
+                var buffer = new byte[1024];
+                var ex = await Assert.ThrowsAsync<OverflowException>(async () =>


It should be wrapped in an IOException

This is just a test I added for existing behavior, so technically that's a breaking change but not really in practice.

Tratcher · 2017-06-01T04:11:23Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/MaxRequestBodySizeTests.cs

+                        "",
+                        "");
+                    await connection.ReceiveForcedEnd(
+                        "HTTP/1.1 413 Payload Too Large",


The BadHttpRequestException is catchable by the app, correct? You only get this 413 if they do not catch it and it bubbles back out from the app pipeline?

Mostly correct. All future reads would also fail, so even if the app swallowed all exceptions and didn't write to the response body, the request draining code would see the exception which would still trigger a 413. If the app writes to the response body before the request is drained (or it's a non-keepalive request that doesn't get drained), then there might not be a 413 response.

benaadams · 2017-06-01T04:41:32Z

Have déjà vu 😄

Good to standardize it as a common cross server feature though

cesarblum · 2017-06-01T17:59:16Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs

-                MaximumSizeLow = ServiceContext.ServerOptions.Limits.MaxRequestBufferSize ?? 0
+                WriterScheduler = InlineScheduler.Default,
+                MaximumSizeHigh = 1,
+                MaximumSizeLow = 1


+1, can you explain the change?

This prevents double buffering. Since the request body is only exposed directly as a Stream, it's always greedily consumed (i.e. it's impossible to inspect data from the request body via a Stream without consuming the data from the pipe). This means there's no functional reason to buffer here.

There could theoretically be a perf impact which is why I posted the CopyToAsync benchmark results. This is still pretty efficient since backpressure doesn't get applied to the client until the connection's Input pipe buffers up to MaxRequestBufferSize.

In theory, this should really be set to MaxRequestBufferSize and this limit should be decided by the transport. Basically our limits for the server shouldn't apply here. The problem is that the request is split for us between start line and headers and the body size...

@davidfowl Are you saying that ideally the request body Pipe would buffer instead of the connection "Input" Pipe?

pakrym · 2017-06-01T18:29:14Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

+            {
+                _context.HasStartedConsumingRequestBody = true;
+                OnReadStart();
+                var ignore = PumpAsync();


Observe exceptions?

PumpAsync is wrapped in a giant try/catch which will Complete the body pipe writer with the error, so the app can observe the exception. I'll add a comment though.

natemcmaster · 2017-06-01T18:57:51Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/CoreStrings.resx

+    <value>The maximum request body size cannot be modified after the app has already started reading from the request body.</value>
+  </data>
+  <data name="MaxRequestBodySizeCannotBeModifiedForUpgradedRequests" xml:space="preserve">
+    <value>The maximum request body size cannot be modified after the request has be upgraded.</value>


...."has been upgraded".

natemcmaster · 2017-06-01T18:59:42Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.FeatureCollection.cs

+                    throw new InvalidOperationException(CoreStrings.MaxRequestBodySizeCannotBeModifiedForUpgradedRequests);
+                }
+
+                MaxRequestBodySize = value;


First validate the value is >= 0

natemcmaster · 2017-06-01T19:05:47Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

@@ -499,13 +528,14 @@ private void ParseChunkedPrefix(ReadableBuffer buffer, out ReadCursor consumed,
                var chunkSize = CalculateChunkSize(ch1, 0);
                ch1 = ch2;

-                do
+                while (reader.ConsumedBytes < 10)


can you give this magic number a name by setting it as a const?

natemcmaster · 2017-06-01T19:11:47Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServerLimits.cs

+            get => _maxRequestBodySize;
+            set
+            {
+                if (value <= 0)


Quirky, but technically 0 is a valid limit, right?

natemcmaster · 2017-06-01T19:19:42Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/IHttpMaxRequestBodySizeFeature.cs

+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
+{
+    /// <summary>
+    /// 


Nit: add a description

Got lazy since I knew this would be moved to HttpAbstractions 😄

natemcmaster · 2017-06-01T19:19:58Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/IHttpMaxRequestBodySizeFeature.cs

+        // TODO: Explain how to override
+        /// <summary>
+        /// Gets or sets the maximum allowed size of the current request body in bytes.
+        /// When set to null, the maximunm request body size is unlimited.


Typo: maximum

natemcmaster · 2017-06-01T19:24:57Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

+            {
+                if (_contentLength > _context.MaxRequestBodySize)
+                {
+                    _context.RejectRequest(RequestRejectionReason.RequestBodyTooLarge);


I've been misled by this too. At one point, someone compared this to explicit throws (e.g. throw MethodThatCreatesException()) and concluded that it doesn't inline as well, enough that it hurts perf. I think that was @pakrym or @benaadams

cesarblum · 2017-06-02T00:41:57Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServerLimits.cs

+        /// <remarks>
+        /// Defaults to null (unlimited).
+        /// </remarks>
+        public long? MaxRequestBodySize


Add tests for default value, valid values and invalid values in KestrelServerLimitsTests.

cesarblum · 2017-06-02T00:43:51Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.FeatureCollection.cs

@@ -202,6 +203,24 @@ string IHttpRequestIdentifierFeature.TraceIdentifier
            set => TraceIdentifier = value;
        }

+        long? IHttpMaxRequestBodySizeFeature.MaxRequestBodySize


Add tests in FrameTests.

- Add IHttpMaxRequestBodySizeFeature - Improve TestConnection.Receive timeout errors

halter73 · 2017-06-05T23:42:04Z

⬆️ 📅

Tratcher · 2017-06-06T00:15:17Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServerLimits.cs

@@ -20,6 +21,10 @@ public class KestrelServerLimits
        // Matches the default large_client_header_buffers in nginx.
        private int _maxRequestHeadersTotalSize = 32 * 1024;

+        // Matches the default maxAllowedContentLength in IIS (~28.6 MB)
+        // https://www.iis.net/configreference/system.webserver/security/requestfiltering/requestlimits#005
+        private long? _maxRequestBodySize = 30000000;


We decided on 16mb in the meeting

I remember deciding the highest default of IIS/nginx which would be this.

No, go read the meeting notes.

I see it in the meeting notes, but there's no reasoning. The decision was to use the larger default which is ~28.6 MB. I think that was written before we looked up what the largest default was. I can change it in the notes if you think that helps.

@DamianEdwards @blowdart

To clarify, clear it with Damian and Barry, don't just change it.

halter73 · 2017-06-06T01:02:50Z

> dotnet run -- -n CopyToAsync  -o Benchmarks@halter73/react-to-logging -o KestrelHttpServer@rel/2.0.0-preview2
[05:40:07.608] Starting scenario CopyToAsync on benchmark server...
[05:40:30.061] Warmup
[05:40:30.102] Starting scenario CopyToAsync on benchmark client...
[05:40:47.397] Scenario CopyToAsync completed on benchmark client
[05:40:47.397] RPS: 10562.37
[05:40:47.398] Stopping scenario CopyToAsync on benchmark client...
[05:40:47.407] Benchmark
[05:40:47.408] Starting scenario CopyToAsync on benchmark client...
[05:41:03.579] Scenario CopyToAsync completed on benchmark client
[05:41:03.579] RPS: 10638.63
[05:41:03.580] Stopping scenario CopyToAsync on benchmark client...
[05:41:03.583] Stopping scenario CopyToAsync on benchmark server...

> dotnet run -- -n CopyToAsync -o Benchmarks@halter73/react-to-logging -o KestrelHttpServer@halter73/max-read
[05:56:08.048] Starting scenario CopyToAsync on benchmark server...
[05:56:31.534] Warmup
[05:56:31.562] Starting scenario CopyToAsync on benchmark client...
[05:56:47.802] Scenario CopyToAsync completed on benchmark client
[05:56:47.802] RPS: 10438.51
[05:56:47.803] Stopping scenario CopyToAsync on benchmark client...
[05:56:47.806] Benchmark
[05:56:47.806] Starting scenario CopyToAsync on benchmark client...
[05:57:04.024] Scenario CopyToAsync completed on benchmark client
[05:57:04.024] RPS: 10563.52
[05:57:04.025] Stopping scenario CopyToAsync on benchmark client...
[05:57:04.030] Stopping scenario CopyToAsync on benchmark server...

davidfowl · 2017-06-06T17:33:39Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.FeatureCollection.cs

+            get => MaxRequestBodySize;
+            set
+            {
+                if (HasStartedConsumingRequestBody)


How does the application check this to make sure it's safe to do to avoid throwing?

Right now, the app has to keep track of whether or not it's read from the request body itself. I know this might break down a little if you are writing some reusable component where you want to change the upload limit, but only if nothing has been uploaded yet. That seems pretty niche though, and nothing uses this feature today ofc.

Do you want to add HasStartedConsumingRequestBody to some request feature? Do you want to do the same for WasUpgraded?

Yes, the problem is you can add middleware anywhere in the pipeline that reads the body. There needs to be a way to not throw here.

Do you want to add HasStartedConsumingRequestBody to some request feature? Do you want to do the same for WasUpgraded?

Yes, or something more generally named like our other bools of this nature (HasResponseStarted etc).

Could add it to the new IHttpMaxRequestBodySizeFeature feature: LimitCanBeChanged or IsMutable

Both of those names are terrible but I agree with the sentiment.

davidfowl · 2017-06-06T17:33:46Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.FeatureCollection.cs

+                {
+                    throw new InvalidOperationException(CoreStrings.MaxRequestBodySizeCannotBeModifiedAfterRead);
+                }
+                if (_wasUpgraded)


davidfowl · 2017-06-06T17:37:34Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

+            {
+                OnReadStart();
+                _context.HasStartedConsumingRequestBody = true;
+                var ignore = PumpAsync();


nit: _ = PumpAsync()

cesarblum · 2017-06-07T00:06:06Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServerLimits.cs

+        /// Gets or sets the maximum allowed size of any request body in bytes.
+        /// When set to null, the maximum request body size is unlimited.
+        /// This limit has no affect on upgraded connections which are always unlimited.
+        /// This can be overridden by <see cref="IHttpMaxRequestBodySizeFeature"/>.


I would change this to "This can be overridden per-request via <see cref="IHttpMaxRequestBodySizeFeature"/>."

cesarblum · 2017-06-07T00:08:51Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServerLimits.cs

@@ -134,6 +139,28 @@ public int MaxRequestHeaderCount
        }

        /// <summary>
+        /// Gets or sets the maximum allowed size of any request body in bytes.
+        /// When set to null, the maximum request body size is unlimited.
+        /// This limit has no affect on upgraded connections which are always unlimited.


s/affect/effect

dnfclas added the cla-already-signed label Jun 1, 2017

davidfowl reviewed Jun 1, 2017

View reviewed changes

Tratcher suggested changes Jun 1, 2017

View reviewed changes

cesarblum reviewed Jun 1, 2017

View reviewed changes

pakrym reviewed Jun 1, 2017

View reviewed changes

natemcmaster reviewed Jun 1, 2017

View reviewed changes

Tratcher mentioned this pull request Jun 1, 2017

Implement an attribute and filter to configure the request body size limit aspnet/Mvc#6352

Closed

halter73 force-pushed the halter73/max-read branch from 86affb8 to 698ac44 Compare June 2, 2017 00:20

halter73 changed the base branch from dev to rel/2.0.0-preview2 June 2, 2017 00:21

cesarblum suggested changes Jun 2, 2017

View reviewed changes

cesarblum mentioned this pull request Jun 2, 2017

Add request body minimum data rate feature #1874

Merged

8 tasks

halter73 force-pushed the halter73/max-read branch from 698ac44 to a094446 Compare June 2, 2017 18:10

halter73 mentioned this pull request Jun 2, 2017

Add IHttpMaxRequestBodySizeFeature aspnet/HttpAbstractions#852

Merged

halter73 added 4 commits June 5, 2017 15:59

Add a request body size limit

1daa905

- Add IHttpMaxRequestBodySizeFeature - Improve TestConnection.Receive timeout errors

PR Feedback: Use IHttpMaxRequestBodySizeFeature from HttpAbstractions

dbba17e

Address PR feedback

ba9d318

Rename RejectRequest to ThrowRequestRejected

16757b0

halter73 force-pushed the halter73/max-read branch from a094446 to 16757b0 Compare June 5, 2017 23:00

cesarblum approved these changes Jun 6, 2017

View reviewed changes

natemcmaster approved these changes Jun 6, 2017

View reviewed changes

Tratcher reviewed Jun 6, 2017

View reviewed changes

More PR feedback

65c5e00

davidfowl reviewed Jun 6, 2017

View reviewed changes

This was referenced Jun 6, 2017

Add IHttpMaxRequestBodySizeFeature.IsReadOnly aspnet/HttpAbstractions#858

Merged

Add IHttpRequestFeature.HasStartedReadingBody aspnet/HttpAbstractions#859

Closed

halter73 mentioned this pull request Jun 6, 2017

Add IHttpUpgradeFeature.WasUpgraded aspnet/HttpAbstractions#860

Closed

Add support for IHttpMaxRequestBodySizeFeature.IsReadOnly

3dbfb00

halter73 force-pushed the halter73/max-read branch from 863ef2f to 3dbfb00 Compare June 6, 2017 23:07

cesarblum reviewed Jun 7, 2017

View reviewed changes

PR feedback: doc comments

ea22318

halter73 merged commit f96c48c into rel/2.0.0-preview2 Jun 7, 2017

natemcmaster deleted the halter73/max-read branch June 12, 2017 21:49

303248153 mentioned this pull request Jul 20, 2017

Post large body cause model mark with [FromBody] become null aspnet/Mvc#6568

Closed

Add a request body size limit #1877

Add a request body size limit #1877

Conversation

halter73 commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher Jun 6, 2017 •

edited

Loading

cesarblum Jun 7, 2017 •

edited

Loading