Bug 28200 - Net::Netmask 1.9104-2 requires constructor change for backwards compatibility
Summary: Net::Netmask 1.9104-2 requires constructor change for backwards compatibility
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-23 00:00 UTC by David Cook
Modified: 2022-12-12 21:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
The code library Koha uses for working with IP addresses has dropped support for abbreviated values in recent releases. This is to tighten up the default security of input value's and we have opted in Koha to follow this change through into our system preferences for the same reason. WARNING: `koha_trusted_proxies` and `ILS-DI:AuthorizedIPs` are both affected. Please check that you are not using abbreviated IP forms for either of these cases. Example: "10.10" is much less explicit than "10.10.0.0/16" and should be avoided.
Version(s) released in:
21.05.00,20.11.07,20.05.13,19.11.22
Circulation function:

Attachments
Bug 28200: Add workaround for Net::Netmask abbreviated values (1.50 KB, patch)
2021-04-23 00:04 UTC, David Cook 		Details | Diff | Splinter Review
Bug 28200: Add workaround for Net::Netmask abbreviated values (1.64 KB, patch)
2021-05-01 16:44 UTC, Victor Grousset/tuxayo 		Details | Diff | Splinter Review
Bug 28200: [Alternate] Remove unit test using abbreviated format (1.60 KB, patch)
2021-05-10 23:35 UTC, David Cook 		Details | Diff | Splinter Review
Bug 28200: [Alternate] Remove unit test using abbreviated format (1.88 KB, patch)
2021-05-11 18:17 UTC, Victor Grousset/tuxayo 		Details | Diff | Splinter Review
Bug 28200: [Alternate] Remove unit test using abbreviated format (1.94 KB, patch)
2021-05-13 06:31 UTC, Martin Renvoize (ashimema) 		Details | Diff | Splinter Review
Bug 28200: [Alternate] Remove unit test using abbreviated format (2.00 KB, patch)
2021-05-17 00:18 UTC, Alvaro Cornejo 		Details | Diff | Splinter Review
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2021-04-23 00:00:37 UTC 
As Jonathan and David noted in Bug 25950, Net::Netmask needs a constructor change from 1.9104-2 in order to accept abbreviated formats.
Comment 1 David Cook 2021-04-23 00:04:28 UTC 
Created attachment 120035 [details] [review]
Bug 28200: Add workaround for Net::Netmask abbreviated values

This patch adds the shortnet workaround to allow Net::Netmask to
parse abbreviated values.

To test:
1) Apply patch
2) Run unit tests
Comment 2 David Cook 2021-04-23 00:05:53 UTC 
Ok, I've tested this on koha-testing-docker stretch (libnet-netmask-perl 1.9022-1), and I can confirm that it hasn't broken anything.

If someone can test that it fixes the problem on D11, that would be great. It should.
Comment 3 Jonathan Druart 2021-04-23 07:01:56 UTC 
What about dropping support for abbreviated formats?
Comment 4 Jonathan Druart 2021-04-23 07:49:32 UTC 
If you disagree with the previous comment you can consider it signed off.

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/Koha/Middleware/RealIP.t 
t/Koha/Middleware/RealIP.t .. ok     
All tests successful.
Files=1, Tests=11,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.46 cusr  0.04 csys =  0.52 CPU)
Result: PASS

kohadev-koha@kohadevbox:/kohadevbox/koha$ more /etc/debian_version 
bullseye/sid

kohadev-koha@kohadevbox:/kohadevbox/koha$ pmvers Net::Netmask
1.9104
Comment 5 Tomás Cohen Arazi (tcohen) 2021-04-23 11:37:38 UTC 
(In reply to Jonathan Druart from comment #4)
> If you disagree with the previous comment you can consider it signed off.
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/Koha/Middleware/RealIP.t 
> t/Koha/Middleware/RealIP.t .. ok     
> All tests successful.
> Files=1, Tests=11,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.46 cusr  0.04
> csys =  0.52 CPU)
> Result: PASS
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ more /etc/debian_version 
> bullseye/sid
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ pmvers Net::Netmask
> 1.9104

