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.