Summary: | Net::Netmask 1.9104-2 requires constructor change for backwards compatibility | ||
---|---|---|---|
Product: | Koha | Reporter: | David Cook <dcook> |
Component: | Architecture, internals, and plumbing | Assignee: | David Cook <dcook> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P5 - low | CC: | fridolin.somers, jonathan.druart, martin.renvoize, mtj, nick, tomascohen, victor, wainuiwitikapark |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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
Bug 28200: Add workaround for Net::Netmask abbreviated values Bug 28200: [Alternate] Remove unit test using abbreviated format Bug 28200: [Alternate] Remove unit test using abbreviated format Bug 28200: [Alternate] Remove unit test using abbreviated format Bug 28200: [Alternate] Remove unit test using abbreviated format |
Description
David Cook
2021-04-23 00:00:37 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 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. What about dropping support for abbreviated formats? 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 (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 (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... 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> 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" 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. 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? (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... 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.. 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 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. I am in favour of dropping it, with a warning in the release notes. 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> Works :) code looks good, QA script ok Count my QA stamp in, I'll try to find someone to sign-off notes: tried with a without the patch to confirm the issue and fix fix in the patch on the number of expected tests (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. (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. 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> All works as expected and after agreed discussions we've picked the 'Alternate' option. Thanks for all the work guys. Passing QA I've had a go at the text for release notes.. hopefully, it's clear enough. (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 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> Thanks Alvaro for the additional check and setting up a local Koha for the occasion, welcome to Koha :) Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.06 Sorry the commit was not in 20.11.06. I've added it so it will be in 20.11.07 Backported: Pushed to 20.05.x branch for 20.05.13 Does this need to be backported to 19.11.x? 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. Not backported to 19.11.x Please see Victor's comment above. (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? Backported to 19.11.x Should the status of this one be updated? Back to "Resolved fixed" |