[maki5]

replaced deprecated chi RealIP middleware with ClientIPFromRemoteAddr and ClientIPFromXFF as a fallback option for proxy

closes ##3082
closes https://github.com/opencloud-eu/opencloud/security/dependabot/258

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 50 complexity

Metric	Results
Complexity	50

View in Codacy

🟢 Coverage 77.60% diff coverage

Metric	Results
Coverage variation	Report missing for 6bfce081
Diff coverage	✅ 77.60% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (6bfce08)	Report Missing	Report Missing	Report Missing
Head commit (b7bb4fb)	92747	25530	27.53%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3464)	125	97	77.60%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

¹ Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[rhafer]

Hm, reading through https://github.com/go-chi/chi/blob/master/README.md#choosing-a-clientip-middleware, go-chi/chi#708 and other issues linked from there, this one is a becoming a bit of a can of worms.

Apparently there is no real one-size-fits-all replacement for the RealIP middleware, depending on the concrete setup different middlewares from the ClientIPFrom... family have to be used and additional configuration would be needed.

For our proxy services:

• When the proxy is directly exposed the ClientIPFromRemoteAddr could be used
• It the used reverse proxy unconditionally set and overwrites a specific header with the client IP it would be ClientIPFromHeader
• If the IPs of all involved reverseproxys are know ClientIPFromXFF
• If the number of trusted reverseproxy hops between the client and our proxy service is know ClientIPFromXFFTrustedProxies

For all other services that used the RealIP middleware it would be good if our proxy service unconditionally adds and overwrites a header with the value resulting from the above ClientIPFrom... middlewares. The "underlying" services could then just use ClientIPFromHeader

And even with the above there will be case where we would not be able to determine the correct remote IP address.

[maki5]

@rhafer if I got it correctly we need an additional config where the ip "extraction" strategy will be specified, based on the value from config we apply the correct chi middleware and through it extract the real ip address. Did I got it right?

[rhafer]

@rhafer if I got it correctly we need an additional config where the ip "extraction" strategy will be specified, based on the value from config we apply the correct chi middleware and through it extract the real ip address. Did I got it right?

Yes, exactly. I wonder however what a good config would look like that fits the above suggesting without getting to complicated.

Any suggestions?

@micbar @butonic maybe you want to chime in here as well as you have more insights into possible setups, especially when it comes to k8s setups (where multiple reverse proxys a probably not an exception)

[butonic]

Agreed, there is no single ClientIPFrom* we can hardcode. But we don't need the admin to pick a chi middleware either. Reverse proxies already answer this with a list of trusted peers: nginx has set_real_ip_from, traefik has forwardedHeaders.trustedIPs. We can do the same and choose the chi middleware per request based on RemoteAddr:

// ClientIP resolves the client IP. Forwarding headers are only honored when the
// peer is a trusted proxy, so clients connecting directly cannot spoof their IP.
func ClientIP(trustedProxies []string, header string) func(http.Handler) http.Handler {
	trusted := make([]netip.Prefix, 0, len(trustedProxies))
	for _, p := range trustedProxies {
		trusted = append(trusted, netip.MustParsePrefix(p)) // validated when parsing the config
	}

	fromForwarded := chimiddleware.ClientIPFromXFF(trustedProxies...)
	if http.CanonicalHeaderKey(header) != "X-Forwarded-For" {
		fromForwarded = chimiddleware.ClientIPFromHeader(header)
	}

	return func(next http.Handler) http.Handler {
		direct := chimiddleware.ClientIPFromRemoteAddr(next)
		// a trusted proxy that sends no usable header (e.g. health checks) falls back to the peer address
		fallback := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			if chimiddleware.GetClientIPAddr(r.Context()).IsValid() {
				next.ServeHTTP(w, r)
				return
			}
			direct.ServeHTTP(w, r)
		})
		forwarded := fromForwarded(fallback)

		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			if peerTrusted(r.RemoteAddr, trusted) {
				forwarded.ServeHTTP(w, r)
				return
			}
			direct.ServeHTTP(w, r)
		})
	}
}

// peerTrusted reports whether the peer that opened the connection is a trusted proxy.
func peerTrusted(remoteAddr string, trusted []netip.Prefix) bool {
	host, _, err := net.SplitHostPort(remoteAddr)
	if err != nil {
		host = remoteAddr // RemoteAddr may already be a bare IP
	}
	ip, err := netip.ParseAddr(host)
	if err != nil {
		return false
	}
	// fold v4-mapped v6 and strip zones, otherwise Prefix.Contains does not match
	ip = ip.Unmap().WithZone("")
	for _, p := range trusted {
		if p.Contains(ip) {
			return true
		}
	}
	return false
}

The config then becomes PROXY_TRUSTED_PROXIES (CIDR list) plus an optional PROXY_CLIENT_IP_HEADER (default X-Forwarded-For, or e.g. CF-Connecting-IP), instead of the strategy enum. I'd drop xff_trusted_hops. chi itself recommends prefixes, and both XFF variants ignore RemoteAddr. With the current xff strategy, anyone who can reach the proxy port directly controls the whole chain.

Open question is the default:

a) empty list. That is the same as remote_addr: secure, but behind traefik we will log the traefik IP, which breaks fail2ban style setups
b) loopback + private ranges (127.0.0.0/8,::1/128,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,fc00::/7). This keeps the current behaviour for container deployments, and clients hitting us directly from the internet still cannot spoof (I'd prefer this)

Some things the LLM noticed in the downstream part of the PR:

• the proxy only sets X-Client-Ip when it resolved an IP. If it didn't, a client supplied X-Client-Ip reaches the backends unchanged. We need to Del it in that case.
• SetXForwarded() in https://github.com/opencloud-eu/opencloud/blob/main/services/proxy/pkg/router/router.go#L202 now writes the traefik IP to X-Forwarded-For, because RemoteAddr is no longer rewritten. reva's utils.GetClientIP reads XFF raw for OCM invites and shares, so we should overwrite XFF with the resolved IP after routing. Then the internal services can just use ClientIPFromXFF() without args, which is the "exactly one trusted hop" case.
• collaboration does not sit behind the OpenCloud proxy. traefik routes to it directly, see https://github.com/opencloud-eu/opencloud/blob/main/devtools/deployments/opencloud_full/collabora.yml#L47-L50, so ClientIPFromHeader("X-Client-Ip") is spoofable there. It needs the same trusted proxies middleware and its own config (or a shared OC_TRUSTED_PROXIES).

@rhafer @maki5 opinions?

[micbar]

@maki5 I pushed a commit on top that follows the suggestion from @butonic in #3464 (comment). The client IP part comes from #3864 where I needed the same
for the audit log.

• trusted proxies instead of the strategy enum, default loopback and private ranges
• your header option stays as PROXY_CLIENT_IP_HEADER
• X-Forwarded-For to the services instead of X-Client-Ip
• collaboration gets the same check

@rhafer please review again 😄

[micbar]

I pushed a fix for the collaboration service special case.

Before: The collaboration service behind the proxy used the opencloud proxy as the last trusted proxy. So the collaboration logs got the localhost.

[micbar]

LGTM from my POV.

Needs a second approval because i added some commits on my own.