Why not dropping support? We should ask the community
Comment 6 David Cook 2021-04-25 23:27:49 UTC 
(In reply to Jonathan Druart from comment #3)
> What about dropping support for abbreviated formats?

It would be a breaking change for existing libraries. 

I have quite a few libraries using the abbreviated format for their ILS-DI. (Although I don't have any using the abbreviated format in koha_trusted_proxies.)

I can always update it, but I'm thinking about libraries that might not have someone. Although I suppose if they're using koha_trusted_proxies or ILS-DI, they probably would have someone who could update the configuration correctly...
Comment 7 Victor Grousset/tuxayo 2021-05-01 16:44:17 UTC 
Created attachment 120350 [details] [review]
Bug 28200: Add workaround for Net::Netmask abbreviated values

This patch adds the shortnet workaround to allow Net::Netmask to
parse abbreviated values.

To test:
1) Apply patch on a Debian 11 Koha
2) Run unit tests
   prove t/Koha/Middleware/RealIP.t
3) redo for a Debian 9

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2021-05-01 16:46:09 UTC 
Works as expected on Debian 11 and Debian 9, tested with and without the patch
env: koha-testing-docker

Not switching to signoff because the ticket is kinda "In discussion"
Comment 9 David Cook 2021-05-03 04:13:29 UTC 
If we are going to drop support for the abbreviated format, then we can update this bug to just change the unit test and maybe print a warning in the release notes or something.
Comment 10 Martin Renvoize (ashimema) 2021-05-06 06:04:01 UTC 
I've not read enough of th background to understand why support was dropped for security reasons in the upstream library?  It feels like if they've disabled it by default for a reason we shouldn't just re-enable it without considering the possible security ramifications.  That said, I wouldn't be opposed to tying that constructor line to yet another system preference that defaults to enabled for upgrades and disabled for new installs..  That way we don't break anyone's setups but encourage the more secure form going forward?
Comment 11 David Cook 2021-05-06 06:24:07 UTC 
(In reply to Martin Renvoize from comment #10)
> I've not read enough of th background to understand why support was dropped
> for security reasons in the upstream library?  It feels like if they've
> disabled it by default for a reason we shouldn't just re-enable it without
> considering the possible security ramifications.  That said, I wouldn't be
> opposed to tying that constructor line to yet another system preference that
> defaults to enabled for upgrades and disabled for new installs..  That way
> we don't break anyone's setups but encourage the more secure form going
> forward?

Personally, I think their labelling it as a "security" change was overblown. My understanding is that they dropped support for the abbreviated format because it *might* be too easy to accidentally specify a more permissive range than one intends. 

I can see how 10.10 is much less explicit than 10.10.0.0/16 but I don't really see the problem.

But at this point in the discussion I am OK with Koha dropping support for the abbreviated form.

I suppose the question is do we leave it as a breaking change or do we automagically fix people's configuration? 

I don't mind manually fixing all my instances, but I also know this stuff really well. 

It looks like HEA doesn't capture the relevant ILS-DI syspref (https://hea.koha-community.org/systempreferences) so I don't know what people have used...
Comment 12 David Cook 2021-05-06 06:25:10 UTC 
Maybe this is a good one for a warning in about.pl. 

I think a bad value in koha-conf.xml will make your Koha fail to start up so you'd find that pretty fast.

And a bad ILS-DI value could be found in about.pl or by any API user..
Comment 13 David Cook 2021-05-10 23:35:35 UTC 
Created attachment 120818 [details] [review]
Bug 28200: [Alternate] Remove unit test using abbreviated format

This patch removes the unit test that uses the abbreviated format,
since it's not supported in newer versions of Net::Netmask without
a workaround.

Test plan:
0. Set up Koha dev environment on Debian 11
1. perl t/Koha/Middleware/RealIP.t
2. Tests should pass
Comment 14 David Cook 2021-05-10 23:36:37 UTC 
I've provided an alternate patch for dropping support for the abbreviated format. 

I'll leave it up to QA and the RM to decide if they want to support the abbreviated format or not. 

It doesn't really matter to me either way, but we need to do something.
Comment 15 Jonathan Druart 2021-05-11 08:28:28 UTC 
I am in favour of dropping it, with a warning in the release notes.
Comment 16 Victor Grousset/tuxayo 2021-05-11 18:17:06 UTC 
Created attachment 120851 [details] [review]
Bug 28200: [Alternate] Remove unit test using abbreviated format

This patch removes the unit test that uses the abbreviated format,
since it's not supported in newer versions of Net::Netmask without
a workaround.

Test plan:
0. Set up Koha dev environment on Debian 11
1. perl t/Koha/Middleware/RealIP.t
2. Tests should pass

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2021-05-11 18:17:26 UTC 
Works :) code looks good, QA script ok
Count my QA stamp in, I'll try to find someone to sign-off
Comment 18 Victor Grousset/tuxayo 2021-05-11 18:19:04 UTC 
notes:
tried with a without the patch to confirm the issue and fix

