Bug 25950 - REMOTE_ADDR set to null if client_ip in X-Forwarded-For matches a koha_trusted_proxies value
Summary: REMOTE_ADDR set to null if client_ip in X-Forwarded-For matches a koha_truste...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on: 23068
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-08 06:25 UTC by David Cook
Modified: 2020-09-18 19:55 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09


Attachments
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests (9.57 KB, patch)
2020-07-08 07:14 UTC, David Cook
Details | Diff | Splinter Review
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests (10.05 KB, patch)
2020-07-08 07:32 UTC, David Cook
Details | Diff | Splinter Review
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests (10.11 KB, patch)
2020-07-09 14:48 UTC, didier
Details | Diff | Splinter Review
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests (10.17 KB, patch)
2020-07-10 08:02 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-07-08 06:25:33 UTC
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.
Comment 1 David Cook 2020-07-08 06:33:15 UTC
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.
Comment 2 David Cook 2020-07-08 07:14:06 UTC
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
Comment 3 David Cook 2020-07-08 07:32:00 UTC
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
Comment 4 David Cook 2020-07-08 07:36:08 UTC
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.
Comment 5 David Cook 2020-07-09 05:41:21 UTC
I am pretty keen to get this going, so I'm going to apply it locally rather than wait.
Comment 6 didier 2020-07-09 14:48:06 UTC
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>
Comment 7 David Cook 2020-07-09 23:47:53 UTC
Thanks, Didier!

For what it's worth, I have this running locally in production now, and it's working well.
Comment 8 Martin Renvoize 2020-07-10 08:02:49 UTC
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>
Comment 9 Martin Renvoize 2020-07-10 08:03:40 UTC
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
Comment 10 David Cook 2020-07-16 03:33:22 UTC
Cheers Martin :)
Comment 11 Jonathan Druart 2020-07-20 12:08:23 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 12 Lucas Gass 2020-07-24 21:28:33 UTC
backported to 20.05.x for 20.05.03
Comment 13 Aleisha Amohia 2020-08-03 21:58:40 UTC
backported to 19.11.x for 19.11.09
Comment 14 Victor Grousset/tuxayo 2020-09-18 19:55:23 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.