[Http][DI] Replace REMOTE_ADDR in trusted proxies with the current REMOTE_ADDR

[mcfedr]

Q	A
Branch?	4.4
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT
Doc PR

Currently handling trusted ips when deploying behind some CDNs/Load balancers such as ELB is difficult because they dont have a constant IP address, its possible to overcome this as is suggested by the docs - https://symfony.com/doc/current/deployment/proxies.html#but-what-if-the-ip-of-my-reverse-proxy-changes-constantly - by settings trusted proxies to $request->server->get('REMOTE_ADDR') - but this has to be done in code, and so becomes dangerous if you code is deployed in different environments.

This change would allow the developer to stick to providing the envvar TRUSTED_PROXIES, and in the environment behind a ELB set the value to the literal string REMOTE_ADDR, and have it replaced at run time. This way in environments that are not using ELB his app is kept safe.

I think doing this replacement in Request:: setTrustedProxies is the best place because it means this feature isn't exposed to other parts of the code that might call Request::getTrustedProxies.

[nicolas-grekas]

I don't think you need this: use '0.0.0.0/0' instead and that should provide what you want.
I think we should update the documentation instead. Would you be up to submit a PR there?

[stof]

@nicolas-grekas this is not equivalent. Proxies can be chained. Trusting REMOTE_ADDR is trusting the first level of proxy. Trusting the whole web allows anyone to spoof their IP address (as they can always send a Forwarded header to pretend that their actual IP is a proxy one)

[stof]

@mcfedr can you check whether your ELB proxies appears as an remote address belonging to the private IP range ? If yes, maybe you could trust the private IP ranges for your use case (though I don't know how isolated your AWS instances are when not using an AWS VPC).

[nicolas-grekas]

Trusting REMOTE_ADDR is trusting the first level of proxy. Trusting the whole web allows anyone to spoof their IP address

you may be right, but I'm missing the why:

    public function isFromTrustedProxy()
    {
        return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies);
    }

This only reads the direct REMOTE_ADDR. When does the difference you describe apply?

[stof]

@nicolas-grekas getClientIp cares about chained proxies. isFromTrustedProxy indeed only cares about the last hop. But that's not the only usage of Request::$trustedProxies.

[nicolas-grekas]

getClientIp uses isFromTrustedProxy, so looks safe to me. Do you have a specific example of what I'm missing?

[stof]

@nicolas-grekas it also uses $this->getTrustedValues when you are from a trusted proxy, which is exactly the case I'm talking about.
Trusting 0.0.0.0/0 will consider any (IPv4) value as trusted.

[mcfedr]

The chain is the problem with using 0.0.0.0/0 because the CDN/LB makes a request from a public IP address (in the case of cloudfront its from this massive range https://ip-ranges.amazonaws.com/ip-ranges.json that changes quite often) - so I always want to trust the first level, but if the client request to cloudfront already contains a x-forwarded-for header cloudfront just extends it, so if i put 0.0.0.0/0 as trusted - i've automatically trusted the proxy before cloudfront as well. (I've firewalled the servers so only CF can make requests to them)

[nicolas-grekas]

if the client request to cloudfront already contains a x-forwarded-for header cloudfront just extends it

Oh, OK! Now I get the issue, it's related to getTrustedValues calling normalizeAndFilterClientIps itself using self::$trustedProxies. Thanks for your explanations and patience :)

[nicolas-grekas]

(with a minor CS suggestion)

[mcfedr]

All done.

[fabpot]

Thank you @mcfedr.