Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a server.maxUploadBytes setting for all upload endpoints #77945

Closed
joshdover opened this issue Sep 18, 2020 · 4 comments
Closed

Introduce a server.maxUploadBytes setting for all upload endpoints #77945

joshdover opened this issue Sep 18, 2020 · 4 comments
Labels
Feature:http performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@joshdover
Copy link
Contributor

There are several HTTP endpoints in Kibana that serve simply to upload different data to Elasticsearch. One issue that users run into with these endpoints is that the default server.maxPayloadBytes setting is too low for these endpoints.

In general, we do not want to increase the server.maxPayloadBytes setting because:

  • It increases Kibana's vulnerability to DOS attacks since parsing and processing large request bodies can be CPU intensive.
  • While some endpoints simply buffer this data to be uploaded to Elasticsearch (eg. Canvas workpads), most endpoints actually parse and do something with the request body.

What we could do is expose a single server.maxUploadBytes setting that is higher and is used by all upload endpoints in Core & Plugins. To make this safer for DOS attacks, we could create an HttpService API that defaults to using this setting & does not parse the payload body. For example:

router.upload(
  { path: '/api/my_upload' },
  async (context, req, res) => {
    req.body // always a Buffer or Stream
    ...
  }
);
@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc performance labels Sep 18, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor

To make this safer for DOS attacks, we could create an HttpService API that defaults to using this setting & does not parse the payload body

That sounds like a good compromise. One question though: Even if POST is probably the most commonly used http method for uploads, I think using others, such as PUT, can be possible, right (thinking of JSON body uploads, not multipart/form-data)? Would we restrict this router.upload API to only register POST methods, or should we be add an additional method parameter when registering the route (which may be non-trivial due to the inference nightmare that the http routing types are)?

An alternative could be to add a upload boolean option to RouteConfigOptions that would have the same effect:

router.post(
  { path: '/api/my_upload', options: { upload: true } },
  async (context, req, res) => {
    req.body // always a Buffer or Stream
    ...
  }
);

@mshustov
Copy link
Contributor

I'm not a fan of boolean flags, they start fighting each other quickly, so it will complicate type declarations. So I'm down for the router.upload form. And yes, we should support a method declaration, not to repeat the same mistake in the HttpResources interface - Once Core's HttpResources can support POST method in addition to GET we can handle SLO Responses coming via both HTTP-Redirect and HTTP-POST bindings from #69506

@lizozom
Copy link
Contributor

lizozom commented Apr 18, 2022

Closing due to no activity. Re-open if needed 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:http performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

6 participants