Web-based self-checkout is fantastic. But it'd be nice to have it a bit more locked down. Currently, anyone who knows/can guess the right URL can access it, even from offsite. I have a proof of concept hack, soon to be a patch, that fixes this. The hack, at the top of the sco*.pl files, something like my $restricttoip = '192.168.1.30'; unless ($restricttoip eq $ENV{'REMOTE_ADDR'}) { print $query->redirect("/cgi-bin/koha/opac-main.pl"); exit; }
Created attachment 40612 [details] [review] Allow web-based self-checkout to be limited to only certain IPs or IP ranges Bug 14407 - Allow web-based self-checkout to be limited to only certain IPs or IP ranges Test plan: 0) Back up your databse 1) Apply this patch 2) In your mysql client use your Koha database and execute: > DELETE FROM systempreferences; > SOURCE ~/kohaclone/installer/data/mysql/sysprefs.sql; -- Should be no errors. > SELECT * FROM systempreferences WHERE variable='SelfCheckAllowByIPRanges'; -- Should see 1 entry > QUIT; 3) Restore your database 4) Run installer/data/mysql/atomicupdate/selfcheckallowbyiprange.sql 5) In your mysql client, using your Koha database, execute: > SELECT * FROM systempreferences WHERE variable='SelfCheckAllowByIPRanges'; -- should see the same entry as above 6) Try access the Self-checkout, self-checkout help, and self-checkout patron images. It should work. 7) Log into the staff client 8) Home -> Koha administration -> Global system preferences 9) -> CIRCULATION Make sure that SelfCheckAllowByIPRanges is set to a computer you can test on 10) Try access the Self-checkout, self-checkout help, and self-checkout patron images from a computer with an IP in the range above. It should work. 11) Try access the Self-checkout, self-checkout help, and self-checkout patron images from a computer with an IP outside of the range above. It should *not* work.
Comment on attachment 40612 [details] [review] Allow web-based self-checkout to be limited to only certain IPs or IP ranges Review of attachment 40612 [details] [review]: ----------------------------------------------------------------- I've not tested the code, but looking at the patch, here are my preliminary comments. ::: installer/data/mysql/atomicupdate/selfcheckallowbyiprange.sql @@ +1,1 @@ > +INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES two things here: 1/ you should probably consider re-naming the atomic update .sql filename - refer to http://wiki.koha-community.org/wiki/Database_updates#If_you_are_a_developer for file naming convention 2/ consider changing the syntax fro 'INSERT INTO' to 'INSERT IGNORE INTO' for compliance with Bug 9071. Otherwise this will likely result in "Failed QA" ::: installer/data/mysql/sysprefs.sql @@ +55,4 @@ > ('AutoSelfCheckAllowed','0','','For corporate and special libraries which want web-based self-check available from any PC without the need for a manual staff login. Most libraries will want to leave this turned off. If on, requires self-check ID and password to be entered in AutoSelfCheckID and AutoSelfCheckPass sysprefs.','YesNo'), > ('AutoSelfCheckID','','','Staff ID with circulation rights to be used for automatic web-based self-check. Only applies if AutoSelfCheckAllowed syspref is turned on.','free'), > ('AutoSelfCheckPass','','','Password to be used for automatic web-based self-check. Only applies if AutoSelfCheckAllowed syspref is turned on.','free'), > +('SelfCheckAllowByIPRanges','',NULL,('Leave blank if not used. Use ranges or simple ip addresses separated by spaces, like <code>192.168.1.1 192.168.0.0/24</code>.)','Short'), The sysprefs are ordered alphabetically (and not necessarily grouped together by functionality). Please fix this, otherwise this will get stuck in QA. ::: opac/sco/sco-main.pl @@ +36,4 @@ > > use CGI qw ( -utf8 ); > use Digest::MD5 qw(md5_base64); > +use Net::IP::Match::XS; you are introducing a new perl dep. from my preliminary look, this module is probably not packaged for Debian / Ubuntu. If that is the case, then until it is packaged as .deb (upstream in the distros or by someone here or by Robin Sheat (provided he has the time and inclination to package it), this patch won't make it to a release. Just an FYI.
Created attachment 40613 [details] [review] Allow web-based self-checkout to be limited to only certain IPs or IP ranges Fixes the comments raised in Comment #2. As for introducing a new perl module - does Koha already have one that is used for testing whether an IP is in a particular range or group of supplied IP addresses? We use Fedora here, so I have no idea what is/isn't in Debian... hopefully this patch can be switched to use an existing library, or else it won't be too much work to get it added to Koha...
The only way we do this currently (AFAIK) is by regex style matching, see how OpacSuppressionByIPRange is used in opac-search.pl. A proper matching, like done in this bug, is probably a better way to do it. However, I'd like to avoid using ::XS modules if possible, mostly because they will require custom compilation for every platform that people could possibly use, and I know that I don't have access to a s390x architecture machine to test on :) Instead, is it possible to use Net::IP::Match, which also isn't in Debian, but will be substantially easier to build.
Setting as assigned for response to comment #4
Created attachment 40852 [details] [review] Bug 14407 - Allow web-based self checkout to be limited by IP addresses/ranges Allow web-based self-checkout to be limited to only certain IPs or IP ranges As per Robin's suggestion in Comment 4, it no longer uses an XS module. I have, however, gone for Net::CIDR instead of Net::IP::Match. For one thing, it looks like it's already packaged for Debian (and, as a bonus, for Fedora too!) For another, Net::IP::Match seemed to want to know a list of addresses etc. to match at "compile time". If this patch is acceptable, would another to adjust opac-search to use Net::CIDR be a nice "tidying to one standard" patch?
(In reply to Nicholas van Oudtshoorn from comment #6) > As per Robin's suggestion in Comment 4, it no longer uses an XS module. I > have, however, gone for Net::CIDR instead of Net::IP::Match. For one thing, > it looks like it's already packaged for Debian (and, as a bonus, for Fedora > too!) For another, Net::IP::Match seemed to want to know a list of addresses > etc. to match at "compile time". Brilliant, Net::CIDR has been in Debian since the dawn of time, so that makes me happier :) You've missed updating the use lines in sco-patron-image.pl though. Also don't forget to update C4/Installer/PerlDependencies.pm (keep an eye on the version version numbers being correct, wheezy has 0.15.) > If this patch is acceptable, would another to adjust opac-search to use > Net::CIDR be a nice "tidying to one standard" patch? I'd make a new bug for that, it'll possibly be more complicated than you expect as it'll have to account for the possibility of the format of a syspref changing, and either correcting that in the database or accepting the old form as well as the new one.
Created attachment 40854 [details] [review] Bug 14407 - Allow web-based self checkout to be limited by IP addresses/ranges (In reply to Robin in Comment #7) >Brilliant, Net::CIDR has been in Debian since the dawn of time, so that makes >me happier :) :-) Glad to have brought a sigh of relief! > You've missed updating the use lines in sco-patron-image.pl though. D'oh! Fixed that now >Also don't forget to update C4/Installer/PerlDependencies.pm (keep an eye on the version version numbers being correct, wheezy has 0.15.) Ah! Hadn't thought about that at all. Should be done now. >> If this patch is acceptable, would another to adjust opac-search to use >> Net::CIDR be a nice "tidying to one standard" patch? >I'd make a new bug for that, it'll possibly be more complicated than you >expect as it'll have to account for the possibility of the format of a syspref >changing, and either correcting that in the database or accepting the old form >as well as the new one. Agreed it would need a new bug. Have had a quick squiz, and it seems that NET::CIDR::range2cidr is perfectly happy with the format required by the current preference. It could, however, be changed to allow for much greater variety... (multiple subnets, individual IPs, etc.)
As far as I can tell, this patch is looking good to me from a packaging-related POV. (In reply to Nicholas van Oudtshoorn from comment #8) > Agreed it would need a new bug. Have had a quick squiz, and it seems that > NET::CIDR::range2cidr is perfectly happy with the format required by the > current preference. It could, however, be changed to allow for much greater > variety... (multiple subnets, individual IPs, etc.) If it's backwards compatible, then that's the best of all worlds. I'd make a new bug for that, it certainly would be nice to gain a bit of standardisation.
Thanks Robin! In that case, I'll change it to "needs signoff"!
Implementation note: Some code seems to be repeated in various places: my @allowedipranges = split(' ', C4::Context->preference('SelfCheckAllowByIPRanges')); if (scalar @allowedipranges > 0) { unless (Net::CIDR::cidrlookup($ENV{'REMOTE_ADDR'}, Net::CIDR::range2cidr(@allowedipranges))) { # action exit; } } Couldn't it be a good candidate to a new function in C4::Auth? Or even a new parameter to C4::Auth::get_template_and_user()?
Sorry, patch does not apply: Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules Merge conflict in C4/Installer/PerlDependencies.pm
I'm interested in this functionality. Without response from Nicholas van Oudtshoorn, I will refactor his patch.
Created attachment 45894 [details] [review] Limit web-based self-checkout to specific IP addresses Allow web-based self-checkout to be limited to only certain IPs or IP ranges. Adds a function to C4::Auth called in_ipset that checks if the current client IP is in a provided IP set, as suggested in comment #11. (This function will be useful for other IP-based restrictions)
Created attachment 45898 [details] [review] Limit web-based self-checkout to specific IP addresses Minor change to the last patch, that simply also removes a no longer needed use Net::CIDR
Created attachment 45899 [details] [review] Limit web-based self-checkout to specific IP addresses Minor change to the last patch, that simply also removes a no longer needed use Net::CIDR. Also removes the whitespace error :-)
Sorry, patch does not apply: Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref Auto-merging installer/data/mysql/sysprefs.sql Auto-merging C4/Installer/PerlDependencies.pm CONFLICT (content): Merge conflict in C4/Installer/PerlDependencies.pm Auto-merging C4/Auth.pm Failed to merge in the changes. Additionally, I think that the new function in C4/Auth.pm would need some testing in t/db_dependent/Auth.t
Created attachment 51812 [details] [review] Bug 14407 - rebased to latest git Realised that this had bitrotted a bit. New patch that applies to the latest git
Created attachment 51813 [details] [review] New patch, with silly typo fixed Test plan: 0) Back up your databse 1) Apply this patch 2) In your mysql client use your Koha database and execute: > DELETE FROM systempreferences; > SOURCE ~/kohaclone/installer/data/mysql/sysprefs.sql; -- Should be no errors. > SELECT * FROM systempreferences WHERE variable='SelfCheckAllowByIPRanges'; -- Should see 1 entry > QUIT; 3) Restore your database 4) Run installer/data/mysql/atomicupdate/selfcheckallowbyiprange.sql 5) In your mysql client, using your Koha database, execute: > SELECT * FROM systempreferences WHERE variable='SelfCheckAllowByIPRanges'; -- should see the same entry as above 6) Try access the Self-checkout, self-checkout help, and self-checkout patron images. It should work. 7) Log into the staff client 8) Home -> Koha administration -> Global system preferences 9) -> CIRCULATION Make sure that SelfCheckAllowByIPRanges is set to a computer you can test on 10) Try access the Self-checkout, self-checkout help, and self-checkout patron images from a computer with an IP in the range above. It should work. 11) Try access the Self-checkout, self-checkout help, and self-checkout patron images from a computer with an IP outside of the range above. It should *not* work.
Pinging anyone interested in looking at gettting this signed off! :-)
> 2) In your mysql client use your Koha database and execute: > > DELETE FROM systempreferences; > > SOURCE ~/kohaclone/installer/data/mysql/sysprefs.sql; > -- Should be no errors. SOURCE: - First I had an error related to Bug 16920. - With this patch applied on top of 16920, I get following error: ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 546
Found in sysprefs.sql ('SelfCheckAllowByIPRanges','',NULL,('Leave blank if not used. Use ranges or simple ip addresses separated by spaces, like <code>192.168.1.1 192.168.0.0/24</code>.)','Short'), NULL,('Leave blank... should be NULL,'(Leave blank...
Created attachment 53760 [details] [review] New patch, addressing the SQL bug Hi Marc, Thanks for taking a look. What a silly mistake! :-) Updated patch should fix it.
Quick test: SQL error is gone. However, I get an 'Internal Server Error' plack-error.log says: Could not compile (...)/opac/sco/sco-main.pl: "in_ipset" is not exported by the C4::Auth module Can't continue after import errors at (...)/opac/sco/sco-main.pl line 40. (I have not to much time for testing until mid August.)
Created attachment 54560 [details] [review] Latest version of patch, with silly typo fixed Hi Marc, Thanks for that. Can't believe I didn't put the fix for that into the patch here - we've been running with it for ages! :-) This version of the patch should have fixed it (and also rebased to master so that it will apply!)
Hi Nicholas, I tested and acces was restricted as expected (except printslip.pl, see below) QA-Tools complain: FAIL opac/sco/help.pl FAIL forbidden patterns forbidden pattern: tab char (line 34) forbidden pattern: tab char (line 35) FAIL opac/sco/printslip.pl FAIL forbidden patterns forbidden pattern: tab char (line 39) forbidden pattern: tab char (line 40) FAIL valid Global symbol "$query" requires explicit package name opac/sco/printslip.pl had compilation errors. FAIL opac/sco/sco-main.pl FAIL forbidden patterns forbidden pattern: tab char (line 60) forbidden pattern: tab char (line 61) forbidden pattern: tab char (line 62) FAIL opac/sco/sco-patron-image.pl FAIL forbidden patterns forbidden pattern: tab char (line 40) forbidden pattern: tab char (line 39)
Created attachment 55746 [details] [review] Latest version of patch, with errors fixed Thanks for that Marc. Hopefully all of those mistakes have now been rectified! :-)
Hi Nicholas The qa tool no longer complains. I re-tested with following scenario (restart plack after each change to syspref): - Client's IP is 192.168.1.32 - SelfCheckAllowByIPRanges is empty go to .../cgi-bin/koha/sco/sco-main.pl OK for all IPs - Set SelfCheckAllowByIPRanges to 192.168.1.32 Allowed for client OK Client with other IP redirected to OPAC main page OK - Set SelfCheckAllowByIPRanges to 192.168 Allowed for client OK Client with other IP redirected to OPAC main page OK - Set SelfCheckAllowByIPRanges to 192.168.1.32 192.168.1.30 192.168.1.10 Allowed for client OK Client with other IP redirected to OPAC main page OK - Set SelfCheckAllowByIPRanges to 192.168.1.15 192.168.1.30 192.168.1.10 Client redirected to OPAC main page OK Client with other IP redirected to OPAC main page OK - Set SelfCheckAllowByIPRanges to 192.168.1.1 192.168.1.0/36 as described for syspref SelfCheckAllowByIPRanges Internal Server Error Plack error log says; 36, as in '192.168.1.30/36', does not make sense at /home/marc/koha/C4/Auth.pm line 2058. Is it a bug or a wrong description for SelfCheckAllowByIPRanges?
Hmmm - I don't see SelfCheckAllowByIPRanges preferencing suggesting one use /36. The library I've used should support something like /24, which is what the preference gives as a suggestion. Is /36 even a legal CIDR for ipv4? (I wouldn't have thought so - you'd be masking out more than the ipv4 address allows, I think...)
(In reply to Nicholas van Oudtshoorn from comment #29) > Hmmm - I don't see SelfCheckAllowByIPRanges preferencing suggesting one use > /36. The library I've used should support something like /24, which is what > the preference gives as a suggestion. Is /36 even a legal CIDR for ipv4? (I > wouldn't have thought so - you'd be masking out more than the ipv4 address > allows, I think...) Sorry, my mistake... - Set SelfCheckAllowByIPRanges to 192.168.1.1 192.168.1.32/24 Allowed for clients with such IP's OK Client with other IP redirected to OPAC main page OK
Created attachment 56064 [details] [review] Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input Tested with scenarios above, works OK Signed-off-by: Marc <veron@veron.ch>
Created attachment 56065 [details] [review] Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input Tested two patches together, with scenario above. Works OK Signed-off-by: Marc <veron@veron.ch>
Comment on attachment 55746 [details] [review] Latest version of patch, with errors fixed Sorry for the noise - made mistakes with the patches while signing off. Reset...
Created attachment 56066 [details] [review] Bug 14407 - Allow restricting SCO to IP or IP range Fix presence of tabs in files, and printslip refering to $query instead of $input Tested with scenarios above, works as expected. Signed-off-by: Marc <veron@veron.ch>
(In reply to Marc Véron from comment #17) > Additionally, I think that the new function in C4/Auth.pm would need some > testing in t/db_dependent/Auth.t Nicholas, Please add some unit tests. Changing status to reflect need for change.
Created attachment 68834 [details] [review] Bug 14407 - Allow restricting SCO to IP or IP range Added some unit tests to this. Not added in the db_dependent directory, given that the new function (in_ipset) doesn't actually touch the database at all. It's meant to be a function that can be reused in the future for other ip based limits in Koha.
Added unit tests for in_ipset
Created attachment 68835 [details] [review] Allow restricting SCO to IP or IP range Sorry for the noise - running on two git instances, and forgot to include the required updates to Auth.pm that testing revealed. Now fails secure if invalid IP ranges are set...
Created attachment 68836 [details] Allow restricting SCO to IP or IP range Ah no! One more bit of noise - a very minor copy-and-paste mistake... in the copyright statement in the new unit test Auth.t! Hopefully this is now it!
Created attachment 70475 [details] [review] Bug 14407: Allow restricting SCO to IP or IP range Converted this to actual applicable patches. I think the test plan is comment #28. -- Mark Tompsett
Created attachment 70476 [details] [review] Bug 14407: Unit tests for in_ipset TEST PLAN --------- 1) apply all pathches 2) sudo koha-shell -c bash kohadev prove t/Auth.t -- should run. 3) run koha qa test tools
Comment on attachment 68836 [details] Allow restricting SCO to IP or IP range Patch attached in a non-patch way making things harder to git bz.
Thanks for fixing that - my bad :-(
Comment on attachment 70476 [details] [review] Bug 14407: Unit tests for in_ipset Review of attachment 70476 [details] [review]: ----------------------------------------------------------------- ::: t/Auth.t @@ +18,5 @@ > +use Modern::Perl; > +use Test::More tests => 10; > + > +use C4::Auth qw / in_ipset /; > +use warnings; Modern::Perl should cover the use warnings situation.
Created attachment 73316 [details] [review] Bug 14407: Follow up add missing atomic update The patch only added the system preference for fresh installs.
Created attachment 73318 [details] [review] Bug 14407: Follow up to add test case and clean noise Comment #28 has a /36 which is invalid CIDR. This triggers a crash and noise. This cleans up the crash and noise, and adds test cases to check for them. prove t/Auth.t -- before missing null case, and /36 case. -- after null case, and /36 with/without warnings.
kohadevbox is triggered from 192.168.50.1, so testing 192.168.0.0/16 worked nicely. :)
(In reply to Marc Véron from comment #28) > - Set SelfCheckAllowByIPRanges to 192.168.1.1 192.168.1.0/36 > as described for syspref SelfCheckAllowByIPRanges > Internal Server Error > Plack error log says; > 36, as in '192.168.1.30/36', does not make sense at > /home/marc/koha/C4/Auth.pm line 2058. > > Is it a bug or a wrong description for SelfCheckAllowByIPRanges? 36 is not valid. 8+8+8+8 = 32. Why would you have a number bigger than 32 on an IPv4 address?
Created attachment 73339 [details] [review] Bug 14407: Allow restricting SCO to IP or IP range Converted this to actual applicable patches. I think the test plan is comment #28. -- Mark Tompsett Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 73340 [details] [review] Bug 14407: Unit tests for in_ipset TEST PLAN --------- 1) apply all pathches 2) sudo koha-shell -c bash kohadev prove t/Auth.t -- should run. 3) run koha qa test tools Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 73341 [details] [review] Bug 14407: Follow up add missing atomic update The patch only added the system preference for fresh installs. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 73342 [details] [review] Bug 14407: Follow up to add test case and clean noise Comment #28 has a /36 which is invalid CIDR. This triggers a crash and noise. This cleans up the crash and noise, and adds test cases to check for them. prove t/Auth.t -- before missing null case, and /36 case. -- after null case, and /36 with/without warnings. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
The question is does the in_ipset return the correct value for bad ranges? Because currently, it assumes open when a bad range is provided.
Please, rebase on master.
(In reply to Michal Denar from comment #54) > Please, rebase on master. Please remember to set to "Patch Does Not Apply" when you request a rebase. :)
Created attachment 80735 [details] [review] Bug 14407: Allow restricting SCO to IP or IP range Converted this to actual applicable patches. I think the test plan is comment #28. -- Mark Tompsett Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80736 [details] [review] Bug 14407: Unit tests for in_ipset TEST PLAN --------- 1) apply all pathches 2) sudo koha-shell -c bash kohadev prove t/Auth.t -- should run. 3) run koha qa test tools Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80737 [details] [review] Bug 14407: Follow up add missing atomic update The patch only added the system preference for fresh installs. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 80738 [details] [review] Bug 14407: Follow up to add test case and clean noise Comment #28 has a /36 which is invalid CIDR. This triggers a crash and noise. This cleans up the crash and noise, and adds test cases to check for them. prove t/Auth.t -- before missing null case, and /36 case. -- after null case, and /36 with/without warnings. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 86123 [details] [review] Bug 14407: Follow up to add test case and clean noise Comment #28 has a /36 which is invalid CIDR. This triggers a crash and noise. This cleans up the crash and noise, and adds test cases to check for them. prove t/Auth.t -- before missing null case, and /36 case. -- after null case, and /36 with/without warnings. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Looked signed off but status was Needs Signoff so I tested it again.
QA: Looking here
Created attachment 88863 [details] [review] Bug 14407: Allow restricting SCO to IP or IP range Converted this to actual applicable patches. I think the test plan is comment #28. -- Mark Tompsett Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 88864 [details] [review] Bug 14407: Unit tests for in_ipset TEST PLAN --------- 1) apply all pathches 2) sudo koha-shell -c bash kohadev prove t/Auth.t -- should run. 3) run koha qa test tools Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 88865 [details] [review] Bug 14407: Follow up add missing atomic update The patch only added the system preference for fresh installs. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 88866 [details] [review] Bug 14407: Follow up to add test case and clean noise Comment #28 has a /36 which is invalid CIDR. This triggers a crash and noise. This cleans up the crash and noise, and adds test cases to check for them. prove t/Auth.t -- before missing null case, and /36 case. -- after null case, and /36 with/without warnings. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
@RM: Do we still have a keyword for packaging? dependency? Net::CIDR is added, but it seems to be installed already with koha-common and friends. The pref allows both start-finish and CIDR IP ranges. Although formally piping the pref into range2cidr converts start-finish ranges into CIDR. Both work. The patch uses CGI header(status=> ...) notation. The codebase also contains the older -status notation. But CGI::Util handles both.
I think dependency works, I will also email Mirko just for confirmation I added a see also to bug 21477 - this is just another case of a thing we already do so no blocker, just mentioning Once Mirko confirms I think this one is good
libnet-server-perl depends on libnet-cidr-perl so we already have it. But it should be declarded as dependency for koha-common for this bug.
(Please declare the dependency in PerlDependencies, I'll do the rest.)
Created attachment 89114 [details] [review] Bug 14407: (RM follow-up) Add dependency for Net::CIDR
Can I just get a thumbs up on adding the dep before pushing, I tried to get the version number in Jessie and use that for the minimum
Dependency is already added in the first patch... though it specifies 0.15 as minumum whereas you're specifies 0.19. I think I'd go with 0.17 (as the most recently listed for Jessie here: https://packages.debian.org/search?searchon=names&keywords=libnet-cidr-perl). Otherwise.. I think we're good to go.
Awesome work all! Pushed to master for 19.05
New feature will not be backported to 18.11.x series.
This is cool, but it's unfortunate the name "ipset" was used, since that's an actual tool that can be used for managing IP sets in the Linux kernel: http://ipset.netfilter.org/ipset.man.html When I saw the function name "in_ipset", I was confused until I saw that we weren't actually using ipset and it was just coincidental naming. Not a criticism. Just sharing my initial confusion with the naming.
We've not released yet.. perhaps we could change it to in_iprange quickly before it's set in stone forever more. What do you think David, does that sound more appropriate?
(In reply to Martin Renvoize from comment #76) > We've not released yet.. perhaps we could change it to in_iprange quickly > before it's set in stone forever more. > > What do you think David, does that sound more appropriate? If folk are willing, I'd say yes to changing it. How about "in_ip_ranges"? If we look at the current "sub in_ipset", we see the following line: my @allowedipranges = split(' ', $ipset); Probably makes sense to use "ranges" or "range_list" since that's the terminology used by Net::CIDR as well: https://metacpan.org/pod/Net::CIDR#@cidr_list=Net::CIDR::range2cidr(@range_list); (Actually, if I really wanted to split hairs, I'd say "@allowedipranges" should probably be changed to "@ip_ranges", as theoretically this function could also be used to interrogate a list of IP ranges to block. Yay code re-use!)
Actually, looking at "sub in_ipset" again, I'd also invert the logic, so that the default return value is 0. That way, if you've misconfigured the value of $ipset, you're not going to accidentally return 1. Also, using $ENV{'REMOTE_ADDR'} won't work properly if you're using a reverse proxy. I don't know how we handle this with Plack, but for CGI users running behind reverse proxies, there's no way this is going to work correctly.