8000 [PROPOSAL][RFC] Enhance trusted_proxies to accept DNS names · Issue #7765 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PROPOSAL][RFC] Enhance trusted_proxies to accept DNS names #7765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
derekclapham opened this issue Apr 21, 2013 · 7 comments
Closed

[PROPOSAL][RFC] Enhance trusted_proxies to accept DNS names #7765

derekclapham opened this issue Apr 21, 2013 · 7 comments
Labels
HttpFoundation

Comments

@derekclapham
Copy link

We are currently in the process of deploying a big Symfony 2.1 application and we've hit a minor snag. The application runs in a cluster of application servers behind 2 or more HAProxy load balancers.

All the servers are Amazon EC2 instances (including the HAProxy load balancers) whose internal IP addresses can change regularly. As a result, managing the "trusted_proxies" array is becoming a bit of a nightmare. At any one time we may have 10 app servers that would need their trusted_proxies config array updated with the new IP addresses of any newly provisioned HAProxy instances.

So, in the short term we've had to use the deprecated "trust_proxy_headers" parameter.

framework:
    trust_proxy_headers: true
    trusted_proxies: ~

I would like to propose that the trusted_proxies parameter be able to handle DNS names. This way we could dynamically update our DNS zone file each time a new load balancer was brought in to the mix.

I'm interested in getting peoples thoughts on the various implications of adding this feature? How would it look from a config perspective? Obviously security and performance need to be taken in to consideration. I've pasted some ideas below:

Option 1:

framework:
    trusted_proxies: [lb-01.mydomain.com, lb-02.mydomain.com]

Option 2:

The idea here being that by flagging the proxy entries as domains, the HttpKernel does not need to ascertain whether it's dealing with domain names or IP addresses (although it is a pretty simple regex)

framework:
    trusted_proxy_lookup: true
    trusted_proxies: [lb-01.mydomain.com, lb-02.mydomain.com]

Option 3:

This is a mixed approach where the HttpKernel would be responsible for determining whether an entry needed an address lookup (gethostbyname) or not. In my opinion this is the most flexible option as it allows for a mix of IPs and DNS names

framework:
    trusted_proxies: [10.1.20.244, lb-02.mydomain.com]

Obviously gethostbyname opterations are expensive but I can't think of any other way around it.

I'm keen to hear peoples thoughts on the above.
@fabpot
Copy link
Member
fabpot commented Apr 21, 2013

We've just adding support for IP ranges, which should be enough and don't have the performance impact that DNS resolving has.

@lazyhammer
Copy link
Contributor

@fabpot
Copy link
Member
fabpot commented Apr 21, 2013

We could handle resolution only once at the container compilation time. Then one can add something like proxy-cluster.mydomain.com to the proxy list and be able to change the list of trusted IPs without touching the configuration. And for those who need dynamic resolution, the option could be added to allow lookups on every request.
@fabpot, what do you think?

@lazyhammer I'd like to avoid adding more complexity if IP ranges work.

@derekclapham
Copy link
Author

@fabpot One of the issues I see with IP ranges is that in the case of EC2, private IP address are given according to RFC 1918 which is 10.0.0.0/8 (CIDR) that's over 16m possible addresses. For the ultra security conscious, this isn't the most secure option.

I suppose the saving grace with EC2 is that I can set up a security group so that the instance itself is protected from every other EC2 instance on their network.

@fabpot
Copy link
Member
fabpot commented Apr 21, 2013

And you can do the resolution yourself before configuring the trusted proxies:

$ip = gethostbyname('example.com');
Request::setTrustedProxies(array($ip));

@derekclapham
Copy link
Author

@fabpot I have given this a bit of thought and have come up with a workaround. I'm mostly happy with it but I have a question that I can't for the life of me answer for myself.

We use GeoIP functionality quite heavily in our app (2.1.10-DEV). We are passing our GeoIP library the client's IP address using the Request::getClientIp() method. The way we are setting our trusted_proxies is in our front controller using:

Request::setTrustedProxies([$request->server->get('REMOTE_ADDR')]);

The catch is, we're also using APC (ie: HttpCache). The HttpCache class rewrites the $_SERVER['REMOTE_ADDR'] value to 127.0.0.1 after it has added the original to the X-Forward-For header chain. This is all fine I understand the logic here.

However, the very first line in Request::getClientIp() is:

$ip = $this->server->get('REMOTE_ADDR');

My issue is that unless you add 127.0.0.1 to your trusted_proxies array, getClientIp will always return 127.0.0.1. Basically when the array_diff happens at the end of the method:

$clientIps = array_diff($clientIps, $trustedProxies);

127.0.0.1 will always be the last element in the array and the return:

return $clientIps ? array_pop($clientIps) : $ip;

will always return 127.0.0.1 regardless of whether the $clientIps condition fails or not.

The only way around this behaviour is to add 127.0.0.1 to the list of trusted proxies. Is this the intended logic? Obviously without HttpCache all is fine.

Am I missing something? Or are we to treat HttpCache as a trusted proxy?

@fabpot
Copy link
Member
fabpot commented Jun 13, 2013

To answer your question, yes, HttpCache acts as any other proxy, so it should be trusted explicitely.

Closing this ticket as there are ways to use DNS names instead of IP as explained in the comments. Adding support for DNS names would add complexity and would have a performance impact (doing the resolution once when the container is compiled would defeat the purpose of the change).

@fabpot fabpot closed this as completed Jun 13, 2013
@samuelvogel samuelvogel mentioned this issue Oct 14, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fabpot @lazyhammer @derekclapham
0