Bug 36867 - ILS-DI AuthorizedIPs should deny explicitly except those listed
Summary: ILS-DI AuthorizedIPs should deny explicitly except those listed
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-15 14:39 UTC by Evelyn Hartline
Modified: 2025-04-28 16:54 UTC (History)
20 users (show)

See Also:
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 (1.90 KB, patch)
2024-05-15 16:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36867: Update preference description and value for libraries using ILS-DI (1.56 KB, patch)
2024-05-15 16:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs (1.90 KB, patch)
2024-05-16 12:03 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36867: Update preference description and value for libraries using ILS-DI (2.71 KB, patch)
2024-05-16 12:03 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs (1.97 KB, patch)
2024-08-21 07:39 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36867: Update preference description and value for libraries using ILS-DI (2.77 KB, patch)
2024-08-21 07:39 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36867: Update preference description (1.43 KB, patch)
2025-03-31 07:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 36867: Do not allow access to ILS-DI for IPs not explicitly listed in ILS-DI:AuthorizedIPs (1.87 KB, patch)
2025-03-31 08:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36867: Update preference description (1.50 KB, patch)
2025-03-31 08:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Evelyn Hartline 2024-05-15 14:39:10 UTC
Currently, ILS-DI: AuthorizedIPs falls back to an allow-all, unless an IP/range is put into the field. This can easily be misconfigured by staff, leading to potential patron data leakage. 

Instead, the behaviour should be to deny all by default when nothing is entered into the field.
Comment 1 Jake Deery 2024-05-15 14:42:18 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
Comment 2 Nick Clemens (kidclamp) 2024-05-15 16:32:19 UTC
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
Comment 3 Nick Clemens (kidclamp) 2024-05-15 16:32:22 UTC
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
Comment 4 Liz Rea 2024-05-15 18:37:10 UTC
Thanks so much for patching this quickly, Nick! Who can do a Signoff?
Comment 5 Nick Clemens (kidclamp) 2024-05-16 12:03:29 UTC
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
Comment 6 Nick Clemens (kidclamp) 2024-05-16 12:03:32 UTC
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 7 Magnus Enger 2024-05-23 07:30:01 UTC
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.
Comment 8 Martin Renvoize (ashimema) 2024-08-21 07:39:20 UTC
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>
Comment 9 Martin Renvoize (ashimema) 2024-08-21 07:39:23 UTC
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>
Comment 10 Jake Deery 2024-08-21 14:59:14 UTC
Hold on,

Is 0:0:0:0/0 correct? Shouldn't it be '0.0.0.0/0,::/0'?

Jake
Comment 11 David Cook 2024-08-22 02:11:54 UTC
(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).
Comment 12 Magnus Enger 2024-08-22 06:25:25 UTC
(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.
Comment 13 Fridolin Somers 2024-09-20 07:43:45 UTC
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.
Comment 14 Jake Deery 2024-10-01 08:41:33 UTC
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.
Comment 15 Andrew Fuerste-Henry 2025-03-28 16:03:55 UTC
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.
Comment 16 Nick Clemens (kidclamp) 2025-03-31 07:42:34 UTC
Created attachment 179905 [details] [review]
Bug 36867: Update preference description

This lists the new syntax for allowing all IPs in the preference
Comment 17 Nick Clemens (kidclamp) 2025-03-31 07:49:09 UTC
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
Comment 18 Martin Renvoize (ashimema) 2025-03-31 08:11:42 UTC
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>
Comment 19 Martin Renvoize (ashimema) 2025-03-31 08:11:47 UTC
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>
Comment 20 Wainui Witika-Park 2025-04-23 01:17:08 UTC
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.
Comment 21 David Cook 2025-04-23 02:28:51 UTC
(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.
Comment 22 Wainui Witika-Park 2025-04-23 23:48:25 UTC
(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
Comment 23 Wainui Witika-Park 2025-04-23 23:48:45 UTC
Applied to 24.05.x-security for 24.05.09
Comment 24 Fridolin Somers 2025-04-24 12:17:46 UTC
Applied to 23.11.x-security for 23.11.14
Comment 25 Paul Derscheid 2025-04-24 19:38:30 UTC
Applied to 24.11.x-security for 24.11.04
Comment 26 Jesse Maseto 2025-04-24 19:47:23 UTC
Applied to 22.11.x-security for 22.11.26
Comment 27 Katrin Fischer 2025-04-28 16:54:38 UTC
Pushed for 25.05!

Well done everyone, thank you!