Skip to content
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.

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

Laravel Trust Proxies * not working as expected with getClientIp() method of Request. #49371

Closed
iamriajul opened this issue Dec 14, 2023 · 12 comments

Comments

@iamriajul
Copy link

Laravel Version

10.24

PHP Version

8.2

Database Driver & Version

No response

Description

Suppose you have a chain of Proxies/Load Balancers for your application.
Now when I set $proxies = '*'; in the TrustProxies middleware, It should trust all the Load Balancers, But it doesn't.
In my case, I have:

  • CloudFront -> Traefik -> Docker Swarm -> Nginx -> Laravel Octane Application.

As per docs of getClientIp(), it says:

Returns the client IP address.
This method can read the client IP address from the "X-Forwarded-For" header when trusted proxies were set via "setTrustedProxies()". The "X-Forwarded-For" header value is a comma+space separated list of IP addresses, the left-most being the original client, and each successive proxy that passed the request adding the IP address where it received the request from.
If your reverse proxy uses a different header name than "X-Forwarded-For", ("Client-Ip" for instance), configure it via the $trustedHeaderSet argument of the Request::setTrustedProxies() method instead.

Notice the statement the left-most being the original client bolded above. As per this when I've X-Forwarded-For with values like ["1**. 1**. 1**. 1**", 2**. 2**. 2**. 2**, 3**. 3**. 3**. 3**, 4**. 4**. 4**. 4**, 5**. 5**. 5**. 5**], The left most being "1**. 1**. 1**. 1**" should be the result of getClientIp() but it doesn't instead it just trusts the last proxy (5**. 5**. 5**. 5**) and think all the others (1, 2, 3, 4) as Client Ips and it doesn't just stop there but it returns 4**. 4**. 4**. 4** when I call getClientIp().

