mccutchen / go-httpbin

A reasonably complete and well-tested golang port of httpbin, with zero dependencies outside the go stdlib.
https://httpbingo.org
MIT License
596 stars 124 forks source link

feat: expose actual remote address #175

Open james-callahan opened 6 months ago

james-callahan commented 6 months ago

I was hoping to use go-httpbin today to figure out what the ip of my load balancer actually comes through as, but it was masked by the cloudflare CDN the request went through

Could you return the .RemoteAddr field somewhere in the response?

Code https://github.com/mccutchen/go-httpbin/blob/00a70e7c4c2f26d8cef0397d0e99f721ad97271c/httpbin/helpers.go#L44-L66

mccutchen commented 6 months ago

Ah, so you'd like go-httpbin sitting behind your load balancer to report the IP address of the load balancer itself, rather than make an attempt to return the actual client IP address? That's interesting, I'd be curious to hear more about your use case here if you're willing and able to share!

I'm reluctant to enable something like this by default, as it would expose potentially sensitive details about the network and infrastructure where an instance of go-httpbin is deployed, but if you or someone else opened a pull request adding a new, optional /remote-addr endpoint (or something along those lines), I'd consider it.

I'd probably want it modeled after the /hostname endpoint added in https://github.com/mccutchen/go-httpbin/pull/81, which returns a dummy value by unless a deployment explicitly enables returning the real hostname in its configuration. (That pull request is a bit noisier than it should be, but https://github.com/mccutchen/go-httpbin/pull/81/commits/1334011060844565b8d309ccf0f9d17048d5e737 would be a good starting point for anyone interested in making this kind of change!)

james-callahan commented 6 months ago

Ah, so you'd like go-httpbin sitting behind your load balancer to report the IP address of the load balancer itself, rather than make an attempt to return the actual client IP address? That's interesting, I'd be curious to hear more about your use case here if you're willing and able to share!

I was trying to figure out what the load balancer(s) were: my thought to debug/discover the system was to throw up a go-httpbin image using the existing CI pipelines, and see where the connections were coming from

I'm reluctant to enable something like this by default, as it would expose potentially sensitive details about the network and infrastructure where an instance of go-httpbin is deployed, but if you or someone else opened a pull request adding a new, optional /remote-addr endpoint (or something along those lines), I'd consider it.

Isn't that usually already available in X-Forwarded-For? At least in the case of multiple reverse proxies. Only the most recent hop would be missing.


I also note that at the moment the client can easily fake their IP as long as you aren't behind fly.io (the first preference in the code) e.g. curl https://my-go-httpbin.internal/get -H "Fly-Client-IP: 4.5.6.7.8" go-httpbin doesn't have any sort of "trust these IPs when they present a forwarded-for header" (and for many cases, an IP list isn't enough!)

mccutchen commented 6 months ago

Isn't that usually already available in X-Forwarded-For? At least in the case of multiple reverse proxies. Only the most recent hop would be missing.

Ha, yes, that's a good point! But doesn't that also eliminate the need for adding a new endpoint?

# original example:
# $ curl -s https://httpbingo.org/headers | jq -r '.headers["X-Forwarded-For"][0]' | awk '{print $NF}'

# slightly more robust/correct example:
$ curl -s https://httpbingo.org/headers | jq -r '.headers["X-Forwarded-For"][0]' | awk -F, '{print $NF}' | tr -d ' '
66.241.125.232

I also note that at the moment the client can easily fake their IP as long as you aren't behind fly.io (the first preference in the code) e.g. curl https://my-go-httpbin.internal/get -H "Fly-Client-IP: 4.5.6.7.8"

Sure, but in this case, the client is only tricking itself, at least in terms of getting "risky" output out of go-httpbin.

The other downside is that the go-httpbin server might log the incorrect remote addr in its logs, but if that's a real concern operators can deploy a "hardened" go-httpbin instance that adds middleware to prevent it.

It would probably be reasonable to add an option to control this, if anyone is interested in making the effort. It would also probably be reasonable to document this potential downside in the Production Considerations section of the README.

james-callahan commented 6 months ago

Only the most recent hop would be missing.

Ha, yes, that's a good point! But doesn't that also eliminate the need for adding a new endpoint?

The most recent hop is missing, i.e. the remoteaddr as seen by go-httpbin.

Sure, but in this case, the client is only tricking itself, at least in terms of getting "risky" output out of go-httpbin.

I'm thinking of a malicious or misconfigured load balancer where the wrong e.g. Fly-Client-IP header is getting added

mccutchen commented 6 months ago

The most recent hop is missing, i.e. the remoteaddr as seen by go-httpbin.

👍 ah yeah, you're right, my mistake. In this case, I'm back to thinking that we could add an additional endpoint here, but that it should be opt-in rather than opt-out.

Sure, but in this case, the client is only tricking itself, at least in terms of getting "risky" output out of go-httpbin.

I'm thinking of a malicious or misconfigured load balancer where the wrong e.g. Fly-Client-IP header is getting added

Maybe I'm missing something, but the use case of deploying a go-httpbin on untrusted/malicious infrastructure is not something I'm particularly worried about; if it's a real concern, I'd be in favor of adding an option to control how exactly go-httpbin determines the "client address" (e.g. "use header Fly-Client-Ip" or "trust these hops in X-Forwarded-For").