caddyhttp: Determine real client IP if trusted proxies configured

[francislavoie]

Followup on #5103 and #5328.

This introduces first-class support for determining the client IP, at the HTTP server level. Also adds a placeholder and writes it to the access logs.

I'm marking this as a draft for now, because this has a certain level of risk, and needs discussion. We need to be extra careful about the implementation.

I'm taking @adam-p's guidance on this from his excellent article https://adam-p.ca/blog/2022/03/x-forwarded-for/. There's a few different valid approaches to this. This implementation takes the left-most, first valid IP. I'm not excluding private IPs, because actually it happens frequently that users want their LAN IP to be used as the real client IP, in home networking contexts.

Currently, the header used is not configurable, I have it just set to X-Forwarded-For which is typically good enough, but I think to be robust we'll need to make that configurable. Do we only allow a single header field, or do we allow a fallback list? I implemented this as a fallback list of headers. This is configured on the server itself, alongside the trusted_proxies.

Another open question (also applicable to #5103, maybe we should figure this out before merging that), should we make trusted IP sources pluggable? We did make it pluggable in #5328. We provide a "static" module by default. The argument for that is that proxies like Cloudflare have a ton of IP ranges, and users managing that in their config is annoying. So a plugin module would be able to periodically fetch and cache their IP list.

I decided that the client_ip should always be set (as long as the RemoteAddr is a valid IP address). So in the logs, it will be identical to the remote_ip unless trusted proxies is configured and the proxy passed the client IP in a trusted header.

[francislavoie]

I think this is essentially good to go. Un-drafting.

[supriyo-biswas]

@francislavoie Thank you for your work on this feature, is there any way I can help in getting this PR merged?

[francislavoie]

I had a few other details I wanted to look at first, haven't had time yet.

I need to think about changes to the remote_ip matcher, we need to deprecate (or remove immediately) forwarded support since it's inherently insecure, maybe add a client_ip matcher instead which uses the client IP from this PR instead.

I might need to adjust the type of the client_ip var.

[francislavoie]

Okay, I think I'm done now?

• I've implemented a client_ip matcher, which pulls from the trusted client IP, or the remote IP address as fallback; safer option than remote_ip forwarded.
• I refactored the IP matching code into a separate file since it started getting pretty chunky inside of matchers.go.
• I've marked remote_ip's forwarded as deprecated.

[mholt]

Wow, this is impressive.

Thank you for working on this, I think it will provide a lot of value and added security for Caddy services.

[robgordon89]

Wow, @francislavoie this works amazingly, tested it fully and also with the "Rate Limit" plugin and it works just perfectly.

Thanks so much for working on this 🥇

[alwismt]

Tried! Just wow @francislavoie 🤗 working fine, it was a really necessary requirement when we do "Rate Limit". Waiting for official release 😊

{
	servers {
		# all traffic come from cloudflare cdn
		trusted_proxies cloudflare
	}
	order rate_limit first
}
sg4.sitefire.com {
	# forward request to golang backend
	reverse_proxy [::]:8080
	# limit reqs
	rate_limit {
		zone dynamic {
			# real client ip
			key {client_ip}
			events 100
			window 1m
		}
	}
	encode gzip
	# just for verify that client_ip works or nor
	respond {client_ip}
}

[mholt]

2.7 beta 1 will be coming soon :)