laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.54k stars 11.02k forks source link

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

Closed iamriajul closed 10 months ago

iamriajul commented 11 months ago

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:

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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

I've sent in a PR for the ips method: https://github.com/laravel/docs/pull/9196

iamriajul commented 11 months ago

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 commented 11 months ago

@iamriajul does the discussion from https://github.com/laravel/framework/issues/44271 help?

iamriajul commented 11 months ago

When $proxies = '*', all proxies should be trusted. And about https://github.com/laravel/framework/issues/44271 , it is about not forwarding the actual header, currently my proxies and nginx forward proper X-Forwarded-* headers to the application.

iamriajul commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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 commented 7 months ago

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 commented 3 months ago

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 commented 2 weeks ago

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!