Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Add header limit parameters #475

Closed
benaadams opened this issue Dec 13, 2015 · 19 comments
Closed

Add header limit parameters #475

benaadams opened this issue Dec 13, 2015 · 19 comments

Comments

@benaadams
Copy link
Contributor

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.

@benaadams benaadams changed the title Add MaxHeadersLength parameter Add MaxHeadersTotalLength parameter Dec 13, 2015
@Tratcher
Copy link
Member

See #479 (comment)

@benaadams
Copy link
Contributor Author

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
Copy link
Contributor Author

Probably simpler code wise if there was only one param.

@Tratcher
Copy link
Member

@blowdart ?

@blowdart
Copy link
Member

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

@muratg
Copy link
Contributor

muratg commented Jan 14, 2016

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

@blowdart
Copy link
Member

Yes

@DamianEdwards
Copy link
Member

post-RC2

@cesarblum
Copy link
Contributor

@muratg Moving to 1.0.0 and assigning to myself.

@cesarblum
Copy link
Contributor

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
Copy link
Member

blowdart commented May 17, 2016

No, we want all the things - separate please

@cesarblum
Copy link
Contributor

So, rounding up the specific things we want:

  • Request line length
  • Request target length (I know @Tratcher would like this to be part of the previous one)
  • Request query length? I'd rather have it as part of target length.
  • Max single header length
  • Max total headers length
  • Max header count

What do you guys think?

@Tratcher
Copy link
Member

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

  • Request line length
  • Max total headers length
  • Max header count

@muratg
Copy link
Contributor

muratg commented May 17, 2016

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

cc @davidfowl

@cesarblum
Copy link
Contributor

cesarblum commented May 17, 2016

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

@cesarblum cesarblum added task and removed bug labels May 17, 2016
@halter73
Copy link
Member

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
Copy link
Contributor

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

@mikeharder
Copy link
Contributor

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

@benaadams
Copy link
Contributor Author

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

@muratg muratg modified the milestones: Backlog, 1.0.0 May 18, 2016
@muratg muratg added enhancement and removed task labels Jul 25, 2016
@muratg muratg modified the milestones: 1.1.0, Backlog Jul 25, 2016
@cesarblum cesarblum changed the title Add MaxHeadersTotalLength parameter Add request limit parameters Jul 27, 2016
@cesarblum cesarblum changed the title Add request limit parameters Add header limit parameters Jul 27, 2016
cesarblum pushed a commit that referenced this issue Aug 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants