Request ips wrong order when using a load balancer

[peppeocchi]

Hi, I recently upgrade one application from Laravel 5.2 to Laravel 5.7
Before the upgrade I was using this library (v3.3) with the configuration below

    'proxies' => '**',
...
    'headers' => [
        (defined('Illuminate\Http\Request::HEADER_FORWARDED') ? Illuminate\Http\Request::HEADER_FORWARDED : 'forwarded') => null,
        Illuminate\Http\Request::HEADER_CLIENT_IP    => 'X_FORWARDED_FOR',
        Illuminate\Http\Request::HEADER_CLIENT_HOST  => null,
        Illuminate\Http\Request::HEADER_CLIENT_PROTO => 'X_FORWARDED_PROTO',
        Illuminate\Http\Request::HEADER_CLIENT_PORT  => 'X_FORWARDED_PORT',
    ]

After the upgrade, I added the TrustProxies middleware below

<?php

namespace App\Http\Middleware;

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

class TrustProxies extends Middleware
{
    /**
     * The trusted proxies for this application.
     *
     * @var array
     */
    protected $proxies = '**';

    /**
     * The headers that should be used to detect proxies.
     *
     * @var int
     */
    protected $headers = Request::HEADER_X_FORWARDED_AWS_ELB;
}

The issue I am having might not be related to this, but basically after the upgrade

$request->ip() // the ec2 ip
$request->ips() // [ec2 ip, client ip]

It seems the order or the ips is inverted, I should be getting first the client ip, then any other ip.

Is this a configuration issue on my side or is there anything else I am missing?

[fideloper]

Hi!

To my knowledge, nothing in this package would change that behavior. At the end of the day, this package's responsibility is purely calling $request->setTrustedProxies(). It's possible that affects something in the underlying Symfony classes (they also provide this functionality). However it might just be the order Symfony puts the Ip() if multiple are given anyway!

I'm guessing you'll get the same when calling the following (which I think the Laravel methods you used just wrap):

$request->getClientIp(); 
$request->getClientIps();

This might be a bug to file upstream in Symfony. The code I'm seeing in Symfony\Component\HttpFoundation is indeed grabbing the first IP:

    public function getClientIp()
    {
        $ipAddresses = $this->getClientIps();

        return $ipAddresses[0];
    }

[fideloper]

The cal to getClientIps() seems a little sketchy to me also:

public function getClientIps()
    {
        $ip = $this->server->get('REMOTE_ADDR');

        if (!$this->isFromTrustedProxy()) {
            return array($ip);
        }

        return $this->getTrustedValues(self::HEADER_X_FORWARDED_FOR, $ip) ?: array($ip);
    }

I'm not sure why that's calling self::HEADER_X_FORWARDED_FOR explicitly when there are options to use other headers. Could be a bug, but their code around this has always been obtuse.

[NFarrington]

This behaviour is expected when updating from TrustedProxy v3.3 to v4+, as the meaning of ** has been removed (1d09591#diff-04ebeb9e366e3ee7a4bd392a708e21bcL85), and made synonymous with * (9618897#diff-04ebeb9e366e3ee7a4bd392a708e21bcR76). This means from v4+, only the REMOTE_ADDR is trusted when using **, rather than all the proxy IPs in the chain.

The Symfony getClientIps() returns the "most trusted" proxy first, not the first IP in the chain:

     * In the returned array the most trusted IP address is first, and the
     * least trusted one last. The "real" client IP address is the last one,
     * but this is also the least trusted one. Trusted proxies are stripped.

To have getClientIp() return the original IP, all proxies in the chain must be trusted. To replicate the old behaviour of **, set $proxies = ['0.0.0.0/0', '2000:0:0:0:0:0:0:0/3'].

[mailtomsk]

This behaviour is expected when updating from TrustedProxy v3.3 to v4+, as the meaning of ** has been removed (1d09591#diff-04ebeb9e366e3ee7a4bd392a708e21bcL85), and made synonymous with * (9618897#diff-04ebeb9e366e3ee7a4bd392a708e21bcR76). This means from v4+, only the REMOTE_ADDR is trusted when using **, rather than all the proxy IPs in the chain.

The Symfony getClientIps() returns the "most trusted" proxy first, not the first IP in the chain:

     * In the returned array the most trusted IP address is first, and the
     * least trusted one last. The "real" client IP address is the last one,
     * but this is also the least trusted one. Trusted proxies are stripped.

To have getClientIp() return the original IP, all proxies in the chain must be trusted. To replicate the old behaviour of **, set $proxies = ['0.0.0.0/0', '2000:0:0:0:0:0:0:0/3'].

This is worked perfectly. Thanks a lot..

[barryvdh]

Thanks. I had the same problem with Laravel Vapor.

[BadChoice]

This really helped!! It was driving me crazy as the api rating was not working but everything else did

[bytestream]

I think the README needs updating because otherwise this is painful...

1. The code says ** is deprecated yet the README says to use it
2. As pointed out above ** doesn't do what it says in the README, and you need to use the workaround mentioned

[fideloper]

I’ll redo the readme soon to clear things up. Hard to find time lately (kids!) but it’s on my radar!

…

On Sat, Apr 25, 2020 at 15:12 Kieran ***@***.***> wrote: I think the README needs updating because otherwise this is painful... 1. The code says ** is deprecated yet the README says to use it 2. As pointed out above ** doesn't do what it says in the README, and you need to use the workaround mentioned — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#115 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSDUZS6B73IZZY36IHBMDROM73HANCNFSM4GCUUHPQ> .

[bytestream]

I'd be happy to send a PR if you just want to replace ** with the above fix :) if you've got larger changes in mind then no worries!

[fideloper]

definitely larger changes in mind! I also need to do directions per version and make it more clear what versions are included in laravel out of the box, etc

…

On Sun, Apr 26, 2020 at 10:19 Kieran ***@***.***> wrote: I'd be happy to send a PR if you just want to replace ** with the above fix :) if you've got larger changes in mind then no worries! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#115 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSDU5B6DJFNEXOW4MLJJTRORGJLANCNFSM4GCUUHPQ> .

[fideloper]

Took a first stab at a simpler/better readme.md but PR's welcome for formatting/explanation changes.