As per the docs (https://laravel.com/docs/10.x/requests#trusting-all-proxies) it says when when $proxies = '*' that means it will trust all proxies but it doesn't. As per my investigation I've found the Middleware only trust the last proxy when $proxies = '*'.
image
image

Steps To Reproduce

It's not reproducible using code.

@driesvints
Copy link
Member

I'm not sure I understand though. If the REMOTE_ADDR is the one coming in and is being trusted, why would you still need to trust all the others? This is set at runtime so it just accepts the REMOTE_ADDR coming in and that's it?

@driesvints
Copy link
Member

@iamriajul is the issue just that you don't get the correct IP? Because taking the first IP is exactly what getClientIp does:

https://github.com/symfony/symfony/blob/0d9562ff6cdda11c71f0eb2bce0076f0d3a8ea9f/src/Symfony/Component/HttpFoundation/Request.php#L755

If you need to inspect multiple forwarded IP's you need to use getClientIps:

https://github.com/symfony/symfony/blob/0d9562ff6cdda11c71f0eb2bce0076f0d3a8ea9f/src/Symfony/Component/HttpFoundation/Request.php#L732

@driesvints
Copy link
Member

I've sent in a PR for the ips method: laravel/docs#9196

@iamriajul
Copy link
Author

I think it's a bug, because when I add all proxies ip addresses (prefixes) to the &proxies property as array then the getClientIp() resolves to the correct user IP address (which is is the left most IP address in the X-Forwarded-For header).

@driesvints
Copy link
Member

@iamriajul does the discussion from #44271 help?

@iamriajul
Copy link
Author

When $proxies = '*', all proxies should be trusted. And about #44271 , it is about not forwarding the actual header, currently my proxies and nginx forward proper X-Forwarded-* headers to the application.

@iamriajul
Copy link
Author

Currently, I don't have any problem because I've configured to trust actual IP addresses of Load Balancers (It is recommended and secure), but for new users it might be an issue. Because isn't so much of automation for this kind of things, previously I only found package for Cloudflare Trust Proxies in the Laravel Community, but none for Cloudfront.

So I had to code it myself.
Here is my solution, Posting it here for now. Planning to maintain a new package for this kind TrustProxies issue.
Only works with Octane with Swoole.

TrustProxies.php

class TrustProxies extends Middleware
{
    protected $proxies = [
        '10.0.0.0/8' // Docker Swarm global scope (overlay) networks,
    ];

    public function handle(Request $request, Closure $next)
    {
        if ($request->hasHeader('x-amz-cf-id')) {
            // Remember Cloudfront proxies for a day
            $cloudfrontProxies = Cache::store('octane')->get('cloudfront_proxies', []);
            $this->proxies = array_merge($this->proxies, $cloudfrontProxies);
        } else if ($request->hasHeader('cf-ray')) {
            // Remember Cloudflare proxies for a day
            $cloudflareProxies = Cache::store('octane')->get('cloudflare_proxies', []);
            $this->proxies = array_merge($this->proxies, $cloudflareProxies);
        }

        return parent::handle($request, $next);
    }
}

AppProvider.php (or any other provider)

class CoreServiceProvider extends ServiceProvider
{
    public function boot()
    {
        /**
         * @var OctaneStore $octaneCache
         */
        $octaneCache = Cache::store('octane');

        // Update CloudFront Proxies every 1 hour
        $octaneCache->interval('cloudfront_proxies', function () {
            $response = Http::get('https://ip-ranges.amazonaws.com/ip-ranges.json');
            return collect($response->json()['prefixes'])
                ->filter(fn ($prefix) => Str::startsWith($prefix['service'], 'CLOUDFRONT'))
                ->map(fn ($prefix) => $prefix['ip_prefix'])
                ->values()
                ->toArray();
        }, seconds: 60 * 60);

        // Update CloudFlare Proxies every 1 hour
        $octaneCache->interval('cloudflare_proxies', function () {
            return LaravelCloudflare::getProxies();
        }, seconds: 60 * 60);

      // More code...
   }
}

@fideloper
Copy link
Contributor

fideloper commented Dec 20, 2023

So, my understanding of how trusted proxies should work is this:

The general way to get the real IP securely is to move "backwards" (Right to Left) through the IPs in the http-x-forwarded-for comparing each to the the trusted proxies list and then taking the next one as the actual IP.

So each proxy should, ideally, be in the list of trusted IP's, and Laravel/Symfony should iterate through each IP and see if it's a trusted proxy. As far as I can tell, Symfony does this (it's...complexish).

The problem is that we often don't know the IP of each proxy in the chain (thanks to public clouds, automated infrastructure, yadda yadda yadda).

I personally think the whole idea of a trusted proxy should be opt-in and we should choose to just trust the left-most item in the x-forwarded-for header if we want to. Symfony (and I'm sure others) leans the other way in that opinion.

Taylor's initial idea of making a more complex case be up to the user to implement makes sense, rather than having Laravel take this on. It's not an easy problem necessarily, and there's security trade-offs to be made.

Here's my custom middleware that "fixes" this for me in a way I'm happy enough about. This adjusts the stock middleware and over-rides protected function setTrustedProxyIpAddressesToTheCallingIp()

<?php

namespace App\Http\Middleware;

use Illuminate\Http\Middleware\TrustProxies as Middleware;
use Illuminate\Http\Request;

class TrustProxies extends Middleware
{
    /**
     * The trusted proxies for this application.
     *
     * @var array<int, string>|string|null
     */
    protected $proxies = "*";

    /**
     * The headers that should be used to detect proxies.
     *
     * @var int
     */
    protected $headers =
        Request::HEADER_X_FORWARDED_FOR |
        Request::HEADER_X_FORWARDED_HOST |
        Request::HEADER_X_FORWARDED_PORT |
        Request::HEADER_X_FORWARDED_PROTO |
        Request::HEADER_X_FORWARDED_AWS_ELB;

    /**
     * Overriding the core functionality
     * @param Request $request
     * @return void
     */
    protected function setTrustedProxyIpAddressesToTheCallingIp(Request $request)
    {
        // Add REMOTE_ADDR as a trusted proxy
        $trustedIps = [$request->server->get('REMOTE_ADDR')];
        // Parse x-forwarded-for IP addresses
        $forwardedIps = collect(explode(',', $request->headers->get('x-forwarded-for')))
            ->map(function($item) {
                return trim($item);
            })->filter()->toArray();

        // Remove real client IP (left-most IP address, and the first item in this array),
        // leaving behind any proxy IP addresses
        if (count($forwardedIps) > 1) {
            array_shift($forwardedIps);
        }

        $request->setTrustedProxies(array_merge($trustedIps, $forwardedIps), $this->getTrustedHeaderNames());
    }
}

I'm not totally sure I parsed x-forwarded-for in a way that's bug-free, and it definitely gives a middle finger to the idea of trusted proxies. That's the trade off I make on a header that's already full of trade-offs and isn't really "perfect" for security (but is "forced" on us by underlying Symfony HTTP setup with getting the client IP address).

I hope that doesn't come off as negative to Symfony, the work everyones done there is amazing.

@driesvints
Copy link
Member

Hi @iamriajul. It seems we can conclude that this is more a Symfony issue than a Laravel issue so feel free to open an issue on their tracker, thanks!

@ekonoval
Copy link

ekonoval commented Apr 9, 2024

Instead of
protected $proxies = '*'; // not working
you can try
protected $proxies = '0.0.0.0/0'; // working

For some reason it worked for me, though I don't see it documented anywhere...

@ohmydevops
Copy link

Instead of protected $proxies = '*'; // not working you can try protected $proxies = '0.0.0.0/0'; // working

For some reason it worked for me, though I don't see it documented anywhere...

Worked for us!

@oguzhankrcb
Copy link

Instead of protected $proxies = '*'; // not working you can try protected $proxies = '0.0.0.0/0'; // working

For some reason it worked for me, though I don't see it documented anywhere...

Worked for me too, thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants