If you have a koha_trusted_proxies value of 192.168.1.0/24 and your client IP is 192.168.1.100, it'll set REMOTE_ADDR to undef.
Actually, if your REMOTE_ADDR is 1.1.1.1 and your X-Forwarded-For is 1.1.1.1 and your koha_trusted_proxies is 1.1.1.0/24, then your REMOTE_ADDR will be reset to null. If there is only 1 value in the X-Forwarded-For, we shouldn't be looking for proxies.
Created attachment 106646 [details] [review] Bug 25950: Remove <client> in X-Forwarded-For from proxy tests This patch removes the <client> ip address in the X-Forwarded-For header from being tested against koha_trusted_proxies. Without this patch, REMOTE_ADDR will be set to null, if the <client> ip address matches against koha_trusted_proxies. To Test: 1. Run the unit test t/Koha/Middleware/RealIP.t
Created attachment 106647 [details] [review] Bug 25950: Remove <client> in X-Forwarded-For from proxy tests This patch removes the <client> ip address in the X-Forwarded-For header from being tested against koha_trusted_proxies. Without this patch, REMOTE_ADDR will be set to null, if the <client> ip address matches against koha_trusted_proxies. To Test: 1. Run the unit test t/Koha/Middleware/RealIP.t
I probably am not doing a great job of explaining this one. Without the patch, the "<client>" in the X-Forwarded-For will be tested against koha_trusted_proxies, even though it's not a proxy. So if the "<client>" matches against koha_trusted_proxies, it won't be used to set REMOTE_ADDR, which means it'll be left as null. With the patch, the "<client>" is extracted from the X-Forwarded-For header data, and then the REMOTE_ADDR (the proxy that set the X-Forwarded-For) and any "<proxy>" values from the X-Forwarded-For header are evaluated against koha_trusted_proxies. If REMOTE_ADDR and "<proxy>" are all trusted, then the "<client>" is used to re-write REMOTE_ADDR. If they're not trusted, the last value not trusted will be used to re-write REMOTE_ADDR. So while you might trust your reverse proxy at REMOTE_ADDR, you might not trust the "<proxy>" values in X-Forwarded-For.
I am pretty keen to get this going, so I'm going to apply it locally rather than wait.
Created attachment 106722 [details] [review] Bug 25950: Remove <client> in X-Forwarded-For from proxy tests This patch removes the <client> ip address in the X-Forwarded-For header from being tested against koha_trusted_proxies. Without this patch, REMOTE_ADDR will be set to null, if the <client> ip address matches against koha_trusted_proxies. To Test: 1. Run the unit test t/Koha/Middleware/RealIP.t Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Thanks, Didier! For what it's worth, I have this running locally in production now, and it's working well.
Created attachment 106763 [details] [review] Bug 25950: Remove <client> in X-Forwarded-For from proxy tests This patch removes the <client> ip address in the X-Forwarded-For header from being tested against koha_trusted_proxies. Without this patch, REMOTE_ADDR will be set to null, if the <client> ip address matches against koha_trusted_proxies. To Test: 1. Run the unit test t/Koha/Middleware/RealIP.t Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This works as expected and I can't see any regressions.. admittedly this isn't my area of expertise but the code appears to be sound and the description makes sense. Passing QA
Cheers Martin :)
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.03
backported to 19.11.x for 19.11.09
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.
Got a random failure, D11 run 265 on Docker_5: 18:04:17 koha_1 | could not parse 2.2.2 at /kohadevbox/koha/Koha/Middleware/RealIP.pm line 117. 18:04:17 koha_1 | 18:04:17 koha_1 | # Failed test 'Trust proxy (2.2.2.2) using abbreviated notation, so use the X-Forwarded-For header for the remote address' 18:04:17 koha_1 | # at t/Koha/Middleware/RealIP.t line 114. 18:04:17 koha_1 | # got: '2.2.2.2' 18:04:17 koha_1 | # expected: '1.1.1.1' 18:04:17 koha_1 | # Looks like you failed 1 test of 3. 18:04:17 koha_1 | 18:04:17 koha_1 | # Failed test 'Test alternative configuration styles' 18:04:17 koha_1 | # at t/Koha/Middleware/RealIP.t line 121.
Maybe not random, it failed once again on D11 (run 266)
(In reply to Jonathan Druart from comment #16) > Maybe not random, it failed once again on D11 (run 266) Is it happening every time? If so, what version of Net::Netmask is installed on D11?
Yes it's failing consistently on D11 % pmvers Net::Netmask 1.9104
(In reply to Jonathan Druart from comment #18) > Yes it's failing consistently on D11 > Well that's good at least. > % pmvers Net::Netmask > 1.9104 It looks like buster uses 1.9104-1 and bullseye uses 1.9104-2... And it looks like 1.9104-2 includes a backport of a security patch that doesn't allow the following formats: '216.240.32' '216.240' '140' '216.240.32/24' '216.240/16' The workaround is to use the "shortnet" option which is specified in the newer versions of the module: https://metacpan.org/pod/release/JMASLAK/Net-Netmask-2.0001/lib/Net/Netmask.pm Note that we use Net::Netmask in a few places, so we'll need to either add the shortnet option, or make people update their systems.
I feel like adding the shortnet => 1 constructor option is the most backwards compatible...
(In reply to David Cook from comment #20) > I feel like adding the shortnet => 1 constructor option is the most > backwards compatible... Oh goodie, they've changed the constructor in a way that is not backwards compatible with 1.9104. Awesome. Fortunately there is another workaround. But jeez... See Bug 28200.