Remote Addr are not handled the same since akrabat/ip-address-middleware >= 2.5

[ymage]

Shlink version

version >=4.4.0

PHP version

8.3

How do you serve Shlink

Self-hosted RoadRunner

Database engine

MySQL

Database version

11.6

Current behavior

Shlink run in docker container which is reverse-proxified with traefik/nginx.
X-Real-IP and X-Forwarded-For are both correctly set with the client address and REMOTE_ADDR is the proxy address.
When the visit is tracked, only the REMOTE_ADDR is registered, which is a 172.16.0.0/12 non routed IP address since akrabat/ip-address-middleware >= 2.5 (akrabat/ip-address-middleware#51)

Expected behavior

Shlink should allow to set trusted_proxies to akrabat/ip-address-middleware in order to get it to ignore them and retrieve the X-Real-IP header IP address

Minimum steps to reproduce

Run a shlink container behind a reverse-proxy with a private class IP address which can't override REMOTE_ADDR.

[acelaya]

So I guess your point is that, after those changes in akrabat/ip-address-middleware, the X-Real_IP and X-Forwarded-For headers are being ignored in some cases?

Can you provide a failing test case that proves it? This is probably the best test to extend

shlink/module/Core/test-api/Action/RedirectTest.php

Line 92 in ed09bf9

$ipAddressConfig = require __DIR__ . '/../../../../config/autoload/ip-address.global.php';

[ymage]

OK, I will try to setup a full test environment to add the required test but I fail to setup it (cf discussion)

[ymage]

So I guess your point is that, after those changes in akrabat/ip-address-middleware, the X-Real_IP and X-Forwarded-For headers are being ignored in some cases?

Can you provide a failing test case that proves it? This is probably the best test to extend

shlink/module/Core/test-api/Action/RedirectTest.php

Line 92 in ed09bf9
$ipAddressConfig = require DIR . '/../../../../config/autoload/ip-address.global.php';

Not sure how to implement the failing test case here because my issue is not about "failing redirection", it's more about the erroneous retrieved/stored remote IP address

[acelaya]

So I guess your point is that, after those changes in akrabat/ip-address-middleware, the X-Real_IP and X-Forwarded-For headers are being ignored in some cases?
Can you provide a failing test case that proves it? This is probably the best test to extend
shlink/module/Core/test-api/Action/RedirectTest.php
Line 92 in ed09bf9
$ipAddressConfig = require DIR . '/../../../../config/autoload/ip-address.global.php';

Not sure how to implement the failing test case here because my issue is not about "failing redirection", it's more about the erroneous retrieved/stored remote IP address

The redirection is based precisely in that IP address resolution. If you add a test case there where a set of headers should resolve a specific IP address that would cause the redirect to match, but it doesn't, then you would have demonstrated the bug.

If the same test passes in an older version of Shlink, I will be able to investigate the difference.

[ymage]

It seems my explanation was not enough accurate : the redirection works fine.
My issue is about the logged IP address which is the one from the proxy, not from the client.
I guess it would better here https://github.com/shlinkio/shlink/blob/develop/module/Core/test/Visit/RequestTrackerTest.php#L60

I'll dig there

[acelaya]

It seems my explanation was not enough accurate : the redirection works fine. My issue is about the logged IP address which is the one from the proxy, not from the client. I guess it would better here https://github.com/shlinkio/shlink/blob/develop/module/Core/test/Visit/RequestTrackerTest.php#L60

I'll dig there

Yes, that was clear, but the logic to resolve the remote IP is the same. The same IP that is resolved for an IP-based redirection will be the one logged as the visitor IP.

The test you point out is a unit test, so you won't be able to mimic the whole user journey, including the logic in akrabat/ip-address-middleware. Basically that test bypasses that and only tests what happens for a hypothetically resolved IP address.

The test I pointed out is an E2E test which covers the whole journey, where you set the "input" received by Shlink, with the headers your proxy is supposedly setting. With those headers your expectation is that it will resolve the visitor IP address, not the proxy one, and in that case, the visitor should be redirected to https://example.com/static-ip-address.

If the proxy IP address is resolved, it will redirect somewhere else, making the test fail.

[ymage]

Thanks @acelaya for your guidance.
I am not sure how to retrieve the logged IP address during the redirect case to test its value against different Headers scenario.
Any idea ?

[acelaya]

You don't really need to check that.

In the test I pointed out, you can just verify that the visitor is redirected to https://example.com/static-ip-address if the visitor's IP address is resolved to 1.2.3.4.

In the loop right before it, that test verifies that's in fact the case when that IP address is set in any of the headers that Akrabat's middleware inspects.

Now, you are saying you have a reverse proxy which is setting those headers with the visitor's IP address, but you still end up with a different one resolved.

If you can manage to put together a new test case there, right under the loop, where you set the headers as your proxy is setting them, and the redirect URL ends up being something other than https://example.com/static-ip-address, then that would have proven something is wrong.

I'm assuming what that test is missing and you are reporting here is a case in which X-Real-IP and/or X-Forwarded-For contain the visitor's IP address, but there's some other header with the proxy's IP address which ends up taking precedence.

[acelaya]

Just for context. I'm requesting all this, because I'm not completely sure your assumption here is correct.

Shlink should allow to set trusted_proxies to akrabat/ip-address-middleware in order to get it to ignore them and retrieve the X-Real-IP header IP address

I have been using Shlink with a reverse proxy for quite some time and IP addresses have always been resolved properly (including with latest version).

Also, it seems like this library already assumes the value in REMOTE_ADDR is the proxy if it has been instructed to check proxied headers (which Shlink does).

https://github.com/akrabat/ip-address-middleware/blob/ff2a118f6c0214fe006156d5e28f1a4195ca6caf/src/IpAddress.php#L210-L214

So the first thing is demonstrating there's in fact a bug and it's not a mis-configuration in the proxy or an incorrect expectation.

[acelaya]

There's some documentation on how to use a reverse proxy with Shlink here https://shlink.io/documentation/advanced/exposing-through-reverse-proxy/

Make sure you have set everything as described there.

The problem may also be specific for traefik.

[ymage]

Well, it WAS working fine before akrabat/ip-address-middleware#51

Its change requires now to configure TrustedProxies setting to make it to ignore the intermediary recognized IP.

Here is my case :

• X-Real-IP header contains the real client IP (correctly forwarded by nginx)
• X-Forwarded-For header contains the real client IP (correctly forwarded by nginx)
• REMOTE_ADDR header contains the Proxy IP (from nginx container)

With the recent akrabat/ip-address-middleware, Shlink gets REMOTE_ADDR as logged IP from ip-address-middleware

[acelaya]

Perfect! That's the scenario you need to reproduce in the E2E test I referenced.

[ymage]

Perfect! That's the scenario you need to reproduce in the E2E test I referenced.

Yes. But I don't know how to test the IP returned from the middleware ?
Which object can I reach from $request / $response to know what Shlink retrieves from the middleware without trustedproxies set or when it's set ?
I would like to retrieve $request->getAttribute('ip_address') from the test.

[ymage]

OK. My scenario is more tricky (traefik -> nginx -> shlink) and I better understand why it is not caught :

X-Real-IP header contains the real client IP (correctly forwarded by nginx) : 1.2.3.4
X-Forwarded-For header contains the real client IP (correctly forwarded by nginx) AND the Proxy IP : 1.2.3.4, 178.12.0.8
REMOTE_ADDR header contains the Proxy IP (from nginx container) : 178.12.0.8

The middleware catches X-Forwarded-For before X-Real-IP (I guess) and read it from right since akrabat/ip-address-middleware#51
So, it gets 178.12.0.8 instead of 1.2.3.4 when it was read from the left.

[acelaya]

If you see the code snippet I referenced from Akrabat's code, you'll notice the value in REMOTE_ADDR is set as the only trusted proxy if the list of proxies is empty, which would make this work for a single proxy.

However you seem to have two proxies, traerfik, then nginx. I wonder if each one is appending their IP to the forwarded header, making the actual client IP the third from the right.

Have you tried if it works with just one proxy?

Another option could be simply changing the order of the headers here https://github.com/shlinkio/shlink/blob/develop/config/autoload/ip-address.global.php#L18. It was set a bit randomly.

[ymage]

I have two proxies and that part is not in my hand :-/
And like I said, that was working before the middleware change and it looks like that's the purpose of TRUSTED_PROXIES settings

I guess my fix is better than changing the headers order in ip-address.global.php#L18.

[acelaya]

I have two proxies and that part is not in my hand :-/

Again, I'm just trying to understand the problem. Stop being so defensive or I'll simply close this and move on with my life.

I didn't say "you have to remove one proxy", I just asked if it works with one proxy, with the only intention to try to prove a hypothesis.

Since this issue has been opened, you have been trying to enforce what you think is the solution, without a single prove that this is going to actually make a difference.

You didn't provide a single reproduction, or provided a failing test to prove nothing.

[ymage]

I am sorry if I look defensive. It was not intended.

About the PR, it was a new one with the test as a commit added : 28e30ff

And to answer your question :
Yes, with only one proxy, the logged ip is the right one
And with both, it is not, like you can see in the provided test.

I really try to fit your expectation to explain which problem I encounter

[ymage]

@acelaya , I apologize, my PR was not the right way to share my proposal.

[ymage]

I tried this test case

        yield 'rule: Reverse Proxy IP address in "REMOTE_ADDR" and in "X-Forwarded-For" set as last' => [
            [
                RequestOptions::HEADERS => ['X-Forwarded-For' => '1.2.3.4, 172.12.0.8'],
            ],
            'https://example.com/static-ip-address',
        ];

With :

• akrabat/ip-address-middleware = 2.4.0 : PASS
• akrabat/ip-address-middleware >= ^2.5 : FAIL

[acelaya]

So, I took the time to put together a reproduction and see what was exactly going on and if there was some "easy" way to get this fixed: #2357

This basically proves that the library expects the list of trusted proxies to be set now, otherwise the wrong IP is resolved when using more than 1 proxy.

However, it's a bit inconvenient that the way to fix this regression requires users to set-up their trusted proxies. People may not really know what that means, or may even have updated Shlink without noticing about the fix, and have a broken instance for some time because they didn't set their trusted proxies.

I think the right course of action here is to try to put together a workaround in Shlink, that ensures the previous behavior is preserved, and release v4.4.3 with that fix.

Then, v4.5.0 can introduce support to set your own trusted proxies, and disable the workaround if they have been provided. This would be treated as a feature.

Then maybe v5.0.0 can drop the workaround entirely, but I'll make that decision when the time comes.

@ymage, on a side note, in future, make sure to not try to rush things. It's super important that there's a common understanding, and you should not assume project maintainers are up to speed with you when you are reporting an issue.

You should absolutely never open an unsolicited PR in a project, unless it's something blatantly obvious, but even in that case, it may be obvious only for you.

Always start by opening an issue, as detailed as possible, explaining the problem, your expectations, and what ended up happening.

Once the problem to be solved has been stated and everyone agrees and understands it, conversations on how to fix it can start, and that's when you can volunteer to provide the PR if you wish.

[ymage]

Thank you @acelaya for your patience and for the time you spent with my issue.

I think I was a bit lost because some others projects react differently and ask for a PR proposal for each bug you report.
I will respect your way of working.

Regards.