Request size limiting plugin

[shashiranjan84]

Creating this pull request to test my code on travis.

[thibaultcha]

There should probably still be a max body size as it is more a matter of security than anything else. And we can document "Your request size limiting settings cannot exceed the maximum size configured in your kong configuration".

[shashiranjan84]

does NGINX has any default value for this field or its upto user to set this value? If not then then even setting it to 0 not needed, simply use the plugin. Using it at both in configuration file and plugin would be confusing

[thibaultcha]

http://nginx.org/en/docs/http/ngx_http_core_module.html#client_max_body_size

[shashiranjan84]

Ok its 1 mb. But if we set a max payload size, plugin would be useless beyond that size limit. I thought whole idea for this pluingin is to give control to user. We do have to mention in our doc that by default this field is disabled.

[thibaultcha]

Like I said, for security reason you want a value. 128m was fine. By putting it to 0 you remove any value, so anybody not using the plugins or when APIs don’t have the plugin enabled, they are vulnerable.

So yes the only solution is to put a value, and document that any value in set when adding the plugin on an API cannot exceed the configured value.

Ideally, if possible, try to catch when a value is set higher than the max value.

On 04 Jun 2015, at 01:59, shashi ranjan notifications@github.com wrote:

In kong.yml #292 (comment):

@@ -90,7 +91,7 @@ nginx: |
real_ip_recursive on;

 # Other Settings

• client_max_body_size 128m;
• client_max_body_size 0m;
  Ok its 1 mb. But if we set a max payload size, plugin would be useless beyond that size limit. I thought whole idea for this pluingin is to give control to user. We do have to mention in our doc that by default this field is disabled.

—
Reply to this email directly or view it on GitHub https://github.com/Mashape/kong/pull/292/files#r31683093.

[nijikokun]

NGINX doesn't allow overriding when a value is set, thus any plugin being enabled to secure an API will not be applied in this case.

Removing the value, and adding a security suggestion within the documentation alerting the user to this will be sufficient enough and allow us to deploy it. From my understanding the principals surrounding KONG is a way for the user to avoid any complex or having to modify the NGINX configuration in any fashion and this is a way towards that principal.

Something else I think that would be beneficial in the future is some form of pre-defined configurations based on #295 that would alleviate any question of security missteps in the community.

[thibaultcha]

Overriding what? We want a global cap of X, defined in the nginx.conf, that will always be applied, for all requests.

Behind that, @shashiranjan84 simply handles manually for API A or B the actual limit. It's not overridden.

In the documentation of the plugin we simply explain that if you set a limit above the global limit set in nginx.conf, it won't work, you need to increase the global limit in nginx.conf.

[ahmadnassri]

there are many issues in the current architecture which would make this plugin (as it is today) with disabling the global limit a bad implementation:

starting with the nginx client_max_body_size this can be applied in multiple contexts: http, server, location, which allows users to have a:

• global value (http) for security
• per server value for exceptions that overrides the global as needed (think of this as "per api")
• per location value that overrides all the above, per request path (this would also be equivalent of "per path" in Kong)

the current architecture allows to only enable plugins "per API", meaning, if you have multiple APIs, you'd have to add the same plugin to each API, manually every time.

so you should be able to "override" the default value per API, and keep the default for the rest.

if its not possible to override the nginx default per-api, then you need to provide the same behaviour in Kong. (a.k.a a default value)

[nijikokun]

Looks good. Lets keep the removal of the NGINX conf setting and merge this, and update the getkong docs with a security suggestion. 👍

Would the security suggestion be good enough the time being @ahmadnassri

[ahmadnassri]

Something else I think that would be beneficial in the future is some form of pre-defined configurations based on #295 that would alleviate any question of security missteps in the community.

I believe this is the right approach, and most powerful.

Looks good. Lets keep the removal of the NGINX conf setting and merge this, and update the getkong docs with a security suggestion.

as an interim solution that makes sense, given that #295 will be prioritized in future releases.

[thibaultcha]

as an interim solution that makes sense, given that #295 will be prioritized in future releases.

I don't see how removing it helps. As an interim solution, keeping it is what makes sense. Then it can be removed. You're making Kong vulnerable by removing it.

[nijikokun]

I don't see how removing it helps. As an interim solution, keeping it is what makes sense. Then it can be removed. You're making Kong vulnerable by removing it.

Because it will break user expectations when they add a plugin and it doesn't work, adding a security suggestion will not break user expectation and will allow this plugin to work without a secondary step out of the box.

[thibaultcha]

It won't break anything if instead of a security suggestion, you add a note. It's 1 paragraph for each. One solution makes Kong vulnerable, the other doesn't, and makes it very unlikely to break user expectation since there is a note about the plugin.

[nijikokun]

It won't break anything if instead of a security suggestion, you add a note. It's 1 paragraph for each. One solution makes Kong vulnerable, the other doesn't, and makes it very unlikely to break user expectation since there is a note about the plugin.

It will break one of the core principals surrounding KONG, which is that the user doesn't need to modify the NGINX configuration.

[thibaultcha]

Who said that was an expectation?

1. The nginx config is available in kong.yml
2. If that is your concern, we can add a YAML property max_body_size, and that's it

[montanaflynn]

In my opinion:

1. Kong shouldn't add a default global limit for the proxy, just as a vanilla nginx install doesn't make any assumptions about your services neither should Kong.

2. If a limit is enabled then Kong should properly handle the Expect header and return 100 if it passes. It would be ideal if somehow the plugin could offload this logic to nginx, not sure if that's possible.

3. I like having both options, setting a global default and overriding it based on plugin configurations.

[thibaultcha]

Nginx does have a default "global limit".

On Jun 4, 2015, at 5:30 AM, Montana Flynn notifications@github.com wrote:

In my opinion:

1. Kong shouldn't add a default global limit for the proxy, just as a vanilla nginx install doesn't make any assumptions about your services neither should Kong.

2. If the limit is enabled then Kong should properly handle the Expect header and return 100 if it passes. It would be ideal if somehow the plugin could offload this logic to nginx, not sure if that's possible.

3. I like having both options, setting a global default and overriding it based on plugin configurations.

—
Reply to this email directly or view it on GitHub.