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: 2021-04-23 00:00 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
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 Gautheron
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 Gautheron 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.
Comment 15 Jonathan Druart 2021-04-21 07:33:40 UTC
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.
Comment 16 Jonathan Druart 2021-04-21 10:27:13 UTC
Maybe not random, it failed once again on D11 (run 266)
Comment 17 David Cook 2021-04-22 05:04:25 UTC
(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?
Comment 18 Jonathan Druart 2021-04-22 07:51:04 UTC
Yes it's failing consistently on D11

% pmvers Net::Netmask
1.9104
Comment 19 David Cook 2021-04-22 23:49:01 UTC
(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.
Comment 20 David Cook 2021-04-22 23:52:04 UTC
I feel like adding the shortnet => 1 constructor option is the most backwards compatible...
Comment 21 David Cook 2021-04-23 00:00:45 UTC
(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.