[request] reject large payloads

[montanaflynn]

It's common to add safeguards against getting DoS'd by large request bodies inside your server logic, would make a great simple plugin for Kong.

Example from node.js:

    if (postBody.length > 1e6) { 
      res.writeHead(413)
      res.end()
      req.connection.destroy()
    }

[SGrondin]

Yes, it's one of the most basic attacks that http servers/libraries need to protect against. Otherwise it's possible to keep a socket open and send data until the server runs out of RAM and crashes. +1, nice catch Montana, this is very important.

[thibaultcha]

We could write a Lua plugin for that and be able to configure it on a per API/per endpoint basis. Though it feels like reinventing the wheel and less reliable than nginx's:

client_max_body_size 10M;

[SGrondin]

Yeah personally I think you should just use that nginx setting globally and be done with it.

[SGrondin]

I've thought about it some more and I think it should be a global default of 1-10Mb and a per-API setting, otherwise some APIs that deal with large files (such as video!) will not work and that would provide a bad user experience.

[thibaultcha]

I thought about it too and that's what I think too. It should be a setting per-API (as a plugin), it would be nice. But it feels bloated. I guess we have no choice but to manually implement that as a plugin. It should be easy and fun to do tho.

[subnetmarco]

This should be a plugin. And it should also handle the 100 Continue request.

[thibaultcha]

Reasons, arguments?

[sonicaghi]

if nginx already supports this, why should we do it as a plugin?

[sonicaghi]

The common guidelines is that everything should be a plugin except when nginx already has those functionalities. For example we're not doing load balancer as plugin since Nginx already has it.

[thibaultcha]

Doing it as a plugin would allow a per-api setting, that is all. But is it really a cleaner option than a global setting? Because if we do this, it is for security reasons, so a global setting acts like a cap, so does it make sense to allow multiple configurations for different APIs? Not 100% sure, even if it would be a small and simple plugin to develop (just less efficient) we need to make sure we have a need for the use case.

[thibaultcha]

We will do a load balancer plugin because we need to manipulate nginx.

[subnetmarco]

The reason is that on Kong every functionality is delivered as a plugin to allow a per-API and per-consumer setting.

Let's say that I want user A to be limited to 10MB, and user B to be limited to 20MB, with a Plugin that would be possible, as opposed to nginx generic setting that is global. Plus the per-API setting.

Even the functionality nginx already has can be implemented as plugins to allow this flexibility.

[shashiranjan84]

if it is implemented as plugin, we would also need to disable NGINX default setting. Basically if user plan to enable plugin then disable NGINX global setting for max payload size .

[subnetmarco]

@shashiranjan84 yes

[sonicaghi]

I see. I actually have a use case. When creators will charge for their APIs, they may set different load caps for different pricing plans. Example: Storage API: $5 per 10mb, $15 per 50mb upload.

[sonicaghi]

Don't forget that a plugin should be per:

1. API
2. Consumer
3. and Endpoint (to-do)

[thibaultcha]

That's a commercial usage, like data usage, which does make sense. Here we're talking about security.

I haven't forgetten lmao

[montanaflynn]

I would like to have an optional global limit (default: off) in kong.yml and allow plugins to override it on a case-by-case basis. All my APIs have a limit of 10mb to avoid DoS'ing my services but for some I might want to allow uploads so I could use a plugin on that particular API or consumer (or endpoint #192).

This could be done with a whitelist/blacklist implementation but it also seems intuitive to introduce hierarchy. In nginx you could apply this rule to everything, a specific server or server location blocks which all overrule the parent block if set.

Nginx calls this directive client_max_body_size and it's available in three "contexts": http, server, location

This should be a plugin. And it should also handle the 100 Continue request.

Even the functionality nginx already has can be implemented as plugins to allow this flexibility.

For now I'm editing the embedded nginx config in kong.yml so I think the easiest, if only temporary, solution would be to add the global rule in kong.yml corresponding to the nginx http block and let it handle all the gritty details for Kong. It would be amazing if we could have Kong leverage nginx's directives in the plugin implementation rather than trying to satisfy the spec in Lua ourselves.

This is a big enough issue that we should get it out in a 0.3.x release ASAP as a global option or plugin.

[thibaultcha]

#292

[subnetmarco]

@shashiranjan84 closed?

[shashiranjan84]

yes