Bug 24739

Summary: Buster ships with Net::Netmask 1.9104 which supports IPv6
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Test SuiteAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: 1joynelson, dcook, jonathan.druart, kyle, m.de.rooy
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05
Attachments: Bug 24739: Skip tests when IPv6 support is found
Bug 24739: Skip tests when IPv6 support is found
Bug 24739: Perform IPv6 tests only if Net::Netmask is correct version
Bug 24739: Skip tests when IPv6 support is found
Bug 24739: Perform IPv6 tests only if Net::Netmask is correct version
Bug 24739: Skip tests when IPv6 support is found
Bug 24739: Perform IPv6 tests only if Net::Netmask is correct version
Bug 24739: (QA follow-up) Get rid of Middlware

Description Martin Renvoize 2020-02-27 09:15:39 UTC
The recently introduced tests for Koha::Middleware::RealIP assume we are running a version of Net::Netmask that does not yet support IPv6, but beggining with Buster such support has been added.
Comment 1 Martin Renvoize 2020-02-27 09:16:52 UTC
Created attachment 99690 [details] [review]
Bug 24739: Skip tests when IPv6 support is found
Comment 2 Martin Renvoize 2020-02-27 09:18:09 UTC
I think my patch is wrong.. What I'm not sure about is whether the Koha side would handle IPv6 addresses correctly yet and as such whether the failure is flagging more work Koha side rather than a need to skip the test.
Comment 3 David Cook 2020-02-27 23:23:02 UTC
My bad. I should have looked more closely at that. 

I suppose the IPv6 handling would be useful if the reverse proxy is trusted, because then it doesn't pass that IP address on to the rest of Koha, but yeah if the REMOTE_ADDR was an IPv6 address I'm not sure. Let's see...

