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.
Created attachment 99690 [details] [review] Bug 24739: Skip tests when IPv6 support is found
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.
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
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...
(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
Created attachment 99729 [details] [review] Bug 24739: Skip tests when IPv6 support is found Signed-off-by: David Cook <dcook@prosentient.com.au>
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
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.
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 [U+1F600].
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>
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>
Excellent work, thanks David.. all checks out to me.. Signing off on your followup :)
QAing
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
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>
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>
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>
(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
*** Bug 24753 has been marked as a duplicate of this bug. ***
Nice work everyone! Pushed to master for 20.05
(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 [U+1F600]. Teamwork is totally awesome! Aww thanks. When you put it that way, I guess I better keep up my efforts heh.
(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.
backported to 19.11.x branch for 19.11.05