Bug 25950

Summary: REMOTE_ADDR set to null if client_ip in X-Forwarded-For matches a koha_trusted_proxies value
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: aleisha, jonathan.druart, kyle, lucas, martin.renvoize, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24538
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09
Bug Depends on: 23068    
Bug Blocks:    
Attachments: Bug 25950: Remove <client> in X-Forwarded-For from proxy tests
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests
Bug 25950: Remove <client> in X-Forwarded-For from proxy tests

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.