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

reverse proxy: fastcgi buffer requests for fastcgi by default #6759

Merged
merged 5 commits into from
Jan 2, 2025

Conversation

WeidiDeng
Copy link
Member

Fix 6757.

Apparently nginx buffers fastcgi requests by default.

We just set a low default buffering to allow most common requests to succeed. Requests without content length and buffering won't work either in theory. But it's likely some requests have empty body, just without content length. If requests do have a no-empty body, the requests will likely hang, as demonstrated here.

A smaller value may be possible to fix the empty body without content length. But use a larger buffer by default to ignore explicit configuration for common cases.

@teddysun
Copy link

teddysun commented Jan 2, 2025

I tested #6759, looks like fixed returns 411 Length Required problem.
But there is a new problem.

Caddyfile:

test.domain.com {
	header {
		Strict-Transport-Security "max-age=31536000; preload"
		X-Content-Type-Options nosniff
		X-Frame-Options SAMEORIGIN
	}
	root * /data/www/test
	encode gzip
	php_fastcgi unix//run/php-fpm/www.sock
	file_server {
		index index.html
	}
	log {
		output file /var/log/caddy/access.log
	}

phpMyAdmin is installed in the root directory of the website, after entering username and password, I can not log in.
and the status shown in the log is 200.

{"level":"info","ts":1735793168.3907735,"logger":"http.log.access.log1","msg":"handled request","request":{"remote_ip":"208.8.8.8","remote_port":"41088","client_ip":"208.8.8.8","proto":"HTTP/3.0","method":"POST","host":"test.domain.com","uri":"/pma/index.php?route=/","headers":{"Content-Type":["application/x-www-form-urlencoded"],"Priority":["u=0, i"],"Dnt":["1"],"Sec-Ch-Ua-Platform":["\"Windows\""],"Sec-Ch-Ua":["\"Google Chrome\";v=\"131\", \"Chromium\";v=\"131\", \"Not_A Brand\";v=\"24\""],"Accept-Encoding":["gzip, deflate, br, zstd"],"Cache-Control":["max-age=0"],"Origin":["null"],"User-Agent":["Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36"],"Cookie":["REDACTED"],"Sec-Fetch-Mode":["navigate"],"Sec-Ch-Ua-Mobile":["?0"],"Upgrade-Insecure-Requests":["1"],"Accept":["text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7"],"Sec-Fetch-Dest":["document"],"Accept-Language":["zh-CN,zh;q=0.9,ja;q=0.8,ja-JP;q=0.7,en-US;q=0.6,en;q=0.5"],"Sec-Fetch-Site":["same-origin"],"Sec-Fetch-User":["?1"],"Content-Length":["139"]},"tls":{"resumed":true,"version":772,"cipher_suite":4865,"proto":"h3","server_name":"test.domain.com"}},"bytes_read":139,"user_id":"","duration":0.007064084,"size":4440,"status":200,"resp_headers":{"Strict-Transport-Security":["max-age=31536000; preload"],"Content-Type":["text/html; charset=utf-8"],"X-Webkit-Csp":["default-src 'self' ;script-src 'self' 'unsafe-inline' 'unsafe-eval';referrer no-referrer;style-src 'self' 'unsafe-inline' ;img-src 'self' data: *.tile.openstreetmap.org;object-src 'none';"],"Cache-Control":["no-store, no-cache, must-revalidate, pre-check=0, post-check=0, max-age=0"],"X-Robots-Tag":["noindex, nofollow"],"Content-Security-Policy":["default-src 'self' ;script-src 'self' 'unsafe-inline' 'unsafe-eval' ;style-src 'self' 'unsafe-inline' ;img-src 'self' data: *.tile.openstreetmap.org;object-src 'none';"],"X-Content-Security-Policy":["default-src 'self' ;options inline-script eval-script;referrer no-referrer;img-src 'self' data: *.tile.openstreetmap.org;object-src 'none';"],"Server":["Caddy"],"X-Permitted-Cross-Domain-Policies":["none"],"Expires":["Thu, 02 Jan 2025 04:46:08 +0000"],"Referrer-Policy":["no-referrer"],"X-Frame-Options":["SAMEORIGIN","DENY"],"X-Content-Type-Options":["nosniff","nosniff"],"Set-Cookie":["REDACTED"],"X-Ob_mode":["1"],"Content-Encoding":["gzip"],"Date":["Thu, 02 Jan 2025 04:46:08 GMT"],"X-Xss-Protection":["1; mode=block"],"Pragma":["no-cache"],"Vary":["Accept-Encoding"],"Last-Modified":["Thu, 02 Jan 2025 04:46:08 +0000"]}}

Downgrade to v2.8.4 or just revert #6661 was no problem.

@WeidiDeng
Copy link
Member Author

@teddysun Your comments about the content-length header is actually correct. The value set is not the right one. Now this should run correctly.

@teddysun
Copy link

teddysun commented Jan 2, 2025

After commit 3ba88fe
I tested again.
All the issues I found were resolved. LGTM.
Thank you!

@spurgeonbj
Copy link

Hi. Checked this out and it fixes the 411 problem and PHPMyAdmin works well as well... Great job! Thank you!

// php-fpm will hang if there is any data in the body though, https://github.com/caddyserver/caddy/issues/5420#issuecomment-2415943516

// TODO: better default buffering for fastcgi requests without content length, in theory a value of 1 should be enough, make it bigger anyway
if module, ok := h.Transport.(caddy.Module); ok && module.CaddyModule().ID.Name() == "fastcgi" && h.RequestBuffers == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that we implement this hack like this cause it means transport implementations don't have control over their buffering setup. Can we make an optional interface that transports can implement to override the default request buffer size? Default to zero, but implement that func in fastcgi transport for this. Would allow any transport plugin to fix this as well if necessary. (But... if we think this is just a temporary hack that will be reworked later anyway once we have full buffering implemented then this is fine?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this shouldn't be a long-term solution, and I think I like the implicit interface idea alright, so let's revisit that in another issue. But yeah, this should do for now.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you everyone who tested this. And especially thank you to @WeidiDeng for the quick fix!

@mholt mholt added the bug 🐞 Something isn't working label Jan 2, 2025
@mholt mholt added this to the v2.9.1 milestone Jan 2, 2025
@mholt mholt merged commit 1bd567d into master Jan 2, 2025
33 checks passed
@mholt mholt deleted the fastcgi_buffer branch January 2, 2025 18:18
@bminer
Copy link

bminer commented Jan 7, 2025

Can someone elaborate more on the request_buffer fix? I am running Caddy v2.9.0 and have the HTTP 411 bug with some pages on my PHP site. If I turn on the request_buffer and set to 4096, I am unable to login to the PHP site. I get HTTP 400 response instead. Was working fine in v2.8.4.

I'm not asking that my particular problem is debugged, I am just trying to understand the solution to this problem. Can someone point me in the right direction?

@spurgeonbj

This comment has been minimized.

@JeDaYoshi
Copy link

@bminer Compile caddy from master for now, or wait until v2.9.1 is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Caddy 2.9.0 returns 411 Length Required for some sites
7 participants