Looks like REMOTE_ADDR gets directly requested by:
- C4/Context.pm
#Used by Koha::Middleware::RealIP, so no drama
- C4/InstallAuth.pm
#Actually this is commented out, so should be removed anyway
- C4/Auth.pm
#Used by SessionRestrictionByIP which is just string equality matching, so 
should be OK
#Saved/retrieved from session, so should be OK
- Koha/Middleware/RealIP.pm
#Parsed with Net::Netmask, so no drama
- opac/ilsdi.pl
#Currently string equality matching, so should be OK
#In the future with https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24537, hopefully will use Net::Netmask too, so should be OK
- opac/opac-detail.pl
#Uses regex for OpacSuppressionByIPRange, which is essentially string matching, so nothing should bust. 
- opac/opac-search.pl
#Uses regex for OpacSuppressionByIPRange, which is essentially string matching, so nothing should bust. 
- opac/opac-restrictedpage.pl
#Uses regex for RestrictedPageLocalIPs, which is essentially string matching, so nothing should bust
Comment 4 David Cook 2020-02-27 23:26:51 UTC
Ah crumbs... just realized I made a typo in the path for the test... t/Koha/Middlware/RealIP.t should be t/Koha/Middleware/RealIP.t...
Comment 5 David Cook 2020-02-27 23:45:45 UTC
(In reply to Martin Renvoize from comment #2)
> I think my patch is wrong.. What I'm not sure about is whether the Koha side
> would handle IPv6 addresses correctly yet and as such whether the failure is
> flagging more work Koha side rather than a need to skip the test.

It seems to me that Koha could handle an IPv6 address in REMOTE_ADDR, so it would be more optimal to test for IPv6 success with Net::Netmask I think.

But your patch works overall. 

Works on koha/koha-testing:master-buster:
1..14
ok 1 - use Koha::Middleware::RealIP;
ok 2 - There is no X-Forwarded-For header, so just use the remote address
ok 3 - Don't trust 1.1.1.1 as a proxy, so use it as the remote address
ok 4 - Trust proxy (1.1.1.1), so use the X-Forwarded-For header for the remote address
ok 5 - Trust multiple proxies (1.1.1.1 and 3.3.3.3), so use the X-Forwaded-For <client> portion for the remote address
ok 6 - Warn on misconfigured koha_trusted_proxies
ok 7 - koha_trusted_proxies is misconfigured so ignore the X-Forwarded-For header
ok 8 - Warn on partially misconfigured koha_trusted_proxies
ok 9 - koha_trusted_proxies contains an invalid value but still includes one correct value, which is relevant, so use X-Forwarded-For header
ok 10 - Trust proxy (1.1.1.1) using CIDR notation, so use the X-Forwarded-For header for the remote address
ok 11 - Trust proxy (1.1.1.1) using abbreviated notation, so use the X-Forwarded-For header for the remote address
ok 12 - Trust proxy (1.1.1.1) using an IP address and netmask separated by a colon, so use the X-Forwarded-For header for the remote address
ok 13 # skip Net::Netmask at 1.9104+ supports IPv6
ok 14 # skip Net::Netmask at 1.9104+ supports IPv6

Works on koha/koha-testing:master
1..14
ok 1 - use Koha::Middleware::RealIP;
ok 2 - There is no X-Forwarded-For header, so just use the remote address
ok 3 - Don't trust 1.1.1.1 as a proxy, so use it as the remote address
ok 4 - Trust proxy (1.1.1.1), so use the X-Forwarded-For header for the remote address
ok 5 - Trust multiple proxies (1.1.1.1 and 3.3.3.3), so use the X-Forwaded-For <client> portion for the remote address
ok 6 - Warn on misconfigured koha_trusted_proxies
ok 7 - koha_trusted_proxies is misconfigured so ignore the X-Forwarded-For header
ok 8 - Warn on partially misconfigured koha_trusted_proxies
ok 9 - koha_trusted_proxies contains an invalid value but still includes one correct value, which is relevant, so use X-Forwarded-For header
ok 10 - Trust proxy (1.1.1.1) using CIDR notation, so use the X-Forwarded-For header for the remote address
ok 11 - Trust proxy (1.1.1.1) using abbreviated notation, so use the X-Forwarded-For header for the remote address
ok 12 - Trust proxy (1.1.1.1) using an IP address and netmask separated by a colon, so use the X-Forwarded-For header for the remote address
ok 13 - Warn on IPv6 koha_trusted_proxies
ok 14 - IPv6 support was added in 1.9104 version of Net::Netmask
Comment 6 David Cook 2020-02-28 00:21:13 UTC
Created attachment 99729 [details] [review]
Bug 24739: Skip tests when IPv6 support is found

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 7 David Cook 2020-02-28 00:21:16 UTC
Created attachment 99730 [details] [review]
Bug 24739: Perform IPv6 tests only if Net::Netmask is correct version

Test plan:
0. Apply patch
1. Set up Koha on stretch (e.g. koha/koha-testing:master)
2. perl t/Koha/Middlware/RealIP.t
3. Observe the following:
Subtest: IPv6 support
    ok 1 - Warn on IPv6 koha_trusted_proxies
    ok 2 - Unable to parse IPv6 address for trusted proxy, so ignore the X-Forwarded-For header
    1..2
ok 13 - IPv6 support

4. Set up Koha on buster (e.g. koha/koha-testing:master-buster)
5. perl t/Koha/Middlware/RealIP.t
6. Observe the following:
Subtest: IPv6 support
    ok 1 - Trust proxy (2001:db8:1234:5678:abcd:1234:abcd:1234) using IPv6 CIDR notation, so use the X-Forwarded-For header for the remote address
    1..1
ok 13 - IPv6 support
Comment 8 David Cook 2020-02-28 00:26:39 UTC
Ok I signed off your patch (forgot to update the patch title).

I also enhanced your patch with a patch of my own, so that it tests IPv6 support if Net::Netmask is a version that supports IPv6 and states that it can't handle IPv6 if it's on a lower version.
Comment 9 Martin Renvoize 2020-02-28 06:58:34 UTC
That's awesome diligent work there David, thanks so much for checking it out and adding the enhanced test on top I was seeing of but unsure where to go with.

I'll work through testing it today for a quick route to QA.

Teamwork is awesome, it's great to have you back involved 😀.
Comment 10 Martin Renvoize 2020-02-28 08:48:56 UTC
Created attachment 99748 [details] [review]
Bug 24739: Skip tests when IPv6 support is found

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-02-28 08:48:59 UTC
Created attachment 99749 [details] [review]
Bug 24739: Perform IPv6 tests only if Net::Netmask is correct version

Test plan:
0. Apply patch
1. Set up Koha on stretch (e.g. koha/koha-testing:master)
2. perl t/Koha/Middlware/RealIP.t
3. Observe the following:
Subtest: IPv6 support
    ok 1 - Warn on IPv6 koha_trusted_proxies
    ok 2 - Unable to parse IPv6 address for trusted proxy, so ignore the X-Forwarded-For header
    1..2
ok 13 - IPv6 support

4. Set up Koha on buster (e.g. koha/koha-testing:master-buster)
5. perl t/Koha/Middlware/RealIP.t
6. Observe the following:
Subtest: IPv6 support
    ok 1 - Trust proxy (2001:db8:1234:5678:abcd:1234:abcd:1234) using IPv6 CIDR notation, so use the X-Forwarded-For header for the remote address
    1..1
ok 13 - IPv6 support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-02-28 08:49:39 UTC
Excellent work, thanks David.. all checks out to me.. Signing off on your followup :)
Comment 13 Marcel de Rooy 2020-02-28 08:55:56 UTC
QAing
Comment 14 Marcel de Rooy 2020-02-28 09:35:27 UTC
Tested a Buster container too. Looks good.

