aspnet / KestrelHttpServer

[Archived] A cross platform web server for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
2.63k stars 529 forks source link

Add header limit parameters #475

Closed benaadams closed 8 years ago

benaadams commented 8 years ago

Failure state: 431 Request Header Fields Too Large (ngnix returns 400 Bad Request)

"The server is unwilling to process the request because either an individual header field, or all the header fields collectively, are too large." RFC 6585

http.sys default MaxRequestBytes 16384 ngnix default large_client_header_buffers 4 * 8k = 32768 apache.tomcat default maxHttpHeaderSize 8192

Will currently be enforced by reverse proxy config (e.g. IIS/nginx) when deployed in recommended configuration - but not in pure Kestrel deployment.

Tratcher commented 8 years ago

See https://github.com/aspnet/KestrelHttpServer/issues/479#issuecomment-164325646

benaadams commented 8 years ago

Question on whether not having a max url/start length (beyond total header size) would have any upstream issues?

Guessing it wouldn't have a great impact beyond anything else (like lots of headers)

benaadams commented 8 years ago

Probably simpler code wise if there was only one param.

Tratcher commented 8 years ago

@blowdart ?

blowdart commented 8 years ago

Work for me. We used to have this with IIS and UrlScan

muratg commented 8 years ago

@blowdart Is this required for RC2/RTM? Or can we punt it to post RTM?

blowdart commented 8 years ago

Yes

DamianEdwards commented 8 years ago

post-RC2

cesarblum commented 8 years ago

@muratg Moving to 1.0.0 and assigning to myself.

cesarblum commented 8 years ago

So do we want just a single parameter to limit request line + header length? No checking request line length individually and individual header lengths?

Ping @Tratcher @halter73 @blowdart @davidfowl

blowdart commented 8 years ago

No, we want all the things - separate please

cesarblum commented 8 years ago

So, rounding up the specific things we want:

What do you guys think?

Tratcher commented 8 years ago

There's a lot of overlap and a lot of knobs here. I think we only need:

muratg commented 8 years ago

@cesarbs Before starting to work on it, can we have a table of the knobs, and default values to review?

cc @davidfowl

cesarblum commented 8 years ago

I like @Tratcher's list, keeps things simple. Plus we risk regressing perf if we're doing too many checks all over the request line/headers processing.

Here's a proposed list of limits, default and response status codes when limits are exceeded:

Limit Default Status code
Request line length 16K 414 ("Request URI Too Long", but realistically that's 99% of legit cases where the request line would be rejected based on length limit)
Headers length 16K 431
Header count 64 431
halter73 commented 8 years ago

Is the 16K "Headers length" limit per header meaning there would be a total limit of 1MB for all headers? Would we indicate to the client which type of header limit it ran into?

cesarblum commented 8 years ago

@halter73 No, not per header. It would be a length limit on all headers collectively.

mikeharder commented 8 years ago

Would be nice to add table columns showing the behavior of other servers, particularly IIS.

benaadams commented 8 years ago

Would be nice to add table columns showing the behavior of other servers, particularly IIS.

@cesarbs numbers should be in all the closed linked bugs at top