Restore ability to relay POST requests with empty bodies

[maggie44]

It appears that the commit in the latest release (https://github.com/cloudflare/cloudflared/commit/e2064c820f32802f58baa027903043fe0ed052e0) was designed to check for specific request methods: "GET", "HEAD", "DELETE", "OPTIONS", "PROPFIND", "SEARCH". If the request matched one of these methods, the request body would be set to empty. If it didn’t match, the body wouldn’t be modified. However, it seems the commit didn’t account for requests like POST, which can also have an empty body. Since POST wasn’t included in the predefined list, the request body wasn’t being set to empty when necessary.

Closes: https://github.com/cloudflare/cloudflared/issues/1337

[itsthejb]

You'll have to excuse my lack of experience with go here. Installed build-essential and golang on Debian Bookworm, make, but then:

GOOS=linux GOARCH=amd64  go build -mod=vendor  -ldflags='-X "main.Version=2022.12.1-281-g7a74b15f" -X "main.BuildTime=2024-10-18-1039 UTC" ' github.com/cloudflare/cloudflared/cmd/cloudflared
vendor/go.opentelemetry.io/otel/attribute/set.go:7:2: cannot find package "." in:
        /root/cloudflared-docker-multiarch/vendor/cmp
vendor/github.com/quic-go/quic-go/internal/wire/transport_parameters.go:10:2: cannot find package "." in:
        /root/cloudflared-docker-multiarch/vendor/slices
make: *** [Makefile:134: cloudflared] Error 1

Looks like I need to checkout out vendor dependencies first?

[maggie44]

Try running:

go mod vendor

[itsthejb]

Unfortunately still same prob. Here's go env:

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.19"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.19/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.8"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/root/cloudflared/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4223151333=/tmp/go-build -gno-record-gcc-switches"

[maggie44]

And that using the latest golang. If golang was installed with apt-get it’s probably out of date.

[itsthejb]

Ah yes - makes sense. apt version is 1.19, so somewhat old. Appears to be building no probs now. Thanks! Will report back shortly

[itsthejb]

Hi @maggie44,

Finally got it built and testing. However, as far as I can see this doesn't fix the issue, I'm afraid.

[maggie44]

There isn't much that changed in the last release by the looks of it: https://github.com/cloudflare/cloudflared/compare/2024.9.1...2024.10.0

Are you using quick tunnels or connecting with a config file?

[maggie44]

Worth double checking you are on the right branch too. If you cloned the fork of this called cloudflared-docker-multiarch, then selected the branch websocket rather than building directly off the main branch of cloudflared-docker-multiarch.

[itsthejb]

Yes, it's from the websocket branch:

root@cloudflared:~/cloudflared# git status
On branch websocket
Your branch is up to date with 'origin/websocket'.

nothing to commit, working tree clean

[itsthejb]

Yes, it is a quic tunnel. However this works fine with 2024.9.1:

Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF Starting tunnel tunnelID=d05945fa-7b50-4d35-aece-c2ddd54d4d2e
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF Version 2024.9.1
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF GOOS: linux, GOVersion: go1.22.2, GoArch: amd64
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF Settings: map[config:/etc/cloudflared/config.yml cred-file:/etc/cloudflared/d05945fa-7b5>
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF Generated Connector ID: 4f9c5ae2-1fbd-493a-8345-986fe57bdf2b
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF cloudflared will not automatically update if installed by a package manager.
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z WRN No ingress rules were defined in provided config (if any) nor from the cli, cloudflared >
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF Initial protocol quic
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF ICMP proxy will use 10.0.0.100 as source for IPv4
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF ICMP proxy will use :: as source for IPv6
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF Starting metrics server on 127.0.0.1:38677/metrics
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z INF You requested 4 HA connections but I can give you at most 2.
Oct 15 13:26:13 cloudflared cloudflared[158]: 2024-10-15T12:26:13Z WRN Your version 2024.9.1 is outdated. We recommend upgrading it to 2024.10.0

[maggie44]

From your first build attempt it logged "main.Version=2022.12.1-281-g7a74b15f". Couldn't understand why the date was off, but the commit seemed right, which made me wonder whether it was the right branch.

And the output of version should show something like

./cloudflared version
cloudflared version 2024.10.0-5-g7a74b15f (built 2024-10-18-1429 UTC)`. 

[itsthejb]

Since I'm in this context, I'm going to try bisecting to find the origin commit. This will take a little while

[maggie44]

Since I'm in this context, I'm going to try bisecting to find the origin commit. This will take a little while

Sounds like a good idea. You could undo each commit one at a time and test it. There aren't many commits between the last working version.

You can skip all the tests and just do the build with make cloudflared. Will speed things up a lot.

[itsthejb]

Alright, completed the bisect and can confirm that the breaking commit is e2064c820f32802f58baa027903043fe0ed052e0

[maggie44]

Probably narrows it to this then:

https://github.com/cloudflare/cloudflared/commit/e2064c820f32802f58baa027903043fe0ed052e0#diff-6ff9dc179d3cf0539f514218792ce1d93001e2b4835a695bde442ad45edf01deR562-R579

requestMethodUsuallyLacksBody looks new.

And this part using metadata:

	switch metadata[HTTPRequestBodyHintKey] {
	case RequestBodyHintEmpty.String():
		return true
	case RequestBodyHintHasData.String():
		return false
	default:
	}

[itsthejb]

Just double checked that simply reverting e2064c820f32802f58baa027903043fe0ed052e0 fixes the issue, but your patch does not. So, yes, looks like you're very much on the right track but not quite there.

I'll be available to try other patches. Just let me know

[maggie44]

The commit just added to this PR should give you your answer. Run it again and see what it prints to console.

[itsthejb]

Have done now. Currently have quite a lot running through the tunnel when it's up, so the output is quite busy:

Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  false
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  false
Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:02 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  false

I believe old behavior is to reply: false is when testing the Proxmox UI, but it's not easy to check. Is that enough to go on? I could spin up another tunnel to test more carefully, but that would be a little annoying to do

[maggie44]

Oct 18 16:21:03 cloudflared cloudflared[2738]: old behavior is to reply:  true
Oct 18 16:21:03 cloudflared cloudflared[2738]: request method is  false

Assuming it's not working still, as nothing changed other than the logging.

Looks like Proxmox is passing through a body in a type of request that doesn't usually have a body. Although web standards advise against it, it's not forbidden, so should probably be supported.

You can try refreshing this branch and trying again. It should work. And it should also print to console to confirm the method and body length.

[itsthejb]

Just built and checked: yes, it does indeed work 👍

Otherwise, I suppose this is really an xterm.js issue, since that's actually the implementation of the console. I suppose I could make an issue over there, although you may be better placed to explain what they're doing that might be a little non-standard?

[maggie44]

Just built and checked: yes, it does indeed work 👍

Otherwise, I suppose this is really an xterm.js issue, since that's actually the implementation of the console. I suppose I could make an issue over there, although you may be better placed to explain what they're doing that might be a little non-standard?

Anything in the console? 'length of body:' or 'method'?

[itsthejb]

So the issue is a GET with a body? Yes that is weird

Oct 18 16:50:25 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:25 cloudflared cloudflared[2836]: method:  HEAD
Oct 18 16:50:25 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:25 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:25 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:25 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:25 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:25 cloudflared cloudflared[2836]: method:  HEAD
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  450
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  POST
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  GET
Oct 18 16:50:27 cloudflared cloudflared[2836]: length of body:  0
Oct 18 16:50:27 cloudflared cloudflared[2836]: method:  POST

[itsthejb]

Still available to test anything for the time being. Or otherwise if you don't need any more, I'll restore from my backup and await the fixed version

[maggie44]

It's not actually the outcome I expected. It seems the requests are being sent as normal, but empty POST requests are what changed.

You could try again with the new commit here.

[itsthejb]

Just tested 49bb66b95ee54101abf77be9bf332c218ddf243b and it also works

[maggie44]

Since we were using Xterm, we ended up exploring WebSocket issues, which turned out to be unrelated.

It appears that the commit in the latest release was designed to check for specific request methods: "GET", "HEAD", "DELETE", "OPTIONS", "PROPFIND", "SEARCH". If the request matched one of these methods, the request body would be set to empty. If it didn’t match, the body wouldn’t be modified. However, it seems the commit didn’t account for requests like POST, which can also have an empty body. Since POST wasn’t included in the predefined list, the request body wasn’t being set to empty when necessary.

Great job with the debugging!

[itsthejb]

Great stuff! And thanks for taking this @maggie44

[maggie44]

Fixed for now by reverting the original commit: https://github.com/cloudflare/cloudflared/commit/f407dbb71253a3320b2e54e0ce33a18895d673f9