=== D10
ok 1 - use Koha::Middleware::RealIP;
ok 2 - There is no X-Forwarded-For header, so just use the remote address
ok 3 - Don't trust 1.1.1.1 as a proxy, so use it as the remote address
ok 4 - Trust proxy (1.1.1.1), so use the X-Forwarded-For header for the remote address
ok 5 - Trust multiple proxies (1.1.1.1 and 3.3.3.3), so use the X-Forwaded-For <client> portion for the remote address
ok 6 - Warn on misconfigured koha_trusted_proxies
ok 7 - koha_trusted_proxies is misconfigured so ignore the X-Forwarded-For header
ok 8 - Warn on partially misconfigured koha_trusted_proxies
ok 9 - koha_trusted_proxies contains an invalid value but still includes one correct value, which is relevant, so use X-Forwarded-For header
ok 10 - Trust proxy (1.1.1.1) using CIDR notation, so use the X-Forwarded-For header for the remote address
ok 11 - Trust proxy (1.1.1.1) using abbreviated notation, so use the X-Forwarded-For header for the remote address
ok 12 - Trust proxy (1.1.1.1) using an IP address and netmask separated by a colon, so use the X-Forwarded-For header for the remote address
# Subtest: IPv6 support
    ok 1 - Trust proxy (2001:db8:1234:5678:abcd:1234:abcd:1234) using IPv6 CIDR notation, so use the X-Forwarded-For header for the remote address
    1..1
ok 13 - IPv6 support

=== D9
ok 1 - use Koha::Middleware::RealIP;
ok 2 - There is no X-Forwarded-For header, so just use the remote address
ok 3 - Don't trust 1.1.1.1 as a proxy, so use it as the remote address
ok 4 - Trust proxy (1.1.1.1), so use the X-Forwarded-For header for the remote address
ok 5 - Trust multiple proxies (1.1.1.1 and 3.3.3.3), so use the X-Forwaded-For <client> portion for the remote address
ok 6 - Warn on misconfigured koha_trusted_proxies
ok 7 - koha_trusted_proxies is misconfigured so ignore the X-Forwarded-For header
ok 8 - Warn on partially misconfigured koha_trusted_proxies
ok 9 - koha_trusted_proxies contains an invalid value but still includes one correct value, which is relevant, so use X-Forwarded-For header
ok 10 - Trust proxy (1.1.1.1) using CIDR notation, so use the X-Forwarded-For header for the remote address
ok 11 - Trust proxy (1.1.1.1) using abbreviated notation, so use the X-Forwarded-For header for the remote address
ok 12 - Trust proxy (1.1.1.1) using an IP address and netmask separated by a colon, so use the X-Forwarded-For header for the remote address
# Subtest: IPv6 support
    ok 1 - Warn on IPv6 koha_trusted_proxies
    ok 2 - Unable to parse IPv6 address for trusted proxy, so ignore the X-Forwarded-For header
    1..2
ok 13 - IPv6 support
Comment 15 Marcel de Rooy 2020-02-28 09:37:15 UTC
Created attachment 99750 [details] [review]
Bug 24739: Skip tests when IPv6 support is found

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2020-02-28 09:37:20 UTC
Created attachment 99751 [details] [review]
Bug 24739: Perform IPv6 tests only if Net::Netmask is correct version

Test plan:
0. Apply patch
1. Set up Koha on stretch (e.g. koha/koha-testing:master)
2. perl t/Koha/Middlware/RealIP.t
3. Observe the following:
Subtest: IPv6 support
    ok 1 - Warn on IPv6 koha_trusted_proxies
    ok 2 - Unable to parse IPv6 address for trusted proxy, so ignore the X-Forwarded-For header
    1..2
ok 13 - IPv6 support

4. Set up Koha on buster (e.g. koha/koha-testing:master-buster)
5. perl t/Koha/Middlware/RealIP.t
6. Observe the following:
Subtest: IPv6 support
    ok 1 - Trust proxy (2001:db8:1234:5678:abcd:1234:abcd:1234) using IPv6 CIDR notation, so use the X-Forwarded-For header for the remote address
    1..1
ok 13 - IPv6 support

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2020-02-28 09:37:24 UTC
Created attachment 99752 [details] [review]
Bug 24739: (QA follow-up) Get rid of Middlware

I like Middleware better ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2020-02-28 09:37:54 UTC
(In reply to David Cook from comment #4)
> Ah crumbs... just realized I made a typo in the path for the test...
> t/Koha/Middlware/RealIP.t should be t/Koha/Middleware/RealIP.t...

Follow-up
Comment 19 Marcel de Rooy 2020-02-28 11:44:07 UTC
*** Bug 24753 has been marked as a duplicate of this bug. ***
Comment 20 Martin Renvoize 2020-02-28 15:07:11 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 David Cook 2020-03-02 02:53:36 UTC
(In reply to Martin Renvoize from comment #9)
> That's awesome diligent work there David, thanks so much for checking it out
> and adding the enhanced test on top I was seeing of but unsure where to go
> with.
> 
> I'll work through testing it today for a quick route to QA.
> 
> Teamwork is awesome, it's great to have you back involved 😀.

Teamwork is totally awesome! Aww thanks. When you put it that way, I guess I better keep up my efforts heh.
Comment 22 Marcel de Rooy 2020-03-02 08:45:36 UTC
(In reply to Marcel de Rooy from comment #19)
> *** Bug 24753 has been marked as a duplicate of this bug. ***

No, bug 24753 has been pushed. Follow-up removed. The QAed version of the patches on this report is out of date.
Comment 23 Joy Nelson 2020-04-01 20:33:07 UTC
backported to 19.11.x branch for 19.11.05