fix in the patch on the number of expected tests
Comment 19 David Cook 2021-05-11 23:21:42 UTC 
(In reply to Jonathan Druart from comment #15)
> I am in favour of dropping it, with a warning in the release notes.

Works for me.
Comment 20 David Cook 2021-05-11 23:25:30 UTC 
(In reply to David Cook from comment #19)
> (In reply to Jonathan Druart from comment #15)
> > I am in favour of dropping it, with a warning in the release notes.
> 
> Works for me.

Upon review, it looks like I always used CIDR notation in koha-conf.xml for koha_trusted_proxies. 

I think the only time I used abbreviated format was for ILS-DI:AuthorizedIPs and that was arguably a legacy format from a pre-existing local customization.

So maybe no one else has used the abbreviated format anyway. 

I figure the ILS-DI API failing will be reasonably obvious too if someone is using the abbreviated format, so not a big drama.
Comment 21 Martin Renvoize (ashimema) 2021-05-13 06:31:44 UTC 
Created attachment 120911 [details] [review]
Bug 28200: [Alternate] Remove unit test using abbreviated format

This patch removes the unit test that uses the abbreviated format,
since it's not supported in newer versions of Net::Netmask without
a workaround.

Test plan:
0. Set up Koha dev environment on Debian 11
1. perl t/Koha/Middleware/RealIP.t
2. Tests should pass

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize (ashimema) 2021-05-13 06:33:04 UTC 
All works as expected and after agreed discussions we've picked the 'Alternate' option.

Thanks for all the work guys.

Passing QA
Comment 23 Martin Renvoize (ashimema) 2021-05-13 06:39:24 UTC 
I've had a go at the text for release notes.. hopefully, it's clear enough.
Comment 24 David Cook 2021-05-14 02:25:49 UTC 
(In reply to Martin Renvoize from comment #23)
> I've had a go at the text for release notes.. hopefully, it's clear enough.

Looks good to me
Comment 25 Alvaro Cornejo 2021-05-17 00:18:03 UTC 
Created attachment 121024 [details] [review]
Bug 28200: [Alternate] Remove unit test using abbreviated format

This patch removes the unit test that uses the abbreviated format,
since it's not supported in newer versions of Net::Netmask without
a workaround.

Test plan:
0. Set up Koha dev environment on Debian 11
1. perl t/Koha/Middleware/RealIP.t
2. Tests should pass

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Alvaro Cornejo <cornejo.alvaro@gmail.com>
Comment 26 Victor Grousset/tuxayo 2021-05-17 00:51:53 UTC 
Thanks Alvaro for the additional check and setting up a local Koha for the occasion, welcome to Koha :)
Comment 27 Jonathan Druart 2021-05-17 14:19:49 UTC 
Pushed to master for 21.05, thanks to everybody involved!
Comment 28 Fridolin Somers 2021-05-18 10:06:40 UTC 
Pushed to 20.11.x for 20.11.06
Comment 29 Fridolin Somers 2021-06-04 08:38:50 UTC 
Sorry the commit was not in 20.11.06.
I've added it so it will be in 20.11.07
Comment 30 Victor Grousset/tuxayo 2021-06-08 23:32:12 UTC 
Backported: Pushed to 20.05.x branch for 20.05.13
Comment 31 Wainui Witika-Park 2021-06-15 23:07:45 UTC 
Does this need to be backported to 19.11.x?
Comment 32 Victor Grousset/tuxayo 2021-06-16 22:41:51 UTC 
It doesn't change how Koha works. It fixes the tests on Debian 11 but for 19.11.x, the CI doesn't use Debian 11 so it won't do anything.
Comment 33 Wainui Witika-Park 2021-06-17 01:58:39 UTC 
Not backported to 19.11.x 

Please see Victor's comment above.
Comment 34 Mason James 2021-09-05 09:03:48 UTC 
(In reply to wainuiwitikapark from comment #33)
> Not backported to 19.11.x 
> 
> Please see Victor's comment above.

hi Wainui
i have added a jenkins build for 19.11 on deb11

can you please add this patch to 19.11, so the jenkins tests pass?
Comment 35 Wainui Witika-Park 2021-09-06 00:09:28 UTC 
Backported to 19.11.x
Comment 36 David Cook 2022-04-04 23:20:50 UTC 
Should the status of this one be updated?
Comment 37 Jonathan Druart 2022-04-05 09:59:17 UTC 
Back to "Resolved fixed"