Summary: | ILS-DI AuthorizedIPs should deny explicitly except those listed | ||
---|---|---|---|
Product: | Koha | Reporter: | Evelyn Hartline <evelyn> |
Component: | Staff interface | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | Pushed to main --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | andrew, dcook, didier.gautheron, fridolin.somers, gmcharlt, jake.deery, jesse, jonathan.druart, laurent.ducos, lisette, lucas, m.de.rooy, magnus, martin.renvoize, nick, paul.derscheid, philippe.blouin, tomascohen, wainuiwitikapark, wizzyrea |
Version: | Main | Keywords: | additional_work_needed |
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This patch updates the ILS-DI authorized IPs preference to deny all IPs not listed in the preference.
Previously if no text was entered the ILS-DI service was accessible by all IPs, now it requires explicitly defining the IPs that can access the service.
Upgrading libraries using ILS-DI should check that they have the necessary IPs defined in the system preference.
|
Version(s) released in: |
25.05.00,24.11.04,24.05.09,23.11.14,22.11.26
|
Circulation function: | |||
Attachments: |
Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs
Bug 36867: Update preference description and value for libraries using ILS-DI Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs Bug 36867: Update preference description and value for libraries using ILS-DI Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs Bug 36867: Update preference description and value for libraries using ILS-DI Bug 36867: Update preference description Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs Bug 36867: Update preference description Bug 36867: (follow-up) Don't check IP if ILS-DI disabled |
Description
Evelyn Hartline
2024-05-15 14:39:10 UTC
I think this is a really important bug, thank you for raising it! If ILS-DI wants explicitly opening up to the world, one should have to explicitly enter e.g. 0.0.0.0/0 or ::/0 Created attachment 166786 [details] [review] Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs This patch forces a check of authorized IPs against the Koha system preference, if nothing is present, no access is allowed To test: 1 - Enable ILS-DI, leave ILS-DI:AuthorizedIPs blank 2 - In another window: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=GetAvailability&id=1+2+99999&id_type=item 3 - You get a response 4 - Apply patch and restart all 5 - Reload ilsdi page, 'Not authorized' 6 - Copy the IP listed as refused and paste into ILS-DI:AuthorizedIPs 7 - Reload the page, you get a response again 8 - Change ILS-DI:AuthorizedIPs to: 0:0:0:0/0 9 - You still have access 10 - Paste your specific IP again, and add a second made up IP (comma separated) 11 - Reload and confirm you still have access Created attachment 166787 [details] [review] Bug 36867: Update preference description and value for libraries using ILS-DI This adds an atomic update to update any libraries with ILS-DI enabled to continue allowing access It also lists the new syntax for allowing all IPs in the preference Thanks so much for patching this quickly, Nick! Who can do a Signoff? Created attachment 166818 [details] [review] Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs This patch forces a check of authorized IPs against the Koha system preference, if nothing is present, no access is allowed To test: 1 - Enable ILS-DI, leave ILS-DI:AuthorizedIPs blank 2 - In another window: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=GetAvailability&id=1+2+99999&id_type=item 3 - You get a response 4 - Apply patch and restart all 5 - Reload ilsdi page, 'Not authorized' 6 - Copy the IP listed as refused and paste into ILS-DI:AuthorizedIPs 7 - Reload the page, you get a response again 8 - Change ILS-DI:AuthorizedIPs to: 0:0:0:0/0 9 - You still have access 10 - Paste your specific IP again, and add a second made up IP (comma separated) 11 - Reload and confirm you still have access Created attachment 166819 [details] [review] Bug 36867: Update preference description and value for libraries using ILS-DI This adds an atomic update to update any libraries with ILS-DI enabled to continue allowing access It also lists the new syntax for allowing all IPs in the preference Comment on attachment 166786 [details] [review] Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs This is obsoleted by a newer version, as fas as I can tell. Created attachment 170526 [details] [review] Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs This patch forces a check of authorized IPs against the Koha system preference, if nothing is present, no access is allowed To test: 1 - Enable ILS-DI, leave ILS-DI:AuthorizedIPs blank 2 - In another window: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=GetAvailability&id=1+2+99999&id_type=item 3 - You get a response 4 - Apply patch and restart all 5 - Reload ilsdi page, 'Not authorized' 6 - Copy the IP listed as refused and paste into ILS-DI:AuthorizedIPs 7 - Reload the page, you get a response again 8 - Change ILS-DI:AuthorizedIPs to: 0:0:0:0/0 9 - You still have access 10 - Paste your specific IP again, and add a second made up IP (comma separated) 11 - Reload and confirm you still have access Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 170527 [details] [review] Bug 36867: Update preference description and value for libraries using ILS-DI This adds an atomic update to update any libraries with ILS-DI enabled to continue allowing access It also lists the new syntax for allowing all IPs in the preference Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Hold on, Is 0:0:0:0/0 correct? Shouldn't it be '0.0.0.0/0,::/0'? Jake (In reply to Jake Deery from comment #10) > Hold on, > > Is 0:0:0:0/0 correct? Shouldn't it be '0.0.0.0/0,::/0'? > > Jake I would also argue that we shouldn't default to all IPs allowed, since it would never be the right thing to do in production (even if it is traditional behaviour). (In reply to David Cook from comment #11) > I would also argue that we shouldn't default to all IPs allowed, since it > would never be the right thing to do in production (even if it is > traditional behaviour). Agreed. Leaving ILS-DI completely open by default is not a good thing. Locking it down as the default is worth some breakage, imho. We at Biblibre do not use the preference ILS-DI AuthorizedIPs, we add allowed/denied IP in Nginx/Apache configuration. So you need a way to allow any IP in this preference. This is also wanted for Koha demo. I think the default should be 'empty box' meaning all access to ILS-DI is denied. The user should then have the option to either specify IPs manually in a comma-separated list (aka current behaviour), or enter '0.0.0.0/0' or '::/0' to allow all IP4 and/or IP6 addresses. Entering 0.0.0.0/0 or ::/0 to allow all IPs is an active decision being made by the user, and something that, whilst being bad practice, is it for us to introduce something some might see as a regression? This should satisfy all use-cases. For Biblibre, they can enter '0.0.0.0/0' and continue to use Apache configs to manage access. For everyone else, we get better security practices. Jake. How can this move forward? It seems the consensus here is that this issue is sufficiently important to justify possibly breaking ILS-DI on update for installations that haven't explicitly allowed any/all IPs. Created attachment 179905 [details] [review] Bug 36867: Update preference description This lists the new syntax for allowing all IPs in the preference I removed the atomic update, so now the default for existing installs will move to denying all unless a library has limited. We will want to highlight this in the release Created attachment 179908 [details] [review] Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs This patch forces a check of authorized IPs against the Koha system preference, if nothing is present, no access is allowed To test: 1 - Enable ILS-DI, leave ILS-DI:AuthorizedIPs blank 2 - In another window: http://localhost:8080/cgi-bin/koha/ilsdi.pl?service=GetAvailability&id=1+2+99999&id_type=item 3 - You get a response 4 - Apply patch and restart all 5 - Reload ilsdi page, 'Not authorized' 6 - Copy the IP listed as refused and paste into ILS-DI:AuthorizedIPs 7 - Reload the page, you get a response again 8 - Change ILS-DI:AuthorizedIPs to: 0:0:0:0/0 9 - You still have access 10 - Paste your specific IP again, and add a second made up IP (comma separated) 11 - Reload and confirm you still have access Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 179909 [details] [review] Bug 36867: Update preference description This lists the new syntax for allowing all IPs in the preference Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Testing this on 24.05.x-security: For testing steps 8 & 9: 8 - Change ILS-DI:AuthorizedIPs to: 0:0:0:0/0 9 - You still have access I don't have access when I do this. (In reply to Jake Deery from comment #10) > Hold on, > > Is 0:0:0:0/0 correct? Shouldn't it be '0.0.0.0/0,::/0'? > > Jake (In reply to Wainui Witika-Park from comment #20) > Testing this on 24.05.x-security: > > For testing steps 8 & 9: > > 8 - Change ILS-DI:AuthorizedIPs to: > 0:0:0:0/0 > 9 - You still have access > > I don't have access when I do this. Sorry, Wainui. That's a typo in the commit message. As Jake mentioned, it should be 0.0.0.0/0 or ::/0. (In reply to David Cook from comment #21) > (In reply to Jake Deery from comment #10) > > Hold on, > > > > Is 0:0:0:0/0 correct? Shouldn't it be '0.0.0.0/0,::/0'? > > > > Jake > > (In reply to Wainui Witika-Park from comment #20) > > Testing this on 24.05.x-security: > > > > For testing steps 8 & 9: > > > > 8 - Change ILS-DI:AuthorizedIPs to: > > 0:0:0:0/0 > > 9 - You still have access > > > > I don't have access when I do this. > > Sorry, Wainui. That's a typo in the commit message. As Jake mentioned, it > should be 0.0.0.0/0 or ::/0. Thank you! Sorry I did not see Jake's comment oops Applied to 24.05.x-security for 24.05.09 Applied to 23.11.x-security for 23.11.14 Applied to 24.11.x-security for 24.11.04 Applied to 22.11.x-security for 22.11.26 Pushed for 25.05! Well done everyone, thank you! t/db_dependent/selenium/ilsdi.t .. # Failed test 'ILS-DI is disabled' # at t/db_dependent/selenium/ilsdi.t line 57. # got: '' # expected: 'ILS-DI is disabled.' # Looks like you failed 1 test of 2. t/db_dependent/selenium/ilsdi.t .. 1/5 # Failed test 'Disabled ILS-DI and enabled OPAC' # at t/db_dependent/selenium/ilsdi.t line 61. # Failed test 'ILS-DI is disabled' # at t/db_dependent/selenium/ilsdi.t line 74. # got: '' # expected: 'ILS-DI is disabled.' # Looks like you failed 1 test of 2. t/db_dependent/selenium/ilsdi.t .. 2/5 # Failed test 'Disabled ILS-DI and disabled OPAC' # at t/db_dependent/selenium/ilsdi.t line 78. Created attachment 181678 [details] [review] Bug 36867: (follow-up) Don't check IP if ILS-DI disabled When ILS-DI is disabled we still expect to return the correct service name/section in the xml, but just indicate that it is disabled. We should skip the check for authorized IPs in this case, as the IP only matters when service is enabled |