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

RX_OVERFLOW returns TCP reset instead of HTTP 414 #2735

Open
sgran opened this issue Jul 22, 2018 · 10 comments · May be fixed by #4207
Open

RX_OVERFLOW returns TCP reset instead of HTTP 414 #2735

sgran opened this issue Jul 22, 2018 · 10 comments · May be fixed by #4207

Comments

@sgran
Copy link

sgran commented Jul 22, 2018

Hi there,

I'm uncertain about whether I am seeing correct behaviour in varnish or not. I am looking at how varnish handles RX_OVERFLOW, and it seems like there is a better behaviour available than closing the socket - HTTP 414 seems designed for this case.

As I say, I'm not certain whether you consider this a bug. I quite understand that closing the connection is the simplest way to deal with an overlong request header. I have a use case where it would be better for my clients if we sent them a 4xx response code for these - request parameters that we see are typically generated programmatically from the output of other queries, so can have quite large sets of parameters to iterate over. It's not the end of the world if each client has to learn that "connection reset" ends up meaning the same thing as "HTTP 414", but it would make it simpler to explain to them what's going on if they were getting a 4xx error code.

The behaviour appears to be by design and not an accident of compilation, but we have seen this behaviour on various versions of varnish from 4.1 to 6.0, on Debian and Ubuntu.

@slimhazard
Copy link
Contributor

I agree with the gist of this, except that "414 URI Too Long" doesn't sound right, unless it was just the URI that lead to the overflow (which would really be something).

"413 Payload Too Large" appears to be the right response for this:
https://tools.ietf.org/html/rfc7231#section-6.5.11

BTW @sgran +1 for the avatar.

@sgran
Copy link
Author

sgran commented Jul 23, 2018

Hi,

Yeah, I am seeing this for the request URI (which is amazing), but it's because the query params are being programmatically generated from other request/reply events - our clients need to learn how to ask for pages instead of all things all at once.

I can see that differentiating between URI and other headers in the return code might be a good thing, so perhaps they do want different responses.

@slimhazard
Copy link
Contributor

We talked about some possible approaches at bugwash today, but @bsdphk was not around and we want to get his input.

A point that was mentioned is that closing the connection causes the least amount of processing for abusive clients -- avoids Varnish getting DOSsed by a flood of oversized requests.

But that doesn't mean that we aren't open to different solutions (just not ready to decide today).

@sgran
Copy link
Author

sgran commented Jul 23, 2018

I quite understand. I got the impression from reading over the structure of things that this is on purpose, and it makes sense in many settings.

I guess I'm hoping for a way to override the default behaviour with either a config switch or in VCL.

@bsdphk
Copy link
Contributor

bsdphk commented Aug 6, 2018

Originally we decided to just close on abusive input, to improve DoS resistance.

That still sounds sensible to me, but if there is a need for more civilized behaviour, we could make a feature to control it (default open to discussion)

@bsdphk
Copy link
Contributor

bsdphk commented Aug 6, 2018

We bugwashed it, and my previous comment stands.

If we want to modulate behaviour, it has to be a featureflag, we cannot ask VCL since we cannot represent the request for inspection.

@zuozp8
Copy link

zuozp8 commented Jun 12, 2024

I have varnish behind nginx.
If nginx uses existing keep-alive connection to varnish then it will treat closing that connection as keep-alive problem (as described in https://trac.nginx.org/nginx/ticket/2022 ) and retry it. I had ~100k retries of one request

@dridi dridi reopened this Jun 12, 2024
@dridi
Copy link
Member

dridi commented Jun 12, 2024

I will find someone to work on this.

@dridi
Copy link
Member

dridi commented Jun 12, 2024

I had ~100k retries of one request

We have a max_retries parameter in varnishd, there should be an equivalent in nginx.

edit: quick search yields proxy_next_upstream_tries

@zuozp8
Copy link

zuozp8 commented Jun 13, 2024

in nginx I have proxy_next_upstream_tries set to 2, but in this comment it's explained that nginx may omit counting attempts that used keep-alive connection and were brutally closed

I tightened uri length limit in nginx and avoid RX_OVERFLOW, but others can encounter it too

@cartoush cartoush linked a pull request Oct 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants